[READY] When available, use complete() and TextChangedP rather than completefunc

[puremourning]

Recently there has been quite a lot of flicker and noticable redraw
latency, ostensibly caused by our auto-triggering approach. Historically
our approach was as follows:

• set completefunc to our CompleteFunc (see later)
• On InsertCharPre, close the popup menu if it is visible
• On TextChangedI, trigger user-defined completion (to re-show the now
  closed popup menu), request completion async and start polling
• When the completion request is done store the result and, trigger
  user-deinfed completion using ctrl-x, ctrl-u (and ctrl-p to prevent
  selecting the top entry)
• In our CompleteFunc, supply the completions stored completion data.

The reason for the closing the popup menu in InsertCharPre was that
TextChangedI is not fired while the popup menu is visible. So we do
this dance.

However, more recent Vims have provided TextChangedP - basically
TextChangedI but triggered while the pum is visible. In addition, the
following new features are useful: complete() - a function to basically
start insert-mode completion with the supplied data, and
completeopt=noselect, which prevents automatically selecting the first
completion. Nice.

But there is one snag. TextChangedP is also triggers when a completion
is selected (e.g. using Ctrl-n or Tab). This is irksome, because it
means that we end up filtering the completions based on the
newly-inserted text. This is not what we want - the user didn't type
that text (they selected it) so we should continue to show all of the
most recent matches. There's no good way to solve this, so we solve it
by recording whether or not the last imput came from a user or not. We
do this using InsertCharPre (setting it to true) and CompleteDone
(setting it to false). The former is when a user types some characters
and the latter when an entry in the menu is selected.

This isn't perfect. As previously mentioned, InsertCharPre is not
triggered when the completion menu is visible, so there is a scenario
which breaks: User tabs to an entry, then adds some more characters.
However, this seems niche enough that the benefits of the new system in
terms of redraw and flicker outweigh this (essentially unfixable) bug.

So the new approach is:

• In InsertCharPre, set the flag to true
• In TextChangedI/TextChangedP, if it's TextChangedP and the flag is
  false, ignore the event.
• Otherwise, in TextChangedI/TextChangedP, call complete() with the
  latest completion response, and fire off another one, staring the poll
  .timer
• When the response is received, just call complete() with the new data.

The only remaining wrinkle is the omnicompleter. complete() cannot be
called from the TextChangedI/TextChangedP autocommand handlers
synchronously (for some reason) - it causes vim to get messed up. This
is reasonable, as complete() is supposed to be used async. So we
workaround that by polling for completion in a 0ms timer, rather than in
athe autocommand handler.

Finally, this commit adds a bunch more tests for completion that test
both the new and the old mechanism. We keep the old mechanism because
the new stuff is too new for our "supported" Vim version.

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

[Please explain in detail why the changes in this PR are needed.]

---

This change is 

[codecov]

Codecov Report

Merging #3645 into master will increase coverage by 0.88%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #3645      +/-   ##
==========================================
+ Coverage   89.37%   90.26%   +0.88%     
==========================================
  Files          22       25       +3     
  Lines        2164     2988     +824     
==========================================
+ Hits         1934     2697     +763     
- Misses        230      291      +61     

[puremourning]

Going to fix coverage. I think the problem is that we only ever report whatever the last script to run was.

[puremourning]

coverage is fixed

[puremourning]

Tested quickly and it works in neovim.

Reviewable status: 0 of 2 LGTMs obtained

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained

---

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

