Show partial shortcut while editing. #6050

Carreau · 2023-07-09T11:33:34Z

And automatically accept/validate shortcuts when valid.

When receiving a modifier only event, use the event Modifiers to show the current partial shortcuts.
we need to handle keyrelease events otherwise the following sequence is incorrect.

Action          –  Shortcut shown
---------------------------------
Press Ctrl      –  Ctrl
Also Press Alt  –  Ctrl-Alt
Release Ctrl    –  Ctrl-Alt

Though now that we handle release, the user cant press a shortcut and then validate with enter as as soon as the user release the shortcut the QLineEdit will become empty. But we don't care as we only want full shortcuts and not modifiers only.

Thus validate as soon a shortcut is valid, and contain a non-modifier key.

There is also this whole Meta/Alt/Ctrl are not the same on MacOS. I did not extract the logic into naprai/utils/interactions as those are Qt Specific, moreover the values of QtEvent.Modifiers are not the same than Qt.Keys.

You will note that there is one weirdness in the order of modifiers when typing these, but I believe this will be fixed upstream. This is due to the fact that when pressing Ctrl then Shift for example, Shift is the Key event, and this will appear at the end. While when doing Shift, then Ctrl, ctrl is the key that will appear at the end.

Closes #6047

Type of change

New feature (non-breaking change which adds functionality)

How has this been tested?

This needs to be tested on more platforms.

Carreau · 2023-07-09T11:36:48Z

In this video:

playing with modifiers to show they update.
Assinignig Ctrl-Shift-Meta-Cmd-T as a modifier.
Then using Alt-Cmd-Return to assign Alt-Cmd modifiers only shortcut.

Untitled.mov

Carreau · 2023-07-09T11:38:20Z

@jni I think you were asking for this also, and were excited about shortcuts editor showing modifiers.

codecov · 2023-07-09T11:52:16Z

Codecov Report

Merging #6050 (8b93178) into main (3cbd0d0) will increase coverage by 0.02%.
Report is 18 commits behind head on main.
The diff coverage is 24.13%.

@@            Coverage Diff             @@
##             main    #6050      +/-   ##
==========================================
+ Coverage   91.62%   91.64%   +0.02%     
==========================================
  Files         579      580       +1     
  Lines       50663    50858     +195     
==========================================
+ Hits        46419    46609     +190     
- Misses       4244     4249       +5

Files Changed	Coverage Δ
napari/_qt/widgets/qt_keyboard_settings.py	`68.47% <24.13%> (-4.10%)`	⬇️

... and 19 files with indirect coverage changes

Carreau · 2023-07-10T11:46:30Z

Note that with napari/docs#200 we should be able to drop modifiers only shortcuts.

psobolewskiPhD · 2023-07-13T01:31:29Z

This looks great, but in your example assigning alt-meta doesn't actually toggle theme?
Personally, I don't think that should be valid—modifiers should modify something: a key or a mouse action—so it's not a big deal, but still a bit odd.

Carreau · 2023-07-13T08:12:24Z

This looks great, but in your example assigning alt-meta doesn't actually toggle theme?
Personally, I don't think that should be valid—modifiers should modify something: a key or a mouse action—so it's not a big deal, but still a bit odd.

I completely agree, but this pull request is not to discuss whether modifiers only keybindings are good to have. I agree they are not and nap7 also says so. It's here just to fix the bug that modifier.

In the default shortcuts there is at least one "Reset Scroll" that is just a modifier (Ctrl), so my goal is just to let people enter shortcuts that are already assigned, and thus contain only modifiers.

psobolewskiPhD · 2023-07-13T22:53:46Z

I understand. But being able to assign alt-meta but then it not working is a bit strange
main doesn't allow alt-meta, just alt or meta, but either actually work to toggle the theme.
Edit: with this PR for example alt alone does work, but alt-meta which can be bound doesn't work to toggle theme.

Carreau · 2023-07-26T08:54:50Z

Was there any actual decisions ? I'm happy to remove the code that allows to bind only modifiers.

psobolewskiPhD · 2023-07-28T00:44:57Z

Very wierd, I still can't get alt-meta I can't get to work on my mac.
But control-shift does work—it's just touchy on the timing, because it's basically Control as the modifier, shift as the keybind.
Anyhow, single/solo modifiers have been around since forevers, so unless the NAP kills them, they should definitely remain IMO. The double modifiers-only that are now possible are new—0.4.18 doesn't let them work—and a bit weird, but do seem to mostly work?
Still I'd consider preventing them, because they arn't currently possible which would leave this as a UI change PR only.

Carreau · 2023-08-04T09:26:27Z

I remove the explicit handling and messaging around Return and Modifiers only.

psobolewskiPhD · 2023-08-07T23:07:29Z

So I'm confused by the current implementation.

