Provided mapping from vscode command ids to internal command ids #5590

JPinkney · 2019-06-26T17:25:04Z

What it does

This PR introduces mappings from vscode command ids to internal theia commands ids as part of #5113 and #4247.

All commands listed here that are not registered under vscode's command id but instead have a command id native to theia are mapped. E.g. workbench.action.files.saveAs is mapped to file.saveAs so that when an extension calls workbench.action.files.saveAs the command that actually gets executed is file.saveAs.
However, there are still a lot of commands missing that either don't have an internal theia command that is relatable or don't have an implementation at all.

Also, when commands are being registered inside of plugins, vscode command ids must be mapped to their corresponding internal command id so that when you use that keybinding it understands to use the mapped internal command id rather than the vscode command id. This is because without the mapping the keybindings thinks that it should execute editor.action.formatSelection which doesn't have a registered handler, rather than monaco.editor.action.formatSelection which does have a handler.

After looking into multiple approaches on how to solve this problem this seems to be the best approach I can find. However, there are some shortcomings to this approach:

Due to the fact we have to map vscode commands to internal theia commands, attempting to add in something like editor.action.formatSelection into keymaps.json will not work because keymaps.json expects that command to be monaco.editor.action.formatSelection.
Finding the vscode command in the keybindings widget will fail because theia's internal command id would be monaco.editor.action.formatSelection rather than editor.action.formatSelection.

As far as I could find, this is the best solution that is doesn't change core or break any existing theia extensions.

How to test

Install vscode-java, alphabotsec.vscode-eclipse-keybindings
Then clone git@github.com:JPinkney/Theia-Extension-Test-Repo.git

How to test execute commands:
Set "java.referencesCodeLens.enabled": true

Open 'helloworld.java'
From the command pallete run 'open Java language server log file'
From within that java file test references
Then start the hosted instance on the cloned project and run Test by commenting this line from the command palette

How to test the keybinding mappings work:
Try these mapped keybindings on the helloworld.java file
alt+shift+r/cmd+alt+r -> rename a variable
ctrl+alt+j/cmd+alt+j -> join lines
ctrl/cmd+shift+c -> comments the line
ctrl+h -> brings up search in side search bar
cmd+l/ctrl+l -> brings up go to line prompt

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

Signed-off-by: Josh Pinkney joshpinkney@gmail.com

JPinkney · 2019-06-26T17:40:43Z

This is a master list of all missing vscode commands from the vscode keybinding page that this PR provides.

A 1 in the registered correctly column means the internal command id representation is the same as vscode api's (and thus nothing needs to be done because its already correctly registered). A 0 or nothing means that there is no command with that command id.

A 1 in the forwarded in plugin column means that the vscode command is is mapped to an internal theia command id. A 0 or nothing either means that it doesn't need to be mapped or an internal theia command doesn't exist.

Command ID	Registered Correctly	Forwarded in Plugin (If there is a compatible command)
Rich Languages Editing
editor.action.triggerSuggest		1
editor.action.triggerParameterHints		1
editor.action.formatDocument		1
editor.action.formatSelection		1
editor.action.revealDefinition	0	0
editor.action.showHover		1
editor.action.peekDefinition	0	0
editor.action.revealDefinitionAside	0	0
editor.action.quickFix		1
editor.action.referenceSearch.trigger		1
editor.action.rename		1
editor.action.inPlaceReplace.down		1
editor.action.inPlaceReplace.up		1
editor.action.smartSelect.expand	0	0
editor.action.smartSelect.shrink		1
editor.action.trimTrailingWhitespace		1
workbench.action.editor.changeLanguageMode	0	0

Navigation
workbench.action.showAllSymbols	0	0
workbench.action.gotoLine		1
workbench.action.quickOpen	0	0
workbench.action.gotoSymbol		1
workbench.actions.view.problems		1
editor.action.marker.nextInFiles		1
editor.action.marker.prevInFiles		1
workbench.action.showCommands	1	0
workbench.action.openPreviousRecentlyUsedEditorInGroup	0	0
workbench.action.navigateBack	0	0
workbench.action.quickInputBack	0	0
workbench.action.navigateForward	0	0

