[RFC] Improve call signatures setup

[blueyed]

Came up with it while looking at #651.

TODO:

• [x] with mode 1, it will trigger indenting when adding : after %i: - likely due to the concealed text being used (addressed in https://github.com/Vimjas/vim-python-pep8-indent/pull/98):

               (value, ..., sep, end, file, flush)                                
  print('sig %i: %r' % (signatures,))                                             
  

• [ ] doc

[blueyed]

Well, this got huge

• undo and resetting should be working properly/better.
• timers support
• call signatures in normal mode Via new/experimental setting g:jedi#show_call_signatures_modes (not yet documented). To configure signatures in normal and insert mode, use 'ni'. The default is 'i' (insert mode only). It can be set through jedi#configure_call_signatures(), e.g. to have them in normal mode only: :call jedi#configure_call_signatures(1, 'n')

[davidhalter]

I will have to test this. But be aware that I'm also trying to release a new Jedi version and that has a slightly higher priority. Expect a delay of like 1-2 months.

[blueyed]

@davidhalter Yeah. I am test-driving this myself currently, and hope others might do so already (I've pinged some issues).

It is really sweet, since I can use 1 instead of 2 again, and I am really liking the overlay in normal mode!

The heart of it seems to be 80caf437dc8545be1bfa2467f89354cc23dea6a8, which uses undojoin and restoring of the previous lines literally instead of by replacing them back. AFAICS it is not possible to change those lines, except for some <c-o> magic etc..

I don't like too wait for too long merging this though - while I can see and appreciate the Jedi work you are doing, you should just use this jedi-vim branch while working on it, and report any issues here.. :)

(I really feel like we should have (fixed) tests for this (sadly all PRs are red currently in general). Don't feel too bad about it though - eventually I'll probably fix the current tests)

[davidhalter]

while I can see and appreciate the Jedi work you are doing, you should just use this jedi-vim branch while working on it, and report any issues here.. :)

You're probably right. My overall laziness is just getting ahead of me.

[davidhalter]

I'm now using it locally. Before I've had so many issues with Jedi that it would not have made sense to destabilize one more thing :)

[davidhalter]

My first impression is that it works really well except for that one strange case, I never seem able to reproduce. Sometimes the call signature won't disappear.

Do you have no issues at all?

[blueyed]

@davidhalter Sorry, I was not using this branch currently.. apparently I got used to it not working properly in Neovim, and therefore did not miss it.. ;) Will use it now again (and have it rebased on master).

What settings are you using? Are you changing g:jedi#show_call_signatures_modes / g:jedi#show_call_signatures_delay? I assume you're using Vim with timers? Are they staying when removing insert mode?

[davidhalter]

I'm using pretty much the basic settings (at least concerning call signatures) and have otherwise not modified much.

I assume you're using Vim with timers?

What are timers?

[blueyed]

The timer feature, which this is based on (:echo has('timers')).

[davidhalter]

:echo has('timers')
0

[blueyed]

That's an old Vim then.. and different code paths that I am using. Try to figure out the pattern when they do not go away, i.e. is is when you're moving to another line in insert mode, leaving insert mode, etc.

[davidhalter]

I'm on it. It's pretty weird. It looks like happens within tuples in function calls.

[blueyed]

@davidhalter Pushed some fixes, should work better.

