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

[Snippets] Fix entering insert mode and handling of word under cursor #1196

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

Conversation

liskin
Copy link
Contributor

@liskin liskin commented Dec 4, 2020

Two fixes for #796

[Snippets] Fix UltiSnips not being able to enter insert mode

UltiSnips snippets have so called tabstops or placeholders that are to be filled in by the user, so after expanding a snippet the cursor is moved to the first such tabstop and vim enters insert mode (or, if there's a default value, select mode).

But this doesn't work with fzf's :Snippets, because UltiSnips uses :startinsert which doesn't work from :normal. Fix this by calling UltiSnips#ExpandSnippet directly (unfortunately needs a virtualedit hack to position the cursor after the snippet name).


[Snippets] Fix when there is word under cursor

UltiSnips#SnippetsInCurrentScope uses the word under cursor, if any, to filter candidate snippets. To make it usable, however, fzf's :Snippets must replace the word instead of appending to it, otherwise the subsequent expansion fails.

Also, use 'i' instead of 'a', to make mappings such as

inoremap <silent> <C-_> <C-\><C-O>:Snippets<CR>

feel natural. This will break workflow for people who used to manually exit insert mode in the middle of a line and then invoke :Snippets, but I'd hazard a guess there aren't many of them as this workflow was already somewhat broken by the first bug.


Fixes: #796

UltiSnips snippets have so called tabstops or placeholders that are to
be filled in by the user, so after expanding a snippet the cursor is
moved to the first such tabstop and vim enters insert mode (or, if
there's a default value, select mode).

But this doesn't work with fzf's :Snippets, because UltiSnips uses
:startinsert which doesn't work from :normal. Fix this by calling
UltiSnips#ExpandSnippet directly (unfortunately needs a virtualedit hack
to position the cursor after the snippet name).
@junegunn
Copy link
Owner

junegunn commented Dec 4, 2020

Thanks for the patch, but I haven't used UltiSnips for a long time.

@akuseru1 Oops, sorry, wrong mention.

@liskin
Copy link
Contributor Author

liskin commented Dec 4, 2020

I intend to discuss this with the UltiSnips folks as it seems I'm still not seeing the whole picture.
I'd welcome your opinion on how safe it is to use startinsert/feedkeys as I have very little experience with those two. Either way, please refrain from merging this just yet, it's likely I'll be pushing improvements. :-)

liskin referenced this pull request in SirVer/ultisnips Dec 4, 2020
UltiSnips#SnippetsInCurrentScope uses the word under cursor, if any, to
filter candidate snippets. To make it usable, however, fzf's :Snippets
must replace the word instead of appending to it, otherwise the
subsequent expansion fails.

Also, use 'i' instead of 'a', to make mappings such as

    inoremap <silent> <C-_> <C-\><C-O>:Snippets<CR>

feel natural. This will break workflow for people who used to manually
exit insert mode in the middle of a line and then invoke :Snippets, but
I'd hazard a guess there aren't many of them as this workflow was
already somewhat broken by the first bug.
Turns out calling UltiSnips#ExpandSnippet makes it think vim is in
normal mode, so it doesn't feedkeys("\<Esc>"), and that breaks entering
the select mode as the keys are typed instead.

Maybe it's time to start considering that UltiSnips may be to blame and
see if this can be improved there. Here's my first inquiry:
SirVer/ultisnips@1ca82f7#r44803007
Well now that we need to use startinsert anyway...
UltiSnips adds a CursorMovedI autocmd that is meant to sync its state
with vim, and this is necessary for some of the features like
placeholders and mirrors and so on. We insert some text in
s:inject_snippet but this doesn't trigger the autocmd, and UltiSnips may
get confused afterwards. To fix this, we need to invoke
UltiSnips#CursorMoved after inserting the text, before invoking
UltiSnips#ExpandSnippet.

This glitch can be reproduced as follows:

1. :Snippets, select APACHE
2. C-J to select the author name placeholder, type "xxx"
3. Esc to normal, G to end, :Snippets, select APACHE
4. C-J to select the author name placeholder, type "xxx"
5. glitch.
Using startinsert and feedkeys feels wrong and brittle. So now that vim
8.2.1978+ has <Cmd> that can be used instead of <C-\><C-O> in insert
mode bindings, we can go back to calling the functions explicitly.

(This should probably be documented somewhere, but it's unlikely to
affect many users, as mapping :Snippets in insert mode never worked well
anyway and few people complained.)
@liskin
Copy link
Contributor Author

liskin commented Feb 5, 2021

In the meantime I discovered a few things:

  1. UltiSnips devs no longer remember the details of why they sometimes startinsert and other times feedkeys("i"), they referred me to try to refactor it and then try the test suite, which I added to my todo list and never got back to :-/
  2. vim 8.2.1978 added <Cmd> that can be used instead of <C-\><C-O> in insert mode bindings without the drawbacks of mode() giving misleading answers, so I can leave the UltiSnips code as it is and not worry about it ever again
  3. there was an occasional glitch after snippet expansions caused by not invoking the CursorMovedI autocmd hook after inserting the selected snippet name in fzf, which I fixed now

I am not a heavy user of UltiSnips these days (not writing much new code in languages that would benefit from it) so there may still be some issues. I'll ping the folks in #796 to test this.

@aldantas
Copy link

aldantas commented Feb 5, 2021

Hi, thank you for the PR. I haven't been using fzf for searching snippets much lately, but I tested it and it fixes the problem of expansion (#796). Also, the placeholders are working as they should now. I didn't understand what the problem with <C-\><C-O> was. For me, it worked with both <C-\><C-O> and <Cmd> (and also witrh <Esc>).

However, I noticed that it was inserting the last character from the pre-expansion word after the last placeholder. For example, in python, when typing the word if, triggering the search and selecting the snippet ife, it expands to:

if condition:
    pass
else:
    passf

And the f at the end still remains even after finishing editing the placeholders.

I was able to fix that by changing your code at line:
let del = empty(matchstr(getline('.'), '\%' . (col('.') - 1) . 'c\S')) ? "" : "\<c-w>"
to:
let del = empty(matchstr(getline('.'), '\%' . (col('.')) . 'c\S')) ? "" : "\<c-w>"

And at line:
execute 'normal! i'.del.s:strip(snip)
to:
execute 'normal! a'.del.s:strip(snip)

But I don't know if that breaks something else.

@liskin
Copy link
Contributor Author

liskin commented Feb 5, 2021

I didn't understand what the problem with <C-\><C-O> was. For me, it worked with both <C-\><C-O> and <Cmd> (and also witrh <Esc>).

Snippets with placeholders with some default text should not work in this PR with <C-\><C-O> (some garbage text appears instead). In the original code, such snippets probably did work, but instead snippets without default placeholder which are supposed to start insert mode didn't start the insert mode properly.

However, I noticed that it was inserting the last character from the pre-expansion word after the last placeholder. For example, in python, when typing the word if, triggering the search and selecting the snippet ife, it expands to:
[…]

This is weird. That's exactly what the <C-\> in <C-\><C-O> is meant to prevent, and <Cmd> shouldn't be moving the cursor either. So this should only happen if you <Esc> to normal mode and then :Snippets. Can you confirm that's what you're seeing or is this happening with <C-\><C-O>/<Cmd> as well?

I'm not really sure what to do about :Snippets in normal mode though. Should we assume the user wants the cursor one column to the right and move it even before UltiSnips#SnippetsInCurrentScope is consulted? Or maybe move the cursor to the end of the word?

I was able to fix that by changing your code at line:
let del = empty(matchstr(getline('.'), '\%' . (col('.') - 1) . 'c\S')) ? "" : "\<c-w>"
to:
let del = empty(matchstr(getline('.'), '\%' . (col('.')) . 'c\S')) ? "" : "\<c-w>"

And at line:
execute 'normal! i'.del.s:strip(snip)
to:
execute 'normal! a'.del.s:strip(snip)

But I don't know if that breaks something else.

This definitely breaks insert mode mappings for me.

liskin added a commit to liskin/fzf that referenced this pull request Feb 3, 2022
Commit 85ae745 forced the use of terminal buffers whenever they're
available to improve consistency and avoid confusion. This, however, has
drawbacks:

* Using a terminal buffer is slightly slower (not noticeable to most
  people but visible to me).

* It breaks certain features of fzf.vim, requiring workarounds like
  junegunn/fzf.vim#1363. And, unfortunately, it
  also breaks junegunn/fzf.vim#1196 (a fix for
  :Snippets in insert-mode which relies on `<Cmd>` and the cursor
  staying behind the end-of-line) which I have no workaround for yet.

This commit adds `g:fzf_prefer_height` to optionally restore the old
behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snippet expansion and word under cursor
3 participants