Update menu when reassigning keybindings or installing keybinding extension #14625

Closed
mousetraps opened this Issue Oct 27, 2016 · 11 comments

Projects

None yet

4 participants

@mousetraps
Member

Below, Ctrl+S has been reassigned to workbench.action.relaodWindow, and Ctrl+I has been assigned to workbench.action.files.save, but he File context menu has not been updated appropriately.

image

@mousetraps mousetraps changed the title from reassinging keyboard shortcuts does not impact the shortcuts displayed in menu bar to reassigning keyboard shortcuts does not impact the shortcuts displayed in menu bar Oct 27, 2016
@bpasero bpasero was assigned by weinand Oct 27, 2016
@bpasero
Member
bpasero commented Oct 28, 2016

Yes, that has always been like that but I think we can fix it easily.

@bpasero bpasero removed their assignment Oct 28, 2016
@bpasero bpasero added the help wanted label Oct 28, 2016
@bpasero bpasero changed the title from reassigning keyboard shortcuts does not impact the shortcuts displayed in menu bar to Update menu when reassigning keybindings Oct 28, 2016
@arthur801031

@bpasero May I work on this? I'm a first time contributor to vscode.

@bpasero
Member
bpasero commented Nov 13, 2016

Sure, feel free.

@arthur801031

@bpasero Could you please help me out with these questions? Thank you!

  • I think I need to make changes in vscode/out/vs/base/common/keybinding.js and vscode/out/vs/code/electron-main/menus.js, but I don't know how to debug these files since the code has already been run when vscode is opened.
  • How do I access user's keybindings.json file for the user's new keys?
@bpasero
Member
bpasero commented Nov 14, 2016

Let me forward the debug questions to our debug team: @roblourens @weinand @isidorn are we able to debug the Electron main side these days?

@arthur801031 I suggest to check how keybindings are resolved today right on startup (https://github.com/Microsoft/vscode/blob/master/src/vs/code/electron-main/menus.ts#L180) and then add a listener for keybindings.json changes to run a similar call to update the menu.

@arthur801031
arthur801031 commented Nov 14, 2016 edited

@bpasero

It seems that Electron does not support updating menus dynamically: https://github.com/Microsoft/vscode/blob/master/src/vs/code/electron-main/menus.ts#L197
If the user quits and restarts vscode, the menus are updated according to keybindings.json. As long as the user restarts vscode, the menus should show the correct keybindings.

@bpasero
Member
bpasero commented Nov 14, 2016

That is true. On startup we read the keybindings from a cached location on disk, then we start a window and ask that window to resolve all keybindings. If the keybindings differ, we update the menu once again. Ugly, but the only way possible atm.

@arthur801031
arthur801031 commented Nov 14, 2016 edited

I've changed 2 keys in keybindings.json and I restarted vscode. When vscode was opened, the menus' shortcut keys have been updated accordingly.
screen shot 2016-11-14 at 5 02 39 pm
screen shot 2016-11-14 at 5 02 25 pm

It seems what mousetraps ran into was that she didn't restart vscode after updating keybindings.json, so the menus were not updated accordingly.

@bpasero
Member
bpasero commented Nov 14, 2016

@arthur801031 yes, this issue is about updating the menu without the need for restart.

@arthur801031

Hi @bpasero, I'm trying to create a listener for keybindings.json, but I can't get the keybindings.json file path right. Here is my code in the registerListeners function of /vscode/out/vs/code/electron-main/menus.js:
screen shot 2016-11-15 at 9 31 02 am

Here is the error output:
screen shot 2016-11-15 at 9 32 17 am

Another question that I have is if I place these code inside src/vs/code/electron-main/menus.ts that piece of code wouldn't run, which means the error message wouldn't even show up. Please help. Thank you so much!

@bpasero bpasero changed the title from Update menu when reassigning keybindings to Update menu when reassigning keybindings or installing keybinding extension Dec 22, 2016
@bpasero
Member
bpasero commented Dec 22, 2016

Also applies to when installing a keymap extension (/cc @chrmarti). happy to review a PR on this one, I think what is needed is to:

  • update the menu when a change to keybindings.json is detected (for example by using ConfigWatcher)
  • update the menu when a keymap extension gets installed (ideally this could be triggered also from the main side by listening to extension events that happen in the shared process)
@bpasero bpasero added this to the January 2017 milestone Jan 7, 2017
@bpasero bpasero self-assigned this Jan 7, 2017
@bpasero bpasero closed this in db859bf Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment