keybindings: add context menu to keyboard shortcuts #12791

pisv · 2023-08-01T12:04:37Z

What it does

Fixes the bulk of #7582.

Adds a context menu to the Keyboard Shorcuts editor with the following menu items:

Copy
Copy Command ID
Copy Command Title
Edit Keybinding...
Edit When Expression...
Add Keybinding...
Remove Keybinding
Reset Keybinding
Show Same Keybindings

This PR does not address the bonus part of #7582, i.e. no keybindings are provided for the menu items.

How to test

Verify that each of the new context menu items works as expected, i.e. similar to how it works in VS Code.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

Fixes the bulk of eclipse-theia#7582. Adds a context menu to the Keyboard Shorcuts editor with the following menu items: - Copy - Copy Command ID - Copy Command Title - Edit Keybinding... - Edit When Expression... - Add Keybinding... - Remove Keybinding - Reset Keybinding - Show Same Keybindings This commit does not address the bonus part of eclipse-theia#7582, i.e. no keybindings are provided for the menu items.

tsmaeder · 2023-10-11T14:07:05Z

packages/keymaps/src/browser/keybindings-widget.tsx

+     * Show keybinding items with the same key sequence as the given item.
+     * @param item the keybinding item
+     */
+    public showSameKeybindings(item: KeybindingItem): void {


public is not necessary.

Thank you for your review and comments, Thomas. Let me try to address your concerns.

The public modifiers can be omitted in TypeScript, of course.

However, I aimed at stylistic consistency with the surrounding code here, where both of the directly preceding methods hasSearch() and clearSearch() have already had the public modifier. Also, I didn't find it mentioned in the style guide at all.

However, if you do dislike them, I can remove all the public modifiers in my code, no problem. In that case, may I humbly suggest the style guide probably needs to be updated accordingly?

You're right, of course, that there is no guidance in the style guide. However, most code in Theia does not use public. I usually remove it when I touch a piece of code. But it's not a reason to reject the PR, of course.

I see. I'll remove all the public modifiers in the KeybindingWidget then, including the old code, for the sake of stylistic consistency, if you don't mind. It seems that I haven't introduced any public modifiers elsewhere.

packages/keymaps/src/browser/keybindings-widget.tsx

tsmaeder · 2023-10-11T14:46:37Z

packages/keymaps/src/browser/keybindings-widget.tsx

+    protected handleItemContextMenu(item: KeybindingItem, index: number, event: React.MouseEvent<HTMLElement>): void {
+        event.preventDefault();
+        this.selectItem(item, index, event.currentTarget);
+        setTimeout(() => this.contextMenuRenderer.render({


Why the setTimout()?

I guess I might be misguided by existing code like
https://github.com/eclipse-theia/theia/blob/ff1ee2b6fcce32fac4384fb1cbf64aa7c9638172/packages/core/src/browser/tree/tree-widget.tsx#L1325C67-L1325C67
or
https://github.com/eclipse-theia/theia/blob/ff1ee2b6fcce32fac4384fb1cbf64aa7c9638172/packages/editor/src/browser/editor-linenumber-contribution.ts#L67C21-L67C21
while playing the "monkey see, monkey do" game.

I can remove it if it is actually unnecessary.

I don't think it should be necessary, the context menu renderer is used in other places without it.

I see. I'll remove it then.

packages/keymaps/src/browser/keybindings-widget.tsx

tsmaeder · 2023-10-11T14:58:44Z

packages/keymaps/src/browser/keymaps-frontend-contribution.ts

@@ -49,6 +51,33 @@ export namespace KeymapsCommands {
        id: 'keymaps.clearSearch',
        iconClass: codicon('clear-all')
    };
+    export const COPY_KEYBINDING: Command = {


I believe these should be i18n-ed

Not sure that I understood it correctly. These commands are not intended to be shown in the command palette and, therefore, don't have a label that could be localized. They are meant to be used only in the context menu of the keyboard shortcuts editor, where each menu item does have a localized label.

You can set a keybinding for those commands, so they are visible in the keybindings widget 🤷

I'm sorry if I'm again missing something, but I still don't think these commands should have a label, as this would have made them accessible to the user from the command palette. These commands would not work correctly if invoked from the command palette, as they require specific arguments, which would not have been provided in this case.

There are similar places in the existing code where such commands don't have a label like

theia/packages/console/src/browser/console-contribution.ts

Lines 25 to 44 in e0c82d1

export namespace ConsoleCommands {

export const SELECT_ALL: Command = {

id: 'console.selectAll'

};

export const COLLAPSE_ALL: Command = {

id: 'console.collapseAll'

};

export const CLEAR: Command = {

id: 'console.clear'

};

export const EXECUTE: Command = {

id: 'console.execute'

};

export const NAVIGATE_BACK: Command = {

id: 'console.navigatePrevious'

};

export const NAVIGATE_FORWARD: Command = {

id: 'console.navigateNext'

};

}

or

theia/packages/output/src/browser/output-commands.ts

Lines 28 to 50 in e0c82d1

export const APPEND: Command = {

id: 'output:append'

};

export const APPEND_LINE: Command = {

id: 'output:appendLine'

};

export const CLEAR: Command = {

id: 'output:clear'

};

export const SHOW: Command = {

id: 'output:show'

};

export const HIDE: Command = {

id: 'output:hide'

};

export const DISPOSE: Command = {

id: 'output:dispose'

};

It looks like a pattern to me, or am I missing something?

Even if everyone else is doing it this way, it might still be wrong ;-). What I'm saying is this: if we're using a command, you can assign a keybinding to it, so the command will show up in the keybinding widget, but it won't show up with the proper title.
Whether we show the command in the command quick-pick should be a separate concern. Does the command show up if you properly enable/disable the handler depending on the context (like having the keybinding editor active)?

Your are right. Looks like I was misguided by existing code then.

I have just checked that the new commands, even when they do have a label, don't actually show up in the command quick-pick.

Do you think that a separate category should also be used for the new commands (e.g. 'Keybindings')?

Thank you for your guidance, I really appreciate it!

Just for the record, it looks like the corresponding commands in VS Code don't have a label. To see it, just search for keybindings. in the Keyboard Shortcuts editor in VS Code. I have suddenly discovered it when trying to find out what category (if any) those commands are in VS Code.

Having said that, I agree with your point in general, and just wanted to be sure we were still on the same page in this specific case.

Do you think that a separate category should also be used for the new commands (e.g. 'Keybindings')?

I think VS Code uses "Preferences" as the category for keybindings-related commands. I don't see any value in behaving differently.

it looks like the corresponding commands in VS Code don't have a label

Having a proper label for commands is the right thing to do, IMO.

OK. I have submitted the requested changes for your consideration.

packages/keymaps/src/browser/keymaps-frontend-contribution.ts

packages/keymaps/src/browser/keymaps-service.ts

Removed excessive `public` modifiers and `setTimeout` call.

Added a label for each of the new commands.

tsmaeder

lgtm now.

pisv mentioned this pull request Aug 1, 2023

keybindings: add context-menu to keyboard shortcuts #7582

Open

6 tasks

vince-fugnitto added the keybindings issues related to keybindings label Aug 1, 2023

JonasHelming requested a review from tsmaeder October 10, 2023 13:02

tsmaeder requested changes Oct 11, 2023

View reviewed changes

pisv added 5 commits October 12, 2023 15:20

Merge branch 'master' into eclipse-theiaGH-7582

d63efc5

Merge branch 'master' into eclipse-theiaGH-7582

7f1762b

Address review comments

fd6e4c6

Removed excessive `public` modifiers and `setTimeout` call.

Merge branch 'master' into eclipse-theiaGH-7582

55962fd

Address review comments

13bc6f5

Added a label for each of the new commands.

tsmaeder self-requested a review October 20, 2023 14:23

tsmaeder approved these changes Oct 20, 2023

View reviewed changes

Merge branch 'master' into eclipse-theiaGH-7582

24d8a2d

tsmaeder merged commit 825c092 into eclipse-theia:master Oct 21, 2023
13 checks passed

pisv deleted the GH-7582 branch October 21, 2023 10:31

vince-fugnitto added this to the 1.43.0 milestone Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keybindings: add context menu to keyboard shortcuts #12791

keybindings: add context menu to keyboard shortcuts #12791

pisv commented Aug 1, 2023

tsmaeder Oct 11, 2023

pisv Oct 12, 2023

tsmaeder Oct 13, 2023

pisv Oct 13, 2023

tsmaeder Oct 11, 2023

pisv Oct 12, 2023

tsmaeder Oct 13, 2023

pisv Oct 13, 2023

tsmaeder Oct 11, 2023

pisv Oct 12, 2023

tsmaeder Oct 13, 2023

pisv Oct 13, 2023

tsmaeder Oct 19, 2023 •

edited

Loading

pisv Oct 19, 2023 •

edited

Loading

pisv Oct 19, 2023

tsmaeder Oct 20, 2023

pisv Oct 20, 2023

tsmaeder left a comment

	export namespace ConsoleCommands {
	export const SELECT_ALL: Command = {
	id: 'console.selectAll'
	};
	export const COLLAPSE_ALL: Command = {
	id: 'console.collapseAll'
	};
	export const CLEAR: Command = {
	id: 'console.clear'
	};
	export const EXECUTE: Command = {
	id: 'console.execute'
	};
	export const NAVIGATE_BACK: Command = {
	id: 'console.navigatePrevious'
	};
	export const NAVIGATE_FORWARD: Command = {
	id: 'console.navigateNext'
	};
	}

	export const APPEND: Command = {
	id: 'output:append'
	};

	export const APPEND_LINE: Command = {
	id: 'output:appendLine'
	};

	export const CLEAR: Command = {
	id: 'output:clear'
	};

	export const SHOW: Command = {
	id: 'output:show'
	};

	export const HIDE: Command = {
	id: 'output:hide'
	};

	export const DISPOSE: Command = {
	id: 'output:dispose'
	};

keybindings: add context menu to keyboard shortcuts #12791

keybindings: add context menu to keyboard shortcuts #12791

Conversation

pisv commented Aug 1, 2023

What it does

How to test

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

pisv Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Oct 19, 2023 •

edited

Loading

pisv Oct 19, 2023 •

edited

Loading