TUI / GUI tooltip with content from ALEHover #1556

gagbo · 2018-05-05T22:50:35Z

This is a RFC / small fix maybe.

I would like to have :ALEHover content displayed in a tooltip like termdebug plugin so beautifully do in vim

I found out that ale already uses the balloon feature that termdebug uses under the hood, therefore I would like to find the correct way to access the string that ale#hover#Show() ultimately shows in the message bar. I discovered that it's not that easy. What do you think the best way is ?

Should I reimplement all functions that autoload/hover.vim uses ?

Should I make more functions public so we can have easier access (ShowDetails ?) ?
Should I change the signatures of some functions so they actually return strings (namely the callback handlers ?) ?

Ultimately, the goal is to allow users to choose a source for the balloonexpr, from loclist like it's currently done, or from :ALEHover content (typically a setting that would be buffer dependent, so it's possible to activate the Hover source only for filetypes where LSP linters are plugged in)

Also, since I was there, I added some guards around the set ballooneval, so neovim or old vim versions won't complain, and also added balloonevalterm because in vim the GUI and TUI tooltips are separated options (but I assumed that we would want them in all possible cases if the user activated the feature).

gagbo · 2018-05-05T23:50:44Z

I added a bunch of " XXX: comments to show the basic changed I had to make to make the feature work as I want (= make the tooltip appear with some relevant text).

If we agree on the basic mechanism, we can start discussing about how making this clean (options for the user, avoid writing information at 2 positions, avoid code duplication...)

EDIT : as a side-note, I didn't touch the tsserver handler because I just wanted a dirty fix to show what needs to be done, but it's basically the same implementation as the LSPHandler, use the string passed to ale#util and pass it to balloon_show. the function handles the splitting alone.

w0rp · 2018-05-08T17:21:26Z

autoload/ale/balloon.vim

 function! ale#balloon#Disable() abort
-    set noballooneval balloonexpr=


I don't think we need to check for features here. We ought not to call these functions if you don't have the features for them.

w0rp · 2018-05-08T17:24:28Z

autoload/ale/balloon.vim

@@ -19,10 +19,43 @@ function! ale#balloon#Expr() abort
    return ale#balloon#MessageForPos(v:beval_bufnr, v:beval_lnum, v:beval_col)
 endfunction

+function! ale#balloon#HoverExpr() abort


ALE already uses balloons for diagnostics. I think we should make hovering over a position show details in a balloon if and only if you turn an option for hovering on, and we should show diagnostics on hover if you have that option on. For echoing hover messages, which is something I would also like to implement, I'd prioritize problems over hover information. For hovering with a mouse cover, it might make more sense to do the opposite.

This change is requested to be able to call the function with mouse position to enable hover information in vim's balloon

This check prevented the 'ALEHover in balloon' feature, since mouse position is almost never cursor position.

gagbo · 2018-05-08T19:15:34Z

I made a few changes accordingly :

there's only one ale#hover#Show() function now taking all the parameters
There was a check on cursor position in ale#hover#HandleLSPResponse() function that prevented handling the answer correctly since mouse and cursor are often in different positions
I'll do the vader corrections when I know I can finish the features.

There's only 1 thing I'm still wondering, but it's more 'vim' than 'ale'. When I run in TUI, the tooltip always disappear after a short delay, whereas in GUI, the tooltip stays longer. Maybe it's because my terminal emulator (kitty) is using openGL and there are redraws implied, I'll have to investigate.

Here are some gifs of what it currently looks like (in TUI (dark) and GUI (light)) the delay in TUI is due to some deferring of ale, it's my bad :

gagbo · 2018-05-08T19:29:29Z

Note : the tests currently fail because vim versions that have +balloon_eval and/or +balloon_eval_term do not necessarily have balloon_show() (which came in patch 0396).

I'm assuming tests are failing because vim versions in test instances are in this gap.

balloonexpr evaluation now works even without balloon_show for basic diagnostics, leaving the balloon_show call to ale#hover#Show, which can then feature guard the call to avoid errors

Also add a small comment to warn readers the different outputs the ale#hover#Show will write to

gagbo · 2018-05-08T20:24:49Z

It seems like it's impossible to use call ale#hover#Show(...) in a background job, so I don't think I can do better than this for now.

The only thing that still irks me is having the return '' after the call to ale#hover#Show() in ale#balloon#MessageForPos(). Since this return value is then sent as a result of balloonexpr, it is fragile, as there is a return from balloonexpr after the call to balloon_show nested in ale#hover#Show(). For the time being it looks like it works (I'll edit with smaller GIFs)

