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

[READY] Rewrite completion system #2657

Merged
merged 2 commits into from Jun 25, 2017

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented May 20, 2017

There is a number of issues with the current completion system:

completion-bug-deleting-characters
Neovim and MacVim are not affected.

  • completion menu disappears after deleting a character and inserting one:

completion-bug-reinserting-character
Neovim and MacVim are not affected.

  • ignore the start column returned by the server. See PR [READY] Reset the start column in omnifunc completer #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). 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

Fixes #961.
Fixes #1282.
Fixes #1881.
Fixes #2192.
Fixes #2500.
Fixes #2574.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented May 20, 2017

Codecov Report

Merging #2657 into master will increase coverage by 0.07%.
The diff coverage is 82.6%.

@@            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

@Valloric
Copy link
Member

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 master and this PR have quite a bit of it in both gvim and console vim. So at least it doesn't make it worse. :)

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... :)

@Valloric
Copy link
Member

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):

  " Remap the backspace key to refresh completions when deleting a character.
  " TODO: should we remap <C-h> too? Should we add an option to specify which

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):

  call s:PollCompletion()
  return ''

Why does this return an empty string?


autoload/youcompleteme.vim, line 690 at r1 (raw file):

function! s:OnBackspaceKey()
  " Using s:StopCompletion here doesn't work.
  " TODO: explain why.

Yes, please. :)


Comments from Reviewable

@puremourning
Copy link
Member

Review status: 0 of 9 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


a discussion (no related file):
For posterity, from gitter seems to trigger semantic completion on a whitespace character in the following test:

i can also confirm that it fixes ycm-core/ycmd#671 when used with ycm-core/ycmd#681

it's triggering completion after space

YCM-new-completion.gif

i hit ' then then

here's the old behaviour

YCM-old-completion.gif

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 .tarn-project

{
  "plugins": {
    "node": {}
  }
}

a discussion (no related file):
For posterity: from gitter.

When a completion request times out, you get tracebacks in Vim, which we want to avoid:


ConnectionError: ('Connection aborted.', BadStatusLine("''",))
Error detected while processing function <SNR>125_PollCompletion[8]..<SNR>125_Pyeval:
line    4:
E859: Failed to convert returned python object to vim value
Error detected while processing function <SNR>125_PollCompletion:
line    9:
E121: Undefined variable: completion_start_column
E15: Invalid expression: {   'start_column': response.completion_start_column,   'candidates': response.completions }
ConnectionError: HTTPConnectionPool(host='127.0.0.1', port=50562): Max retries exceeded with url: /completions (Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x10a459c50>: Failed to establish a new connection: [Errno 61] ...

Comments from Reviewable

@bstaletic
Copy link
Collaborator

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 .ycm_extra_conf.py:

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 -isystem flags.

I added a spin animation on the command line while loading the completions to show that it doesn't block the Vim UI

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 Waiting for completion results followed by thte same spinning line? Though my suggestion may be a bit too long.


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 240 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

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.

My first thought was, since <CR> and <C-h> have the same key codes, we should treat them the same way. But then, upon further thinking, I'm with @Valloric. I can't believe I forgot about my own use case. Those two need not have the same key code and they don't on my system, running Suckless terminal and plain old vim in terminal. I even have remapped <C-h> to move to the split or tmux pane to the right.

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 <C-h> differences between Vim and Neovim click here.


autoload/youcompleteme.vim, line 616 at r1 (raw file):

  " semantic trigger. If we don't, the completion menu stays open while waiting
  " for the server response.
  let s:completion = s:default_completion

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…

Yes, please. :)

Heh, because here we don't want to throw away the completed text, rather just delete a character.

s:StopCompletion() closes the pop-up menu and throw away the completed text with <C-e>, while here we close pop-up menu without throwing away the completed text using <C-y>.

P.S. Vimscrip is a beautiful language!


Comments from Reviewable

@micbou micbou changed the title [RFC] Rewrite completion system [WIP] Rewrite completion system May 21, 2017
@micbou micbou force-pushed the completion-system-rewrite branch 2 times, most recently from 55de038 to 4074333 Compare May 21, 2017 15:55
@micbou
Copy link
Collaborator Author

micbou commented May 21, 2017

Thanks for testing and reviewing. I've pushed some changes.

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 Waiting for completion results followed by thte same spinning line? Though my suggestion may be a bit too long.

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 ycm_state object. That's the code I wrote in a vimrc for the demo:

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.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

For posterity, from gitter seems to trigger semantic completion on a whitespace character in the following test:

i can also confirm that it fixes ycm-core/ycmd#671 when used with ycm-core/ycmd#681

