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

Allow soft-wrapping of lines in the signature help window #4252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented Aug 4, 2024

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

Fixes #4171

Or at least proposes a fix. Here's a short demo:
https://asciinema.org/a/UJlR80blt6OvuwUIqb4swnWUW

This looks ugly, but at least no data is lost due to lines going off screen.


This change is Reviewable

Copy link

codecov bot commented Aug 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.88%. Comparing base (80728a1) to head (710acf5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4252      +/-   ##
==========================================
+ Coverage   89.80%   89.88%   +0.07%     
==========================================
  Files          37       37              
  Lines        4758     4763       +5     
==========================================
+ Hits         4273     4281       +8     
+ Misses        485      482       -3     

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this out and I quite liked it but there were cases where it was I think net worse than what we have now.
This is a shame because it's just really hard to solve for both problems of horizontal positioning being "sane" and inducing as much info on the screen as possible. tricky tricky.

Reviewable status: 0 of 2 LGTMs obtained

@bstaletic
Copy link
Collaborator Author

Right, the shifting to the left is broken in this pull request.
Let's leave it for now and maybe I'll get back to it during the weekend.

@hippie68
Copy link

Thank you for working on this. Today I was searching for how to enable this feature only to find out that it doesn't exist yet.

@bstaletic
Copy link
Collaborator Author

I'll be getting back to this pull request, this week.
My idea is to only set wrap when the popup is all the way left in column 1. We'll see how that goes.

@bstaletic
Copy link
Collaborator Author

I think I got this working.
@hippie68 would you mind trying this branch/patch?

@hippie68
Copy link

This latest update works well for me. I tested it with those overly long Python function signatures, which now wrap properly. It works like I would have expected YouCompleteMe's signature help window to work out of the box.

@bstaletic bstaletic force-pushed the sig_help_wrap branch 2 times, most recently from d6e059f to 710acf5 Compare September 12, 2024 00:50
This looks ugly, but at least no data is lost due to lines going off
screen.
@hippie68
Copy link

There's one minor cosmetic issue left: when the text wraps, there should be a space appended at the end of each wrapping line. At least that's the style all other pop-up windows have. I'm not sure if that is possible here though.

In the example screenshot, one space would be inserted after "Opt" and "flus" each to have the right pop-up border width match the left one.

ycm-wrap-space

@hippie68
Copy link

Never mind, I just noticed that this is normal wrapping behavior even for docstrings. Which brings me to the question if there is anything else left that is stopping this pull request from being merged. I have been trying this feature for quite some time now and I would love to see it included in YCM by default.

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


python/ycm/signature_help.py line 162 at r4 (raw file):

    col = int( screen_pos[ 'curscol' ] ) - 2

  # Vim stops shifting the popup to the left if we turn on soft-wrapping.

I think this is just lazy programming by the absolute waster that implemented this left-shifting in Vim.

https://github.com/vim/vim/blob/master/src/popupwin.c#L1437-L1446

vim/vim#4466

Part of me is like "we should change the Vim behaviour rather than work it out here because, well, that was the whole reason that I implemented it in Vim rather than here"....

Basically by enabling 'wrap' we have to implement that PR's logic in here instead: https://github.com/vim/vim/blob/master/src/popupwin.c#L1447-L1466


python/ycm/signature_help.py line 165 at r4 (raw file):

  # Instead, we want to first shift the popup to the left and then
  # and then turn on wrapping.
  max_line_length = len( max(

marginally more efficient (and simpler) to write:

max( len( item[ 'text' ] ) for item in buf_lines )

python/ycm/signature_help.py line 169 at r4 (raw file):

    key = lambda item: len( item[ 'text' ] ) )[ 'text' ] )
  vim_width = vimsupport.GetIntValue( '&columns' )
  line_available = vim_width - int( screen_pos[ 'col' ] )

Shouldn't this be col here rather than screen_pos[ 'col' ] ? (assuming that col > 0).

in the previous stanza we offset the col position according to some padding and positioning logic. I think that this calculation needs to account for both sides of the padding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signature hints for certain Python libs truncated, no line breaks
3 participants