nvim-telescope / telescope.nvim

Find, Filter, Preview, Pick. All lua, all the time.
MIT License
15.95k stars 838 forks source link

lsp_definitions() jump_type ignored if the target buffer is opened in a window #2690

Open konart opened 1 year ago

konart commented 1 year ago

Description

Not sure if bug or "just as planned" but from the description:

        {jump_type}     (string)   how to goto definition if there is only one
                                   and the definition file is different from
                                   the current file, values: "tab", "split",
                                   "vsplit", "never"
...
        {reuse_win}     (boolean)  jump to existing window if buffer is
                                   already opened (default: false)

Given that the user have a method under the cursor and method's definition in the same buffer

I'd expect that setting jump_type to vsplit and (optionally?) setting reuse_win to false (default) will create a new window with the same buffer.

What actually happens: cursos simply jumps to definition withou leaving the existing window.

Neovim version

NVIM v0.9.1
Build type: Release
LuaJIT 2.1.0-beta3

Operating system and version

macOS 13.5

Telescope version / branch / rev

master

checkhealth telescope

telescope: require("telescope.health").check()

Checking for required plugins ~
- OK plenary installed.
- OK nvim-treesitter installed.

Checking external dependencies ~
- OK rg: found ripgrep 13.0.0
- OK fd: found fd 8.7.0

===== Installed extensions ===== ~

Telescope Extension: `yank_history` ~
- No healthcheck provided

Telescope Extension: `zf-native` ~
- No healthcheck provided

Steps to reproduce

  1. Move cursor to a method X such that it has its definition in the same file\buffer
  2. Call require("telescope.builtin").lsp_definitions({jump_type="vsplit"})

Expected behavior

New vertical split created with the definition of X under the cursor

Actual behavior

Cursor jumps to a definition in active window.

cristiansofronie commented 1 year ago

Well there is thing thing in the source code:

      if uri ~= flattened_results[1].uri and uri ~= flattened_results[1].targetUri then
        if opts.jump_type == "tab" then
          vim.cmd "tabedit"
        elseif opts.jump_type == "split" then
          vim.cmd "new"
        elseif opts.jump_type == "vsplit" then
          vim.cmd "vnew"
        end
      end

      vim.lsp.util.jump_to_location(flattened_results[1], offset_encoding, opts.reuse_win)

Basically it only creates new splits or tabs if the uri (which is a string of the form file://buffer_filename) differs from the current tab uri. uri and targetUri are the same things, targetUri is the prefered richer version but both refer to the file where the result is as per the protocol.

So this seems intentional. It only jumps within the current window and doesn't create one. If you would want to do that maybe a separate option would be needed.

freddiehaddad commented 1 year ago

I see what you mean. I think the explanations in the file are misleading then:

--- Goto the definition of the type of the word under the cursor, if there's only one,
--- otherwise show all options in Telescope
---@param opts table: options to pass to the picker
---@field jump_type string: how to goto definition if there is only one and the definition file is different from the current file, values: "tab", "tab drop", "split", "vsplit", "never"
---@field fname_width number: defines the width of the filename section (default: 30)
---@field show_line boolean: show results text (default: true)
---@field trim_text boolean: trim results text (default: false)
---@field reuse_win boolean: jump to existing window if buffer is already opened (default: false)
---@field file_encoding string: file encoding for the previewer
builtin.lsp_type_definitions = require_on_exported_call("telescope.builtin.__lsp").type_definitions

One (including myself) would read this as:

map('n', 'gd', function() require('telescope.builtin').lsp_definitions({ jump_type = 'vsplit', reuse_win = true }) end, { desc = 'Goto definition' })
jump_type = 'vsplit' -- A window will be split vertically if the definition is in a different file. 
reuse_win = true -- If the file is already open in a split window, then reuse that window, not split again.

Also, the new split is an empty buffer. So it seems like a bug where it creates the split but doesn't load the file into the buffer because it's already open.

You can reproduce by setting the above keymap, opening a file that references a definition in a different file, moving the cursor to that reference, activating the keymap, returning to the same spot and activating the keymap again.

ahouseago commented 9 months ago

I found this issue after running a PackerSync and wondering why jumping to definitions was broken.

The default behaviour was changed in #2324. Unfortunately the author's assumption that everyone would want to reuse the window wasn't correct, at least for me! I already have a binding for jumping to the definition in the same window: don't use the jump_type and it'll do that.

I'll see if I can create a PR to change that behaviour to a configurable option but haven't contributed to telescope before so it might take me a while 🙂

gabzim commented 2 months ago

any progress on this? I agree with @ahouseago that the assumption that we'd want to reuse the window is incorrect. I often open in a vsplit the definition even if the function is in the same file I'm looking at but in a different part of the file.

jamestrew commented 1 month ago

Maybe we can add a reuse_buf (default: true - the current behavior) option that augments the italicized part of "how to goto definition if there is only one and the definition file is different from the current file"