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

fix(java): don't accumulate on_attach, and make more configurable #1388

Merged
merged 2 commits into from
Sep 4, 2023
Merged

fix(java): don't accumulate on_attach, and make more configurable #1388

merged 2 commits into from
Sep 4, 2023

Conversation

agriffis
Copy link
Contributor

@agriffis agriffis commented Aug 28, 2023 •
edited

This is a replacement for #1300. It keeps the fixes from that PR:

  1. Reverse dependency between nvim-jdtls and nvim-lspconfig so the former isn't loaded unnecessarily.
  2. Enable cmp doc preview.
  3. Use which-key to register all the keymap to avoid the unamed(+prefix) group.
  4. Don't accumulate calls to on_attach for each java buffer opened.
  5. Register keymaps and setup dap after the LSP fully attached.

and adds the following fixes:

  1. Lazy load the plugin with ft = { "java" } but additionally handle the race condition internally by calling attach_jdtls() directly when the plugin loads.
  2. Properly check whether debug is enabled with Util.has("nvim-dap") and mason_registry.is_installed("java-debug-adapter") (the original code in feat(java): better java lang support #1300 called mason_registry.has_package() which isn't the right check)

and improves configuration with the following optional in opts:

  • opts.root_dir function to find root dir for given filename
  • opts.project_name function to find project name for given root dir
  • opts.jdtls_config_dir function to find config dir for given project name
  • opts.jdtls_workspace_dir function to find workspace dir for given project name
  • opts.cmd base command, so you can use { "java", ... } instead of the script
  • opts.full_cmd function to compute the full command from the base command
  • opts.jdtls table (or function modifying/returning table) of configuration for require("jdtls").start_or_attach(config)
  • opts.dap can be false to disable, or table (or function modifying/returning table) of configuration for require("jdtls").setup_dap(config)
  • opts.test can be false to disable
  • opts.on_attach can augment the existing LspAttach autocmd for additional keybindings

For example you can configure a replacement jdtls command to avoid the script:

return {
  "mfussenegger/nvim-jdtls",
  opts = {
    cmd = { "java", ... }
  },
}

@agriffis agriffis mentioned this pull request Aug 28, 2023
Closed
@agriffis
Copy link
Contributor Author

@aqav @appelgriebsch I made another pass to move defaults into opts. Let me know if you can find the time to review!

@aqav
Copy link
Contributor

aqav commented Aug 29, 2023

Good work, I have tested the fixes from #1300 and added, they all work as expected

I really like that you allow users to override the config, like cmd, excellent move

There are little questions I meet when I test:

  1. I am not really sure if the user needs to invoke the attach_jdtls in row 224, I deleted this row and tested, and everything is still fine, so how to reproduce the "race condition and autocmd won't file' you mentioned
  2. When I enter a Java file in a project, the file type autocmd is invoked multiple times(If delete 224, it's invoked 2 times, if not, it's invoked 3 times), but only occurs in the first buffer, subsequent buffers do not have this problem
  3. When I enter a single java file(like a Hello.java) that is not in a project, I notice there is a warning message from lsp "msg_show Could not resolve java executable: Index 1 out of bounds for length 1"

The 2 and 3 also occur in my version #1300.

@agriffis
Copy link
Contributor Author

agriffis commented Aug 29, 2023 •
edited

  1. I am not really sure if the user needs to invoke the attach_jdtls in row 224, I deleted this row and tested, and everything is still fine, so how to reproduce the "race condition and autocmd won't file' you mentioned

This does not happen for me.

  1. When I enter a Java file in a project, the file type autocmd is invoked multiple times(If delete 224, it's invoked 2 times, if not, it's invoked 3 times), but only occurs in the first buffer, subsequent buffers do not have this problem

Your filetype auto-commands are running twice for some reason. This bug is why item 1 appears to work for you.

  1. When I enter a single java file(like a Hello.java) that is not in a project, I notice there is a warning message from lsp "msg_show Could not resolve java executable: Index 1 out of bounds for length 1"

This doesn't happen for me.

I should mention that I am testing this personally by installing it as .config/nvim/lua/plugins/java.lua instead of running a fork of LazyVim. I doubt that matters, but for the sake of comparing our setups.

Could you share your full neovim configuration and version?

@agriffis
Copy link
Contributor Author

If you'd like to look at mine, it's here:

https://github.com/agriffis/skel/tree/master/neovim/.config/nvim

@agriffis
Copy link
Contributor Author

Also I'm running neovim nightly, which might matter because there was a change to how nested autocmds are handled. Related issues folke/lazy.nvim#228 and neovim/neovim#23368

@aqav
Copy link
Contributor

aqav commented Aug 30, 2023 •
edited

My neovim version is v0.9.1, I test it when installing it in .config/nvim/lua/plugins/java.lua or .local/share/nvim/lazy/LazyVim/lua/lazyvim/plugins/extras/lang/java.lua, and test it in a pure new install Lazyvim, got same issue

I think it's very likely that this is due to the related issues you mentioned, but it is not your fault

Anyway, please go on with your PR progress

@agriffis
Copy link
Contributor Author

@folke I think you could merge this now. It's a pretty big improvement over the original java extra.

@folke folke merged commit 15022f4 into LazyVim:main Sep 4, 2023
3 checks passed
@folke
Copy link
Collaborator

folke commented Sep 4, 2023

Thanks!

@github-actions github-actions bot mentioned this pull request Aug 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
3 participants
@agriffis @aqav @folke