Editor/Window Management
workbench.action.newWindow	0
workbench.action.closeWindow	0
workbench.action.closeActiveEditor	1
workbench.action.closeFolder	0
workbench.action.navigateEditorGroups	0
workbench.action.splitEditor	0
workbench.action.focusFirstEditorGroup	0
workbench.action.focusSecondEditorGroup	0
workbench.action.focusThirdEditorGroup	0
workbench.action.focusPreviousGroup	0
workbench.action.focusNextGroup	0
workbench.action.moveEditorLeftInGroup	0
workbench.action.moveEditorRightInGroup	0
workbench.action.moveActiveEditorGroupLeft	0
workbench.action.moveActiveEditorGroupRight	0
workbench.action.moveEditorToNextGroup	0
workbench.action.moveEditorToPreviousGroup	0

File Management
workbench.action.files.newUntitledFile	1
workbench.action.files.openFile	1
workbench.action.files.save	1
workbench.action.files.saveAll	1
workbench.action.files.saveAs		1
workbench.action.closeActiveEditor	1
workbench.action.closeOtherEditors	1
workbench.action.closeEditorsInGroup	1
workbench.action.closeEditorsInOtherGroups	1
workbench.action.closeEditorsToTheLeft	1
workbench.action.closeEditorsToTheRight	1
workbench.action.closeAllEditors	1
workbench.action.reopenClosedEditor	0
workbench.action.keepEditor	0
workbench.action.openNextRecentlyUsedEditorInGroup	0
workbench.action.openPreviousRecentlyUsedEditorInGroup	0
workbench.action.files.copyPathOfActiveFile	0
workbench.action.files.revealActiveFileInWindows	0
workbench.action.files.showOpenedFileInNewWindow	0
workbench.files.action.compareFileWith	0

Display
workbench.action.toggleFullScreen	0
workbench.action.toggleZenMode	0
workbench.action.exitZenMode	0
workbench.action.zoomIn	0
workbench.action.zoomOut	0
workbench.action.zoomReset	0
workbench.action.toggleSidebarVisibility	0
workbench.view.explorer		1
workbench.view.search		1
workbench.view.scm		1
workbench.view.debug		1
workbench.view.extensions		1
workbench.action.output.toggleOutput		1
workbench.action.quickOpenView	0
workbench.action.terminal.openNativeConsole		1
markdown.showPreview	0
markdown.showPreviewToSide	0
workbench.action.terminal.toggleTerminal	0

Search
workbench.view.search		1
workbench.action.replaceInFiles	0
toggleSearchCaseSensitive	0
toggleSearchWholeWord	0
toggleSearchRegex	0
workbench.action.search.toggleQueryDetails	0
search.action.focusNextSearchResult	0
search.action.focusPreviousSearchResult	0
history.showNext	0
history.showPrevious	0

Preferences
workbench.action.openSettings	0
workbench.action.openWorkspaceSettings	0
workbench.action.openGlobalKeybindings	0
workbench.action.openSnippets	0
workbench.action.selectTheme	0
workbench.action.configureLocale	0

Debug
editor.debug.action.toggleBreakpoint	1
workbench.action.debug.start	1
workbench.action.debug.continue	1
workbench.action.debug.run	1
workbench.action.debug.pause	1
workbench.action.debug.stepInto	1
workbench.action.debug.stepOut	1
workbench.action.debug.stepOver	1
workbench.action.debug.stop	1
editor.debug.action.showDebugHover	Looks like it exists but can't find in keybinding widget?

Tasks
workbench.action.tasks.build	0
workbench.action.tasks.test	0

Extensions
workbench.extensions.action.installExtension	0
workbench.extensions.action.showInstalledExtensions	0
workbench.extensions.action.listOutdatedExtensions	0
workbench.extensions.action.showRecommendedExtensions	0
workbench.extensions.action.showPopularExtensions	0
workbench.extensions.action.updateAllExtensions	0

