-
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] Stop auto hover timer if buffer changes or cursor moves #4193
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4193 +/- ##
===========================================
+ Coverage 78.22% 89.59% +11.36%
===========================================
Files 29 34 +5
Lines 2760 4440 +1680
===========================================
+ Hits 2159 3978 +1819
+ Misses 601 462 -139 |
9e44f32
to
030c10e
Compare
To be clear, this has never actually worked. For some reason, the auto hover just ends up not working at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
autoload/youcompleteme.vim
line 770 at r2 (raw file):
if g:ycm_auto_hover ==# 'CursorHold' && s:enable_hover call s:StopPoller( s:pollers.command )
This stops any outstanding async command request. I’m not sure that’s the right thing to do because the request might be for something else.
We probably need to do something like set a flag and ignore the response in the hover response handler?maybe set a flag on the poller object and check that in the handler. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
autoload/youcompleteme.vim
line 770 at r2 (raw file):
You are definitely right that this idea is not the right approach. It was very late at night when I had tried to implement a quick and dirty fix for this.
maybe set a flag on the poller object and check that in the handler.
That sounds like it might work, but the trouble is I could not find a way to repro the original issue - change buffer between firing an async request and receiving the response which ends up causing... was it a backtrace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
autoload/youcompleteme.vim
line 770 at r2 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
You are definitely right that this idea is not the right approach. It was very late at night when I had tried to implement a quick and dirty fix for this.
maybe set a flag on the poller object and check that in the handler.
That sounds like it might work, but the trouble is I could not find a way to repro the original issue - change buffer between firing an async request and receiving the response which ends up causing... was it a backtrace?
Yes, it's a traceback because b:ycm_hover
might be undefined if the buffer changes.
Maybe we could just check if b:ycm_hover
still exists when we enter s:ShowHoverResult
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
autoload/youcompleteme.vim
line 770 at r2 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Yes, it's a traceback because
b:ycm_hover
might be undefined if the buffer changes.Maybe we could just check if
b:ycm_hover
still exists when we enters:ShowHoverResult
?
I think that just papers over the crack. The issue is that when the cursor moves, we should not display the hover popup, period. Checking for b:ycm_hover just avoids a crash but doesn't really fix the issue.
I think we should do this:
- in
s:pollers.command
add a key which describes the request. - here, only StopPoller if the request is a 'hover' request.
we could add a fairly generic "CancelCommand( expected_command_type )` too if you think that would be clearer.
030c10e
to
0742d74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
autoload/youcompleteme.vim
line 770 at r2 (raw file):
Previously, puremourning (Ben Jackson) wrote…
I think that just papers over the crack. The issue is that when the cursor moves, we should not display the hover popup, period. Checking for b:ycm_hover just avoids a crash but doesn't really fix the issue.
I think we should do this:
- in
s:pollers.command
add a key which describes the request.- here, only StopPoller if the request is a 'hover' request.
we could add a fairly generic "CancelCommand( expected_command_type )` too if you think that would be clearer.
Done, but I would appreciate some manual testing.
From a brief messing around, seems to work.
0742d74
to
36a1a54
Compare
Rebased as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 2 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
Thanks for sending a PR! |
PR Prelude
Thank you for working on YCM! :)
Please complete these steps and check these boxes (by putting an
x
insidethe brackets) before filing your PR:
rationale for why I haven'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.]
Lack of tests is again just so I can preserve my sanity.
This was just reported on gitter. If the user changes buffer while we are running thte hover time, it causes a traceback.
[cont]: https://github.com/ycm-core/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/ycm-core/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)