Fix LSP References Single-Result Jump Behavior #2281

jls83 · 2022-12-22T18:41:04Z

Description

When there is only a single result for a call to lsp_references, the previous cursor position is not added to the jumplist. This is inconsistent with the behavior of other Telescope built-ins.

This PR changes the implementation of the lsp.references method to use the list_or_jump helper function already in use for other LSP-related items (e.g. definitions, type_definitions, implementations). It also adds an optional results filter to list_or_jump via the results_filter attribute in opts.

Type of change

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list relevant details about your configuration

Find a symbol that has a single usage & call require('telescope.builtin').lsp_references(). Try jumping back (via <C-o>) to the previous cursor position.
Confirm symbols with multiple usages work as intended.

Configuration:

Neovim version (nvim --version): v0.8.1
Operating system and version: Debian (testing)

Checklist:

My code follows the style guidelines of this project (stylua)
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)

jls83 · 2023-01-07T22:09:32Z

@Conni2461 sorry for the ping, but do I need to directly assign someone for the CI workflow and/or a review? Thanks!

lua/telescope/builtin/__lsp.lua

Conni2461 · 2023-01-10T20:31:07Z

lua/telescope/builtin/__lsp.lua

+    end
+  end
+
+  return list_or_jump("textDocument/references", "LSP References", opts)


params.context = { includeDeclaration = vim.F.if_nil(opts.include_declaration, true) }

we can either pass in params as opts.

Not sure what this is referring to, sorry!

I think this comment is referring to the need to support the include_declaration option for lsp_references.

See the following code:

telescope.nvim/lua/telescope/builtin/__lsp.lua

Lines 17 to 19 in 5fff2a1

params.context = { includeDeclaration = vim.F.if_nil(opts.include_declaration, true) }

vim.lsp.buf_request(opts.bufnr, "textDocument/references", params, function(err, result, ctx, _)

@gbroques thanks for the clarification! I added some logic in list_or_jump to handle this case.

Conni2461 · 2023-01-10T20:33:56Z

lua/telescope/builtin/__lsp.lua

-      -- jump to location
-      local location = locations[1]
-      local bufnr = opts.bufnr
-      if location.filename then
-        bufnr = vim.uri_to_bufnr(vim.uri_from_fname(location.filename))
-      end
-      vim.api.nvim_win_set_buf(0, bufnr)
-      vim.api.nvim_win_set_cursor(0, { location.lnum, location.col - 1 })


maybe look at: #2229 (comment)

I've not check what they said but if you say it works, i'll will do some investigating/testing

Maybe something changed upstream and now the objects are the same. But we still have 0.7 support so we need to make sure it works for both versions.

I didn't run into the problem in the linked issue, but I've been making changes with NeoVim v0.8.1 so you might be right.

@Conni2461 let me know if anything needs to be updated here.

Conni2461 · 2023-01-10T20:37:30Z

I am sorry about the late review. There is no excuse i should have done it a long time ago :|

Thanks for the PR :) I've also thought that using the same function should be possible, so thanks for implementing it.

I've done a quick review. opts.results_filter kinda gives us a new feature that should be documented for the all pickers that use list_or_jump. Do you know how to do that?

To fix CI you just have to rebase your branch because we fixed the tests already on master. There should be no conflicts, so rebase should be straight forward

jls83 · 2023-01-12T01:48:51Z

No worries on the delay! End of the year is always a busy time :)

I added docs for the affected pickers, but I'm not sure if anything else needs to be done besides the CI pushing the changes to the branch once the workflow permissions are granted. Also, I noticed a subtle bug with my initial implementation -- if someone passed in a results_filter fn for the lsp_references picker, it would be discarded in favor of the "remove current line" filter. I added some extra logic to run both filters if available.

Let me know if you need anything else!

gbroques · 2023-06-24T20:24:46Z

Thank you for the work on this @Conni2461 and @jls83!

This PR looks stale.

Anything I could do to help?

nieomylnieja · 2024-01-18T10:40:55Z

