[READY] Display diagnostics and parse initial buffer asynchronously using timers #2636

micbou · 2017-05-06T12:03:13Z

Currently, we wait for an autocommand to trigger to display diagnostics. This is not ideal as users have to do some actions like moving the cursor or editing the buffer to see them. We can improve that by using the timers feature introduced in Vim 7.4.1578 to display diagnostics asynchronously. Here's a demo:

By displaying diagnostics asynchronously, we can drop the CursorHold and CursorHoldI autocommands as well as the g:ycm_allow_changing_updatetime option. In addition, we can now parse the current buffer and display diagnostics on the TextChanged event instead of the CursorMoved one. Note that we still need to check b:changed_tick for the InsertLeave event.

Given that our policy is to support the latest version of Ubuntu LTS which is 16.04, and that version bundles Vim 7.4.1689, we can increase our Vim version requirement to 7.4.1578.

Next steps will be to use timers for parsing the buffer when the server becomes ready and for completions.

Fixes #2165.

This change is

codecov-io · 2017-05-06T12:16:43Z

Codecov Report

Merging #2636 into master will increase coverage by 0.81%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2636      +/-   ##
==========================================
+ Coverage   87.67%   88.48%   +0.81%     
==========================================
  Files          19       19              
  Lines        1947     1946       -1     
==========================================
+ Hits         1707     1722      +15     
+ Misses        240      224      -16

Valloric · 2017-05-06T17:26:49Z

Given that our policy is to support the latest version of Ubuntu LTS which is 16.04, and that version bundles Vim 7.4.1689, we can increase our Vim version requirement to 7.4.1578.

With that, I'm sold. 👍 Fantastic PR, thank you! I've been wishing to get rid of the CursorHold nonsense for triggering diagnostics for years!

with minor inline comment.

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

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

    call timer_stop( s:timers.handle_file_parse_request )
    let s:timers.handle_file_parse_request = timer_start(
          \ 100, function( 's:HandleFileParseRequest' ) )

This 100 number should be extracted into a named constant (a script-level var in vimscript might be the best we can do). The var name should reflect the unit of the number, since 100 doesn't tell me is this millis or seconds or what.

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

  if !s:Pyeval( "ycm_state.FileParseRequestReady()" )
    let s:timers.handle_file_parse_request = timer_start(
          \ 100, function( 's:HandleFileParseRequest' ) )

Same as above.

Comments from Reviewable

micbou · 2017-05-06T17:59:07Z

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

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

Previously, Valloric (Val Markovic) wrote…

This 100 number should be extracted into a named constant (a script-level var in vimscript might be the best we can do). The var name should reflect the unit of the number, since 100 doesn't tell me is this millis or seconds or what.

Done.

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

Previously, Valloric (Val Markovic) wrote…

Same as above.

Done.

Comments from Reviewable

puremourning · 2017-05-06T19:19:03Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):
Awesome.

I need to try this out in anger, but meanwhile i have a few questions (disccussion points reallly; i could probably work out all the answers, but I'm being rude and lazy and just asking instead.... hope that's OK :S )

About when we send the file parse request

From a brief look at the diff I think we're now calling OnFileReadyToParse whenever the text is changed in normal mode. Did we previously only do that after a 2 second delay? Do we have concerns that this could potentially lead to increased load on the server, and potentially a backlog of parse requests?
Following on from above, how often is TextChanged called when running something like a global search/replace? I wonder that the client might flood the server when Vim makes programmatic changes to a buffer.
I assume that we still send the parse request on buffer load and buffer visit and only if the content has changed?
It might be just me, but when diags don't appear for some time (usually due to it taking ages), i tend to wiggle the cursor (jkjkjkjkjk). I'll have to try out the new mechanism to see if it is stil possible to sort of force YCM to update its diags without blocking call (e.g. YcmDiags).

About the timers mechanism

Are there any race conditions to worry about in terms of cancelling the timer on buffer close, on YcmRestartServer, etc. ? It doesn't look like we are doing that now, so just checking it isn't required.
what informed the decision to use 100ms? 10 times per second. I guess it was just arbitrary? I've always believed that 250ms was "human timescales" but that's just empirical from experience. I suggest we should have the timeout as high as possible so that the experience still feels smooth. Polling is a brute force mechanism, so we should be as gentle as possible :)

