fix various keybinding/shortcut bugs #5604

kne42 · 2023-03-05T19:36:45Z

This PR addresses bugs where the improper symbols would be displayed for e.g. ⌘V on Macs, as well as improper capturing on Macs where e.g. Control would be read as Meta and vice versa. This relies on an upstream fix in app-model and uses the logic there to properly capture the correct modifier keys, regardless of if Qt is switching Control with Meta or not.

fixes #5047
fixes #5332
fixes #5579

depends on pyapp-kit/app-model#82

codecov · 2023-03-05T19:56:37Z

Codecov Report

Merging #5604 (a5df12d) into main (1f67a82) will increase coverage by 1.55%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #5604      +/-   ##
==========================================
+ Coverage   88.30%   89.86%   +1.55%     
==========================================
  Files         611      611              
  Lines       51621    51629       +8     
==========================================
+ Hits        45584    46396     +812     
+ Misses       6037     5233     -804

Impacted Files	Coverage Δ
napari/_qt/widgets/qt_keyboard_settings.py	`72.08% <20.00%> (+1.67%)`	⬆️
napari/utils/interactions.py	`75.23% <100.00%> (+2.85%)`	⬆️

... and 60 files with indirect coverage changes

psobolewskiPhD · 2023-03-05T22:19:30Z

I tested this (along with the app-model PR) and it all worked as advertised! 🎉
I'll try and take a closer look at the code tomorrow evening.

Czaki · 2023-03-06T00:24:19Z

napari/_qt/widgets/qt_keyboard_settings.py

+        if event_key in (
+            Qt.Key.Key_Control,
+            Qt.Key.Key_Shift,
+            Qt.Key.Key_Alt,
+            Qt.Key.Key_Meta,
+            Qt.Key.Key_Delete,
+        ):


Suggested change

if event_key in (

Qt.Key.Key_Control,

Qt.Key.Key_Shift,

Qt.Key.Key_Alt,

Qt.Key.Key_Meta,

Qt.Key.Key_Delete,

):

if event_key in {

Qt.Key.Key_Control,

Qt.Key.Key_Shift,

Qt.Key.Key_Alt,

Qt.Key.Key_Meta,

Qt.Key.Key_Delete,

}:

what's the advantage of switching to a set instead of a tuple here?

It should be speedup and style.

But I do some additional tests and it looks like in this case the speedup is not observed:

In [14]: %timeit Qt.Key.Key_Control in (Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,) 6.84 µs ± 380 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [15]: %timeit Qt.Key.Key_Delete in (Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,) 6.95 µs ± 131 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [16]: %timeit Qt.Key.Key_Control in {Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,} 6.83 µs ± 159 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [17]: %timeit Qt.Key.Key_Delete in {Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,} 7.05 µs ± 323 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

The additional test shows that creation time is essential:

In [18]: a = {Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,} In [19]: b = (Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,) In [20]: %timeit Qt.Key.Key_Delete in a 1.04 µs ± 29.5 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [21]: %timeit Qt.Key.Key_Delete in b 1.08 µs ± 22.5 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [22]: %timeit Qt.Key.Key_Control in b 966 ns ± 43.4 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [23]: %timeit Qt.Key.Key_Control in a 958 ns ± 18 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

So next test:

In [10]: event_key = Qt.Key.Key_Control In [24]: %timeit event_key in a 52.8 ns ± 4.48 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [25]: %timeit event_key in b 41 ns ± 0.744 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [26]: event_key = Qt.Key.Key_Delete In [27]: %timeit event_key in a 47.4 ns ± 0.934 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [28]: %timeit event_key in b 76.6 ns ± 0.942 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

but put this in function does not help:

In [34]: def check_set(key): ...: return key in {Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,} ...: In [35]: def check_tuple(key): ...: return key in (Qt.Key.Key_Control, Qt.Key.Key_Shift, Qt.Key.Key_Alt, Qt.Key.Key_Meta, Qt.Key.Key_Delete,) ...: In [36]: event_key = Qt.Key.Key_Delete In [37]: %timeit check_set(event_key) 5.96 µs ± 157 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [38]: %timeit check_tuple(event_key) 5.84 µs ± 219 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [39]: event_key = Qt.Key.Key_Control In [40]: %timeit check_set(event_key) 5.85 µs ± 104 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each) In [41]: %timeit check_tuple(event_key) 5.81 µs ± 136 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

