LSP Java backend for the Java layer

[yyoncho]

Fixes #10654

Added new backend for the Java.

[yyoncho]

Pull request must be updated once #10486 is in.

[yyoncho]

Pull request must be updated once #10486 is in.

[sdwolfz]

@yyoncho do you have any small and simple java project I can clone and try this out locally before merging?

[sdwolfz]

@yyoncho do you have any small and simple java project I can clone and try this out locally before merging?

[yyoncho]

@sdwolfz some of the stuff from this request will be redundant when #10486 is in. Can you merge that one first?

[yyoncho]

@sdwolfz some of the stuff from this request will be redundant when #10486 is in. Can you merge that one first?

[yyoncho]

As for the sample project you might test with the following project: https://github.com/codecentric/java8-examples

[yyoncho]

As for the sample project you might test with the following project: https://github.com/codecentric/java8-examples

[sdwolfz]

I will take a look, but that is a big PR and it might take a while until I build up the knowledge to tackle it.

[sdwolfz]

I will take a look, but that is a big PR and it might take a while until I build up the knowledge to tackle it.

[smile13241324]

I have just been involved in #11161 and wanted to start integrating the missing LSP servers anyway then I have found the various open PRs for these.

Is there something I can do to help getting this PR merged @sdwolfz?

[smile13241324]

I have just been involved in #11161 and wanted to start integrating the missing LSP servers anyway then I have found the various open PRs for these.

Is there something I can do to help getting this PR merged @sdwolfz?

[sdwolfz]

@smile13241324 If you can test this PR and confirm it works properly then I will merge it, did not get around to test it yet, and it will be a while until I can grasp all that is happening in #10486, I rather merge small changes first and have improvements be done afterwards instead of big changes at once.

[sdwolfz]

@smile13241324 If you can test this PR and confirm it works properly then I will merge it, did not get around to test it yet, and it will be a while until I can grasp all that is happening in #10486, I rather merge small changes first and have improvements be done afterwards instead of big changes at once.

[smile13241324]

I was just scanning the list of open PRs for LSP server integrations and stumbled over yours.

I am also wanting this to be merged so I have made a code review and found some smaller issues you should have a look at.

[smile13241324]

@sdwolfz I will happily do so, and report back here.

As to the various LSP improvements discussed in #10486, yes please be super careful with them, from my point of view there is lots of stuff unclarified here.

For example:

