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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
c82fc4a
extend keystrokes to mod keys
g547315 Sep 25, 2023
835f745
fix doubled keys
g547315 Oct 3, 2023
b9d2db3
remove comments
g547315 Oct 3, 2023
12f6921
remove console-log
g547315 Oct 4, 2023
77330ad
Merge branch 'jupyterlab:main' into allow-mapping-to-mod-keys
m158261 Oct 12, 2023
6cfdf12
Removed whitespace from keystrokes
m158261 Oct 17, 2023
2167496
Run prettier
m158261 Oct 17, 2023
d9ddb6c
Updated normalizeKeystroke to account for empty string
m158261 Oct 17, 2023
4efbfc7
Check for empty string in key
m158261 Oct 17, 2023
5e5e65c
Check for empty string
m158261 Oct 17, 2023
d4e384c
Checking for whitespace
m158261 Oct 17, 2023
36fcbad
Trim whitespace
m158261 Oct 17, 2023
d520616
Added check for keys
m158261 Oct 19, 2023
4926fd0
refactor add space for puley modifier key
g547315 Oct 20, 2023
9bf4c47
Changed tests to reflect mod keys creating events
m158261 Oct 20, 2023
85a1362
Changed tests to match key bindings
m158261 Oct 20, 2023
5ed4103
Revert test changes
m158261 Oct 20, 2023
d9a3f6f
remove added white space
g547315 Oct 20, 2023
3ebd720
Merge branch 'jupyterlab:main' into allow-mapping-to-mod-keys
g547315 Oct 20, 2023
ab4c7bf
Merge branch 'allow-mapping-to-mod-keys' of ssh://ssh.github.com:443/…
g547315 Oct 20, 2023
c248e7a
Removed ctrl key from test to check functionality
m158261 Oct 23, 2023
10e25fc
Modified test
m158261 Oct 23, 2023
a89b601
delay modifier key only execution
g547315 Oct 31, 2023
7818bf5
Merge branch 'allow-mapping-to-mod-keys' of ssh://ssh.github.com:443/…
g547315 Oct 31, 2023
7ffa17c
Update packages/commands/src/index.ts
g547315 Nov 8, 2023
d115929
re-write
g547315 Nov 9, 2023
203c38a
removed redundantcode
g547315 Nov 9, 2023
1008417
Ran Prettier to fix failing test
g547315 Nov 9, 2023
e773b5e
Update packages/commands/src/index.ts
g547315 Nov 14, 2023
ef7bc6f
Refactored func and test
g547315 Nov 16, 2023
2cca0ab
Merge branch 'main' into allow-mapping-to-mod-keys
g547315 Dec 4, 2023
5a1bfd9
modifier key bindings trigger on key holds
g547315 Dec 4, 2023
5e13b06
reverted changed test
g547315 Dec 5, 2023
4468167
undo reverted test changes
g547315 Dec 7, 2023
109298a
Ran Prettier
g547315 Dec 7, 2023
b004546
Starts the timer for mod keys only before updating the keystrokes list
brichet Dec 12, 2023
dd5451b
Merge pull request #1 from brichet/modifier-timer
g547315 Dec 13, 2023
843012b
Merge branch 'main' into allow-mapping-to-mod-keys
brichet Dec 13, 2023
088a379
Update packages/commands/src/index.ts
g547315 Dec 14, 2023
d670fce
added new variable with a shorter delay
g547315 Dec 15, 2023
7d385f6
Merge branch 'allow-mapping-to-mod-keys' of ssh://ssh.github.com:443/…
g547315 Dec 15, 2023
57d8170
ran yarn run api and committing changes
g547315 Dec 15, 2023
b3c4237
ran linter
g547315 Dec 15, 2023
aef9685
refactor variable to camelcase
g547315 Mar 11, 2024
1873564
refactor variable to camelcase
g547315 Mar 12, 2024
4b67ed0
updated api
g547315 Mar 13, 2024
c8b1f39
Merge branch 'main' into allow-mapping-to-mod-keys
g547315 Mar 21, 2024
73873be
Clear modifier timeout if an exact match is found
brichet Apr 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/application/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,9 @@ export class Application<T extends Widget = Widget> {
case 'keydown':
this.evtKeydown(event as KeyboardEvent);
break;
case 'keyup':
this.evtKeyup(event as KeyboardEvent);
break;
case 'contextmenu':
this.evtContextMenu(event as PointerEvent);
break;
Expand Down Expand Up @@ -610,6 +613,7 @@ export class Application<T extends Widget = Widget> {
protected addEventListeners(): void {
document.addEventListener('contextmenu', this);
document.addEventListener('keydown', this, !this._bubblingKeydown);
document.addEventListener('keyup', this, !this._bubblingKeydown);
window.addEventListener('resize', this);
}

Expand All @@ -626,6 +630,19 @@ export class Application<T extends Widget = Widget> {
this.commands.processKeydownEvent(event);
}

/**
* A method invoked on a document `'keyup'` event.
*
* #### Notes
* The default implementation of this method invokes the key up
* processing method of the application command registry.
*
* A subclass may reimplement this method as needed.
*/
protected evtKeyup(event: KeyboardEvent): void {
this.commands.processKeyupEvent(event);
}

/**
* A method invoked on a document `'contextmenu'` event.
*
Expand Down
77 changes: 66 additions & 11 deletions packages/commands/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -511,13 +511,8 @@ export class CommandRegistry {
* events will not invoke commands.
*/
processKeydownEvent(event: KeyboardEvent): void {
// Bail immediately if playing back keystrokes
// or if the event has been processed
if (
event.defaultPrevented ||
this._replaying ||
CommandRegistry.isModifierKeyPressed(event)
) {
// Bail immediately if playing back keystrokes.
if (event.defaultPrevented || this._replaying) {
return;
}

Expand All @@ -532,6 +527,26 @@ export class CommandRegistry {
return;
}

// Check that only mod key(s) have been pressed.
if (CommandRegistry.isModifierKeyPressed(event)) {
// Find the exact match for the modifier keys.
let { exact } = Private.matchKeyBinding(
this._keyBindings,
[keystroke],
event
);
if (exact) {
// If the mod keys match an exact shortcut, start a dedicated timer.
event.preventDefault();
event.stopPropagation();
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.

}
return;
}

// Add the keystroke to the current key sequence.
this._keystrokes.push(keystroke);

Expand Down Expand Up @@ -599,6 +614,36 @@ export class CommandRegistry {
this._holdKeyBindingPromises.set(event, permission);
}

/**
* Process a ``keyup`` event to clear the timer on the modifier, if it exists.
*
* @param event - The event object for a `'keydown'` event.
*/
processKeyupEvent(event: KeyboardEvent): void {
this._clearModifierTimer();
}

/**
* Start or restart the timeout on the modifier keys.
*
* This timeout will end only if the keys are hold.
*/
private _startModifierTimer(exact: CommandRegistry.IKeyBinding): void {
this._clearModifierTimer();
this._timerModifierID = window.setTimeout(() => {
this._executeKeyBinding(exact);
}, Private.modifierkeyTimeOut);
}

/**
* Clear the timeout on modifier keys.
*/
private _clearModifierTimer(): void {
if (this._timerModifierID !== 0) {
clearTimeout(this._timerModifierID);
this._timerModifierID = 0;
}
}
/**
* Start or restart the pending timeout.
*/
Expand Down Expand Up @@ -688,6 +733,7 @@ export class CommandRegistry {
*/
private _clearPendingState(): void {
this._clearTimer();
this._clearModifierTimer();
this._exactKeyMatch = null;
this._keystrokes.length = 0;
this._keydownEvents.length = 0;
Expand All @@ -707,6 +753,7 @@ export class CommandRegistry {
}

private _timerID = 0;
private _timerModifierID = 0;
private _replaying = false;
private _keystrokes: string[] = [];
private _keydownEvents: KeyboardEvent[] = [];
Expand Down Expand Up @@ -1245,6 +1292,9 @@ export namespace CommandRegistry {
if (parts.cmd && Platform.IS_MAC) {
mods += 'Cmd ';
}
if (!parts.key) {
return mods.trim();
}
return mods + parts.key;
}

Expand Down Expand Up @@ -1328,9 +1378,6 @@ export namespace CommandRegistry {
export function keystrokeForKeydownEvent(event: KeyboardEvent): string {
let layout = getKeyboardLayout();
let key = layout.keyForKeydownEvent(event);
if (!key || layout.isModifierKey(key)) {
g547315 marked this conversation as resolved.
Show resolved Hide resolved
return '';
}
let mods = [];
if (event.ctrlKey) {
mods.push('Ctrl');
Expand All @@ -1344,7 +1391,10 @@ export namespace CommandRegistry {
if (event.metaKey && Platform.IS_MAC) {
mods.push('Cmd');
}
mods.push(key);
if (!layout.isModifierKey(key)) {
mods.push(key);
}
// for purely modifier key strings
return mods.join(' ');
}
}
Expand All @@ -1363,6 +1413,11 @@ namespace Private {
*/
export const KEYBINDING_HOLD_TIMEOUT = 1000;

/**
* The timeout in ms for triggering a modifer key binding.
*/
export const modifierkeyTimeOut = 500;

/**
* A convenience type alias for a command func.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/commands/tests/src/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1508,11 +1508,11 @@ describe('@lumino/commands', () => {
expect(keystroke).to.equal('');
});

it('should return nothing for keys that are marked as modifier in keyboard layout', () => {
it('should return keys that are marked as modifier in keyboard layout', () => {
let keystroke = CommandRegistry.keystrokeForKeydownEvent(
new KeyboardEvent('keydown', { keyCode: 17, ctrlKey: true })
);
expect(keystroke).to.equal('');
expect(keystroke).to.equal('Ctrl');
});
});
});
Expand Down
1 change: 1 addition & 0 deletions review/api/application.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export class Application<T extends Widget = Widget> {
deregisterPlugin(id: string, force?: boolean): void;
protected evtContextMenu(event: PointerEvent): void;
protected evtKeydown(event: KeyboardEvent): void;
protected evtKeyup(event: KeyboardEvent): void;
protected evtResize(event: Event): void;
getPluginDescription(id: string): string;
handleEvent(event: Event): void;
Expand Down
1 change: 1 addition & 0 deletions review/api/commands.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export class CommandRegistry {
mnemonic(id: string, args?: ReadonlyPartialJSONObject): number;
notifyCommandChanged(id?: string): void;
processKeydownEvent(event: KeyboardEvent): void;
processKeyupEvent(event: KeyboardEvent): void;
usage(id: string, args?: ReadonlyPartialJSONObject): string;
}

Expand Down
Loading