gagbo · 2018-05-08T20:49:01Z

The last (I think) 'pure design' decision remaining is knowing if we should print Hover output to both messages and balloon or just balloon when enabled and available. The issue with having both output is for multiline output (that trigger a new split and take the cursor), because not having the cursor in the same window anymore breaks Hover (see the GIFs).

I suppose we should also pass v:beval_winnr in some way to ale#hover#Show in order to send the proper message to lsp, but I'll check that later.
And then at some point I'll test the feature with diagnostics to check priority.
And then at some point I'll try to find a way to test this in Vader (I don't know yet how to send arbitrary LSP messages, nor how to send mouse events, nor how to check for a function call instead of a return value)

gagbo · 2018-05-08T21:36:58Z

  Starting Vader: C:\testplugin\test\test_history_saving.vader
(1/6) [  GIVEN] Some imaginary filetype
(1/6) [EXECUTE] History should be set when commands are run
(1/6) [EXECUTE] (X) ['command', 'job_id', 'status'] should be equal to ['command', 'exit_code', 'job_id', 'status']

Something happened in the merge, hopefully it's some Appveyor bug so it will magically repair in the next commit, otherwise I don't see how this operation broke anything

It is clearer that we only rely on l:options to get the relevant data to build the LSP Response string

The issue was caused by not using a buffer-specific version of getline() to cap the value of the column sent in the message to LSP. Therefore a cursor on column 10 in an inactive window could send a message with column=0, if the active window had a buffer with too few lines

