Improve snippet-insertion.

[L3MON4D3]

Right now we handle snippet-insertion (how the snippet is jumped to/from, if history is enabled) pretty naively, by always inserting the snippet inside the currently active node. This leads to annoying issues (jumping back far in the buffer, snippets aren't jumped into when jumping from an buffer-position-adjacent one), whose symptoms could be eliminated with things like region_check_events, but it's time for a proper solution.

This PR fixes these issues by always inserting the snippet at the node which contains the position it is expanded at, and by leaving the currently active snippet if the new snippet is not inside it.

This also enables an easy fix for #859, since we now know exactly which node has to be updated when the text in a nested snippet is changed.
It should also make unlinking snippets due to deleted text more reliable, right now we look for also-affected snippets by following node.next/prev (essentially by simulating jump(1) which is kind of stupid tbh), but if we have a proper tree of expanded snippets, this can be implemented much better (I think :D)

This is by no means finished, but the searching and insertion is implemented, leaving the current snippet and the other fixes enabled by this don't work yet

[L3MON4D3]

Leaving the current node upon insertion should work now, here's a short demo (because I'm enjoying this very much :D)

1688741136.mp4

Notice how the extmark-highlights are dis/enabled when leaving the current node/entering the node the snippet is expanded in, and how the jumps behave as expected

An issue with this new behaviour is that enabling region_check_events is now very annoying, because it will jump to the very end of the buffer. Thinking of just ripping that out, there's no real reason for enabling it anymore once this is merged.

[sandersantema]

The demo video file seems to be corrupt so I can't see for certain, but I assume this PR is ready for testing? I'll give it a go if so.

[L3MON4D3]

Oh that would be awesome, thank you!
I'd say wait a bit, there are a few pretty common bugs I should be able to iron out rather quickly, I'll let you know when I think I have the most common ones squashed.
(no idea what's wrong with the video, I'll try uploading it again)

[L3MON4D3]

Alright, got the ones I know about, feel free to take it for a drive :D

[sandersantema]

I hope this bug reporting isn't too barren, I've got time to try it out but not much time to do in-depth debugging.

This is the first error I've encountered

E5108: Error executing lua ...site/pack/packer/start/LuaSnip/lua/luasnip/util/mark.lua:82: attempt to index a nil value
stack traceback:
        ...site/pack/packer/start/LuaSnip/lua/luasnip/util/mark.lua:82: in function 'pos_begin_end_raw'
        ...ite/pack/packer/start/LuaSnip/lua/luasnip/nodes/util.lua:238: in function 'binarysearch_pos'
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:457: in function 'find_snippettree_position'
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:602: in function 'trigger_expand'
        ...nvim/site/pack/packer/start/LuaSnip/lua/luasnip/init.lua:228: in function 'snip_expand'
        ...nvim/site/pack/packer/start/LuaSnip/lua/luasnip/init.lua:340: in function 'expand'
        ...hare/nvim/site/pack/packer/opt/nvim-cmp/lua/cmp/core.lua:490: in function <...hare/nvim/site/pack/packer/opt/nvim-cmp/lua/cmp/core.lua:436>
        ...site/pack/packer/opt/nvim-cmp/lua/cmp/utils/feedkeys.lua:47: in function <...site/pack/packer/opt/nvim-cmp/lua/cmp/utils/feedkeys.lua:45>

This happened while autocompleting if ... with the then ... end snippet which afaik comes from lua_ls. This is the relevant cmp mapping:

    ['<Tab>'] = cmp.mapping(function(fallback)
      if luasnip.expandable() and not cmp.get_selected_entry() then
        luasnip.expand()
        cmp.close()
      elseif cmp.visible() then
        cmp.confirm({ select = true })
      else
        fallback()
      end
    end, { 'i', 's' }),

Let me know if you need anymore information.

[L3MON4D3]

Aaah, I guess this happens if a deleted snippet exists while inserting a new one.. Should be an easy fix 🤞

