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

Modifier-only shortcut #114

Closed
lwouis opened this issue Oct 29, 2019 · 126 comments
Closed

Modifier-only shortcut #114

lwouis opened this issue Oct 29, 2019 · 126 comments

Comments

@lwouis
Copy link

lwouis commented Oct 29, 2019

I'm building an app that triggers when ⌥⇥ is pressed, and should then reacts when is released. I would like to let the user decide which shortcut they want to use.

Using RecorderControl, the UI refuses to validate the as a key. It seems it is viewed only as a modifier, and can't be its own key. However in my current implementation, I'm using the proper keyCode and it's working fine, so I don't think there is any technical limitation at play here. Same thing for Shortcut(keyEquivalent: "⌥"): it is returning nil.

Why can't a shortcut be just a modifier key like option or command? Is there a reason or is it just an oversight?

@Kentzo
Copy link
Owner

Kentzo commented Oct 29, 2019

AFAIK Cocoa's controls (NSMenuItem and NSButton) couldn't have such a shortcut and then no one requested it before.

I think that's a reasonable request though.

@Kentzo
Copy link
Owner

Kentzo commented Nov 11, 2019

I wonder what should be the correct value of -[SRShortcut keyCode] when it is "empty".

One option is to change keyCode's type to a larger fixed-width unsigned integer and use it's maximum value to represent "no value".

Would that work well for Swift though? Having an optional seems like a better approach. In other hand, majority of the users will probably keep this feature off (it will definitely be off by default). Forcing developers to unwrap the value is not good.

I need to take a look at how Apple's frameworks address this problem.

@Kentzo Kentzo pinned this issue Nov 11, 2019
@Kentzo
Copy link
Owner

Kentzo commented Nov 11, 2019

Probably a better approach is to change the type to a UInt32 enum and use its maximum as None.

@Kentzo
Copy link
Owner

Kentzo commented Nov 11, 2019

@lwouis How would you expect the user to end recording of modifier flags only shortcut? Currently the control relies on pressing a non-modifier key to end it.

Kentzo added a commit that referenced this issue Nov 12, 2019
@Kentzo
Copy link
Owner

Kentzo commented Nov 12, 2019

Please take a look at the issue-114 branch.

Kentzo added a commit that referenced this issue Nov 12, 2019
Kentzo added a commit that referenced this issue Nov 12, 2019
@lwouis
Copy link
Author

lwouis commented Nov 16, 2019

Please take a look at the issue-114 branch.

Hey @Kentzo, thanks for working on implementing that feature! That's really nice to see!

I took a look at the branch and its latest commit. There are quite a few changes and it's difficult for me to tell if that's correct since i'm unfamiliar with the codebase. On a short-term scale i can spend time manually testing if there is a way for you to share a build of that branch, or maybe i can build it myself easily? I'll check the build instructions later today when i'm not on my phone. I'm unfamiliar with the CI build/tests also, and that would be a big factor in assessing the branch i imagine

@Kentzo
Copy link
Owner

Kentzo commented Nov 16, 2019

At this time don't worry on validity of the code, just take a look at the changes from an end user perspective: that the feature is indeed works in a way you'd expect it. I.e. add it to your or some test project and see if new feature (-[SRRecorderControl allowsModifierFlagsOnlyShortcut]) does what you want.

@lwouis
Copy link
Author

lwouis commented Nov 22, 2019

