It seems like its been a while since its been active and there have been two solutions proposed (if i’m understanding them correctly):
but they have both been closed. As it’s needed for quite a few open issues (and to fully get vscode keymap extensions working), I thought I should bring it up to see what can be done about it/if there is a solution in mind.
It’s needed for:
vim vscode extension: https://github.com/theia-ide/theia/issues/5125
Eclipse Keybindings Extensions has no Effect: https://github.com/theia-ide/theia/issues/5113
register monaco commands used by VS Code extensions: https://github.com/theia-ide/theia/issues/4247
Implement editor.action.rename comand: https://github.com/theia-ide/theia/issues/5215
Implement editor.action.formatXXX commands: https://github.com/theia-ide/theia/issues/5216
[original thread by Josh Pinkney]
It looks like for some of the commands we need to wait until we upgrade monaco which is being tracked here.
@jpinkney feel free to reopen #4247 and continue working on it, there are several issues with it:
The problem with the code in VS Code Java is that it assumes all actions are using 0-based actions that need to be converted to 1-based before being used. I think it’s better to explicitly do the conversion for well known actions only, that’s what I did in https://github.com/theia-ide/theia/pull/4338/files
The problem is: if we don’t want the conversion (as done in vscode), how does a command opt out of it?
I’m still trying to wrap my head around everything and I have a quick question. Is there a reason some commands are prefixed with monaco?
This may be a dumb question because I haven’t truly wrapped my head around the problem yet but can we remove the prefix of monaco and then use the known commands in plugin-ext to do the conversions for commands that we know are 1-based? Similiar to https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts#L449
The reason I believe the monaco prefix needs to be removed is users coming from VSCode would expect to set editor.action.formatSelection not monaco.editor.action.formatSelection if they are manually editing their keymaps.json and just a conversion of knowncommands would not cover that case
At the beginning we wanted to avoid confusion with 1-based Monaco and 0-based LSP commands, so monaco APIs were explicitly prefixed. They also were supposed to be used only internally in monaco extension. VS Code extensions though can call any Monaco commands so they either shuold be in command registry without the prefix or the plugin system should do some sort of forwarding. Conversion should be done anyway in the plugins system. I also don’t know the best course of actions, will need to sit and play with different solutions. Ideally it should not cause much disturbance in core and for other Theia extensions and be encapsulated in the plugin system.
I had an idea that would preserve the monaco prefix (for theia extension capabilities) while still allowing the plugin system to have access to monaco commands.
What if we registered monaco commands without the prefix inside of the plugin extension.
I realize that the major con to this is that technically monaco.editor.action.formatSelection and editor.action.formatSelection will both be registered (for example).
However, there are a few things to gain.
By registering VSCode commands inside of plugin extension without monaco prefix we already
gain a ton of compatible commands with VSCode without losses in core or theia extensions
We know exactly which commands are monaco commands and thus can do the conversions like
how VSCode does it
I’m not sure if this is the best course of action but it’s just a thought
@jpinkney Sorry, i did not have time to look at PRs this week, fixing scm stuff. I will start reviewing today and look at your PRs.
How do you want to register Monaco commands only within the plugin system?
CommandRegistry is shared. Would you introduce some kind of delegation?
@anton-kosyakov I’m just looking into this now and trying to establish all the edge cases but I suppose since CommandRegistry is shared we wouldn’t need to register them in the plugin extension, we would just be able to register them in monaco package both with and without prefix.
As far as I can see there is only really two ways to approach this issue: we can either forward the calls that come from the plugins to commands like editor.action.formatSelect to monaco.editor.action.formatSelect via https://github.com/theia-ide/theia/blob/master/packages/plugin-ext/src/plugin/type-converters.ts#L449 which would work but has the drawback that the keybindings view would show monaco.editor.action.formatSelect instead of editor.action.formatSelect which would probably confuse users.
OTOH we could register monaco commands both with the monaco prefix and without the monaco prefix that way it maintains compatibility with existing extensions while still providing the plugin system with all the available monaco commands. This of course has the drawback of having a lot more commands showing up when users open the keybindings view. I’m not sure which approach to take, or potentially if there is some other case that I’m missing but it seems like every potential way of thinking about it has its own pros and major cons.