L3MON4D3 / LuaSnip

Snippet Engine for Neovim written in Lua.
Apache License 2.0
3.34k stars 237 forks source link

Cursor is not immediately at correct place after expanding snippet #1010

Open echasnovski opened 12 months ago

echasnovski commented 12 months ago

Hi!

This is a result of my investigation into echasnovski/mini.nvim#484. In short my original problem: expanding snippet in a way which triggers scroll animation leads to some text change and exiting Select mode. I found it to be a two-part problem.


Right after snippet expansion cursor is not at the place it needs to be. I verified this by adding these lines here:

    _G.info = {
        cur_line = vim.fn.getline("."),
        cur_pos = vim.api.nvim_win_get_cursor(0),
    }

The core reason seems to be that any_select() utility function does not feeds keys immediately, as it uses default n flag. I tried using several variations with x flag, which did place cursor immediately but resulted into some weird Select mode behavior (for example, not exiting it right away after first <Esc> but only after second <Esc>).

My investigation stopped here.


Animating scroll when Select mode is active seems to reset it. I have addressed this in 'mini.animate'.

L3MON4D3 commented 12 months ago

Hey! Yeah, that's a bit of a rough area, since due to missing api feedkeys has to be used to set the visual selection. I had tried to feedkeys to execute immediately in the past, but without success so far, so not that confident in getting that to work :sweat_smile:

I skimmed over the issue you linked, but couldn't really tell what sequence of events is causing the issues. Could you go into that a bit?

echasnovski commented 12 months ago

Hey! Yeah, that's a bit of a rough area, since due to missing api feedkeys has to be used to set the visual selection. I had tried to feedkeys to execute immediately in the past, but without success so far, so not that confident in getting that to work 😅

I feel you. This is indeed tricky.

I skimmed over the issue you linked, but couldn't really tell what sequence of events is causing the issues. Could you go into that a bit?

Sure. Here is the basic rundown of what seems to happen:

L3MON4D3 commented 12 months ago

Ah okay, that helped, ty!

Are you sure that expanding alone (not counting the subsequent jump into the snippet) can trigger WinScrolled? Only nvim_buf_set_text is used while expanding (at least I'm pretty sure that's all we do in terms of cursor/buffer-manipulations)

Maybe the successive cursor_set_keys in any_select are at fault? The first one causes one WinScrolled, and the second one causes another while animate is still animating the first one?

And if they are at fault, I don't think (or can't tell how, at least :D ) running feedkeys immediately would solve the issue

echasnovski commented 12 months ago

Are you sure that expanding alone (not counting the subsequent jump into the snippet) can trigger WinScrolled? Only nvim_buf_set_text is used while expanding (at least I'm pretty sure that's all we do in terms of cursor/buffer-manipulations)

Yes, pretty sure. A WinScrolled event is basically triggered when top window line has changed. There are situations when setting text does that.

Here is an init file to reproduce:

vim.o.scrolloff = 10

vim.api.nvim_buf_set_lines(0, 0, -1, false, vim.split(string.rep('a', 1000), ''))
vim.api.nvim_win_set_cursor(0, { vim.api.nvim_buf_line_count(0), 0 })
vim.cmd('normal! zb')

local add_text_after_cursor = function()
  local cur_pos = vim.api.nvim_win_get_cursor(0)

  local text = { 'This', 'is', 'big', 'snippet' }
  vim.api.nvim_buf_set_text(0, cur_pos[1] - 1, cur_pos[2] + 1, cur_pos[1] - 1, cur_pos[2] + 1, text)
end

vim.keymap.set('n', '<M-m>', add_text_after_cursor)

_G.n_winscrolled = 0
vim.cmd('au WinScrolled * lua _G.n_winscrolled = _G.n_winscrolled + 1')

Steps:

L3MON4D3 commented 12 months ago

Ahh okay, yup. And running all luasnip-cursor-manipulations at once would fix this because mini.animate uses defer (or something similar) to enque its scroll?

echasnovski commented 12 months ago

Ahh okay, yup. And running all luasnip-cursor-manipulations at once would fix this because mini.animate uses defer (or something similar) to enque its scroll?