it's triggering completion after space

YCM-new-completion.gif

i hit ' then then

here's the old behaviour

YCM-old-completion.gif

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 .tarn-project

{
  "plugins": {
    "node": {}
  }
}

It seems to be fixed by leaving semantic mode and resetting completions before the first s:Complete() call. Could you try again with the new changes?


a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

For posterity: from gitter.

When a completion request times out, you get tracebacks in Vim, which we want to avoid:


ConnectionError: ('Connection aborted.', BadStatusLine("''",))
Error detected while processing function <SNR>125_PollCompletion[8]..<SNR>125_Pyeval:
line    4:
E859: Failed to convert returned python object to vim value
Error detected while processing function <SNR>125_PollCompletion:
line    9:
E121: Undefined variable: completion_start_column
E15: Invalid expression: {   'start_column': response.completion_start_column,   'candidates': response.completions }
ConnectionError: HTTPConnectionPool(host='127.0.0.1', port=50562): Max retries exceeded with url: /completions (Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x10a459c50>: Failed to establish a new connection: [Errno 61] ...

Strange, the completion_start_column key should always exist. Could you check the value of the response variable returned by GetCompletionResponse when the error occurs?


autoload/youcompleteme.vim, line 240 at r1 (raw file):

Previously, bstaletic wrote…

My first thought was, since <CR> and <C-h> have the same key codes, we should treat them the same way. But then, upon further thinking, I'm with @Valloric. I can't believe I forgot about my own use case. Those two need not have the same key code and they don't on my system, running Suckless terminal and plain old vim in terminal. I even have remapped <C-h> to move to the split or tmux pane to the right.

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 <C-h> differences between Vim and Neovim click here.

I added <C-h> to the list of keys. I also added the <silent> attribute so that if the user has already mapped the key to something else, we don't override it.


autoload/youcompleteme.vim, line 616 at r1 (raw file):

Previously, bstaletic wrote…

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.

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…

Why does this return an empty string?

This function is called in a mapping through the expression register <C-R>= (:h c_CTRL-R_=) which inserts its return value. We don't insert anything if we return an empty string. Added a comment.


autoload/youcompleteme.vim, line 690 at r1 (raw file):

Previously, bstaletic wrote…

Heh, because here we don't want to throw away the completed text, rather just delete a character.

s:StopCompletion() closes the pop-up menu and throw away the completed text with <C-e>, while here we close pop-up menu without throwing away the completed text using <C-y>.

P.S. Vimscrip is a beautiful language!

That's exactly why.


python/ycm/youcompleteme.py, line 278 at r3 (raw file):

  def SendCompletionRequest( self, force_semantic = False ):

I renamed the function because we now actually send the request.


python/ycm/youcompleteme.py, line 299 at r3 (raw file):

  def CompletionRequestReady( self ):
    return bool( self._latest_completion_request and
                 self._latest_completion_request.Done() )

I renamed the function to be consistent with FileParseRequestReady.


Comments from Reviewable

@Valloric
Copy link
Member

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…

I renamed the function because we now actually send the request.

Ha, I was mulling over should I point that out or not. :)


Comments from Reviewable

@puremourning
Copy link
Member

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


a discussion (no related file):

Previously, micbou wrote…

Strange, the completion_start_column key should always exist. Could you check the value of the response variable returned by GetCompletionResponse when the error occurs?

If i add a echom to print the value, I get

  let response = s:Pyeval( 'ycm_state.GetCompletionResponse()' )
  echom 'response.completions = ' . response.completions
...
E859: Failed to convert returned python object to vim value

I think the issue is something like: GetCompletionResponse raises an exception, and returns "nothing"? Vim can't convert that into anything like a dict, so errors and doesn't set it?


Comments from Reviewable

@puremourning
Copy link
Member

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…

If i add a echom to print the value, I get

  let response = s:Pyeval( 'ycm_state.GetCompletionResponse()' )
  echom 'response.completions = ' . response.completions
...
E859: Failed to convert returned python object to vim value

