Use custom context to scope key bindings for language server #751
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -31,10 +45,53 @@ public class LanguageServerPlugin extends AbstractUIPlugin { | |||
| public LanguageServerPlugin() { | |||
| } | |||
|
|
|||
| public static class WindowListener implements IPartListener2 { | |||
There was a problem hiding this comment.
Can you please give explicit name here, such as AttachLSP4EContextPartListener ?
| IContextService contextService = PlatformUI.getWorkbench() | ||
| .getService(IContextService.class); | ||
| IPartListener2 listener = new WindowListener(contextService); | ||
| partService.addPartListener(listener); |
There was a problem hiding this comment.
Shouldn't the listener be removed on bundle stop?
|
The initial target branch |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are some problems with promiscuously scoped keybindings in the core platform that mean that some keybindings used by LSP4e don't always work or are at odds with the accelerators that show up in the menus, making them less discoverable.
The problems our users observe are Rename (shows up as F2 in the editor context menu, even though we switched to alt-shift-R some time ago when we had trouble with F2 not working, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=525413) and sometimes format (cmd-shift-F on a Mac).
The culprit with F2 seems to be https://github.com/eclipse-platform/eclipse.platform.ui/blob/1dc77e0823e8b8bfb703ac2ea9c53266555a629e/bundles/org.eclipse.ui.workbench.texteditor/plugin.xml#L503 which binds it to 'show information' clashing with https://github.com/eclipse-platform/eclipse.platform.ui/blob/1dc77e0823e8b8bfb703ac2ea9c53266555a629e/bundles/org.eclipse.ui/plugin.xml#L114
The latter causes F2 to show up as the accelerator in menus, the former causes it not to work in the text editor.
Haven't got to the bottom of what's happening with format, but I think it might be a peer plugin as it mysteriously started working again on update to our plugin, so that might indicate order of installation affecting things in the case of a tie.
There are two abstraction/scoping mechanisms for key bindings as I understand it, scheme and context. If I create a custom scheme within the
bindingsextension block inplugin.xmland set the key bindings to use that rather than the defaultorg.eclipse.ui.defaultAcceleratorConfigurationthen both the menu hints and the accelerators themselves work correctly, provided the user selects that scheme in preferences. That feels like the wrong approach - the scheme is more a mechanism for users to 'make eclipse like Emacs or IntelliJ'.So I think the solution that circumvents any promiscuous behaviour by core or other plugins is to have a context that is active when the active editor is LSP-enabled. I've bodged together example code and it seemed to work.
Any thoughts from anyone? I'm not proposing to submit it in this state, I hasten to add! I wondered if anyone has any better ideas and in particular someone more familiar with eclipse extension points can think of a declarative way of activating a context - LSP4e has the nice
hasLanguageServerproperty tester but as far as I can see there isn't anything declarative likeactiveWhenfor the context service, hence the ugly use of workbench-level events in my example code. My company in fact uses its ownIContextwith our own language server plugin but we actually use an editor subclass, which means we can override theinitializeKeyBindingScopesmethod. LSP4e at the moment allows consuming plugins to use the generic editor directly and adds in its custom behaviour entirely through platform extensions, which is the preferred way of doing it, so forcing consumers to use a custom editor subclass isn't really an option.