Hey @Kentzo I tested the branch today! Thanks for working on this so swiftly by the way, I really appreciate it! I was a bit overwhelmed recently but I finally found time today to test your work! I think it's a good first draft, however I see some issues:

  • The recording UX is "modifiers-only" or "modifiers-optional". It seems I can't have a recording UI that can accept "^" and "^a" for example. It would be nice if the user could type either a modifier-only shortcut, or a modifier+key shortcut. This is not needed for my specific project, but I think it would useful to have the flexibility overall. It can wait for another ticket maybe though. You asked earlier "How would you expect the user to end recording of modifier flags only shortcut?". I think it should be: current logic if the user does modifier+key (key ends capture), and new logic if the user presses modifiers then just releases without any key (all modifiers pressed are added to the shortcut, and capture ends when all modifiers keys are up).

  • The recording UX is a bit unintuitive for multiple-modifiers-only shortcuts like "^⌥". If you press "⌥" then it ends right away. You need to first press all modifiers you want except one, then click the UI capture element, then press the last modifiers in the combo, then it will record a shortcut with all modifiers. I think my proposal for ending recording above would address that nicely.

  • Shortcut(keyEquivalent: value) still returns nil for modifier-only shortcuts. This means I can use RecorderControl with allowsModifierFlagsOnlyShortcut = true to show the user a recording UI, but I can't then use it to construct a Shortcut object required to listen to that shortcut later on using GlobalShortcutMonitor for instance.

@Kentzo
Copy link
Owner

Kentzo commented Nov 23, 2019

I think it should be: current logic if the user does modifier+key (key ends capture), and new logic if the user presses modifiers then just releases without any key (all modifiers pressed are added to the shortcut, and capture ends when all modifiers keys are up)

That is logically sound. However it may be annoying to an end user. I would like to see a real case where such mix is needed and is helpful.

IIRC the keyUp: will be sent for each change. So at the end there will be only modifier flag pressed.

The control may require a user to hold the modifier flags for a few seconds to end the recording.

The recording UX is a bit unintuitive for multiple-modifiers-only shortcuts like "^⌥".

That is true. This issue can be partially addressed by the delegate of the RecorderControl. However, it won't allow implementations that have to accept both "^" and "⌥".

Shortcut(keyEquivalent: value) still returns nil for modifier-only shortcuts.

That is an oversight. Good catch! Please use the designated initializer for now.


Perhaps it's better to subclass the control and provide implementation just for the modifier flags recorder.

Side question: did you consider something like a pop-up menu instead for your use case?

@lwouis
Copy link
Author

lwouis commented Nov 24, 2019

That is an oversight. Good catch! Please use the designated initializer for now.

I'm afraid I don't know which initializer you are referring to here. I'm using Shortcut(keyEquivalent: value) in the ShortcutRecorder branch of AltTab. Could you clarify what I should use instead? This is the most important part for us actually, as:

Side question: did you consider something like a pop-up menu instead for your use case?

We are actually using a dropdown menu at the moment:

image image

In the future, I would love to use RecorderControl instead so that users can decide to have multiple modifiers to trigger AltTab. However, as a first step, we could just keep the dropdown, and instantiate Shortcut(keyEquivalent: value) with the modifiers from the dropdown. It doesn't work in master or in branch 114 at the moment though, unfortunately

Also note, in the current screenshot above, how the tab key is a KeyCode number. This would really benefit from using RecorderControl, but this is doable with today's SR master, so just a matter of us implementing it, which should happen soon

@Kentzo
Copy link
Owner

Kentzo commented Nov 25, 2019

Could you clarify what I should use instead?

Shortcut(code: .none, modifierFlags: flag, characters: nil, charactersIgnoringModifiers: nil)

@lwouis
Copy link
Author

lwouis commented Nov 25, 2019

I updated the code to use that constructor. It works everywhere now (!) except one use-case:

When using GlobalShortcutMonitor to monitor a modifier-only shortcut, for forKeyEvent: .up, it doesn't trigger the shortcut. Shortcuts with modifiers+keycode trigger fine, however modifiers-only like only .option don't trigger on key up.

@bryanforbes
Copy link

Has there been any more work on this?

@Kentzo
Copy link
Owner

Kentzo commented Jan 22, 2020

I'm mostly gathering feedback at this moment.

Please explain your use-case and what's missing in the current implementation.

@bryanforbes
Copy link

I'm making a program that listens for a global hotkey to mute/unmute the default input device. I want to be able to listen to key combinations like ctrl-option-command and fn-command.

@Kentzo
Copy link
Owner

Kentzo commented Jan 22, 2020

Would you like a key down or key up event?

@bryanforbes
Copy link

I need both for push-to-talk

@Kentzo
Copy link
Owner