With the upcoming change in ale_set_balloons default value (see Pull Request dense-analysis#1565), this check will be useless

gagbo · 2018-05-11T17:24:26Z

I've fixed my issue with the preview window (I forgot to replace a getline call with a getbufline call)

So now the behaviour for balloonexpr is :

If there is a diagnostic message, immediately return it in the balloon for showing and be done with it.
If there is no diagnostic message, then call ale#hover#Show with the cursor position to get information about the symbol under the cursor.

On the points that may need a little fixing so the whole feature feels less experimental :

All calls to ale#hover#Show (most notably those from :ALEHover command) will show a balloon if ale_set_balloons is 1. Separating properly the cases where ale#hover#Show() is called from a command or from balloonexpr should be done, but I'm not sure what the best way is right now. Is adding a flag to all ale#hover#Show calls and a flag in s:hover_map cool ?
In the HandleLSPResponse I had to remove the check for cursor movement (basically checking that the position of the cursor is still the one in hover_map) so the feature works. Maybe adding this 'fromBalloonExpr flag' in the hover_map would make us able to put the check back in just for ALEHover purposes ?

The whole inspiration for this idea came from Bram's termdebug plugin, which sets a local flag in balloonexpr and print/reset the flag accordingly in the callback

The goal of this flag is to make `:ALEHover` calls not pop a balloon under the cursor, since the user has probably no interest in their cursor while typing the command The flag is a default argument which is overridden only in ballonexpr call of ale#hover#Show, and stays set in the hover_map until the callback for the LSP handles it. There are no automated tests for this feature right now, and the nature of the addition (one optional argument in the API) should make it transparent to existing tests. Since the differentiation is now possible, the check for moved cursor has been put back in ale#hover#HandleLSPResponse

Using get() is safer than trying to access directly with ., as the tests show.

gagbo · 2018-05-11T18:18:26Z

The last commit raised the timeout on one failing appveyor test, since the output from the tests seemed to show that the linter was not finished when the check was called.

Feel free to revert it.

EDIT : Small gif to show the feature finalized (Notice how the mouse do not trigger the message in the command line, and how the :ALEHover call do not trigger the mouse balloon)
This is in gvim 8.0 (1-1806) and the tooltips go out of the frame because of the way I set the Peek window

w0rp · 2018-05-15T20:47:13Z

It sounds like you hit a similar issue to what I hit with Vim. Vim doesn't like you showing messages via :echo from an async context, but NeoVim is okay with it. It would show the message, then immediately vanish. The messages for diagnostics that are echoed work because the message echoing gets truncated, and some things can happen in a synchronous context, which came from data from an async context. Balloons are probably similar, knowing how Vim works.

w0rp

This is looking pretty damn good. Nice stuff. 👍

w0rp · 2018-05-15T20:49:46Z

autoload/ale/hover.vim

+" Currently, the callbacks displays the info
+" - in the balloon if requested from balloonexpr and balloon_show is detected
+" - as status message otherwise
+function! ale#hover#Show(buffer, line, col, ...) abort


Keeping track of the arguments will probably get a bit tedious as we go on. I think it's probably best to just write a single argument here instead, which contains all of the arguments in on Dictionary. That's probably the most future-proof option.

Optional arguments is my (lazy) way of making changes without having to delve too much in changing the tests to make them work, this is definitely something I want to make better. I'll implement the dictionary, and call it opt (for optional arguments).

The get() function should help with default values anyway.

w0rp · 2018-05-15T20:50:52Z

autoload/ale/hover.vim

-        \|| min([l:column, l:end]) isnot min([l:options.column, l:end])
-            " Cancel display the message if the cursor has moved.
-            return
+        if !get(l:options, 'hover_from_balloonexpr', 0)  " If the call did __not__


Move the comments on the RHS to lines above instead. That's my preferred style.

w0rp · 2018-05-15T20:54:07Z

autoload/ale/hover.vim

-            call ale#util#ShowMessage(a:response.body.displayString)
+            if get(l:options, 'hover_from_balloonexpr', 0)
+            \&& exists('*balloon_show')
+            \&& ale#Var(l:options.buffer, 'set_balloons')


We'll need to think about the meaning of the set_balloons setting. I guess it makes sense how you've done it here. I doubt many people really look at the balloons for the error messages at the moment.

I say we should do it how you've done it here, and update the documentation to say that the option also enables hover information for LSP and tsserver.

I might update the tsserver stuff myself, or you can give it a go if you want. If you install TypeScript into node_modules for a project, it should be pretty easy to get it running tsserver comes with TypeScript.

I assumed that the ale#hover callbacks were only called by ale#hover#Show, and therefore the value of set_ballons in this context is only relevant for the Hover information to begin with. I think this specific part of the code is not related to the actual design decision, it's really just a sanity check on what the user asked for relatively to balloons before we make it appear.

The part that makes the meaning of set_balloons matter in a "hover vs diagnostics" way is in ale#balloon#MessageForPos (where I decided that if diagnostics are found, show them and don't even try to get hover information for the position).

This optional dictionary has documentation just before the function using it, ale#hover#Show, and allows easier extension in the future.

gagbo · 2018-05-16T09:33:42Z

It sounds like you hit a similar issue to what I hit with Vim. Vim doesn't like you showing messages via :echo from an async context, but NeoVim is okay with it. It would show the message, then immediately vanish. The messages for diagnostics that are echoed work because the message echoing gets truncated, and some things can happen in a synchronous context, which came from data from an async context. Balloons are probably similar, knowing how Vim works.

I think the "nice" way to make this work if to call the whole ale#balloon#MessageForPos() asynchronously from balloonexpr function (this is what termdebug does). Currently, the balloonexpr either return directly a string (when there's a diagnostic) or calls the hover method that will create an async job which will eventually call balloon_show after balloonexpr returned '' in ale#balloon#MessageForPos().

But as far as I understand, it's not possible to create a job for vim functions, so I think the easy way is to leave it this way, and hope that async support for vim functions will happen some day (neovim can already create children processes can't it ?)

w0rp · 2018-05-16T20:23:44Z

Let's go with it, and get others to try it. Cheers! 🍻

gagbo added 3 commits May 6, 2018 00:38

Guard the ballooneval settings

c6a45b1

Mark main objectives to do to get nice Hover

288c791

Make tweaks to make the tooltip work - See " XXX: comments

d8c7a86

gagbo added 2 commits May 6, 2018 01:52

Guard balloon_show call

a86d494

Use return instead of finish for functions

d5bbb5e

w0rp reviewed May 8, 2018

View reviewed changes

gagbo added 4 commits May 8, 2018 20:20

ale#hover#show : Add optional arguments to specify arbtirary position

f4ab2b8

This change is requested to be able to call the function with mouse position to enable hover information in vim's balloon

ale#ballon#Disable : Remove feature guards

90c9a64

ale#balloon : Show 'ALEHover' output on balloon if no diagnostic found

daed3e3

ale#hover#HandleLSPResponse : remove the check for cursor position

c41e062

This check prevented the 'ALEHover in balloon' feature, since mouse position is almost never cursor position.

gagbo added 3 commits May 8, 2018 22:05

ale#balloon#MessageForPos : Change the return of balloonexpr

284ec91

