[READY]: Signature help

[puremourning]

PR Prelude

• I have read and understood YCM's [CONTRIBUTING][cont] document.
• 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.
• 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

Closes #234

This change is WIP, prototype to support argument hints using popup windows.

Nothing is enabled if the required functions don't exist, as most of them are extremely new.

Fundamentally, the code is in 'signature_help.py` but breaks down into:

• Maintaining some state about the current signature help ACIVE/INACTIEV and the anchor point (see below). this is required so that we can tell ycmd to keep checking for signature help until there is no more
• Popup window buffer creation. This is just using the signature labels and text properties to highlight current argument and current signature.
• Popup window placement. this is the fiddly part.

For the placement, I decided I didn't want to follow the cursor like VSCode does. I find this annoying and it means that if there are many arguments, as you approach the right hand side of the screen, the arguments you want start to get truncated or wrapped. Instead we try to anchor the menu to the position where it first appeared. This is done by some offsetting from the cursor position, as finding the actual screen row/col of a particular mark is not straightforward.

The placement goes a bit gnarly towards the side of the screen, so probably needs offsetting left to make it fit. It may be better to implement this in vim code rather than here, as the PUM does exactly the same thing.

Please see ycm-core/ycmd#1255 for demos.

---

This change is 

[bstaletic]

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

---

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

    " Immediately call previous completion to avoid flickers.
    call s:Complete()
    call s:RequestCompletion()

Why the name change? Same question for InvokeSemanticCompletion/RequestSemanticCompletion.

---

python/ycm/client/base_request.py, line 81 at r1 (raw file):

        result = _JsonFromFuture( future )
        _logger.debug( 'RX: %s', result )
        return result

Is this log line really necessary? Same for the changes below.

---

third_party/ycmd, line 1 at r1 (raw file):

Subproject commit ecbf296746fe80c96e9d0d86f088e98669292df7

I'm guessing this is now pointing to a commit in your fork.

[puremourning]

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

---

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

Previously, bstaletic (Boris Staletic) wrote…

Why the name change? Same question for InvokeSemanticCompletion/RequestSemanticCompletion.

The function was originally named InvokeCopletion because that's what it did: called feedkeys( ^x^u^p ). Now what it does is it just sends the completion request and starts the timer.

I found the use of "invoke" to be arguably misleading, as it's s:Complete that actually invokes vim's completion system.

FWIW this is in its own commit so can take it or leave it, but I personally find it more intuitive.

---

python/ycm/client/base_request.py, line 81 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is this log line really necessary? Same for the changes below.

I have often found it frustrating to debug the YCM client <-> ycmd communication. I usually just break out Wireshark and snoop the loopback interface. I added this protocol logging for debugging and testing in debug mode. I have already found it useful when testing this change and some others. Most users don't use debug logging so I'm not too worried about the impact. Would also be useful for bug reports where users are required to use debug logging, we can see the actual client/server comms in the same way that we have it for the ycmd->completer server logging.

---

third_party/ycmd, line 1 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I'm guessing this is now pointing to a commit in your fork.

yeah will tidy that up later.

[puremourning]

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

---

python/ycm/client/base_request.py, line 81 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I have often found it frustrating to debug the YCM client <-> ycmd communication. I usually just break out Wireshark and snoop the loopback interface. I added this protocol logging for debugging and testing in debug mode. I have already found it useful when testing this change and some others. Most users don't use debug logging so I'm not too worried about the impact. Would also be useful for bug reports where users are required to use debug logging, we can see the actual client/server comms in the same way that we have it for the ycmd->completer server logging.

Again, this should really be a separate PR like the rename one. Sorry I bundled the whole branch together.

[bstaletic]

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

---

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

Previously, puremourning (Ben Jackson) wrote…

The function was originally named InvokeCopletion because that's what it did: called feedkeys( ^x^u^p ). Now what it does is it just sends the completion request and starts the timer.

I found the use of "invoke" to be arguably misleading, as it's s:Complete that actually invokes vim's completion system.

FWIW this is in its own commit so can take it or leave it, but I personally find it more intuitive.

It's fine. Leave it.

---

python/ycm/client/base_request.py, line 81 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Again, this should really be a separate PR like the rename one. Sorry I bundled the whole branch together.

Alright, moving on.

---

third_party/ycmd, line 1 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

yeah will tidy that up later.

