[READY] Reset the start column in omnifunc completer #2489

puremourning · 2017-01-03T17:54:56Z

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:

I have read and understood YCM's CONTRIBUTING document.
I have read and understood YCM's CODE_OF_CONDUCT document.
I have included tests for the changes in my PR. If not, I have included a
rationale for why I haven't.
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

A number of issues have been raised over the years where YCM doesn't interact great with the user's omnifunc. This is commonly because we ignore the omnifunc's findstart response, and use our own implementation (base.CompletionStartColumn).

In combination with ycm-core/ycmd#681 this change uses the omnifunc's start column for completions and fixes ycm-core/ycmd#671 and others, such as:

Note: This is just an initial test for sharing the code to gauge reaction. Not fully tested yet.

This change is

puremourning · 2017-01-04T17:43:01Z

Note: requires upping the Vim version to 7.4.775 https://github.com/vim/vim/tree/v7.4.775

I think we should wrap in a has_patch() though that is annoying because it increases our testing burden.

puremourning · 2017-01-04T18:00:36Z

Hmmm, actually this noselect setting has a problem vs what we have now: scope.

It applies to everything, whereas our approach only applies to our triggered completions.

I just noticed this when using it. Darn.

puremourning · 2017-01-04T19:02:21Z

Turns out it isn't needed :) woop \o/

Edit: spoke to soon, it is.

Valloric · 2017-01-05T03:01:00Z

@puremourning Wow you really took that JS-with-single-quotes issue personally. :D

I see you're still going through the discovery process here a bit; my preliminary take is that I'm not at all against the approach this PR is taking. We would though just bump our min Vim requirement instead of using has_patch; latest Ubuntu LTS has 7.4.1689 so we can bump up to that without worries (we always aim for out-of-the-box support for latest Ubuntu LTS).

puremourning · 2017-01-05T14:54:57Z

heh. It was the straw that broke the camel's back. I've said before that i've had about 4 goes at fixing this.

I'm still annoyed that it doesn't seem possible without requiring the user to have new completeopt switches forced on them, which is somewhat of an unreasonable thing for us to do (because it breaks the mental model, etc.).

I'm thinking that we might have to only enable this if the user herself added noinsert and noselect to completeopt. Though I will keep digging. As it stands, I don't think we can just force it on the user.

Example use-case where it breaks (where I noticed it):

I use YCM for general identifier, etc. completions. I don't expect this to select the first item.
I use <CTRL-X><CTRL-L> for whole-line completion. I expect this to select the first item.

The approach included in this patch doesn't work with the feedkeys of <C-X><C-O><C-P> because our completefunc gets called on the <C-O> but then complete_check immediately returns True because of the pending <C-P> in the buffer. This is irksome, and probably the reason we didn't do it this way from the start?

Worth saying, however, that this approach does work for omnifuncs because they are required to return synchronously, so there is hope for omnifuncs, but my javascript completer bug may be harder (or not possible) without blocking the UI thread - which is out of the question.

micbou · 2017-01-05T16:46:01Z

A workaround could be to set the noinsert and noselect options to completeopt just before the feedkeys call then set completeopt back to its original value. This may not be acceptable performance wise but it's worth a shot.

Also, may I suggest to move the omnifunc related changes to another PR so that this PR does not depend on updating the ycmd submodule? This would make it easier to test and review the changes.

puremourning · 2017-01-05T17:00:30Z

I tried setting the completeopt just before feedkeys yesterday and it didn't seem to work. I didn't look into it in detail, but i will try again.

[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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2657)

zzbot · 2017-06-25T16:38:08Z

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

micbou · 2017-06-25T16:49:55Z

Implemented by PR #2657.

micbou · 2017-06-27T13:10:06Z

I missed the part where the start column is reset in the omnifunc completer. This would fix issue #1322 but PR ycm-core/ycmd#681 needs to be merged first. Reopening.

puremourning · 2017-06-27T21:40:32Z

Test case:

create a file Omni.vim with the following contents

function! BenOmniTest( findstart, base )
  if a:findstart
    " Maker it put everything at the beginning of the line
    return 0
  endif

  " Some random results for testing
  return [ 'test', 'testing' ]
endfunction

set omnifunc=BenOmniTest

