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

Make unregisterall() remove UserDefaults + Make it a public method #184

Conversation

stormychel
Copy link

  • unregisterall() now also removes UserDefaults entries
  • made unregisterall() public since it can be useful to clean up all shortcuts from an app

@sindresorhus
Copy link
Owner

I'm not sure it makes sense to expose this anymore. The recommended API is with .events() and it automatically unregisters when it's no longer listening to events.

@stormychel
Copy link
Author

We have some stuck hotkeys in our app that could benefit from this, but we could off course use a custom branch to deal with it,, whatever works best for you.
Thanks for making this available!

@stormychel
Copy link
Author

Do you want me to split up the PR so only "unregisterall() now also removes UserDefaults entries" can get merged?
It is a TODO in your code...

@sindresorhus
Copy link
Owner

I think it makes more sense if it's called resetAll(). I got confused by its use-case because of its name. I assume you need this to reset all the shortcuts?

I'm ok with exposing it then. But it will need doc comments. And it should probably be moved to below public static func reset(_ names: Name...) {.

@stormychel
Copy link
Author

It depends whether you think this is useful for others. We are already on a custom branch, and also have a method "public static func unregisterUnknownShortcuts(knownShortcutNames: [KeyboardShortcuts.Name]) {
}" which removes shortcuts unknown by the app so maybe our needs are a bit more than what others would typically need so I don't know whether these are useful for everyone. Will make the changes you requested + doc comments, thanks for reviewing!

@sindresorhus
Copy link
Owner

static func unregisterUnknownShortcuts(knownShortcutNames: [KeyboardShortcuts.Name]) {

This one is indeed too niche.

@stormychel
Copy link
Author

static func unregisterUnknownShortcuts(knownShortcutNames: [KeyboardShortcuts.Name]) {

This one is indeed too niche.

Totally agree :)

I will clean up my PR and add docs over the weekend.

@sindresorhus if we have a shortcut that is stuck, even after doing our unregisterUnknownShortcuts() thing, is there anything we can do to get rid of it without wiping user defaults for app completely? One of our Macs has a shortcut that is stuck this way, hence our efforts to create unregisterUnknownShortcuts(), but even this does not solve it, while the method definitely works. This is only on one development machine, I am 100% sure this is from testing work branches, and tbh I'd just clear defaults at this point, but we want to make sure this does not happen to users. Any ideas on how this could happen?

- relocate unregisterAll() to below public static func reset()
@stormychel
Copy link
Author

@sindresorhus added docs & relocated method: 5dda3b4

@sindresorhus
Copy link
Owner

I think it makes more sense if it's called resetAll().

@stormychel
Copy link
Author

I think it makes more sense if it's called resetAll().

Yes, I forgot, my bad, done now: 0af13cf

Comment on lines +267 to +271
Reset all keyboard shortcuts and remove their stored values from `UserDefaults`.

This method resets all keyboard shortcuts registered via `KeyboardShortcuts` and removes any associated data stored in `UserDefaults`.

Use this method to completely reset the state of all keyboard shortcuts in your app, including removing any saved user-defined shortcuts and default shortcuts.
Copy link
Owner

Choose a reason for hiding this comment

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

These 3 paragraphs pretty much say the same thing.


Use this method to completely reset the state of all keyboard shortcuts in your app, including removing any saved user-defined shortcuts and default shortcuts.

- Note: This action cannot be undone. All shortcuts will be removed, including any default shortcuts defined in `Name`. If you want to reset shortcuts back to their default values, consider using `.reset(_:)` instead.
Copy link
Owner

Choose a reason for hiding this comment

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

If you want to reset shortcuts back to their default values, consider using .reset(_:) instead.

I don't think it makes sense that this has different behavior than .reset(). resetAll should just be reset for all.

@sindresorhus
Copy link
Owner

You have some lint violations. See the diff.

@stormychel
Copy link
Author

Nevermind, I will just keep on using my forked version, got other stuff to do than endlessly working on one simple PR.
Thanks for considering though!

@stormychel stormychel closed this Oct 9, 2024
@stormychel stormychel deleted the 9-make-private-static-func-unregisterall-public branch October 9, 2024 07:40
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.

2 participants