Kentzo commented Jan 22, 2020

When there are multiple modifier flags, it's not straightforward when exactly the shortcut should be invoked. Say you have fn-command pressed: should it be invoked as soon as any of the keys is lifted or only after all keys?

@bryanforbes
Copy link

Thanks for the explanation. In my mind, key down would be fired when all of the modifiers are pressed and key up when any one of them is lifted (since the shortcut is no longer being pressed). When do key down and key up fire for something like cmd-a currently?

@Kentzo
Copy link
Owner

Kentzo commented Jan 22, 2020

The up and down events are triggered by the non-modifier key, i.e. a. Then modifier flags are checked at this very moment. If it's match, then the shortcut gets invoked.

Consider this scenario: you have a shortcut for ctrl-shift, ctrl-cmd-shift is currently pressed. Now, depending on the order of of key up events the shortcut may or may not be triggered. This ambiguity is confusing and hard to explain to the end user.

I am thinking about this behavior: the shortcut is determined by the trailing key up-only events. E.g.

fn down - cmd down - cmd up - fn up = fn-cmd
fn down - cmd down - cmd up - ctrl down - fn up - ctrl up = fn-ctrl

What do you think?

@lwouis
Copy link
Author

lwouis commented Jan 23, 2020

I like the logic described by @bryanforbes. In @Kentzo example of ctrl-shift, ctrl-cmd-shift,

  • If both are defined shortcuts:
    • cmd up -> the ctrl-cmd-shift shortcut is up-triggered
    • ctrl or shift up -> both ``ctrl-cmd-shiftandctrl-shift` shortcuts are up-triggered
  • If only ctrl-shift is a defined shortcut:
    • cmd up -> nothing happens
    • ctrl or shift up -> ctrl-shift shortcuts is up-triggered

@Kentzo described an alternate logic:

the shortcut is determined by the trailing key up-only events

I think it's more confusing than @bryanforbes proposal, because the user is able to press some modifiers, release them, but they don't trigger their defined shortcut because other keys were pressed in the flow.

In the example (I assume both fn-cmd and fn-ctrl are defined shortcuts):

  • fn down - cmd down - cmd up - fn up = fn-cmd -> this one is interesting because I could imagine use-cases that would prefer triggering on "all keys up" instead of "first key up". We could have this be a parameter the modifiers-only shortcut to solve the choice.
  • fn down - cmd down - cmd up - ctrl down - fn up - ctrl up = fn-ctrl -> this one has the issue I mentioned where the user pressed cmd but it had no effect which may confuse them. Here I think this behavior is more direct/simple:

fn down - cmd down - cmd up -> [up-trigger fn-cmd if "first-key-up" param was selected] - ctrl down - fn up -> [up-trigger fn-cmd if "all-keys-up" param was selected + up-trigger fn-ctrl if "first-key-up" param was selected] - ctrl up -> [up-trigger fn-ctrl if "all-keys-up" param was selected on the shortcut]

So to recap:

  • I think some apps may want an up-trigger when the all keys are down (other keys not in the shortcut can be down too) and one key goes up, and other apps may want to trigger only after all keys are up. That can be a param on the modifiers-only shortcut constructor
  • I prefer the more direct logic of looking for: all shortcut keys down -> at this point look for shortcut keys up (either one or all) regardless of other keys that could be pressed that are not part of this shortcut; they may influence trigger of another shortcut, but not the current one.

@Kentzo
Copy link
Owner

Kentzo commented Apr 6, 2020

You have to subclass style to disable the constraint and exclude if from the alwaysConstraints property.

@lwouis
Copy link
Author

lwouis commented Apr 6, 2020

I subclassed RecorderControlStyle and disabled the constraints, but still not results. Also I can't remove the constraint from the alwaysConstraints array because it is readonly. I tried to look for a way to do this in the RecorderControlStyle.m file but it seems that there is an internal _ alwaysConstraints variable that's never exposed, and that is later synthesized as the readonly array alwaysConstraints.

class FitWidthStyle: RecorderControlStyle {
    convenience init(_ identifier: String?, _ components: RecorderControlStyle.Components?) {
        self.init(identifier: identifier, components: components)
        // alwaysConstraints.first(where: { $0.identifier == "SR_alignmentGuide_suggestedWidth"})!.isActive = false
        alwaysConstraints.forEach { $0.isActive = false }
        displayingConstraints.forEach { $0.isActive = false }
    }
}

@Kentzo
Copy link
Owner

Kentzo commented Apr 6, 2020

init is too soon. You need to deactivate the constraint in addConstraints then modify the getter of alwaysConstraints to not return it (otherwise it will be re-activated by the control).

@lwouis
Copy link
Author

lwouis commented Apr 7, 2020

init is too soon. You need to deactivate the constraint in addConstraints then modify the getter of alwaysConstraints to not return it (otherwise it will be re-activated by the control).

I tried to implement something around those guidelines. I couldn't get it to work. I think it's fine for now, especially since it would be messing with internals and making it risky on updates later down the line.

I'm actually surprised nobody asked for a way to have a smaller control before. The current one is quite large by default

By the way, regarding this ticket, I think you could close it. I have released a version of AltTab with ShortcutRecorder finally integrated a few days ago, and it seems to be working great for users 👍

@Kentzo
Copy link
Owner

Kentzo commented Apr 7, 2020

I have removed the explicit constraint for intrinsic width. The control now relies on the intrinsicContentSize provided by the style alone. In other words it should be enough to override intrinsicContentSize and return NSViewNoIntrinsicMetric for width.

Constraint for the height is still present as it's defined by the background assets.

Other things you may try to make the control smaller:

  • Custom style with other font / smaller font size
  • Adjust margins in style metrics
  • Smaller assets for background
  • Return a shorter string in noValueNormalLabel, noValueRecordingLabel and noValueDisableLabel

@Kentzo
Copy link
Owner

Kentzo commented Apr 7, 2020

Feel free to create a new issue to include a smaller style into the framework. I did that in the past and it worked. I included a complimentary "bonus" of $150 though :)

Kentzo added a commit that referenced this issue Apr 7, 2020
Kentzo added a commit that referenced this issue Apr 7, 2020
Kentzo added a commit that referenced this issue Apr 7, 2020
Kentzo added a commit that referenced this issue Apr 7, 2020
Kentzo added a commit that referenced this issue Apr 7, 2020
@Kentzo Kentzo closed this as completed Apr 7, 2020
@Kentzo
Copy link
Owner

Kentzo commented May 11, 2020

@lwouis Can you check if SRAXGlobalShortcutMonitor works for you when you change kCGEventTapOptionDefault to kCGEventTapOptionListenOnly?

@Kentzo Kentzo unpinned this issue May 11, 2020
@lwouis
Copy link
Author

lwouis commented May 11, 2020

@Kentzo I'm not sure what you mean. Currently in AltTab, I use kCGEventTapOptionDefault, which i need since I do modify events (i.e. absorb them if they match within ShortcutRecorder). What would be the point of testing if kCGEventTapOptionListenOnly works?

@Kentzo
Copy link
Owner

Kentzo commented May 11, 2020

Ah I see, indeed you do need kCGEventTapOptionDefault.

I'm considering what'd be a better default for ShortcutRecorder in general.

@lwouis
Copy link
Author

lwouis commented May 12, 2020

I would say least-priviledge is nice as a default, as long as people can opt-in the default mode, it's fine to have listenonly be the default, no?

@Kentzo
Copy link
Owner

Kentzo commented May 12, 2020

My thoughts exactly. Please take a look at the develop branch.

@lwouis
Copy link
Author

lwouis commented May 13, 2020

You mean that commit, right? ae35edd

I checked it and it seems nice to me. You let users set the RunLoop and also active/passive event filtering. Seems all good to me.

You may want to note that some users in AltTab complained that sometimes the keyboard shortcuts stop working. I have tried my best but so far I have no idea how this is happening. It may simply be a macOS bug in that CGEvent api. So keep that in mind perhaps.

I still don't understand to this day for example, why these are happening; what triggers them.

@Kentzo
Copy link
Owner

Kentzo commented May 23, 2020

I still don't understand to this day for example, why these are happening; what triggers them.

You may get that type, even if you didn't register for it explicitly, e.g. if system decides your handler takes too long to execute. Installed "tap" appears to be called synchronously by macOS''s input system.

Thus I suggest to avoid doing any work in the handler and instead handle asynchronously.

You may want to note that some users in AltTab complained that sometimes the keyboard shortcuts stop working.

Perhaps that's exactly what was happening? Ask your users to collect logs for your app next time it happens: SR traces the message. Note that traces are short lived, the best way to capture them is to reproduce the issue while log stream.

@lwouis
Copy link
Author

lwouis commented May 24, 2020

if system decides your handler takes too long to execute.

Do you have any proof that this actually happens? It would seem like such an oversight from Apple not to document it if that's the case. Very interesting in any case. I'll try to make the code returns as fast as possible. Thank you for the info!

@Kentzo
Copy link
Owner

Kentzo commented May 24, 2020

I have encountered it myself while working on this feature. I then added the necessary ‘if’.

The doc in the header says that these messages are out of band.

@lwouis
Copy link
Author

lwouis commented Jun 11, 2020

Hi @Kentzo!

So, we've been trying to identify a keyboard issue with AltTab's community for a while now. Today, I analyzed new logs from a user, and I see this message in these logs:

#Error Unexpected key code 0 for the FlagsChanged event

This message is coming from ShortcutRecorder. After digging a bit, I found the log line:

os_trace("#Error Unexpected key code %hu for the FlagsChanged event", keyCode);

I'm quite puzzled as to how this is happening. From the caller (AltTab), we call get a CGEvent from CGEvent.tapCreate. Then we construct a NSEvent based on that CGEvent:

let event_ = NSEvent(cgEvent: cgEvent)

Then we create a new NSEvent based off that NSEvent (to workaround the issue we had with calling .characters outside the main thread; I think you remember this workaround):

let event = NSEvent.keyEvent(with: event_.type, location: event_.locationInWindow, modifierFlags: event_.modifierFlags, timestamp: event_.timestamp, windowNumber: event_.windowNumber, context: nil, characters: "", charactersIgnoringModifiers: "", isARepeat: type == .flagsChanged ? false : event_.isARepeat, keyCode: event_.keyCode)

At this point we call event.sr_keyEventType to learn if a modifier key was went up or down. Within this method, the log line is logged.

I can't imagine why the keyCode would be 0. From the NSEvent header, it doesn't even seem like 0 should be a possible value:

/* Device-independent bits found in event modifier flags */
typedef NS_OPTIONS(NSUInteger, NSEventModifierFlags) {
    NSEventModifierFlagCapsLock           = 1 << 16, // Set if Caps Lock key is pressed.
    NSEventModifierFlagShift              = 1 << 17, // Set if Shift key is pressed.
    NSEventModifierFlagControl            = 1 << 18, // Set if Control key is pressed.
    NSEventModifierFlagOption             = 1 << 19, // Set if Option or Alternate key is pressed.
    NSEventModifierFlagCommand            = 1 << 20, // Set if Command key is pressed.
    NSEventModifierFlagNumericPad         = 1 << 21, // Set if any key in the numeric keypad is pressed.
    NSEventModifierFlagHelp               = 1 << 22, // Set if the Help key is pressed.
    NSEventModifierFlagFunction           = 1 << 23, // Set if any function key is pressed.
    
    // Used to retrieve only the device-independent modifier flags, allowing applications to mask off the device-dependent modifier flags, including event coalescing information.
    NSEventModifierFlagDeviceIndependentFlagsMask    = 0xffff0000UL
};

Do you have any idea how this scenario would be happening?

@Kentzo
Copy link
Owner

Kentzo commented Jun 11, 2020

@lwouis Btw, try https://github.com/Kentzo/ShortcutRecorder/blob/develop/Library/SRShortcut.h#L80 to avoid some ugliness in your code to circumvent that characters bug.

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

No branches or pull requests

3 participants