Anyway, thanks, as always for working on this!

autoload/youcompleteme.vim, line 486 at r3 (raw file):

    exec s:python_command "ycm_state.OnFileReadyToParse()"

    call timer_stop( s:timers.handle_file_parse_request.id )

I take it that it is legal to pass an invalid ID here?

Comments from Reviewable

bstaletic · 2017-05-07T00:06:10Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):
@puremourning I'll try to address your concerns as good as I can.

Requests

Do we have concerns that this could potentially lead to increased load on the server, and potentially a backlog of parse requests?

I belive, in theory this could produce more requests, but judging by my limited testing, it is not an issue. I'll explain what I tried later on.

how often is TextChanged called when running something like a global search/replace?

TextChanged is triggered once every time b:changedtick is incremented while vim is in normal mode. Which means once for every match+substitution.

I assume that we still send the parse request on buffer load and buffer visit and only if the content has changed?

Function s:OnFileReadToParse() is now called on TextChanged, InsertEnter and InsertLeave. But the actual call to ycm_state.OnFileReadyToParse() is called in three cases.

The first moment the server becomes ready.
Buffer has changed (i.e. b:changedtick != b:ycm_changedtick)
Parsing her been forced.

wiggle the cursor (jkjkjkjkjk). I'll have to try out the new mechanism to see if it is stil possible to sort of force YCM to update its diags without blocking call

As far as I can tell, that won't be necessary any more as the diagnostics will be updated as soon as possible thanks to the timers being used.

Timer mechanism

what informed the decision to use 100ms? 10 times per second. I guess it was just arbitrary? I've always believed that 250ms was "human timescales"

I just tried incrementing the timer period by 100ms.

100ms - Seems instant/
200ms - Noticable if one is paying attention, in real world, one would have better things to do than measure response.
300ms - Delay starts being noticable, but is far from being too long.
400ms - Delay is clearly noticable, but doesn't annoy me too much.
500ms - This started being annoying.

Obviously these observations are heavily subjective.

How I tested

To have everything snappy I used an almost trivial c++ file:

#include <iostream>
#include <vector>

class A {
	std::vector<int> myVector;
public:
	A(int il) : myVector(il) {}
	void is_empty() {
		if (myVector.empty()) std::cout << "(empty)";
	}
	std::vector<int>&& stealVector() {
		return std::move(myVector);
	}
};

int main() {
	A a(5);
	std::vector<int> b{2};
	b = a.stealVector();
	a.is_empty();
	return 0;
}

To put as much stress on the server as possible I pasted a few random lines of text from my browser which created ~330 b:changedtick increments and thus OnBufferReadyToParse notifications. This all worked very fast.

The next "test" was quick edits by spamming x and p along with walking the cursor around the file. Still, everything worked like it should.

And the final test was on ycmd's IdentifierCompleter.cpp. First, this required about a second to compile and display diagnostics. So here, the question isn't if the diagnostics showed up instantly, rather if they have shown about a second after the last change in a bunch. Once again, pasting a few lines of text and spamming vim normal mode commands worked as expected. The diagnostics were updated about a second after I stopped making changes.

@micbou Thanks for the amazing PR.

autoload/youcompleteme.vim, line 486 at r3 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I take it that it is legal to pass an invalid ID here?

Yes, one can pass any integer to timer_stop() and vim won't complain.

Comments from Reviewable

micbou · 2017-05-07T00:37:48Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):
@bstaletic Thanks for testing.

From a brief look at the diff I think we're now calling OnFileReadyToParse whenever the text is changed in normal mode. Did we previously only do that after a 2 second delay? Do we have concerns that this could potentially lead to increased load on the server, and potentially a backlog of parse requests?

No, we were calling OnFileReadyToParse each time the cursor moved in normal mode and then calling it again 2 second later to display the diagnostics if the request was done. Cursor will generally move when a change is made to the buffer so sending a FileReadyToParse notification on the TextChanged event instead of the CursorMoved one shouldn't increase much the server workload.

Following on from above, how often is TextChanged called when running something like a global search/replace? I wonder that the client might flood the server when Vim makes programmatic changes to a buffer.

I've checked it and TextChanged is only triggered once for a global search/replace.