Sure, just don't forget about it.

[bstaletic]

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

---

python/ycm/signature_help.py, line 48 at r1 (raw file):

def SetUpPopupWindow( popup_win_id, buf_lines ):
  # FIXME: For some reason the below does not seem to work.

Maybe you've hit a vim bug. One of the latest commits solves 'wrap' support for popup windows.

vim/vim@402502d

---

python/ycm/signature_help.py, line 149 at r1 (raw file):

  # the cursor position.
  cur_pos = vimsupport.CurrentLineAndColumn()
  delta = tuple( map( sub, cur_pos, state.anchor ) ) # subtract each elem

Possibly a stupid question, but where's sub defined?

[puremourning]

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

---

python/ycm/signature_help.py, line 48 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Maybe you've hit a vim bug. One of the latest commits solves 'wrap' support for popup windows.

vim/vim@402502d

Yeah i have changed to use the popup dict option wrap. I still not sure why setting from python layer doesn't work for the other option.

---

python/ycm/signature_help.py, line 149 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Possibly a stupid question, but where's sub defined?

from operator import sub at the top

[bstaletic]

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

---

python/ycm/signature_help.py, line 48 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Yeah i have changed to use the popup dict option wrap. I still not sure why setting from python layer doesn't work for the other option.

Does it work if you use vim newer than 8.1.1430?

---

python/ycm/signature_help.py, line 149 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

from operator import sub at the top

Like I said, stupid question.

[puremourning]

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

---

python/ycm/signature_help.py, line 48 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Does it work if you use vim newer than 8.1.1430?

I'm using the latest commit as of this morning.

[bstaletic]

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

---

python/ycm/signature_help.py, line 48 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I'm using the latest commit as of this morning.

Alright, nevermind.

[bstaletic]

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

---

python/ycm/signature_help.py, line 52 at r3 (raw file):

  # win = vim.windows[ GetIntValue( 'win_id2win( {} )'.format(
  #   popup_win_id ) ) ]
  # win.options[ 'signcolumn' ] = 'no'

This works for me in vim 8.1.1467.

[bstaletic]

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

---

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

  if force_parsing || s:Pyeval( "ycm_state.NeedsReparse()" )
    " We switched buffers or somethuing, so claer.
    " FIXME: sig hekp should be buffer local?

Typo: s/hekp/help

What happens without this line added?

[puremourning]

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

---

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

Previously, bstaletic (Boris Staletic) wrote…

Typo: s/hekp/help

What happens without this line added?

It's quite funny. If you have 2 buffers open, one which support sig help and one which doesn't, then when you get some signatures in buffer 1, then switch to buffer 2 without exiting insert mode. every key you press in buffer 2 has the sig help attached from buffer 1, because it never refreshes.

A better solution actually might be to make the OmniCompleter return no signatures or something, but this seems the right thing to do in any case.

[bstaletic]

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

---

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

Previously, puremourning (Ben Jackson) wrote…

It's quite funny. If you have 2 buffers open, one which support sig help and one which doesn't, then when you get some signatures in buffer 1, then switch to buffer 2 without exiting insert mode. every key you press in buffer 2 has the sig help attached from buffer 1, because it never refreshes.

A better solution actually might be to make the OmniCompleter return no signatures or something, but this seems the right thing to do in any case.

I'm fine with this as is, again, just being curious why is it needed.

[bstaletic]

Reviewed 4 of 4 files at r5, 2 of 2 files at r6, 3 of 3 files at r7.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

plugin/youcompleteme.vim, line 298 at r7 (raw file):

" Populate any other (undocumented) options set in the ycmd
" default_settings.json. This ensures that any part of ycm that uses ycmd code
" will have the default set. I'm looking at you, Omni-completer.

Why don't we decouple OmniCompleter like in this commit?

---

python/ycm/client/signature_help_request.py, line 29 at r7 (raw file):

from ycm.client.base_request import ( BaseRequest, DisplayServerException,
                                      MakeServerException )
from ycm import vimsupport

vimsupport and ToUnicode aren't used.

[bstaletic]

Reviewed 2 of 3 files at r8, 3 of 4 files at r9.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

.gitmodules, line 27 at r9 (raw file):

[submodule "third_party/vim-themis"]
	path = third_party/vim-themis
	url = https://github.com/thinca/vim-themis

Woah! A testing framework!