" By default (if possible) use the new complete()/TextChangedP API
let s:completion_api = get( g:,
                          \ 'ycm_use_completion_api',

this is (deliberately) undocumented:

1. it's used in the tests to run in both modes
2. It's a get-out-of-jail free card. If there's a serious bug, we can revert the behaviour by setting this flag.

---

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

endfunction

put this line back

---

test/.vimspector.json, line 8 at r1 (raw file):

        "command": [
          "node",
          "/Users/ben/Development/vim-debug-adapter/dist/index.js"

errrrrrr... yeah don't submit this file yet.

---

test/completion.common.vim, line 1 at r1 (raw file):

function! s:CheckCompletionItems( expected_props, ... )

This "completion.common.vim' is tests that are run against both the "new" and the "old" completion APIs.

---

test/tags, line 1 at r1 (raw file):

!_TAG_FILE_FORMAT	2	/extended format; --format=1 will not append ;" to lines/

errrr... don't submit this either!

---

test/vimrc, line 6 at r1 (raw file):

set lines=30
set columns=80
set shortmess+=c

this is just to stfu the annotating warnings when completing.

[bstaletic]

This isn't perfect. As previously mentioned, InsertCharPre is not triggered when the completion menu is visible, so there is a scenario which breaks: User tabs to an entry, then adds some more characters. However, this seems niche enough that the benefits of the new system in terms of redraw and flicker outweigh this (essentially unfixable) bug.

This will be a problem for C projects. Since there are no namespaces, tons of identifiers start with the same handful of letters. In C I often hit this case.

1. Type TIM_I
2. <Tab> until TIM_IT_CC1
3. <BS>2

The embedded (freestanding) projects are especially hit by this, because the MCU support libraries, like libopencm3, are full of macros. To save you the trouble of setting something like that up, think of <string.h> and all the str* functions.

However, it might be good enough if we special case the handling of <BS>.

These are my first thoughts and I'll give this a good test tomorrow.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

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

  set completeopt+=menuone
  if s:completion_api == s:COMPLETION_COMPLETE
    set completeopt+=noselect

I tried noselect in one of my PRs and then I decided I don't like it. It will affect <C-x><C-n> behaviour, which arguably should not be touched by YCM. I sometimes like to tell YCM to get out of my way and use the regular <C-x> mode. However, with manual completion triggering, I expect the first entry to be selected. It saves a key stroke, since I'm already at the "please insert that thing for me" stage.

[puremourning]

I actually tested it (after writing the comment) and it works correctly. Confession - not sure why.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

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

Previously, bstaletic (Boris Staletic) wrote…

I tried noselect in one of my PRs and then I decided I don't like it. It will affect <C-x><C-n> behaviour, which arguably should not be touched by YCM. I sometimes like to tell YCM to get out of my way and use the regular <C-x> mode. However, with manual completion triggering, I expect the first entry to be selected. It saves a key stroke, since I'm already at the "please insert that thing for me" stage.

Yeah I was thinking about this. Maybe I can just temp set it before calling complete()

[bstaletic]

When was complete() introduced? If 8.1.2269 is new enough, I'd rather we wait for Ubuntu 20.04 to come out and just do the switch. There's almost a month until the ubuntu release, so there's time for testing.

Reviewed 6 of 10 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

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

"    appreciably, but is only supported on more recent vims.
let s:COMPLETION_COMPLETEFUNC = 0
let s:COMPLETION_COMPLETE = 1

These two variables are named too similarly, in my opinion. How about renaming COMPLETION_COMPLETEFUNC to COMPLETION_FEEDKEYS?

---

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

Previously, puremourning (Ben Jackson) wrote…

this is (deliberately) undocumented:

1. it's used in the tests to run in both modes
2. It's a get-out-of-jail free card. If there's a serious bug, we can revert the behaviour by setting this flag.

Please remember to mark informational comments as such, otherwise Reviewable will treat the discussion as "unresolved", when there's nothing to resolve.

---

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

" Fall back to using completefunc if the TextChangedP API isn't available
if !exists( '##TextChangedP' ) ||

I think we can just assume TextChangedP exists.

---

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

    " the completion menu is open. We handle this by closing the completion menu
    " on the keys that delete a character in insert mode.
    for key in [ "<BS>", "<C-h>" ]

Since the new completion mechanism doesn't need these mappings, it should also resolve #2696

---

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

just temp set it before calling complete()

That's what I thought too. I hope that works.

---

test/completion.common.vim, line 16 at r1 (raw file):

        \ abbrs,
        \ 'not matched: '
        \ .. string( a:expected_props )

Double dot?

---

test/vimrc, line 6 at r1 (raw file):

annotating warnings

I'm guessing s/annotating/annoying/. Also "warnings" sounds more severe than "messages" as vim help calls them. I'm not missing anything, right?

---

test/lib/run_test.vim, line 296 at r1 (raw file):

                \ 's' )

  if exists( '$COVERAGE' ) && pyxeval( '_cov is not None' )

You can use py3eval too, but no big deal.

When is _cov None?

---

test/lib/run_test.vim, line 349 at r1 (raw file):

    import coverage
  except ImportError:
    return None

Huh? Why would this fail? Why are we "swallowing" the exception?

[puremourning]

Actually looks like f_complete() has been around since vim 7.4, as has noselect.

It's TextChangedP that we need to wait for. That was added in vim/vim@5a09343

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

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

Previously, bstaletic (Boris Staletic) wrote…

These two variables are named too similarly, in my opinion. How about renaming COMPLETION_COMPLETEFUNC to COMPLETION_FEEDKEYS?

I'll think of something

---

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

Previously, bstaletic (Boris Staletic) wrote…

Please remember to mark informational comments as such, otherwise Reviewable will treat the discussion as "unresolved", when there's nothing to resolve.

done.

---

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

Previously, bstaletic (Boris Staletic) wrote…

I think we can just assume TextChangedP exists.

In my ubuntu 18.04 lts VM's vim, TextChangedP does not exist

VIM - Vi IMproved 8.0 (2016 Sep 12, compiled Mar 18 2020 18:29:15)
Included patches: 1-1453

---

test/completion.common.vim, line 16 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Double dot?

it's the "new" string concat operator. the version of vim we run in the tests supports this

---

test/vimrc, line 6 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

annotating warnings

I'm guessing s/annotating/annoying/. Also "warnings" sounds more severe than "messages" as vim help calls them. I'm not missing anything, right?

no you're right on both counts.

---

test/lib/run_test.vim, line 296 at r1 (raw file):

When is _cov None?

When coverage can't be imported (such as when you're me and you're using a virtualenv and trying to test vim-only coverage)

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Huh? Why would this fail? Why are we "swallowing" the exception?

Because this import fails in a virtualenv, and I want to be able to test the vim coverage.

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

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

Previously, puremourning (Ben Jackson) wrote…

I'll think of something

went with COMPLETION_TEXTCHANGEDP and COMPLETION_COMPLETEFUNC

---

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

Previously, bstaletic (Boris Staletic) wrote…

Since the new completion mechanism doesn't need these mappings, it should also resolve #2696

Good shout. Asked them to test it. I think you're right that it should fix any such issue, but testing that is not top priority for me on this PR :)

---

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

Previously, bstaletic (Boris Staletic) wrote…

just temp set it before calling complete()

That's what I thought too. I hope that works.

looks like it works :)

    let old_completeopt = &completeopt
    set completeopt+=noselect
    call complete( s:completion.completion_start_column,
                 \ s:completion.completions )
    let &completeopt = old_completeopt

