From 1c6adf4065ccc019aa803dbf53c732097cacff49 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 22 Nov 2024 13:10:32 -0800 Subject: [PATCH 01/23] mode 2031 should send updates on any color palette change Related #2755 From the mode 2031 spec[1]: > Send CSI ? 2031 h to the terminal to enable unsolicited DSR (device status > report) messages for color palette updates and CSI ? 2031 l respectively to > disable it again. > > The sent out DSR looks equivalent to the already above mentioned. This > notification is not just sent when dark/light mode has been changed by the > operating system / desktop, but also if the user explicitly changed color > scheme, e.g. by configuration. My reading of this paired with the original discussion is that this is meant to be sent out for anything that could possibly change terminal colors. Previous to this commit, we only sent out the DSR when the actual system light/dark mode changed. This commit changes it to send out the DSR on any operation that _may_ change the terminal colors. [1]: https://contour-terminal.org/vt-extensions/color-palette-update-notifications/#example-source-code --- src/Surface.zig | 58 +++++++++++++++++++++-------------- src/apprt/surface.zig | 6 ++-- src/termio/stream_handler.zig | 8 ++++- 3 files changed, 46 insertions(+), 26 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index ee2fe1ac5..ae38b8ba2 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -820,21 +820,28 @@ pub fn handleMessage(self: *Surface, msg: Message) !void { }, .unlocked); }, - .color_change => |change| try self.rt_app.performAction( - .{ .surface = self }, - .color_change, - .{ - .kind = switch (change.kind) { - .background => .background, - .foreground => .foreground, - .cursor => .cursor, - .palette => |v| @enumFromInt(v), + .color_change => |change| { + // On any color change, we have to report for mode 2031 + // if it is enabled. + self.reportColorScheme(false); + + // Notify our apprt + try self.rt_app.performAction( + .{ .surface = self }, + .color_change, + .{ + .kind = switch (change.kind) { + .background => .background, + .foreground => .foreground, + .cursor => .cursor, + .palette => |v| @enumFromInt(v), + }, + .r = change.color.r, + .g = change.color.g, + .b = change.color.b, }, - .r = change.color.r, - .g = change.color.g, - .b = change.color.b, - }, - ), + ); + }, .set_mouse_shape => |shape| { log.debug("changing mouse shape: {}", .{shape}); @@ -898,7 +905,7 @@ pub fn handleMessage(self: *Surface, msg: Message) !void { .renderer_health => |health| self.updateRendererHealth(health), - .report_color_scheme => try self.reportColorScheme(), + .report_color_scheme => |force| self.reportColorScheme(force), .present_surface => try self.presentSurface(), @@ -935,8 +942,18 @@ fn passwordInput(self: *Surface, v: bool) !void { try self.queueRender(); } -/// Sends a DSR response for the current color scheme to the pty. -fn reportColorScheme(self: *Surface) !void { +/// Sends a DSR response for the current color scheme to the pty. If +/// force is false then we only send the response if the terminal mode +/// 2031 is enabled. +fn reportColorScheme(self: *Surface, force: bool) void { + if (!force) { + self.renderer_state.mutex.lock(); + defer self.renderer_state.mutex.unlock(); + if (!self.renderer_state.terminal.modes.get(.report_color_scheme)) { + return; + } + } + const output = switch (self.config_conditional_state.theme) { .light => "\x1B[?997;2n", .dark => "\x1B[?997;1n", @@ -3643,12 +3660,7 @@ pub fn colorSchemeCallback(self: *Surface, scheme: apprt.ColorScheme) !void { self.notifyConfigConditionalState(); // If mode 2031 is on, then we report the change live. - const report = report: { - self.renderer_state.mutex.lock(); - defer self.renderer_state.mutex.unlock(); - break :report self.renderer_state.terminal.modes.get(.report_color_scheme); - }; - if (report) try self.reportColorScheme(); + self.reportColorScheme(false); } pub fn posToViewport(self: Surface, xpos: f64, ypos: f64) terminal.point.Coordinate { diff --git a/src/apprt/surface.zig b/src/apprt/surface.zig index 58faa9633..f3fd71432 100644 --- a/src/apprt/surface.zig +++ b/src/apprt/surface.zig @@ -58,8 +58,10 @@ pub const Message = union(enum) { /// Health status change for the renderer. renderer_health: renderer.Health, - /// Report the color scheme - report_color_scheme: void, + /// Report the color scheme. The bool parameter is whether to force or not. + /// If force is true, the color scheme should be reported even if mode + /// 2031 is not set. + report_color_scheme: bool, /// Tell the surface to present itself to the user. This may require raising /// a window and switching tabs. diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index 37d176de3..4e82b5a19 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -126,6 +126,9 @@ pub const StreamHandler = struct { if (self.default_cursor) self.setCursorStyle(.default) catch |err| { log.warn("failed to set default cursor style: {}", .{err}); }; + + // The config could have changed any of our colors so update mode 2031 + self.surfaceMessageWriter(.{ .report_color_scheme = false }); } inline fn surfaceMessageWriter( @@ -767,7 +770,7 @@ pub const StreamHandler = struct { self.messageWriter(msg); }, - .color_scheme => self.surfaceMessageWriter(.{ .report_color_scheme = {} }), + .color_scheme => self.surfaceMessageWriter(.{ .report_color_scheme = true }), } } @@ -892,6 +895,9 @@ pub const StreamHandler = struct { ) !void { self.terminal.fullReset(); try self.setMouseShape(.text); + + // Reset resets our palette so we report it for mode 2031. + self.surfaceMessageWriter(.{ .report_color_scheme = false }); } pub fn queryKittyKeyboard(self: *StreamHandler) !void { From 70cc2d9793e87567560b56d03f42dcab0f8583f9 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 23 Nov 2024 09:40:00 -0800 Subject: [PATCH 02/23] termio: copy input command to avoid memory corruption Fixes #2779 --- src/termio/shell_integration.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/termio/shell_integration.zig b/src/termio/shell_integration.zig index cd4d88dce..06f2abc67 100644 --- a/src/termio/shell_integration.zig +++ b/src/termio/shell_integration.zig @@ -78,7 +78,7 @@ pub fn setup( try setupXdgDataDirs(alloc_arena, resource_dir, env); break :shell .{ .shell = .elvish, - .command = command, + .command = try alloc_arena.dupe(u8, command), }; } @@ -86,7 +86,7 @@ pub fn setup( try setupXdgDataDirs(alloc_arena, resource_dir, env); break :shell .{ .shell = .fish, - .command = command, + .command = try alloc_arena.dupe(u8, command), }; } @@ -94,7 +94,7 @@ pub fn setup( try setupZsh(resource_dir, env); break :shell .{ .shell = .zsh, - .command = command, + .command = try alloc_arena.dupe(u8, command), }; } From 2b30186259c4e1ab2956d982a9b4b38ab687456d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 23 Nov 2024 09:52:49 -0800 Subject: [PATCH 03/23] surface needs to preserve original config working-directory Fixes #2785 --- src/Surface.zig | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Surface.zig b/src/Surface.zig index b2530936a..e6184baca 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -376,7 +376,13 @@ pub fn init( // We want a config pointer for everything so we get that either // based on our conditional state or the original config. - const config: *const configpkg.Config = if (config_) |*c| c else config_original; + const config: *const configpkg.Config = if (config_) |*c| config: { + // We want to preserve our original working directory. We + // don't need to dupe memory here because termio will derive + // it. We preserve this so directory inheritance works. + c.@"working-directory" = config_original.@"working-directory"; + break :config c; + } else config_original; // Get our configuration var derived_config = try DerivedConfig.init(alloc, config); From f7e1d0b83b9067c6dd3d482134b8305c6e39752c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 23 Nov 2024 10:21:38 -0800 Subject: [PATCH 04/23] apprt/embedded: update our cached config on config_change action Fixes #2784 --- src/apprt/embedded.zig | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index ccafb9aa6..6a4411a85 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -85,26 +85,38 @@ pub const App = struct { }; core_app: *CoreApp, - config: *const Config, opts: Options, keymap: input.Keymap, + /// The configuration for the app. This is owned by this structure. + config: Config, + /// The keymap state is used for global keybinds only. Each surface /// also has its own keymap state for focused keybinds. keymap_state: input.Keymap.State, - pub fn init(core_app: *CoreApp, config: *const Config, opts: Options) !App { + pub fn init( + core_app: *CoreApp, + config: *const Config, + opts: Options, + ) !App { + // We have to clone the config. + const alloc = core_app.alloc; + var config_clone = try config.clone(alloc); + errdefer config_clone.deinit(); + return .{ .core_app = core_app, - .config = config, + .config = config_clone, .opts = opts, .keymap = try input.Keymap.init(), .keymap_state = .{}, }; } - pub fn terminate(self: App) void { + pub fn terminate(self: *App) void { self.keymap.deinit(); + self.config.deinit(); } /// Returns true if there are any global keybinds in the configuration. @@ -370,11 +382,11 @@ pub const App = struct { } } - pub fn wakeup(self: App) void { + pub fn wakeup(self: *const App) void { self.opts.wakeup(self.opts.userdata); } - pub fn wait(self: App) !void { + pub fn wait(self: *const App) !void { _ = self; } @@ -450,6 +462,19 @@ pub const App = struct { }, }, + .config_change => switch (target) { + .surface => {}, + + // For app updates, we update our core config. We need to + // clone it because the caller owns the param. + .app => if (value.config.clone(self.core_app.alloc)) |config| { + self.config.deinit(); + self.config = config; + } else |err| { + log.err("error updating app config err={}", .{err}); + }, + }, + else => {}, } } @@ -573,7 +598,7 @@ pub const Surface = struct { errdefer app.core_app.deleteSurface(self); // Shallow copy the config so that we can modify it. - var config = try apprt.surface.newConfig(app.core_app, app.config); + var config = try apprt.surface.newConfig(app.core_app, &app.config); defer config.deinit(); // If we have a working directory from the options then we set it. @@ -1831,7 +1856,7 @@ pub const CAPI = struct { // This is only supported on macOS if (comptime builtin.target.os.tag != .macos) return; - const config = app.config; + const config = &app.config; // Do nothing if we don't have background transparency enabled if (config.@"background-opacity" >= 1.0) return; From e031eb17f4b2e9b9541f1f549cd6216be7216a67 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 23 Nov 2024 15:09:39 -0800 Subject: [PATCH 05/23] config: clone() needs to preserve conditionals of replay steps Fixes #2791 This goes back to the previous implementation of clone that directly cloned values rather than using replay steps, because replay steps don't preserve conditionals on the iterator. This works. Another fix will be when we support some sort of conditional syntax and the replay iterator can yield that information. We have a unit test here so we can verify that then. --- src/config/Config.zig | 128 +++++++++++++++++++++++++++++++++---- src/config/conditional.zig | 11 ++++ 2 files changed, 128 insertions(+), 11 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 66e9e9ff9..55cd55606 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2995,22 +2995,41 @@ pub fn cloneEmpty( /// Create a copy of this configuration. /// -/// This will not re-read referenced configuration files except for the -/// theme, but the config-file values will be preserved. +/// This will not re-read referenced configuration files and operates +/// purely in-memory. pub fn clone( self: *const Config, alloc_gpa: Allocator, -) !Config { - // Create a new config with a new arena - var new_config = try self.cloneEmpty(alloc_gpa); - errdefer new_config.deinit(); +) Allocator.Error!Config { + // Start with an empty config + var result = try self.cloneEmpty(alloc_gpa); + errdefer result.deinit(); + const alloc_arena = result._arena.?.allocator(); - // Replay all of our steps to rebuild the configuration - var it = Replay.iterator(self._replay_steps.items, &new_config); - try new_config.loadIter(alloc_gpa, &it); - try new_config.finalize(); + // Copy our values + inline for (@typeInfo(Config).Struct.fields) |field| { + if (!@hasField(Key, field.name)) continue; + @field(result, field.name) = try cloneValue( + alloc_arena, + field.type, + @field(self, field.name), + ); + } - return new_config; + // Preserve our replay steps. We copy them exactly to also preserve + // the exact conditionals required for some steps. + try result._replay_steps.ensureTotalCapacity( + alloc_arena, + self._replay_steps.items.len, + ); + for (self._replay_steps.items) |item| { + result._replay_steps.appendAssumeCapacity( + try item.clone(alloc_arena), + ); + } + assert(result._replay_steps.items.len == self._replay_steps.items.len); + + return result; } fn cloneValue( @@ -3204,6 +3223,24 @@ const Replay = struct { conditions: []const Conditional, arg: []const u8, }, + + fn clone( + self: Step, + alloc: Allocator, + ) Allocator.Error!Step { + return switch (self) { + .arg => |v| .{ .arg = try alloc.dupe(u8, v) }, + .expand => |v| .{ .expand = try alloc.dupe(u8, v) }, + .conditional_arg => |v| conditional: { + var conds = try alloc.alloc(Conditional, v.conditions.len); + for (v.conditions, 0..) |cond, i| conds[i] = try cond.clone(alloc); + break :conditional .{ .conditional_arg = .{ + .conditions = conds, + .arg = try alloc.dupe(u8, v.arg), + } }; + }, + }; + } }; const Iterator = struct { @@ -5235,6 +5272,75 @@ test "clone preserves conditional state" { try testing.expectEqual(.dark, dest._conditional_state.theme); } +test "clone can then change conditional state" { + // This tests a particular bug sequence where: + // 1. Load light + // 2. Convert to dark + // 3. Clone dark + // 4. Convert to light + // 5. Config is still dark (bug) + const testing = std.testing; + const alloc = testing.allocator; + var arena = ArenaAllocator.init(alloc); + defer arena.deinit(); + const alloc_arena = arena.allocator(); + + // Setup our test theme + var td = try internal_os.TempDir.init(); + defer td.deinit(); + { + var file = try td.dir.createFile("theme_light", .{}); + defer file.close(); + try file.writer().writeAll(@embedFile("testdata/theme_light")); + } + { + var file = try td.dir.createFile("theme_dark", .{}); + defer file.close(); + try file.writer().writeAll(@embedFile("testdata/theme_dark")); + } + var light_buf: [std.fs.max_path_bytes]u8 = undefined; + const light = try td.dir.realpath("theme_light", &light_buf); + var dark_buf: [std.fs.max_path_bytes]u8 = undefined; + const dark = try td.dir.realpath("theme_dark", &dark_buf); + + var cfg_light = try Config.default(alloc); + defer cfg_light.deinit(); + var it: TestIterator = .{ .data = &.{ + try std.fmt.allocPrint( + alloc_arena, + "--theme=light:{s},dark:{s}", + .{ light, dark }, + ), + } }; + try cfg_light.loadIter(alloc, &it); + try cfg_light.finalize(); + + var cfg_dark = try cfg_light.changeConditionalState(.{ .theme = .dark }); + defer cfg_dark.deinit(); + + try testing.expectEqual(Color{ + .r = 0xEE, + .g = 0xEE, + .b = 0xEE, + }, cfg_dark.background); + + var cfg_clone = try cfg_dark.clone(alloc); + defer cfg_clone.deinit(); + try testing.expectEqual(Color{ + .r = 0xEE, + .g = 0xEE, + .b = 0xEE, + }, cfg_clone.background); + + var cfg_light2 = try cfg_clone.changeConditionalState(.{ .theme = .light }); + defer cfg_light2.deinit(); + try testing.expectEqual(Color{ + .r = 0xFF, + .g = 0xFF, + .b = 0xFF, + }, cfg_light2.background); +} + test "changed" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/config/conditional.zig b/src/config/conditional.zig index 3be6b1fab..d46b33399 100644 --- a/src/config/conditional.zig +++ b/src/config/conditional.zig @@ -61,6 +61,17 @@ pub const Conditional = struct { value: []const u8, pub const Op = enum { eq, ne }; + + pub fn clone( + self: Conditional, + alloc: Allocator, + ) Allocator.Error!Conditional { + return .{ + .key = self.key, + .op = self.op, + .value = try alloc.dupe(u8, self.value), + }; + } }; test "conditional enum match" { From 4042041b61314d165187b2651b47b335a6e3badb Mon Sep 17 00:00:00 2001 From: Gregory Anders Date: Sat, 23 Nov 2024 17:40:54 -0600 Subject: [PATCH 06/23] termio: track whether fg/bg color is explicitly set Make the foreground_color and background_color fields in the Terminal struct optional values so that we can determine if a foreground or background color was explicitly set with an OSC 10 or OSC 11 sequence. This makes the logic a bit simpler to reason about (i.e. `foreground_color` is now always "the color set by an OSC 10 sequence" while `default_foreground_color` is always "the color set by the config file") and also fixes an issue where an OSC 10 or OSC 11 query would not report the correct color after a config update changed the foreground or background color. The `cursor_color` field was already optional, with the same semantics (it is only non-null when explicitly set with an OSC 12) so this brings all three of these fields into alignment. --- src/termio/Termio.zig | 6 ++-- src/termio/stream_handler.zig | 56 +++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/termio/Termio.zig b/src/termio/Termio.zig index 9ed3ffc94..e7b391419 100644 --- a/src/termio/Termio.zig +++ b/src/termio/Termio.zig @@ -200,9 +200,9 @@ pub fn init(self: *Termio, alloc: Allocator, opts: termio.Options) !void { .default_cursor_style = opts.config.cursor_style, .default_cursor_blink = opts.config.cursor_blink, .default_cursor_color = default_cursor_color, - .cursor_color = default_cursor_color, - .foreground_color = opts.config.foreground.toTerminalRGB(), - .background_color = opts.config.background.toTerminalRGB(), + .cursor_color = null, + .foreground_color = null, + .background_color = null, }; }; diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index 4e82b5a19..2fc9e92af 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -52,20 +52,20 @@ pub const StreamHandler = struct { default_cursor_blink: ?bool, default_cursor_color: ?terminal.color.RGB, - /// Actual cursor color. This can be changed with OSC 12. + /// Actual cursor color. This can be changed with OSC 12. If unset, falls + /// back to the default cursor color. cursor_color: ?terminal.color.RGB, /// The default foreground and background color are those set by the user's - /// config file. These can be overridden by terminal applications using OSC - /// 10 and OSC 11, respectively. + /// config file. default_foreground_color: terminal.color.RGB, default_background_color: terminal.color.RGB, - /// The actual foreground and background color. Normally this will be the - /// same as the default foreground and background color, unless changed by a - /// terminal application. - foreground_color: terminal.color.RGB, - background_color: terminal.color.RGB, + /// The foreground and background color as set by an OSC 10 or OSC 11 + /// sequence. If unset then the respective color falls back to the default + /// value. + foreground_color: ?terminal.color.RGB, + background_color: ?terminal.color.RGB, /// The response to use for ENQ requests. The memory is owned by /// whoever owns StreamHandler. @@ -1197,9 +1197,12 @@ pub const StreamHandler = struct { const color = switch (kind) { .palette => |i| self.terminal.color_palette.colors[i], - .foreground => self.foreground_color, - .background => self.background_color, - .cursor => self.cursor_color orelse self.foreground_color, + .foreground => self.foreground_color orelse self.default_foreground_color, + .background => self.background_color orelse self.default_background_color, + .cursor => self.cursor_color orelse + self.default_cursor_color orelse + self.foreground_color orelse + self.default_foreground_color, }; var msg: termio.Message = .{ .write_small = .{} }; @@ -1342,34 +1345,35 @@ pub const StreamHandler = struct { } }, .foreground => { - self.foreground_color = self.default_foreground_color; + self.foreground_color = null; _ = self.renderer_mailbox.push(.{ - .foreground_color = self.foreground_color, + .foreground_color = self.default_foreground_color, }, .{ .forever = {} }); self.surfaceMessageWriter(.{ .color_change = .{ .kind = .foreground, - .color = self.foreground_color, + .color = self.default_foreground_color, } }); }, .background => { - self.background_color = self.default_background_color; + self.background_color = null; _ = self.renderer_mailbox.push(.{ - .background_color = self.background_color, + .background_color = self.default_background_color, }, .{ .forever = {} }); self.surfaceMessageWriter(.{ .color_change = .{ .kind = .background, - .color = self.background_color, + .color = self.default_background_color, } }); }, .cursor => { - self.cursor_color = self.default_cursor_color; + self.cursor_color = null; + _ = self.renderer_mailbox.push(.{ - .cursor_color = self.cursor_color, + .cursor_color = self.default_cursor_color, }, .{ .forever = {} }); - if (self.cursor_color) |color| { + if (self.default_cursor_color) |color| { self.surfaceMessageWriter(.{ .color_change = .{ .kind = .cursor, .color = color, @@ -1422,9 +1426,9 @@ pub const StreamHandler = struct { const color: terminal.color.RGB = switch (key) { .palette => |palette| self.terminal.color_palette.colors[palette], .special => |special| switch (special) { - .foreground => self.foreground_color, - .background => self.background_color, - .cursor => self.cursor_color, + .foreground => self.foreground_color orelse self.default_foreground_color, + .background => self.background_color orelse self.default_background_color, + .cursor => self.cursor_color orelse self.default_cursor_color, else => { log.warn("ignoring unsupported kitty color protocol key: {}", .{key}); continue; @@ -1485,15 +1489,15 @@ pub const StreamHandler = struct { .special => |special| { const msg: renderer.Message = switch (special) { .foreground => msg: { - self.foreground_color = self.default_foreground_color; + self.foreground_color = null; break :msg .{ .foreground_color = self.default_foreground_color }; }, .background => msg: { - self.background_color = self.default_background_color; + self.background_color = null; break :msg .{ .background_color = self.default_background_color }; }, .cursor => msg: { - self.cursor_color = self.default_cursor_color; + self.cursor_color = null; break :msg .{ .cursor_color = self.default_cursor_color }; }, else => { From 2d3709f3545c8f2dbeafc51f67129da6481b4306 Mon Sep 17 00:00:00 2001 From: Anmol Wadhwani <4815989+anmolw@users.noreply.github.com> Date: Fri, 22 Nov 2024 20:31:40 +0530 Subject: [PATCH 07/23] Strip theme location in fish completions Co-authored-by: trag1c --- src/build/fish_completions.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/build/fish_completions.zig b/src/build/fish_completions.zig index 64fbea44e..2ac67bdad 100644 --- a/src/build/fish_completions.zig +++ b/src/build/fish_completions.zig @@ -53,7 +53,7 @@ fn writeFishCompletions(writer: anytype) !void { if (std.mem.startsWith(u8, field.name, "font-family")) try writer.writeAll(" -f -a \"(ghostty +list-fonts | grep '^[A-Z]')\"") else if (std.mem.eql(u8, "theme", field.name)) - try writer.writeAll(" -f -a \"(ghostty +list-themes)\"") + try writer.writeAll(" -f -a \"(ghostty +list-themes | sed -E 's/^(.*) \\(.*\\$/\\1/')\"") else if (std.mem.eql(u8, "working-directory", field.name)) try writer.writeAll(" -f -k -a \"(__fish_complete_directories)\"") else { From 10e37a3deefcb3e433f4dfa77336cafdd363edc2 Mon Sep 17 00:00:00 2001 From: Kyaw Date: Sun, 24 Nov 2024 17:08:07 +0700 Subject: [PATCH 08/23] config: support loading from "Application Support" directory on macOS --- src/apprt/glfw.zig | 2 +- src/config/Config.zig | 43 ++++++++++++++++++++++---------- src/os/macos.zig | 54 ++++++++++++++++++++++++++++++++++++++++ src/os/macos_version.zig | 21 ---------------- src/os/main.zig | 3 +-- 5 files changed, 86 insertions(+), 37 deletions(-) create mode 100644 src/os/macos.zig delete mode 100644 src/os/macos_version.zig diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index e793615d5..bf4c44ad0 100644 --- a/src/apprt/glfw.zig +++ b/src/apprt/glfw.zig @@ -724,7 +724,7 @@ pub const Surface = struct { /// Set the shape of the cursor. fn setMouseShape(self: *Surface, shape: terminal.MouseShape) !void { if ((comptime builtin.target.isDarwin()) and - !internal_os.macosVersionAtLeast(13, 0, 0)) + !internal_os.macos.isAtLeastVersion(13, 0, 0)) { // We only set our cursor if we're NOT on Mac, or if we are then the // macOS version is >= 13 (Ventura). On prior versions, glfw crashes diff --git a/src/config/Config.zig b/src/config/Config.zig index 55cd55606..e2843508a 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1809,10 +1809,15 @@ pub fn deinit(self: *Config) void { /// Load the configuration according to the default rules: /// /// 1. Defaults -/// 2. XDG Config File +/// 2. Configuration Files /// 3. CLI flags /// 4. Recursively defined configuration files /// +/// Configuration files are loaded in the follow order: +/// +/// 1. XDG Config File +/// 2. "Application Support" Config File on macOS +/// pub fn load(alloc_gpa: Allocator) !Config { var result = try default(alloc_gpa); errdefer result.deinit(); @@ -2394,25 +2399,37 @@ pub fn loadFile(self: *Config, alloc: Allocator, path: []const u8) !void { try self.expandPaths(std.fs.path.dirname(path).?); } -/// Load the configuration from the default configuration file. The default -/// configuration file is at `$XDG_CONFIG_HOME/ghostty/config`. -pub fn loadDefaultFiles(self: *Config, alloc: Allocator) !void { - const config_path = try internal_os.xdg.config(alloc, .{ .subdir = "ghostty/config" }); - defer alloc.free(config_path); - - self.loadFile(alloc, config_path) catch |err| switch (err) { +/// Load optional configuration file from `path`. All errors are ignored. +pub fn loadOptionalFile(self: *Config, alloc: Allocator, path: []const u8) void { + self.loadFile(alloc, path) catch |err| switch (err) { error.FileNotFound => std.log.info( - "homedir config not found, not loading path={s}", - .{config_path}, + "optional config file not found, not loading path={s}", + .{path}, ), - else => std.log.warn( - "error reading config file, not loading err={} path={s}", - .{ err, config_path }, + "error reading optional config file, not loading err={} path={s}", + .{ err, path }, ), }; } +/// Load configurations from the default configuration files. The default +/// configuration file is at `$XDG_CONFIG_HOME/ghostty/config`. +/// +/// On macOS, `$HOME/Library/Application Support/$CFBundleIdentifier/config` +/// is also loaded. +pub fn loadDefaultFiles(self: *Config, alloc: Allocator) !void { + const xdg_path = try internal_os.xdg.config(alloc, .{ .subdir = "ghostty/config" }); + defer alloc.free(xdg_path); + self.loadOptionalFile(alloc, xdg_path); + + if (builtin.os.tag == .macos) { + const app_support_path = try internal_os.macos.getAppSupportDir(alloc, "config"); + defer alloc.free(app_support_path); + self.loadOptionalFile(alloc, app_support_path); + } +} + /// Load and parse the CLI args. pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { switch (builtin.os.tag) { diff --git a/src/os/macos.zig b/src/os/macos.zig new file mode 100644 index 000000000..62ba87c5a --- /dev/null +++ b/src/os/macos.zig @@ -0,0 +1,54 @@ +const std = @import("std"); +const builtin = @import("builtin"); +const assert = std.debug.assert; +const objc = @import("objc"); +const Allocator = std.mem.Allocator; + +pub const NSOperatingSystemVersion = extern struct { + major: i64, + minor: i64, + patch: i64, +}; + +/// Verifies that the running macOS system version is at least the given version. +pub fn isAtLeastVersion(major: i64, minor: i64, patch: i64) bool { + assert(builtin.target.isDarwin()); + + const NSProcessInfo = objc.getClass("NSProcessInfo").?; + const info = NSProcessInfo.msgSend(objc.Object, objc.sel("processInfo"), .{}); + return info.msgSend(bool, objc.sel("isOperatingSystemAtLeastVersion:"), .{ + NSOperatingSystemVersion{ .major = major, .minor = minor, .patch = patch }, + }); +} + +pub const NSSearchPathDirectory = enum(c_ulong) { + NSApplicationSupportDirectory = 14, +}; + +pub const NSSearchPathDomainMask = enum(c_ulong) { + NSUserDomainMask = 1, +}; + +pub fn getAppSupportDir(alloc: Allocator, sub_path: []const u8) ![]u8 { + assert(builtin.target.isDarwin()); + + const err: ?*anyopaque = undefined; + const NSFileManager = objc.getClass("NSFileManager").?; + const manager = NSFileManager.msgSend(objc.Object, objc.sel("defaultManager"), .{}); + const url = manager.msgSend( + objc.Object, + objc.sel("URLForDirectory:inDomain:appropriateForURL:create:error:"), + .{ + NSSearchPathDirectory.NSApplicationSupportDirectory, + NSSearchPathDomainMask.NSUserDomainMask, + @as(?*anyopaque, null), + true, + &err, + }, + ); + const path = url.getProperty(objc.Object, "path"); + const c_str = path.getProperty([*:0]const u8, "UTF8String"); + const app_support_dir = std.mem.sliceTo(c_str, 0); + + return try std.fs.path.join(alloc, &.{ app_support_dir, "com.mitchellh.ghostty", sub_path }); +} diff --git a/src/os/macos_version.zig b/src/os/macos_version.zig deleted file mode 100644 index e0b21560e..000000000 --- a/src/os/macos_version.zig +++ /dev/null @@ -1,21 +0,0 @@ -const std = @import("std"); -const builtin = @import("builtin"); -const assert = std.debug.assert; -const objc = @import("objc"); - -/// Verifies that the running macOS system version is at least the given version. -pub fn macosVersionAtLeast(major: i64, minor: i64, patch: i64) bool { - assert(builtin.target.isDarwin()); - - const NSProcessInfo = objc.getClass("NSProcessInfo").?; - const info = NSProcessInfo.msgSend(objc.Object, objc.sel("processInfo"), .{}); - return info.msgSend(bool, objc.sel("isOperatingSystemAtLeastVersion:"), .{ - NSOperatingSystemVersion{ .major = major, .minor = minor, .patch = patch }, - }); -} - -pub const NSOperatingSystemVersion = extern struct { - major: i64, - minor: i64, - patch: i64, -}; diff --git a/src/os/main.zig b/src/os/main.zig index 22765f546..073129300 100644 --- a/src/os/main.zig +++ b/src/os/main.zig @@ -8,7 +8,6 @@ const file = @import("file.zig"); const flatpak = @import("flatpak.zig"); const homedir = @import("homedir.zig"); const locale = @import("locale.zig"); -const macos_version = @import("macos_version.zig"); const mouse = @import("mouse.zig"); const openpkg = @import("open.zig"); const pipepkg = @import("pipe.zig"); @@ -21,6 +20,7 @@ pub const hostname = @import("hostname.zig"); pub const passwd = @import("passwd.zig"); pub const xdg = @import("xdg.zig"); pub const windows = @import("windows.zig"); +pub const macos = @import("macos.zig"); // Functions and types pub const CFReleaseThread = @import("cf_release_thread.zig"); @@ -37,7 +37,6 @@ pub const freeTmpDir = file.freeTmpDir; pub const isFlatpak = flatpak.isFlatpak; pub const home = homedir.home; pub const ensureLocale = locale.ensureLocale; -pub const macosVersionAtLeast = macos_version.macosVersionAtLeast; pub const clickInterval = mouse.clickInterval; pub const open = openpkg.open; pub const pipe = pipepkg.pipe; From 6c615046ba8d164d99712808104120f968453e09 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 25 Nov 2024 13:26:21 -0800 Subject: [PATCH 09/23] config: only change conditional state if there are relevant changes Related to #2775 This makes it so that `changeConditionalState` only does something if the conditional state (1) has changes and (2) those changes are relevant to the current conditional state. By "relevant" we mean that the conditional state being changed is state that is actually used by the configuration. --- src/config/Config.zig | 133 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 124 insertions(+), 9 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 55cd55606..0f8be9662 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1793,6 +1793,10 @@ _diagnostics: cli.DiagnosticList = .{}, /// determine if a conditional configuration matches or not. _conditional_state: conditional.State = .{}, +/// The conditional keys that are used at any point during the configuration +/// loading. This is used to speed up the conditional evaluation process. +_conditional_set: std.EnumSet(conditional.Key) = .{}, + /// The steps we can use to reload the configuration after it has been loaded /// without reopening the files. This is used in very specific cases such /// as loadTheme which has more details on why. @@ -2610,6 +2614,10 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { /// on the new state. The caller must free the old configuration if they /// wish. /// +/// This returns null if the conditional state would result in no changes +/// to the configuration. In this case, the caller can continue to use +/// the existing configuration or clone if they want a copy. +/// /// This doesn't re-read any files, it just re-applies the same /// configuration with the new conditional state. Importantly, this means /// that if you change the conditional state and the user in the interim @@ -2618,7 +2626,30 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { pub fn changeConditionalState( self: *const Config, new: conditional.State, -) !Config { +) !?Config { + // If the conditional state between the old and new is the same, + // then we don't need to do anything. + relevant: { + inline for (@typeInfo(conditional.Key).Enum.fields) |field| { + const key: conditional.Key = @field(conditional.Key, field.name); + + // Conditional set contains the keys that this config uses. So we + // only continue if we use this key. + if (self._conditional_set.contains(key) and !equalField( + @TypeOf(@field(self._conditional_state, field.name)), + @field(self._conditional_state, field.name), + @field(new, field.name), + )) { + break :relevant; + } + } + + // If we got here, then we didn't find any differences between + // the old and new conditional state that would affect the + // configuration. + return null; + } + // Create our new configuration const alloc_gpa = self._arena.?.child_allocator; var new_config = try self.cloneEmpty(alloc_gpa); @@ -2765,6 +2796,9 @@ pub fn finalize(self: *Config) !void { // This setting doesn't make sense with different light/dark themes // because it'll force the theme based on the Ghostty theme. if (self.@"window-theme" == .auto) self.@"window-theme" = .system; + + // Mark that we use a conditional theme + self._conditional_set.insert(.theme); } } @@ -3029,6 +3063,9 @@ pub fn clone( } assert(result._replay_steps.items.len == self._replay_steps.items.len); + // Copy the conditional set + result._conditional_set = self._conditional_set; + return result; } @@ -5258,14 +5295,13 @@ test "clone preserves conditional state" { var a = try Config.default(alloc); defer a.deinit(); - var b = try a.changeConditionalState(.{ .theme = .dark }); - defer b.deinit(); - try testing.expectEqual(.dark, b._conditional_state.theme); - var dest = try b.clone(alloc); + a._conditional_state.theme = .dark; + try testing.expectEqual(.dark, a._conditional_state.theme); + var dest = try a.clone(alloc); defer dest.deinit(); // Should have no changes - var it = b.changeIterator(&dest); + var it = a.changeIterator(&dest); try testing.expectEqual(@as(?Key, null), it.next()); // Should have the same conditional state @@ -5315,7 +5351,7 @@ test "clone can then change conditional state" { try cfg_light.loadIter(alloc, &it); try cfg_light.finalize(); - var cfg_dark = try cfg_light.changeConditionalState(.{ .theme = .dark }); + var cfg_dark = (try cfg_light.changeConditionalState(.{ .theme = .dark })).?; defer cfg_dark.deinit(); try testing.expectEqual(Color{ @@ -5332,7 +5368,7 @@ test "clone can then change conditional state" { .b = 0xEE, }, cfg_clone.background); - var cfg_light2 = try cfg_clone.changeConditionalState(.{ .theme = .light }); + var cfg_light2 = (try cfg_clone.changeConditionalState(.{ .theme = .light })).?; defer cfg_light2.deinit(); try testing.expectEqual(Color{ .r = 0xFF, @@ -5341,6 +5377,25 @@ test "clone can then change conditional state" { }, cfg_light2.background); } +test "clone preserves conditional set" { + const testing = std.testing; + const alloc = testing.allocator; + + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + "--theme=light:foo,dark:bar", + "--window-theme=auto", + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + var clone1 = try cfg.clone(alloc); + defer clone1.deinit(); + + try testing.expect(clone1._conditional_set.contains(.theme)); +} + test "changed" { const testing = std.testing; const alloc = testing.allocator; @@ -5355,6 +5410,44 @@ test "changed" { try testing.expect(!source.changed(&dest, .@"font-size")); } +test "changeConditionalState ignores irrelevant changes" { + const testing = std.testing; + const alloc = testing.allocator; + + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + "--theme=foo", + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expect(try cfg.changeConditionalState( + .{ .theme = .dark }, + ) == null); + } +} + +test "changeConditionalState applies relevant changes" { + const testing = std.testing; + const alloc = testing.allocator; + + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + "--theme=light:foo,dark:bar", + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + var cfg2 = (try cfg.changeConditionalState(.{ .theme = .dark })).?; + defer cfg2.deinit(); + + try testing.expect(cfg2._conditional_set.contains(.theme)); + } +} test "theme loading" { const testing = std.testing; const alloc = testing.allocator; @@ -5386,6 +5479,9 @@ test "theme loading" { .g = 0x3A, .b = 0xBC, }, cfg.background); + + // Not a conditional theme + try testing.expect(!cfg._conditional_set.contains(.theme)); } test "theme loading preserves conditional state" { @@ -5534,7 +5630,7 @@ test "theme loading correct light/dark" { try cfg.loadIter(alloc, &it); try cfg.finalize(); - var new = try cfg.changeConditionalState(.{ .theme = .dark }); + var new = (try cfg.changeConditionalState(.{ .theme = .dark })).?; defer new.deinit(); try testing.expectEqual(Color{ .r = 0xEE, @@ -5561,3 +5657,22 @@ test "theme specifying light/dark changes window-theme from auto" { try testing.expect(cfg.@"window-theme" == .system); } } + +test "theme specifying light/dark sets theme usage in conditional state" { + const testing = std.testing; + const alloc = testing.allocator; + + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + "--theme=light:foo,dark:bar", + "--window-theme=auto", + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expect(cfg.@"window-theme" == .system); + try testing.expect(cfg._conditional_set.contains(.theme)); + } +} From a39aa7e89daf9eb734be631520b7d1e1c1b1f06e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 25 Nov 2024 15:12:14 -0800 Subject: [PATCH 10/23] apprt/gtk: only show config reload toast if app config changes This resolves the toast showing up every time the surface config changes which can be relatively frequent under certain circumstances such as theme changes. --- src/apprt/gtk/App.zig | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 0cee1938e..bb5309333 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -462,7 +462,7 @@ pub fn performAction( .equalize_splits => self.equalizeSplits(target), .goto_split => self.gotoSplit(target, value), .open_config => try configpkg.edit.open(self.core_app.alloc), - .config_change => self.configChange(value.config), + .config_change => self.configChange(target, value.config), .reload_config => try self.reloadConfig(target, value), .inspector => self.controlInspector(target, value), .desktop_notification => self.showDesktopNotification(target, value), @@ -818,18 +818,32 @@ fn showDesktopNotification( c.g_application_send_notification(g_app, n.body.ptr, notification); } -fn configChange(self: *App, new_config: *const Config) void { +fn configChange( + self: *App, + target: apprt.Target, + new_config: *const Config, +) void { _ = new_config; - self.syncConfigChanges() catch |err| { - log.warn("error handling configuration changes err={}", .{err}); - }; + switch (target) { + // We don't do anything for surface config change events. There + // is nothing to sync with regards to a surface today. + .surface => {}, - if (adwaita.enabled(&self.config)) { - if (self.core_app.focusedSurface()) |core_surface| { - const surface = core_surface.rt_surface; - if (surface.container.window()) |window| window.onConfigReloaded(); - } + .app => { + self.syncConfigChanges() catch |err| { + log.warn("error handling configuration changes err={}", .{err}); + }; + + // App changes needs to show a toast that our configuration + // has reloaded. + if (adwaita.enabled(&self.config)) { + if (self.core_app.focusedSurface()) |core_surface| { + const surface = core_surface.rt_surface; + if (surface.container.window()) |window| window.onConfigReloaded(); + } + } + }, } } From adc59be9776373d128ee599d76af03b5c77300fd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 25 Nov 2024 16:04:16 -0800 Subject: [PATCH 11/23] os: more error handling on reading the app support dir --- src/config/Config.zig | 16 ++++------- src/os/macos.zig | 64 +++++++++++++++++++++++++++++-------------- 2 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index e2843508a..d88347e2e 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1809,14 +1809,10 @@ pub fn deinit(self: *Config) void { /// Load the configuration according to the default rules: /// /// 1. Defaults -/// 2. Configuration Files -/// 3. CLI flags -/// 4. Recursively defined configuration files -/// -/// Configuration files are loaded in the follow order: -/// -/// 1. XDG Config File -/// 2. "Application Support" Config File on macOS +/// 2. XDG config dir +/// 3. "Application Support" directory (macOS only) +/// 4. CLI flags +/// 5. Recursively defined configuration files /// pub fn load(alloc_gpa: Allocator) !Config { var result = try default(alloc_gpa); @@ -2423,8 +2419,8 @@ pub fn loadDefaultFiles(self: *Config, alloc: Allocator) !void { defer alloc.free(xdg_path); self.loadOptionalFile(alloc, xdg_path); - if (builtin.os.tag == .macos) { - const app_support_path = try internal_os.macos.getAppSupportDir(alloc, "config"); + if (comptime builtin.os.tag == .macos) { + const app_support_path = try internal_os.macos.appSupportDir(alloc, "config"); defer alloc.free(app_support_path); self.loadOptionalFile(alloc, app_support_path); } diff --git a/src/os/macos.zig b/src/os/macos.zig index 62ba87c5a..fe312c6e1 100644 --- a/src/os/macos.zig +++ b/src/os/macos.zig @@ -4,15 +4,9 @@ const assert = std.debug.assert; const objc = @import("objc"); const Allocator = std.mem.Allocator; -pub const NSOperatingSystemVersion = extern struct { - major: i64, - minor: i64, - patch: i64, -}; - /// Verifies that the running macOS system version is at least the given version. pub fn isAtLeastVersion(major: i64, minor: i64, patch: i64) bool { - assert(builtin.target.isDarwin()); + comptime assert(builtin.target.isDarwin()); const NSProcessInfo = objc.getClass("NSProcessInfo").?; const info = NSProcessInfo.msgSend(objc.Object, objc.sel("processInfo"), .{}); @@ -21,20 +15,24 @@ pub fn isAtLeastVersion(major: i64, minor: i64, patch: i64) bool { }); } -pub const NSSearchPathDirectory = enum(c_ulong) { - NSApplicationSupportDirectory = 14, -}; +pub const AppSupportDirError = Allocator.Error || error{AppleAPIFailed}; -pub const NSSearchPathDomainMask = enum(c_ulong) { - NSUserDomainMask = 1, -}; +/// Return the path to the application support directory for Ghostty +/// with the given sub path joined. This allocates the result using the +/// given allocator. +pub fn appSupportDir( + alloc: Allocator, + sub_path: []const u8, +) AppSupportDirError![]u8 { + comptime assert(builtin.target.isDarwin()); -pub fn getAppSupportDir(alloc: Allocator, sub_path: []const u8) ![]u8 { - assert(builtin.target.isDarwin()); - - const err: ?*anyopaque = undefined; const NSFileManager = objc.getClass("NSFileManager").?; - const manager = NSFileManager.msgSend(objc.Object, objc.sel("defaultManager"), .{}); + const manager = NSFileManager.msgSend( + objc.Object, + objc.sel("defaultManager"), + .{}, + ); + const url = manager.msgSend( objc.Object, objc.sel("URLForDirectory:inDomain:appropriateForURL:create:error:"), @@ -43,12 +41,36 @@ pub fn getAppSupportDir(alloc: Allocator, sub_path: []const u8) ![]u8 { NSSearchPathDomainMask.NSUserDomainMask, @as(?*anyopaque, null), true, - &err, + @as(?*anyopaque, null), }, ); + + // I don't think this is possible but just in case. + if (url.value == null) return error.AppleAPIFailed; + + // Get the UTF-8 string from the URL. const path = url.getProperty(objc.Object, "path"); - const c_str = path.getProperty([*:0]const u8, "UTF8String"); + const c_str = path.getProperty(?[*:0]const u8, "UTF8String") orelse + return error.AppleAPIFailed; const app_support_dir = std.mem.sliceTo(c_str, 0); - return try std.fs.path.join(alloc, &.{ app_support_dir, "com.mitchellh.ghostty", sub_path }); + return try std.fs.path.join(alloc, &.{ + app_support_dir, + "com.mitchellh.ghostty", + sub_path, + }); } + +pub const NSOperatingSystemVersion = extern struct { + major: i64, + minor: i64, + patch: i64, +}; + +pub const NSSearchPathDirectory = enum(c_ulong) { + NSApplicationSupportDirectory = 14, +}; + +pub const NSSearchPathDomainMask = enum(c_ulong) { + NSUserDomainMask = 1, +}; From b9345e8d6abbbca506fd18061af1f2c1c7c9906b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 25 Nov 2024 16:10:55 -0800 Subject: [PATCH 12/23] try to abstract bundle ID to a zig file --- src/apprt/gtk/App.zig | 3 ++- src/apprt/gtk/ConfigErrorsWindow.zig | 3 ++- src/apprt/gtk/Surface.zig | 3 ++- src/apprt/gtk/Window.zig | 2 +- src/apprt/gtk/inspector.zig | 3 ++- src/build_config.zig | 14 ++++++++++++++ src/main_ghostty.zig | 2 +- src/os/macos.zig | 3 ++- 8 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 0cee1938e..5e41fcdae 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -14,6 +14,7 @@ const std = @import("std"); const assert = std.debug.assert; const Allocator = std.mem.Allocator; const builtin = @import("builtin"); +const build_config = @import("../../build_config.zig"); const apprt = @import("../../apprt.zig"); const configpkg = @import("../../config.zig"); const input = @import("../../input.zig"); @@ -181,7 +182,7 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { } } - const default_id = "com.mitchellh.ghostty"; + const default_id = comptime build_config.bundle_id; break :app_id if (builtin.mode == .Debug) default_id ++ "-debug" else default_id; }; diff --git a/src/apprt/gtk/ConfigErrorsWindow.zig b/src/apprt/gtk/ConfigErrorsWindow.zig index 6d4cda21b..3ff52908e 100644 --- a/src/apprt/gtk/ConfigErrorsWindow.zig +++ b/src/apprt/gtk/ConfigErrorsWindow.zig @@ -3,6 +3,7 @@ const ConfigErrors = @This(); const std = @import("std"); const Allocator = std.mem.Allocator; +const build_config = @import("../../build_config.zig"); const configpkg = @import("../../config.zig"); const Config = configpkg.Config; @@ -53,7 +54,7 @@ fn init(self: *ConfigErrors, app: *App) !void { c.gtk_window_set_title(gtk_window, "Configuration Errors"); c.gtk_window_set_default_size(gtk_window, 600, 275); c.gtk_window_set_resizable(gtk_window, 0); - c.gtk_window_set_icon_name(gtk_window, "com.mitchellh.ghostty"); + c.gtk_window_set_icon_name(gtk_window, build_config.bundle_id); _ = c.g_signal_connect_data(window, "destroy", c.G_CALLBACK(>kDestroy), self, null, c.G_CONNECT_DEFAULT); // Set some state diff --git a/src/apprt/gtk/Surface.zig b/src/apprt/gtk/Surface.zig index aef67b308..9a361c228 100644 --- a/src/apprt/gtk/Surface.zig +++ b/src/apprt/gtk/Surface.zig @@ -5,6 +5,7 @@ const Surface = @This(); const std = @import("std"); const Allocator = std.mem.Allocator; +const build_config = @import("../../build_config.zig"); const configpkg = @import("../../config.zig"); const apprt = @import("../../apprt.zig"); const font = @import("../../font/main.zig"); @@ -1149,7 +1150,7 @@ pub fn showDesktopNotification( defer c.g_object_unref(notification); c.g_notification_set_body(notification, body.ptr); - const icon = c.g_themed_icon_new("com.mitchellh.ghostty"); + const icon = c.g_themed_icon_new(build_config.bundle_id); defer c.g_object_unref(icon); c.g_notification_set_icon(notification, icon); diff --git a/src/apprt/gtk/Window.zig b/src/apprt/gtk/Window.zig index e220ac03b..23265c101 100644 --- a/src/apprt/gtk/Window.zig +++ b/src/apprt/gtk/Window.zig @@ -103,7 +103,7 @@ pub fn init(self: *Window, app: *App) !void { // to disable this so that terminal programs can capture F10 (such as htop) c.gtk_window_set_handle_menubar_accel(gtk_window, 0); - c.gtk_window_set_icon_name(gtk_window, "com.mitchellh.ghostty"); + c.gtk_window_set_icon_name(gtk_window, build_config.bundle_id); // Apply class to color headerbar if window-theme is set to `ghostty` and // GTK version is before 4.16. The conditional is because above 4.16 diff --git a/src/apprt/gtk/inspector.zig b/src/apprt/gtk/inspector.zig index f5bdf8a24..119e20a6c 100644 --- a/src/apprt/gtk/inspector.zig +++ b/src/apprt/gtk/inspector.zig @@ -2,6 +2,7 @@ const std = @import("std"); const Allocator = std.mem.Allocator; const assert = std.debug.assert; +const build_config = @import("../../build_config.zig"); const App = @import("App.zig"); const Surface = @import("Surface.zig"); const TerminalWindow = @import("Window.zig"); @@ -141,7 +142,7 @@ const Window = struct { self.window = gtk_window; c.gtk_window_set_title(gtk_window, "Ghostty: Terminal Inspector"); c.gtk_window_set_default_size(gtk_window, 1000, 600); - c.gtk_window_set_icon_name(gtk_window, "com.mitchellh.ghostty"); + c.gtk_window_set_icon_name(gtk_window, build_config.bundle_id); // Initialize our imgui widget try self.imgui_widget.init(); diff --git a/src/build_config.zig b/src/build_config.zig index 715552e03..1448f9de5 100644 --- a/src/build_config.zig +++ b/src/build_config.zig @@ -103,6 +103,20 @@ pub const app_runtime: apprt.Runtime = config.app_runtime; pub const font_backend: font.Backend = config.font_backend; pub const renderer: rendererpkg.Impl = config.renderer; +/// The bundle ID for the app. This is used in many places and is currently +/// hardcoded here. We could make this configurable in the future if there +/// is a reason to do so. +/// +/// On macOS, this must match the App bundle ID. We can get that dynamically +/// via an API but I don't want to pay the cost of that at runtime. +/// +/// On GTK, this should match the various folders with resources. +/// +/// There are many places that don't use this variable so simply swapping +/// this variable is NOT ENOUGH to change the bundle ID. I just wanted to +/// avoid it in Zig coe as much as possible. +pub const bundle_id = "com.mitchellh.ghostty"; + /// True if we should have "slow" runtime safety checks. The initial motivation /// for this was terminal page/pagelist integrity checks. These were VERY /// slow but very thorough. But they made it so slow that the terminal couldn't diff --git a/src/main_ghostty.zig b/src/main_ghostty.zig index 071d4d530..b3df80538 100644 --- a/src/main_ghostty.zig +++ b/src/main_ghostty.zig @@ -141,7 +141,7 @@ fn logFn( // Initialize a logger. This is slow to do on every operation // but we shouldn't be logging too much. - const logger = macos.os.Log.create("com.mitchellh.ghostty", @tagName(scope)); + const logger = macos.os.Log.create(build_config.bundle_id, @tagName(scope)); defer logger.release(); logger.log(std.heap.c_allocator, mac_level, format, args); } diff --git a/src/os/macos.zig b/src/os/macos.zig index fe312c6e1..d405cd161 100644 --- a/src/os/macos.zig +++ b/src/os/macos.zig @@ -1,5 +1,6 @@ const std = @import("std"); const builtin = @import("builtin"); +const build_config = @import("../build_config.zig"); const assert = std.debug.assert; const objc = @import("objc"); const Allocator = std.mem.Allocator; @@ -56,7 +57,7 @@ pub fn appSupportDir( return try std.fs.path.join(alloc, &.{ app_support_dir, - "com.mitchellh.ghostty", + build_config.bundle_id, sub_path, }); } From 2cbc2833d10046333b698d1caa601bc7ef85de98 Mon Sep 17 00:00:00 2001 From: Gregory Anders Date: Tue, 26 Nov 2024 08:59:51 -0600 Subject: [PATCH 13/23] termio: fixes to kitty color reporting The kitty color report response is an OSC, not a CSI, so change `[` to `]`. Colors which are unset should be reported with `{key}=`. --- src/termio/stream_handler.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/termio/stream_handler.zig b/src/termio/stream_handler.zig index 2fc9e92af..64915f704 100644 --- a/src/termio/stream_handler.zig +++ b/src/termio/stream_handler.zig @@ -1418,7 +1418,7 @@ pub const StreamHandler = struct { var buf = std.ArrayList(u8).init(self.alloc); defer buf.deinit(); const writer = buf.writer(); - try writer.writeAll("\x1b[21"); + try writer.writeAll("\x1b]21"); for (request.list.items) |item| { switch (item) { @@ -1435,7 +1435,7 @@ pub const StreamHandler = struct { }, }, } orelse { - log.warn("no color configured for {}", .{key}); + try writer.print(";{}=", .{key}); continue; }; From 39fbd7db4baa36db8366379b1955df8c533e5bd9 Mon Sep 17 00:00:00 2001 From: Isaac Mills Date: Tue, 26 Nov 2024 11:10:00 -0700 Subject: [PATCH 14/23] Prevent GTK from initializing Vulkan. This improves startup time --- src/apprt/gtk/App.zig | 7 ++++++- src/termio/Exec.zig | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 0cee1938e..95942bc9a 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -99,9 +99,11 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { c.gtk_get_micro_version(), }); + // Disabling Vulkan can improve startup times by hundreds of + // milliseconds on some systems if (version.atLeast(4, 16, 0)) { // From gtk 4.16, GDK_DEBUG is split into GDK_DEBUG and GDK_DISABLE - _ = internal_os.setenv("GDK_DISABLE", "gles-api"); + _ = internal_os.setenv("GDK_DISABLE", "gles-api,vulkan"); _ = internal_os.setenv("GDK_DEBUG", "opengl"); } else if (version.atLeast(4, 14, 0)) { // We need to export GDK_DEBUG to run on Wayland after GTK 4.14. @@ -110,7 +112,10 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { // reassess... // // Upstream issue: https://gitlab.gnome.org/GNOME/gtk/-/issues/6589 + _ = internal_os.setenv("GDK_DISABLE", "vulkan"); _ = internal_os.setenv("GDK_DEBUG", "opengl,gl-disable-gles"); + } else { + _ = internal_os.setenv("GDK_DISABLE", "vulkan"); } if (version.atLeast(4, 14, 0)) { diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 07aa43c42..41f86958e 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -843,6 +843,7 @@ const Subprocess = struct { // Don't leak these environment variables to child processes. if (comptime build_config.app_runtime == .gtk) { env.remove("GDK_DEBUG"); + env.remove("GDK_DISABLE"); env.remove("GSK_RENDERER"); } From 9171cb5c295614a71d50f399cee6eb048f37ca0d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 10:49:45 -0800 Subject: [PATCH 15/23] config: implement `clone` for RepeatableLink Fixes #2819 --- src/config/Config.zig | 30 +++++++++++++++++++++++------- src/input/Link.zig | 18 ++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 253e96420..bf29994c3 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -4610,17 +4610,33 @@ pub const RepeatableLink = struct { } /// Deep copy of the struct. Required by Config. - pub fn clone(self: *const Self, alloc: Allocator) error{}!Self { - _ = self; - _ = alloc; - return .{}; + pub fn clone( + self: *const Self, + alloc: Allocator, + ) Allocator.Error!Self { + // Note: we don't do any errdefers below since the allocation + // is expected to be arena allocated. + + var list = try std.ArrayListUnmanaged(inputpkg.Link).initCapacity( + alloc, + self.links.items.len, + ); + for (self.links.items) |item| { + const copy = try item.clone(alloc); + list.appendAssumeCapacity(copy); + } + + return .{ .links = list }; } /// Compare if two of our value are requal. Required by Config. pub fn equal(self: Self, other: Self) bool { - _ = self; - _ = other; - return true; + const itemsA = self.links.items; + const itemsB = other.links.items; + if (itemsA.len != itemsB.len) return false; + for (itemsA, itemsB) |*a, *b| { + if (!a.equal(b)) return false; + } else return true; } /// Used by Formatter diff --git a/src/input/Link.zig b/src/input/Link.zig index adc52a270..37b45dbd1 100644 --- a/src/input/Link.zig +++ b/src/input/Link.zig @@ -4,6 +4,8 @@ //! action types. const Link = @This(); +const std = @import("std"); +const Allocator = std.mem.Allocator; const oni = @import("oniguruma"); const Mods = @import("key.zig").Mods; @@ -59,3 +61,19 @@ pub fn oniRegex(self: *const Link) !oni.Regex { null, ); } + +/// Deep clone the link. +pub fn clone(self: *const Link, alloc: Allocator) Allocator.Error!Link { + return .{ + .regex = try alloc.dupe(u8, self.regex), + .action = self.action, + .highlight = self.highlight, + }; +} + +/// Check if two links are equal. +pub fn equal(self: *const Link, other: *const Link) bool { + return std.meta.eql(self.action, other.action) and + std.meta.eql(self.highlight, other.highlight) and + std.mem.eql(u8, self.regex, other.regex); +} From e3621e81b74327bffee11d59823e6fa0e2855a4c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 13:17:35 -0800 Subject: [PATCH 16/23] apprt/gtk: use proper env var for vulkan disable on <= 4.14 --- src/apprt/gtk/App.zig | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 95942bc9a..93f08c459 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -100,9 +100,11 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { }); // Disabling Vulkan can improve startup times by hundreds of - // milliseconds on some systems + // milliseconds on some systems. We don't use Vulkan so we can just + // disable it. if (version.atLeast(4, 16, 0)) { - // From gtk 4.16, GDK_DEBUG is split into GDK_DEBUG and GDK_DISABLE + // From gtk 4.16, GDK_DEBUG is split into GDK_DEBUG and GDK_DISABLE. + // For the remainder of "why" see the 4.14 comment below. _ = internal_os.setenv("GDK_DISABLE", "gles-api,vulkan"); _ = internal_os.setenv("GDK_DEBUG", "opengl"); } else if (version.atLeast(4, 14, 0)) { @@ -112,14 +114,14 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { // reassess... // // Upstream issue: https://gitlab.gnome.org/GNOME/gtk/-/issues/6589 - _ = internal_os.setenv("GDK_DISABLE", "vulkan"); - _ = internal_os.setenv("GDK_DEBUG", "opengl,gl-disable-gles"); + _ = internal_os.setenv("GDK_DEBUG", "opengl,gl-disable-gles,vulkan-disable"); } else { - _ = internal_os.setenv("GDK_DISABLE", "vulkan"); + _ = internal_os.setenv("GDK_DEBUG", "vulkan-disable"); } if (version.atLeast(4, 14, 0)) { - // We need to export GSK_RENDERER to opengl because GTK uses ngl by default after 4.14 + // We need to export GSK_RENDERER to opengl because GTK uses ngl by + // default after 4.14 _ = internal_os.setenv("GSK_RENDERER", "opengl"); } From e20b27de848afaa167c70d838a2f98f28c6ac0b5 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 14:27:44 -0800 Subject: [PATCH 17/23] terminal: eraseChars was checking wide char split boundary on wrong cell Fixes #2817 The test is pretty explanatory. I also renamed `end` to `count` since I think this poor naming was the reason for the bug. In `eraseChars`, the `count` (nee `end`) is the number of cells to erase, not the index of the last cell to erase. --- src/terminal/Terminal.zig | 44 +++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index f5784b6ab..8de914a3e 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -1955,13 +1955,9 @@ pub fn deleteChars(self: *Terminal, count_req: usize) void { } pub fn eraseChars(self: *Terminal, count_req: usize) void { - const count = @max(count_req, 1); - - // Our last index is at most the end of the number of chars we have - // in the current line. - const end = end: { + const count = end: { const remaining = self.cols - self.screen.cursor.x; - var end = @min(remaining, count); + var end = @min(remaining, @max(count_req, 1)); // If our last cell is a wide char then we need to also clear the // cell beyond it since we can't just split a wide char. @@ -1979,7 +1975,7 @@ pub fn eraseChars(self: *Terminal, count_req: usize) void { // protected modes. We need to figure out how to make `clearCells` or at // least `clearUnprotectedCells` handle boundary conditions... self.screen.splitCellBoundary(self.screen.cursor.x); - self.screen.splitCellBoundary(end); + self.screen.splitCellBoundary(self.screen.cursor.x + count); // Reset our row's soft-wrap. self.screen.cursorResetWrap(); @@ -1997,7 +1993,7 @@ pub fn eraseChars(self: *Terminal, count_req: usize) void { self.screen.clearCells( &self.screen.cursor.page_pin.node.data, self.screen.cursor.page_row, - cells[0..end], + cells[0..count], ); return; } @@ -2005,7 +2001,7 @@ pub fn eraseChars(self: *Terminal, count_req: usize) void { self.screen.clearUnprotectedCells( &self.screen.cursor.page_pin.node.data, self.screen.cursor.page_row, - cells[0..end], + cells[0..count], ); } @@ -6104,6 +6100,36 @@ test "Terminal: eraseChars wide char boundary conditions" { } } +test "Terminal: eraseChars wide char splits proper cell boundaries" { + const alloc = testing.allocator; + var t = try init(alloc, .{ .rows = 1, .cols = 30 }); + defer t.deinit(alloc); + + // This is a test for a bug: https://github.com/ghostty-org/ghostty/issues/2817 + // To explain the setup: + // (1) We need our wide characters starting on an even (1-based) column. + // (2) We need our cursor to be in the middle somewhere. + // (3) We need our count to be less than our cursor X and on a split cell. + // The bug was that we split the wrong cell boundaries. + + try t.printString("x食べて下さい"); + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("x食べて下さい", str); + } + + t.setCursorPos(1, 6); // At: て + t.eraseChars(4); // Delete: て下 + t.screen.cursor.page_pin.node.data.assertIntegrity(); + + { + const str = try t.plainString(alloc); + defer testing.allocator.free(str); + try testing.expectEqualStrings("x食べ さい", str); + } +} + test "Terminal: eraseChars wide char wrap boundary conditions" { const alloc = testing.allocator; var t = try init(alloc, .{ .rows = 3, .cols = 8 }); From 2e939b617e8e20652e25067b041595441ff6b380 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 15:10:29 -0800 Subject: [PATCH 18/23] config: clone should copy diagnostics Fixes #2800 --- src/cli/diagnostics.zig | 55 +++++++++++++++++++++++++++++++++++++++++ src/config/Config.zig | 3 +++ 2 files changed, 58 insertions(+) diff --git a/src/cli/diagnostics.zig b/src/cli/diagnostics.zig index e4d390c03..8090684fd 100644 --- a/src/cli/diagnostics.zig +++ b/src/cli/diagnostics.zig @@ -34,6 +34,14 @@ pub const Diagnostic = struct { try writer.print("{s}", .{self.message}); } + + pub fn clone(self: *const Diagnostic, alloc: Allocator) Allocator.Error!Diagnostic { + return .{ + .location = try self.location.clone(alloc), + .key = try alloc.dupeZ(u8, self.key), + .message = try alloc.dupeZ(u8, self.message), + }; + } }; /// The possible locations for a diagnostic message. This is used @@ -61,6 +69,19 @@ pub const Location = union(enum) { if (!@hasDecl(Iter, "location")) return .none; return iter.location() orelse .none; } + + pub fn clone(self: *const Location, alloc: Allocator) Allocator.Error!Location { + return switch (self.*) { + .none, + .cli, + => self.*, + + .file => |v| .{ .file = .{ + .path = try alloc.dupe(u8, v.path), + .line = v.line, + } }, + }; + } }; /// A list of diagnostics. The "_diagnostics" field must be this type @@ -88,11 +109,45 @@ pub const DiagnosticList = struct { // We specifically want precompute for libghostty. .lib => true, }; + const Precompute = if (precompute_enabled) struct { messages: std.ArrayListUnmanaged([:0]const u8) = .{}, + + pub fn clone( + self: *const Precompute, + alloc: Allocator, + ) Allocator.Error!Precompute { + var result: Precompute = .{}; + try result.messages.ensureTotalCapacity(alloc, self.messages.items.len); + for (self.messages.items) |msg| { + result.messages.appendAssumeCapacity( + try alloc.dupeZ(u8, msg), + ); + } + return result; + } } else void; + const precompute_init: Precompute = if (precompute_enabled) .{} else {}; + pub fn clone( + self: *const DiagnosticList, + alloc: Allocator, + ) Allocator.Error!DiagnosticList { + var result: DiagnosticList = .{}; + + try result.list.ensureTotalCapacity(alloc, self.list.items.len); + for (self.list.items) |*diag| result.list.appendAssumeCapacity( + try diag.clone(alloc), + ); + + if (comptime precompute_enabled) { + result.precompute = try self.precompute.clone(alloc); + } + + return result; + } + pub fn append( self: *DiagnosticList, alloc: Allocator, diff --git a/src/config/Config.zig b/src/config/Config.zig index bf29994c3..2dc732752 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -3063,6 +3063,9 @@ pub fn clone( ); } + // Copy our diagnostics + result._diagnostics = try self._diagnostics.clone(alloc_arena); + // Preserve our replay steps. We copy them exactly to also preserve // the exact conditionals required for some steps. try result._replay_steps.ensureTotalCapacity( From a482224da8577246d4a31f72d298a25352e0df9d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 25 Nov 2024 19:57:29 -0800 Subject: [PATCH 19/23] renderer: set QoS class of the renderer thread on macOS This sets the macOS QoS class of the renderer thread. Apple recommends[1] that all threads should have a QoS class set, and there are many benefits[2] to that, mainly around power management moreso than performance I'd expect. In this commit, I start by setting the QoS class of the renderer thread. By default, the renderer thread is set to user interactive, because it is a UI thread after all. But under some conditions we downgrade: - If the surface is not visible at all (i.e. another window is fully covering it or its minimized), we set the QoS class to utility. This is lower than the default, previous QoS and should help macOS unschedule the workload or move it to a different core. - If the surface is visible but not focused, we set the QoS class to user initiated. This is lower than user interactive but higher than default. The renderer should remain responsive but not consume as much time as it would if it was user interactive. I'm unable to see any noticable difference in anything from these changes. Unfortunately it doesn't seem like Apple provides good tools to play around with this. We should continue to apply QoS classes to our other threads on macOS. [1]: https://developer.apple.com/documentation/apple-silicon/tuning-your-code-s-performance-for-apple-silicon?preferredLanguage=occl [2]: https://blog.xoria.org/macos-tips-threading/ --- src/os/macos.zig | 41 +++++++++++++++++++++++++++++ src/renderer/Thread.zig | 58 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/src/os/macos.zig b/src/os/macos.zig index d405cd161..53dfd1719 100644 --- a/src/os/macos.zig +++ b/src/os/macos.zig @@ -62,6 +62,47 @@ pub fn appSupportDir( }); } +pub const SetQosClassError = error{ + // The thread can't have its QoS class changed usually because + // a different pthread API was called that makes it an invalid + // target. + ThreadIncompatible, +}; + +/// Set the QoS class of the running thread. +/// +/// https://developer.apple.com/documentation/apple-silicon/tuning-your-code-s-performance-for-apple-silicon?preferredLanguage=occ +pub fn setQosClass(class: QosClass) !void { + return switch (std.posix.errno(pthread_set_qos_class_self_np( + class, + 0, + ))) { + .SUCCESS => {}, + .PERM => error.ThreadIncompatible, + + // EPERM is the only known error that can happen based on + // the man pages for pthread_set_qos_class_self_np. I haven't + // checked the XNU source code to see if there are other + // possible errors. + else => @panic("unexpected pthread_set_qos_class_self_np error"), + }; +} + +/// https://developer.apple.com/library/archive/documentation/Performance/Conceptual/power_efficiency_guidelines_osx/PrioritizeWorkAtTheTaskLevel.html#//apple_ref/doc/uid/TP40013929-CH35-SW1 +pub const QosClass = enum(c_uint) { + user_interactive = 0x21, + user_initiated = 0x19, + default = 0x15, + utility = 0x11, + background = 0x09, + unspecified = 0x00, +}; + +extern "c" fn pthread_set_qos_class_self_np( + qos_class: QosClass, + relative_priority: c_int, +) c_int; + pub const NSOperatingSystemVersion = extern struct { major: i64, minor: i64, diff --git a/src/renderer/Thread.zig b/src/renderer/Thread.zig index 91e355480..cc63889fa 100644 --- a/src/renderer/Thread.zig +++ b/src/renderer/Thread.zig @@ -4,8 +4,10 @@ pub const Thread = @This(); const std = @import("std"); const builtin = @import("builtin"); +const assert = std.debug.assert; const xev = @import("xev"); const crash = @import("../crash/main.zig"); +const internal_os = @import("../os/main.zig"); const renderer = @import("../renderer.zig"); const apprt = @import("../apprt.zig"); const configpkg = @import("../config.zig"); @@ -92,6 +94,10 @@ flags: packed struct { /// This is true when the view is visible. This is used to determine /// if we should be rendering or not. visible: bool = true, + + /// This is true when the view is focused. This defaults to true + /// and it is up to the apprt to set the correct value. + focused: bool = true, } = .{}, pub const DerivedConfig = struct { @@ -199,6 +205,9 @@ fn threadMain_(self: *Thread) !void { }; defer crash.sentry.thread_state = null; + // Setup our thread QoS + self.setQosClass(); + // Run our loop start/end callbacks if the renderer cares. const has_loop = @hasDecl(renderer.Renderer, "loopEnter"); if (has_loop) try self.renderer.loopEnter(self); @@ -237,6 +246,36 @@ fn threadMain_(self: *Thread) !void { _ = try self.loop.run(.until_done); } +fn setQosClass(self: *const Thread) void { + // Thread QoS classes are only relevant on macOS. + if (comptime !builtin.target.isDarwin()) return; + + const class: internal_os.macos.QosClass = class: { + // If we aren't visible (our view is fully occluded) then we + // always drop our rendering priority down because it's just + // mostly wasted work. + // + // The renderer itself should be doing this as well (for example + // Metal will stop our DisplayLink) but this also helps with + // general forced updates and CPU usage i.e. a rebuild cells call. + if (!self.flags.visible) break :class .utility; + + // If we're not focused, but we're visible, then we set a higher + // than default priority because framerates still matter but it isn't + // as important as when we're focused. + if (!self.flags.focused) break :class .user_initiated; + + // We are focused and visible, we are the definition of user interactive. + break :class .user_interactive; + }; + + if (internal_os.macos.setQosClass(class)) { + log.debug("thread QoS class set class={}", .{class}); + } else |err| { + log.warn("error setting QoS class err={}", .{err}); + } +} + fn startDrawTimer(self: *Thread) void { // If our renderer doesn't support animations then we never run this. if (!@hasDecl(renderer.Renderer, "hasAnimations")) return; @@ -273,10 +312,16 @@ fn drainMailbox(self: *Thread) !void { switch (message) { .crash => @panic("crash request, crashing intentionally"), - .visible => |v| { + .visible => |v| visible: { + // If our state didn't change we do nothing. + if (self.flags.visible == v) break :visible; + // Set our visible state self.flags.visible = v; + // Visibility affects our QoS class + self.setQosClass(); + // If we became visible then we immediately trigger a draw. // We don't need to update frame data because that should // still be happening. @@ -293,7 +338,16 @@ fn drainMailbox(self: *Thread) !void { // check the visible state themselves to control their behavior. }, - .focus => |v| { + .focus => |v| focus: { + // If our state didn't change we do nothing. + if (self.flags.focused == v) break :focus; + + // Set our state + self.flags.focused = v; + + // Focus affects our QoS class + self.setQosClass(); + // Set it on the renderer try self.renderer.setFocus(v); From f12ac32c973091f698fc521d9f293ae477875dce Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 16:35:27 -0800 Subject: [PATCH 20/23] README: clarify config docs location --- README.md | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 3c3a2460d..861e0937a 100644 --- a/README.md +++ b/README.md @@ -107,25 +107,40 @@ palette = 7=#a89984 palette = 15=#fbf1c7 ``` -You can view all available configuration options and their documentation -by executing the command `ghostty +show-config --default --docs`. Note that -this will output the full default configuration with docs to stdout, so -you may want to pipe that through a pager, an editor, etc. +#### Configuration Documentation + +There are multiple places to find documentation on the configuration options. +All locations are identical (they're all generated from the same source): + +1. There are HTML and Markdown formatted docs in the + `$prefix/share/ghostty/docs` directory. This directory is created + when you build or install Ghostty. The `$prefix` is `zig-out` if you're + building from source (or the specified `--prefix` flag). On macOS, + `$prefix` is the `Contents/Resources` subdirectory of the `.app` bundle. + +2. There are man pages in the `$prefix/share/man` directory. This directory + is created when you build or install Ghostty. + +3. In the CLI, you can run `ghostty +show-config --default --docs`. + Note that this will output the full default configuration with docs to + stdout, so you may want to pipe that through a pager, an editor, etc. + +4. In the source code, you can find the configuration structure in the + [Config structure](https://github.com/ghostty-org/ghostty/blob/main/src/config/Config.zig). + The available keys are the keys verbatim, and their possible values are typically + documented in the comments. + +5. Not documentation per se, but you can search for the + [public config files](https://github.com/search?q=path%3Aghostty%2Fconfig&type=code) + of many Ghostty users for examples and inspiration. > [!NOTE] > -> You'll see a lot of weird blank configurations like `font-family =`. This +> You may see strange looking blank configurations like `font-family =`. This > is a valid syntax to specify the default behavior (no value). The > `+show-config` outputs it so it's clear that key is defaulting and also > to have something to attach the doc comment to. -You can also see and read all available configuration options in the source -[Config structure](https://github.com/ghostty-org/ghostty/blob/main/src/config/Config.zig). -The available keys are the keys verbatim, and their possible values are typically -documented in the comments. You also can search for the -[public config files](https://github.com/search?q=path%3Aghostty%2Fconfig&type=code) -of many Ghostty users for examples and inspiration. - > [!NOTE] > > Configuration can be reloaded on the fly with the `reload_config` From abafb81a1b5180d8cf009f7eb68bbea5b6141b8c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 26 Nov 2024 16:50:04 -0800 Subject: [PATCH 21/23] apprt/gtk: update app color scheme state Fixes #2781 This commit contains two separate changes but very related: 1. We update the color scheme state of the app on app start. This is necessary so that the configuration properly reflects the conditional state of the theme at the app level (i.e. the window headerbar). 2. We take ownership of the new config when it changes, matching macOS. This ensures that things like our GTK headerbar update when the theme changes but more generally whenever any config changes. And some housekeeping: - We remove runtime CSS setup from init. We can do it on the first tick of `run` instead. This will probably save some CPU cycles especially when we're just notifying a single instance to create a new window. - I moved dbus event setup to `run` as well. We don't need to know these events unless we're actually running the app. Similar to the above, should save some CPU time on single instance runs. --- src/apprt/gtk/App.zig | 88 ++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 34 deletions(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index f81c1a76a..ead41de7c 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -385,22 +385,6 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { if (config.@"initial-window") c.g_application_activate(gapp); - // Register for dbus events - if (c.g_application_get_dbus_connection(gapp)) |dbus_connection| { - _ = c.g_dbus_connection_signal_subscribe( - dbus_connection, - null, - "org.freedesktop.portal.Settings", - "SettingChanged", - "/org/freedesktop/portal/desktop", - "org.freedesktop.appearance", - c.G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, - >kNotifyColorScheme, - core_app, - null, - ); - } - // Internally, GTK ensures that only one instance of this provider exists in the provider list // for the display. const css_provider = c.gtk_css_provider_new(); @@ -409,12 +393,6 @@ pub fn init(core_app: *CoreApp, opts: Options) !App { @ptrCast(css_provider), c.GTK_STYLE_PROVIDER_PRIORITY_APPLICATION + 3, ); - loadRuntimeCss(core_app.alloc, &config, css_provider) catch |err| switch (err) { - error.OutOfMemory => log.warn( - "out of memory loading runtime CSS, no runtime CSS applied", - .{}, - ), - }; return .{ .core_app = core_app, @@ -831,14 +809,20 @@ fn configChange( target: apprt.Target, new_config: *const Config, ) void { - _ = new_config; - switch (target) { // We don't do anything for surface config change events. There // is nothing to sync with regards to a surface today. .surface => {}, .app => { + // We clone (to take ownership) and update our configuration. + if (new_config.clone(self.core_app.alloc)) |config_clone| { + self.config.deinit(); + self.config = config_clone; + } else |err| { + log.warn("error cloning configuration err={}", .{err}); + } + self.syncConfigChanges() catch |err| { log.warn("error handling configuration changes err={}", .{err}); }; @@ -892,7 +876,7 @@ fn syncConfigChanges(self: *App) !void { // Load our runtime CSS. If this fails then our window is just stuck // with the old CSS but we don't want to fail the entire sync operation. - loadRuntimeCss(self.core_app.alloc, &self.config, self.css_provider) catch |err| switch (err) { + self.loadRuntimeCss() catch |err| switch (err) { error.OutOfMemory => log.warn( "out of memory loading runtime CSS, no runtime CSS applied", .{}, @@ -956,15 +940,14 @@ fn syncActionAccelerator( } fn loadRuntimeCss( - alloc: Allocator, - config: *const Config, - provider: *c.GtkCssProvider, + self: *const App, ) Allocator.Error!void { - var stack_alloc = std.heap.stackFallback(4096, alloc); + var stack_alloc = std.heap.stackFallback(4096, self.core_app.alloc); var buf = std.ArrayList(u8).init(stack_alloc.get()); defer buf.deinit(); const writer = buf.writer(); + const config: *const Config = &self.config; const window_theme = config.@"window-theme"; const unfocused_fill: Config.Color = config.@"unfocused-split-fill" orelse config.background; const headerbar_background = config.background; @@ -1027,7 +1010,7 @@ fn loadRuntimeCss( // Clears any previously loaded CSS from this provider c.gtk_css_provider_load_from_data( - provider, + self.css_provider, buf.items.ptr, @intCast(buf.items.len), ); @@ -1076,11 +1059,17 @@ pub fn run(self: *App) !void { self.transient_cgroup_base = path; } else log.debug("cgroup isolation disabled config={}", .{self.config.@"linux-cgroup"}); + // Setup our D-Bus connection for listening to settings changes. + self.initDbus(); + // Setup our menu items self.initActions(); self.initMenu(); self.initContextMenu(); + // Setup our initial color scheme + self.colorSchemeEvent(self.getColorScheme()); + // On startup, we want to check for configuration errors right away // so we can show our error window. We also need to setup other initial // state. @@ -1114,6 +1103,26 @@ pub fn run(self: *App) !void { } } +fn initDbus(self: *App) void { + const dbus = c.g_application_get_dbus_connection(@ptrCast(self.app)) orelse { + log.warn("unable to get dbus connection, not setting up events", .{}); + return; + }; + + _ = c.g_dbus_connection_signal_subscribe( + dbus, + null, + "org.freedesktop.portal.Settings", + "SettingChanged", + "/org/freedesktop/portal/desktop", + "org.freedesktop.appearance", + c.G_DBUS_SIGNAL_FLAGS_MATCH_ARG0_NAMESPACE, + >kNotifyColorScheme, + self, + null, + ); +} + // This timeout function is started when no surfaces are open. It can be // cancelled if a new surface is opened before the timer expires. pub fn gtkQuitTimerExpired(ud: ?*anyopaque) callconv(.C) c.gboolean { @@ -1394,7 +1403,7 @@ fn gtkNotifyColorScheme( parameters: ?*c.GVariant, user_data: ?*anyopaque, ) callconv(.C) void { - const core_app: *CoreApp = @ptrCast(@alignCast(user_data orelse { + const self: *App = @ptrCast(@alignCast(user_data orelse { log.err("style change notification: userdata is null", .{}); return; })); @@ -1426,9 +1435,20 @@ fn gtkNotifyColorScheme( else .light; - for (core_app.surfaces.items) |surface| { - surface.core_surface.colorSchemeCallback(color_scheme) catch |err| { - log.err("unable to tell surface about color scheme change: {}", .{err}); + self.colorSchemeEvent(color_scheme); +} + +fn colorSchemeEvent( + self: *App, + scheme: apprt.ColorScheme, +) void { + self.core_app.colorSchemeEvent(self, scheme) catch |err| { + log.err("error updating app color scheme err={}", .{err}); + }; + + for (self.core_app.surfaces.items) |surface| { + surface.core_surface.colorSchemeCallback(scheme) catch |err| { + log.err("unable to tell surface about color scheme change err={}", .{err}); }; } } From ba4185f6b78124da5b91f0675f62f8e7e31deb4d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 27 Nov 2024 08:31:49 -0800 Subject: [PATCH 22/23] macos: disable background opacity/blur in native fullscreen See #2840 --- .../Features/Terminal/TerminalController.swift | 18 +++++++++++++++++- src/config/Config.zig | 4 ++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/macos/Sources/Features/Terminal/TerminalController.swift b/macos/Sources/Features/Terminal/TerminalController.swift index 81c74987b..698551f3e 100644 --- a/macos/Sources/Features/Terminal/TerminalController.swift +++ b/macos/Sources/Features/Terminal/TerminalController.swift @@ -94,6 +94,16 @@ class TerminalController: BaseTerminalController { } } + + override func fullscreenDidChange() { + super.fullscreenDidChange() + + // When our fullscreen state changes, we resync our appearance because some + // properties change when fullscreen or not. + guard let focusedSurface else { return } + syncAppearance(focusedSurface.derivedConfig) + } + //MARK: - Methods @objc private func ghosttyConfigDidChange(_ notification: Notification) { @@ -204,7 +214,13 @@ class TerminalController: BaseTerminalController { } // If we have window transparency then set it transparent. Otherwise set it opaque. - if (surfaceConfig.backgroundOpacity < 1) { + + // Window transparency only takes effect if our window is not native fullscreen. + // In native fullscreen we disable transparency/opacity because the background + // becomes gray and widgets show through. + if (!window.styleMask.contains(.fullScreen) && + surfaceConfig.backgroundOpacity < 1 + ) { window.isOpaque = false // This is weird, but we don't use ".clear" because this creates a look that diff --git a/src/config/Config.zig b/src/config/Config.zig index 2dc732752..97ac19226 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -527,6 +527,10 @@ palette: Palette = .{}, /// The opacity level (opposite of transparency) of the background. A value of /// 1 is fully opaque and a value of 0 is fully transparent. A value less than 0 /// or greater than 1 will be clamped to the nearest valid value. +/// +/// On macOS, background opacity is disabled when the terminal enters native +/// fullscreen. This is because the background becomes gray and it can cause +/// widgets to show through which isn't generally desirable. @"background-opacity": f64 = 1.0, /// A positive value enables blurring of the background when background-opacity From 5b01cb353de47a0053c313e3bc20170cbece679e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 27 Nov 2024 08:46:03 -0800 Subject: [PATCH 23/23] config: need to dupe filepath for diagnostics Fixes #2800 The source string with the filepath is not guaranteed to exist beyond the lifetime of the parse operation. We must copy it. --- src/cli/args.zig | 13 ++++++++----- src/cli/diagnostics.zig | 4 ++-- src/config/Config.zig | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/cli/args.zig b/src/cli/args.zig index 3e378f347..454ca360e 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -104,7 +104,7 @@ pub fn parse( try dst._diagnostics.append(arena_alloc, .{ .key = try arena_alloc.dupeZ(u8, arg), .message = "invalid field", - .location = diags.Location.fromIter(iter), + .location = try diags.Location.fromIter(iter, arena_alloc), }); continue; @@ -145,7 +145,7 @@ pub fn parse( try dst._diagnostics.append(arena_alloc, .{ .key = try arena_alloc.dupeZ(u8, key), .message = message, - .location = diags.Location.fromIter(iter), + .location = try diags.Location.fromIter(iter, arena_alloc), }); }; } @@ -1140,7 +1140,7 @@ pub fn ArgsIterator(comptime Iterator: type) type { } /// Returns a location for a diagnostic message. - pub fn location(self: *const Self) ?diags.Location { + pub fn location(self: *const Self, _: Allocator) error{}!?diags.Location { return .{ .cli = self.index }; } }; @@ -1262,12 +1262,15 @@ pub fn LineIterator(comptime ReaderType: type) type { } /// Returns a location for a diagnostic message. - pub fn location(self: *const Self) ?diags.Location { + pub fn location( + self: *const Self, + alloc: Allocator, + ) Allocator.Error!?diags.Location { // If we have no filepath then we have no location. if (self.filepath.len == 0) return null; return .{ .file = .{ - .path = self.filepath, + .path = try alloc.dupe(u8, self.filepath), .line = self.line, } }; } diff --git a/src/cli/diagnostics.zig b/src/cli/diagnostics.zig index 8090684fd..40fed3001 100644 --- a/src/cli/diagnostics.zig +++ b/src/cli/diagnostics.zig @@ -56,7 +56,7 @@ pub const Location = union(enum) { pub const Key = @typeInfo(Location).Union.tag_type.?; - pub fn fromIter(iter: anytype) Location { + pub fn fromIter(iter: anytype, alloc: Allocator) Allocator.Error!Location { const Iter = t: { const T = @TypeOf(iter); break :t switch (@typeInfo(T)) { @@ -67,7 +67,7 @@ pub const Location = union(enum) { }; if (!@hasDecl(Iter, "location")) return .none; - return iter.location() orelse .none; + return (try iter.location(alloc)) orelse .none; } pub fn clone(self: *const Location, alloc: Allocator) Allocator.Error!Location { diff --git a/src/config/Config.zig b/src/config/Config.zig index 2dc732752..a3bc79099 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2988,7 +2988,7 @@ pub fn parseManuallyHook( if (command.items.len == 0) { try self._diagnostics.append(alloc, .{ - .location = cli.Location.fromIter(iter), + .location = try cli.Location.fromIter(iter, alloc), .message = try std.fmt.allocPrintZ( alloc, "missing command after {s}",