• Why is the new layer called c++-lsp-layer when it is actually a specific layer to work with additions to the lsp protocol specific to a certain lsp server (cquery).
• Why do we need derived layers, for lsp? I would rather have expected to have everything lsp specific defined in lsp-layer this may also include keybindings. This layer would then be required by each language layer making the language specific adjustments. Or the language layer may require a specific compatibility layer for the used LSP server like the one discussed for cquery which would then load additional keybindings specific to the respective lsp extensions and require the lsp layer. In both cases there is no need for some kind of explicit inheritance mechanism.
• I have noticed that also some non c++ related layers have been changed namely some files in the core like spacemacs-editing/packages.el of which I am not sure what the effects are.
• The loading of (defun lsp/init-lsp-mode () contains code not being part of a use-package block, I am not sure whether this will pose a problem but I would have expected to see the code in the config block as usual.

[smile13241324]

@sdwolfz I will happily do so, and report back here.

As to the various LSP improvements discussed in #10486, yes please be super careful with them, from my point of view there is lots of stuff unclarified here.

For example:

• Why is the new layer called c++-lsp-layer when it is actually a specific layer to work with additions to the lsp protocol specific to a certain lsp server (cquery).
• Why do we need derived layers, for lsp? I would rather have expected to have everything lsp specific defined in lsp-layer this may also include keybindings. This layer would then be required by each language layer making the language specific adjustments. Or the language layer may require a specific compatibility layer for the used LSP server like the one discussed for cquery which would then load additional keybindings specific to the respective lsp extensions and require the lsp layer. In both cases there is no need for some kind of explicit inheritance mechanism.
• I have noticed that also some non c++ related layers have been changed namely some files in the core like spacemacs-editing/packages.el of which I am not sure what the effects are.
• The loading of (defun lsp/init-lsp-mode () contains code not being part of a use-package block, I am not sure whether this will pose a problem but I would have expected to see the code in the config block as usual.

[haydenflinner]

From #10486, I don't think the new layer is meant to be built on top of, but as an example of what the core LSP support layer could do.

Anyway, this is all getting a bit OT -- just added lsp-c-c++ to this PR as a usage example, as hard to evaluate the proposed lsp layer changes in isolation. Perhaps I should withdraw the PR altogether -- I'd welcome some feedback from some of the maintainers one way or the other. @syl20bnr ?

I'm also not sure that the author of this PR meant to delete that README line, as there's no code in the commit that apparently changes anything that's there so I don't see how ENSIME could have been broken.

[haydenflinner]

From #10486, I don't think the new layer is meant to be built on top of, but as an example of what the core LSP support layer could do.

Anyway, this is all getting a bit OT -- just added lsp-c-c++ to this PR as a usage example, as hard to evaluate the proposed lsp layer changes in isolation. Perhaps I should withdraw the PR altogether -- I'd welcome some feedback from some of the maintainers one way or the other. @syl20bnr ?

I'm also not sure that the author of this PR meant to delete that README line, as there's no code in the commit that apparently changes anything that's there so I don't see how ENSIME could have been broken.

[haydenflinner]

Am currently trying out this PR (by git pull --rebase https://github.com/yyoncho/spacemacs.git develop).

I get these errors when starting Spacemacs

                            Warnings:
                                - package lsp-mode not initialized in layer java, you may consider removing this
                                  package from the package list or use the :toggle keyword instead of a `when'
                                  form.      
                                - package lsp-ui not initialized in layer java, you may consider removing this
                                  package from the package list or use the :toggle keyword instead of a `when'
                                  form.      
                                - package company-lsp not initialized in layer java, you may consider removing
                                  this package from the package list or use the :toggle keyword instead of a
                                  `when' form.      

It works great on a simple project I generated with IntelliJ, i.e. just a Main.java. However, I also downloaded the git repo for Apache POI, and I can't get it to work. I have built it in IntelliJ through gradle, so all dependencies are downloaded (not in Eclipse because I'm not installing that crap), but still get the Error from the language Server: Unable to find file [the file I'm in] (Internal Error). I also tried on a fresh clone of the repo. There is a .classpath file at the main level of the repo. Note that I have added the absolute paths to the repos in my .spacemacs, although that feels like a hack that probably shouldn't be merged in.

[haydenflinner]

Am currently trying out this PR (by git pull --rebase https://github.com/yyoncho/spacemacs.git develop).

I get these errors when starting Spacemacs

                            Warnings:
                                - package lsp-mode not initialized in layer java, you may consider removing this
                                  package from the package list or use the :toggle keyword instead of a `when'
                                  form.      
                                - package lsp-ui not initialized in layer java, you may consider removing this
                                  package from the package list or use the :toggle keyword instead of a `when'
                                  form.      
                                - package company-lsp not initialized in layer java, you may consider removing
                                  this package from the package list or use the :toggle keyword instead of a
                                  `when' form.      

It works great on a simple project I generated with IntelliJ, i.e. just a Main.java. However, I also downloaded the git repo for Apache POI, and I can't get it to work. I have built it in IntelliJ through gradle, so all dependencies are downloaded (not in Eclipse because I'm not installing that crap), but still get the Error from the language Server: Unable to find file [the file I'm in] (Internal Error). I also tried on a fresh clone of the repo. There is a .classpath file at the main level of the repo. Note that I have added the absolute paths to the repos in my .spacemacs, although that feels like a hack that probably shouldn't be merged in.

[yyoncho]

@haydenflinner

It works great on a simple project I generated with IntelliJ, i.e. just a Main.java. However, I also downloaded the git repo for Apache POI, and I can't get it to work.

AFAIK the support for Guava in JDT LS server is not that great, so I would recomment to try bigger maven project instead. I guess the gradle project might be imported as well but this would require some manual configuration and probably asking for help in JDT LS project.

I have built it in IntelliJ through gradle, so all dependencies are downloaded (not in Eclipse because I'm not installing that crap), but still get the Error from the language Server: Unable to find file [the file I'm in] (Internal Error). I also tried on a fresh clone of the repo. There is a .classpath file at the main level of the repo. Note that I have added the absolute paths to the repos in my .spacemacs, although that feels like a hack that probably shouldn't be merged in.

There is pending bug for the "absolute path" configuration. Once it is done you will be asked whether you want to import the project once you open it.

[yyoncho]

@haydenflinner

It works great on a simple project I generated with IntelliJ, i.e. just a Main.java. However, I also downloaded the git repo for Apache POI, and I can't get it to work.

AFAIK the support for Guava in JDT LS server is not that great, so I would recomment to try bigger maven project instead. I guess the gradle project might be imported as well but this would require some manual configuration and probably asking for help in JDT LS project.

I have built it in IntelliJ through gradle, so all dependencies are downloaded (not in Eclipse because I'm not installing that crap), but still get the Error from the language Server: Unable to find file [the file I'm in] (Internal Error). I also tried on a fresh clone of the repo. There is a .classpath file at the main level of the repo. Note that I have added the absolute paths to the repos in my .spacemacs, although that feels like a hack that probably shouldn't be merged in.

There is pending bug for the "absolute path" configuration. Once it is done you will be asked whether you want to import the project once you open it.

[yyoncho]

@haydenflinner I tried importing https://github.com/apache/poi and it worked fine by checkouting the repo and adding it to list of workspace folders). Note that initial import might take some time because the JDT is downloading the sources.

[yyoncho]

@haydenflinner I tried importing https://github.com/apache/poi and it worked fine by checkouting the repo and adding it to list of workspace folders). Note that initial import might take some time because the JDT is downloading the sources.

[smile13241324]

Just reviewed your code again. Your changes are fine. However I have found another small issue which would be great if you could have a look at.

Could you please have a look at my comment for layers.el and remove the direct dependency on lsp layer.

[smile13241324]

Thanks for doing the requested changes. I will do a manual test now on a gradle and maven project.

[smile13241324]

I have created two sample projects with netbeans and then tested all mentioned key bindings in the java layer for lsp. For the lsp server I have downloaded the latest version from the given link in the documentation.

You can find the sample project here. They are called gradleTest and mavenTest and consist of one main class with an trivial test class in a separate package.

Here is what I have found, for each line I have marked whether it is something we can fix after the initial merge (Optional) or whether it is something we should fix right now (Must be fixed):

• lsp-java has been installed though the respective backend has not been selected yet -> package declaration should use toggle on lsp backend -> Optional
• gd -> asks for gtags, then jumps to definition when declined -> evil navigation procedures should work out of the box, spacemacs/jump-handlers must be set during layer setup to prefer lsp over gtags -> Optional
  See

  (add-to-list 'spacemacs-jump-handlers-python-mode
               '(anaconda-mode-find-definitions :async t)))

• SPC m g t this keybinding is not defined and should be removed from documentation or be defined -> Must be fixed
• SPC m g a this keybinding does not "Open type" but calls "xref-find-apropos" which is something completely different. It should be renamed to "search in project" or something similiar. -> Must be fixed
• SPC m g A Peek type using lsp-ui -> keybinding does ask for symbol, should rather take symbol under cursor -> Optional
• SPC m p u Update user settings -> Should be renamed into "refresh user settings", update user settings is a bit misleading as there is no dialog changing user settings involved here but only the init file is reloaded by the lsp server. -> Must be fixed
• SPC m r a i Add import -> Add import does not work as expected for gradle project, I have tried to add an import on my class under point, the import was not added -> Optional
• SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional
• SPC m r c f Create field -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e c Extract constant -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e l Extract local -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e m Extract method -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed
• SPC m a n Actionable notifications -> got no message back when used in my sample projects not sure what it should do -> Optional
• SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional
• SPC m m t C-a Clean and run all tests -> C-a binding is wrong should be bound to C-a but is bound to C - a instead -> Must be fixed

[smile13241324]

I have created two sample projects with netbeans and then tested all mentioned key bindings in the java layer for lsp. For the lsp server I have downloaded the latest version from the given link in the documentation.

You can find the sample project here. They are called gradleTest and mavenTest and consist of one main class with an trivial test class in a separate package.

Here is what I have found, for each line I have marked whether it is something we can fix after the initial merge (Optional) or whether it is something we should fix right now (Must be fixed):

• lsp-java has been installed though the respective backend has not been selected yet -> package declaration should use toggle on lsp backend -> Optional
• gd -> asks for gtags, then jumps to definition when declined -> evil navigation procedures should work out of the box, spacemacs/jump-handlers must be set during layer setup to prefer lsp over gtags -> Optional
  See

  (add-to-list 'spacemacs-jump-handlers-python-mode
               '(anaconda-mode-find-definitions :async t)))

• SPC m g t this keybinding is not defined and should be removed from documentation or be defined -> Must be fixed
• SPC m g a this keybinding does not "Open type" but calls "xref-find-apropos" which is something completely different. It should be renamed to "search in project" or something similiar. -> Must be fixed
• SPC m g A Peek type using lsp-ui -> keybinding does ask for symbol, should rather take symbol under cursor -> Optional
• SPC m p u Update user settings -> Should be renamed into "refresh user settings", update user settings is a bit misleading as there is no dialog changing user settings involved here but only the init file is reloaded by the lsp server. -> Must be fixed
• SPC m r a i Add import -> Add import does not work as expected for gradle project, I have tried to add an import on my class under point, the import was not added -> Optional
• SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional
• SPC m r c f Create field -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e c Extract constant -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e l Extract local -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m r e m Extract method -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional
• SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed
• SPC m a n Actionable notifications -> got no message back when used in my sample projects not sure what it should do -> Optional
• SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional
• SPC m m t C-a Clean and run all tests -> C-a binding is wrong should be bound to C-a but is bound to C - a instead -> Must be fixed

[yyoncho]

I have created two sample projects with netbeans and then tested all mentioned key bindings in the java layer for lsp. For the lsp server I have downloaded the latest version from the given link in the documentation.

You can find the sample project here. They are called gradleTest and mavenTest and consist of one main class with an trivial test class in a separate package.

Here is what I have found, for each line I have marked whether it is something we can fix after the initial merge (Optional) or whether it is something we should fix right now (Must be fixed):

lsp-java has been installed though the respective backend has not been selected yet -> package declaration should use toggle on lsp backend -> Optional
gd -> asks for gtags, then jumps to definition when declined -> evil navigation procedures should work out of the box, spacemacs/jump-handlers must be set during layer setup to prefer lsp over gtags -> Optional

Do the gd works on classes ouside of the project(e. g. String)?

See
(add-to-list 'spacemacs-jump-handlers-python-mode
'(anaconda-mode-find-definitions :async t)))
SPC m g t this keybinding is not defined and should be removed from documentation or be defined -> Must be fixed

I will add the missing binding, it is new command.

SPC m g a this keybinding does not "Open type" but calls "xref-find-apropos" which is something completely different. It should be renamed to "search in project" or something similiar. -> Must be fixed

In lsp java case xref-find-appropos is looking only for java types and it behaves the same as Open Type in Eclipse/Idea. I could rename it to "search type in project".

SPC m g A Peek type using lsp-ui -> keybinding does ask for symbol, should rather take symbol under cursor -> Optional
SPC m p u Update user settings -> Should be renamed into "refresh user settings", update user settings is a bit misleading as there is no dialog changing user settings involved here but only the init file is reloaded by the lsp server. -> Must be fixed

Ok.

SPC m r a i Add import -> Add import does not work as expected for gradle project, I have tried to add an import on my class under point, the import was not added -> Optional

As I mentioned above, I suspect that the project is not configured correctly in lsp-java--workspace-folders. Do you see the code actions on the right side? (check the screenshot).

SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional

this will work only if you are on a undeclared field.

SPC m r c f Create field -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e c Extract constant -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e l Extract local -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e m Extract method -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed

When you trigger build you may see the output in the messages buffer, I am not sure what are your expectations here.

SPC m a n Actionable notifications -> got no message back when used in my sample projects not sure what it should do -> Optional

This will be triggered only if the server has send an actionable notification, e. g. if the automatic pom update is disabled and the file watch is enabled and you update the pom.xml.

SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional

This is not part of this review should I go and fix it?

SPC m m t C-a Clean and run all tests -> C-a binding is wrong should be bound to C-a but is bound to C - a instead -> Must be fixed

ditto.

[yyoncho]

I have created two sample projects with netbeans and then tested all mentioned key bindings in the java layer for lsp. For the lsp server I have downloaded the latest version from the given link in the documentation.

You can find the sample project here. They are called gradleTest and mavenTest and consist of one main class with an trivial test class in a separate package.

Here is what I have found, for each line I have marked whether it is something we can fix after the initial merge (Optional) or whether it is something we should fix right now (Must be fixed):

lsp-java has been installed though the respective backend has not been selected yet -> package declaration should use toggle on lsp backend -> Optional
gd -> asks for gtags, then jumps to definition when declined -> evil navigation procedures should work out of the box, spacemacs/jump-handlers must be set during layer setup to prefer lsp over gtags -> Optional

Do the gd works on classes ouside of the project(e. g. String)?

See
(add-to-list 'spacemacs-jump-handlers-python-mode
'(anaconda-mode-find-definitions :async t)))
SPC m g t this keybinding is not defined and should be removed from documentation or be defined -> Must be fixed

I will add the missing binding, it is new command.

SPC m g a this keybinding does not "Open type" but calls "xref-find-apropos" which is something completely different. It should be renamed to "search in project" or something similiar. -> Must be fixed

In lsp java case xref-find-appropos is looking only for java types and it behaves the same as Open Type in Eclipse/Idea. I could rename it to "search type in project".

SPC m g A Peek type using lsp-ui -> keybinding does ask for symbol, should rather take symbol under cursor -> Optional
SPC m p u Update user settings -> Should be renamed into "refresh user settings", update user settings is a bit misleading as there is no dialog changing user settings involved here but only the init file is reloaded by the lsp server. -> Must be fixed

Ok.

SPC m r a i Add import -> Add import does not work as expected for gradle project, I have tried to add an import on my class under point, the import was not added -> Optional

As I mentioned above, I suspect that the project is not configured correctly in lsp-java--workspace-folders. Do you see the code actions on the right side? (check the screenshot).

SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional

this will work only if you are on a undeclared field.

SPC m r c f Create field -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e c Extract constant -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e l Extract local -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

same as above

SPC m r e m Extract method -> Tried that on my class "lsp-java-execute-matching-action: Unable to find action" was briefly shown -> Optional

SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed

When you trigger build you may see the output in the messages buffer, I am not sure what are your expectations here.

SPC m a n Actionable notifications -> got no message back when used in my sample projects not sure what it should do -> Optional

This will be triggered only if the server has send an actionable notification, e. g. if the automatic pom update is disabled and the file watch is enabled and you update the pom.xml.

SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional

This is not part of this review should I go and fix it?

SPC m m t C-a Clean and run all tests -> C-a binding is wrong should be bound to C-a but is bound to C - a instead -> Must be fixed

ditto.

[yyoncho]

@haydenflinner as a side note which version of java do you use?

[yyoncho]

@haydenflinner as a side note which version of java do you use?

[smile13241324]

Do the gd works on classes ouside of the project(e. g. String)?
No, when I create String asdf = "asdf"; and press gd on String I am asked File GTAGS not found. Run 'gtags'? (y or n) after declining the jump is done.

When I check spacemacs-jump-handlers it contains spacemacs/helm-gtags-maybe-dwim evil-goto-definition dumb-jump-go. spacemacs-jump-handlers-java-mode contains only spacemacs/helm-gtags-maybe-dwim.

However when I double check python layer I am having the same behaviour, so maybe thats a general issue. Lets investigate this after the initial merge.

[smile13241324]

Do the gd works on classes ouside of the project(e. g. String)?
No, when I create String asdf = "asdf"; and press gd on String I am asked File GTAGS not found. Run 'gtags'? (y or n) after declining the jump is done.

When I check spacemacs-jump-handlers it contains spacemacs/helm-gtags-maybe-dwim evil-goto-definition dumb-jump-go. spacemacs-jump-handlers-java-mode contains only spacemacs/helm-gtags-maybe-dwim.

However when I double check python layer I am having the same behaviour, so maybe thats a general issue. Lets investigate this after the initial merge.

[smile13241324]

In lsp java case xref-find-appropos is looking only for java types and
it behaves the same as Open Type in Eclipse/Idea. I could rename it to "search type in project".

Agreed :)

[smile13241324]

In lsp java case xref-find-appropos is looking only for java types and
it behaves the same as Open Type in Eclipse/Idea. I could rename it to "search type in project".

Agreed :)

[yyoncho]

@smile13241324
evil-goto-definition is calling xref-find-definition which in case of lsp-mode must use lsp backend. I believe that you have another layer (e. g. ggtags) which causing the issue. I suspect that smt is replacing the xref backend as well so in the end no lsp method is called when you are calling gd (thus goto string does not work). This could be checked by setting lsp-print-io to t and calling gd.

[yyoncho]

@smile13241324
evil-goto-definition is calling xref-find-definition which in case of lsp-mode must use lsp backend. I believe that you have another layer (e. g. ggtags) which causing the issue. I suspect that smt is replacing the xref backend as well so in the end no lsp method is called when you are calling gd (thus goto string does not work). This could be checked by setting lsp-print-io to t and calling gd.

[yyoncho]

@smile13241324 the question about the java version was for actually for you. Can you share your java version?

[yyoncho]

@smile13241324 the question about the java version was for actually for you. Can you share your java version?

[smile13241324]

As I mentioned above, I suspect that the project is not configured correctly in lsp-java--workspace-folders. Do you see the code actions on the right side? (check the screenshot).

I do not see these code actions when I copy the line ArrayList al; however SPC m r a i Add import and SPC m e a lsp-execute-code-action do work as expected now. Maybe I have forgotten to restart emacs after I have changed lsp-java--workspace-folders, lets count this as ok.

Here is as it looks on my machine:

SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional
This is not part of this review should I go and fix it?

Is it not? I have just noticed this when I was using the maven keybindings in the lsp section so I have expected these to be LSP specific. If these key bindings are general to the java layer just leave it for now.

Same for the C-a binding if it is not LSP specific just leave it for now.

@yyoncho my java version is:

openjdk version "1.8.0_172"
OpenJDK Runtime Environment (build 1.8.0_172-b11)
OpenJDK 64-Bit Server VM (build 25.172-b11, mixed mode)

[smile13241324]

As I mentioned above, I suspect that the project is not configured correctly in lsp-java--workspace-folders. Do you see the code actions on the right side? (check the screenshot).

I do not see these code actions when I copy the line ArrayList al; however SPC m r a i Add import and SPC m e a lsp-execute-code-action do work as expected now. Maybe I have forgotten to restart emacs after I have changed lsp-java--workspace-folders, lets count this as ok.

Here is as it looks on my machine:

SPC m m c r Clean and compile -> Maven clean compile causes two parallel compile buffers prompting the user for killing the other this should be fixed -> Optional
This is not part of this review should I go and fix it?

Is it not? I have just noticed this when I was using the maven keybindings in the lsp section so I have expected these to be LSP specific. If these key bindings are general to the java layer just leave it for now.

Same for the C-a binding if it is not LSP specific just leave it for now.

@yyoncho my java version is:

openjdk version "1.8.0_172"
OpenJDK Runtime Environment (build 1.8.0_172-b11)
OpenJDK 64-Bit Server VM (build 25.172-b11, mixed mode)

[yyoncho]

@smile13241324 Great! Not seeing the actions is a bit odd, I guess is related to lsp-ui. I will do the change tomorrow, to summarize it:

1. I will add missing binding lsp-goto-type-definition
2. I will update the documentation for xref-find-appropos

[yyoncho]

@smile13241324 Great! Not seeing the actions is a bit odd, I guess is related to lsp-ui. I will do the change tomorrow, to summarize it:

1. I will add missing binding lsp-goto-type-definition
2. I will update the documentation for xref-find-appropos

[smile13241324]

SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional
this will work only if you are on a undeclared field.

Works, when done on the right place.

As to SPC m r c f Create field SPC m r e c Extract constant SPC m r e l Extract local SPC m r e m Extract method

Do not longer show an error message so I expect that they work now, however it is not always clear to me on which symbols these operations should be done i.e. Extract local, on a local variable in a methods context I would have expected to have it moved to the enclosing class context.

Anyway I am finding myself always using SPC m e a and then picking my choice from helm than using single refactor operations. So I expect its just me :).

[smile13241324]

SPC m r c p Create parameter -> Tried that on my class did not do anything -> Optional
this will work only if you are on a undeclared field.

Works, when done on the right place.

As to SPC m r c f Create field SPC m r e c Extract constant SPC m r e l Extract local SPC m r e m Extract method

Do not longer show an error message so I expect that they work now, however it is not always clear to me on which symbols these operations should be done i.e. Extract local, on a local variable in a methods context I would have expected to have it moved to the enclosing class context.

Anyway I am finding myself always using SPC m e a and then picking my choice from helm than using single refactor operations. So I expect its just me :).

[smile13241324]

SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed

When you trigger build you may see the output in the messages buffer, I am not sure what are your expectations here.

Well I would have expected to see a compile buffer opened by popwin with the actual messages from the compile process, just like it is done when I call the build system specific versions. Currently when I execute SPC m c c I am not getting any information about the build status.

However I have just seen that this function comes from lsp-java so I think this is also out of scope here.

Thanks for checking my gtags error. I have done as you described and it looks like that LSP is called, however only after ggtags has been called causing that annoying prompt. When I remove gtags layer it just works. I think this is a general issue with gtags layer. So this is also out of scope for this PR.

Finally you have reached my last comment :) thanks for your patience. When you have made the last two adjustments I will recommend this to be merged.

[smile13241324]

SPC m c c Build project -> Does not work, tried to compile my gradle as well as my maven project, in both cases nothing happened, however the build system specific version work pretty well. -> Must be fixed

When you trigger build you may see the output in the messages buffer, I am not sure what are your expectations here.

Well I would have expected to see a compile buffer opened by popwin with the actual messages from the compile process, just like it is done when I call the build system specific versions. Currently when I execute SPC m c c I am not getting any information about the build status.

However I have just seen that this function comes from lsp-java so I think this is also out of scope here.

Thanks for checking my gtags error. I have done as you described and it looks like that LSP is called, however only after ggtags has been called causing that annoying prompt. When I remove gtags layer it just works. I think this is a general issue with gtags layer. So this is also out of scope for this PR.

Finally you have reached my last comment :) thanks for your patience. When you have made the last two adjustments I will recommend this to be merged.

[yyoncho]

I have updated the review. The missing shortcut ,gt was actually bound to ,gd .

Build project exposed from lsp-java is more like rebuild and show the new warnings/errors. The process is happening in JDT server thus no popup console. As a side note, I have debugger integration for lsp-java via https://github.com/yyoncho/dap-mode and I will update the layer once dap-mode is in melpa.

[yyoncho]

I have updated the review. The missing shortcut ,gt was actually bound to ,gd .

Build project exposed from lsp-java is more like rebuild and show the new warnings/errors. The process is happening in JDT server thus no popup console. As a side note, I have debugger integration for lsp-java via https://github.com/yyoncho/dap-mode and I will update the layer once dap-mode is in melpa.

[smile13241324]

Great :). I have had a look at your latest changes and it looks pretty good now.

