Fix mode 2031 reporting light when using a non-conditional theme despite a dark GTK color scheme

[UnsaltedScholar]

Fixes #13604

First reported in #13448

Edit (mostly copied from my disclaimer edit in my other PR):
I truly apologize for not including this earlier, I was a tad overeager after my vouch request was approved and I was quite rushed and ended up creating this pull request on my phone during lunch and had several errands afterwards.

AI Usage Disclaimer:
GPT 5.6 Sol was used to understand the project structure. I asked it to tell me where the mode 2031 theme switch occurred and it pointed me toward the function that I originally edited. Just to be sure I asked it to review my changes and it reminded me to change the test that was affected.

I apologize again for forgetting to add this, it won't happen again.

[tristan957]

@jcollie I'll let you merge this after your review since you were involved already.

[jcollie]

This "fixes" the issue, but I don't think that it's the correct fix. Mode 2031 color reports basically "cheated" and used the Ghostty config theme to cache the last known color scheme to avoid sending multiple DSRs to TUIs when the scheme hadn't changed. This change fixes the problem that DSRs were not sent when the system theme changes by always making the theme part of the conditional set, but that will force extraneous Ghostty config changes when the system theme changes but the user configured Ghostty for a single theme for both light and dark system themes.

I think that the proper fix is for the core app to maintain a cache of the current system theme and send out mode 2031 DSRs separately from reloading the config.

ghostty/src/App.zig

Lines 390 to 413 in 48d85ea

	pub fn colorSchemeEvent(
	self: *App,
	rt_app: *apprt.App,
	scheme: apprt.ColorScheme,
	) !void {
	const new_scheme: configpkg.ConditionalState.Theme = switch (scheme) {
	.light => .light,
	.dark => .dark,
	};
	// If our scheme didn't change, then we don't do anything.
	if (self.config_conditional_state.theme == new_scheme) return;
	// Setup our conditional state which has the current color theme.
	self.config_conditional_state.theme = new_scheme;
	// Request our configuration be reloaded because the new scheme may
	// impact the colors of the app.
	_ = try rt_app.performAction(
	.app,
	.reload_config,
	.{ .soft = true },
	);
	}

[UnsaltedScholar]

@jcollie

Is it preferable to keep the mode 2031 color-scheme cache entirely at the app level, as you suggested, or would it be more beneficial to retain a per-surface cache so surfaces with different effective window-theme settings can report their own schemes correctly (assuming this is something Ghostty wants to allow)?

As a corollary to this, should I remove conditional_state from Termio.DerivedConfig, since its current use appears limited to mode 2031 reporting, or should it remain, even with the duplicated conditional state, for any potential Termio features in the future?

Something like this (this compiles/fixes the issue on linux, have no way to test MacOS currently):

Edit: This particular blurb is an example that was partially generated by AI (specifically the report_changed bool block and the added comments). This code is not intended as the final version (which will be written by me); it is solely meant to be a quick example implementation to confirm if I was thinking in the right direction.

diff --git a/src/App.zig b/src/App.zig
--- a/src/App.zig
+++ b/src/App.zig
@@ -8,6 +8,12 @@ font_grid_set: font.SharedGridSet,
 last_notification_time: ?std.Io.Timestamp = null,
 last_notification_digest: u64 = 0,

+/// The current color scheme reported by the application runtime. This is
+/// intentionally separate from the conditional configuration state because
+/// mode 2031 must track the runtime scheme even when no conditional
+/// configuration depends on it.
+color_scheme: apprt.ColorScheme,
+
 /// The conditional state of the configuration. See the equivalent field
 /// in the Surface struct for more information. In this case, this applies
 /// to the app-level config and as a default for new surfaces.
@@ -37,6 +43,7 @@ pub fn init(
         .surfaces = .empty,
         .mailbox = .{},
         .font_grid_set = font_grid_set,
+        .color_scheme = .light,
         .config_conditional_state = .{},
     };
 }
