microsoft / vscode-vs-keybindings

Visual Studio Keymap for Visual Studio Code
MIT License
55 stars 39 forks source link

Keyboard shortcuts from extension not applied #29

Open dardino opened 3 years ago

dardino commented 3 years ago

Steps to Reproduce:

  1. install vscode insider
  2. install ms-vscode.vs-keybindings
  3. open keyboard shortcut preferences: image
  4. see the difference between the shortcut in the menu and the setting for "Save all" command
  5. try to save all with menu defined shortcut works
  6. try to save all with preferences defined shortcut doesn't works

Does this issue occur when all extensions are disabled?: Not applicable

vscodebot[bot] commented 3 years ago

(Experimental duplicate detection) Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

dardino commented 3 years ago

similar to: https://github.com/microsoft/vscode/issues/85798

sandy081 commented 3 years ago

@alexdima @deepak1556 Assigning it to you both because

  1. One report is about execution of keybinding not working
  2. Menu is showing different keybinding.

Please let me know if there is something I can help with.

alexdima commented 3 years ago

It looks like the are actually two distinct commands:

I think that perhaps at one point we migrated the File menu from workbench.action.files.saveAll to saveAll and forgot to update the VS keymap extension.

@bpasero Assigning to you to clear up what the VS keymap should do here @rebornix Assigning to you for adoption in the VS keymap extension

bpasero commented 3 years ago

saveAll sounds like the correct action to me because workbench.action.files.saveAll seems to originate from the open editors view, but I am not sure why we need to have it:

https://github.com/microsoft/vscode/blob/07a458dd6818766aae91d1d7ff68baa83497b503/src/vs/workbench/contrib/files/browser/views/openEditorsView.ts#L754-L774

Adding @isidorn who maybe added it?

isidorn commented 3 years ago

@bpasero looks like historical reasons. We could remove it, but probably some users are using it, so I would prefer to leave the duplicated ids.

bpasero commented 3 years ago

Ok, then I suggest that the keymap extension is updated to use the saveAll command and not the other one that is open editors specific.

rebornix commented 3 years ago

@chrisdias is now the owner of the keymap ;)