@sdwolfz I think this is ready to be merged. It works pretty well in maven as well as gradle projects. Of course there is some polish left to be done but I think this belongs into separate PRs.

[smile13241324]

Great :). I have had a look at your latest changes and it looks pretty good now.

@sdwolfz I think this is ready to be merged. It works pretty well in maven as well as gradle projects. Of course there is some polish left to be done but I think this belongs into separate PRs.

[smile13241324]

@yyoncho Whoa dab mode sounds really cool, I have just compared the UI with realgud and it looks pretty much more IDEish. Also it would allow to simply use an existing debugging server instead of making a debugger integration for realgud right?

[smile13241324]

@yyoncho Whoa dab mode sounds really cool, I have just compared the UI with realgud and it looks pretty much more IDEish. Also it would allow to simply use an existing debugging server instead of making a debugger integration for realgud right?

[yyoncho]

@smile13241324 Yes, you pretty much have to add an adapter(runner) for your language. Here it is the python example - https://github.com/yyoncho/dap-mode/blob/master/dap-python.el#L41 - it starts the python debug server and from now on dap-mode takes care. And yes, the main goal is to have IDEish experience without losing emacs extensibility, e. g. you may attach a hook when a breakpoint is hit. Link to the list of available
adapters https://microsoft.github.io/debug-adapter-protocol/implementors/adapters/

