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

Fix logic for .globalKeyboardShortcut() #193

Merged
merged 4 commits into from
Dec 7, 2024

Conversation

cweider
Copy link
Contributor

@cweider cweider commented Nov 30, 2024

Evaluating globalKeyboardShortcut(_:) revealed several issues with related to special keys:

  • Shortcuts using the Key.space would produce a KeyEquivalent that specified the "S" key. (cause: first being used on the keyToCharacter() return value, "space")
  • Shortcuts using Key.f* would produce a KeyEquivalent that specified the "F" key. (cause: same as above)
  • Shortcuts using Key.keypad* would produce a KeyEquivalent that was not limited to the number pad. (cause: neither SwiftUI nor AppKit make this distinction when producing menu items)

Considering these issues, a reevaluation of special keys' handling is called for. Excepting the Carbon integration that drives the actual keyboard shortcuts, there are three distinct contexts where a Shortcut is used:

  • Recoder: where an NSString is produced to go into the TextField displaying the shortcut's current value
  • NSMenuItem: for specifying keyEquivalent and keyEquivalentModifierMask
  • toSwiftUI: for creating a SwiftUI.KeyboardShortcut

Ideally, each of these three do their work in analogous ways. This branch accomplishes this with the following:

  • SpecialKey enumerates the cases that require extra attention. This is drawn directly from keys used in keyToCharacterMapping and used to ensure that each special case is handled.
  • Three mappings for SpecialKey are created for each of the three contexts where the handling is important: presentableDescription, swiftUIKeyEquivalent, and appKitMenuItemKeyEquivalent.
  • In these three contexts if there is a SpecialKey, the defined mapping is what rules. keyToCharacter() is used if and only if there is no SpecialKey available. An assertion to this effect is added.

Fixes: #192

@cweider
Copy link
Contributor Author

cweider commented Nov 30, 2024

For testing, this is a snippet that you can go and stick any old place:

Menu("Menu", systemImage: "filemenu.and.selection") {
	Button("Shortcut 1") {
		// no-op
	}
	.globalKeyboardShortcut(.testShortcut1)

	Button("Shortcut 2") {
		// no-op
	}
	.globalKeyboardShortcut(.testShortcut2)

	Button("Shortcut 3") {
		// no-op
	}
	.globalKeyboardShortcut(.testShortcut3)

	Button("Shortcut 4") {
		// no-op
	}
	.globalKeyboardShortcut(.testShortcut4)
}

@cweider
Copy link
Contributor Author

cweider commented Nov 30, 2024

Actual Expected
SwiftUI KeyboardShortcut - Actual SwiftUI KeyboardShortcut - Expected

Evaluating `globalKeyboardShortcut(_:)` revealed several issues
with related to special keys:

- Shortcuts using the `Key.space` would produce a `KeyEquivalent`
  that specified the "S" key.
  (cause: `first` being used on the `keyToCharacter()` return
  value, `"space"`)
- Shortcuts using `Key.f*` would produce a `KeyEquivalent` that
  Specified the "F" key.
  (cause: same as above)
- Shortcuts using `Key.keypad*` would produce a `KeyEquivalent`
  that was not limited to the number pad.
  (cause: neither SwiftUI nor AppKit make this distinction when
  producing menu items)

Considering these issues, a reevaluation of special keys' handling
is called for. Excepting the `Carbon` integration that drives the
actual keyboard shortcuts, there are three distinct contexts where
a `Shortcut` is used:

- `Recoder`: where an `NSString` is produced to go into the
  `TextField` displaying the shortcut's current value
- `NSMenuItem`: for specifying `keyEquivalent` and
  `keyEquivalentModifierMask`
- `toSwiftUI`: for creating a `SwiftUI.KeyboardShortcut`

Ideally, each of these three do their work in analogous ways. This
branch accomplishes this with the following:

- `SpecialKey` enumerates the cases that require extra attention.
  This is drawn directly from keys used in `keyToCharacterMapping`
  and used to ensure that each special case is handled.
- Three mappings for `SpecialKey` are created for each of the three
  contexts where the handling is important:
  `presentableDescription`, `swiftUIKeyEquivalent`, and
  `appKitMenuItemKeyEquivalent`.
- In these three contexts if there is a `SpecialKey`, the defined
  mapping is what rules. `keyToCharacter()` is used if and only if
  there is no `SpecialKey` available. An assertion to this effect
  is added.

Fixes: sindresorhus#192
@cweider cweider force-pushed the key-equivalent-mappings branch from c6a0371 to 4bdbf5c Compare November 30, 2024 06:43
@cweider
Copy link
Contributor Author

cweider commented Nov 30, 2024

I should add that cweider/KeyboardShortcuts@swiftui-key-equivalent is the minimal change for SwiftUI. That this, keyEquivalent, and description each took different approaches to the similar problem motivated this patch.

@sindresorhus
Copy link
Owner

This is looking very good. Thanks for working fixing this 🙏

Avoid repeating ourselves when creating these objects in a
parameterized way (continue using literals where reasonable to get
compile-time checks).
@cweider cweider force-pushed the key-equivalent-mappings branch from be009d9 to 6e9bed1 Compare December 7, 2024 03:16
@sindresorhus sindresorhus changed the title Refactor/fix handling of special keys in integrations Fix logic for .globalKeyboardShortcut() Dec 7, 2024
@sindresorhus sindresorhus merged commit 323d9f8 into sindresorhus:main Dec 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect KeyEquivalent generated by for globalKeyboardShortcut(_:) with .space key
2 participants