Right now it would "fix" it because it would immediately enter Select mode, i.e. before 'mini.animate' decides to start scroll animation. If cursor is dealt with separately from starting Select mode, I'll see what I kind do further.

L3MON4D3 commented 12 months ago

Okay I've been trying for some time now, but I can't get past the SELECT-but-not-SELECT-mode, where everything seems like SELECT, until you start typing and the text just.. moves :/

What if luasnip sets some kind of flag to indicate that there is currently an expansion going on, and animate can act accordingly?

echasnovski commented 12 months ago

Okay I've been trying for some time now, but I can't get past the SELECT-but-not-SELECT-mode, where everything seems like SELECT, until you start typing and the text just.. moves :/

Yeah, it does seem very strange indeed. I am borderline convinced that this is some kind of issue in Neovim (coming from Vim, probably) itself. Maybe some minimal reproducible example will be enough for an issue there?

What if luasnip sets some kind of flag to indicate that there is currently an expansion going on, and animate can act accordingly?

I'd have to think about it, but generally it needs to be a very strong case for adding special conditions for some external plugins (outside of 'mini.nvim'). The use case at hand doesn't look like it.

L3MON4D3 commented 12 months ago

TBH, something general like animate.disable()/animate.reenable(), which just sets some flag that prevents the animation-black-magic from initializing in WinScrolled sounds reasonable, I'd be open to adding the corresponding calls in our selection-routine. That said, the issue doesn't seem too bad, one jump forward/back should bring everything back into order.

Maybe some minimal reproducible example will be enough for an issue there?

Oh yeah, that would be great to have. Unfortunately, recording the keys and feeding them without the rest of luasnip in the background is working fine, so there's something more subtle going on. I'm not all that motivated to get into that, if someone else wants to, be my guest :D

echasnovski commented 12 months ago

TBH, something general like animate.disable()/animate.reenable(), which just sets some flag that prevents the animation-black-magic from initializing in WinScrolled sounds reasonable, I'd be open to adding the corresponding calls in our selection-routine.

That is already possible using buffer-local vim.b.minianimate_disable variable.

I mean, adding these lines here works for this particular use case, but I wouldn't call this a robust solution:

        local buf_id = vim.api.nvim_get_current_buf()
        local minianimate_disable_cache = vim.b[buf_id].minianimate_disable
        vim.b[buf_id].minianimate_disable = true
        vim.schedule(function()
            vim.b[buf_id].minianimate_disable = minianimate_disable_cache
        end)

Oh yeah, that would be great to have. Unfortunately, recording the keys and feeding them without the rest of luasnip in the background is working fine, so there's something more subtle going on.

That is peculiar. My initial guess would be that it is related to some other feedkeys() usage.

L3MON4D3 commented 12 months ago

That is already possible using buffer-local vim.b.minianimate_disable variable.

Ah, nice! True, not robust, but maybe something to stick in ones personal config as a bandaid:

local ls = require("luasnip")
local orig_se = ls.snip_expand
ls.snip_expand = function(...)
    local buf_id = vim.api.nvim_get_current_buf()
    local minianimate_disable_cache = vim.b[buf_id].minianimate_disable
    vim.b[buf_id].minianimate_disable = true
    vim.schedule(function()
        vim.b[buf_id].minianimate_disable = minianimate_disable_cache
    end)

    orig_se(...)
end

Might work, could you try it?

echasnovski commented 12 months ago

Might work, could you try it?

I tried, and it doesn't universally work. I use expand_or_jump in my workflow which uses local snip_expand() function, so overriding exported one in the table won't have effect.

I mean, adding this snippet directly to source as I've described works. It is not working if using bandaid approach.

L3MON4D3 commented 12 months ago

Ah yeah, there's that. Overriding the package-snip_expand is necessary for cmp_luasnip, and then one wrapper for each function that uses the local snip_expand, so expand_or_jump, expand, lsp_expand.. Not that great, but should work until the real issue is fixed. I'd obviously prefer to not incorporate those bandaids into luasnip itself, so if there's a more-or-less comfortable way to apply them otherwise, I'm all for it :D