Covimerage can generate coverage info that is understood by codecov.

[puremourning]

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

---

.gitmodules, line 27 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Woah! A testing framework!

Covimerage can generate coverage info that is understood by codecov.

Oops. How did that get in there.

[codecov]

Codecov Report

Merging #3412 into master will decrease coverage by 3.84%.
The diff coverage is 38.41%.

@@            Coverage Diff             @@
##           master    #3412      +/-   ##
==========================================
- Coverage   94.56%   90.72%   -3.85%     
==========================================
  Files          20       22       +2     
  Lines        2062     2210     +148     
==========================================
+ Hits         1950     2005      +55     
- Misses        112      205      +93

[bstaletic]

Reviewed 1 of 3 files at r10, 9 of 16 files at r11, 2 of 2 files at r12.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

autoload/youcompleteme.vim, line 705 at r12 (raw file):

  " TODO: Do we really need this here?
  call timer_stop( s:pollers.signature_help.id )

If I remember correctly, we were defensively stopping timers because vim used to lose command line contents whenever a timer would trigger.

---

azure/linux/build_vim.sh, line 16 at r12 (raw file):

  ./configure --with-features=huge \
              --enable-terminal \
              --enable-multibyte \

At least --enable-terminal is the default in a huge build. Multibyte might be as well.

[puremourning]

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

---

autoload/youcompleteme.vim, line 705 at r12 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

If I remember correctly, we were defensively stopping timers because vim used to lose command line contents whenever a timer would trigger.

oh could be.

---

azure/linux/build_vim.sh, line 16 at r12 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

At least --enable-terminal is the default in a huge build. Multibyte might be as well.

I'll check. the huge feature set changes with time, and I'm used to typing these options.

---

plugin/youcompleteme.vim, line 298 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Why don't we decouple OmniCompleter like in this commit?

I think we could, as you know, but that looks like a more substantial change than I was planning here. TBH this change probably doesn't belong here.

[puremourning]

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

a discussion (no related file):
Need to find a way to get the codecov data for the python layer from the Vim tests.

Would be nice to submit some viml coverage too.

---

[bstaletic]

Reviewed 4 of 12 files at r13, 4 of 7 files at r14.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

a discussion (no related file):

Would be nice to submit some viml coverage too.

Can we make covimerage work?

---

---

autoload/youcompleteme.vim, line 705 at r12 (raw file):

Previously, puremourning (Ben Jackson) wrote…

oh could be.

It definitely were the case for quite some time. Perhaps we should wait until the next vim version requirement bump before dropping these.

---

azure/linux/build_vim.sh, line 16 at r12 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I'll check. the huge feature set changes with time, and I'm used to typing these options.

It's not really a big deal, it's just redundant.

---

plugin/youcompleteme.vim, line 298 at r7 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think we could, as you know, but that looks like a more substantial change than I was planning here. TBH this change probably doesn't belong here.

I wasn't implying that we should decouple the omnicompleter in this PR, rather make a separate PR for the omnicompleter and then stop worrying about it here.

---

test/lib/third_party/coveragepy, line 1 at r14 (raw file):

Subproject commit 6831811a12c97724d09d5affe46c7dfd444eb222

Is this supposed to be here? Coveragepy is also registered as a submodule and, unless I'm mistaken, is in the python/test_requirements.txt.

[puremourning]

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

---

test/lib/run_test.vim, line 33 at r23 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

It looks very weird to me.

Done.

Though in thinking about it, it might be for the same reason we set rtp in the first place - to avoid any nonsense being loaded.

---

test/lib/run_test.vim, line 40 at r23 (raw file):

Previously, puremourning (Ben Jackson) wrote…

good question. again, this is just copy/paste from vim's own test framework. it doesn't seem to cause any problems. nviminfo means "name the viminfo file 'vnminfo'"

Done.

---

test/lib/run_test.vim, line 50 at r23 (raw file):

Previously, puremourning (Ben Jackson) wrote…

sure. again, copy/paste from vim

Done.

---

test/lib/autoload/youcompleteme/test/setup.vim, line 15 at r23 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Couldn't runtimepath be appended with the parent directory of plugin here? That would make vim find this plugin and source these plugin/**/*.vim, but it would also find after, ftplugin, autoload etc. Does runtimepath here make more sense?

this line is actually trying to forcefully re-iniitalise YCM from scratch. YCM is already in the rtp, we just want to sort of do a kind of clean-slate at this point.

