Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
MNT: Add modifier key press handling to macosx backend#21512
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This works as advertised. Not an expert on macOS backend, so not sure this is the "right" fix, but....
src/_macosx.m Outdated
| match up with what the front-end and does the front-end even handle modifier keys by themselves? | ||
| // flagsChanged gets called on single modifier keypresses | ||
| // The modifier + second key gets handled in convertKeyEvent in KeyUp/KeyDown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does it also work for e.g. "ctrl+shift"? (dunno, just asking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a ctrl, shift, f for example produces this:
youpressedcmdNoneNoneyoupressedshiftNoneNoneyoupressedcmd+FNoneNoneyoupressedcmdNoneNone
I think the extra command at the end is there due to the shift also being let up and changing state while command is still pressed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On other backends, the second press is "cmd+shift", not just shift. (Well, ctrl+shift on non-osx.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, so this was more complicated than I had hoped... The flagsChanged gets called on any press OR release, which is why I was printing out "pressed" when it was really a "release".
I think this means we have to keep track of the state of the keys ourselves and what is going up/down. I think this now matches what other backends do, including press/release properly.
Uh oh!
There was an error while loading.Please reload this page.
fc77052 to2120ca0Comparesrc/_macosx.m Outdated
| lastCommand = false; | ||
| keyChangeCommand = true; | ||
| keyrelease = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not a huge deal, but I guess each of these pairs can be grouped to
bool isPress =true;// false for releaseif ([eventmodifierFlags] & NSEventModifierFlagCommand) { lastCommand = !lastCommand keyChangeCommand =true isPress = lastCommand;}elseif (...) ...}else {// Unknown flagreturn;}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
NSEventModifierFlagCommand will only be true when it is held down, so we would miss the release with that implementation I think... We could probably squash the two cases into a single if block with your suggestion, but maybe I'm missing a more elegant way to keep track of the push/release here:
if ((([eventmodifierFlags] & NSEventModifierFlagCommand) && !lastCommand) || (!([eventmodifierFlags] & NSEventModifierFlagCommand) && lastCommand)) {// Command pressed or released lastCommand = !lastCommand; keyChangeCommand =true; keypress = lastCommand; } ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, sorry, I missed the inversion. Perhaps a good place for a^ (xor)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 Yep, a good place for it, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, although probably another mac user should test :-)
anntzer commentedNov 26, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ah, perhaps also update the table athttps://github.com/matplotlib/matplotlib/blob/main/doc/users/explain/event_handling.rst#event-connections (under "note")? |
Good suggestion about updating the table.
|
Looks like the original author was from Mexico where shift+2 is " (http://kbdlayout.info/KBDLA/), probably the entry should be fixed for qwerty.
Perhttps://en.wikipedia.org/wiki/AltGr_key I guess that's "left alt/option"
I guess no one has, probably that was capslocked indeed? |
Thanks for the table update. I'll merge in a day or two if the macos crew doesn't manifest themselves for a second review :) |
I've not been following. Has it diverged significantly from when I reviewed? I can try it again. |
Mostly just check that things work just as advertised in the table under event_handling.rst. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
modulo@jklymak checking, per the above.
There was quite a bit of logic rework since your initial review, and this now handles modifier key_release events too. Mostly just mash some keys and make sure you get back what you're expecting when testing with the script included in the example. If you have an AltGr key, it would be good to update the table entry for that too, as I can't check that. |
I've never heard of an AltGr key so I guess if don't have one. I'll try and check tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Works well for me, and fixes issues as advertised. I haven't reviewed the code in_macosx.m, but the doc update looks good.
Oh, and I don't think macs have AltGr keys, so leaving that empty is fine. |
The flagsChanged event handles single presses of modifier keys,so we need to send the corresponding string to our key press handlersfrom within that routine. Modifier + second key is handled withinthe KeyUp/KeyDown routines already, so leave those alone.
Handle the key-press and release events on macosx.
This removes half of the else-if blocks and combines thekeypress/keyrelease for each key into a single statement with negationfor whether it was a press or release being handled.
- This updates the if conditional to use XOR logic instead of the two negated and/or conditions.- It also handles the shift modifier when combined with other modifier keys, as before (ctrl + shift + cmd) would have not printed out the middle shift.
I am going to self-merge this based on the 3 approvals and to avoid future rebases. It is a bugfix, but it has been broken for a while, so I don't think we should worry about backporting this. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
The flagsChanged event handles single presses of modifier keys, so we need to send the corresponding string to our key press handlers from within that routine. Modifier + second key is handled within the KeyUp/KeyDown routines already, so leave those alone.
Taking the example from issue#20486
this adds the single modifier key presses back in. Before this, the (modifier + key) "cmd+a" would show up, but there wasn't a single "cmd" (modifier) preceding it.
I'm not sure how to send key press events through the terminal natively, so there are no automated tests.
closes#20486
closes#9837
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).