mfussenegger / nvim-jdtls

Extensions for the built-in LSP support in Neovim for eclipse.jdt.ls
GNU General Public License v3.0
1.09k stars 62 forks source link

Adaptions for multiple clients #369

Closed TheBlob42 closed 1 year ago

TheBlob42 commented 1 year ago

I am usually working on multiple Java projects within the same neovim instance. Because of this I have encountered several issues that this PR should address and fix.

Don't cache the data directory

In the current implementation of setup.lua the "latest" data directory is being cached and used for commands like JdtShowLogs and JdtWipeDataAndRestart:

To solve this I have removed the data_dir variable from setup.lua and instead extract the correct directory based on the client used for the current buffer dynamically. Since the corresponding commands are not called regularly I don't expect any performance impact of not having a cache here.

Do not overwrite dap configurations of "older" projects

Calling setup_dap_main_class_configs for the first time, the already existing dap configurations for java are being saved. The newly created one for the current project are then added to them.

When we now call the function again for another Java project later on the new configurations for the new projects will be added BUT the ones for first project will be overwritten because the saved configurations will not be updated. Therefore I only have the dap configurations for the "newest" project.

My solution is to remove the orig_configurations cache variable. On a configuration update I check based on the cwd and name of the configuration which ones needs to be updated and just add the rest to the already saved ones. This way opening a new project will add its configuration (and not replacing anything). Reloading the configurations for a project will not add these entries again but instead replace the existing ones with the new entries.

Use correct JDTLS for starting a debug session

start_debug_adapter does not pass a buffer to the util.execute_command therefore the function will search through all JDTLS clients and by default uses the first one (clients[1].request(...)) it finds. If we start a debugging session for a project which is not attached to this client DAP will not jump to the correct locations in the code etc. which makes debugging very hard.

To solve this I save the root_dir of the current client as cwd of the debug configuration (these two values should be equal by default). On starting the debug adapter we can use this information to extract the correct client to use for this debug session.

The approach is a little "wonky" as we are fetching the correct client, to then fetch one of the attached buffers to then use the buffer to fetch the correct client again, but I did not wanted to adapt the signature of util.execute_command here.

Final note

Sorry for all this text :smile:

If you do not understand some of my solutions feel free to ask and if you see something which could be enhanced please tell me. Also I am unsure on some of the variable names, so always happy for input here. I am currently using this code since a few days and so far could not identify any issues with it, but I will continue monitoring it. If you and/or other users would give it a try to check for issues I would appreciate this.

Spacefreak18 commented 1 year ago

Great work,

I think i may need this as well. I am pointing to your repo and am trying it out.

Do I no longer need to pass in the project name? Does neovim now infer the project name from the first pom.xml it can find in a parent directory of the source file?

mfussenegger commented 1 year ago

My solution is to remove the orig_configurations cache variable. On a configuration update I check based on the cwd and name of the configuration which ones needs to be updated and just add the rest to the already saved ones. This way opening a new project will add its configuration (and not replacing anything). Reloading the configurations for a project will not add these entries again but instead replace the existing ones with the new entries.

I think merging by name could be another option, but the variant in this PR seems fine too.

TheBlob42 commented 1 year ago

I think merging by name could be another option, but the variant in this PR seems fine too.

Sounds interesting, is there an easy way to perform such a merge?
I thought about the vim.tbl_merge and vim.tbl_deep_extend but were dealing with a list like table here.

mfussenegger commented 1 year ago

I think merging by name could be another option, but the variant in this PR seems fine too.

Sounds interesting, is there an easy way to perform such a merge? I thought about the vim.tbl_merge and vim.tbl_deep_extend but were dealing with a list like table here.

You would have to remove old entries and add new ones. Similar to https://github.com/mfussenegger/nvim-dap/blob/f0573ea26f29702ad9aa1546e102adb2f5b7ac3a/lua/dap/ext/vscode.lua#L140-L155

But not necessary. I can merge this as is (with the suggested commented added) and you could follow up on this if you think it's an improvement.

TheBlob42 commented 1 year ago

A I completely forgot about using table.remove(...) for such a logic, thanks for the example

I have added the suggested comment and will think about a refactor of that "merge" logic. Would open a new PR if I can come up with an easier (to understand) implementation for it