I assume that we still send the parse request on buffer load and buffer visit and only if the content has changed?

Since PR #2629, we always parse the buffer on load and visit even if its content didn't change because its parsing may be affected by changes from other buffers. However, I realize that it doesn't make sense to parse the buffer when leaving insert mode if we parse it when the buffer has changed. This means that we can remove the OnFileReadyToParsecall from the InsertLeaveautocommand and drop the b:ycm_changedtick variable. I'll give it a try.

It might be just me, but when diags don't appear for some time (usually due to it taking ages), i tend to wiggle the cursor (jkjkjkjkjk). I'll have to try out the new mechanism to see if it is stil possible to sort of force YCM to update its diags without blocking call (e.g. YcmDiags).

You were triggering the CursorMoved event until the request was done and the diagnostics could be displayed. You shouldn't have to do this anymore.

Are there any race conditions to worry about in terms of cancelling the timer on buffer close, on YcmRestartServer, etc. ? It doesn't look like we are doing that now, so just checking it isn't required.

When leaving a buffer, the new one is parsed and thus the timer is cancelled. When restarting the server, the request is reset so the pending timer cannot end until it's cancelled by a new parse. We should probably add a function that stops all timers and call it on the BufLeave event and on the :YcmRestartServer command.

what informed the decision to use 100ms? 10 times per second. I guess it was just arbitrary? I've always believed that 250ms was "human timescales" but that's just empirical from experience. I suggest we should have the timeout as high as possible so that the experience still feels smooth. Polling is a brute force mechanism, so we should be as gentle as possible :)

100ms because it's a round number and 10ms is too low while 1s is too high.

Comments from Reviewable

micbou · 2017-05-07T00:47:26Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, micbou wrote…

@bstaletic Thanks for testing.

From a brief look at the diff I think we're now calling OnFileReadyToParse whenever the text is changed in normal mode. Did we previously only do that after a 2 second delay? Do we have concerns that this could potentially lead to increased load on the server, and potentially a backlog of parse requests?

No, we were calling OnFileReadyToParse each time the cursor moved in normal mode and then calling it again 2 second later to display the diagnostics if the request was done. Cursor will generally move when a change is made to the buffer so sending a FileReadyToParse notification on the TextChanged event instead of the CursorMoved one shouldn't increase much the server workload.

Following on from above, how often is TextChanged called when running something like a global search/replace? I wonder that the client might flood the server when Vim makes programmatic changes to a buffer.

I've checked it and TextChanged is only triggered once for a global search/replace.

I assume that we still send the parse request on buffer load and buffer visit and only if the content has changed?

Since PR #2629, we always parse the buffer on load and visit even if its content didn't change because its parsing may be affected by changes from other buffers. However, I realize that it doesn't make sense to parse the buffer when leaving insert mode if we parse it when the buffer has changed. This means that we can remove the OnFileReadyToParsecall from the InsertLeaveautocommand and drop the b:ycm_changedtick variable. I'll give it a try.

It might be just me, but when diags don't appear for some time (usually due to it taking ages), i tend to wiggle the cursor (jkjkjkjkjk). I'll have to try out the new mechanism to see if it is stil possible to sort of force YCM to update its diags without blocking call (e.g. YcmDiags).

You were triggering the CursorMoved event until the request was done and the diagnostics could be displayed. You shouldn't have to do this anymore.

Are there any race conditions to worry about in terms of cancelling the timer on buffer close, on YcmRestartServer, etc. ? It doesn't look like we are doing that now, so just checking it isn't required.

When leaving a buffer, the new one is parsed and thus the timer is cancelled. When restarting the server, the request is reset so the pending timer cannot end until it's cancelled by a new parse. We should probably add a function that stops all timers and call it on the BufLeave event and on the :YcmRestartServer command.

what informed the decision to use 100ms? 10 times per second. I guess it was just arbitrary? I've always believed that 250ms was "human timescales" but that's just empirical from experience. I suggest we should have the timeout as high as possible so that the experience still feels smooth. Polling is a brute force mechanism, so we should be as gentle as possible :)

100ms because it's a round number and 10ms is too low while 1s is too high.

TextChanged is not triggered when leaving insert mode so we can't drop b:ycm_changedtick.

Comments from Reviewable