balloonexpr evaluation now works even without balloon_show for basic diagnostics, leaving the balloon_show call to ale#hover#Show, which can then feature guard the call to avoid errors

ale#hover#Response : Feature guard balloon_show calls

0ca9747

ale#hover : always display 'Hover' information in messages

d5ba722

Also add a small comment to warn readers the different outputs the ale#hover#Show will write to

Merge remote-tracking branch 'upstream/master' into balloon_ALEHover

159a371

gagbo added 4 commits May 11, 2018 17:32

Merge remote-tracking branch 'upstream/master' into balloon_ALEHover

4a49a05

{LSP,TS}Response : use only variables from the Response

f22e7a9

It is clearer that we only rely on l:options to get the relevant data to build the LSP Response string

{LSP,TS}Response : Remove redundant checks for balloon_show call

e25b763

With the upcoming change in ale_set_balloons default value (see Pull Request dense-analysis#1565), this check will be useless

gagbo added 3 commits May 11, 2018 20:01

ale#hover#hover_map : Protect accesses to hover_map

e826b8b

Using get() is safer than trying to access directly with ., as the tests show.

Raise timeout to try to get Appveyor happy

7156ad4

gagbo mentioned this pull request May 11, 2018

Add some fancy GIFs for showing off ALE features #1558

Closed

gagbo changed the title ~~[WIP] TUI / GUI tooltip with content from ALEHover~~ TUI / GUI tooltip with content from ALEHover May 13, 2018

w0rp requested changes May 15, 2018

View reviewed changes

gagbo added 3 commits May 16, 2018 09:00

Merge remote-tracking branch 'upstream/master' into balloon_ALEHover

1cbc3d4

Review : Fix comments

32c50d4

Review : pass the optional argument 'called_from_balloonexpr' in a Dict

273ed54

This optional dictionary has documentation just before the function using it, ale#hover#Show, and allows easier extension in the future.

adriaanzon self-assigned this May 16, 2018

adriaanzon removed their assignment May 16, 2018

w0rp approved these changes May 16, 2018

View reviewed changes

w0rp merged commit 3a3c244 into dense-analysis:master May 16, 2018

gagbo deleted the balloon_ALEHover branch May 16, 2018 20:46

w0rp mentioned this pull request May 16, 2018

Update documentation for mouseover LSP hover support #1586

Closed

gagbo mentioned this pull request May 2, 2019

Balloons not functional in terminal vim 8.1.1073 #2442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TUI / GUI tooltip with content from ALEHover #1556

TUI / GUI tooltip with content from ALEHover #1556

gagbo commented May 5, 2018

gagbo commented May 5, 2018 •

edited

Loading

w0rp May 8, 2018

w0rp May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 11, 2018

gagbo commented May 11, 2018 •

edited

Loading

w0rp commented May 15, 2018

w0rp left a comment

w0rp May 15, 2018

gagbo May 16, 2018

w0rp May 15, 2018

w0rp May 15, 2018

gagbo May 16, 2018

gagbo commented May 16, 2018

w0rp commented May 16, 2018

		function! ale#balloon#Disable() abort
		set noballooneval balloonexpr=

TUI / GUI tooltip with content from ALEHover #1556

TUI / GUI tooltip with content from ALEHover #1556

Conversation

gagbo commented May 5, 2018

gagbo commented May 5, 2018 • edited Loading

w0rp May 8, 2018

Choose a reason for hiding this comment

w0rp May 8, 2018 • edited Loading

Choose a reason for hiding this comment

gagbo commented May 8, 2018 • edited Loading

gagbo commented May 8, 2018

gagbo commented May 8, 2018 • edited Loading

gagbo commented May 8, 2018

gagbo commented May 8, 2018 • edited Loading

gagbo commented May 11, 2018

gagbo commented May 11, 2018 • edited Loading

w0rp commented May 15, 2018

w0rp left a comment

Choose a reason for hiding this comment

w0rp May 15, 2018

Choose a reason for hiding this comment

gagbo May 16, 2018

Choose a reason for hiding this comment

w0rp May 15, 2018

Choose a reason for hiding this comment

w0rp May 15, 2018

Choose a reason for hiding this comment

gagbo May 16, 2018

Choose a reason for hiding this comment

gagbo commented May 16, 2018

w0rp commented May 16, 2018

gagbo commented May 5, 2018 •

edited

Loading

w0rp May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 8, 2018 •

edited

Loading

gagbo commented May 11, 2018 •

edited

Loading