@@ -61,12 +68,16 @@ pub fn colorSchemeEvent(
     rt_app: *apprt.App,
     scheme: apprt.ColorScheme,
 ) !void {
+    // Cache the runtime color scheme independently of the configuration.
+    // New surfaces use this as their initial mode 2031 report value.
+    self.color_scheme = scheme;
+
     const new_scheme: configpkg.ConditionalState.Theme = switch (scheme) {
         .light => .light,
         .dark => .dark,
     };

-    // If our scheme didn't change, then we don't do anything.
+    // Only reload configuration when its conditional state changed.
     if (self.config_conditional_state.theme == new_scheme) return;

     // Setup our conditional state which has the current color theme.
     
diff --git a/src/Surface.zig b/src/Surface.zig
--- a/src/Surface.zig
+++ b/src/Surface.zig
@@ -2,6 +2,7 @@
             .size = size,
             .full_config = config,
             .config = try termio.Termio.DerivedConfig.init(alloc, config),
+            .color_scheme = app.color_scheme,
             .backend = .{ .exec = io_exec },
             .mailbox = io_mailbox,
             .renderer_state = &self.renderer_state,
@@ -28,20 +29,37 @@ pub fn colorSchemeCallback(self: *Surface, scheme: apprt.ColorScheme) !void {
     crash.sentry.thread_state = self.crashThreadState();
     defer crash.sentry.thread_state = null;

+    // Update the runtime state used by mode 2031 independently of the
+    // conditional configuration. This state is protected by the renderer
+    // mutex because the IO thread reads it while encoding reports.
+    const report_changed: bool = report_changed: {
+        self.renderer_state.mutex.lockUncancelable(global.io());
+        defer self.renderer_state.mutex.unlock(global.io());
+
+        if (self.io.color_scheme == scheme) break :report_changed false;
+        self.io.color_scheme = scheme;
+        break :report_changed true;
+    };
+
+    // Report runtime scheme changes without requiring a config reload.
+    if (report_changed) {
+        self.queueIo(
+            .{ .color_scheme_report = .{ .force = false } },
+            .unlocked,
+        );
+    }
+
     const new_scheme: configpkg.ConditionalState.Theme = switch (scheme) {
         .light => .light,
         .dark => .dark,
     };

-    // If our scheme didn't change, then we don't do anything.
+    // Only reload configuration when its conditional state changed.
     if (self.config_conditional_state.theme == new_scheme) return;

     // Setup our conditional state which has the current color theme.
     self.config_conditional_state.theme = new_scheme;
     self.notifyConfigConditionalState();
-
-    // If mode 2031 is on, then we report the change live.
-    self.queueIo(.{ .color_scheme_report = .{ .force = false } }, .unlocked);
 }

 pub fn posToViewport(self: Surface, xpos: f64, ypos: f64) terminal.point.Coordinate {

diff --git a/src/termio/Options.zig b/src/termio/Options.zig
--- a/src/termio/Options.zig
+++ b/src/termio/Options.zig
@@ -6,6 +6,9 @@ full_config: *const Config,
 /// The derived configuration for this termio implementation.
 config: termio.Termio.DerivedConfig,

+/// The current runtime color scheme used for mode 2031 reports.
+color_scheme: apprt.ColorScheme,
+
 /// The backend for termio that implements where reads/writes are sourced.
 backend: termio.Backend,

diff --git a/src/termio/Termio.zig b/src/termio/Termio.zig
--- a/src/termio/Termio.zig
+++ b/src/termio/Termio.zig
@@ -4,6 +4,10 @@ backend: termio.Backend,
 /// The derived configuration for this termio implementation.
 config: DerivedConfig,

+/// The current runtime color scheme used for mode 2031 reports. This is
+/// protected by the renderer state mutex.
+color_scheme: apprt.ColorScheme,
+
 /// The terminal emulator internal state. This is the abstract "terminal"
 /// that manages input, grid updating, etc. and is renderer-agnostic. It
 /// just stores internal state about a grid.
@@ -32,7 +36,6 @@ pub const DerivedConfig = struct {
     osc_color_report_format: configpkg.Config.OSCColorReportFormat,
     clipboard_write: configpkg.ClipboardAccess,
     enquiry_response: []const u8,
-    conditional_state: configpkg.ConditionalState,

     pub fn init(
         alloc_gpa: Allocator,
@@ -53,7 +56,6 @@ pub const DerivedConfig = struct {
             .osc_color_report_format = config.@"osc-color-report-format",
             .clipboard_write = config.@"clipboard-write",
             .enquiry_response = try alloc.dupe(u8, config.@"enquiry-response"),
-            .conditional_state = config._conditional_state,

             // This has to be last so that we copy AFTER the arena allocations
             // above happen (Zig assigns in order).
@@ -77,6 +79,7 @@ pub const DerivedConfig = struct {
         .alloc = alloc,
         .terminal = term,
         .config = opts.config,
+        .color_scheme = opts.color_scheme,
         .renderer_state = opts.renderer_state,
         .renderer_wakeup = opts.renderer_wakeup,
         .renderer_mailbox = opts.renderer_mailbox,
@@ -110,7 +113,7 @@ pub fn colorSchemeReportLocked(self: *Termio, td: *ThreadData, force: bool) !voi
     if (!force and !self.renderer_state.terminal.modes.get(.report_color_scheme)) {
         return;
     }
-    const scheme: terminalpkg.device_status.ColorScheme = switch (self.config.conditional_state.theme) {
+    const scheme: terminalpkg.device_status.ColorScheme = switch (self.color_scheme) {
         .light => .light,
         .dark => .dark,
     };

[jcollie]

@jcollie

Is it preferable to keep the mode 2031 color-scheme cache entirely at the app level, as you suggested, or would it be more beneficial to retain a per-surface cache so surfaces with different effective window-theme settings can report their own schemes correctly (assuming this is something Ghostty wants to allow)?

This would be a cache of the system color scheme so app-level caching is more appropriate since the surfaces all belong to the same system.

As a corollary to this, should I remove conditional_state from Termio.DerivedConfig, since its current use appears limited to mode 2031 reporting, or should it remain, even with the duplicated conditional state, for any potential Termio features in the future?

If it's not used to anything else, it should be removed.

Something like this (this compiles/fixes the issue on linux, have no way to test MacOS currently):

Don't have the time to look at the diff in detail now, will take a look when the PR is updated, but that seems like the right approach.

[UnsaltedScholar]

This would be a cache of the system color scheme so app-level caching is more appropriate since the surfaces all belong to the same system.

If it's not used to anything else, it should be removed.

The rework I just pushed should be correct in this regard now (and is much more minimal than the "example" I provided above). If there are any further problems, please let me know.

The only notable thing is that it defaults to dark mode, I think this is reasonable given most people use dark terminals, but if this is a problem I can change it.

[jcollie]

The basics are looking good but I don't think that we're sending reports now.

[UnsaltedScholar]

Sorry for the delay. I made the changes you requested, however, when I was manually testing it I noticed some related problems that I fixed in the last two commits.

For one, reloading the config from the right-click context menu after changing the window-theme setting didn't update everything correctly. The second issue I noticed when fixing the first was that reloading caused a duplicate toast/notification to appear when the color scheme actually needed to change (it didn't duplicate when switching from system to dark if the system theme was already dark).

If you guys would rather those last two things be addressed in a different PR I can do that as well, but they seemed relevant enough to what I was fixing here so I added them.

Minutia:
A third issue I noticed was that window-theme = auto doesn't really behave exactly as described, I think. I have background set to pure black and foreground to pure white, but the window theme still changes when I change KDE theme (it basically acts like a copy of the system option ignoring the other Ghostty config options). I didn't fix that as I thought it was beyond the scope of this PR.

The AI usage disclaimer from before applies, though I did also have it review the current changes to help make sure I didn't mess anything up in other parts of Ghostty.

[jcollie]

A third issue I noticed was that window-theme = auto doesn't really behave exactly as described, I think.

Yeah I think that the description ofwindow-theme=auto isn't very good. But I think that it's better to start a discussion to come to a consensus before starting on a PR.

[jcollie]

This looks good enough for now. I've got some ideas for follow-ups later but let's get this in so we can get feedback.

[gotenksIN]

spent my morning finding out why pi was loading light colour scheme despite window-theme being set to dark, rebuilt with this PR and its fixed now :D