So finaly checks show that access to qt enum members is slow

In [50]: %timeit Qt.Key.Key_Control == Qt.Key.Key_Control 1.86 µs ± 20.1 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [51]: %timeit hash(event_key) 54.8 ns ± 2.46 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each) In [52]: %timeit hash(Qt.Key.Key_Control) 958 ns ± 15.8 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each) In [53]: %timeit event_key == event_key 34.2 ns ± 0.577 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

So everything that comes from python data structures is not important in access to qt enums.

So only style left.

ok, I can put this set in a constant then

brisvag

Marking red to avoid accidental merge, since it depends on an external pr to be merged and incorporated as dependency.

Czaki · 2023-03-07T09:53:55Z

Why test are passing in cases when this should not work with not fixed app-model?

psobolewskiPhD · 2023-03-07T10:10:03Z

Why test are passing in cases when this should not work with not fixed app-model?

Same reason they pass without this PR at all? 🤔

Czaki · 2023-03-07T10:29:00Z

Same reason they pass without this PR at all? 🤔

So it means that this PR does not contain proper test? 🤔

kne42 · 2023-03-07T17:29:06Z

So it means that this PR does not contain proper test? 🤔

For the symbol swapping I can create tests since that would be fairly trivial (with patching I think). I was also thinking of straight-up porting this logic to app-model (see this issue), and we could test there, what do you think of doing that at the cost of this PR taking a bit longer? We could then make a release on app-model for that and the other dependency for this PR since they're related and not stagger dependency bumping.

The GUI aspect is a bit more difficult to test but I could try covering it. Much of the codebase lacks proper GUI tests.

Carreau · 2023-03-08T14:25:20Z

Does that also includes fixes of #5064 ?

kne42 · 2023-03-08T18:31:01Z

It does not, but I can look into porting that over too.

kne42 · 2023-03-08T18:36:59Z

it does fix #5047 however

Carreau · 2023-03-09T09:34:47Z

It does not, but I can look into porting that over too.

That's fine, I can rebase later. I would just hope that there were more review/merge of pending PRs, as otherwise things like this keep happening, where work conflicts and we need to keep rebasing.

Czaki · 2023-03-09T10:15:18Z

I would just hope that there were more review/merge of pending PRs

please ask for a specific PR on zulip #reviews stream. Most of us spend free/unpaid time on maintenance napari and it is easy to lose some PR.

Carreau · 2023-03-13T18:20:41Z

please ask for a specific PR on zulip #reviews stream. Most of us spend free/unpaid time on maintenance napari and it is easy to lose some PR.

Sorry if I made you feel you were not doing enough, I know the feeling and I am doing my best to also help unblock by reviewing/rebasing fixing trivial mistake in CI.

kne42 · 2023-03-29T18:14:39Z

Hi all, sorry for the delay, I've been on vacation and just got back. It looks like @Czaki was advocating for more GUI test coverage, would you still like this prior to merge (since it will further delay the bugfix)? Also are there any opinions on moving some of the symbol rendering to app-model and making sure it's tested on that end?

Dependency was merged.

brisvag · 2023-03-30T07:59:38Z

I think we're fine without gui-specific tests... The think we should make sure we test is that if someone changes something elsewhere we don't suddenly lose some of this stuff.

Czaki · 2023-03-30T11:53:31Z

This PR mainly changes GUI code. And as it is not covered, then the core-dev who accepts it needs to perform manual testing. I may find time for this during weekend.

kne42 · 2023-05-19T21:56:08Z

Hi @Czaki, I wanted to check if you've had an opportunity to give this a test and/or if you still would like to see GUI tests in this PR!

jaimergp · 2023-05-22T08:45:09Z

Hi @kne42, I talked with @Czaki about this last Friday and the consensus is that having a test would be ideal. Local checks have been successful, but we need an automated method to prevent regressions in the future.

I am happy to assist / take over if you need help, just let me know!

Carreau · 2023-05-30T09:50:54Z