[yyoncho]

@smile13241324 Yes, you pretty much have to add an adapter(runner) for your language. Here it is the python example - https://github.com/yyoncho/dap-mode/blob/master/dap-python.el#L41 - it starts the python debug server and from now on dap-mode takes care. And yes, the main goal is to have IDEish experience without losing emacs extensibility, e. g. you may attach a hook when a breakpoint is hit. Link to the list of available
adapters https://microsoft.github.io/debug-adapter-protocol/implementors/adapters/

[sdwolfz]

Thanks for all the hard work @yyoncho and @smile13241324. I'll try it out for myself and then merge it.

[sdwolfz]

Thanks for all the hard work @yyoncho and @smile13241324. I'll try it out for myself and then merge it.

[sdwolfz]

A few more changes needed:

1. The ~/.emacs.d/workspace/ directory needs to be added to .gitignore since that is created by java-lsp. I don't know if you can control the location of that directory (java is known to leave around annoying directories like that) but of you can please move it next to where the lsp server files sit.

2. Please add to the docs a one liner shell command to download and extract the lsp server files.

3. I don't like that you need to manually specify the project workspace directories in the lsp-java--workspace-folders variable, looks to be a private variable, but I guess this is related to the java-lsp package and there is nothing you can do about it here. Ideally it would infer the current project via projectile (maybe this would be a feature request for upstream).

