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 setting popup max width #416

Merged

Conversation

thomasfaingnaert
Copy link
Collaborator

@thomasfaingnaert thomasfaingnaert commented Jun 26, 2019

In response to: #395 (comment).
Tested with Vim 8.1.1592 and NVIM v0.4.0-1149-g8b263c7a6.

Note that Vim and Neovim have slightly different behaviour, Vim wraps on every character:
Screenshot from 2019-06-26 13-21-47

whereas Neovim only wraps at whitespace:
Screenshot from 2019-06-26 13-22-14

Should I try to change it so both Vim and Neovim behave the same?

@clason
Copy link
Contributor

clason commented Jun 26, 2019

Works nicely for neovim; I personally prefer its behavior (wrap at whitespace only).

Shameless request: is it possible to (optionally) make the popup preferentially open above (or to the right of) the cursor (with correspondingly changed anchor)?

@thomasfaingnaert
Copy link
Collaborator Author

@clason You mean opening it at that anchor in all cases? If so, this can easily be done by changing the anchor (Neovim) and pos (Vim) options.

@clason
Copy link
Contributor

clason commented Jun 26, 2019

@thomasfaingnaert Well, basically switching the current behavior (below unless too close to the bottom) to its opposite (above unless too close to the top) -- or rather, make it a vimrc option since tastes differ. (It's not critical, just a thought.)

let &l:readonly = 0
let &l:modifiable = 1

normal! gggqGgg
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you just give the maxwidth in line 233 call s:adjust_float_placement(l:bufferlines, l:maxwidth) and enable soft wrap?
Imo turning it modifiable, format and unmodifiable again feels kind of hackish.

If the current solution has any benefit, it should be done in s:setcontent, as it effects the content itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jerdna-regeiz
If you mean something like this:

diff --git a/autoload/lsp/ui/vim/output.vim b/autoload/lsp/ui/vim/output.vim
index 7b52f4c..718b01d 100644
--- a/autoload/lsp/ui/vim/output.vim
+++ b/autoload/lsp/ui/vim/output.vim
@@ -211,7 +211,7 @@ function! lsp#ui#vim#output#preview(data) abort
 
     if s:supports_floating && s:winid && g:lsp_preview_float
       if has('nvim')
-        call s:adjust_float_placement(l:bufferlines, l:maxwidth)
+        call s:adjust_float_placement(l:bufferlines, 10)
         call s:add_float_closing_hooks()
       endif
       doautocmd User lsp_float_opened

then that doesn't work: it only displays the first line of the popup. Moving it to s:setcontent would indeed be better.

Copy link
Contributor

@jerdna-regeiz jerdna-regeiz Jun 26, 2019

Choose a reason for hiding this comment

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

Well, then it depends on the source or some other settings, as it wraps each row nicely for me.
But in setcontent it looks great and a lot less hacky =)

Actually this does now apply to normal preview as well (which is nice in my opinion!). Maybe we should just call the option g:lsp_preview_max_width as it now applies to every flavor of it.

plugin/lsp.vim Outdated
@@ -28,6 +28,7 @@ let g:lsp_highlight_references_enabled = get(g:, 'lsp_highlight_references_enabl
let g:lsp_preview_float = get(g:, 'lsp_preview_float', 1)
let g:lsp_preview_autoclose = get(g:, 'lsp_preview_autoclose', 1)
let g:lsp_preview_doubletap = get(g:, 'lsp_preview_doubletap', [function('lsp#ui#vim#output#focuspreview')])
let g:lsp_popup_max_width = get(g:, 'lsp_popup_max_width', -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either call both float or popup (meaning g:lsp_preview_float and g:lsp_popup_max_width).

@jerdna-regeiz
Copy link
Contributor

jerdna-regeiz commented Jun 26, 2019

Should I try to change it so both Vim and Neovim behave the same?

Guess it would be a lot of work for very little gain.
I like the nvim way, but I guess it depends on who you ask and supporting both styles in both nvim an vim will be an overkill.

Imo it is nice as it is now, regardless of different line wraps.

@clason
Copy link
Contributor

clason commented Jul 15, 2019

Regardless of my bikeshedding earlier, I think it's fine as is, too, and would like to see this get merged. Or is there something still missing? I'd be happy to help test.

@prabirshrestha prabirshrestha merged commit 9175167 into prabirshrestha:master Jul 18, 2019
@thomasfaingnaert thomasfaingnaert deleted the popup-max-width branch July 21, 2019 17:52
@clason
Copy link
Contributor

clason commented Jul 21, 2019

I feel really bad about this, but there's one issue with this PR that I didn't notice before the PeekDefinition PR was merged (thank you!): Setting preview-max-width causes any popup to have reflown text (i.e., all line breaks in a paragraph are removed and the resulting long line is split at max-width). This makes the PeekDefinition useless, since it doesn't show the definition accurately (for one, it misses the location due to the reflow). On the other hand, the hover information mentioned in the issue is rather useless without the max-width...

As far as I can see, there are two options:

  1. Don't remove line breaks in the source before reflowing
  2. Only apply max-width if the preview text contains only a single line (i.e., no line breaks).
    What would be the easiest option? (Option 3, make max-width a flag that can be passed to some popups, doesn't strike me as sensible.)

EDIT: I see from the code that the reflowing is manual (gggqGgg) -- is there a way to achieve the same behavior with softwrap? This is on neovim (HEAD), I should mention.

@clason
Copy link
Contributor

clason commented Jul 22, 2019

Looking at the code a bit more, I see that wrap is already set for the floating window, so I am actually a bit puzzled why this doesn't work for the single line.

Replacing the if clause in s:setcontent by

if g:lsp_preview_max_width > 0
    let l:width = min([l:width,g:lsp_preview_max_width])
endif              

in floatingpreview shows the correct behavior on PeekDefinition (max_width is respected, long lines are wrapped, but no linebreaks are removed), but not on Hover (`max_width is also respected, but the line is not wrapped).

So I think the underlying issue that led me to request max_width is actually the handling of either single-line or specifically Hover popups.

(On a related note, I saw a trick for getting "borders" for neovim floats: Open a slightly larger float of reverse highlight (light for dark or dark for light) under the actual float, see neovim/neovim#9718 (comment))

@thomasfaingnaert
Copy link
Collaborator Author

@clason
I tried a different approach in #440. Could you try it and let me know how well it works?

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.

4 participants