Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GTK: performable keys that map to menu accelerators always perform their action #4522

Closed
mitchellh opened this issue Jan 3, 2025 · 7 comments · Fixed by #5421
Closed

GTK: performable keys that map to menu accelerators always perform their action #4522

mitchellh opened this issue Jan 3, 2025 · 7 comments · Fixed by #5421
Labels
input Keyboard or mouse input os/linux
Milestone
1.1.0

Comments

@mitchellh
Copy link
Contributor

When a keybinding is available as a menu item, we set the keybinding as that menu items accelerator. When that is pressed, we go directly through the performBinding machinery. Unfortunately, since this happens before reaching the surface, we never call keyCallback and get all the input method inputs for this.

We need to somehow hopefully tell GTK when an action is not performed that we want it to fall through to the normal input event propagation stack.
@mitchellh mitchellh added input Keyboard or mouse input os/linux and removed os/linux labels Jan 3, 2025
@mitchellh
Copy link
Contributor Author

This applies to macOS too.

@mitchellh mitchellh changed the title GTK: performable keys that map to menu accelerators always perform their action performable keys that map to menu accelerators always perform their action Jan 3, 2025
This was referenced Jan 3, 2025
Closed
Merged
@mitchellh
Copy link
Contributor Author

macOS resolution in #4591

mitchellh added a commit that referenced this issue Jan 4, 2025
@mitchellh
macOS: Input Improvements (#4591)
32c4a9d 
Sorry for the vague title. This PR addresses multiple issues:

1. Fixes #4540 
2. #4522 is fixed for macOS only
3. Fixes #4590 
4. Fixes an untracked issue where `command+key` events will not send
release events for Kitty keyboard protocol, something I only noticed
while working on this.

There are multiple components to this PR.

## Part 1: `App/Surface.keyEventIsBinding`

This new API (also available in libghostty as
`ghostty_surface_key_is_binding`) returns a boolean true if the given
key event would match a binding trigger if it was the next key event
sent. It does not process the binding now.

This can be used by event handlers that intercept key events to
determine if it should send the event to Ghostty. This helps resolve
#4590 for us but is also part of all resolved issues.

## Part 2: macOS `performKeyEquivalent` changes

macOS calls `performKeyEquivalent` for any key combination that may
trigger a key equivalent. if this returns `true` then it is handled and
macOS ceases processing the event.

We were already using this to intercept things like `Ctrl+/` which
triggers a context menu in macOS Sequoia. But we now expand this to
intercept all events to check for bindings. This lets us fix #4590.

Additionally, it's been changed to special case `cmd+period`. I'm sure
more need to be added.

## Part 3: NSEvent local listener for command keyUp events

macOS simply doesn't send `keyUp` events for key events with command
pressed. The only way to work around this is to register an `NSEvent`
local listener. We now do this. This fixes the untracked issue noted
above.
@mitchellh
Copy link
Contributor Author

Marked as Linux since macOS is now fixed.

@mitchellh mitchellh changed the title performable keys that map to menu accelerators always perform their action GTK: performable keys that map to menu accelerators always perform their action Jan 4, 2025
@mitchellh mitchellh added this to the 1.0.2 milestone Jan 4, 2025
@mitchellh
Copy link
Contributor Author

This is actually a non-issue on GTK because the surface gets the key events first and only surface events are performable.

@mitchellh mitchellh reopened this Jan 7, 2025
@mitchellh
Copy link
Contributor Author

Actually this is an issue on GTK.

@mitchellh mitchellh mentioned this issue Jan 7, 2025
Closed
@westerlind
Copy link

For information, it is apparently possible to work around this issue on Linux by adding a second keybinding for copy_to_clipboard like the following that I have in my config at the moment:

keybind = ctrl+shift+c=copy_to_clipboard
keybind = performable:ctrl+c=copy_to_clipboard

So if there is a second keybinding for copy_to_clipboard that does not include the performable part, it works as intended (at least when it is configured exactly as the above)

@westerlind
Copy link

I have to correct what I said in my last post above.

It seemed to me at first like it was possible to work around this issue by adding a second keybinding for copy_to_clipboard, but I must have simply done things in a specific order several times that made it look like that, even though it wasn't actually the case..

It seems like it's rather that if I add keybind = performable:ctrl+c=copy_to_clipboard to the config, that works, but only after I have right clicked at least once in Ghostty after launching it.

Sorry for the confusion.

mitchellh added a commit that referenced this issue Jan 29, 2025
@mitchellh
input: performable bindings aren't part of the reverse mapping
14926b3 
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.
@mitchellh mitchellh mentioned this issue Jan 29, 2025
Merged
mitchellh added a commit that referenced this issue Jan 29, 2025
@mitchellh
input: performable bindings aren't part of the reverse mapping (#5421)
fefda69 
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.
liby pushed a commit to liby/ghostty that referenced this issue Feb 11, 2025
@mitchellh @liby
input: performable bindings aren't part of the reverse mapping
66b790e 
Fixes ghostty-org#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.
rockorager pushed a commit to rockorager/ghostty that referenced this issue Feb 12, 2025
@mitchellh @rockorager
input: performable bindings aren't part of the reverse mapping
469e0e6 
Fixes ghostty-org#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.
rockorager pushed a commit to rockorager/ghostty that referenced this issue Feb 12, 2025
@mitchellh @rockorager
input: performable bindings aren't part of the reverse mapping
55fec6c 
Fixes ghostty-org#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
input Keyboard or mouse input os/linux
Projects
None yet
Milestone
1.1.0
Development

Successfully merging a pull request may close this issue.

input: performable bindings aren't part of the reverse mapping ghostty-org/ghostty
2 participants
@mitchellh @westerlind