vince-fugnitto · 2019-06-26T17:51:11Z

@JPinkney can you rebase your PR so that CI can pass?
It needs the following change #5587

paul-marechal

Due to the fact we have to map vscode commands to internal theia commands, attempting to add in something like editor.action.formatSelection into keymaps.json will not work because keymaps.json expects that command to be monaco.editor.action.formatSelection.

It sounds fine to me since we are trying to develop the Theia platform, adding VS Code support. So it makes sense that our services expect our own structures. Now if we happen to load keybindings / commands from an external source, the mapping as you've done is perfect (in case we ever parse an actual keymaps.json coming from VS Code, in this case we could benefit from what is done here by having a parser doing the mapping).

Finding the vscode command in the keybindings widget will fail because theia's internal command id would be monaco.editor.action.formatSelection rather than editor.action.formatSelection.

Dependency-wise, I'm not sure we should make the keybindings extension depend on the plugin-ext, so it goes along what you are saying: It would be hard to have an inverse lookup. But then again, why would someone using a Theia application look for the VS Code ids? Unless we were to change each command to match VS Code, but I don't know what kind of issues that would bring or if it is something we even want to do.

It is a bit unfortunate that we have to do such a mapping, but it sounds like the best solution :)

packages/plugin-ext/src/common/known-commands.ts

packages/plugin-ext/src/plugin/command-registry.ts

vince-fugnitto · 2019-06-26T22:10:48Z

packages/plugin-ext/src/plugin/types-impl.ts

+    *  Copyright (c) Microsoft Corporation. All rights reserved.
+    *  Licensed under the MIT License. See License.txt in the project root for license information.
+    *--------------------------------------------------------------------------------------------*/
+    // copied from https://github.com/microsoft/vscode/blob/master/src/vs/workbench/api/common/extHostTypes.ts


We require a CQ for the copied code.
https://github.com/theia-ide/theia/wiki/Registering-CQs

Do you have to be an official commiter in order to register a CQ? Following the document linked, I login and go to the project page but there isn't anywhere that says intellectual property or create a contribution questionnaire

packages/plugin-ext/src/common/object.ts

akosyakov · 2019-07-01T07:37:10Z

@JPinkney we should be able to parse keybindings from VS Code. It would be fine with me to register Monaco command twice as you suggested before, if there is no way to get rid of monaco prefix without breaking. It won't require maintaining mapping as well. monaco prefixed keybindings then should be removed from UI.

Very cool that you sorted out generic transformation between monaco and vscode APIs. Did you test it with code lenses? Could you provide how you test it?

tsmaeder · 2019-08-14T09:43:21Z

@vince-fugnitto @marechal-p @akosyakov what do we need to do to move this PR forward? File a CQ?

vince-fugnitto · 2019-08-14T12:09:14Z

@vince-fugnitto @marechal-p @akosyakov what do we need to do to move this PR forward? File a CQ?

Yes, I believe a CQ should be registered for the copied code (only a committer can successfully register a CQ), the conflicts should be addressed, and perhaps document in the PR's description on how to test the PR? I think the only thing reviewers have done for the moment is to look at the source code.

akosyakov · 2019-08-14T12:56:08Z

@tsmaeder rebasing and providing How to test description that changes are verified independent, not by an author, then someone has to test it.

Also please file an issue that we should be able to parse VS Code keybindings, i.e. without monaco prefix. We can think how to handle it separately.

tolusha · 2019-08-15T05:49:14Z

@vince-fugnitto @akosyakov @JPinkney
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20661

We have completed a high level triage of the attachment based on Parallel IP
[1]. You may proceed to check the content into the Eclipse repository. We
will perform a comprehensive analysis at a later date based on IP work queue.

akosyakov · 2019-08-15T10:09:31Z

@tolusha that's cool, but PR needs rebase and explanation how to test from the user perspective.