4. Please squash all commits into one and rebase with latest develop.

Anyway, thanks for the great work, it's really close to being merged.

[robbyoconnor]

Also, fix formatting issues. Pay attention to CI failures.

[robbyoconnor]

Also, fix formatting issues. Pay attention to CI failures.

[yyoncho]

@sdwolfz

The lsp-java-workspace-dir can controll the workspace location, I could change the default value in lsp-java or in spacemacs. Please share your thoughts on what will be the best solution.

There are emacs-lsp/lsp-java#39 and emacs-lsp/lsp-mode#345 . I am planning to fix them in some of the upcoming releases of lsp-java/lsp-mode + treemacs treemacs integration (e. g. it list all maven deps, namespaces, classes, folder containing errors etc).

[yyoncho]

@sdwolfz

The lsp-java-workspace-dir can controll the workspace location, I could change the default value in lsp-java or in spacemacs. Please share your thoughts on what will be the best solution.

There are emacs-lsp/lsp-java#39 and emacs-lsp/lsp-mode#345 . I am planning to fix them in some of the upcoming releases of lsp-java/lsp-mode + treemacs treemacs integration (e. g. it list all maven deps, namespaces, classes, folder containing errors etc).

[yyoncho]

@robbyoconnor can you advice on fixing the formatting issues, here it what I see in the failed job:

Cannot open load file: No such file or directory, /opt/spacedoc/emacs-tools/docfmt/run.el
Documentation formatting script failed

[yyoncho]

@robbyoconnor can you advice on fixing the formatting issues, here it what I see in the failed job:

Cannot open load file: No such file or directory, /opt/spacedoc/emacs-tools/docfmt/run.el
Documentation formatting script failed

[sdwolfz]

I think you only need to rebase to fix that issue as it's using some old code. Once you rebase you should get a diff in case your formatting is wrong.

Ideally the workspace related issues should be fixed as close to upstream as possible. But I don't want to delay this PR to much.

[sdwolfz]

I think you only need to rebase to fix that issue as it's using some old code. Once you rebase you should get a diff in case your formatting is wrong.

Ideally the workspace related issues should be fixed as close to upstream as possible. But I don't want to delay this PR to much.

[yyoncho]

Rebased & squashed.

[yyoncho]

Rebased & squashed.

[sdwolfz]

Thank you ❤️!
And congratulations on your first Spacemacs PR 🎉!
Cherry-picked into develop branch, you can safely delete your branch.

[sdwolfz]

Thank you ❤️!
And congratulations on your first Spacemacs PR 🎉!
Cherry-picked into develop branch, you can safely delete your branch.