From e2e12efbbf8331eee5aae1c676065269664f4789 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 10 Dec 2024 21:05:33 -0800 Subject: [PATCH 1/2] keybind: format leader bindings into multiple entries **Context** Currently, if there are multiple keybindings with a shared prefix, they are grouped into a nested series of Binding.Sets. For example, as reported in #2734, the following bindings: keybind = ctrl+z>1=goto_tab:1 keybind = ctrl+z>2=goto_tab:2 keybind = ctrl+z>3=goto_tab:3 Result in roughly the following structure (in pseudo-code): Keybinds{ Trigger("ctrl+z"): Value.leader{ Trigger("1"): Value.leaf{action: "goto_tab:1"}), Trigger("2"): Value.leaf{action: "goto_tab:2"}), Trigger("3"): Value.leaf{action: "goto_tab:3"}), } } When this is formatted into a string (and therefore in +list-keybinds), it is turned into the following as Value.format just concatenates all the sibling bindings ('1', '2', '3') into consecutive bindings, and this is then fed into a single configuration entry: keybind = ctrl+z>1=goto_tab:1>3=goto_tab:3>2=goto_tab:2 **Fix** To fix this, Value needs to produce a separate configuration entry for each sibling binding in the Value.leader case. So we can't produce the entry (formatter.formatEntry) in Keybinds and need to pass information down the Value tree to the leaf nodes, each of which will produce a separate entry with that function. This is accomplished with the help of a new Value.formatEntries method that recursively builds up the prefix for the keybinding, finally flushing it to the formatter when it reaches a leaf node. This is done without extra allocations by using a FixedBufferStream with the same buffer as before, sharing it between calls to nested siblings of the same prefix. **Caveats** We do not track the order in which the bindings were added so the order is not retained in the formatConfig output. Resolves #2734 --- src/config/Config.zig | 61 +++++++++++++++++++++++++++++++++++++------ src/input/Binding.zig | 35 +++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index fa531dc7e..47174aa82 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -4213,14 +4213,9 @@ pub const Keybinds = struct { } } - try formatter.formatEntry( - []const u8, - std.fmt.bufPrint( - &buf, - "{}{}", - .{ k, v }, - ) catch return error.OutOfMemory, - ); + var buffer_stream = std.io.fixedBufferStream(&buf); + try std.fmt.format(buffer_stream.writer(), "{}", .{k}); + try v.formatEntries(&buffer_stream, formatter); } } @@ -4254,6 +4249,56 @@ pub const Keybinds = struct { try list.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); try std.testing.expectEqualSlices(u8, "a = shift+a=csi:hello\n", buf.items); } + + // Regression test for https://github.com/ghostty-org/ghostty/issues/2734 + test "formatConfig multiple items" { + const testing = std.testing; + var buf = std.ArrayList(u8).init(testing.allocator); + defer buf.deinit(); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var list: Keybinds = .{}; + try list.parseCLI(alloc, "ctrl+z>1=goto_tab:1"); + try list.parseCLI(alloc, "ctrl+z>2=goto_tab:2"); + try list.formatEntry(formatterpkg.entryFormatter("keybind", buf.writer())); + + const want = + \\keybind = ctrl+z>1=goto_tab:1 + \\keybind = ctrl+z>2=goto_tab:2 + \\ + ; + try std.testing.expectEqualStrings(want, buf.items); + } + + test "formatConfig multiple items nested" { + const testing = std.testing; + var buf = std.ArrayList(u8).init(testing.allocator); + defer buf.deinit(); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const alloc = arena.allocator(); + + var list: Keybinds = .{}; + try list.parseCLI(alloc, "ctrl+a>ctrl+b>n=new_window"); + try list.parseCLI(alloc, "ctrl+a>ctrl+b>w=close_window"); + try list.parseCLI(alloc, "ctrl+a>ctrl+c>t=new_tab"); + try list.parseCLI(alloc, "ctrl+b>ctrl+d>a=previous_tab"); + try list.formatEntry(formatterpkg.entryFormatter("a", buf.writer())); + + // NB: This does not currently retain the order of the keybinds. + const want = + \\a = ctrl+a>ctrl+b>w=close_window + \\a = ctrl+a>ctrl+b>n=new_window + \\a = ctrl+a>ctrl+c>t=new_tab + \\a = ctrl+b>ctrl+d>a=previous_tab + \\ + ; + try std.testing.expectEqualStrings(want, buf.items); + } }; /// See "font-codepoint-map" for documentation. diff --git a/src/input/Binding.zig b/src/input/Binding.zig index a467bfc2b..ebccac196 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -1149,6 +1149,41 @@ pub const Set = struct { }, } } + + /// Writes the configuration entries for the binding + /// that this value is part of. + /// + /// The value may be part of multiple configuration entries + /// if they're all part of the same prefix sequence (e.g. 'a>b', 'a>c'). + /// These will result in multiple separate entries in the configuration. + /// + /// `buffer_stream` is a FixedBufferStream used for temporary storage + /// that is shared between calls to nested levels of the set. + /// For example, 'a>b>c=x' and 'a>b>d=y' will re-use the 'a>b' written + /// to the buffer before flushing it to the formatter with 'c=x' and 'd=y'. + pub fn formatEntries(self: Value, buffer_stream: anytype, formatter: anytype) !void { + switch (self) { + .leader => |set| { + // We'll rewind to this position after each sub-entry, + // sharing the prefix between siblings. + const pos = try buffer_stream.getPos(); + + var iter = set.bindings.iterator(); + while (iter.next()) |binding| { + buffer_stream.seekTo(pos) catch unreachable; // can't fail + try std.fmt.format(buffer_stream.writer(), ">{s}", .{binding.key_ptr.*}); + try binding.value_ptr.*.formatEntries(buffer_stream, formatter); + } + }, + + .leaf => |leaf| { + // When we get to the leaf, the buffer_stream contains + // the full sequence of keys needed to reach this action. + try std.fmt.format(buffer_stream.writer(), "={s}", .{leaf.action}); + try formatter.formatEntry([]const u8, buffer_stream.getWritten()); + }, + } + } }; /// Leaf node of a set is an action to trigger. This is a "leaf" compared From 495e4081e4945042eb918b66942b59a60d0d6f57 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 11 Dec 2024 09:21:31 -0800 Subject: [PATCH 2/2] fix: NoSpaceLeft => OutOfMemory NoSpaceLeft is not permitted to be returned in this context, so turn it into OutOfMemory when we fail to write to the buffer. --- src/config/Config.zig | 2 +- src/input/Binding.zig | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 47174aa82..0bcfc743e 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -4214,7 +4214,7 @@ pub const Keybinds = struct { } var buffer_stream = std.io.fixedBufferStream(&buf); - try std.fmt.format(buffer_stream.writer(), "{}", .{k}); + std.fmt.format(buffer_stream.writer(), "{}", .{k}) catch return error.OutOfMemory; try v.formatEntries(&buffer_stream, formatter); } } diff --git a/src/input/Binding.zig b/src/input/Binding.zig index ebccac196..b451b5ec9 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -1171,7 +1171,7 @@ pub const Set = struct { var iter = set.bindings.iterator(); while (iter.next()) |binding| { buffer_stream.seekTo(pos) catch unreachable; // can't fail - try std.fmt.format(buffer_stream.writer(), ">{s}", .{binding.key_ptr.*}); + std.fmt.format(buffer_stream.writer(), ">{s}", .{binding.key_ptr.*}) catch return error.OutOfMemory; try binding.value_ptr.*.formatEntries(buffer_stream, formatter); } }, @@ -1179,7 +1179,7 @@ pub const Set = struct { .leaf => |leaf| { // When we get to the leaf, the buffer_stream contains // the full sequence of keys needed to reach this action. - try std.fmt.format(buffer_stream.writer(), "={s}", .{leaf.action}); + std.fmt.format(buffer_stream.writer(), "={s}", .{leaf.action}) catch return error.OutOfMemory; try formatter.formatEntry([]const u8, buffer_stream.getWritten()); }, }