Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initialize ExecuteCommandProvider for the server's capabilities #424

Merged
merged 1 commit into from
Nov 14, 2017
Merged

Initialize ExecuteCommandProvider for the server's capabilities #424

merged 1 commit into from
Nov 14, 2017

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Nov 10, 2017

Fixes #399

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

fbricon
fbricon requested changes Nov 10, 2017
View reviewed changes
Copy link
Contributor

@fbricon fbricon left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only returns delegated commands. We also need to report the ones from the LSP extension provided by jdt.ls
Sorry my mistake, spec says it should be the commands executable from workspace/executeCommand.

fbricon
fbricon reviewed Nov 10, 2017
View reviewed changes
org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandlerTest.java Outdated
Mockito.when(preferenceManager.getClientPreferences()).thenReturn(mockCapabilies);
InitializeResult result = initialize(false);
List<String> commands = result.getCapabilities().getExecuteCommandProvider().getCommands();
assertTrue(commands.size() > 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertFalse(commands.isEmpty())

fbricon
fbricon reviewed Nov 10, 2017
View reviewed changes
org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandlerTest.java Outdated
Mockito.when(mockCapabilies.isExecuteCommandDynamicRegistrationSupported()).thenReturn(Boolean.TRUE);
Mockito.when(preferenceManager.getClientPreferences()).thenReturn(mockCapabilies);
InitializeResult result = initialize(true);
assertTrue(result.getCapabilities().getExecuteCommandProvider() == null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNull(result.getCapabilities().getExecuteCommandProvider())

fbricon
fbricon reviewed Nov 10, 2017
View reviewed changes
org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandlerTest.java Outdated

@Test
public void testExecuteCommandProvider() throws Exception {
ClientPreferences mockCapabilies = Mockito.mock(ClientPreferences.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use static imports for Mockito stuff, makes the code less bloated

fbricon
fbricon requested changes Nov 10, 2017
View reviewed changes
Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic registration is missing from JDTLanguageServer.didChangeConfiguration(DidChangeConfigurationParams params)

@snjeza
Copy link
Contributor Author

snjeza commented Nov 11, 2017

retest this please

@fbricon
Copy link
Contributor

fbricon commented Nov 11, 2017

Nope still missing dynamic registration in
https://github.com/eclipse/eclipse.jdt.ls/blob/9817e229dc135bea0ebd268ac77b889d55bd1d74/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java#L208-L253

@snjeza
Copy link
Contributor Author

snjeza commented Nov 11, 2017

@fbricon See https://github.com/snjeza/eclipse.jdt.ls/blob/ad4f85ac499a7a4d578290439a95e6e06dedc249/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandlerTest.java#L85

@fbricon
Copy link
Contributor

fbricon commented Nov 11, 2017

@snjeza no, the client never receives the commands if dynamic registration is on, that's why you need to add it to JDTLanguageServer.didChangeConfiguration, just like the others.

fbricon
fbricon reviewed Nov 13, 2017
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java Outdated
@@ -249,6 +249,13 @@ public void didChangeConfiguration(DidChangeConfigurationParams params) {
unregisterCapability(Preferences.RENAME_ID, Preferences.TEXT_DOCUMENT_RENAME);
}
}
if (preferenceManager.getClientPreferences().isExecuteCommandDynamicRegistrationSupported()) {
if (preferenceManager.getPreferences().isExecuteCommandEnabled()) {
registerCapability(Preferences.EXECUTE_COMMAND_ID, Preferences.WORKSPACE_EXECUTE_COMMAND);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure you need to return new ExecuteCommandOptions(new ArrayList<>(commands)) (see CodeLensOptions L233)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to update JDTLanguageServerTest as well

@snjeza snjeza force-pushed the issue-399 branch 2 times, most recently from 427d143 to 7e214f7 Compare November 13, 2017 21:22
@snjeza
Copy link
Contributor Author

snjeza commented Nov 13, 2017

retest this please

fbricon
fbricon reviewed Nov 13, 2017
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java Outdated
if (preferenceManager.getClientPreferences().isExecuteCommandDynamicRegistrationSupported()) {
if (preferenceManager.getPreferences().isExecuteCommandEnabled()) {
unregisterCapability(Preferences.EXECUTE_COMMAND_ID, Preferences.WORKSPACE_EXECUTE_COMMAND);
registerCapability(Preferences.EXECUTE_COMMAND_ID, Preferences.WORKSPACE_EXECUTE_COMMAND, new ExecuteCommandCapabilities(true));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/ExecuteCommandCapabilities(true)/new ExecuteCommandOptions(new ArrayList<>(WorkspaceExecuteCommandHandler.getCommands())

@snjeza
Initialize ExecuteCommandProvider for the server's capabilities
14d5d17 
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza
Copy link
Contributor Author

snjeza commented Nov 13, 2017

I have updated the PR.

@fbricon
Copy link
Contributor

fbricon commented Nov 14, 2017

Ok so vscode receives:

[Trace - 21:52:28] Received request 'client/registerCapability - (5)'.
Params: {
    "registrations": [
        {
            "id": "18163e21-97e7-4ae8-b51c-5cdb20e639b1",
            "method": "workspace/executeCommand",
            "registerOptions": {
                "commands": [
                    "java.edit.organizeImports",
                    "vscode.java.fetchUsageData",
                    "vscode.java.resolveClasspath",
                    "vscode.java.resolveMainClass",
                    "vscode.java.updateDebugSettings",
                    "vscode.java.buildWorkspace",
                    "vscode.java.startDebugSession"
                ]
            }
        }
    ]

@fbricon fbricon mentioned this pull request Nov 14, 2017
Open
@fbricon fbricon merged commit 8dd902b into eclipse-jdtls:master Nov 14, 2017
@fbricon
Copy link
Contributor

fbricon commented Nov 14, 2017

The PR does the job, however, I'm wondering whether we should migrate all other commands (classFileContents, projectConfigurationUpdate, buildWorkspace) to workspace/executeCommand too, that'd make them discoverable by client developers.

@gorkem, @aeschli, @tsmaeder WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon fbricon approved these changes

@gorkem gorkem Awaiting requested review from gorkem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@snjeza @fbricon