For one, I had problems with lazy-loading on InsertEnter: the completion would show up, but then it was removed again.. (since deoplete appeared to call jedi-vim's omnifunc, which then cleared the signatures again).

[davidhalter]

I was just able to reproduce the error but now it's gone again thanks to your changes. I'm testing.

[davidhalter]

I have had issues, but it wasn't as bad anymore. Apart from that I think your PR also indirectly uncovered a VIM issue :/

Vim: Caught deadly signal SEGV
*** Error in `vim': corrupted double-linked list: 0x0000562d7e4f1d00 ***
                                                                        Segmentation fault (core dumped)

[blueyed]

Thanks for trying it already! I have found some issues myself, and will update it later.

[blueyed]

Pushed some fixups from my local changes, but it's still WIP.

The only issue I've seen recently was that conceallevel was not set under some circumstances.

[davidhalter]

Hmm it's strange. My VIM 7.4 still sometimes fails with segmentation faults. This is obviously not your fault. It's something the VIM maintainers should care about. However it's probably still not a good idea to merge this, until this issue is resolved.

I cannot exactly reproduce it, but it looks like it happens when both completion and call signatures are active. It has probably to do with the undo stuff.

VIM - Vi IMproved 7.4 (2013 Aug 10, compiled Nov 24 2016 16:44:48)
Included patches: 1-1689
Extra patches: 8.0.0056

[blueyed]

Good point. Maybe we could/should detect any visible popup menu (pumvisible()) in some places.

I've added a fix to remove the callsigs in normal mode before saving when using callsigs in normal mode.. :) (and rebased it)

[davidhalter]

I'm testing again. I hope this was it :)

[blueyed]

@davidhalter Are you actually using it for normal mode? (I found this a bit too confusing myself)

[joshuarubin]

I had the conflict with deoplete causing call signatures to fail. This patch fixed it. Is anything holding up the merge?

[davidhalter]

segmentation faults of vim :) I'd have to test the last couple of changes again, though.

[copy]

I like these changes. I've tested them and they work well (no crashes on vim 8).

Now that a b:_jedi_callsig_orig is used to store and restore the original buffer, couldn't we get rid of the conceal stuff and just put the call signature in the buffer directly? That'd avoid all problems with other syntax plugins.

[blueyed]

@copy Thanks for your review and testing!

couldn't we get rid of the conceal stuff and just put the call signature in the buffer directly?

The conceal stuff is used to make things bold etc also, isn't it?

[copy]

The conceal stuff is used to make things bold etc also, isn't it?

Indeed, it's also used for the background colour. Still, the buffer replacement can be simplified by removing the original code from the "magic" line. That should make it less fragile at least.

[blueyed]

Still, the buffer replacement can be simplified by removing the original code from the "magic" line.

Sounds good. Can you make a comment on the actual code, please?

[blueyed]

Rebased.

[copy]

I found a small issue: When the line with the call signature contains a backslash, it is duplicated (sometimes more than once). For example:

import sys
\
sys.get_asyncgen_hooks(

Typing after the ( causes the buffer to look like this:

import sys
\\
sys.get_asyncgen_hooks(a

[blueyed]

@copy The escaping should be fixed now.

[blueyed]

Using this as a daily driver now again myself.

Found a good workaround when using python-pep8-indent: https://github.com/Vimjas/vim-python-pep8-indent/pull/98, which will use the orig lines that jedi-vim stores now before concealing.

[blueyed]

Hopefully fixed the tests by now (review carefully!).

I think this could be merged soon in general - revisited all the previous comments also.

[blueyed]

@davidhalter This is huge (sorry!), but really useful in general and has gotten quite some good feedback from others. Now it is waiting on yours, and then I will clean it up.

[davidhalter]

I'll try to test it. Might take a few weeks since I'm currently on vacation.

[codecov-io]

Codecov Report

Merging #652 into master will decrease coverage by 10.57%. The diff coverage is 29.62%.

@@             Coverage Diff             @@
##           master     #652       +/-   ##
===========================================
- Coverage   51.54%   40.97%   -10.58%     
===========================================
  Files           3        3               
  Lines         807      920      +113     
  Branches      165      193       +28     
===========================================
- Hits          416      377       -39     
- Misses        354      499      +145     
- Partials       37       44        +7

Flag	Coverage Δ
#py27	40.21% <29.62%> (-9.54%)	:arrow_down:
#py37	39.45% <29.62%> (-9.41%)	:arrow_down:
#py38	?

Impacted Files	Coverage Δ
pythonx/jedi_vim.py	41.37% <29.62%> (-8.21%)	:arrow_down:
pythonx/jedi_vim_debug.py	0% <0%> (-46.56%)	:arrow_down:
test/test_integration.py	86.66% <0%> (-0.57%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8d24b83...f2a84f0. Read the comment docs.

[davidhalter]

I assume you still want me to review this. While I can see that you listed some of the changes you did above, I would be interested in what you're exactly trying to solve. There's a lot of comments and I'm not sure what exactly you're trying to do here. After that I will try to review it.

[blueyed]

@davidhalter better wait.. I have WIP here to use Vim's popup / Neovim's floating window here additionally, deprecating the conceal method.

But input would be appreciated already on how to configure / enable this.. use a new value 3? Or use it for 2 if supported? Might also make sense to have a dict for config here, since there are several options (prefer above/below cursor, right/left from cursor (I had the idea to display it right next to the cursor, when at the end of a line, starting with the current argument then)). Therefore instead of a 3rd mode we could enable it when the config value is a dict - allowing to add more options there lated.

[davidhalter]

I feel like if we can use floating windows or whatever that doesn't modify source code, we just ditch conceal. It has been a pain point for so long.

For a certain amount of time (maybe until an older LTS Ubuntu supports this feature), we could use the value 3.

I hope that this is going to be the only mode available in 2022 or something. And because of that I would just use a setting show_call_signatures > 0 as showing the floating Windows. I would then probably add another setting like call_signature_configuration, where people would be able to override the stuff you mentioned. So I don't really think we should break the API here, because I actually like the on/off setting and a separate dict.

What do you think?

[blueyed]

we just ditch conceal. It has been a pain point for so long.

well, we can keep it for older Vim versions, no?

Apart from that the idea was to use a dict instead of 3. I.e. if a dict is used it will use the new API, where you could possibly also set a value to force the cmdline mode (or "conceal").

I.e. it would transfer the setting internally to a dict always, mapping the current 1 or 2 values to "cmdline" or "conceal", but defaulting to "floatwin".

I.e. let g:jedi#show_call_signatures = {} should enable the new setting if supported, falling back to "conceal" (or even "cmdline" if conceal is not available).

But that sounds rather complicated in general I agree.

I'd just like to avoid a new set of distinct settings, when they can be grouped together.

[davidhalter]

I don't have a strong opinion. I'm probably a sleight bit in favor of separating the settings, but I'm not even sure about that. Feel free to do it however you want.

[blueyed]

@davidhalter Should be good as a daily driver for testing already. I'd like to finish this soon.

[davidhalter]

Sorry not sure I understand. Do you want me to test it a bit? Just note that I only have VIM. So you need to do the neovim testing yourself.

[blueyed]

@davidhalter yes, please test/use it. Note that there are two methods for Vim already, but it's better to test the newer one.

[davidhalter]

I'm starting to test it.

Are the color changes intentional? I'm totally fine with them, there's just a lot of commits and I'm not sure where some of them come from.

Also it unfortunately doesn't help that even before the changes there were quite a few bugs (especially about the conceal thing, but probably there are also Jedi bugs).

[blueyed]

Are the color changes intentional? I'm totally fine with them, there's just a lot of commits and I'm not sure where some of them come from.

Not necessarily, I guess it's mostly due to ./syntax/jedi_signature.vim no being committed, which I just did.. :)

[davidhalter]

I have been testing this for the past few weeks, it looks pretty good. As far as I can see, the signatures colors are still missing (I pulled just now). Maybe you can look at this again. On master everything seems right.

The conflict is probably because of #966. I'm pretty happy to merge when those two things get resolved.

[blueyed]

As far as I can see, the signatures colors are still missing (I pulled just now). Maybe you can look at this again.

Are you using Vim 7.4.X still? (i.e. g:jedi#show_call_signatures = 1) The newer methods are available only later, and IIRC require to enable them using let g:jedi#show_call_signatures = 3.

[davidhalter]

Yeah, I'm using still 7.4 :) Old Ubuntu 16.04.

Obviously I'll have to update soon, but it's still in a good shape IMO. It would still be nice I guess to support jedi-vim at least until this retires (will happen in Ubuntu 20.04).

However if the colors work with g:jedi#show_call_signatures = 1 in later releases, I'm fine with just ignoring this issue. It's not really important.

What methods would be needed to get the colors working on 7.4?

[blueyed]

What methods would be needed to get the colors working on 7.4?

I assume they are broken in Vim without the popup window then in general - will have to investigate/test it, not much time for it currently though.

[blueyed]

What methods would be needed to get the colors working on 7.4?

I cannot really tell what you are missing there. Are you using a colorscheme?

Using the following minimal vimrc:

let script_dir = fnamemodify(expand('<sfile>'), ':h')
let &runtimepath = script_dir.','.&runtimepath.','.script_dir.'/after'

set nocompatible
set shortmess-=F
colorscheme default

set completeopt=menuone,longest,preview

let g:jedi#force_py_version = '3'

let g:jedi#auto_vim_configuration = 1
let g:jedi#show_call_signatures=1

syntax on
filetype plugin indent on

It looks like this:

Commenting the "colorscheme" I see thiss:

This is on master.

On this branch it looks like the first case always, also without a colorscheme. This is due to https://github.com/davidhalter/jedi-vim/blob/470bb10f5ab9706ce713e8a2eb3d25e36b8c25e0/autoload/jedi.vim#L699-L701, where it does not check for g:colors_name anymore, but just the highlight (on master: https://github.com/davidhalter/jedi-vim/blob/12e97c7a04f8fd80afe0de0f1c9181f2c7bc53ca/after/syntax/python.vim#L25-L28)

[blueyed]

For reference, this is how it looks on this branch with my config (Neovim, and Vim, using the new method (let g:jedi#show_call_signatures = 3):

With let g:jedi#show_call_signatures = 1:

[davidhalter]

@blueyed I figured out why the colors are different, no fixes from your side required.

This happens because previously we checked with if exists('g:colors_name'). Now you use if hlexists('TabLine') and if hlexists('CursorLine'). So if I want different colors, I can easily overwrite those (Not that tit's important).

BTW: I think we could also start thinking about retiring the ifs there and just require people to have CursorLine/TabLine in their syntax. However that's definitely for another PR.

[davidhalter]

BTW: While there's obviously conflicts, I'm still interested in this change. If you think this is still in a good shape, we should probably just merge it and do one last review.

[blueyed]

@davidhalter

BTW: While there's obviously conflicts, I'm still interested in this change. If you think this is still in a good shape, we should probably just merge it and do one last review.

I've been using this branch since a long time already (with Neovim), and therefore think it is (or rather was ;)) in good shape. Rebased it. There's at least an issue/conflict with tests that should get addressed (https://github.com/davidhalter/jedi-vim/commit/e4daf7a74c501381a62254b2cd922d74d6e62ac7).

[davidhalter]

What do you think about using show_call_signatures = 1 for floating windows and 3 for the conceal stuff? In case the VIM version is not good enough, we could fallback to 3.

[blueyed]

What do you think about using show_call_signatures = 1 for floating windows and 3 for the conceal stuff? In case the VIM version is not good enough, we could fallback to 3.

:+1: Wouldn't really be backward-compatible, but in the long run the conceal stuff should get removed / phased out anyway.