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

Added support for changeable SwiftUI .keyboardShortcuts, based on the… #181

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

scornflake
Copy link

This adds support for SwiftUI menu items (they now auto update when shortcuts are changed).

Also; it won't throw up a UI when modifying the existing shortcut. The implementation ... isn't great (it relies on comparing the instance points of NSMenuItems), but I can't find any other alternative where I can correctly identify that a SwiftUI generated NSMenuItem is related to a given shortcut.

  • Added support for changeable SwiftUI .keyboardShortcuts, based on the work done by @mbenoukaiss
  • Modified RecorderCocoa so that it doesn't show a dialog if we are reassigning one of our own menu items

… work done by @mbenoukaiss

Modified RecorderCocoa so that it doesn't show a dialog if we are reassigning one of our own menu items
Copy link

@JaviSoto JaviSoto left a comment

Choose a reason for hiding this comment

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

Nice!

// Holds the state of the shortcut, and changes that state when the shortcut changes
// This causes the related NSMenuItem to also update (yipeee)
@available(macOS 11.0, *)
struct KeyboardShortcutView<Content: View>: View {

Choose a reason for hiding this comment

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

I think the idiomatic way to do this would be to use ViewModifier instead

Copy link
Author

@scornflake scornflake Jul 18, 2024

Choose a reason for hiding this comment

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

Yup - there's a view modifier directly above (which uses KeyboardShortcutView). I'm only using KeyboardShortcutView as a wrapper to store state so that the view/menu changes when the shortcut changes. If this is possible using a viewModifier function, happy to learn about it and adjust.

Copy link
Owner

Choose a reason for hiding this comment

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

No, that's a View extension, not a ViewModifier.

Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved

@ViewBuilder
var shortcutBody: some View {
if let shortcut, let keyEquivalent = shortcut.toKeyEquivalent() {
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to do if and ruin the view id when it changes. Instead, use https://developer.apple.com/documentation/financekitui/addordertowalletbutton/keyboardshortcut(_:)-883ia#

Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/SwiftUI+Extensions.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/Shortcut.swift Outdated Show resolved Hide resolved
Sources/KeyboardShortcuts/RecorderCocoa.swift Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I recommend going over the code and also the pull request diff and look for ways to clean it up, fix typos (in comments too), etc. It's still a bit away from being in a mergable state.

@scornflake
Copy link
Author

Re the comment about going over code and cleaning up: anything specific? I thought it was clean.
Any guidelines you want me to follow?

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.

3 participants