Add organize imports command by yaohaizh · Pull Request #341

Add organize imports command by yaohaizh · Pull Request #341 · redhat-developer/vscode-java

Choose a reason for hiding this comment

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

Something happened with the indentation here and below.

Choose a reason for hiding this comment

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

basically tabs were replaced with spaces. I think we need to set a project preference for indentation. But until then we should try to keep indentation coherent all over the place.
Ultimately we can do a major sweep to format all files at once, but that's the subject of another commit.

Choose a reason for hiding this comment

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

I would like to keep the indentation consistent, and will accept either tab or whitespace.

As I went through the vscode-java codebase, there are mixed with tab and whitespaces. We need make them consistent anyway.

I can't bind a keyboard shortcut to the organize import command from the VSCode UI.

It works when editing the keybindings.json manually.

However, when calling the shortcut on a file with imports to organize, an error occurs on the server:


[Error - 19:07:04] 24-Oct-2017 7:02:16 PM Failed to resolve [object Object]
Illegal character in path at index 0: [object Object]
java.net.URISyntaxException: Illegal character in path at index 0: [object Object]
	at java.net.URI$Parser.fail(URI.java:2848)
	at java.net.URI$Parser.checkChars(URI.java:3021)
	at java.net.URI$Parser.parseHierarchical(URI.java:3105)
	at java.net.URI$Parser.parse(URI.java:3063)
	at java.net.URI.<init>(URI.java:588)
	at org.eclipse.jdt.ls.core.internal.JDTUtils.toURI(JDTUtils.java:591)
	at org.eclipse.jdt.ls.core.internal.JDTUtils.resolveCompilationUnit(JDTUtils.java:107)
	at org.eclipse.jdt.ls.core.internal.JDTDelegateCommandHandler.organizeImports(JDTDelegateCommandHandler.java:50)
	at org.eclipse.jdt.ls.core.internal.JDTDelegateCommandHandler.executeCommand(JDTDelegateCommandHandler.java:39)
	at org.eclipse.jdt.ls.core.internal.handlers.WorkspaceExecuteCommandHandler$1.run(WorkspaceExecuteCommandHandler.java:133)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
	at org.eclipse.jdt.ls.core.internal.handlers.WorkspaceExecuteCommandHandler.executeCommand(WorkspaceExecuteCommandHandler.java:128)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$2(JDTLanguageServer.java:258)
	at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
	at java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
	at java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:443)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

Choose a reason for hiding this comment

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

Can we skip the vscode prefix? Having all commands under the java namespace is more consistent

Choose a reason for hiding this comment

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

again, skip vscode prefix

Choose a reason for hiding this comment

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

This cannot be skip. There are two commands here actually.

One is appeared on the VSCode UI side, which is starting with prefix "vscode"
Another one is just starting with "java...", which used for underlying talking with JDT.LS, which is neutral.

Choose a reason for hiding this comment

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

Does having the same command id for client and server cause any issue?

Choose a reason for hiding this comment

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

need to check the uri is actually an Uri

Choose a reason for hiding this comment

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

any way to reuse what we have L156?

Choose a reason for hiding this comment

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

can we call the command from a folder (not just files) in the explorer?

Choose a reason for hiding this comment

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

can we enable it for folders too?

Any idea why calling a keyboard shortcut doesn't work? With:

{
    "key": "alt+cmd+o",
    "when": "editorTextFocus && !editorReadnly",
    "command": "java.edit.organizeImports"
}

the command receives an empty Object, instead of a URI, so nothing happens

It seems the keybinding doesn't provide any parameters as the context menu do. We need change the behavior of these commands.

Signed-off-by: Yaohai Zheng <yaozheng@microsoft.com>

For adding commands of "Organize Imports" in folder/workspace in VSCode, there is an issue here. We cannot determine current project(Could we? ). If so, the "Organize Imports" will be appeared in all VSCode instance, which is not desired behavior.

yaohaizh changed the title ~~Add organize imports command[WIP]~~ Add organize imports command

Oct 31, 2017

Ok, we can figure out how to get folders working later on the client side. Though I still think we can have it supported on the server side

@aeschli that's what @yaohaizh did. The question is how to enable it for folders of java projects?

Merged as 9779734 (with keybinding to Shift+Alt+o)