As far as I understood for a user there are 2 issues should be solved: (1) now a command can be triggered from code lenses and (2) custom keybindings can be used. Is it right? I assume @JPinkney used some VS Code extensions to ensure it. It would be good to share it to speed up review.

akosyakov · 2019-08-26T08:21:47Z

@JPinkney @tsmaeder Would you mind If I rebase it and write down test cases?

JPinkney · 2019-08-27T12:22:13Z

@akosyakov Yeah! You can do that, ping me when you're done and i'll test

tsmaeder · 2019-08-27T12:42:42Z

@akosyakov @JPinkney is not shirking responsiblity, but got shanghaied into doing some OpenShift related work.

akosyakov · 2019-10-07T15:26:30Z

I'm very busy this week. @eclipse-theia/eclipse-theia Could someone help testing it?

@JPinkney defined some tests in How to test section of the description.
I wanted to test also typescript VS Code extension:
- typesctipt VS Code extension for testing: ts_plugins.zip
- uninstall native typescript extension
- With this PR refactorings like extract method, variable and so on should work. Pay attention is there any issue with positions, i.e. they should NOT be bogusly shifted by 1 character.

elaihau · 2019-10-11T04:00:56Z

I am trying to test this pull request with the typescript VS Code extension.

Could Anton @akosyakov or someone kindly provide me some guidance on how to uninstall native Theia typescript extension ?

Thank you in advance !

kittaakos · 2019-10-11T06:20:56Z

how to uninstall native Theia typescript extension

You can remove the @theia/typescript dependency from the example app's package.json.

akosyakov · 2019-10-11T10:19:18Z

@JPinkney Could you remind pleas why cannot we drop monaco. prefix and register Monaco command as is once?

packages/plugin-ext/src/common/objects.ts

packages/plugin-ext/src/common/types.ts

packages/plugin-ext/src/plugin/types-impl.ts

JPinkney · 2019-10-11T11:22:23Z

@akosyakov In the PR it is registering without monaco prefix as well: https://github.com/eclipse-theia/theia/pull/5590/files#diff-3e78733f92a6f6b63b219e5fec2097f6R55 and then it hides anything with the monaco prefix in the keybinding view. From what I understood we had to keep the monaco prefix because other extenders of theia might be using it?

akosyakov · 2019-10-11T12:29:25Z

From what I understood we had to keep the monaco prefix because other extenders of theia might be using it?

We can ask extenders to drop monaco prefix. It was more to avoid changing core and monaco extensions, only apply changes to the plugin system. But since we cannot avoid it, i wonder can we just drop monaco prefix? Otherwise we introduce for example an implicit dependency from core to monaco by checking kebindings.

There are excluded commands which should not be registered. I think we should keep it like is but rather than that rest of monaco commands can be registered only without a prefix.

akosyakov · 2019-10-11T12:52:34Z

Typescript VS Code extension works better with this PR 🎉 I can use refactoring code actions.

I have issues with keybindings:

change a keybidnig for Rename Symbol from f2 to f3
it works for f3 👍
but it is still available for f2 😢

akosyakov · 2019-10-29T07:53:14Z

@JPinkney let me know when it is possible to test again. I see that tests are failing. Please also search through the repo for command invocations with monaco. prefix. We should drop them.

akosyakov · 2019-11-05T10:24:03Z

@JPinkney is it good now for testing? there seem to be some conflicting files

JPinkney · 2019-11-05T14:56:06Z

@akosyakov Just fixed the conflict, should be good to test now

akosyakov

Super strange, the old keybinding still works. I've tried to give a different keybiding for rename symbols and format document.

akosyakov · 2019-11-05T16:31:04Z

packages/monaco/src/browser/monaco-command.ts