Upping this one ☝️ could also help if need be :)

Conni2461 · 2024-01-18T10:50:58Z

uff thats a 2022 PR, i'll finish it this weekend. thanks for the ping

skip-checks: true

jls83 · 2024-01-18T19:36:50Z

@Conni2461 I fixed up my branch with master & made some of the requested changes. Let me know if you need anything else!

jls83 marked this pull request as ready for review December 22, 2022 18:41

Conni2461 reviewed Jan 10, 2023

View reviewed changes

lua/telescope/builtin/__lsp.lua Outdated Show resolved Hide resolved

Conni2461 reviewed Jan 10, 2023

View reviewed changes

jls83 force-pushed the fix_lsp_references_jumps branch from 6f16402 to 9dce80b Compare January 12, 2023 00:53

Conni2461 self-assigned this Jan 18, 2024

jls83 added 6 commits January 18, 2024 18:55

Refactor lsp.references to use the list_or_jump fn

76c2ddc

Use type check to confirm results_filter is fn

524aa3e

Update docstrings for affected builtins

302f0a3

Add logic to combine a passed-in result_filter for lsp refs

99e0e41

Add comments for combo filter logic

c094f1a

Add includeDeclaration in params.context

a46205f

jls83 force-pushed the fix_lsp_references_jumps branch from d86ab9e to a46205f Compare January 18, 2024 18:56

[docgen] Update doc/telescope.txt

5abf5cc

skip-checks: true

Remove extra require call for lsp_type_definitions

863b307

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LSP References Single-Result Jump Behavior #2281

Fix LSP References Single-Result Jump Behavior #2281

jls83 commented Dec 22, 2022 •

edited

Loading

jls83 commented Jan 7, 2023

Conni2461 Jan 10, 2023

jls83 Jan 12, 2023

gbroques Jun 24, 2023 •

edited

Loading

jls83 Jan 18, 2024

Conni2461 Jan 10, 2023

Conni2461 Jan 10, 2023

jls83 Jan 12, 2023

jls83 Jan 18, 2024

Conni2461 commented Jan 10, 2023 •

edited

Loading

jls83 commented Jan 12, 2023

gbroques commented Jun 24, 2023 •

edited

Loading

nieomylnieja commented Jan 18, 2024

Conni2461 commented Jan 18, 2024

jls83 commented Jan 18, 2024

	params.context = { includeDeclaration = vim.F.if_nil(opts.include_declaration, true) }

	vim.lsp.buf_request(opts.bufnr, "textDocument/references", params, function(err, result, ctx, _)

Fix LSP References Single-Result Jump Behavior #2281

Are you sure you want to change the base?

Fix LSP References Single-Result Jump Behavior #2281

Conversation

jls83 commented Dec 22, 2022 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

jls83 commented Jan 7, 2023

Conni2461 Jan 10, 2023

Choose a reason for hiding this comment

jls83 Jan 12, 2023

Choose a reason for hiding this comment

gbroques Jun 24, 2023 • edited Loading

Choose a reason for hiding this comment

jls83 Jan 18, 2024

Choose a reason for hiding this comment

Conni2461 Jan 10, 2023

Choose a reason for hiding this comment

Conni2461 Jan 10, 2023

Choose a reason for hiding this comment

jls83 Jan 12, 2023

Choose a reason for hiding this comment

jls83 Jan 18, 2024

Choose a reason for hiding this comment

Conni2461 commented Jan 10, 2023 • edited Loading

jls83 commented Jan 12, 2023

gbroques commented Jun 24, 2023 • edited Loading

nieomylnieja commented Jan 18, 2024

Conni2461 commented Jan 18, 2024

jls83 commented Jan 18, 2024

jls83 commented Dec 22, 2022 •

edited

Loading

gbroques Jun 24, 2023 •

edited

Loading

Conni2461 commented Jan 10, 2023 •

edited

Loading

gbroques commented Jun 24, 2023 •

edited

Loading