From 712da4288f214ff1267d34927025e923a884439f Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 18 Nov 2024 14:37:28 -0800 Subject: [PATCH 01/15] config: add basic conditional system core logic (no syntax yet) Note: this doesn't have any syntax the user can use in a configuration yet. This just implements a core, tested system. --- src/config.zig | 1 + src/config/conditional.zig | 84 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 src/config/conditional.zig diff --git a/src/config.zig b/src/config.zig index 5af7832dd..02268d4e3 100644 --- a/src/config.zig +++ b/src/config.zig @@ -2,6 +2,7 @@ const builtin = @import("builtin"); const formatter = @import("config/formatter.zig"); pub const Config = @import("config/Config.zig"); +pub const conditional = @import("config/conditional.zig"); pub const string = @import("config/string.zig"); pub const edit = @import("config/edit.zig"); pub const url = @import("config/url.zig"); diff --git a/src/config/conditional.zig b/src/config/conditional.zig new file mode 100644 index 000000000..3be6b1fab --- /dev/null +++ b/src/config/conditional.zig @@ -0,0 +1,84 @@ +const std = @import("std"); +const builtin = @import("builtin"); +const assert = std.debug.assert; +const Allocator = std.mem.Allocator; + +/// Conditionals in Ghostty configuration are based on a static, typed +/// state of the world instead of a dynamic key-value set. This simplifies +/// the implementation, allows for better type checking, and enables a +/// typed C API. +pub const State = struct { + /// The theme of the underlying OS desktop environment. + theme: Theme = .light, + + /// The target OS of the current build. + os: std.Target.Os.Tag = builtin.target.os.tag, + + pub const Theme = enum { light, dark }; + + /// Tests the conditional against the state and returns true if it matches. + pub fn match(self: State, cond: Conditional) bool { + switch (cond.key) { + inline else => |tag| { + // The raw value of the state field. + const raw = @field(self, @tagName(tag)); + + // Since all values are enums currently then we can just + // do this. If we introduce non-enum state values then this + // will be a compile error and we should fix here. + const value: []const u8 = @tagName(raw); + + return switch (cond.op) { + .eq => std.mem.eql(u8, value, cond.value), + .ne => !std.mem.eql(u8, value, cond.value), + }; + }, + } + } +}; + +/// An enum of the available conditional configuration keys. +pub const Key = key: { + const stateInfo = @typeInfo(State).Struct; + var fields: [stateInfo.fields.len]std.builtin.Type.EnumField = undefined; + for (stateInfo.fields, 0..) |field, i| fields[i] = .{ + .name = field.name, + .value = i, + }; + + break :key @Type(.{ .Enum = .{ + .tag_type = std.math.IntFittingRange(0, fields.len - 1), + .fields = &fields, + .decls = &.{}, + .is_exhaustive = true, + } }); +}; + +/// A single conditional that can be true or false. +pub const Conditional = struct { + key: Key, + op: Op, + value: []const u8, + + pub const Op = enum { eq, ne }; +}; + +test "conditional enum match" { + const testing = std.testing; + const state: State = .{ .theme = .dark }; + try testing.expect(state.match(.{ + .key = .theme, + .op = .eq, + .value = "dark", + })); + try testing.expect(!state.match(.{ + .key = .theme, + .op = .ne, + .value = "dark", + })); + try testing.expect(state.match(.{ + .key = .theme, + .op = .ne, + .value = "light", + })); +} From f016c5028c87badec3257afa7dd8cbcf003ae1f1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 18 Nov 2024 14:48:40 -0800 Subject: [PATCH 02/15] config: Replay.Step supports a conditional arg --- src/config/Config.zig | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 6e569e795..75bf0225b 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -23,6 +23,8 @@ const internal_os = @import("../os/main.zig"); const cli = @import("../cli.zig"); const Command = @import("../Command.zig"); +const conditional = @import("conditional.zig"); +const Conditional = conditional.Conditional; const formatterpkg = @import("formatter.zig"); const themepkg = @import("theme.zig"); const url = @import("url.zig"); @@ -1743,6 +1745,10 @@ _arena: ?ArenaAllocator = null, /// the configuration. _diagnostics: cli.DiagnosticList = .{}, +/// The conditional truths for the configuration. This is used to +/// determine if a conditional configuration matches or not. +_conditional_state: conditional.State = .{}, + /// 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. @@ -3127,6 +3133,14 @@ const Replay = struct { /// A base path to expand relative paths against. expand: []const u8, + + /// A conditional argument. This arg is parsed only if all + /// conditions match (an "AND"). An "OR" can be achieved by + /// having multiple conditional arg entries. + conditional_arg: struct { + conditions: []const Conditional, + arg: []const u8, + }, }; const Iterator = struct { @@ -3141,7 +3155,6 @@ const Replay = struct { if (self.idx >= self.slice.len) return null; defer self.idx += 1; switch (self.slice[self.idx]) { - .arg => |arg| return arg, .expand => |base| self.config.expandPaths(base) catch |err| { // This shouldn't happen because to reach this step // means that it succeeded before. Its possible since @@ -3150,6 +3163,19 @@ const Replay = struct { // In that really unfortunate case, we log a warning. log.warn("error expanding paths err={}", .{err}); }, + + .arg => |arg| return arg, + + .conditional_arg => |v| conditional: { + // All conditions must match. + for (v.conditions) |cond| { + if (!self.config._conditional_state.match(cond)) { + break :conditional; + } + } + + return v.arg; + }, } } } From 234e3986f9ec8656dcdffb25caaf17c1a6b93b2c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 18 Nov 2024 15:00:06 -0800 Subject: [PATCH 03/15] config: function to change conditional state --- src/config/Config.zig | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/config/Config.zig b/src/config/Config.zig index 75bf0225b..31199429e 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2560,6 +2560,36 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { } } +/// Change the state of conditionals and reload the configuration +/// based on the new state. This returns a new configuration based +/// on the new state. The caller must free the old configuration if they +/// wish. +/// +/// 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 +/// deleted a file that was referenced in the configuration, then the +/// configuration can still be reloaded. +/// TODO: totally untested +pub fn changeConditionalState( + self: *Config, + new: conditional.State, +) !Config { + // Create our new configuration + const alloc_gpa = self._arena.?.child_allocator; + var new_config = try default(alloc_gpa); + errdefer new_config.deinit(); + + // Set our conditional state so the replay below can use it + new_config._conditional_state = new; + + // 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); + + return new_config; +} + /// Expand the relative paths in config-files to be absolute paths /// relative to the base directory. fn expandPaths(self: *Config, base: []const u8) !void { From 04a61e753aae494cf4785ae4dcfc0d80ccfe219a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 18 Nov 2024 16:46:49 -0800 Subject: [PATCH 04/15] config: some docs updates --- src/config/Config.zig | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 31199429e..ea50210a2 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -334,6 +334,18 @@ const c = @cImport({ /// /// To see a list of available themes, run `ghostty +list-themes`. /// +/// A theme file is simply another Ghostty configuration file. They share +/// the same syntax and same configuration options. A theme can set any valid +/// configuration option so please do not use a theme file from an untrusted +/// source. The built-in themes are audited to only set safe configuration +/// options. +/// +/// Some options cannot be set within theme files. The reason these are not +/// supported should be self-evident. A theme file cannot set `theme` or +/// `config-file`. At the time of writing this, Ghostty will not show any +/// warnings or errors if you set these options in a theme file but they will +/// be silently ignored. +/// /// Any additional colors specified via background, foreground, palette, etc. /// will override the colors specified in the theme. theme: ?[]const u8 = null, @@ -2634,7 +2646,8 @@ fn loadTheme(self: *Config, theme: []const u8) !void { // (2) We want to free existing memory that we aren't using anymore // as a result of reloading the configuration. // - // Point 2 is strictly a result of aur approach to point 1. + // Point 2 is strictly a result of aur approach to point 1, but it is + // a nice property to have to limit memory bloat as much as possible. // Keep track of our replay length prior to loading the theme // so that we can replay the previous config to override values. From df4e616e71e09e4c193a2c6f4d52662601833666 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 18 Nov 2024 19:19:06 -0800 Subject: [PATCH 05/15] config: theme loading unit tests --- src/config/Config.zig | 229 ++++++++++++++++++++----------- src/config/testdata/theme_simple | 2 + src/config/theme.zig | 1 - 3 files changed, 153 insertions(+), 79 deletions(-) create mode 100644 src/config/testdata/theme_simple diff --git a/src/config/Config.zig b/src/config/Config.zig index ea50210a2..de0df92f5 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2675,6 +2675,8 @@ fn loadTheme(self: *Config, theme: []const u8) !void { self.* = new_config; } +/// Call this once after you are done setting configuration. This +/// is idempotent but will waste memory if called multiple times. pub fn finalize(self: *Config) !void { const alloc = self._arena.?.allocator(); @@ -2720,7 +2722,9 @@ pub fn finalize(self: *Config) !void { // to look up defaults which is kind of expensive. We only do this // on desktop. const wd_home = std.mem.eql(u8, "home", wd); - if (comptime !builtin.target.isWasm()) { + if ((comptime !builtin.target.isWasm()) and + (comptime !builtin.is_test)) + { if (self.command == null or wd_home) command: { // First look up the command using the SHELL env var if needed. // We don't do this in flatpak because SHELL in Flatpak is always @@ -2790,7 +2794,9 @@ pub fn finalize(self: *Config) !void { if (std.mem.eql(u8, wd, "inherit")) self.@"working-directory" = null; // Default our click interval - if (self.@"click-repeat-interval" == 0) { + if (self.@"click-repeat-interval" == 0 and + (comptime !builtin.is_test)) + { self.@"click-repeat-interval" = internal_os.clickInterval() orelse 500; } @@ -3019,82 +3025,6 @@ pub const ChangeIterator = struct { } }; -const TestIterator = struct { - data: []const []const u8, - i: usize = 0, - - pub fn next(self: *TestIterator) ?[]const u8 { - if (self.i >= self.data.len) return null; - const result = self.data[self.i]; - self.i += 1; - return result; - } -}; - -test "parse hook: invalid command" { - const testing = std.testing; - var cfg = try Config.default(testing.allocator); - defer cfg.deinit(); - const alloc = cfg._arena.?.allocator(); - - var it: TestIterator = .{ .data = &.{"foo"} }; - try testing.expect(try cfg.parseManuallyHook(alloc, "--command", &it)); - try testing.expect(cfg.command == null); -} - -test "parse e: command only" { - const testing = std.testing; - var cfg = try Config.default(testing.allocator); - defer cfg.deinit(); - const alloc = cfg._arena.?.allocator(); - - var it: TestIterator = .{ .data = &.{"foo"} }; - try testing.expect(!try cfg.parseManuallyHook(alloc, "-e", &it)); - try testing.expectEqualStrings("foo", cfg.@"initial-command".?); -} - -test "parse e: command and args" { - const testing = std.testing; - var cfg = try Config.default(testing.allocator); - defer cfg.deinit(); - const alloc = cfg._arena.?.allocator(); - - var it: TestIterator = .{ .data = &.{ "echo", "foo", "bar baz" } }; - try testing.expect(!try cfg.parseManuallyHook(alloc, "-e", &it)); - try testing.expectEqualStrings("echo foo bar baz", cfg.@"initial-command".?); -} - -test "clone default" { - const testing = std.testing; - const alloc = testing.allocator; - - var source = try Config.default(alloc); - defer source.deinit(); - var dest = try source.clone(alloc); - defer dest.deinit(); - - // Should have no changes - var it = source.changeIterator(&dest); - try testing.expectEqual(@as(?Key, null), it.next()); - - // I want to do this but this doesn't work (the API doesn't work) - // try testing.expectEqualDeep(dest, source); -} - -test "changed" { - const testing = std.testing; - const alloc = testing.allocator; - - var source = try Config.default(alloc); - defer source.deinit(); - var dest = try source.clone(alloc); - defer dest.deinit(); - dest.@"font-thicken" = true; - - try testing.expect(source.changed(&dest, .@"font-thicken")); - try testing.expect(!source.changed(&dest, .@"font-size")); -} - /// A config-specific helper to determine if two values of the same /// type are equal. This isn't the same as std.mem.eql or std.testing.equals /// because we expect structs to implement their own equality. @@ -5024,3 +4954,146 @@ test "test entryFormatter" { try p.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); try std.testing.expectEqualStrings("a = 584y 49w 23h 34m 33s 709ms 551µs 615ns\n", buf.items); } + +const TestIterator = struct { + data: []const []const u8, + i: usize = 0, + + pub fn next(self: *TestIterator) ?[]const u8 { + if (self.i >= self.data.len) return null; + const result = self.data[self.i]; + self.i += 1; + return result; + } +}; + +test "parse hook: invalid command" { + const testing = std.testing; + var cfg = try Config.default(testing.allocator); + defer cfg.deinit(); + const alloc = cfg._arena.?.allocator(); + + var it: TestIterator = .{ .data = &.{"foo"} }; + try testing.expect(try cfg.parseManuallyHook(alloc, "--command", &it)); + try testing.expect(cfg.command == null); +} + +test "parse e: command only" { + const testing = std.testing; + var cfg = try Config.default(testing.allocator); + defer cfg.deinit(); + const alloc = cfg._arena.?.allocator(); + + var it: TestIterator = .{ .data = &.{"foo"} }; + try testing.expect(!try cfg.parseManuallyHook(alloc, "-e", &it)); + try testing.expectEqualStrings("foo", cfg.@"initial-command".?); +} + +test "parse e: command and args" { + const testing = std.testing; + var cfg = try Config.default(testing.allocator); + defer cfg.deinit(); + const alloc = cfg._arena.?.allocator(); + + var it: TestIterator = .{ .data = &.{ "echo", "foo", "bar baz" } }; + try testing.expect(!try cfg.parseManuallyHook(alloc, "-e", &it)); + try testing.expectEqualStrings("echo foo bar baz", cfg.@"initial-command".?); +} + +test "clone default" { + const testing = std.testing; + const alloc = testing.allocator; + + var source = try Config.default(alloc); + defer source.deinit(); + var dest = try source.clone(alloc); + defer dest.deinit(); + + // Should have no changes + var it = source.changeIterator(&dest); + try testing.expectEqual(@as(?Key, null), it.next()); + + // I want to do this but this doesn't work (the API doesn't work) + // try testing.expectEqualDeep(dest, source); +} + +test "changed" { + const testing = std.testing; + const alloc = testing.allocator; + + var source = try Config.default(alloc); + defer source.deinit(); + var dest = try source.clone(alloc); + defer dest.deinit(); + dest.@"font-thicken" = true; + + try testing.expect(source.changed(&dest, .@"font-thicken")); + try testing.expect(!source.changed(&dest, .@"font-size")); +} + +test "theme loading" { + 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", .{}); + defer file.close(); + try file.writer().writeAll(@embedFile("testdata/theme_simple")); + } + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const path = try td.dir.realpath("theme", &path_buf); + + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + try std.fmt.allocPrint(alloc_arena, "--theme={s}", .{path}), + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expectEqual(Color{ + .r = 0x12, + .g = 0x3A, + .b = 0xBC, + }, cfg.background); +} + +test "theme priority is lower than config" { + 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", .{}); + defer file.close(); + try file.writer().writeAll(@embedFile("testdata/theme_simple")); + } + var path_buf: [std.fs.max_path_bytes]u8 = undefined; + const path = try td.dir.realpath("theme", &path_buf); + + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + "--background=#ABCDEF", + try std.fmt.allocPrint(alloc_arena, "--theme={s}", .{path}), + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expectEqual(Color{ + .r = 0xAB, + .g = 0xCD, + .b = 0xEF, + }, cfg.background); +} diff --git a/src/config/testdata/theme_simple b/src/config/testdata/theme_simple new file mode 100644 index 000000000..def333740 --- /dev/null +++ b/src/config/testdata/theme_simple @@ -0,0 +1,2 @@ +# A simple theme +background = #123ABC diff --git a/src/config/theme.zig b/src/config/theme.zig index 4616d6363..b851ec3d4 100644 --- a/src/config/theme.zig +++ b/src/config/theme.zig @@ -112,7 +112,6 @@ pub fn open( path: []const u8, file: std.fs.File, } { - // Absolute themes are loaded a different path. if (std.fs.path.isAbsolute(theme)) { const file: std.fs.File = try openAbsolute( From d2cdc4f717b6169a885185868c12eeba3209c642 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 10:46:11 -0800 Subject: [PATCH 06/15] cli: parse auto structs --- src/cli/args.zig | 108 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 2 deletions(-) diff --git a/src/cli/args.zig b/src/cli/args.zig index 5fdaf6d8b..e26ea9759 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -310,8 +310,9 @@ fn parseIntoField( value orelse return error.ValueRequired, ) orelse return error.InvalidValue, - .Struct => try parsePackedStruct( + .Struct => try parseStruct( Field, + alloc, value orelse return error.ValueRequired, ), @@ -378,9 +379,79 @@ fn parseTaggedUnion(comptime T: type, alloc: Allocator, v: []const u8) !T { return error.InvalidValue; } +fn parseStruct(comptime T: type, alloc: Allocator, v: []const u8) !T { + return switch (@typeInfo(T).Struct.layout) { + .auto => parseAutoStruct(T, alloc, v), + .@"packed" => parsePackedStruct(T, v), + else => @compileError("unsupported struct layout"), + }; +} + +fn parseAutoStruct(comptime T: type, alloc: Allocator, v: []const u8) !T { + const info = @typeInfo(T).Struct; + comptime assert(info.layout == .auto); + + // We start our result as undefined so we don't get an error for required + // fields. We track required fields below and we validate that we set them + // all at the bottom of this function (in addition to setting defaults for + // optionals). + var result: T = undefined; + + // Keep track of which fields were set so we can error if a required + // field was not set. + const FieldSet = std.StaticBitSet(info.fields.len); + var fields_set: FieldSet = FieldSet.initEmpty(); + + // We split each value by "," + var iter = std.mem.splitSequence(u8, v, ","); + loop: while (iter.next()) |entry| { + // Find the key/value, trimming whitespace. The value may be quoted + // which we strip the quotes from. + const idx = mem.indexOf(u8, entry, ":") orelse return error.InvalidValue; + const key = std.mem.trim(u8, entry[0..idx], whitespace); + const value = value: { + var value = std.mem.trim(u8, entry[idx + 1 ..], whitespace); + + // Detect a quoted string. + if (value.len >= 2 and + value[0] == '"' and + value[value.len - 1] == '"') + { + // Trim quotes since our CLI args processor expects + // quotes to already be gone. + value = value[1 .. value.len - 1]; + } + + break :value value; + }; + + inline for (info.fields, 0..) |field, i| { + if (std.mem.eql(u8, field.name, key)) { + try parseIntoField(T, alloc, &result, key, value); + fields_set.set(i); + continue :loop; + } + } + + // No field matched + return error.InvalidValue; + } + + // Ensure all required fields are set + inline for (info.fields, 0..) |field, i| { + if (!fields_set.isSet(i)) { + const default_ptr = field.default_value orelse return error.InvalidValue; + const typed_ptr: *const field.type = @alignCast(@ptrCast(default_ptr)); + @field(result, field.name) = typed_ptr.*; + } + } + + return result; +} + fn parsePackedStruct(comptime T: type, v: []const u8) !T { const info = @typeInfo(T).Struct; - assert(info.layout == .@"packed"); + comptime assert(info.layout == .@"packed"); var result: T = .{}; @@ -847,6 +918,39 @@ test "parseIntoField: struct with parse func" { try testing.expectEqual(@as([]const u8, "HELLO!"), data.a.v); } +test "parseIntoField: struct with basic fields" { + const testing = std.testing; + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var data: struct { + value: struct { + a: []const u8, + b: u32, + c: u8 = 12, + } = undefined, + } = .{}; + + // Set required fields + try parseIntoField(@TypeOf(data), alloc, &data, "value", "a:hello,b:42"); + try testing.expectEqualStrings("hello", data.value.a); + try testing.expectEqual(42, data.value.b); + try testing.expectEqual(12, data.value.c); + + // Set all fields + try parseIntoField(@TypeOf(data), alloc, &data, "value", "a:world,b:84,c:24"); + try testing.expectEqualStrings("world", data.value.a); + try testing.expectEqual(84, data.value.b); + try testing.expectEqual(24, data.value.c); + + // Missing require dfield + try testing.expectError( + error.InvalidValue, + parseIntoField(@TypeOf(data), alloc, &data, "value", "a:hello"), + ); +} + test "parseIntoField: tagged union" { const testing = std.testing; var arena = ArenaAllocator.init(testing.allocator); From 7d2dee2bc30723fa2fd647417f05b6fc3381fc7b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 11:05:11 -0800 Subject: [PATCH 07/15] cli: parseCLI form works with optionals --- src/cli/args.zig | 55 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/src/cli/args.zig b/src/cli/args.zig index e26ea9759..076137dd5 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -13,7 +13,7 @@ const DiagnosticList = diags.DiagnosticList; // `--long value`? Not currently allowed. // For trimming -const whitespace = " \t"; +pub const whitespace = " \t"; /// The base errors for arg parsing. Additional errors can be returned due /// to type-specific parsing but these are always possible. @@ -209,7 +209,7 @@ fn canTrackDiags(comptime T: type) bool { /// This may result in allocations. The allocations can only be freed by freeing /// all the memory associated with alloc. It is expected that alloc points to /// an arena. -fn parseIntoField( +pub fn parseIntoField( comptime T: type, alloc: Allocator, dst: *T, @@ -250,10 +250,32 @@ fn parseIntoField( 1 => @field(dst, field.name) = try Field.parseCLI(value), // 2 arg = (self, input) => void - 2 => try @field(dst, field.name).parseCLI(value), + 2 => switch (@typeInfo(field.type)) { + .Struct, + .Union, + .Enum, + => try @field(dst, field.name).parseCLI(value), + + .Optional => { + @field(dst, field.name) = undefined; + try @field(dst, field.name).?.parseCLI(value); + }, + else => @compileError("unexpected field type"), + }, // 3 arg = (self, alloc, input) => void - 3 => try @field(dst, field.name).parseCLI(alloc, value), + 3 => switch (@typeInfo(field.type)) { + .Struct, + .Union, + .Enum, + => try @field(dst, field.name).parseCLI(alloc, value), + + .Optional => { + @field(dst, field.name) = undefined; + try @field(dst, field.name).?.parseCLI(alloc, value); + }, + else => @compileError("unexpected field type"), + }, else => @compileError("parseCLI invalid argument count"), } @@ -387,7 +409,7 @@ fn parseStruct(comptime T: type, alloc: Allocator, v: []const u8) !T { }; } -fn parseAutoStruct(comptime T: type, alloc: Allocator, v: []const u8) !T { +pub fn parseAutoStruct(comptime T: type, alloc: Allocator, v: []const u8) !T { const info = @typeInfo(T).Struct; comptime assert(info.layout == .auto); @@ -918,6 +940,29 @@ test "parseIntoField: struct with parse func" { try testing.expectEqual(@as([]const u8, "HELLO!"), data.a.v); } +test "parseIntoField: optional struct with parse func" { + const testing = std.testing; + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var data: struct { + a: ?struct { + const Self = @This(); + + v: []const u8, + + pub fn parseCLI(self: *Self, _: Allocator, value: ?[]const u8) !void { + _ = value; + self.* = .{ .v = "HELLO!" }; + } + } = null, + } = .{}; + + try parseIntoField(@TypeOf(data), alloc, &data, "a", "42"); + try testing.expectEqual(@as([]const u8, "HELLO!"), data.a.?.v); +} + test "parseIntoField: struct with basic fields" { const testing = std.testing; var arena = ArenaAllocator.init(testing.allocator); From 13d5f37e503a32ba6014bb1ecbdfd6ee21adb445 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 11:05:11 -0800 Subject: [PATCH 08/15] config: theme parses light/dark but only loads light for now --- src/config/Config.zig | 106 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 3 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index de0df92f5..545858036 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -348,7 +348,7 @@ const c = @cImport({ /// /// Any additional colors specified via background, foreground, palette, etc. /// will override the colors specified in the theme. -theme: ?[]const u8 = null, +theme: ?Theme = null, /// Background color for the window. background: Color = .{ .r = 0x28, .g = 0x2C, .b = 0x34 }, @@ -2625,11 +2625,12 @@ fn expandPaths(self: *Config, base: []const u8) !void { } } -fn loadTheme(self: *Config, theme: []const u8) !void { +fn loadTheme(self: *Config, theme: Theme) !void { // Find our theme file and open it. See the open function for details. + // TODO: handle dark const themefile = (try themepkg.open( self._arena.?.allocator(), - theme, + theme.light, &self._diagnostics, )) orelse return; const path = themefile.path; @@ -4626,6 +4627,105 @@ pub const AutoUpdate = enum { download, }; +/// See theme +pub const Theme = struct { + light: []const u8, + dark: []const u8, + + pub fn parseCLI(self: *Theme, alloc: Allocator, input_: ?[]const u8) !void { + const input = input_ orelse return error.ValueRequired; + if (input.len == 0) return error.ValueRequired; + + // If there is a comma, equal sign, or colon, then we assume that + // we're parsing a light/dark mode theme pair. Note that "=" isn't + // actually valid for setting a light/dark mode pair but I anticipate + // it'll be a common typo. + if (std.mem.indexOf(u8, input, ",") != null or + std.mem.indexOf(u8, input, "=") != null or + std.mem.indexOf(u8, input, ":") != null) + { + self.* = try cli.args.parseAutoStruct( + Theme, + alloc, + input, + ); + return; + } + + // Trim our value + const trimmed = std.mem.trim(u8, input, cli.args.whitespace); + + // Set the value to the specified value directly. + self.* = .{ + .light = try alloc.dupeZ(u8, trimmed), + .dark = self.light, + }; + } + + /// Deep copy of the struct. Required by Config. + pub fn clone(self: *const Theme, alloc: Allocator) Allocator.Error!Theme { + return .{ + .light = try alloc.dupeZ(u8, self.light), + .dark = try alloc.dupeZ(u8, self.dark), + }; + } + + /// Used by Formatter + pub fn formatEntry( + self: Theme, + formatter: anytype, + ) !void { + var buf: [4096]u8 = undefined; + if (std.mem.eql(u8, self.light, self.dark)) { + try formatter.formatEntry([]const u8, self.light); + return; + } + + const str = std.fmt.bufPrint(&buf, "light:{s},dark:{s}", .{ + self.light, + self.dark, + }) catch return error.OutOfMemory; + try formatter.formatEntry([]const u8, str); + } + + test "parse Theme" { + const testing = std.testing; + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + // Single + { + var v: Theme = undefined; + try v.parseCLI(alloc, "foo"); + try testing.expectEqualStrings("foo", v.light); + try testing.expectEqualStrings("foo", v.dark); + } + + // Single whitespace + { + var v: Theme = undefined; + try v.parseCLI(alloc, " foo "); + try testing.expectEqualStrings("foo", v.light); + try testing.expectEqualStrings("foo", v.dark); + } + + // Light/dark + { + var v: Theme = undefined; + try v.parseCLI(alloc, " light:foo, dark : bar "); + try testing.expectEqualStrings("foo", v.light); + try testing.expectEqualStrings("bar", v.dark); + } + + var v: Theme = undefined; + try testing.expectError(error.ValueRequired, v.parseCLI(alloc, null)); + try testing.expectError(error.ValueRequired, v.parseCLI(alloc, "")); + try testing.expectError(error.InvalidValue, v.parseCLI(alloc, "light:foo")); + try testing.expectError(error.InvalidValue, v.parseCLI(alloc, "dark:foo")); + } +}; + pub const Duration = struct { /// Duration in nanoseconds duration: u64 = 0, From d2566287e911cadf2fae1c158be053057770968d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 13:33:57 -0800 Subject: [PATCH 09/15] config: load dark/light theme based on conditional state --- src/config/Config.zig | 150 ++++++++++++++++++++++++++++++-- src/config/testdata/theme_dark | 1 + src/config/testdata/theme_light | 1 + 3 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 src/config/testdata/theme_dark create mode 100644 src/config/testdata/theme_light diff --git a/src/config/Config.zig b/src/config/Config.zig index 545858036..fa46ef04d 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2582,7 +2582,6 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { /// that if you change the conditional state and the user in the interim /// deleted a file that was referenced in the configuration, then the /// configuration can still be reloaded. -/// TODO: totally untested pub fn changeConditionalState( self: *Config, new: conditional.State, @@ -2598,6 +2597,7 @@ pub fn changeConditionalState( // 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(); return new_config; } @@ -2626,11 +2626,18 @@ fn expandPaths(self: *Config, base: []const u8) !void { } fn loadTheme(self: *Config, theme: Theme) !void { + // Load the correct theme depending on the conditional state. + // Dark/light themes were programmed prior to conditional configuration + // so when we introduce that we probably want to replace this. + const name: []const u8 = switch (self._conditional_state.theme) { + .light => theme.light, + .dark => theme.dark, + }; + // Find our theme file and open it. See the open function for details. - // TODO: handle dark const themefile = (try themepkg.open( self._arena.?.allocator(), - theme.light, + name, &self._diagnostics, )) orelse return; const path = themefile.path; @@ -2650,10 +2657,6 @@ fn loadTheme(self: *Config, theme: Theme) !void { // Point 2 is strictly a result of aur approach to point 1, but it is // a nice property to have to limit memory bloat as much as possible. - // Keep track of our replay length prior to loading the theme - // so that we can replay the previous config to override values. - const replay_len = self._replay_steps.items.len; - // Load into a new configuration so that we can free the existing memory. const alloc_gpa = self._arena.?.child_allocator; var new_config = try default(alloc_gpa); @@ -2666,9 +2669,44 @@ fn loadTheme(self: *Config, theme: Theme) !void { var iter: Iter = .{ .r = reader, .filepath = path }; try new_config.loadIter(alloc_gpa, &iter); + // Setup our replay to be conditional. + for (new_config._replay_steps.items) |*item| switch (item.*) { + .expand => {}, + + // Change our arg to be conditional on our theme. + .arg => |v| { + const alloc_arena = new_config._arena.?.allocator(); + const conds = try alloc_arena.alloc(Conditional, 1); + conds[0] = .{ + .key = .theme, + .op = .eq, + .value = @tagName(self._conditional_state.theme), + }; + item.* = .{ .conditional_arg = .{ + .conditions = conds, + .arg = v, + } }; + }, + + .conditional_arg => |v| { + const alloc_arena = new_config._arena.?.allocator(); + const conds = try alloc_arena.alloc(Conditional, v.conditions.len + 1); + conds[0] = .{ + .key = .theme, + .op = .eq, + .value = @tagName(self._conditional_state.theme), + }; + @memcpy(conds[1..], v.conditions); + item.* = .{ .conditional_arg = .{ + .conditions = conds, + .arg = v.arg, + } }; + }, + }; + // Replay our previous inputs so that we can override values // from the theme. - var slice_it = Replay.iterator(self._replay_steps.items[0..replay_len], &new_config); + var slice_it = Replay.iterator(self._replay_steps.items, &new_config); try new_config.loadIter(alloc_gpa, &slice_it); // Success, swap our new config in and free the old. @@ -3138,7 +3176,9 @@ const Replay = struct { log.warn("error expanding paths err={}", .{err}); }, - .arg => |arg| return arg, + .arg => |arg| { + return arg; + }, .conditional_arg => |v| conditional: { // All conditions must match. @@ -5197,3 +5237,95 @@ test "theme priority is lower than config" { .b = 0xEF, }, cfg.background); } + +test "theme loading correct light/dark" { + 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); + + // Light + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + try std.fmt.allocPrint( + alloc_arena, + "--theme=light:{s},dark:{s}", + .{ light, dark }, + ), + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expectEqual(Color{ + .r = 0xFF, + .g = 0xFF, + .b = 0xFF, + }, cfg.background); + } + + // Dark + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + cfg._conditional_state = .{ .theme = .dark }; + var it: TestIterator = .{ .data = &.{ + try std.fmt.allocPrint( + alloc_arena, + "--theme=light:{s},dark:{s}", + .{ light, dark }, + ), + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + try testing.expectEqual(Color{ + .r = 0xEE, + .g = 0xEE, + .b = 0xEE, + }, cfg.background); + } + + // Light to Dark + { + var cfg = try Config.default(alloc); + defer cfg.deinit(); + var it: TestIterator = .{ .data = &.{ + try std.fmt.allocPrint( + alloc_arena, + "--theme=light:{s},dark:{s}", + .{ light, dark }, + ), + } }; + try cfg.loadIter(alloc, &it); + try cfg.finalize(); + + var new = try cfg.changeConditionalState(.{ .theme = .dark }); + defer new.deinit(); + try testing.expectEqual(Color{ + .r = 0xEE, + .g = 0xEE, + .b = 0xEE, + }, new.background); + } +} diff --git a/src/config/testdata/theme_dark b/src/config/testdata/theme_dark new file mode 100644 index 000000000..e07a5ad28 --- /dev/null +++ b/src/config/testdata/theme_dark @@ -0,0 +1 @@ +background = #EEEEEE diff --git a/src/config/testdata/theme_light b/src/config/testdata/theme_light new file mode 100644 index 000000000..3b3115081 --- /dev/null +++ b/src/config/testdata/theme_light @@ -0,0 +1 @@ +background = #FFFFFF From b7f1eaa14568bab988ea135dec98dc005b88ddbf Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 15:35:42 -0800 Subject: [PATCH 10/15] apprt: action to change conditional state, implement for embedded --- include/ghostty.h | 1 + src/Surface.zig | 51 +++++++++++++++++++++++++++---------- src/apprt/action.zig | 8 ++++++ src/apprt/embedded.zig | 57 ++++++++++++++++++++++++++++++++++-------- src/apprt/glfw.zig | 1 + src/apprt/gtk/App.zig | 1 + src/config.zig | 1 + src/config/Config.zig | 2 +- 8 files changed, 97 insertions(+), 25 deletions(-) diff --git a/include/ghostty.h b/include/ghostty.h index 41e3b3fe2..d0426e995 100644 --- a/include/ghostty.h +++ b/include/ghostty.h @@ -567,6 +567,7 @@ typedef enum { GHOSTTY_ACTION_SECURE_INPUT, GHOSTTY_ACTION_KEY_SEQUENCE, GHOSTTY_ACTION_COLOR_CHANGE, + GHOSTTY_ACTION_CONFIG_CHANGE_CONDITIONAL_STATE, } ghostty_action_tag_e; typedef union { diff --git a/src/Surface.zig b/src/Surface.zig index fbb589638..27a3fb5a8 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -94,11 +94,6 @@ keyboard: Keyboard, /// less important. pressed_key: ?input.KeyEvent = null, -/// The current color scheme of the GUI element containing this surface. -/// This will default to light until the apprt sends us the actual color -/// scheme. This is used by mode 3031 and CSI 996 n. -color_scheme: apprt.ColorScheme = .light, - /// The hash value of the last keybinding trigger that we performed. This /// is only set if the last key input matched a keybinding, consumed it, /// and performed it. This is used to prevent sending release/repeat events @@ -121,6 +116,12 @@ size: renderer.Size, /// the lifetime of. This makes updating config at runtime easier. config: DerivedConfig, +/// The conditional state of the configuration. This can affect +/// how certain configurations take effect such as light/dark mode. +/// This is managed completely by Ghostty core but an apprt action +/// is sent whenever this changes. +config_conditional_state: configpkg.ConditionalState, + /// This is set to true if our IO thread notifies us our child exited. /// This is used to determine if we need to confirm, hold open, etc. child_exited: bool = false, @@ -480,6 +481,7 @@ pub fn init( .io_thr = undefined, .size = size, .config = derived_config, + .config_conditional_state = .{}, }; // The command we're going to execute @@ -777,7 +779,7 @@ pub fn needsConfirmQuit(self: *Surface) bool { /// surface. pub fn handleMessage(self: *Surface, msg: Message) !void { switch (msg) { - .change_config => |config| try self.changeConfig(config), + .change_config => |config| try self.updateConfig(config), .set_title => |*v| { // We ignore the message in case the title was set via config. @@ -935,7 +937,7 @@ fn passwordInput(self: *Surface, v: bool) !void { /// Sends a DSR response for the current color scheme to the pty. fn reportColorScheme(self: *Surface) !void { - const output = switch (self.color_scheme) { + const output = switch (self.config_conditional_state.theme) { .light => "\x1B[?997;2n", .dark => "\x1B[?997;1n", }; @@ -1058,8 +1060,25 @@ fn updateRendererHealth(self: *Surface, health: renderer.Health) void { }; } -/// Update our configuration at runtime. -fn changeConfig(self: *Surface, config: *const configpkg.Config) !void { +/// This should be called anytime `config_conditional_state` changes +/// so that the apprt can reload the configuration. +fn notifyConfigConditionalState(self: *Surface) void { + self.rt_app.performAction( + .{ .surface = self }, + .config_change_conditional_state, + {}, + ) catch |err| { + log.warn("failed to notify app of config state change err={}", .{err}); + }; +} + +/// Update our configuration at runtime. This can be called by the apprt +/// to set a surface-specific configuration that differs from the app +/// or other surfaces. +pub fn updateConfig( + self: *Surface, + config: *const configpkg.Config, +) !void { // Update our new derived config immediately const derived = DerivedConfig.init(self.alloc, config) catch |err| { // If the derivation fails then we just log and return. We don't @@ -3590,11 +3609,17 @@ pub fn colorSchemeCallback(self: *Surface, scheme: apprt.ColorScheme) !void { crash.sentry.thread_state = self.crashThreadState(); defer crash.sentry.thread_state = null; - // If our scheme didn't change, then we don't do anything. - if (self.color_scheme == scheme) return; + const new_scheme: configpkg.ConditionalState.Theme = switch (scheme) { + .light => .light, + .dark => .dark, + }; - // Set our new scheme - self.color_scheme = scheme; + // 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; + self.notifyConfigConditionalState(); // If mode 2031 is on, then we report the change live. const report = report: { diff --git a/src/apprt/action.zig b/src/apprt/action.zig index 1f954c37c..aef6937a8 100644 --- a/src/apprt/action.zig +++ b/src/apprt/action.zig @@ -193,6 +193,13 @@ pub const Action = union(Key) { /// such as OSC 10/11. color_change: ColorChange, + /// The state of conditionals in the configuration has changed, so + /// the configuration should be reloaded. The apprt doesn't need + /// to do a full physical reload; it should call the + /// `changeConditionalState` function and then `updateConfig` + /// on the app or surface. + config_change_conditional_state, + /// Sync with: ghostty_action_tag_e pub const Key = enum(c_int) { new_window, @@ -228,6 +235,7 @@ pub const Action = union(Key) { secure_input, key_sequence, color_change, + config_change_conditional_state, }; /// Sync with: ghostty_action_u diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index 877bddf0d..5288c4a7b 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -430,6 +430,28 @@ pub const App = struct { comptime action: apprt.Action.Key, value: apprt.Action.Value(action), ) !void { + // Special case certain actions before they are sent to the + // embedded apprt. + self.performPreAction(target, action, value); + + log.debug("dispatching action target={s} action={} value={}", .{ + @tagName(target), + action, + value, + }); + self.opts.action( + self, + target.cval(), + @unionInit(apprt.Action, @tagName(action), value).cval(), + ); + } + + fn performPreAction( + self: *App, + target: apprt.Target, + comptime action: apprt.Action.Key, + value: apprt.Action.Value(action), + ) void { // Special case certain actions before they are sent to the embedder switch (action) { .set_title => switch (target) { @@ -443,19 +465,32 @@ pub const App = struct { }, }, + .config_change_conditional_state => switch (target) { + .app => {}, + .surface => |surface| action: { + // Build our new configuration. We can free the memory + // immediately after because the surface will derive any + // values it needs to. + var new_config = self.config.changeConditionalState( + surface.config_conditional_state, + ) catch |err| { + // Not a big deal if we error... we just don't update + // the config. We log the error and move on. + log.warn("error changing config conditional state err={}", .{err}); + break :action; + }; + defer new_config.deinit(); + + // Update our surface. + surface.updateConfig(&new_config) catch |err| { + log.warn("error updating surface config for state change err={}", .{err}); + break :action; + }; + }, + }, + else => {}, } - - log.debug("dispatching action target={s} action={} value={}", .{ - @tagName(target), - action, - value, - }); - self.opts.action( - self, - target.cval(), - @unionInit(apprt.Action, @tagName(action), value).cval(), - ); } }; diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index 54c53139c..3c866a1de 100644 --- a/src/apprt/glfw.zig +++ b/src/apprt/glfw.zig @@ -225,6 +225,7 @@ pub const App = struct { .renderer_health, .color_change, .pwd, + .config_change_conditional_state, => log.info("unimplemented action={}", .{action}), } } diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index b00d65bce..8815ef39f 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -487,6 +487,7 @@ pub fn performAction( .render_inspector, .renderer_health, .color_change, + .config_change_conditional_state, => log.warn("unimplemented action={}", .{action}), } } diff --git a/src/config.zig b/src/config.zig index 02268d4e3..b7e818f8e 100644 --- a/src/config.zig +++ b/src/config.zig @@ -7,6 +7,7 @@ pub const string = @import("config/string.zig"); pub const edit = @import("config/edit.zig"); pub const url = @import("config/url.zig"); +pub const ConditionalState = conditional.State; pub const FileFormatter = formatter.FileFormatter; pub const entryFormatter = formatter.entryFormatter; pub const formatEntry = formatter.formatEntry; diff --git a/src/config/Config.zig b/src/config/Config.zig index fa46ef04d..eb7346c97 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2583,7 +2583,7 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { /// deleted a file that was referenced in the configuration, then the /// configuration can still be reloaded. pub fn changeConditionalState( - self: *Config, + self: *const Config, new: conditional.State, ) !Config { // Create our new configuration From 0e006dbd8dfdda84e0131fda4b07a58405a4afef Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 15:51:22 -0800 Subject: [PATCH 11/15] config: disable window-theme=auto if light/dark mode theme is configured --- src/config/Config.zig | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index eb7346c97..634a1c624 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -970,7 +970,9 @@ keybind: Keybinds = .{}, /// The theme to use for the windows. Valid values: /// /// * `auto` - Determine the theme based on the configured terminal -/// background color. +/// background color. This has no effect if the "theme" configuration +/// has separate light and dark themes. In that case, the behavior +/// of "auto" is equivalent to "system". /// * `system` - Use the system theme. /// * `light` - Use the light theme regardless of system theme. /// * `dark` - Use the dark theme regardless of system theme. @@ -2721,7 +2723,17 @@ pub fn finalize(self: *Config) !void { // We always load the theme first because it may set other fields // in our config. - if (self.theme) |theme| try self.loadTheme(theme); + if (self.theme) |theme| { + try self.loadTheme(theme); + + // If we have different light vs dark mode themes, disable + // window-theme = auto since that breaks it. + if (self.@"window-theme" == .auto and + !std.mem.eql(u8, theme.light, theme.dark)) + { + self.@"window-theme" = .system; + } + } // If we have a font-family set and don't set the others, default // the others to the font family. This way, if someone does From 243b6f8c8dfa3a660b5e13de737d16807eaa8333 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 16:00:16 -0800 Subject: [PATCH 12/15] config: macos-titlebar-style transparent disabled with light/dark mode --- src/config/Config.zig | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 634a1c624..c30f7d4e0 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1518,6 +1518,11 @@ keybind: Keybinds = .{}, /// but its one I think is the most aesthetically pleasing and works in /// most cases. /// +/// BUG: If a separate light/dark mode theme is configured with "theme", +/// then `macos-titlebar-style = transparent` will not work correctly. To +/// avoid ugly titlebars, `macos-titlebar-style` will become `native` if +/// a separate light/dark theme is configured. +/// /// Changing this option at runtime only applies to new windows. @"macos-titlebar-style": MacTitlebarStyle = .transparent, @@ -2728,10 +2733,16 @@ pub fn finalize(self: *Config) !void { // If we have different light vs dark mode themes, disable // window-theme = auto since that breaks it. - if (self.@"window-theme" == .auto and - !std.mem.eql(u8, theme.light, theme.dark)) - { - self.@"window-theme" = .system; + if (!std.mem.eql(u8, theme.light, theme.dark)) { + // 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; + + // This is buggy with different light/dark themes and is noted + // in the documentation. + if (self.@"macos-titlebar-style" == .transparent) { + self.@"macos-titlebar-style" = .native; + } } } From 67d5eaa6affe93b0a2fa818b2e374d0fdb314912 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 16:06:34 -0800 Subject: [PATCH 13/15] config: update some docs --- src/config/Config.zig | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index c30f7d4e0..9ee818b8b 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -312,9 +312,14 @@ const c = @cImport({ /// Example: `hinting`, `no-hinting`, `force-autohint`, `no-force-autohint` @"freetype-load-flags": FreetypeLoadFlags = .{}, -/// A theme to use. If the theme is an absolute pathname, Ghostty will attempt -/// to load that file as a theme. If that file does not exist or is inaccessible, -/// an error will be logged and no other directories will be searched. +/// A theme to use. This can be a built-in theme name, a custom theme +/// name, or an absolute path to a custom theme file. Ghostty also supports +/// specifying a different them to use for light and dark mode. Each +/// option is documented below. +/// +/// If the theme is an absolute pathname, Ghostty will attempt to load that +/// file as a theme. If that file does not exist or is inaccessible, an error +/// will be logged and no other directories will be searched. /// /// If the theme is not an absolute pathname, two different directories will be /// searched for a file name that matches the theme. This is case sensitive on @@ -348,6 +353,20 @@ const c = @cImport({ /// /// Any additional colors specified via background, foreground, palette, etc. /// will override the colors specified in the theme. +/// +/// To specify a different theme for light and dark mode, use the following +/// syntax: `light:theme-name,dark:theme-name`. For example: +/// `light:rose-pine-dawn,dark:rose-pine`. Whitespace around all values are +/// trimmed and order of light and dark does not matter. Both light and dark +/// must be specified in this form. In this form, the theme used will be +/// based on the current desktop environment theme. +/// +/// There are some known bugs with light/dark mode theming. These will +/// be fixed in a future update: +/// +/// - macOS: titlebar tabs style is not updated when switching themes. +/// - macOS: native titlebar style is not supported. +/// theme: ?Theme = null, /// Background color for the window. From 03b60ab2eff7df452e23320ebfd77d7ee39ef01d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 19 Nov 2024 16:11:54 -0800 Subject: [PATCH 14/15] apprt/gtk: support light/dark mode change on the fly --- src/apprt/gtk/App.zig | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/src/apprt/gtk/App.zig b/src/apprt/gtk/App.zig index 8815ef39f..99148fd87 100644 --- a/src/apprt/gtk/App.zig +++ b/src/apprt/gtk/App.zig @@ -462,6 +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_conditional_state => self.configChangeConditionalState(target), .inspector => self.controlInspector(target, value), .desktop_notification => self.showDesktopNotification(target, value), .set_title => try self.setTitle(target, value), @@ -487,7 +488,6 @@ pub fn performAction( .render_inspector, .renderer_health, .color_change, - .config_change_conditional_state, => log.warn("unimplemented action={}", .{action}), } } @@ -817,6 +817,35 @@ fn showDesktopNotification( c.g_application_send_notification(g_app, n.body.ptr, notification); } +fn configChangeConditionalState( + self: *App, + target: apprt.Target, +) void { + const surface: *CoreSurface = switch (target) { + .app => return, + .surface => |v| v, + }; + + // Build our new configuration. We can free the memory + // immediately after because the surface will derive any + // values it needs to. + var new_config = self.config.changeConditionalState( + surface.config_conditional_state, + ) catch |err| { + // Not a big deal if we error... we just don't update + // the config. We log the error and move on. + log.warn("error changing config conditional state err={}", .{err}); + return; + }; + defer new_config.deinit(); + + // Update our surface. + surface.updateConfig(&new_config) catch |err| { + log.warn("error updating surface config for state change err={}", .{err}); + return; + }; +} + /// Reload the configuration. This should return the new configuration. /// The old value can be freed immediately at this point assuming a /// successful return. From f8a8b0464cb280dfde53c036e85e5c98225b487b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 20 Nov 2024 14:07:28 -0800 Subject: [PATCH 15/15] renderer/opengl: fix memory leak when copying font features --- src/config/Config.zig | 15 +++++++++++---- src/renderer/Metal.zig | 4 ++-- src/renderer/OpenGL.zig | 4 ++-- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 9ee818b8b..e68ad3da8 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -3478,10 +3478,17 @@ pub const RepeatableString = struct { /// Deep copy of the struct. Required by Config. pub fn clone(self: *const Self, alloc: Allocator) Allocator.Error!Self { // Copy the list and all the strings in the list. - const list = try self.list.clone(alloc); - for (list.items) |*item| { - const copy = try alloc.dupeZ(u8, item.*); - item.* = copy; + var list = try std.ArrayListUnmanaged([:0]const u8).initCapacity( + alloc, + self.list.items.len, + ); + errdefer { + for (list.items) |item| alloc.free(item); + list.deinit(alloc); + } + for (self.list.items) |item| { + const copy = try alloc.dupeZ(u8, item); + list.appendAssumeCapacity(copy); } return .{ .list = list }; diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 7b1b83b8d..fbd6c401b 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -379,7 +379,7 @@ pub const DerivedConfig = struct { const custom_shaders = try config.@"custom-shader".clone(alloc); // Copy our font features - const font_features = try config.@"font-feature".list.clone(alloc); + const font_features = try config.@"font-feature".clone(alloc); // Get our font styles var font_styles = font.CodepointResolver.StyleStatus.initFill(true); @@ -398,7 +398,7 @@ pub const DerivedConfig = struct { return .{ .background_opacity = @max(0, @min(1, config.@"background-opacity")), .font_thicken = config.@"font-thicken", - .font_features = font_features, + .font_features = font_features.list, .font_styles = font_styles, .cursor_color = if (!cursor_invert and config.@"cursor-color" != null) diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 6749645ef..481f5b0db 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -290,7 +290,7 @@ pub const DerivedConfig = struct { const custom_shaders = try config.@"custom-shader".clone(alloc); // Copy our font features - const font_features = try config.@"font-feature".list.clone(alloc); + const font_features = try config.@"font-feature".clone(alloc); // Get our font styles var font_styles = font.CodepointResolver.StyleStatus.initFill(true); @@ -309,7 +309,7 @@ pub const DerivedConfig = struct { return .{ .background_opacity = @max(0, @min(1, config.@"background-opacity")), .font_thicken = config.@"font-thicken", - .font_features = font_features, + .font_features = font_features.list, .font_styles = font_styles, .cursor_color = if (!cursor_invert and config.@"cursor-color" != null)