While the info text at the bottom no longer mentions confirming via return, double modifiers like alt-meta on macOS can still be bound (but don't actually work) using return.
Likewise single modifiers can still be bound using return—these have been around for a long time in napari and include some defaults, so barring a NAP I wouldn't remove—and they do work.

And automatically accept/validate shortcuts when valid. - When receiving a modifier only event, use the event Modifiers to show the current partial shortcuts. - we need to handle keyrelease events otherwise the following sequence is incorrect. ``` Action – Shortcut shown --------------------------------- Press Ctrl – Ctrl Also Press Alt – Ctrl-Alt Release Ctrl – Ctrl-Alt ``` Though now that we handle release, the user cant press a shortcut and then validate with enter as as soon as the user release the shortcut the QLineEdit will become empty. Thus validate as soon a shortcut is valid, and contain a non-modifier key. This in turn make it impossible to assign a modifiers only keybinding, I assume this is quite rare, but we can use on of the keys that are invalid as shortcut to validate modifiers only shortcuts. I choose Qt.Return, aka `Enter`. I'm not quite confident on the Return/Enter, and the way to assign modifiers only shortcuts. There is also this whole Meta/Alt/Ctrl are not the same on MacOS. I did not extract the logic into naprai/utils/interactions as those are Qt Specific, moreover the values of QtEvent.Modifiers are not the same than Qt.Keys. You will note that there is one weirdness in the order of modifiers when typing these, but I believe this will be fixed upstream. Closes napari#6047

Carreau · 2023-08-08T13:07:06Z

Something fishy is going on with keybinding anyway. I don't know why Return still allows to bind as it's clearly in the path to prevent not accept the modification.

But I added an extra layer that seem to mostly do the trick.

I still managed to break my installation setting by pressing some combinaison of modifiers and tab/escape at the point that napari did not want to load settings.

I don't quite understand what is going on but I'm going su assume that there is some QLineEdit magic and it does some things on focus and and we are trying to shove a square peg into a round hole.

Carreau · 2023-08-08T13:07:37Z

napari/_qt/widgets/qt_keyboard_settings.py

@@ -352,7 +354,7 @@ def _show_bind_shortcut_error(

        message = trans._(
            "<b>{new_shortcut}</b> is not a valid keybinding.",
-            new_shortcut=new_shortcut,
+            new_shortcut=Shortcut(new_shortcut).platform,


Show platform specific error messages.

Carreau · 2023-08-08T13:08:34Z

rebased on master also as there was some weird things going on with Ctrl/Cmd on mac that would sometime display improperly. After rebase this seem fine.

psobolewskiPhD

looks good now, thanks @Carreau

kne42

great work, LGTM!

napari/_qt/widgets/qt_keyboard_settings.py

Co-authored-by: Kira Evans <contact@kne42.me>

Carreau · 2023-08-20T08:48:02Z

thanks !

github-actions bot assigned Carreau Jul 9, 2023

github-actions bot added the qt Relates to qt label Jul 9, 2023

Carreau added the feature New feature or request label Jul 9, 2023

Carreau added this to the 0.5 milestone Jul 9, 2023

Carreau marked this pull request as ready for review July 12, 2023 11:32

Carreau force-pushed the short-2 branch from 8217b6d to 7c67c1c Compare August 4, 2023 09:21

Carreau added 3 commits August 8, 2023 14:39

remove ability to add modifiers only shortcuts

aa6c11b

prevent non-single modifiers

1451916

Carreau force-pushed the short-2 branch from 7c67c1c to 1451916 Compare August 8, 2023 13:02

Carreau commented Aug 8, 2023

View reviewed changes

psobolewskiPhD requested a review from kne42 August 11, 2023 02:41

psobolewskiPhD approved these changes Aug 11, 2023

View reviewed changes

kne42 approved these changes Aug 18, 2023

View reviewed changes

napari/_qt/widgets/qt_keyboard_settings.py Outdated Show resolved Hide resolved

psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Aug 19, 2023

Update napari/_qt/widgets/qt_keyboard_settings.py

8b93178

Co-authored-by: Kira Evans <contact@kne42.me>

melonora merged commit c3054e8 into napari:main Aug 20, 2023
35 checks passed

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show partial shortcut while editing. #6050

Show partial shortcut while editing. #6050

Carreau commented Jul 9, 2023 •

edited

Carreau commented Jul 9, 2023

Carreau commented Jul 9, 2023

codecov bot commented Jul 9, 2023 •

edited

Carreau commented Jul 10, 2023

psobolewskiPhD commented Jul 13, 2023

Carreau commented Jul 13, 2023

psobolewskiPhD commented Jul 13, 2023 •

edited

Carreau commented Jul 26, 2023

psobolewskiPhD commented Jul 28, 2023

Carreau commented Aug 4, 2023

psobolewskiPhD commented Aug 7, 2023

Carreau commented Aug 8, 2023

Carreau Aug 8, 2023

Carreau commented Aug 8, 2023

psobolewskiPhD left a comment

kne42 left a comment

Carreau commented Aug 20, 2023 via email

Show partial shortcut while editing. #6050

Show partial shortcut while editing. #6050

Conversation

Carreau commented Jul 9, 2023 • edited

Type of change

How has this been tested?

Carreau commented Jul 9, 2023

Carreau commented Jul 9, 2023

codecov bot commented Jul 9, 2023 • edited

Codecov Report

Carreau commented Jul 10, 2023

psobolewskiPhD commented Jul 13, 2023

Carreau commented Jul 13, 2023

psobolewskiPhD commented Jul 13, 2023 • edited

Carreau commented Jul 26, 2023

psobolewskiPhD commented Jul 28, 2023

Carreau commented Aug 4, 2023

psobolewskiPhD commented Aug 7, 2023

Carreau commented Aug 8, 2023

Carreau Aug 8, 2023

Choose a reason for hiding this comment

Carreau commented Aug 8, 2023

psobolewskiPhD left a comment

Choose a reason for hiding this comment

kne42 left a comment

Choose a reason for hiding this comment

Carreau commented Aug 20, 2023 via email

Carreau commented Jul 9, 2023 •

edited

codecov bot commented Jul 9, 2023 •

edited

psobolewskiPhD commented Jul 13, 2023 •

edited