-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[READY] Rewrite completion system #2657
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2657 +/- ##
=========================================
+ Coverage 88.83% 88.9% +0.07%
=========================================
Files 20 20
Lines 1943 1938 -5
=========================================
- Hits 1726 1723 -3
+ Misses 217 215 -2 |
Thanks for working on this! Reworking the completion system is a daunting task indeed. I've given this a spin on gvim and console vim on Ubuntu 16.04 and can confirm that the bugs are fixed. Flicker remains unchanged though to my eyes; both Given that it fixes all the other issues, I'm more than happy to see this merged (from a functionality perspective). Haven't look at the code yet though... :) |
Review status: 0 of 9 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. autoload/youcompleteme.vim, line 240 at r1 (raw file):
I'd say: yes to the first, no to the second. The first answer might be debatable, but the second one I'm more confident in. autoload/youcompleteme.vim, line 676 at r1 (raw file):
Why does this return an empty string? autoload/youcompleteme.vim, line 690 at r1 (raw file):
Yes, please. :) Comments from Reviewable |
Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. a discussion (no related file): i can also confirm that it fixes ycm-core/ycmd#671 when used with ycm-core/ycmd#681 it's triggering completion after space i hit ' then then here's the old behaviour in practice, it just looks like a flicker because you would just type ) and get on with your life, but thought i'd mention it here is the
a discussion (no related file): When a completion request times out, you get tracebacks in Vim, which we want to avoid:
Comments from Reviewable |
I've given this a try. Just like @Valloric I can confirm all the mentioned bugs are fixed, except the one that has to do with strts column. Unlike @Valloric I think I can see an improvement in flickering. I've tested on a "small" C++ file that looks like this: #include <boost/bimap.hpp>
#include <boost/python.hpp>
struct a_struct {
int a;
int b;
int c;
float x;
float y;
float z;
char very_long_char;
};
int main(void) {
a_struct very_long_struct_name;
very_long_struct_name.
return 0;
} The included headers are there just to make compiling take a while. Here's my def FlagsForFile(filename):
return { 'flags' : ['-xc++',
'-std=c++1z',
'-isystem',
'/usr/include/c++/6.3.1',
'-isystem',
'/usr/include/c++/6.3.1/x86_64-pc-linux-gnu',
'-isystem',
'/usr/include/c++/6.3.1/backward',
'-isystem',
'/usr/lib/clang/3.9.1/include',
'-isystem',
'/usr/include',
'-Wall',
'-Wextra',
'-pedantic',
'-Wno-main-return-type',
]} Uhm, please don't ask me about the
I believe it would be a good idea to leave it in. As a YCM user for a few years, my instinct tells me UI is blocked. So a discreete reminder would be nice. Just a spinning line may not be enough in my opinion. How about something like Reviewed 9 of 9 files at r1. autoload/youcompleteme.vim, line 240 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
My first thought was, since That said, Neovim does behave differently than Vim and does treat those two the same, so I'm not sure what's the smart thing to do here. For further reading on autoload/youcompleteme.vim, line 616 at r1 (raw file):
I think we should keep this. I believe that keeping the previous menu until the server responds with a new set of semantic completions would be more confusing to the end users as opposed to how much one flicker would annoy them. autoload/youcompleteme.vim, line 690 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Heh, because here we don't want to throw away the completed text, rather just delete a character.
P.S. Vimscrip is a beautiful language! Comments from Reviewable |
55de038
to
4074333
Compare
Thanks for testing and reviewing. I've pushed some changes.
Some users will probably want to disable it or customize it so I think it would be better to define an API for that. In fact, it's already possible to implement your own spin animation by accessing the let s:spin_symbols = [ "-", "\\", "|", "/" ]
let s:spin_index = 0
let s:spinner = s:spin_symbols[ s:spin_index ]
function! StartSpinning()
let s:loading_index = 0
let s:timer_id = timer_start( 100, "Spin" )
endfunction
function! Spin( ... )
if !py3eval( "ycm_state.CompletionRequestReady()" )
echo s:spinner
let s:spin_index = s:spin_index + 1
if s:spin_index >= len( s:spin_symbols )
let s:spin_index = 0
endif
let s:spinner = get( s:spin_symbols, s:spin_index )
let s:timer_id = timer_start( 100, "Spin" )
else
echo ''
endif
endfunction
autocmd TextChangedI * call StartSpinning() Reviewed 8 of 9 files at r1, 1 of 2 files at r2, 3 of 3 files at r3. a discussion (no related file): Previously, puremourning (Ben Jackson) wrote…
It seems to be fixed by leaving semantic mode and resetting completions before the first a discussion (no related file): Previously, puremourning (Ben Jackson) wrote…
Strange, the autoload/youcompleteme.vim, line 240 at r1 (raw file): Previously, bstaletic wrote…
I added autoload/youcompleteme.vim, line 616 at r1 (raw file): Previously, bstaletic wrote…
I agree but more importantly, resetting completions here is needed if we don't want to display the previous ones after entering a new line. I removed the comment. autoload/youcompleteme.vim, line 676 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
This function is called in a mapping through the expression register autoload/youcompleteme.vim, line 690 at r1 (raw file): Previously, bstaletic wrote…
That's exactly why. python/ycm/youcompleteme.py, line 278 at r3 (raw file):
I renamed the function because we now actually send the request. python/ycm/youcompleteme.py, line 299 at r3 (raw file):
I renamed the function to be consistent with Comments from Reviewable |
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. python/ycm/youcompleteme.py, line 278 at r3 (raw file): Previously, micbou wrote…
Ha, I was mulling over should I point that out or not. :) Comments from Reviewable |
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. a discussion (no related file): Previously, micbou wrote…
If i add a let response = s:Pyeval( 'ycm_state.GetCompletionResponse()' )
echom 'response.completions = ' . response.completions
...
I think the issue is something like: Comments from Reviewable |
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. a discussion (no related file): Previously, puremourning (Ben Jackson) wrote…
OK i got the value printed by doing this: exec s:python_until_eof
import logging
response_p = ycm_state.GetCompletionResponse()
print( 'response: {0}'.format( response_p ) )
EOF
let response = s:Pyeval( 'response_p' )
So i think the issue is that Vim can't deserialise the 'errors' thing to its internal format? Tis change in for e in errors:
with HandleServerException( truncate = True ):
raise MakeServerException( e )
if 'completions' in response and 'completion_start_column' in response:
return {
'completions': response[ 'completions' ],
'completion_start_column': response[ 'completion_start_column' ]
} Comments from Reviewable |
Reviewed 5 of 9 files at r1, 2 of 3 files at r3. Comments from Reviewable |
Alright, thanks for sharing the code. I agree that users would most likely like to customise it, so leaving it out would be the best. Reviewed 3 of 3 files at r3. autoload/youcompleteme.vim, line 240 at r1 (raw file): Previously, micbou wrote…
Sounds good to me. Just one note, it's Comments from Reviewable |
4074333
to
752e491
Compare
Reviewed 1 of 1 files at r4. a discussion (no related file):
Yes, that's the issue. Fixed. autoload/youcompleteme.vim, line 240 at r1 (raw file): Previously, bstaletic (Boris Staletic) wrote…
Sorry, I mixed up the two because of the Comments from Reviewable |
9525ae1
to
cb83e99
Compare
Reviewed 1 of 1 files at r5, 2 of 2 files at r7. a discussion (no related file): Previously, micbou wrote…
I have tried the example above and it seems to work. I didn't get the semantic completion after entering the second space. The tests are passing as well, so this is as far as I can see. Comments from Reviewable |
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. Comments from Reviewable |
cb83e99
to
60fb435
Compare
I fixed a few bugs and did some refactoring. I am going to give this PR one more week of testing before changing its status to ready (unless a major issue is found). Reviewed 1 of 1 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, 1 of 1 files at r8. autoload/youcompleteme.vim, line 684 at r7 (raw file):
Removed this function as it was only used in autoload/youcompleteme.vim, line 526 at r8 (raw file):
Renamed to be consistent with autoload/youcompleteme.vim, line 534 at r8 (raw file):
Renamed the parameter from autoload/youcompleteme.vim, line 575 at r8 (raw file):
I did remove this condition but it is actually needed to avoid the autoload/youcompleteme.vim, line 580 at r8 (raw file):
Don't display last completions if we are inside a comment/string or on a blank line. Fix a bug where completions were displayed after a C++-style comment Comments from Reviewable |
With the new system, users will complain about the impossibility to close the completion menu with the Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. Comments from Reviewable |
I believe that we should try to fix that issue here. It may be an edge case, but with our policy stating "every commit is a release", I think we should not break basic Vim functionality. Reviewed 1 of 1 files at r8. Comments from Reviewable |
Bring fully asynchronous completion by polling for completions with a timer then calling completefunc once the completions are ready. Use the start column returned by the server in completefunc. Immediately display the last completion on the TextChangedI event to prevent the popup menu disappearing while waiting for the completions. Handle the TextChangedI event not being triggered while the completion menu is open by closing the menu when inserting a character through the InsertCharPre event, and when deleting a character on the <BS> and <C-h> keys.
@zzbot r+ Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. Comments from Reviewable |
📌 Commit 377e472 has been approved by |
[READY] Rewrite completion system There is a number of issues with the current completion system: - UI is blocked until the completions are returned by the server, the request timed out, or a key is pressed by the user. This leads to a rough experience when completions take too much time: cursor disappearing and timeout errors (see #2192 and #2574). Users even [increase the timeout by manually editing the `completion_request.py` file](https://github.com/Valloric/YouCompleteMe/blob/master/python/ycm/client/completion_request.py#L30) to avoid these errors, which exacerbate the issue and, in some cases, make the plugin unusable (see #2574). - no fuzzy matching from omnifunc when forcing semantic completion. See #961; - no fuzzy matching when deleting characters. Vim filtering is used instead: ![completion-bug-deleting-characters](https://cloud.githubusercontent.com/assets/10026824/26276156/f298c6de-3d71-11e7-92da-d22186239c27.gif) Neovim and MacVim are not affected. - completion menu disappears after deleting a character and inserting one: ![completion-bug-reinserting-character](https://cloud.githubusercontent.com/assets/10026824/26276192/b3ed0f7a-3d72-11e7-8c64-523a0a59cbdc.gif) Neovim and MacVim are not affected. - ignore the start column returned by the server. See PR #2489. - subject to flickers. This one depends a lot on the version of Vim. Completion is almost flicker-free in Neovim and MacVim. Not too bad in console Vim (except in fast terminal like [alacritty](https://github.com/jwilm/alacritty)). Awful in gVim GTK2 (a bit better on GTK3) and gVim on Windows. This PR is an attempt at fixing all of these issues while reducing flickers as best as possible (due to how completion works in Vim, a flicker-free experience is impossible to achieve). Here's how: - poll for completions using a timer and call `completefunc` once the completions are ready. Use the start column returned by the server in `completefunc`; - immediately display the last completions on the `TextChangedI` event to prevent the popup menu disappearing while waiting for the completions. This reduces flickering; - use the `InsertCharPre` event to close the completion menu just before inserting a character. This way the `TextChangedI` event is triggered when the character is inserted (this event is not fired when the completion menu is visible). This replaces the `refresh` option set to `always` in `completefunc` and the `s:cursor_moved` hack; - remap the backspace key to close the completion menu when deleting a character and thus triggering the `TextChangedI` event; - send a request with `force_semantic` set to `True` when forcing semantic completion instead of calling the omnifunc. This fixes the issue where there is no fuzzy matching for custom omnifuncs. Here's a demo where I added a spin animation on the command line while loading the completions to show that it doesn't block the Vim UI: ![async-completion-for-real](https://cloud.githubusercontent.com/assets/10026824/26277295/0f16a718-3d86-11e7-90f3-8a56bbf53f9f.gif) Fixes #961. Fixes #1282. Fixes #1881. Fixes #2192. Fixes #2500. Fixes #2574. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2657) <!-- Reviewable:end -->
☀️ Test successful - status-appveyor, status-travis |
[READY] Restore cursor position after omnifunc call When compiled without C-family support, YCM will use the default omnifunc from Vim (`ccomplete#Complete`) to provide semantic completion. This omnifunc calls [`searchdecl`](http://vimdoc.sourceforge.net/htmldoc/eval.html#searchdecl()) to find a declaration, which is supposed to move the cursor to that declaration. However, the cursor is not moved when called through the omni completion mapping (`CTRL-X CTRL-O`). Since PR #2657, YCM calls the omnifunc outside completion mode and thus the cursor is moved to the found declaration after typing `.` or `->`. Considering this `searchdecl` trick may be used by other omnifuncs, we fix the issue by always restoring the cursor position after calling the omnifunc. Fixes #2698. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2707) <!-- Reviewable:end -->
Since I've updated the plug-in I'm affected by this, |
@sesselastronaut That's because of these lines: if has("gui_running")
inoremap <C-Space> <C-x><C-o>
else
inoremap <Nul> <C-x><C-o>
endif in your vimrc. You don't need them anymore. See issue #2699. |
@micbou thanks for your response. I've removed those lines but still get the same behaviour featuring the
options from the linked threat but they do not change it either. |
What's the output of |
|
Thanks. I see the issue. We don't check if :set filetype? ? |
it is a latex document and |
@micbou thanks for that, I've updated the plugin and the error is gone but unfortunately the autocompletion with I've tried setting Don't know if this info is helpful to track this down but strangely enough with |
Could you paste the contents of the ycmd stderr logfile? You can open it directly in Vim with the |
ycmd stderr logfile output as follows:
|
Run the |
Ok, |
…ble, r=puremourning [READY] Do not disable omnifunc when filetype completion is disabled Prior to PR #2657, it was possible to trigger Vim's omnifunc with `<C-Space>` even if semantic completion was disabled for the current filetype through the `g:ycm_filetype_specific_completion_to_disable` option. It worked because `<C-Space>` was mapped to `<C-X><C-O><C-P>`, which are the keys to trigger the omnifunc. PR #2657 changed that by making `<C-Space>` directly call the `SendCompletionRequest` function with `force_semantic` sets to `True`. This change was necessary to get fuzzy matching with the omnifunc (see issue #961) but broke the `<C-Space>` behavior when filetype completion is disabled. This PR restores that behavior. Fixes #2950. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2978) <!-- Reviewable:end -->
There is a number of issues with the current completion system:
completion_request.py
file to avoid these errors, which exacerbate the issue and, in some cases, make the plugin unusable (see YCM hangs on completion probably due to interference in map command #2574).Neovim and MacVim are not affected.
Neovim and MacVim are not affected.
This PR is an attempt at fixing all of these issues while reducing flickers as best as possible (due to how completion works in Vim, a flicker-free experience is impossible to achieve). Here's how:
completefunc
once the completions are ready. Use the start column returned by the server incompletefunc
;TextChangedI
event to prevent the popup menu disappearing while waiting for the completions. This reduces flickering;InsertCharPre
event to close the completion menu just before inserting a character. This way theTextChangedI
event is triggered when the character is inserted (this event is not fired when the completion menu is visible). This replaces therefresh
option set toalways
incompletefunc
and thes:cursor_moved
hack;TextChangedI
event;force_semantic
set toTrue
when forcing semantic completion instead of calling the omnifunc. This fixes the issue where there is no fuzzy matching for custom omnifuncs.Here's a demo where I added a spin animation on the command line while loading the completions to show that it doesn't block the Vim UI:
Fixes #961.
Fixes #1282.
Fixes #1881.
Fixes #2192.
Fixes #2500.
Fixes #2574.
This change is