I think the issue is something like: GetCompletionResponse raises an exception, and returns "nothing"? Vim can't convert that into anything like a dict, so errors and doesn't set it?

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' )
ConnectionError: ('Connection aborted.', BadStatusLine("''",))
response: {u'errors': [{u'exception': {u'request': {u'body': u'{"column": 8, "file_name": "/Users/ben/Development/Swift/Test/SwiftTest/
SwiftTest/ViewController.swift", "line": 16, "flags": ["/Users/ben/Development/Swift/Test/SwiftTest/SwiftTest/AppDelegate.swift", "-tar
get", "x86_64-apple-macosx10.12", "-enable-objc-interop", "-sdk", "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform
/Developer/SDKs/MacOSX10.12.sdk", "-I", "/Users/ben/Development/Swift/Test/SwiftTest/build/Release", "-F", "/Users/ben/Development/Swif
t/Test/SwiftTest/build/Release", "-g", "-serialize-debugging-options", "-O", "-module-name", "SwiftTest", "-emit-objc-header-path", "/U
sers/ben/Development/Swift/Test/SwiftTest/build/SwiftTest.build/Release/SwiftTest.build/Objects-normal/x86_64/SwiftTest-Swift.h", "-num
-threads", "8"], "contents": "//\\n//  ViewController.swift\\n//  SwiftTest\\n//\\n//  Created by Ben Jackson on 20/05/2017.\\n//  Copy
right \\u00a9 2017 Ben Jackson. All rights reserved.\\n//\\n\\nimport Cocoa\\n\\nclass ViewController: NSViewController {\\n\\n    over
ride func viewDidLoad() {\\n        super.viewDidLoad()\\n\\n        b\\n    }\\n\\n    @IBOutlet weak var button_: NSButton!\\n}\\n\\n
"}', u'url': u'http://127.0.0.1:56334/completions', u'TYPE': u'PreparedRequest', u'headers': {u'_store': {u'content-length': [u'Content
-Length', u'1132'], u'x-http-hmac': [u'x-http-hmac', u'oa9gbzO+MlRWiUoN07pxFp9A4m5GfKrwK+12WDUwh+c='], u'accept-encoding': [u'Accept-En
coding', u'gzip, deflate'], u'accept': [u'Accept', u'*/*'], u'user-agent': [u'User-Agent', u'python-requests/2.9.1'], u'connection': [u
'Connection', u'keep-alive'], u'content-type': [u'content-type', u'application/json']}, u'TYPE': u'CaseInsensitiveDict'}, u'hooks': {u'
response': []}, u'_cookies': {u'_now': 1495397816, u'_policy': {u'strict_rfc2965_unverifiable': True, u'strict_ns_domain': 0, u'_allowe
d_domains': None, u'rfc2109_as_netscape': None, u'rfc2965': False, u'strict_domain': False, u'_now': 1495397816, u'strict_ns_set_path':
 False, u'strict_ns_unverifiable': False, u'strict_ns_set_initial_dollar': False, u'hide_cookie2': False, u'_blocked_domains': [], u'TY
PE': u'instance', u'netscape': True}, u'_cookies_lock': {u'_Verbose__verbose': False, u'_RLock__owner': None, u'TYPE': u'_RLock', u'_RL
ock__block': u'<thread.lock object at 0x108ffa930>', u'_RLock__count': 0}, u'_cookies': {}, u'TYPE': u'RequestsCookieJar'}, u'method':
u'POST'}, u'TYPE': u'ConnectionError', u'response': None}, u'traceback': u'Traceback (most recent call last):\n  File "/Users/ben/.vim/
bundle/YouCompleteMe/third_party/ycmd/ycmd/../ycmd/handlers.py", line 103, in GetCompletions\n    .ComputeCandidates( request_data ) )\
n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/ycmd/../ycmd/completers/completer.py", line 217, in ComputeCandidates\n
   candidates = self._GetCandidatesFromSubclass( request_data )\n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/ycmd/../
ycmd/completers/completer.py", line 233, in _GetCandidatesFromSubclass\n    raw_completions = self.ComputeCandidatesInner( request_data
 )\n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/ycmd/../ycmd/completers/swift/swift_completer.py", line 263, in Compu
teCandidatesInner\n    for completion in self._FetchCompletions( request_data ) ]\n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_p
arty/ycmd/ycmd/../ycmd/completers/swift/swift_completer.py", line 293, in _FetchCompletions\n    request_data )\n  File "/Users/ben/.vi
m/bundle/YouCompleteMe/third_party/ycmd/ycmd/../ycmd/completers/swift/swift_completer.py", line 196, in _GetResponse\n    headers = ext
ra_headers )\n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/requests/requests/api.py", line 53, in request\
n    return session.request(method=method, url=url, **kwargs)\n  File "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/third_part
y/requests/requests/sessions.py", line 468, in request\n    resp = self.send(prep, **send_kwargs)\n  File "/Users/ben/.vim/bundle/YouCo
mpleteMe/third_party/ycmd/third_party/requests/requests/sessions.py", line 576, in send\n    r = adapter.send(request, **kwargs)\n  Fil
e "/Users/ben/.vim/bundle/YouCompleteMe/third_party/ycmd/third_party/requests/requests/adapters.py", line 426, in send\n    raise Conne
ctionError(err, request=request)\nConnectionError: (\'Connection aborted.\', BadStatusLine("\'\'",))\n', u'message': u'(\'Connection ab
orted.\', BadStatusLine("\'\'",))'}], u'completions': [], u'completion_start_column': 9}
Error detected while processing function <SNR>126_PollCompletion[15]..<SNR>126_Pyeval:
line    4:
E859: Failed to convert returned python object to vim value
Error detected while processing function <SNR>126_PollCompletion:
line   16:
E121: Undefined variable: completion_start_column
E15: Invalid expression: {   'start_column': response.completion_start_column,   'candidates': response.completions }

So i think the issue is that Vim can't deserialise the 'errors' thing to its internal format?

Tis change in completion_request.py fixes it:

      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

@vheon
Copy link
Contributor

vheon commented May 21, 2017

Reviewed 5 of 9 files at r1, 2 of 3 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

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.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 240 at r1 (raw file):

Previously, micbou wrote…

I added <C-h> to the list of keys. I also added the <silent> attribute so that if the user has already mapped the key to something else, we don't override it.

Sounds good to me. Just one note, it's <unique> not <silent>. The code is good, but your comment made me look up Vim documentation.


Comments from Reviewable

@micbou micbou force-pushed the completion-system-rewrite branch from 4074333 to 752e491 Compare May 22, 2017 00:23
@micbou
Copy link
Collaborator Author

micbou commented May 22, 2017

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


a discussion (no related file):

So i think the issue is that Vim can't deserialise the 'errors' thing to its internal format?

Yes, that's the issue. Fixed.


autoload/youcompleteme.vim, line 240 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Sounds good to me. Just one note, it's <unique> not <silent>. The code is good, but your comment made me look up Vim documentation.

Sorry, I mixed up the two because of the silent! at the start of the command.


Comments from Reviewable

@micbou micbou force-pushed the completion-system-rewrite branch 3 times, most recently from 9525ae1 to cb83e99 Compare May 22, 2017 10:39
@bstaletic
Copy link
Collaborator

Reviewed 1 of 1 files at r5, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


a discussion (no related file):

Previously, micbou wrote…

It seems to be fixed by leaving semantic mode and resetting completions before the first s:Complete() call. Could you try again with the new changes?

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 :lgtm: as far as I can see.


Comments from Reviewable

@Valloric
Copy link
Member

This looks pretty good to me! :lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@micbou micbou force-pushed the completion-system-rewrite branch from cb83e99 to 60fb435 Compare May 28, 2017 21:12
@micbou
Copy link
Collaborator Author

micbou commented May 28, 2017

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.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


autoload/youcompleteme.vim, line 684 at r7 (raw file):

    call feedkeys( "\<C-e>", 'n' )
  endif
endfunction

Removed this function as it was only used in s:OnInsertChar.


autoload/youcompleteme.vim, line 526 at r8 (raw file):

function! s:OnInsertChar()

Renamed to be consistent with OnDeleteChar.


autoload/youcompleteme.vim, line 534 at r8 (raw file):

function! s:OnDeleteChar( key )

Renamed the parameter from char to key since it's actually a key (<BS> or <C-h>), not a character.


autoload/youcompleteme.vim, line 575 at r8 (raw file):

  endif

  if &completefunc == "youcompleteme#CompleteFunc" &&

I did remove this condition but it is actually needed to avoid the E764: Option 'completefunc' is not set error.


autoload/youcompleteme.vim, line 580 at r8 (raw file):

        \ !s:OnBlankLine()
    " Immediately call previous completion to avoid flickers.
    call s:Complete()

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

@micbou micbou changed the title [WIP] Rewrite completion system [RFC] Rewrite completion system May 29, 2017
@micbou
Copy link
Collaborator Author

micbou commented Jun 4, 2017

With the new system, users will complain about the impossibility to close the completion menu with the <C-y> key. It's already an issue with the current system (see #1282) but it's way worse with the new one. We could remap the <C-y> key to really close the completion menu. We could even allow the user to change the key through an option. Should we implement a way to close the menu in this PR?


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@bstaletic
Copy link
Collaborator

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.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@micbou micbou changed the title [RFC] Rewrite completion system [WIP] Rewrite completion system Jun 8, 2017
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.
@bstaletic
Copy link
Collaborator

@zzbot r+


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Jun 25, 2017

📌 Commit 377e472 has been approved by bstaletic

@zzbot
Copy link
Contributor

zzbot commented Jun 25, 2017

⌛ Testing commit 377e472 with merge 6ba6b56...

zzbot added a commit that referenced this pull request Jun 25, 2017
[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 -->
@zzbot
Copy link
Contributor

zzbot commented Jun 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: bstaletic
Pushing 6ba6b56 to master...

@zzbot zzbot merged commit 377e472 into ycm-core:master Jun 25, 2017
@micbou micbou deleted the completion-system-rewrite branch June 25, 2017 16:43
zzbot added a commit that referenced this pull request Jul 7, 2017
[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 -->
@sesselastronaut
Copy link

sesselastronaut commented Jul 18, 2017

autoload/youcompleteme.vim, line 575 at r8 (raw file):

  endif

  if &completefunc == "youcompleteme#CompleteFunc" &&

I did remove this condition but it is actually needed to avoid the E764: Option 'completefunc' is not set error.

Since I've updated the plug-in I'm affected by this, <C-Space> now throws the E764 error above. So far couldn't find any solution. I've g:ycm_auto_trigger set to 0 as pointed out here #1634 but this doesn't change the issue. Many thanks for any indication to find the fly in the ointment. Attached my vimrc.txt

@micbou
Copy link
Collaborator Author

micbou commented Jul 18, 2017

@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.

@sesselastronaut
Copy link

sesselastronaut commented Jul 18, 2017

@micbou thanks for your response. I've removed those lines but still get the same behaviour featuring the E764: Option 'completefunc' is not set error. I've also tried the

let g:ycm_key_invoke_completion = '<leader><leader>'
let g:ycm_key_list_stop_completion = ['<C-y>', '<CR>']

options from the linked threat but they do not change it either.

@micbou
Copy link
Collaborator Author

micbou commented Jul 18, 2017

What's the output of :imap <C-Space>?

@sesselastronaut
Copy link

i <C-Space> * <C-R>=<SNR>50_InvokeSemanticCompletion()<CR>

@micbou
Copy link
Collaborator Author

micbou commented Jul 18, 2017

Thanks. I see the issue. We don't check if completefunc is set to youcompleteme#CompleteFunc when forcing semantic completion. I'll work on a fix. Still, I am wondering why it's not set in your case. What is the filetype of the current buffer:

:set filetype?

?

@sesselastronaut
Copy link

it is a latex document and :set filetype? returns filetype=tex.
Many thanks in advance for your effort to fix this!

@micbou micbou mentioned this pull request Jul 31, 2017
12 tasks
@sesselastronaut
Copy link

sesselastronaut commented Aug 2, 2017

@micbou thanks for that, I've updated the plugin and the error is gone but unfortunately the autocompletion with <C-Space> still doesn't work for me.

I've tried settinglet g:ycm_key_invoke_completion = '<C-Space>' which is afaik the default anyway. The output of :imap <C-Space> is i <C-Space> * <C-R>=<SNR>95_InvokeSemanticCompletion()<CR>.

Don't know if this info is helpful to track this down but strangely enough with let g:ycm_key_invoke_completion = '<C-Space>' set/uncommented I've observed the following behaviour: when I open a latex file and type \cite{ or \cite{}followed by <C-Space> nothing really happens. However leaving insert mode immediately results in The ycmd server SHUT DOWN (restart w... the instructions in the documentation. message.

@micbou
Copy link
Collaborator Author

micbou commented Aug 2, 2017

Could you paste the contents of the ycmd stderr logfile? You can open it directly in Vim with the :YcmToggleLogs command. Use <TAB> to cycle through the logfiles.

@sesselastronaut
Copy link

sesselastronaut commented Aug 2, 2017

ycmd stderr logfile output as follows:

2017-08-02 16:19:20,636 - ERROR - ycm_core library not detected; you need to compile it by running the build.py script. See the documentation for more details.
Traceback (most recent call last):
  File "/home/olsen/.vim/bundle/YouCompleteMe/third_party/ycmd/ycmd/server_utils.py", line 96, in CompatibleWithCurrentCore
    ycm_core = ImportCore()
  File "/home/olsen/.vim/bundle/YouCompleteMe/third_party/ycmd/ycmd/server_utils.py", line 88, in ImportCore
    import ycm_core as ycm_core
ImportError: No module named ycm_core

@micbou
Copy link
Collaborator Author

micbou commented Aug 2, 2017

Run the install.py script in /home/olsen/.vim/bundle/YouCompleteMe folder and try again.

@sesselastronaut
Copy link

Ok, cmake was missing on my system. After installing it and running the install.py the completion with <C-Space> seems to be working now :) many thanks for tracking this down!

@bstaletic bstaletic mentioned this pull request Aug 14, 2017
12 tasks
zzbot added a commit that referenced this pull request Apr 14, 2018
…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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment