From e854b38872adc38050c39b6f2e8f580268d1e08c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 23 Jan 2025 14:11:10 -0800 Subject: [PATCH] cli: allow renaming config fields to maintain backwards compatibility Fixes #4631 This introduces a mechanism by which parsed config fields can be renamed to maintain backwards compatibility. This already has a use case -- implemented in this commit -- for `background-blur-radius` to be renamed to `background-blur`. The remapping is comptime-known which lets us do some comptime validation. The remap check isn't done unless no fields match which means for well-formed config files, there's no overhead. For future improvements: - We should update our config help generator to note renamed fields. - We could offer automatic migration of config files be rewriting them. - We can enrich the value type with more metadata to help with config gen or other tooling. --- macos/Sources/Ghostty/Ghostty.Config.swift | 2 +- src/apprt/embedded.zig | 2 +- src/apprt/gtk/winproto/wayland.zig | 2 +- src/apprt/gtk/winproto/x11.zig | 2 +- src/cli/args.zig | 52 ++++++++++++++++++++++ src/config/Config.zig | 13 +++++- src/config/c_get.zig | 12 ++--- 7 files changed, 73 insertions(+), 12 deletions(-) diff --git a/macos/Sources/Ghostty/Ghostty.Config.swift b/macos/Sources/Ghostty/Ghostty.Config.swift index 2a24b0257..9c8042c63 100644 --- a/macos/Sources/Ghostty/Ghostty.Config.swift +++ b/macos/Sources/Ghostty/Ghostty.Config.swift @@ -339,7 +339,7 @@ extension Ghostty { var backgroundBlurRadius: Int { guard let config = self.config else { return 1 } var v: Int = 0 - let key = "background-blur-radius" + let key = "background-blur" _ = ghostty_config_get(config, &v, key, UInt(key.count)) return v; } diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index 44c4c5f20..890901c07 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -1958,7 +1958,7 @@ pub const CAPI = struct { _ = CGSSetWindowBackgroundBlurRadius( CGSDefaultConnectionForThread(), nswindow.msgSend(usize, objc.sel("windowNumber"), .{}), - @intCast(config.@"background-blur-radius".cval()), + @intCast(config.@"background-blur".cval()), ); } diff --git a/src/apprt/gtk/winproto/wayland.zig b/src/apprt/gtk/winproto/wayland.zig index 7a28fc92c..8df3e57b3 100644 --- a/src/apprt/gtk/winproto/wayland.zig +++ b/src/apprt/gtk/winproto/wayland.zig @@ -176,7 +176,7 @@ pub const Window = struct { pub fn init(config: *const Config) DerivedConfig { return .{ - .blur = config.@"background-blur-radius".enabled(), + .blur = config.@"background-blur".enabled(), .window_decoration = config.@"window-decoration", }; } diff --git a/src/apprt/gtk/winproto/x11.zig b/src/apprt/gtk/winproto/x11.zig index 4f607d1ef..7a6b8b4c7 100644 --- a/src/apprt/gtk/winproto/x11.zig +++ b/src/apprt/gtk/winproto/x11.zig @@ -165,7 +165,7 @@ pub const Window = struct { pub fn init(config: *const Config) DerivedConfig { return .{ - .blur = config.@"background-blur-radius".enabled(), + .blur = config.@"background-blur".enabled(), .has_decoration = switch (config.@"window-decoration") { .none => false, .auto, .client, .server => true, diff --git a/src/cli/args.zig b/src/cli/args.zig index 23dcf7733..166b2daf5 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -38,6 +38,12 @@ pub const Error = error{ /// "DiagnosticList" and any diagnostic messages will be added to that list. /// When diagnostics are present, only allocation errors will be returned. /// +/// If the destination type has a decl "renamed", it must be of type +/// std.StaticStringMap([]const u8) and contains a mapping from the old +/// field name to the new field name. This is used to allow renaming fields +/// while still supporting the old name. If a renamed field is set, parsing +/// will automatically set the new field name. +/// /// Note: If the arena is already non-null, then it will be used. In this /// case, in the case of an error some memory might be leaked into the arena. pub fn parse( @@ -49,6 +55,24 @@ pub fn parse( const info = @typeInfo(T); assert(info == .Struct); + comptime { + // Verify all renamed fields are valid (source does not exist, + // destination does exist). + if (@hasDecl(T, "renamed")) { + for (T.renamed.keys(), T.renamed.values()) |key, value| { + if (@hasField(T, key)) { + @compileLog(key); + @compileError("renamed field source exists"); + } + + if (!@hasField(T, value)) { + @compileLog(value); + @compileError("renamed field destination does not exist"); + } + } + } + } + // Make an arena for all our allocations if we support it. Otherwise, // use an allocator that always fails. If the arena is already set on // the config, then we reuse that. See memory note in parse docs. @@ -367,6 +391,16 @@ pub fn parseIntoField( } } + // Unknown field, is the field renamed? + if (@hasDecl(T, "renamed")) { + for (T.renamed.keys(), T.renamed.values()) |old, new| { + if (mem.eql(u8, old, key)) { + try parseIntoField(T, alloc, dst, new, value); + return; + } + } + } + return error.InvalidField; } @@ -1104,6 +1138,24 @@ test "parseIntoField: tagged union missing tag" { ); } +test "parseIntoField: renamed field" { + const testing = std.testing; + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var data: struct { + a: []const u8, + + const renamed = std.StaticStringMap([]const u8).initComptime(&.{ + .{ "old", "a" }, + }); + } = undefined; + + try parseIntoField(@TypeOf(data), alloc, &data, "old", "42"); + try testing.expectEqualStrings("42", data.a); +} + /// An iterator that considers its location to be CLI args. It /// iterates through an underlying iterator and increments a counter /// to track the current CLI arg index. diff --git a/src/config/Config.zig b/src/config/Config.zig index e32a3485f..16e08bf08 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -42,6 +42,15 @@ const c = @cImport({ @cInclude("unistd.h"); }); +/// Renamed fields, used by cli.parse +pub const renamed = std.StaticStringMap([]const u8).initComptime(&.{ + // Ghostty 1.1 introduced background-blur support for Linux which + // doesn't support a specific radius value. The renaming is to let + // one field be used for both platforms (macOS retained the ability + // to set a radius). + .{ "background-blur-radius", "background-blur" }, +}); + /// The font families to use. /// /// You can generate the list of valid values using the CLI: @@ -649,7 +658,7 @@ palette: Palette = .{}, /// need to set environment-specific settings and/or install third-party plugins /// in order to support background blur, as there isn't a unified interface for /// doing so. -@"background-blur-radius": BackgroundBlur = .false, +@"background-blur": BackgroundBlur = .false, /// The opacity level (opposite of transparency) of an unfocused split. /// Unfocused splits by default are slightly faded out to make it easier to see @@ -5854,7 +5863,7 @@ pub const AutoUpdate = enum { download, }; -/// See background-blur-radius +/// See background-blur pub const BackgroundBlur = union(enum) { false, true, diff --git a/src/config/c_get.zig b/src/config/c_get.zig index 6804b0ae0..251a95e77 100644 --- a/src/config/c_get.zig +++ b/src/config/c_get.zig @@ -192,21 +192,21 @@ test "c_get: background-blur" { defer c.deinit(); { - c.@"background-blur-radius" = .false; + c.@"background-blur" = .false; var cval: u8 = undefined; - try testing.expect(get(&c, .@"background-blur-radius", @ptrCast(&cval))); + try testing.expect(get(&c, .@"background-blur", @ptrCast(&cval))); try testing.expectEqual(0, cval); } { - c.@"background-blur-radius" = .true; + c.@"background-blur" = .true; var cval: u8 = undefined; - try testing.expect(get(&c, .@"background-blur-radius", @ptrCast(&cval))); + try testing.expect(get(&c, .@"background-blur", @ptrCast(&cval))); try testing.expectEqual(20, cval); } { - c.@"background-blur-radius" = .{ .radius = 42 }; + c.@"background-blur" = .{ .radius = 42 }; var cval: u8 = undefined; - try testing.expect(get(&c, .@"background-blur-radius", @ptrCast(&cval))); + try testing.expect(get(&c, .@"background-blur", @ptrCast(&cval))); try testing.expectEqual(42, cval); } }