bstaletic · 2017-05-07T00:53:46Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

TextChanged is not triggered when leaving insert mode so we can't drop b:ycm_changedtick.

There's also TextChangedI which is the same as TextChanged, but is made for insert mode.

Comments from Reviewable

micbou · 2017-05-07T00:55:35Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, bstaletic wrote…

TextChanged is not triggered when leaving insert mode so we can't drop b:ycm_changedtick.

There's also TextChangedI which is the same as TextChanged, but is made for insert mode.

Yes, that's probably why TextChanged doesn't trigger in this case.

Comments from Reviewable

bstaletic · 2017-05-07T12:45:59Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, micbou wrote…

Yes, that's probably why TextChanged doesn't trigger in this case.

I tried to remove the changedtick tracking and instead of updating diagnostics on InsertLeave, update on TextChangedI. It works, but doesn't trigger until the completion menu is closed. Which, now that I think of it, is perfectly reasonable.

The changes I made:

diff --git a/autoload/youcompleteme.vim b/autoload/youcompleteme.vim
index 93d1db4d..9cbf9358 100644
--- a/autoload/youcompleteme.vim
+++ b/autoload/youcompleteme.vim
@@ -478,18 +478,17 @@ function! s:OnFileReadyToParse( ... )
     return
   endif
 
-  " We only want to send a new FileReadyToParse event notification if the buffer
-  " has changed since the last time we sent one, or if forced.
-  if force_parsing || b:changedtick != get( b:, 'ycm_changedtick', -1 )
-    exec s:python_command "ycm_state.OnFileReadyToParse()"
+  if !s:AllowedToCompleteInCurrentBuffer()
+    return
+  endif
 
-    call timer_stop( s:timers.handle_file_parse_request.id )
-    let s:timers.handle_file_parse_request.id = timer_start(
-          \ s:timers.handle_file_parse_request.wait_milliseconds,
-          \ function( 's:HandleFileParseRequest' ) )
+  exec s:python_command "ycm_state.OnFileReadyToParse()"
+
+  call timer_stop( s:timers.handle_file_parse_request.id )
+  let s:timers.handle_file_parse_request.id = timer_start(
+        \ s:timers.handle_file_parse_request.wait_milliseconds,
+        \ function( 's:HandleFileParseRequest' ) )
 
-    let b:ycm_changedtick = b:changedtick
-  endif
 endfunction
 
 
@@ -549,6 +548,8 @@ function! s:OnTextChangedInsertMode()
   if s:omnifunc_mode && !s:Pyeval( 'base.LastEnteredCharIsIdentifierChar()')
     let s:omnifunc_mode = 0
   endif
+
+  call s:OnFileReadyToParse()
 endfunction
 
 
@@ -576,7 +577,6 @@ function! s:OnInsertLeave()
   endif
 
   let s:omnifunc_mode = 0
-  call s:OnFileReadyToParse()
   exec s:python_command "ycm_state.OnInsertLeave()"
   if g:ycm_autoclose_preview_window_after_completion ||
         \ g:ycm_autoclose_preview_window_after_insertion

Comments from Reviewable

micbou · 2017-05-07T13:00:04Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, bstaletic wrote…

I tried to remove the changedtick tracking and instead of updating diagnostics on InsertLeave, update on TextChangedI. It works, but doesn't trigger until the completion menu is closed. Which, now that I think of it, is perfectly reasonable.

The changes I made:

diff --git a/autoload/youcompleteme.vim b/autoload/youcompleteme.vim
index 93d1db4d..9cbf9358 100644
--- a/autoload/youcompleteme.vim
+++ b/autoload/youcompleteme.vim
@@ -478,18 +478,17 @@ function! s:OnFileReadyToParse( ... )
     return
   endif
 
-  " We only want to send a new FileReadyToParse event notification if the buffer
-  " has changed since the last time we sent one, or if forced.
-  if force_parsing || b:changedtick != get( b:, 'ycm_changedtick', -1 )
-    exec s:python_command "ycm_state.OnFileReadyToParse()"
+  if !s:AllowedToCompleteInCurrentBuffer()
+    return
+  endif
 
