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

Accept individual modifier keys as valid keybindings #637

Merged
merged 48 commits into from
Apr 4, 2024

Conversation

g547315
Copy link
Contributor

@g547315 g547315 commented Sep 25, 2023

References

#15090

  • Trigger functionalities on key down and key up of Alt or Alt + Shift

Code changes

Allow the key down event listener to return a mod key or combination of mods to be processed and matched to its keybinding and the keyboard shortcut associated with it

Tests have been changed to reflect the new functionality of mod keys. As mod keys now have additional functionality, pressing a mod key will break the sequence when entering a key sequence i.e. [Shift K, Shift L]. The test 'should ignore modifier keys pressed in the middle of key sequence' has been changed to reflect this. A similar change has been made to 'should process key sequences that use different modifier keys' to remove unnecessary keystrokes.

'should return nothing for keys that are marked as modifier in keyboard layout' has been modified as mod keys now return keyboard events.

To see the full interned functionality please combine this PR with:

  • Added and styled overlay UI element #633

User-facing changes

None

Backwards-incompatible changes

None

@g547315
Copy link
Contributor Author

g547315 commented Sep 25, 2023

Copy link
Contributor

@brichet brichet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @g547315.

I have some comments below.

Would it be possible to wait for a delay before returning the modifier keys combination (when there are only modifier keys pressed) ?
That could avoid triggering the event linked to that modifier key, while only using the modifier in a shortcut.

packages/commands/src/index.ts Outdated Show resolved Hide resolved
packages/commands/src/index.ts Outdated Show resolved Hide resolved
packages/commands/src/index.ts Show resolved Hide resolved
@tonyfast
Copy link

should these modify combinations be configurable?

@g547315
Copy link
Contributor Author

g547315 commented Oct 3, 2023

should these modify combinations be configurable?

could you elaborate please
@tonyfast

@g547315 g547315 requested a review from brichet October 3, 2023 15:52
@tonyfast
Copy link

tonyfast commented Oct 3, 2023

should these modify combinations be configurable?

modifier keys for assistive technology vary with locale, operating system, and vendor. there might be edge cases where this specific modifier combination is challenge for certain users. the most assistive solution would allow for configurable modifier key codes with Alt + Shift as the default.

@g547315 g547315 mentioned this pull request Oct 3, 2023
@krassowski
Copy link
Member

The failure is relevant:

Run for file in review/api/application.api.md review/api/commands.api.md; do
review/api/application.api.md
review/api/commands.api.md

Needs running yarn run api and committing changes

Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>
@g547315
Copy link
Contributor Author

g547315 commented Dec 14, 2023

@brichet Thanks for the second pair of eyes and the contributions to the PR.
Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.

Mod keys bindings are being detected and matched, however they are not executed consistently or at all

  • Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

@brichet
Copy link
Contributor

brichet commented Dec 14, 2023

@brichet Thanks for the second pair of eyes and the contributions to the PR. Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.

Mod keys bindings are being detected and matched, however they are not executed consistently or at all

  • Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

This is nice for testing @g547315.

It works well on my side, when holding the key for 1s (it currently uses the CHORD_TIMEOUT variable).

Peek 2023-12-14 18-56

Let's create a new variable with a shorter delay for this feature.

@g547315
Copy link
Contributor Author

g547315 commented Dec 15, 2023

@brichet Thanks for the second pair of eyes and the contributions to the PR. Having tested the code change via my dev environment and this binder link Link (Please click the launch binder button) I can't seem to consistently see the intended functionality.
Mod keys bindings are being detected and matched, however they are not executed consistently or at all

  • Please note the binder instance has styled overlay UI element that should be displayed when Mod keys bindings are executed

This is nice for testing @g547315.

It works well on my side, when holding the key for 1s (it currently uses the CHORD_TIMEOUT variable).

Peek 2023-12-14 18-56 Peek 2023-12-14 18-56

Let's create a new variable with a shorter delay for this feature.

New variable added and set to 500ms

@g547315 g547315 requested a review from brichet December 15, 2023 15:10
@g547315 g547315 closed this Dec 15, 2023
@g547315 g547315 reopened this Dec 15, 2023
review/api/widgets.api.md Outdated Show resolved Hide resolved
@g547315
Copy link
Contributor Author

g547315 commented Mar 28, 2024

This pull request should to be ready for merging. However, I am unable to continue working on it after March 28, 2024. If further refinements are identified, I welcome any future contributors to pick up and progress the work

@brichet
Copy link
Contributor

brichet commented Mar 28, 2024

Thanks again @g547315 for working on this.

Since I worked on it, it would be nice to have another opinion before merging.
@krassowski do you think we can merge this PR ?

this._startModifierTimer(exact);
} else {
// Otherwise stop potential existing timer.
this._clearModifierTimer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the ping @brichet!

Should the modifier timer also be stopped if an exact match for a chord is found?

For example if the sequence is Ctrl Z (not Ctrl + Z but Ctrl followed by Z) we would get two events (one for Ctrl, one for Z). My impression from just reading the code (I could be wrong) is that currently it would trigger both the handler for Ctrl and for the Ctrl followed by Z chord.

In other words, should _clearModifierTimer be called in _clearPendingState too?

Copy link
Contributor

@brichet brichet Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, but this behavior seems OK if this feature is only used to display overlays.
You may want to keep the modifier key(s) pressed (and the overlays visible) to trigger another shortcut.

If we keep it that way, we should probably document it somewhere, to avoid using this modifier key(s) for unexpected action.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to keep the modifier key(s) pressed (and the overlays visible) to trigger another shortcut.

Yes, but I also do not want to have the overlay visible ("sticky") after I already pressed the combo I was interested in. I think if user is pressing two shortcuts and they need overlays, say Ctrl + C and Ctrl + V it is fine it they do that in sequence:

  • Ctrl: shows overlay
  • Ctrl + C: overlay hides immediately
  • Ctrl: shows overlay again
  • Ctrl + V: overlay hides immediately again

Copy link
Contributor

@brichet brichet Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the overlay visible or not depends on the command that will be triggered.
Here it is only about the timeout before triggering the command, so you're right we better clear it if an other command is triggered. BTW the timeout is also cleared on key up event with this PR.
I updated it.

@krassowski krassowski changed the title extend keystrokes to mod keys Accept individual modifier keys as valid keybindings Mar 30, 2024
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@krassowski krassowski merged commit ededf7f into jupyterlab:main Apr 4, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants