search

cocoabits / MASShortcut

Modern framework for managing global keyboard shortcuts compatible with Mac App Store. More details:
http://blog.shpakovski.com/2012/07/global-keyboard-shortcuts-in-cocoa.html
BSD 2-Clause "Simplified" License
1.52k stars 220 forks source link

Shortcut handler fired when "changing" shortcut to it's current value #37

Closed pencil closed 10 years ago

pencil commented 10 years ago

Steps to Reproduce

  1. Click the MASShortcutView to record a new shortcut.
  2. Type Cmd+F10
  3. Click the MASShortcutView to change the shortcut.
  4. Type Cmd+F10

    Expected Behaviour

Shortcut handler does not fire since the user is actually recording the shortcut.

Actual Behaviour

Shortcut handler is fired.

Video

changing_shortcut

joeljfischer commented 10 years ago

+1

shpakovski commented 10 years ago

Hi guys, thanks a lot for the feedback! I thought about this case for a while, but not sure how to fix it easily. The thing is that the shortcut “handler” is fully decoupled from the shortcut “assigner”:

Typically, you define the code block for reacting to the shortcut in the AppDelegate.m. However, the view responsible for re-defining the shortcut is usually managed in another location ie. in GeneralPreferenceViewController.m.

To avoid the UX “problem” demonstrated by video, the MASShortcutView component must know about the shortcut handler and “disable” it. Or, the internal hotkey processor must know that “some” view is now editing “some” shortcut. So, fixing this would require quite deep coupling between model and view components.

One possible solution would be to manually check the editing property of the MASShortcutView in the shortcut handler block. Please let me know if I miss something, or you have an idea for a better workaround…

Hope this helps! Vadim

pencil commented 10 years ago

The solution I have in mind is to temporary delete the shortcut from the NSUserDefaults once the user starts editing the shortcut. From what I understand, we would have to set the shortcutValue to nil. Maybe we would have to backup the original value somewhere in case the user cancels the recording. Would that work?

joeljfischer commented 10 years ago

That could work, although I would worry about crashes or force quits in this state losing the stored default. An edge case, sure, but it might happen. On Jul 18, 2014 7:09 AM, "Nils Caspar" notifications@github.com wrote:

The solution I have in mind is to temporary delete the shortcut from the NSUserDefaults once the user starts editing the shortcut. From what I understand, we would have to set the shortcutValue to nil. Maybe we would have to backup the original value somewhere in case the user cancels the recording. Would that work?

— Reply to this email directly or view it on GitHub https://github.com/shpakovski/MASShortcut/issues/37#issuecomment-49419706 .

shpakovski commented 10 years ago

@pencil I think you have to try how it works.

@joeljfischer Well, I would hope for better :)

Overall, this situation is quite interesting. Maybe there is no need to handle each shortcut this way? For example, you set the shortcut to open the Search Panel from the status bar. Then you start editing the shortcut, press that already taken hotkey, and BOOM — the Search Panel pops up! So what? :) Now you can be sure that it is active until you hit the Delete button. I am not a user experience expert, but this sounds like quite expected behavior.

joeljfischer commented 10 years ago

Personally, I would expect that while editing a shortcut, all shortcuts would be disabled, because I'm in a special "editing" mode. I would also expect it to tell me if a shortcut is already taken, whether disabled or not (I believe you already do this). I don't think a deep coupling is necessary, but a coupling of a sort may be required.

For instance, a message telling the model that a hotkey is being edited, so that the model can temporarily disable the hotkeys it knows about, or at least calling their handlers, and a message when the hotkey stops being edited, so that it can re-enable all shortcuts firing.

If that is too difficult, I can do something similar manually. I'm thinking I could observe the editing property and fire a notification when one starts being edited (and stops being edited), and use that to disable the handlers manually.

shpakovski commented 10 years ago

I don't think a deep coupling is necessary, but a coupling of a sort may be required.

This is a key. I do not want to couple MASShortcut and MASShortcutView at all. I like that MASShortcut has no idea about any MASShortcutView interactions.

shpakovski commented 10 years ago

I'm thinking I could observe the editing property and fire a notification when one starts being edited (and stops being edited), and use that to disable the handlers manually.`

Sounds great!