Add nvim floating window support (replaces #3314) #3470

kevinclark · 2020-11-27T18:17:11Z

This continues the work begun in #3341 by @jargonzombies and responds to the feedback from @w0rp. In particular, it splits out configuration of the individual floating-window usecases and pulls that code into a separate file.

Known issues:

~~Needs tests. Since it's my first time working in viml, I wanted to make sure the basics were right before digging in here.~~ Done and passing as of 60287b0.
When floating preview coupled with g:ale_cursor_detail, the window is redrawn in a tight loop. Presumably in the non-floating version, moving into the preview window (via stay_here) keeps this from happening. But stay_here doesn't make sense in a floating-window world, so we may need another fix. This now fixed in c39a27c.

As far as I'm concerned this is ready to go. Just waiting on review and a merge.

Also: * Extract floating preview to its own file. * Ignore 'stay_here' option. Moving into the floating preview window seems confusing at best. * Re-use existing floating preview window if it's still up. * Flush out floating preview documentation. Known issues: * Needs tests. * When floating preview coupled with g:ale_cursor_detail, the window is redrawn in a tight loop. Presumably in the non-floating version, moving into the preview window (via stay_here) keeps this from happening.

kevinclark · 2020-11-27T18:20:18Z

@w0rp I'm going to work on tests next. I'd appreciate some direction re: the noted stay_here issue. @jargonzombies initial patch moved control into the floating preview window if stay_here was set, but it seems pretty jarring that control is suddenly in this tiny float. May or may not be obvious to a user that the float is a real buffer that can be window-closed or moved out of. Is there another way to stop the loop until CursorMoved,TabLeave,WinLeave,InsertEnter? (Struck because the stay_here issue is resolved).

Was still hitting race conditions where close happened mid call.

kevinclark · 2020-11-28T06:58:06Z

Currently considering progress on this blocked by #3448 since I can't run passing tests locally. Working on that first.

Floating previews open a new window, so when that window is written to, it moves briefly there at a different position than the original window. This makes repeated positions detected when positions are tracked at a s: level. Instead, we change the variable to window scoped, which only fires a message if the cursor has changed from the last position in *that window*.

kevinclark · 2020-12-13T21:13:37Z

Just an update here - I've got a PR in to fix #3448. It's waiting on review/feedback/merge. I'm happy to write the tests to finish this up, but wanted a basic "this seems like the right thing" sanity check before I go learn vader.

kevinclark · 2020-12-27T01:41:33Z

Still waiting on any kind of feedback. 🤷‍♂️

Just maintaining my own branch that I’m using daily in the meantime. Works fine afaict.

cj · 2020-12-29T20:07:38Z

@kevinclark with Rubocop, the floating preview is missing the option that caused the error:

kevinclark · 2020-12-29T20:35:15Z

@cj I’ll take a look. Would you mind supplying any ale options you’ve got set so I can repro?

cj · 2020-12-29T20:51:58Z

@kevinclark of course - https://gist.github.com/cj/39eeb5de63691f114f50832b444fdc3f, let me know if you need any thing else!

kevinclark · 2020-12-29T21:39:37Z

@cj Thanks! And just in case it matters (I don’t think it should) - what version of NeoVim?

cj · 2020-12-29T22:09:15Z

NVIM v0.5.0-dev+950-g5ce328df4
Build type: Release
LuaJIT 2.1.0-beta3

kevinclark · 2020-12-30T05:33:57Z

@cj While this shows up because of the floating code, it's actually unrelated. Looks like the preview window paths don't use the same formatting as the cursor detail paths.

In any case, it's a small change to make it consistent. I've tossed up a preliminary PR here. Will work up tests shortly.

dnaaun · 2020-12-30T17:41:06Z

Thanks for this @kevinclark !

So another nice-to-have would be g:ale_floating_cursor (in the vein of g:ale_virtualtext_cursor), which would automatically start a floating win when the cursor is on a line with an error/warning.

But I'm already very grateful for this PR! Maybe someone else (like me) should take this on after we get this PR merged in it's current form.

kevinclark · 2020-12-30T18:13:28Z

@davidatbu I think what you're looking for already exists. If you turn on g:ale_cursor_detail and g:ale_floating_preview then the cursor detail will use the floating window instead of the split preview. I'm running with this setting.

There is a slight difference in output, which @cj points out and I refer to in #3505 where the detail vs text output of the locline entry is used, but it was useful enough for me that I hadn't noticed.

And some cleanup of floating_preview test state

kevinclark · 2021-01-01T00:24:43Z

Open question (possibly for a follow up PR): when using ALEDocumentation, should floating windows be an option?

kevinclark · 2021-01-06T06:17:14Z

@hsanson Huh. That works fine for me. To clarify - when you say ale from git master - do you mean the ale from my branch? I hadn't merged from ale master in a little while, but I have now and it's still working fine. A little repetitive with g:ale_hover_cursor on (which I pulled from your ALEInfo and wouldn't use in this combination myself 🤷‍♂️), but working correctly AFAICT.

hsanson · 2021-01-06T06:37:10Z

Sorry, not master... I am using your branch:

git checkout -b kevinclark-nvim-floating master
git pull git://github.com/kevinclark/ale.git nvim-floating

nvim -v
NVIM v0.5.0-888-g8b2887bd5
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3
Compilation: /usr/bin/cc -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -O2 -g -Og -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wmissing-prototypes -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fno-common -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DMIN_LOG_LEVEL=3 -I/home/hs/Apps/neovim.git/build/config -I/home/hs/Apps/neovim.git/src -I/home/hs/Apps/neovim.git/.deps/usr/include -I/usr/include -I/home/hs/Apps/neovim.git/build/src/nvim/auto -I/home/hs/Apps/neovim.git/build/include
Compiled by hs@ex

Features: +acl +iconv +tui
See ":help feature-compile"

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/share/nvim"

Run :checkhealth for more info

kevinclark · 2021-01-06T06:41:10Z

@hsanson Got it. I'm on nvim 0.4.4 - I'll move to 0.5 and see if there's a change. Appreciate the responsiveness!

kevinclark · 2021-01-06T06:49:28Z

@hsanson Works with nightly 0.5 too. How odd. Are you seeing any test failures when you run them?

hsanson · 2021-01-06T07:15:57Z

Ok, so I tried with a minimal vim configuration and worked fine. After commenting line by line my config I found this particular setting causes the issue to occur:

set scrolloff=9999      " Always keep the cursor at the center of window.

Commenting this line solves the issue and the PR seems to work as expected. Unfortunately I am inclined to believe this configuration is common in the wild (see https://vim.fandom.com/wiki/Keep_your_cursor_centered_vertically_on_the_screen) so this PR can potentially break things to many people as it did for me.

Maybe is a bug in neovim itself that ALE has no way of working around. Would be good if someone can test this in vim to see if the same problem occurs.

kevinclark · 2021-01-07T04:27:19Z

@hsanson Thanks so much for tracking that down - I'll do some digging and see if I can't isolate the problem.

kevinclark · 2021-01-12T05:38:30Z

@hsanson So, with nightly nvim I can't get the branch to fail setting scroll off and I'm seeing test pass with this diff:

diff --git a/test/test_floating_preview.vader b/test/test_floating_preview.vader
index 43415556..46213641 100644
--- a/test/test_floating_preview.vader
+++ b/test/test_floating_preview.vader
@@ -1,4 +1,7 @@
 Before:
+  Save scrolloff
+  set scrolloff=9999
+
   let g:ale_floating_preview = 0
   let g:ale_hover_to_floating_preview = 0
   let g:ale_detail_to_floating_preview = 0

Kind of stumped. Does it blow up on that two line ruby file with that scrolloff or does it need to be something larger?

hsanson · 2021-01-12T09:11:49Z

@kevinclark I updated my neovim installation to latest master and the issue I had no longer occurs. There are many changes between my previous version and the latest one so is difficult to tell what was the problem.

I think this works as expected and can be merged but I also think @w0rp should check on this and merge it.

hsanson

Changes look good and tested it works as expected using latest neovim build.

kevinclark · 2021-01-12T17:22:09Z

@hsanson Thank you for the help!

w0rp

I just have a couple of comments here. Otherwise this is fine.

Sorry for not looking anything to do with ALE for a while. Given the times we are in and my day job, I haven't felt like doing too much with ALE for now.

w0rp · 2021-01-13T10:49:03Z

autoload/ale/floating_preview.vim

+    endif
+
+    " Remove the close autocmd so it doesn't happen mid update
+    augroup NvimFloating


We should prefix groups we create with ALE so we don't conflict with other groups.

w0rp · 2021-01-13T10:49:53Z

doc/ale.txt

+g:ale_floating_preview                                       *g:ale_floating_preview*
+
+  Type: |Number|
+  Default: `1`


The default is 0.

w0rp · 2021-01-13T10:54:00Z

@hsanson GitHub really needs a private message system some day. I've given you Admin permissions for the ALE repo, so if you couldn't merge something before, you definitely should be able to merge things now.

kevinclark · 2021-01-13T18:13:41Z

@w0rp @hsanson I've made those changes. Let me know if there's anything else needed to get this in.

Also - it'd make work going forward a lot easier if tests were portable. In master right now some of the testing tooling is from the host system instead of the docker container. I've fixed that in #3471. If I could get some attention there, I'd appreciate it.

kevinclark · 2021-01-14T18:08:02Z

Awesome, thank you!

puresick · 2021-02-08T14:19:29Z

Thank you for this feature PR - Great work!

jan-grimo and others added 4 commits November 27, 2020 10:11

Add nvim floating window hover support

b391247

Add configuration for float to replace preview

34706eb

preview#ShowFloating: qualify local variables

c7279b4

Make floating preview open more robust

6f95a7e

Was still hitting race conditions where close happened mid call.

kevinclark changed the title ~~Add nvim floating window support~~ Add nvim floating window support (replaces #3314) Nov 27, 2020

Fix viml lint issues

a960b40

kevinclark force-pushed the nvim-floating branch from eec1f07 to a960b40 Compare November 27, 2020 20:24

Fix help doc reverence to ale's preview window.

effed22

kevinclark mentioned this pull request Nov 28, 2020

Unit tests don't run on master via ./run_tests and docker #3448

Closed

kevinclark force-pushed the nvim-floating branch from c39a27c to 416d604 Compare November 28, 2020 21:23

kevinclark mentioned this pull request Dec 30, 2020

Use ale_echo_msg_format with cursor details #3505

Closed

kevinclark added 3 commits December 30, 2020 21:29

g:ale_floating_preview cleanup

fe0ade0

floating_preview: add ALEDetail tests

e964d1b

Fix fecs test missing runtime call

6066b1e

And some cleanup of floating_preview test state

Add ALEHover floating preview tests

60287b0

Merge remote-tracking branch 'upstream/master' into nvim-floating

bf7255f

Merge remote-tracking branch 'upstream/master' into nvim-floating

9067cd9

hsanson reviewed Jan 12, 2021

View reviewed changes

hsanson requested a review from w0rp January 12, 2021 09:12

hsanson added the enhancement label Jan 12, 2021

w0rp requested changes Jan 13, 2021

View reviewed changes

kevinclark added 3 commits January 13, 2021 09:46

Rename floating_preview autogroup

97231f7

Fix floating_preview doc - default is 0

53d22b1

Merge remote-tracking branch 'upstream/master' into nvim-floating

c8b6cc9

w0rp approved these changes Jan 14, 2021

View reviewed changes

w0rp merged commit 39f393e into dense-analysis:master Jan 14, 2021

kevinclark deleted the nvim-floating branch January 14, 2021 18:08

yen3 mentioned this pull request Feb 14, 2021

Add a border for floating preview window (with a POC patch) #3589

Closed

hsanson mentioned this pull request Jul 23, 2021

Balloons not functional in terminal vim #3800

Closed

derlaft mentioned this pull request Aug 28, 2021

Support neovim floatwindow #2418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nvim floating window support (replaces #3314) #3470

Add nvim floating window support (replaces #3314) #3470

kevinclark commented Nov 27, 2020 •

edited

Loading

kevinclark commented Nov 27, 2020 •

edited

Loading

kevinclark commented Nov 28, 2020

kevinclark commented Dec 13, 2020

kevinclark commented Dec 27, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 29, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 29, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 30, 2020

dnaaun commented Dec 30, 2020

kevinclark commented Dec 30, 2020

kevinclark commented Jan 1, 2021

kevinclark commented Jan 6, 2021 •

edited

Loading

hsanson commented Jan 6, 2021

kevinclark commented Jan 6, 2021

kevinclark commented Jan 6, 2021

hsanson commented Jan 6, 2021

kevinclark commented Jan 7, 2021

kevinclark commented Jan 12, 2021

hsanson commented Jan 12, 2021

hsanson left a comment

kevinclark commented Jan 12, 2021

w0rp left a comment

w0rp Jan 13, 2021

w0rp Jan 13, 2021

w0rp commented Jan 13, 2021

kevinclark commented Jan 13, 2021

kevinclark commented Jan 14, 2021

puresick commented Feb 8, 2021

Add nvim floating window support (replaces #3314) #3470

Add nvim floating window support (replaces #3314) #3470

Conversation

kevinclark commented Nov 27, 2020 • edited Loading

kevinclark commented Nov 27, 2020 • edited Loading

kevinclark commented Nov 28, 2020

kevinclark commented Dec 13, 2020

kevinclark commented Dec 27, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 29, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 29, 2020

cj commented Dec 29, 2020

kevinclark commented Dec 30, 2020

dnaaun commented Dec 30, 2020

kevinclark commented Dec 30, 2020

kevinclark commented Jan 1, 2021

kevinclark commented Jan 6, 2021 • edited Loading

hsanson commented Jan 6, 2021

kevinclark commented Jan 6, 2021

kevinclark commented Jan 6, 2021

hsanson commented Jan 6, 2021

kevinclark commented Jan 7, 2021

kevinclark commented Jan 12, 2021

hsanson commented Jan 12, 2021

hsanson left a comment

Choose a reason for hiding this comment

kevinclark commented Jan 12, 2021

w0rp left a comment

Choose a reason for hiding this comment

w0rp Jan 13, 2021

Choose a reason for hiding this comment

w0rp Jan 13, 2021

Choose a reason for hiding this comment

w0rp commented Jan 13, 2021

kevinclark commented Jan 13, 2021

kevinclark commented Jan 14, 2021

puresick commented Feb 8, 2021

kevinclark commented Nov 27, 2020 •

edited

Loading

kevinclark commented Nov 27, 2020 •

edited

Loading

kevinclark commented Jan 6, 2021 •

edited

Loading