~~I've tried it locally on Mac, and it seem some logic is wrong.~~

~~Pressing ⌘E for a shortcut ends up assigning it to ^E, vice versa.~~

Carreau · 2023-05-30T09:56:55Z

Note that on main, trying to Edit a shortcut press say ⌘E, ends up while editing with Control+E, with Control fully spelled, while with this PR this become Ctrl-E, with the short spelling of Ctrl.

~~There also seem to not work with existing custom shortcuts, i.e. if on main I set a shortcut to ⌘⇧D and checkout this PR, the shortcut appear empty in the preference windows,~~ (never mind)

Carreau · 2023-05-30T11:54:44Z

Ok it seem for whatever reason my local install was not picking up the newer App-Model and it does work.

I've found another bug though, I can't assign the ⌘E shortcut as alt-E is the shortcut on international keyboard for the ´ combining key, and seem to affect any other combining keys do you want me to open another issue for this ?

kne42 · 2023-06-12T21:27:18Z

I've found another bug though, I can't assign the ⌘E shortcut as alt-E is the shortcut on international keyboard for the ´ combining key, and seem to affect any other combining keys do you want me to open another issue for this ?

yes please, if im understanding correctly, you want to set the shortcut as ⌘E but it becomes ´ instead? seems like it could get pretty hairy if that's the case

I am happy to assist / take over if you need help, just let me know!

@jaimergp I am super stressed/busy right now so any help with the GUI tests would be greatly appreciated!

jni · 2023-06-22T05:45:27Z

