input: performable bindings aren't part of the reverse mapping (#5421)

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

@@ -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

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;