[L3MON4D3]

Took a bit too long, but that should be that

[sandersantema]

I wouldn't say long but rather very quick 😄. Anyhow I haven't experienced any other bugs. I haven't used it extensively however but will do so in the coming days. If I encounter anything I'll let you know, but so far a very nice improvement!

[sandersantema]

Seems that I might not have actually tested this pr after reporting the first bug, I thought gh pr checkout 941 would do the job but I'm not completely sure if it did or packer might have changed it back to main. So I'm not sure how valid my previous comment was:

Anyhow I haven't experienced any other bugs.

I'm certain I've merged this pr locally now so I'll continue to use it and see if any errors pop up.

[L3MON4D3]

True, I should've said that it was a bit more than I anticipated :D

I think this is in really good shape now, maybe one or two small errors in the last few commits, but all the big pieces, so to speak, are in place, and should work

Thank you, I'm very happy to finally get rid of that pain-point :)
(I'm currently least confident in gracefully handling deletion of snippets (like deleting their buffer-text), so go wild with that)

[L3MON4D3]

Seems that I might not have actually tested this pr after reporting the first bug, I thought gh pr checkout 941 would do the job but I'm not completely sure if it did or packer might have changed it back to main. So I'm not sure how valid my previous comment was:

Anyhow I haven't experienced any other bugs.

I'm certain I've merged this pr locally now so I'll continue to use it and see if any errors pop up.

Oh, it must've worked, the traceback contains code only present in this PR

[sandersantema]

like deleting their buffer-text

What exactly do you mean by that? The text inserted into the buffer after expanding the snippet?

[L3MON4D3]

Jup, just that.
It causes the marks we use to remember node-positions in the buffer to shift around in weird ways, and we can only really detect this by catching an error, which requires that lots of code just catches these errors and handles them appropriately (by removing the snippet, or passing the error up, so the snippet can be removed somewhere else).
All that is pretty fickle, and I think the code handles the issues I'm aware of correctly, but I haven't yet used it enough to be really confident in it

[sandersantema]

Found one :)

The following auto-snippet:

  s(
    {
      trig = ';([^;]*);',
      wordTrig = false,
      regTrig = true,
      trigEngine = 'ecma',
    },
    fmta('_<>', {
      d(
        1,
        function(_, snip)
          return string.len(snip.captures[1]) > 1
              and sn(1, t(string.format('{%s}', snip.captures[1])))
            or sn(1, t(snip.captures[1]))
        end
      ),
    })
  ),

And relevant config:

luasnip.config.setup({
  history = true,
  update_events = 'TextChanged,TextChangedI',
})

Then s;n; expands to _nand s;foo; to _{foo} while on the main branch s;n; expands to s_n and s;foo; to s_{foo} as expected.

[L3MON4D3]

Ahhhhh it's because we don't yet have a fix for this from main on here xD
Rebasing.. 😅
Had me stumped there for a second :D

[leiserfg]

s PR is ready for

The video is not corrupted, you are probably using firefox and it does not play the videos @L3MON4D3 uploads.

[L3MON4D3]

A few words on the latest changes (because they don't seem to stop 😅):
history=true would, with this improved model, mean that the (unfortunately) popular jump/tab-combo (ls.jump if possible, tab otherwise) would break horribly (think expanding a snippet somewhere, then a snippet (textually) before that, <Tab> => one would not insert a tab, but jump back to the first snippet).

The obvious solution is to disable history, so the jump from the textually-before snippet is not possible, but that would (currently) also mean that it is not possible to jump back into previous expansions.

So, history is now split into 3 options, one for deleting previous expansions, one for linking these together (ie. allowing jumps between them), and one for linking nested expansions.
Combined with a function for jumping into the node at any position in the buffer (ls.activate_node()), even if the snippet is not linked up, I think there should be enough flexibility to get a (even more I'd say) satisfactory experience, even with the jump/tab-combo.