@@ -273,7 +274,7 @@ export class MonacoEditorCommandHandlers implements CommandContribution {
    }
    protected newMonacoActionHandler(action: MonacoCommand): MonacoEditorCommandHandler {
        const delegate = action.delegate;
-        return delegate ? this.newCommandHandler(delegate) : this.newActionHandler(action.id);
+        return delegate ? this.newDelegateHandler(delegate) : this.newActionHandler(action.id);


It does not seem that newCommandHandler is used anywhere else. Should not we just change it?

akosyakov · 2019-11-05T16:40:58Z

Oh, I suspect the issue is that no keybinding matches in our registry, so then the default Monaco keyboard handler is called. 🙈 We should replace Monaco keybinding service with the null implementation.

akosyakov

I think this PR is good. The issue with calling default Monaco keyboard handler if our registry does not have such keybinding is present on the master as well.

@JPinkney could you file a follow-up issue for it? I will explain on the issue how to fix it.

akosyakov · 2019-11-05T16:45:12Z

Adding a record to CHANGELOG with a breaking change that monaco. prefix was dropped would be helpful.

Not sure what my request was.

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>

JPinkney requested review from benoitf and evidolob as code owners June 26, 2019 17:25

JPinkney force-pushed the monaco-keys2 branch from ac2e6af to c7b0cda Compare June 26, 2019 18:03

JPinkney mentioned this pull request Jun 26, 2019

Eclipse Keybindings Extensions has no Effect #5113

Open

paul-marechal reviewed Jun 26, 2019

View reviewed changes

vince-fugnitto reviewed Jun 26, 2019

View reviewed changes

akosyakov reviewed Jul 1, 2019

View reviewed changes

packages/plugin-ext/src/common/object.ts Outdated Show resolved Hide resolved

JPinkney force-pushed the monaco-keys2 branch from c7b0cda to 69046aa Compare July 8, 2019 13:15

akosyakov added commands issues related to application commands monaco issues related to monaco vscode issues related to VSCode compatibility keybindings issues related to keybindings labels Jul 9, 2019

vince-fugnitto mentioned this pull request Jul 18, 2019

TreeItem on click commands don't work #5744

Closed

tsmaeder mentioned this pull request Aug 14, 2019

Languages Sprint 171 eclipse-che/che#14221

Closed

24 tasks

vince-fugnitto mentioned this pull request Aug 14, 2019

add force refresh button in navigator #5940

Merged

1 task

tsmaeder mentioned this pull request Aug 29, 2019

Languages Sprint 172 eclipse-che/che#14368

Closed

41 tasks

tsmaeder mentioned this pull request Sep 24, 2019

Languages Sprint 173 eclipse-che/che#14640

Closed

30 tasks

JPinkney force-pushed the monaco-keys2 branch 2 times, most recently from fe30277 to cf6b330 Compare September 24, 2019 17:35

akosyakov requested review from azatsarynnyy and svenefftinge October 7, 2019 15:21

akosyakov reviewed Oct 11, 2019

View reviewed changes

packages/plugin-ext/src/common/objects.ts Show resolved Hide resolved

akosyakov reviewed Oct 11, 2019

View reviewed changes

packages/plugin-ext/src/common/types.ts Show resolved Hide resolved

akosyakov reviewed Oct 11, 2019

View reviewed changes

packages/plugin-ext/src/plugin/types-impl.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Oct 11, 2019

Languages Sprint 174 eclipse-che/che#14855

Closed

26 tasks

akosyakov mentioned this pull request Oct 22, 2019

Enter key not working in Latex Workshop extension usage #6408

Open

JPinkney force-pushed the monaco-keys2 branch from e0fb066 to 41df06c Compare October 28, 2019 20:33

tsmaeder mentioned this pull request Oct 31, 2019

Languages Sprint 175 eclipse-che/che#15049

Closed

25 tasks

JPinkney force-pushed the monaco-keys2 branch from 41df06c to d266d81 Compare November 3, 2019 02:04

JPinkney force-pushed the monaco-keys2 branch from d266d81 to 8efa9fc Compare November 5, 2019 14:55

akosyakov reviewed Nov 5, 2019

View reviewed changes

akosyakov approved these changes Nov 5, 2019

View reviewed changes

Added mappings from vscode commands to internal commands

4495b30

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>

JPinkney force-pushed the monaco-keys2 branch from 8efa9fc to 4495b30 Compare November 7, 2019 15:33

JPinkney merged commit 9161b50 into eclipse-theia:master Nov 7, 2019

tsmaeder mentioned this pull request Nov 21, 2019

Languages Sprint 176 eclipse-che/che#15269

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provided mapping from vscode command ids to internal command ids #5590

Provided mapping from vscode command ids to internal command ids #5590

JPinkney commented Jun 26, 2019 •

edited

Loading

JPinkney commented Jun 26, 2019 •

edited

Loading

vince-fugnitto commented Jun 26, 2019

paul-marechal left a comment •

edited

Loading

vince-fugnitto Jun 26, 2019

JPinkney Jul 8, 2019

akosyakov commented Jul 1, 2019 •

edited

Loading

tsmaeder commented Aug 14, 2019

vince-fugnitto commented Aug 14, 2019

akosyakov commented Aug 14, 2019

tolusha commented Aug 15, 2019 •

edited

Loading

akosyakov commented Aug 15, 2019 •

edited

Loading

akosyakov commented Aug 26, 2019

JPinkney commented Aug 27, 2019

tsmaeder commented Aug 27, 2019

akosyakov commented Oct 7, 2019 •

edited

Loading

elaihau commented Oct 11, 2019

kittaakos commented Oct 11, 2019

akosyakov commented Oct 11, 2019

JPinkney commented Oct 11, 2019

akosyakov commented Oct 11, 2019 •

edited

Loading

akosyakov commented Oct 11, 2019 •

edited

Loading

akosyakov commented Oct 29, 2019 •

edited

Loading

akosyakov commented Nov 5, 2019

JPinkney commented Nov 5, 2019

akosyakov left a comment

akosyakov Nov 5, 2019

akosyakov commented Nov 5, 2019

akosyakov left a comment

akosyakov commented Nov 5, 2019

Provided mapping from vscode command ids to internal command ids #5590

Provided mapping from vscode command ids to internal command ids #5590

Conversation

JPinkney commented Jun 26, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

JPinkney commented Jun 26, 2019 • edited Loading

vince-fugnitto commented Jun 26, 2019

paul-marechal left a comment • edited Loading

Choose a reason for hiding this comment

vince-fugnitto Jun 26, 2019

Choose a reason for hiding this comment

JPinkney Jul 8, 2019

Choose a reason for hiding this comment

akosyakov commented Jul 1, 2019 • edited Loading

tsmaeder commented Aug 14, 2019

vince-fugnitto commented Aug 14, 2019

akosyakov commented Aug 14, 2019

tolusha commented Aug 15, 2019 • edited Loading

akosyakov commented Aug 15, 2019 • edited Loading

akosyakov commented Aug 26, 2019

JPinkney commented Aug 27, 2019

tsmaeder commented Aug 27, 2019

akosyakov commented Oct 7, 2019 • edited Loading

elaihau commented Oct 11, 2019

kittaakos commented Oct 11, 2019

akosyakov commented Oct 11, 2019

JPinkney commented Oct 11, 2019

akosyakov commented Oct 11, 2019 • edited Loading

akosyakov commented Oct 11, 2019 • edited Loading

akosyakov commented Oct 29, 2019 • edited Loading

akosyakov commented Nov 5, 2019

JPinkney commented Nov 5, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov Nov 5, 2019

Choose a reason for hiding this comment

akosyakov commented Nov 5, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Nov 5, 2019

JPinkney commented Jun 26, 2019 •

edited

Loading

JPinkney commented Jun 26, 2019 •

edited

Loading

paul-marechal left a comment •

edited

Loading

akosyakov commented Jul 1, 2019 •

edited

Loading

tolusha commented Aug 15, 2019 •

edited

Loading

akosyakov commented Aug 15, 2019 •

edited

Loading

akosyakov commented Oct 7, 2019 •

edited

Loading

akosyakov commented Oct 11, 2019 •

edited

Loading

akosyakov commented Oct 11, 2019 •

edited

Loading

akosyakov commented Oct 29, 2019 •

edited

Loading