-    call timer_stop( s:timers.handle_file_parse_request.id )
-    let s:timers.handle_file_parse_request.id = timer_start(
-          \ s:timers.handle_file_parse_request.wait_milliseconds,
-          \ function( 's:HandleFileParseRequest' ) )
+  exec s:python_command "ycm_state.OnFileReadyToParse()"
+
+  call timer_stop( s:timers.handle_file_parse_request.id )
+  let s:timers.handle_file_parse_request.id = timer_start(
+        \ s:timers.handle_file_parse_request.wait_milliseconds,
+        \ function( 's:HandleFileParseRequest' ) )
 
-    let b:ycm_changedtick = b:changedtick
-  endif
 endfunction
 
 
@@ -549,6 +548,8 @@ function! s:OnTextChangedInsertMode()
   if s:omnifunc_mode && !s:Pyeval( 'base.LastEnteredCharIsIdentifierChar()')
     let s:omnifunc_mode = 0
   endif
+
+  call s:OnFileReadyToParse()
 endfunction
 
 
@@ -576,7 +577,6 @@ function! s:OnInsertLeave()
   endif
 
   let s:omnifunc_mode = 0
-  call s:OnFileReadyToParse()
   exec s:python_command "ycm_state.OnInsertLeave()"
   if g:ycm_autoclose_preview_window_after_completion ||
         \ g:ycm_autoclose_preview_window_after_insertion

We can't do that as diagnostics constantly updating while typing is rather annoying.

Comments from Reviewable

bstaletic · 2017-05-07T13:09:31Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, micbou wrote…

We can't do that as diagnostics constantly updating while typing is rather annoying.

Okay, I understand that some (many?) will find it annoying. I personally don't mind it and don't see it as too distracting.

Also, it won't update if a new character is inserted before the update from the previous one. So while "light show" diagnostics is annoying, it really didn't feel like that.

Comments from Reviewable

micbou · 2017-05-07T15:24:30Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, bstaletic wrote…

Okay, I understand that some (many?) will find it annoying. I personally don't mind it and don't see it as too distracting.

Also, it won't update if a new character is inserted before the update from the previous one. So while "light show" diagnostics is annoying, it really didn't feel like that.

Another issue is that we extract identifiers when sending a FileReadyToParse notification so if we update on TextChangedI, we get completion for the word we are currently typing:

Comments from Reviewable

bstaletic · 2017-05-07T17:31:06Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):

Previously, micbou wrote…

Another issue is that we extract identifiers when sending a FileReadyToParse notification so if we update on TextChangedI, we get completion for the word we are currently typing:

Fair enough. I didn't check for that behaviour. I guess "keep diagnostics until InsertLeave" is a good solution.

Comments from Reviewable

bstaletic · 2017-05-10T18:12:12Z

I'm not too worried about the possible backlog @puremourning mentioned.

What's the policy on using homu with stale LGTM? And when should one use homu's r+ instead of r=name?

Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Valloric · 2017-05-10T19:47:53Z

@bstaletic Since there's no [READY] tag on the PR, don't trigger homu. In general, use your best judgement for when to trigger homu; if you're not sure should you do it, just leave an LGTM and say you didn't trigger homu because you're unsure.

Using r+ vs r=name doesn't really matter, but we usually use r=name with name being the username of the person who sent the first LGTM. But again, we don't really care too much about this.

Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

bstaletic · 2017-05-10T20:34:17Z

@Valloric Alright, thanks. Do we count the stale LGTM's? I know that Reviewable doesn't count them but still reports thtem and that PR's got merged with stale LGTM or two.

micbou · 2017-05-10T21:47:11Z

I am waiting for @puremourning and @vheon to test the PR before marking it as ready. This PR involves code that's not tested and an increase in the Vim version requirement so we need to be careful here.

Do we count the stale LGTM's?

I do if all discussions are resolved (or can be considered as resolved).

Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

puremourning · 2017-05-10T23:43:51Z

I'm conflicted. I do want to test this and give it a solid amount of attention, but at the same time (and perhaps more importantly) I absolutely don't want to be a bottleneck to progress. Unfortunately and most regrettably I'm still restricted, so I can't promise a timeframe, but I will try and look at it this week/end. So I think the best plan is to not wait for me if I can't but that I will do what I can, because this is a really fantastic PR and super valuable.

micbou · 2017-05-13T01:51:30Z