---

test/lib/plugin/screendump.vim, line 95 at r23 (raw file):

Previously, puremourning (Ben Jackson) wrote…

we don't actually use this (yet), but it is used extensively in the vim regression tests.

Done.

---

test/lib/plugin/shared.vim, line 50 at r23 (raw file):

Previously, puremourning (Ben Jackson) wrote…

this is also pretty much spam from vim that we don't use.

Done.

---

test/lib/plugin/shared.vim, line 258 at r23 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

It relies on the current 'ignorecase'. Is that option set anywhere in the test setup?

removed

[bstaletic]

Reviewed 3 of 3 files at r27, 1 of 1 files at r28, 2 of 3 files at r29, 3 of 3 files at r30.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

python/ycm/signature_help.py, line 27 at r30 (raw file):

import vim
import json
import logging

logging isn't used any more, so now flake8 is complaining.

[puremourning]

I think this is ready to merge.

There's probably tons more stuff to do, and I suspect taht we will have a firehose of complaints, but maybe time to rip the band-aid off?

Reviewed 1 of 3 files at r7, 24 of 36 files at r23, 4 of 7 files at r25, 3 of 5 files at r26, 2 of 3 files at r27, 1 of 1 files at r28, 3 of 3 files at r29, 1 of 2 files at r31.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

---

autoload/youcompleteme.vim, line 705 at r12 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

It definitely were the case for quite some time. Perhaps we should wait until the next vim version requirement bump before dropping these.

I think we can close this off ?

---

plugin/youcompleteme.vim, line 298 at r7 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

I wasn't implying that we should decouple the omnicompleter in this PR, rather make a separate PR for the omnicompleter and then stop worrying about it here.

Can we resolve this disussion?

---

python/ycm/signature_help.py, line 27 at r30 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

logging isn't used any more, so now flake8 is complaining.

Done.

[bstaletic]

Nah, we'll be fine. I'm guessing the coverage dropped because of vimscript coverage, that's fine, we'll work on it in time.

Reviewed 1 of 2 files at r31.
Reviewable status: 1 of 2 LGTMs obtained

[puremourning]

OK let's go.

[mergify]

Thanks for sending a PR!

[nineKnight]

Hello guys. What a nice job!
However, I found something inconvenient.

memset(..., ..., ...);

If the passing argument is not void * type, YCM with clangd cannot prompt any more. It could be optimized like this:

memset((void *)..., ..., ...);

It's what I say inconvenient. Do you think so?

[puremourning]

we’re just showing what clangd suggests. Maybe it could decay then pointer type? I don’t know that would be a q for clangd guys.

[nineKnight]

Understand. Thanks for the reply.

[Amudtogal]

Hi,
thanks for your ongoing work!
It might be a stupid question, but I couldn't find the installation instructions for the signature help...
I am using Nvim for C++ and Python, and YCM works just fine. However, I have no idea how to trigger the signature help popup: When I accept a completion (e.g. atan2) it completes the text, but not the parentheses. As soon as I continue typing (opening parentheses) the completion disappears...
Am I missing something here, or is Nvim not supported?
Thanks for any help!

[bstaletic]

I am using Nvim

Neovim implements a completely different API for popup windows, which are needed for this PR. Only vim's way of spawning popups is supported.

When I accept a completion (e.g. atan2) it completes the text, but not the parentheses.

That's expected behaviour.

As soon as I continue typing (opening parentheses) the completion disappears...

Again, expected, but were you using vim, a different popup would have appeared with signature help.

[Amudtogal]

Ahhh, this explains everything 😄
Thanks for the fast reply!

[puremourning]

Yeah that's a good point. For the avoidance of doubt: neovim is not supported for signature help in YCM (neovim is not officially supported by YCM team at all).

We have no plans to support the equivalent APIs in neovim, but if the delta is small, we would consider community contributions, provided reliable regression tests are added.

[puremourning]

I did a quick check and asked the neovim guys. The neovim api :help api-floats is much lower level than the vim one :help popup-window and therefore will require potentially significant work on our side to support both. I asked about pollyfilling the api and there was a vague suggestion that they might pollyfill the vim APIs with the underlying neovim one. When that happens, signature help should just work.

[Amudtogal]

Thanks for following up!
If I only knew a bit more about the whole APIs...