Shougo / neosnippet.vim

neo-snippet plugin
Other
1.12k stars 108 forks source link

`g:neosnippet#enable_completed_snippet` doesn't remove the inserted text #398

Closed roxma closed 6 years ago

roxma commented 7 years ago

As per https://github.com/autozimu/LanguageClient-neovim/pull/121#issuecomment-326219688

The g:neosnippet#enable_completed_snippet of neosnippet dosen't remove the inserted text, which doesn't work for non-parameter-expansion use case. Is this intensional?

neosnippet

The completed snippet support is useful for language server support. But this behavior makes it wierd for non-parameter-expansion use case.

roxma commented 7 years ago

Maybe I'm misusing neosnippet, here's the v:completed_item:

{'word': 'template', 'menu': '[+] ', 'snippet_word': 'template', 'snippet': '<template>
  ${0}
</template>
', 'info': 'Scaffold <template> with html
snippet@0', 'kind': '', 'abbr': 'template with html'}
Shougo commented 7 years ago

Ah, I get it. It should be fixed.

But, why v:completed_item.snippet is set? I don't think it can be set by Vim script.

roxma commented 7 years ago

But, why v:completed_item.snippet is set?

This is injected by NCM with a timer, checking the v:completed_item.

Let me explain current NCM behavior:

In parameter expansion for foo(param1, param2), with neosnippet, NCM will change the v:completed_item as this:

{'word': 'foo', 'snippet': '(param1, param2)'}

foo is removed by NCM in v:completed_item.snippet so that the user is free to choose to expand the snippet, or simply accept the word.

With other ultisnips/snipmate,

{'word': 'foo', 'snippet': 'foo(param1, param2)'}

Since ultisnips/snipmate doesn't recognize v:completed_item.snippet, NCM will inject the snippet by calling its API.

This behavior used to work just fine, until the first non-parameter-expansion use case for completed item I meet.

roxma commented 7 years ago

I don't think it can be set by Vim script.

It can be set, since it's already working.

https://github.com/Shougo/neosnippet.vim/blob/master/autoload/neosnippet/mappings.vim#L27

  let snippet = neosnippet#parser#_get_completed_snippet(
        \ v:completed_item, neosnippet#util#get_cur_text(),
        \ neosnippet#util#get_next_text())

https://github.com/Shougo/neosnippet.vim/blob/master/autoload/neosnippet/parser.vim#L288

  if has_key(item, 'snippet')
    return item.snippet
  endif

This is where I find that neosnippet recognize v:completed_item.snippet

Shougo commented 7 years ago

OK. I get it.

roxma commented 7 years ago

I think there may be other case where current behavior will faill.

When multiple sources is working together, the completion engine may adjust the item['word'] and item['startcol'] to merge items from different sources.

When the item['word'] is different from the original word given by the completion source, neosnippet may not be able to know which one is the original word it needs to remove.

It seems I should change NCM to use neosnippet API instead. NCM internally keeps track of the original word for injecting the snippet into the engine.

Shougo commented 7 years ago

It seems I should change NCM to use neosnippet API instead. NCM internally keeps track of the original word for injecting the snippet into the engine.

Hm. It may be better. You can call call neosnippet#view#_insert() directly. If you want to remove the trigger, neosnippet#view#_expand() is better.

roxma commented 7 years ago

Will work on that. Closing this issue.

Thanks for your help.

Shougo commented 7 years ago

OK.

roxma commented 7 years ago

I've read some of the source code, and it seems call neosnippet#view#_insert() is not what I'm looking for.

My point is: NCM itself doesn't expand the snippet or trigger the expansion. It simply tells neosnippet there's a new snippet. If the user wants to expand the snippet, then press the expand key, otherwise he/she could simply accept the completed word.

Here's my current approach, which is quite hacky.

In short, NCM injects the snippet into neosnippet#variables#current_neosnippet(). And it remembers to cleanup the injected snippet afterwards.

While this approach has some disavantages.

Here's my proposal:

Things would be a lot easier if a common convension is set up so that snippet engines recognize the v:completed_item.snippet and v:completed_item.snippet_word.

The snippet_word is useful, for example, in ncm, the popup menu looks like this when there's a file named datetime.noww. A more complicated situation would be non-parameter-expansion snippets.

from datetime import datetime

now = datetime.now
     |         now  [+] now(cls, tz=None) |
     |datetime.noww [ ] ~buf              |

The completion items are

[{
    'word': 'datetime.now',
    'snippet':'now($1)$0',
    'snippet_word': 'now',
    'abbr': '         now'
},  {
    'word': 'datetime.noww'
}]

Notice that the now method is left-padded with datetime. to be merged with the items from filepath completion. Without snippet_word, the engine may not be able to know it should replace now instead of datetime.now.

This is also related to https://github.com/neovim/neovim/issues/7179, since vim doesn't forward custom field name like snippet and snippet_word. Currently ncm also uses some dirty hack to get this to work.

Shougo commented 7 years ago

This is also related to neovim/neovim#7179, since vim doesn't forward custom field name like snippet and snippet_word. Currently ncm also uses some dirty hack to get this to work.

I know it. But this is neovim/Vim feature. To implement it, the rewrite completion system is needed.

roxma commented 7 years ago

Yeah, but would you implement the other part, to recognize the v:completed_item.snippet and v:completed_item.snippet_word?

Shougo commented 7 years ago

I've read some of the source code, and it seems call neosnippet#view#_insert() is not what I'm looking for.

I have said neosnippet#view#_expand().

roxma commented 7 years ago

I have said neosnippet#view#_expand()

Sorry, my bad

But it seems _expand doesn't help either.

My point is: NCM itself doesn't expand the snippet or trigger the expansion. It simply tells neosnippet there's a new snippet. If the user wants to expand the snippet, then press the expand key, otherwise he/she could simply accept the completed word.

If I call _expand directly, NCM will trigger the expansion, and it forces the user to accept the snippet automatically.

In the following case, if I press ce and change method name foo into foo_ex, I wouldn't want to accept the snippet automatically, 'cause the parameters are already there.

obj.|foo(param1, param2)
Shougo commented 7 years ago

I don't know why snippet_word fixes the problem. And I think snippet_word should be snippet_trigger.

roxma commented 7 years ago

And I think snippet_word should be snippet_trigger.

I'll accept the name.

I don't know why snippet_word fixes the problem.

It doesn't fix snippet-name collision and snippet-cleanup issue. It's a refinement over the original v:completed_item.snippet version.

It's useful, because the snippet_trigger may be different from the v:completed_item.word, due to the merging process of completion engine, startcol is adjusted, and word is padded.

from datetime import datetime

now = datetime.now
     |         now  [+] now(cls, tz=None) |
     |datetime.noww [ ] ~buf              |

In this case, notice the difference between word and snippet_trigger:

startcol: 7
[{
    'word': 'datetime.now',
    'snippet':'now($1)$0',
    'snippet_trigger': 'now',
    'abbr': '         now'
},  {
    'word': 'datetime.noww'
}]

The difference is due to the merging process. This is how completion items look like before the merge. They have different startcol.

startcol: 16
[{
    'word': 'now',
    'snippet':'now($1)$0',
}]

startcol: 7
[{
    'word': 'datetime.noww'
}]
Shougo commented 7 years ago

OK. I will add the support.

Shougo commented 7 years ago

I have not much development time. Please wait.

HerringtonDarkholme commented 7 years ago

Hi @roxma . I do agree with @Shougo 's opinion that neosnippet#view#_expand() is probably enough.

Automatic snippet expansion is the default behavior for other editors, so this is quite natural for Vim to adopt the same behavior.

On the other hand, snippet expansion is totally new for Vim users. It's hard to say how users will accept it.

NCM itself doesn't expand the snippet or trigger the expansion.

I would argue that NCM expanding snippet feels more natural. And if users doesn't like expansion, they can simply not confirm the completion(e.g. press enter key). Completion still can be done by , .

Finally, the new feature is still in its alpha stage. The current proposal requires nontrivial changes for three party: Vim itself, snippet library and completion manager. If automatic expansion is adopted, at least we can skip changing snippet library. Also, snippet registering and cleanup can be waived, leading to a simpler implementation. Simpler implementation gives us more freedom to change it later, after we release this new feature and await users' feedback.

Shougo commented 7 years ago

I think the auto completion plugin should not do this. My proposal is: https://github.com/neovim/neovim/issues/7179#issuecomment-326861381

It is snippet plugin feature.

roxma commented 7 years ago

@HerringtonDarkholme

I would argue that NCM expanding snippet feels more natural.

It will require keymappings for expansion either way, and I prefer to offer minimal default key mapping. As a user with a snippet plugin installed, (s)he should already be familiar with the snippet plugin. It would still be nice to use the old key configured by himself for expansion.

As for the completion plugin, I'll put the key mappings for triggering expansion on <Enter> into documentation. The user is always free to choose.

The current proposal requires nontrivial changes for three party: Vim itself, snippet library and completion manager.

Actually NCM has already skipped vim and snippet library for completion-snippet feature, with some hack. But it feels more clean to co-operate if the three parts has common, well-defined convension. This is why I send those feature requests. They're not required to be finished, if not, NCM will stick with current implementation.

Shougo commented 6 years ago

I will add the support in neosnippet.

Shougo commented 6 years ago

neosnippet will use v:completed_item.user_data instead.

Shougo commented 6 years ago

I have added user_data support.

Shougo commented 6 years ago

neovim and Vim8 supports user_data feature. nvim-completion-manager should use user_data instead.