run vim -S omni.vim test.rb (ruby just because YCM will complete in it and it isn't natively supported)
type this_is_a_test. and see that you get the two suggestions above
select one and see that the result is the whole line is now just the suggestion (it's replaced from column 0)

PASS :)

puremourning · 2017-06-27T21:43:20Z

When ycm-core/ycmd#681 is merged I will update ycmd submodule and this is ready

puremourning · 2017-07-02T22:01:11Z

I've updated ycmd, so this is now ready. Changes in the ycmd bump:

490b3ee2 Auto merge of #681 - puremourning:js-start-col, r=micbou
b5742d31 Allow completers to set the start column where our interpretation doesn't agree with the completion engine
20d18d21 Auto merge of #786 - abutcher-gh:support-embeddable-python, r=micbou
b40b8685 Add tests for GetStandardLibraryIndexInSysPath.
7001830b Drop line continuations in favor of parens.
a39fc07a Fix line length.
3614c9c7 Support using Python embedded distribution.
99f469e1 Auto merge of #784 - micbou:expand-rust-src-path, r=bstaletic
52d2152f Expand Rust source path
973a31fd Auto merge of #783 - micbou:fix-include-pch-flag, r=Valloric
0d0afc77 Fix -include-pch flag tests
9182dedd Add tests for -include-pch flag
6816a341 Auto merge of #781 - micbou:update-gocode, r=vheon
3828d57d Update Gocode to latest version
0a4ee4f4 Auto merge of #780 - micbou:fix-clear-candidates-memory-leak, r=vheon
20585cf4 Fix memory leak when clearing candidates
39ff7076 Auto merge of #779 - puremourning:rhel-fix, r=micbou
1569d6c3 Fix RHEL detection. It actually reports as RedHatEnterpriseServer
ea168d80 Auto merge of #774 - micbou:extend-get-utf8-string, r=Valloric
d12dddce Auto merge of #778 - micbou:fix-unused-parameter, r=bstaletic
602d8519 Auto merge of #777 - micbou:case-swapped-text, r=bstaletic
cd27d766 Fix unused variable in benchmark
8b5bce17 Auto merge of #775 - puremourning:centos-warn, r=micbou
565afa64 Optimize strings comparison when sorting candidates
3b3f3039 Add some advice for CentOS/REHL users
2fac9be3 Extend GetUtf8String
53e4a4e1 Auto merge of #772 - bstaletic:optional_chunk_test, r=Valloric
128966a5 Clang completer - optional completion chunk test
64ddce48 Auto merge of #769 - micbou:google-benchmark, r=bstaletic
dff08841 Add benchmark infrastructure

vheon · 2017-07-02T22:39:13Z

@puremourning looks like the tests are failing :(

puremourning · 2017-07-02T22:49:54Z

err ok, there's a nonzero chance the tests broke with this change....

…er a known failure

puremourning · 2017-07-02T23:56:54Z

OK that should fix the tests. Mostly they were sending the wrong findstart response - it's 0-based.

codecov-io · 2017-07-03T00:07:51Z

Codecov Report

Merging #2489 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2489      +/-   ##
==========================================
+ Coverage    88.9%   88.91%   +<.01%     
==========================================
  Files          20       20              
  Lines        1938     1939       +1     
==========================================
+ Hits         1723     1724       +1     
  Misses        215      215

puremourning · 2017-07-03T00:12:30Z

new demo/testcase above

micbou · 2017-07-03T00:14:49Z

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bstaletic · 2017-07-03T01:44:04Z

@zzbot r=micbou

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

zzbot · 2017-07-03T01:44:05Z

📌 Commit 5c98b69 has been approved by micbou

zzbot · 2017-07-03T01:44:07Z

⌛ Testing commit 5c98b69 with merge 25a2e31...

[READY] Reset the start column in omnifunc completer # 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. - [X] 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 A number of issues have been raised over the years where YCM doesn't interact great with the user's omnifunc. This is commonly because we ignore the omnifunc's `findstart` response, and use our own implementation (`base.CompletionStartColumn`). In combination with ycm-core/ycmd#681 this change uses the omnifunc's start column for completions and fixes ycm-core/ycmd#671 and others, such as: - #1322 probably (not yet tested) - #1957 - #1816 Note: This is just an initial test for sharing the code to gauge reaction. Not fully tested yet. [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/2489)

zzbot · 2017-07-03T01:58:45Z

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 25a2e31 to master...

puremourning mentioned this pull request Jan 4, 2017

[READY] Allow completers to set the start column ycm-core/ycmd#681

Merged

puremourning force-pushed the use-server-start-column branch from 6a535d7 to 8c0a9e1 Compare March 6, 2017 22:33

puremourning mentioned this pull request Apr 10, 2017

Java backend ycm-core/ycmd#45

Closed

micbou mentioned this pull request May 20, 2017

[READY] Rewrite completion system #2657

Merged

micbou closed this Jun 25, 2017

micbou reopened this Jun 27, 2017

micbou changed the title ~~[RFC] Allow the completer (omnifunc or server) to reset the start column~~ [RFC] Reset the start column in omnifunc completer Jun 27, 2017

puremourning force-pushed the use-server-start-column branch from 8c0a9e1 to 3638526 Compare June 27, 2017 21:40

Use the omnifunc-supplied start column

c398450

puremourning force-pushed the use-server-start-column branch from 3638526 to c398450 Compare June 29, 2017 18:16

Update ycmd submodule

e15c492

puremourning changed the title ~~[RFC] Reset the start column in omnifunc completer~~ [READY] Reset the start column in omnifunc completer Jul 2, 2017

Fix tests supplying findstart. It's 0-based. Known failure is no long…

5c98b69

…er a known failure

zzbot merged commit 5c98b69 into ycm-core:master Jul 3, 2017

micbou mentioned this pull request Jul 3, 2017

omnifunc not respecting findstart when word break present #1322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Reset the start column in omnifunc completer #2489

[READY] Reset the start column in omnifunc completer #2489

puremourning commented Jan 3, 2017 •

edited by Valloric

puremourning commented Jan 4, 2017

puremourning commented Jan 4, 2017

puremourning commented Jan 4, 2017 •

edited

Valloric commented Jan 5, 2017

puremourning commented Jan 5, 2017 •

edited

micbou commented Jan 5, 2017

puremourning commented Jan 5, 2017

zzbot commented Jun 25, 2017

micbou commented Jun 25, 2017

micbou commented Jun 27, 2017 •

edited

puremourning commented Jun 27, 2017

puremourning commented Jun 27, 2017

puremourning commented Jul 2, 2017

vheon commented Jul 2, 2017

puremourning commented Jul 2, 2017

puremourning commented Jul 2, 2017

codecov-io commented Jul 3, 2017 •

edited

puremourning commented Jul 3, 2017

micbou commented Jul 3, 2017

bstaletic commented Jul 3, 2017

zzbot commented Jul 3, 2017

zzbot commented Jul 3, 2017

zzbot commented Jul 3, 2017

[READY] Reset the start column in omnifunc completer #2489

[READY] Reset the start column in omnifunc completer #2489

Conversation

puremourning commented Jan 3, 2017 • edited by Valloric

PR Prelude

Why this change is necessary and useful

puremourning commented Jan 4, 2017

puremourning commented Jan 4, 2017

puremourning commented Jan 4, 2017 • edited

Valloric commented Jan 5, 2017

puremourning commented Jan 5, 2017 • edited

micbou commented Jan 5, 2017

puremourning commented Jan 5, 2017

zzbot commented Jun 25, 2017

micbou commented Jun 25, 2017

micbou commented Jun 27, 2017 • edited

puremourning commented Jun 27, 2017

puremourning commented Jun 27, 2017

puremourning commented Jul 2, 2017

vheon commented Jul 2, 2017

puremourning commented Jul 2, 2017

puremourning commented Jul 2, 2017

codecov-io commented Jul 3, 2017 • edited

Codecov Report

puremourning commented Jul 3, 2017

micbou commented Jul 3, 2017

bstaletic commented Jul 3, 2017

zzbot commented Jul 3, 2017

zzbot commented Jul 3, 2017

zzbot commented Jul 3, 2017

puremourning commented Jan 3, 2017 •

edited by Valloric

puremourning commented Jan 4, 2017 •

edited

puremourning commented Jan 5, 2017 •

edited

micbou commented Jun 27, 2017 •

edited

codecov-io commented Jul 3, 2017 •

edited