From ce2a3773d2dc2e6461efd3f5fcca6d5888e69ba1 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 29 Jan 2025 14:08:04 -0800 Subject: [PATCH] input: performable bindings aren't part of the reverse mapping Fixes #4522 This is a bit of a hammer-meets-nail solution, but it's a simple solution to the problem. The reverse mapping is used to find the binding that an action is bound to, and it's used by apprt's to populate the accelerator label in the UI. The problem is that accelerators in GTK are handled early in the event handling process and its difficult to get that event mapping to a specific surface. Therefore, the "performable" prefix was not working. On macOS, this issue didn't exist because there exists an OS mechanism to install an event handler earlier than the menu system. This commit changes the reverse mapping to only include bindings that are not performable. This way, the keybind always reaches the surface and can be handled by `Surface.keyCallback` which processes `performable`. The caveat is that performable bindings will not show up in the UI for menu items. This is documented in this commit now. They still work, its just a UI issue. --- src/config/Config.zig | 6 +++++ src/input/Binding.zig | 56 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 1e1bca74af..0ed98bdea8 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1015,6 +1015,12 @@ class: ?[:0]const u8 = null, /// performable (acting identically to not having a keybind set at /// all). /// +/// Performable keybinds will not appear as menu shortcuts in the +/// application menu. This is because the menu shortcuts force the +/// action to be performed regardless of the state of the terminal. +/// Performable keybinds will still work, they just won't appear as +/// a shortcut label in the menu. +/// /// Keybind triggers are not unique per prefix combination. For example, /// `ctrl+a` and `global:ctrl+a` are not two separate keybinds. The keybind /// set later will overwrite the keybind set earlier. In this case, the diff --git a/src/input/Binding.zig b/src/input/Binding.zig index a1e759bf86..19c1031957 100644 --- a/src/input/Binding.zig +++ b/src/input/Binding.zig @@ -284,11 +284,11 @@ pub const Action = union(enum) { scroll_page_fractional: f32, scroll_page_lines: i16, - /// Adjust the current selection in a given direction. Does nothing if no + /// Adjust the current selection in a given direction. Does nothing if no /// selection exists. /// /// Arguments: - /// - left, right, up, down, page_up, page_down, home, end, + /// - left, right, up, down, page_up, page_down, home, end, /// beginning_of_line, end_of_line /// /// Example: Extend selection to the right @@ -1230,6 +1230,13 @@ pub const Set = struct { /// This is a conscious decision since the primary use case of the reverse /// map is to support GUI toolkit keyboard accelerators and no mainstream /// GUI toolkit supports sequences. + /// + /// Performable triggers are also not present in the reverse map. This + /// is so that GUI toolkits don't register performable triggers as + /// menu shortcuts (the primary use case of the reverse map). GUI toolkits + /// such as GTK handle menu shortcuts too early in the event lifecycle + /// for performable to work so this is a conscious decision to ease the + /// integration with GUI toolkits. reverse: ReverseMap = .{}, /// The entry type for the forward mapping of trigger to action. @@ -1494,6 +1501,11 @@ pub const Set = struct { // unbind should never go into the set, it should be handled prior assert(action != .unbind); + // This is true if we're going to track this entry as + // a reverse mapping. There are certain scenarios we don't. + // See the reverse map docs for more information. + const track_reverse: bool = !flags.performable; + const gop = try self.bindings.getOrPut(alloc, t); if (gop.found_existing) switch (gop.value_ptr.*) { @@ -1505,7 +1517,7 @@ pub const Set = struct { // If we have an existing binding for this trigger, we have to // update the reverse mapping to remove the old action. - .leaf => { + .leaf => if (track_reverse) { const t_hash = t.hash(); var it = self.reverse.iterator(); while (it.next()) |reverse_entry| it: { @@ -1522,8 +1534,9 @@ pub const Set = struct { .flags = flags, } }; errdefer _ = self.bindings.remove(t); - try self.reverse.put(alloc, action, t); - errdefer _ = self.reverse.remove(action); + + if (track_reverse) try self.reverse.put(alloc, action, t); + errdefer if (track_reverse) self.reverse.remove(action); } /// Get a binding for a given trigger. @@ -2373,6 +2386,39 @@ test "set: maintains reverse mapping" { } } +test "set: performable is not part of reverse mappings" { + const testing = std.testing; + const alloc = testing.allocator; + + var s: Set = .{}; + defer s.deinit(alloc); + + try s.put(alloc, .{ .key = .{ .translated = .a } }, .{ .new_window = {} }); + { + const trigger = s.getTrigger(.{ .new_window = {} }).?; + try testing.expect(trigger.key.translated == .a); + } + + // trigger should be non-performable + try s.putFlags( + alloc, + .{ .key = .{ .translated = .b } }, + .{ .new_window = {} }, + .{ .performable = true }, + ); + { + const trigger = s.getTrigger(.{ .new_window = {} }).?; + try testing.expect(trigger.key.translated == .a); + } + + // removal of performable should do nothing + s.remove(alloc, .{ .key = .{ .translated = .b } }); + { + const trigger = s.getTrigger(.{ .new_window = {} }).?; + try testing.expect(trigger.key.translated == .a); + } +} + test "set: overriding a mapping updates reverse" { const testing = std.testing; const alloc = testing.allocator;