Small update. I removed the unused timer_id parameter from s:HandleFileParseRequest and the Vim version check for adding the c flag to shortmess. We could also remove the _PatchBasedOnVimVersion function but I prefer to do that in another PR as it involves a lot of changes, especially in tests. I don't mind doing it in this PR though if you think we should.

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

Comments from Reviewable

puremourning · 2017-05-13T16:54:55Z

Review status: all files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):
I tried this out. I think there might be a problem with not sending the parse request when initially opening a file?

steps to repro:

put this in err.cpp

int main( int arg, char** arg ) {}

observe error like "redefined parameter 'arg'"
then: vim err.cpp

expect: error is displayed
actual: no error

move the cursor around

expect: error is displayed
actual: no error

:s/main/test
expect: error is displayed
actual: no error

but:

if you enter insert mode, error is displayed
if you :e! error is displayed
if you :vspli another_file then <C-W>P (visit the buffer again), it shows the error

Comments from Reviewable

bstaletic · 2017-05-13T17:55:29Z

I've tried debugging what @puremourning mentioned and here's what I've found.

After opening vim and visiting the first buffer for thte first time s:OnBufferReadyToParse() is not triggered.

Moving the cursor around i normal mode doesn't help as diagnostics are not updated no CursorMove any more.

Substitute command (:s) is behaving strangely. If something else updated and displayed the diagnostics :s will work as expected (i.e. diagnostics will be updated), but if nothing triggered the diagnostics update prior to :s then @puremourning's observations stand.

One more thing I've noticed. For some reason, YCM is issuing 4 exec s:python_command "ycm_state.OnFileReadyToParse()" calls from inside s:OnBufferReadyToParse().

I'll try to debug some more after lunch/dinner.

bstaletic · 2017-05-13T17:58:06Z

Actually, I was wrong about :s. It does update the diagnostics of the file, but doesn't do so from the very start of the editing session.

micbou · 2017-05-13T18:11:45Z

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):

Previously, puremourning (Ben Jackson) wrote…

I tried this out. I think there might be a problem with not sending the parse request when initially opening a file?

steps to repro:

put this in err.cpp
int main( int arg, char** arg ) {}
observe error like "redefined parameter 'arg'"

then: vim err.cpp

expect: error is displayed
actual: no error

move the cursor around

expect: error is displayed
actual: no error

:s/main/test
expect: error is displayed
actual: no error

but:

if you enter insert mode, error is displayed

if you :e! error is displayed

if you :vspli another_file then <C-W>P (visit the buffer again), it shows the error

I've included the changes that parse the initial buffer when the server is ready. This should fix the issue you are experiencing.

Comments from Reviewable

micbou · 2017-05-15T01:17:29Z

Reviewed 2 of 3 files at r7, 1 of 1 files at r8.
Review status: 4 of 5 files reviewed at latest revision, 7 unresolved discussions.

autoload/youcompleteme.vim, line 463 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think i would call this "CheckServerReady" or "PollServerReady" so that it is clear that it is polling for the event, rather than reacting to it.

I picked PollServerReady. I also renamed the s:timers variable to s:pollers as it fits better.

autoload/youcompleteme.vim, line 496 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Ditto here. I'd be tempted to call this CheckForParseResponse or something as it is polling for the response from the server.

I renamed it to PollFileParseReponse to be consistent.

Comments from Reviewable

zzbot · 2017-05-15T19:59:37Z