---

test/completion.common.vim, line 16 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

it's the "new" string concat operator. the version of vim we run in the tests supports this

Done.

[bstaletic]

Reviewed 3 of 10 files at r1, 3 of 3 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

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

Previously, puremourning (Ben Jackson) wrote…

Good shout. Asked them to test it. I think you're right that it should fix any such issue, but testing that is not top priority for me on this PR :)

Absolutely agreed.

---

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

Previously, puremourning (Ben Jackson) wrote…

looks like it works :)

    let old_completeopt = &completeopt
    set completeopt+=noselect
    call complete( s:completion.completion_start_column,
                 \ s:completion.completions )
    let &completeopt = old_completeopt

Woohoo!

---

autoload/youcompleteme.vim, line 749 at r2 (raw file):

  if a:popup_is_visible && !s:last_char_inserted_by_user
    " If the last "input" wasn't from a user typeing (i.e. didn't come from

s/typeing/typing

---

autoload/youcompleteme.vim, line 778 at r2 (raw file):

    " flicker and to ensure that tabbing throught the list does not continually
    " filter the list to the selected element. (Though i confress, i haven't
    " worked out why yet)

The flicker is understandable. Before this PR, we used to close the pum, delete the character and then reopen as fast as we can, to minimize the wait time between two time points of pum getting drawn.

---

autoload/youcompleteme.vim, line 889 at r2 (raw file):

  " We can't call complete() syncrhounsouly. this happens with the omnicompleter
  " Why is typing in comments slow ?

I'm sure these two lines make total sense to you right now, but they are pretty unclear.

---

autoload/youcompleteme.vim, line 969 at r2 (raw file):

  endif

  call s:StopPoller( s:pollers.signature_help )

Why are you stopping signature polling here?

---

test/completion.test.vim, line 18 at r2 (raw file):

endfunction

exe 'source' expand( "<sfile>:p:h" ) .. '/completion.common.vim'

Doesn't this need the .. operator between 'source' and expand?

---

test/completion.test.vim, line 22 at r2 (raw file):

function! Test_Using_New_API()
  let debug_info = split( execute( 'YcmDebugInfo' ), "\n" )
  

Trailing whitespace.

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Because this import fails in a virtualenv, and I want to be able to test the vim coverage.

Wouldn't pip instll coverage solve the import problem? I'm probably missing something obvious.

[puremourning]

Reviewed 6 of 10 files at r1, 1 of 3 files at r2, 5 of 5 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

autoload/youcompleteme.vim, line 749 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

s/typeing/typing

Done.

---

autoload/youcompleteme.vim, line 778 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

The flicker is understandable. Before this PR, we used to close the pum, delete the character and then reopen as fast as we can, to minimize the wait time between two time points of pum getting drawn.

yeah, though it's still required with this new API, which surprised me. I've worked out why - it's because vim might close the pum if it doens't think the typed string matches anything in the list, so we have to disabuse it of that notion e.g. when we're fuzzy matching.c

