telescope.nvim icon indicating copy to clipboard operation
telescope.nvim copied to clipboard

Fix jump regression in LSP references action handler

Open adriangoransson opened this issue 1 year ago • 2 comments

Description

This change restores the possibility to exclude the current line when invoking the lsp_references picker.

  • opts.include_current_line is by default unset, so the previous equality check would fail unless the option was set explicitly.
  • vim.api.nvim_win_get_cursor() returns both line and column, so it can't be directly compared with the returned line number.
  • The actual comparison was expecting quickfix-like items, when in actuality, we were dealing with raw LSP location objects.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Triggering lsp_references on a symbol which is only referenced in one other location jumps directly to that location.
  • Triggering lsp_references on a symbol which is referenced in multiple other locations lists reference locations. References on the cursor line where the picker was triggered are excluded.
  • Triggering lsp_references with opts = { include_current_line = true } yields the same behavior as it does today.

Configuration:

  • Neovim version (nvim --version):

    NVIM v0.9.5 Build type: Release LuaJIT 2.1.1713773202

  • Operating system and version:

    macOs 14.4.1 (23E224)

Checklist:

  • [X] My code follows the style guidelines of this project (stylua)
  • [X] I have performed a self-review of my own code
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] ~I have made corresponding changes to the documentation (lua annotations)~ N/A, I think?

adriangoransson avatar May 07 '24 17:05 adriangoransson

Everything has been working for a couple of days, but I got warning messages:

locations_to_items must be called with valid offset encoding

I added offset_encoding as a parameter to action_handlers to resolve it. It's not very pretty, and currently not annotated correctly either, but the warnings are gone. Any assistance to make it nicer is kindly accepted!

adriangoransson avatar May 08 '24 14:05 adriangoransson

Thanks! For the offset_encoding part, I think it's probably better to call vim.lsp.util.locations_to_items earlier, before calling apply_action_handler, then pass the return value of it to apply_action_handler.

This is what we used to do before my (terrible :sweat_smile:) refactor. See here: https://github.com/nvim-telescope/telescope.nvim/blob/b744cf59752aaa01561afb4223006de26f3836fd/lua/telescope/builtin/__lsp.lua#L25-L67

jamestrew avatar May 12 '24 01:05 jamestrew

For the offset_encoding part, I think it's probably better to call vim.lsp.util.locations_to_items earlier, before calling apply_action_handler, then pass the return value of it to apply_action_handler.

Thanks for the help (and for your work on the plugin! 🌟). I think I managed to solve it in an okay way, by letting action handlers receive and process both the lsp locations as well as the qf items. It requires some discipline to keep the processing "synced" between the two, but it keeps the calling function (that doesn't yet know which representation it is going to use) simpler.

I added a couple of refactoring commits to the PR as well, but can drop those if you wish.

adriangoransson avatar May 12 '24 19:05 adriangoransson

thanks lgtm!

jamestrew avatar May 14 '24 02:05 jamestrew