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.

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

fix: restore cursor to correct position on picker close #2850

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jamestrew
Copy link
Contributor

@jamestrew jamestrew commented Jan 6, 2024

I think we have several factors contributing to flaky (off by one error) behaviors in the cursor position on picker close.

Mainly, I think we should be scheduling nvim_win_set_cursor. Without doing so, we've had to "arbitrarily" +1 on the columns to compensate:

col = pos[2] + 1
elseif row == nil then
row, col = pos[1], pos[2] + 1

This is made worse by a couple of other factors

  1. ripgrep gives columns in 1-index whereas nvim_win_set_cursor expects 0-index.
    This wasn't previously an issue probably due to not scheduling nvim_win_set_cursor but I think we were just depending on UB.
  2. not accounting for original mode on select_default. like this
    I think this is no longer necessary and was used to compensate for the lack of scheduling set cursor.
    if cursor_valid and a.nvim_get_mode().mode == "i" and picker._original_mode ~= "i" then
    pcall(a.nvim_win_set_cursor, original_win_id, { original_cursor[1], original_cursor[2] + 1 })
    end

I recognize this is a very sensitive part of our code that we've had repeat issues with so I still need to do some testing and verify this closes a few issues without any obvious or previously experienced regressions

closes #2319 and semi-related #2319 (comment)
closes #2849
closes #1366

TODO (all with both norma/insert mode as original mode):

  • check 0/1 index of columns in lsp/treesitter/etc pickers
    • all except treesitter picker was using 1-index row/col (treesitter cursor position was actually broken) so taking 1-index as the true column index
  • closing pickers without selecting anything (no change)
  • select_default with no row/col info (find_files)
  • select_default with row/col/both info (live_grep with/without --invert-match for no col info)
  • actions.close() calls (like in Wrong cursor position after actions.close #2849)
  • strange cursor move when telescope closes #2319
  • anything else?

@jamestrew jamestrew force-pushed the fix/close-cursor-pos branch 5 times, most recently from 9b27ff0 to 2a369c3 Compare January 7, 2024 02:01
@jamestrew jamestrew marked this pull request as ready for review January 7, 2024 02:02
@Conni2461
Copy link
Member

let me run this for a couple of days before merging :D but thanks for addressing this issue :)

@Conni2461
Copy link
Member

i rebased the branch and continue to run it till friday, if there arent any issue till then, i think this is good to go thanks :)

@jamestrew
Copy link
Contributor Author

I found an regression with :Telescope help_tags related to opening an existing buffer at a different location.

  1. :Telescope help_tags -> search for bufhidden -> this opens the options.txt doc buffer
  2. :Telescope help_tags -> search for hidden -> same options.txt but doesn't move, still on bufhidden

@jamestrew
Copy link
Contributor Author

jamestrew commented Jan 13, 2024

I found an regression with :Telescope help_tags related to opening an existing buffer at a different location.

1. `:Telescope help_tags` -> search for `bufhidden` -> this opens the `options.txt` doc buffer

2. `:Telescope help_tags` -> search for `hidden` -> same `options.txt` but doesn't move, still on `bufhidden`

The problem is that we're scheduling cursor position inside actions.close() so when a picker calls actions.close() then does something that moves the cursor under the current window, the previously scheduled cursor position moves the cursor again.

That why I had to schedule this so it happens after the cursor position is reset
https://github.com/nvim-telescope/telescope.nvim/pull/2850/files#diff-f192865bab9995c5a69e48b85b5d4534094c885a1556b10bb1e9407b0e0570d0R387-R390

For this help_tags issue, I can fix it by doing the same by scheduling this

if cmd == "default" or cmd == "horizontal" then
vim.cmd("help " .. selection.value)
elseif cmd == "vertical" then
vim.cmd("vert help " .. selection.value)
elseif cmd == "tab" then
vim.cmd("tab help " .. selection.value)
end

Ideally, we just defer/schedule everything after actions.close() to happen after cursor position is set. I gotta think about how to do this without significant refactors...
I don't like the idea of throwing vim.schedule case by case on things after actions.close(). This really puts the responsibility on each picker actions way too much.

@jamestrew
Copy link
Contributor Author

Yeah idno how to cleanly fix this without API changes and big refactors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
0.2
  
Awaiting triage
2 participants