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.

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

macOS: Input Improvements #4591

Merged
merged 6 commits into from
Jan 4, 2025
Merged

macOS: Input Improvements #4591

merged 6 commits into from
Jan 4, 2025

Conversation

mitchellh
Copy link
Contributor

@mitchellh mitchellh commented Jan 4, 2025

Sorry for the vague title. This PR addresses multiple issues:

  1. Fixes macOS: super+period doesn't reach Surface keyCallback #4540
  2. GTK: performable keys that map to menu accelerators always perform their action #4522 is fixed for macOS only
  3. Fixes macOS: Keybindings should take precedence over standard menu shortcuts #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 mitchellh marked this pull request as ready for review January 4, 2025 22:08
@mitchellh mitchellh enabled auto-merge January 4, 2025 22:10
@mitchellh mitchellh merged commit 32c4a9d into main Jan 4, 2025
48 checks passed
@mitchellh mitchellh deleted the macos-input branch January 4, 2025 22:22
@github-actions github-actions bot added this to the 1.0.2 milestone Jan 4, 2025
@AlexJuca
Copy link
Contributor

AlexJuca commented Jan 4, 2025

@mitchellh macOS prioritizes global or application-level shortcuts (e.g., ⌘C for copy, ⌘V for paste). The keyDown event is handled, but the keyUp is ignored by the O.S., so NSEvent is the only other viable solution.

Your solution is pretty good.

@mitchellh
Copy link
Contributor Author

Your solution is pretty good.

Awesome. Thanks!

@@ -405,7 +411,7 @@
A5985CE52C33060F00C57AD3 /* man */,
A5A1F8842A489D6800D1E8BC /* terminfo */,
FC5218F92D10FFC7004C93E0 /* zsh */,
9351BE8E2D22937F003B3499 /* nvim */,
9351BE8E2D22937F003B3499 /* vim */,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I noticed that in this PR, both vim and nvim folders are renamed to "vim" in the Xcode project references. While the final build output still correctly maintains the distinction between vim and nvim folders, I'm curious about the motivation behind this change in the project structure.

I see that both folders contain identical content at the moment, but having two identically named "vim" folders in the Resources group (as shown in the Xcode project navigator) might be a bit confusing for future maintenance. Could you share the reasoning behind this unification of names in the Xcode project?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I didn’t do this on purpose at all. So this was somehow a mistake. I’ll take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants