Skip to content
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

Option to abbreviate completion info with completeopt+=popup #3547

Closed
12 tasks done
dhleong opened this issue Nov 23, 2019 · 28 comments · Fixed by #3682
Closed
12 tasks done

Option to abbreviate completion info with completeopt+=popup #3547

dhleong opened this issue Nov 23, 2019 · 28 comments · Fixed by #3682
Labels
Good First Issue Good for new contributors PR Welcome A good quality PR for this feature would be considered.

Comments

@dhleong
Copy link
Contributor

dhleong commented Nov 23, 2019

Issue Prelude

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your issue:

  • I have read and understood YCM's [CONTRIBUTING][cont] document.
  • I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
  • I have read and understood YCM's [README][readme], especially the
    [Frequently Asked Questions][faq] section.
  • I have searched YCM's issue tracker to find issues similar to the one I'm
    about to report and couldn't find an answer to my problem. ([Example Google
    search.][search])
  • If filing a bug report, I have included the output of vim --version.
  • If filing a bug report, I have included the output of :YcmDebugInfo.
  • If filing a bug report, I have attached the contents of the logfiles using
    the :YcmToggleLogs command.
  • If filing a bug report, I have included which OS (including specific OS
    version) I am using.
  • If filing a bug report, I have included a minimal test case that reproduces
    my issue, including what I expected to happen and what actually happened.
  • If filing a installation failure report, I have included the entire output
    of install.py (or cmake/make/ninja) including its invocation
  • I understand this is an open-source project staffed by volunteers and
    that any help I receive is a selfless, heartfelt gift of their free time. I
    know I am not entitled to anything and will be polite and courteous.
  • I understand my issue may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Thank you for adhering to this process! It ensures your issue is resolved
quickly and that neither your nor our time is needlessly wasted.

Issue Details

In general, the completion detail that YCM provides is great and I've been enjoying it for years. However, with the new completepopup option (set completeopt+=popup) it can be too long:

Screen Shot 2019-11-23 at 9 36 57 AM

As an option, or perhaps automatically if popup is set (or maybe an option that takes effect if popup is set?) it'd be nice to omit the extra info to the right of the completion type, since the popup makes it redundant anyway, and because it means the popup is too squished to be legible.

Alternatively, you may prefer that this be fixed upstream in vim by having a min-width config (or something) in the completepopup option?

Thanks for your time!

@puremourning
Copy link
Member

don't you think the extraordinarily long signatures would be a problem for the popup too ? I think the issue here is that the signature info is long, not specifically anything to do with popups. Even without completeopt+=popup, the completion info is wider than your screen.

You didn't include any steps to reproduce, so it's unclear if this is a bug or not, nor which completer server we would change if it were.

@dhleong
Copy link
Contributor Author

dhleong commented Nov 23, 2019

It's not a bug—this is a feature request/RFC—but for what it's worth the screenshot is from the typescript completer (--ts-completer). The repro steps are just invoke completion as normal on an array type. However the experience is probably the same with any completer that provides long signatures like this.

As you say, the signature itself is wider than the screen, but if the signature is removed from the pum then it can be viewed in the popup, which also includes documentation. The signature may wrap, but I don't think that's worse than it getting cutoff in the pum, and having documentation might actually be a beter UX. Normally I have completeopt+=preview so it goes into the preview window, but I only rarely look at it since it can be far away from the pum. I think the popup mode instead of preview here could be nicer.

@puremourning
Copy link
Member

The problem in implementing this is that there's no way to know of the 'preview' contents (which might be displayed in a popup) actually even contains the data that would be in the 'extra' menu info. That's true of course for an arbitrary language/server, but it's even true for a given (specific) language/server.

I kind of take the point that this display isn't optimal, but I don't know that there's a simple, canonical, maintainable way we could improve it. I also don't think this problem is likely to be unique to YCM. Perhaps you're right that the 'preview' popup should have a minimum width (and probably fall back to the preview window in that case). You could try vim-dev for that.

@puremourning
Copy link
Member

I'm going to leave this open, because we may be able to improve this, but it's probably going to remain in the ideas or PRs welcome category until we can think up a good solution.

@dhleong
Copy link
Contributor Author

dhleong commented Nov 23, 2019

Aren't the preview contents from the info key on the completion item? Probably requires more testing but just commenting out the 'menu': ... bit in completion_request.py results in:

Screen Shot 2019-11-23 at 10 48 19 AM

EDIT: A "real" solution would probably be more complicated than this, of course, but wanted to try it and throw it out there as a POC. A real solution would probably might want the option to do this on a per-filetype basis, only if info is available, etc. Probably still tricky, as you say.

@puremourning
Copy link
Member

Even your POC just shows how much information is lost by removing the info key. Now you don't have any info at all for the other non-selected options. IMO this is not an acceptable regression.

@dhleong
Copy link
Contributor Author

dhleong commented Nov 23, 2019

I see your point, but I suppose it depends on your perspective. For me, having the long signature on every method starts to become noisy. With this it's more focused, showing me the detailed information only on the thing I'm actively looking at. I can imagine that some people would like the signature on every method though; that's why I suggested it being an option. I also understand, though, that every option introduced means more complexity to maintain, and I can definitely appreciate that.

EDIT: I got most of the way through writing up a post to vim-dev, but it sounds like the upcoming flip option is probably intended to solve this:

	flip		When TRUE (the default) and the position is relative
			to the cursor, flip to below or above the cursor to
			avoid overlap with the |popupmenu-completion| or
			another popup with a higher "zindex".  When there is
			no space above/below the cursor then show the popup to
			the side of the popup or popup menu.
			{not implemented yet}

@mogelbrod
Copy link

Even your POC just shows how much information is lost by removing the info key. Now you don't have any info at all for the other non-selected options. IMO this is not an acceptable regression.

I would love an option to hide the signature in the completion menu, allowing us to rely on the popup to display the signature instead. It reduces the signal to noise ratio of the completion list IMHO. I think this is how vscode intellisense works (not that it's necessarily the golden standard or anything, but still):

vscode intellisense

@puremourning
Copy link
Member

I mean I guess we could suppress it if completeopt contained popup but I think that's workflow breaking for all other users.

Options are bad, so we need to come up with a heuristic that automatically makes it work without switches or dials.

@mogelbrod
Copy link

I mean I guess we could suppress it if completeopt contained popup but I think that's workflow breaking for all other users.

Yeah, I think it boils down to whether or not the current language/project has "verbose" signatures. For this reason I can't think of a solution that doesn't involve adding an option. An option would be changeable by the user globally, and via FileType autocommands if the user had a preference for a particular language/project.

Options are bad, so we need to come up with a heuristic that automatically makes it work without switches or dials.

When would the heuristic run? Before showing the first completion menu within each file? On every keypress? Having YCM switch between including/excluding the entire signature on a case by case basis sounds like it would be confusing/annoying UX 😟 If an heuristic was implemented and it wasn't perfect I'm guessing it wouldn't be long until someone requested the ability to configure it.

@puremourning
Copy link
Member

You make good points. I think actually if you think about it, there's already an option:

  • Option 1 : Use completeopt+=preview
  • Option 2: Don't.

@mogelbrod
Copy link

Really appreciating the quick responses!

Using +=preview still appears to cause scrolling of the active window as described in #390 and #2015, unfortunately. The popup is much more unobstrusive, when it has enough room at least 👍

Are you implying that no change could/will be made to the current behaviour, or that this could be implemented?

I mean I guess we could suppress it if completeopt contained popup but I think that's workflow breaking for all other users.

I hope I'm not coming off as demanding. completeopt+=popup is just a very useful feature, and I'd be sad if I couldn't use it with YCM for most typescript projects.

@puremourning
Copy link
Member

Does anyone have a minimal reproduction case ? I don't see one on any of the related issues.

@puremourning
Copy link
Member

Screenshot 2019-12-30 at 11 03 48

Thoughts ? This truncates the signature when it gets > 1/3 of screen width and sticks the full signature in the preview popup.

@puremourning
Copy link
Member

TBH with typescript, I think the issues is just that the signature is in both the extra menu info and the preview window. I'm not sure why we have that exactly

@puremourning
Copy link
Member

Please try out the PR in #3567 and provide feedback

@bstaletic
Copy link
Collaborator

the signature is in both the extra menu info and the preview window. I'm not sure why we have that exactly

It's the same in python, java and clangd.
C# doesn't put the signature in the preview.
Rust and Go place the function name in the preview.

@mogelbrod
Copy link

Thoughts ? This truncates the signature when it gets > 1/3 of screen width and sticks the full signature in the preview popup.

It's a big improvement! As both you and @bstaletic wrote it will result in signature duplication in multiple languages though. I still think there's a case for the allowing the signature to be hidden in the completion menu. See the screenshots below for a comparison.
The signature less version was implemented by hard coding 'menu': '' in _ConvertCompletionDataToVimData().

Screenshot 2019-12-30 at 12 56 30

Screenshot 2019-12-30 at 12 57 02

@puremourning
Copy link
Member

didn't we just recently add the signature to the extra menu info for typescript? Maybe there's just a better thing we could put there, like the return value or something ? Having that extra info there allows you to see it for all visible items, rather than just the "selected" one. Remember that YCM's whole thing is that you just keep typing rather than scrolling through the menu, which is why we try to make things visible rather than hidden.

@mogelbrod
Copy link

The popup/preview window has the advantage of allowing multilines signatures which can improve readability for longer ones such as the one you posted.

I just tried Python and it doesn't appear to be consistent regarding signatures in the docs, doh.
with
without

Since the signature sometimes differ in style between completion and docs it seems non-trivial to determine whether to prepend it to the popup or not.

Sorry for bringing more questions rather than solutions, maybe someone else has some input or ideas?

@puremourning
Copy link
Member

puremourning commented Dec 30, 2019

I guess Jedi is using the type annotations/hints. Maybe they aren't there for str.encode in your installation.

they are for mine:

Screenshot 2019-12-30 at 17 18 49

For me, the truncation approach seems to work reasonably well:

Screenshot 2019-12-30 at 17 19 26

Edit: Actually, in your case the preview popup is probably just scrolled. I can't see the scrollbar in your colour scheme.

@puremourning
Copy link
Member

For the record I almost never use typescript, so I'm completely ambivalent about whether or not we keep the signature in the popup for that language. Personally it seems like the signature we're creating contains too much non-data-ink and doesn't really add much value, so I would be OK to remove it, but I do feel that's a breaking change, and that it might be better to parse the 'displayParts' more intelligently to see if we can construct something shorter that contains useful info (such as just the names, not types of arguments maybe)

@puremourning puremourning added Good First Issue Good for new contributors PR Welcome A good quality PR for this feature would be considered. labels Jan 4, 2020
@mogelbrod
Copy link

I guess Jedi is using the type annotations/hints. Maybe they aren't there for str.encode in your installation.

Yeah, looks like we've got different docs.

Edit: Actually, in your case the preview popup is probably just scrolled. I can't see the scrollbar in your colour scheme.

That could definitely have been the issue, but after colorizing the scrollbar nothing appeared to be missing (got no scrollbar in the popup), so I think it's just a case of mismatched docs.

For the record I almost never use typescript, so I'm completely ambivalent about whether or not we keep the signature in the popup for that language. Personally it seems like the signature we're creating contains too much non-data-ink and doesn't really add much value, so I would be OK to remove it, but I do feel that's a breaking change, and that it might be better to parse the 'displayParts' more intelligently to see if we can construct something shorter that contains useful info (such as just the names, not types of arguments maybe)

The issue mostly pertains to languages with verbose typing, TypeScript being the latest addition. There's an issue in the TypeScript repo regarding this (Shorten method signatures) posted in 2017 that's still open. I imagine that it would be somewhat complicated to write a TypeScript signature shortener that handles all possible edge cases (nested objects/generics etc.), but I could take a stab at some Regex hacking if you think it would be reasonable.

puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Jan 28, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Feb 9, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Feb 9, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Feb 9, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Feb 12, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Mar 4, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Mar 26, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 10, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 13, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 14, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 17, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 17, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue Apr 30, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue May 5, 2020
@puremourning
Copy link
Member

This issue has been open and my WIP branch available for quite some time now. Nobody has offered a PR to complete that work, or offered better suggested heuristics. I gather from that that there is not much interest In the change ? I'll leave this 1 more week then close it.

@dhleong
Copy link
Contributor Author

dhleong commented May 9, 2020

I'm definitely still interested! I just checked out your fork and it feels pretty great so far. I like that I don't have any preview windows moving around, and it's quite usable. I think in the future the potential to expand the (+N overloads) thing in there is fantastic (see below):

Screen Shot 2020-05-09 at 9 34 04 AM

One issue I ran into (also evidenced in the above image) is that, in typescript, the preview_info seems to already contain the extra_menu_info, so one tweak I would request is that if extra_menu_info.startsWith(preview_info) then we just do a replace instead of append.

@puremourning
Copy link
Member

How interested? Like make a PR with tests and documentation interested?

dhleong added a commit to dhleong/YouCompleteMe that referenced this issue May 13, 2020
puremourning added a commit to puremourning/YouCompleteMe that referenced this issue May 13, 2020
@dhleong
Copy link
Contributor Author

dhleong commented May 13, 2020

Added a small handful of tests and fixed the behavior I was seeing with the duplicated signatures from Typescript, and documented the new option—see PR linked above!

puremourning added a commit to puremourning/YouCompleteMe that referenced this issue May 26, 2020
@mergify mergify bot closed this as completed in #3682 Jun 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue Good for new contributors PR Welcome A good quality PR for this feature would be considered.
Projects
None yet
4 participants