Just checked and this works locally once app-model is updated. Alt-E is a problem but Cmd-Alt-E is not, and Alt-E is also a problem on main, so I suggest we merge this. (Optionally after merging in @Czaki's suggestion of changing list to set.)

While typing it shows "Meta-E" or "Meta-Alt-E", it would be cool if the Mac-native symbols appeared while typing but that can be updated later imho. @Czaki I also think this should go in 0.4.18, I've added the milestone accordingly, but of course you can modify it if you disagree.

psobolewskiPhD · 2023-06-22T07:02:00Z

While typing it shows "Meta-E" or "Meta-Alt-E", it would be cool if the Mac-native symbols appeared while typing but that can be updated later imho.

@jni For me, this PR does that: #5064 but I'm not sure how it interacts with this one.

@Czaki I also think this should go in 0.4.18, I've added the milestone accordingly, but of course you can modify it if you disagree.

This PR fixes issue related to #5103 which has 0.5.0 milestone, so that one would also need back porting...

Carreau · 2023-06-22T07:13:27Z

so that one would also need back porting...

If you backport a lot, I can remind you of my backport bot

Czaki · 2023-06-22T07:25:11Z

This PR fixes issue related to #5103 which has 0.5.0 milestone, so that one would also need back porting...

but I do not backport anything related to app model keys.

This PR addresses bugs where the improper symbols would be displayed for e.g. ⌘V on Macs, as well as improper capturing on Macs where e.g. Control would be read as Meta and vice versa. This relies on an upstream fix in `app-model` and uses the logic there to properly capture the correct modifier keys, regardless of if Qt is switching Control with Meta or not. fixes napari#5047 fixes napari#5332 fixes napari#5579 depends on pyapp-kit/app-model#82

kne42 added dependencies Pull requests that update a dependency file bugfix PR with bugfix labels Mar 5, 2023

github-actions bot added the qt Relates to qt label Mar 5, 2023

github-actions bot assigned kne42 Mar 5, 2023

Czaki reviewed Mar 6, 2023

View reviewed changes

psobolewskiPhD approved these changes Mar 6, 2023

View reviewed changes

brisvag previously requested changes Mar 7, 2023

View reviewed changes

Carreau mentioned this pull request Mar 28, 2023

Bugfix: improve layout of Preferences > Shortcuts tables #5679

Merged

12 tasks

kne42 added 4 commits March 30, 2023 09:19

fix various keybinding bugs

ea3cc55

shorten import

bc444f5

remove unused key subs

6d1ac7c

bump app-model version

a5df12d

kne42 force-pushed the kb-bugfixes branch from 5672760 to a5df12d Compare March 30, 2023 16:21

psobolewskiPhD mentioned this pull request May 3, 2023

Changing brush size with mouse movement while holding specified modifiers #5753

Merged

7 tasks

psobolewskiPhD mentioned this pull request May 5, 2023

Show platform specific shortcut while editing. #5064

Merged

12 tasks

kne42 mentioned this pull request May 26, 2023

npe2 TODOs #5874

Open

12 tasks

Carreau approved these changes May 30, 2023

View reviewed changes

Carreau mentioned this pull request Jun 13, 2023

Setting shortcuts and Combining keys has weird behavior. #5930

Open

jni approved these changes Jun 22, 2023

View reviewed changes

jni added bugfix PR with bugfix and removed bugfix PR with bugfix labels Jun 22, 2023

jni added this to the 0.4.18 milestone Jun 22, 2023

jni modified the milestones: 0.4.18, 0.5.0 Jun 22, 2023

jni merged commit 29a19c1 into napari:main Jun 22, 2023
36 checks passed

Czaki mentioned this pull request Jun 23, 2023

v0.4.18 - release process #5911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix various keybinding/shortcut bugs #5604

fix various keybinding/shortcut bugs #5604

kne42 commented Mar 5, 2023 •

edited

codecov bot commented Mar 5, 2023 •

edited

psobolewskiPhD commented Mar 5, 2023

Czaki Mar 6, 2023

kne42 Mar 7, 2023

Czaki Mar 30, 2023

kne42 Mar 30, 2023

brisvag left a comment

Czaki commented Mar 7, 2023

psobolewskiPhD commented Mar 7, 2023

Czaki commented Mar 7, 2023

kne42 commented Mar 7, 2023

Carreau commented Mar 8, 2023

kne42 commented Mar 8, 2023

kne42 commented Mar 8, 2023

Carreau commented Mar 9, 2023

Czaki commented Mar 9, 2023

Carreau commented Mar 13, 2023

kne42 commented Mar 29, 2023

brisvag commented Mar 30, 2023

Czaki commented Mar 30, 2023

kne42 commented May 19, 2023

jaimergp commented May 22, 2023

Carreau commented May 30, 2023 •

edited

Carreau commented May 30, 2023 •

edited

Carreau commented May 30, 2023

kne42 commented Jun 12, 2023

jni commented Jun 22, 2023

psobolewskiPhD commented Jun 22, 2023

Carreau commented Jun 22, 2023

Czaki commented Jun 22, 2023

fix various keybinding/shortcut bugs #5604

fix various keybinding/shortcut bugs #5604

Conversation

kne42 commented Mar 5, 2023 • edited

codecov bot commented Mar 5, 2023 • edited

Codecov Report

psobolewskiPhD commented Mar 5, 2023

Czaki Mar 6, 2023

Choose a reason for hiding this comment

kne42 Mar 7, 2023

Choose a reason for hiding this comment

Czaki Mar 30, 2023

Choose a reason for hiding this comment

kne42 Mar 30, 2023

Choose a reason for hiding this comment

brisvag left a comment

Choose a reason for hiding this comment

Czaki commented Mar 7, 2023

psobolewskiPhD commented Mar 7, 2023

Czaki commented Mar 7, 2023

kne42 commented Mar 7, 2023

Carreau commented Mar 8, 2023

kne42 commented Mar 8, 2023

kne42 commented Mar 8, 2023

Carreau commented Mar 9, 2023

Czaki commented Mar 9, 2023

Carreau commented Mar 13, 2023

kne42 commented Mar 29, 2023

brisvag commented Mar 30, 2023

Czaki commented Mar 30, 2023

kne42 commented May 19, 2023

jaimergp commented May 22, 2023

Carreau commented May 30, 2023 • edited

Carreau commented May 30, 2023 • edited

Carreau commented May 30, 2023

kne42 commented Jun 12, 2023

jni commented Jun 22, 2023

psobolewskiPhD commented Jun 22, 2023

Carreau commented Jun 22, 2023

Czaki commented Jun 22, 2023

kne42 commented Mar 5, 2023 •

edited

codecov bot commented Mar 5, 2023 •

edited

Carreau commented May 30, 2023 •

edited

Carreau commented May 30, 2023 •

edited