---

autoload/youcompleteme.vim, line 889 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm sure these two lines make total sense to you right now, but they are pretty unclear.

Done.

---

autoload/youcompleteme.vim, line 969 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why are you stopping signature polling here?

Previously we were stopping it in InsertCharPre then retriggering it in TextCHnagedI. Now we don't bother cancelling it in InsertCharPre and just cancel it whenever we re-request it (i.e. from TextChangedI/P - it's just simpler (we actually do the same for the completion request)

---

test/completion.test.vim, line 18 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Doesn't this need the .. operator between 'source' and expand?

it does not. execute takes a list of {expr1} arguments.

---

test/completion.test.vim, line 22 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Trailing whitespace.

done

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Wouldn't pip instll coverage solve the import problem? I'm probably missing something obvious.

it doesn't appear to. something about the way we run the tests and/or the way the embedded python in vim works seems to prevent it from loading from the virtual env.

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

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

    let old_completeopt = &completeopt
    set completeopt+=noselect
    call complete( s:completion.completion_start_column,

we need to be more careful here that we only call this strictly in insert mode.

There may be times that we leave insert mode temporarily (such as Ctrl-r and timers) but don't get InsertEnter/InsertLeave events. Otherwise we get nasty traceback E785 complete may only be used in insert mode.

[puremourning]

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

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

Previously, puremourning (Ben Jackson) wrote…

we need to be more careful here that we only call this strictly in insert mode.

There may be times that we leave insert mode temporarily (such as Ctrl-r and timers) but don't get InsertEnter/InsertLeave events. Otherwise we get nasty traceback E785 complete may only be used in insert mode.

Done

[bstaletic]

Reviewed 1 of 5 files at r3, 2 of 2 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

autoload/youcompleteme.vim, line 918 at r4 (raw file):

    " We can't call complete() syncrhounsouly. this happens with the omnicompleter
    " Why is typing in comments slow ?

Confusing comments are repeated here.

---

autoload/youcompleteme.vim, line 1004 at r4 (raw file):

    " mode _now_ do nothing (FIXME: or should we queue a timer ?)
    if count( [ 'i', 'R' ], mode() ) == 0
      return

Is this enough? Can a timer fire after this line, but before complete() is called? If it can, we should make this check as late as possible.

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

it doesn't appear to. something about the way we run the tests and/or the way the embedded python in vim works seems to prevent it from loading from the virtual env.

Can you tell me how to repro? I'd like to at least understand when is coverage not available.

[puremourning]

Reviewed 2 of 2 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

autoload/youcompleteme.vim, line 918 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Confusing comments are repeated here.

Done.

---

autoload/youcompleteme.vim, line 1004 at r4 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is this enough? Can a timer fire after this line, but before complete() is called? If it can, we should make this check as late as possible.

Timer can't fire between here and the end of this function. That would be madness. Timers only fire when vim is waiting for input, sleeping, etc.

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Can you tell me how to repro? I'd like to at least understand when is coverage not available.

here's what I did just now:

pyenv virtualenv -p python3 minimal
pyenv activate minimal
grep coverage python/ycm/test_requirements.txt
pip install 'coverage<5.0'
vim --clean --noplugin
:py3 import coverage

Get ModuleNotFoundError: No module named 'coverage'

[puremourning]

Reviewed 1 of 1 files at r5.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

[bstaletic]

This looks good, but I want to actually try it out in vim before giving it a green light.

Reviewed 4 of 5 files at r3, 1 of 1 files at r5.
Reviewable status: 0 of 2 LGTMs obtained

---

autoload/youcompleteme.vim, line 1004 at r4 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Timer can't fire between here and the end of this function. That would be madness. Timers only fire when vim is waiting for input, sleeping, etc.

I was hoping that's the case.

---

test/lib/run_test.vim, line 349 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

here's what I did just now:

pyenv virtualenv -p python3 minimal
pyenv activate minimal
grep coverage python/ycm/test_requirements.txt
pip install 'coverage<5.0'
vim --clean --noplugin
:py3 import coverage

Get ModuleNotFoundError: No module named 'coverage'

I can't repro. I'm not going to block this any longer though.

[puremourning]

I can't repro

Oh really. Hmm. I'll have to look at my environment then. I seem to recall spending ages looking at this before.

[puremourning]

I think the reason that it doesn't work for me is that I use statically-linked python which presumably doesn't include library-relative paths like the dynamic one.

[bstaletic]

I've made a quick test and the part that worried me works. Like I said, .

[puremourning]

Reviewable status: complete! 2 of 2 LGTMs obtained

[mergify]

Thanks for sending a PR!

[mergify]

Thanks for sending a PR!