☔ The latest upstream changes (presumably #2639) made this pull request unmergeable. Please resolve the merge conflicts.

Use the timers feature to display diagnostics asynchronously instead of waiting for an autocommand to trigger. Increase Vim version requirement to 7.4.1578. Drop the CursorHold and CursorHoldI autocommands. Parse buffer on the TextChanged autocommand instead of CursorMoved.

micbou · 2017-05-16T18:33:57Z

Reviewed 1 of 3 files at r7, 1 of 1 files at r9, 1 of 1 files at r10, 3 of 3 files at r11, 4 of 4 files at r12.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

python/ycm/youcompleteme.py, line 768 at r11 (raw file):

      return

    if self.IsServerReady():

Tests added to cover this change.

Comments from Reviewable

vheon · 2017-05-16T19:54:55Z

Reviewed 1 of 4 files at r1, 2 of 3 files at r11, 3 of 4 files at r12.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

Comments from Reviewable

Valloric · 2017-05-17T02:13:24Z

@micbou I see this PR is overflowing with LGTM's, but no READY tag. Feel free to kick off zzbot when you're ready. 👍

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

Comments from Reviewable

Patch EventNotification instead of BaseRequest in event notification tests.

micbou · 2017-05-17T16:48:18Z

Next step is to use timers for completions.

@zzbot r+

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

Comments from Reviewable

zzbot · 2017-05-17T16:48:19Z

📌 Commit 26ac0c8 has been approved by micbou

zzbot · 2017-05-17T16:48:21Z

⌛ Testing commit 26ac0c8 with merge c1c4ebd...

…cbou [READY] Display diagnostics and parse initial buffer asynchronously using timers Currently, we wait for an autocommand to trigger to display diagnostics. This is not ideal as users have to do some actions like moving the cursor or editing the buffer to see them. We can improve that by using [the timers feature introduced in Vim 7.4.1578](vim/vim@975b527) to display diagnostics asynchronously. Here's a demo: ![ycm-diagnostics-async](https://cloud.githubusercontent.com/assets/10026824/25772105/5aac4706-3263-11e7-8304-643a39599d29.gif) By displaying diagnostics asynchronously, we can drop the `CursorHold` and `CursorHoldI` autocommands as well as [the `g:ycm_allow_changing_updatetime` option](https://github.com/Valloric/YouCompleteMe/blob/5a806dcb301beeeac4df9bb84a4f7e4069d40c43/README.md#the-gycm_allow_changing_updatetime-option). In addition, we can now parse the current buffer and display diagnostics on the `TextChanged` event instead of the `CursorMoved` one. Note that we still need to check `b:changed_tick` for the `InsertLeave` event. Given that our policy is to support the latest version of Ubuntu LTS which is 16.04, and that version [bundles Vim 7.4.1689](http://packages.ubuntu.com/xenial/vim), we can increase our Vim version requirement to 7.4.1578. Next steps will be to use timers for parsing the buffer when the server becomes ready and for completions. Fixes #2165.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2636)

zzbot · 2017-05-17T17:18:14Z

☀️ Test successful - status-travis
Approved by: micbou
Pushing c1c4ebd to master...

… r=bstaletic [READY] Completely remove g:ycm_allow_changing_updatetime option Forgot to remove the `g:ycm_allow_changing_updatetime` option from `plugin/youcompleteme.vim` in PR #2636.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2656)

Correct vim 7.4 patch requirement # PR Prelude Thank you for working on YCM! :) **Please complete these steps and check these boxes (by putting an `x` inside the brackets) _before_ filing your PR:** - [x] I have read and understood YCM's [CONTRIBUTING][cont] document. - [x] I have read and understood YCM's [CODE_OF_CONDUCT][code] document. - [ ] I have included tests for the changes in my PR. If not, I have included a rationale for why I haven't. - [x] **I understand my PR may be closed if it becomes obvious I didn't actually perform all of these steps.** # Why this change is necessary and useful [Please explain **in detail** why the changes in this PR are needed.] With #2636 we bumped minimum required vim version to 7.4.1578, but in one place we didn't change the value. Yes, this PR is just one number change, but inconsitency bothers me. [cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md [code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2697)

[READY] Remove obsolete notes in documentation Both notes are obsolete: [libclang correctly returns the type for `auto` since version 3.8](ycm-core/ycmd#353) and diagnostics are automatically refreshed after applying a fix-it since PR #2636.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2782)

micbou force-pushed the display-diagnostics-asynchronously branch from 7e6c065 to e190b67 Compare May 6, 2017 12:05

micbou force-pushed the display-diagnostics-asynchronously branch 2 times, most recently from 251976b to 28a02da Compare May 6, 2017 17:54

micbou force-pushed the display-diagnostics-asynchronously branch 2 times, most recently from 8672253 to 9e51637 Compare May 13, 2017 01:31

micbou changed the title ~~[RFC] Display diagnostics asynchronously using timers~~ [RFC] Display diagnostics and parse initial buffer asynchronously using timers May 13, 2017

micbou force-pushed the display-diagnostics-asynchronously branch 2 times, most recently from 2659eba to 76972db Compare May 15, 2017 01:01

micbou force-pushed the display-diagnostics-asynchronously branch 2 times, most recently from 0ecbefb to ac80483 Compare May 15, 2017 11:46

micbou added 2 commits May 15, 2017 23:57

Parse current buffer when server is ready

3ac2951

micbou force-pushed the display-diagnostics-asynchronously branch from ac80483 to 23a518a Compare May 15, 2017 21:57

Add syntax keywords tests

26ac0c8

Patch EventNotification instead of BaseRequest in event notification tests.

micbou force-pushed the display-diagnostics-asynchronously branch from 23a518a to 26ac0c8 Compare May 17, 2017 11:09

micbou changed the title ~~[RFC] Display diagnostics and parse initial buffer asynchronously using timers~~ [READY] Display diagnostics and parse initial buffer asynchronously using timers May 17, 2017

zzbot merged commit 26ac0c8 into ycm-core:master May 17, 2017

zzbot mentioned this pull request May 17, 2017

[READY] Moved change tracking to python. Per buffer diagnostics #2631

Merged

4 tasks

micbou mentioned this pull request May 19, 2017

[READY] Completely remove g:ycm_allow_changing_updatetime option #2656

Merged

manipnip mentioned this pull request Jun 4, 2017

Error detected while processing function <SNR>107_PollServerReady[ neovim/neovim#6840

Open

micbou deleted the display-diagnostics-asynchronously branch June 19, 2017 01:18

bstaletic mentioned this pull request Jun 28, 2017

Correct vim 7.4 patch requirement #2697

Merged

4 tasks

micbou mentioned this pull request Sep 17, 2017

[READY] Remove obsolete notes in documentation #2782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Display diagnostics and parse initial buffer asynchronously using timers #2636

[READY] Display diagnostics and parse initial buffer asynchronously using timers #2636

micbou commented May 6, 2017 •

edited

Loading

codecov-io commented May 6, 2017 •

edited

Loading

Valloric commented May 6, 2017

micbou commented May 6, 2017

puremourning commented May 6, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

bstaletic commented May 10, 2017

Valloric commented May 10, 2017

bstaletic commented May 10, 2017

micbou commented May 10, 2017

puremourning commented May 10, 2017

micbou commented May 13, 2017 •

edited

Loading

puremourning commented May 13, 2017

bstaletic commented May 13, 2017

bstaletic commented May 13, 2017

micbou commented May 13, 2017

micbou commented May 15, 2017

zzbot commented May 15, 2017

micbou commented May 16, 2017

vheon commented May 16, 2017

Valloric commented May 17, 2017

micbou commented May 17, 2017

zzbot commented May 17, 2017

zzbot commented May 17, 2017

zzbot commented May 17, 2017

[READY] Display diagnostics and parse initial buffer asynchronously using timers #2636

[READY] Display diagnostics and parse initial buffer asynchronously using timers #2636

Conversation

micbou commented May 6, 2017 • edited Loading

codecov-io commented May 6, 2017 • edited Loading

Codecov Report

Valloric commented May 6, 2017

micbou commented May 6, 2017

puremourning commented May 6, 2017

About when we send the file parse request

About the timers mechanism

bstaletic commented May 7, 2017

Requests

Timer mechanism

How I tested

micbou commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

micbou commented May 7, 2017

bstaletic commented May 7, 2017

bstaletic commented May 10, 2017

Valloric commented May 10, 2017

bstaletic commented May 10, 2017

micbou commented May 10, 2017

puremourning commented May 10, 2017

micbou commented May 13, 2017 • edited Loading

puremourning commented May 13, 2017

bstaletic commented May 13, 2017

bstaletic commented May 13, 2017

micbou commented May 13, 2017

micbou commented May 15, 2017

zzbot commented May 15, 2017

micbou commented May 16, 2017

vheon commented May 16, 2017

Valloric commented May 17, 2017

micbou commented May 17, 2017

zzbot commented May 17, 2017

zzbot commented May 17, 2017

zzbot commented May 17, 2017

micbou commented May 6, 2017 •

edited

Loading

codecov-io commented May 6, 2017 •

edited

Loading

micbou commented May 13, 2017 •

edited

Loading