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

Vim popup #3817

Merged
merged 3 commits into from Jul 23, 2021
Merged

Vim popup #3817

merged 3 commits into from Jul 23, 2021

Conversation

@benknoble
Copy link
Contributor

@benknoble benknoble commented Jul 14, 2021

This PR adds vim-popups as an alternative to neovim's floating windows for things like ALEHover.

Not sure about testing this, but it works! At least for ALEHover and cursor_detail.

Related: #3703

benknoble added 3 commits Jul 14, 2021
Details on implementation
-------------------------
- we make use of the |popupwin| api
- we split implementations (Nvim* vs. Vim* prefix) and call the right
  one based on has('nvim')
- we follow a similar structure in each function, using the relevant API
  - popup_list, win_execute, popup_settext in VimShow
  - popup_create in VimCreate
  - popup_close in VimClose

Some differences
----------------
- we DON'T have VimPrepareWindowContent because we use arguments to
  popup_create for borders, padding, etc., and it also takes care of
  buffer creation.
- we follow the protocol of setting and using w:preview for information,
  but we only need the ID
- InsertEnter is the only autocommand required, because of
  popup_create's moved argument. Any cursor movement with 'any' will
  close the popup. This in turns means VimClose is only called from
  InsertMode, so no mode-restoration necessary
- we don't tweak too much in the buffer because vim's popup buffers
  already have most relevant settings and aren't editable without
  calling popup functions.
- I enabled scrollbars, close buttons, dragging, and resizing
- vim popups get as big as they need to by default, so no worrying about
  truncating/hiding/size

Note: we might want to consider changing w:preview to w:ale_preview to
avoid clashes if someone else tries to use the same variable
@benknoble
Copy link
Contributor Author

@benknoble benknoble commented Jul 14, 2021

/cc @hsanson now that tests are passing.

Copy link
Contributor

@hsanson hsanson left a comment

The changes look good and I tested it does not break NeoVim. I do not have a recent version of Vim that support popupwin so I am trusting this works and will merge as is a requested feature by many.

@hsanson hsanson merged commit 530b38d into dense-analysis:master Jul 23, 2021
7 checks passed
7 checks passed
@github-actions
build_image
Details
@github-actions
test_ale (--vim-80-only)
Details
@github-actions
test_ale (--vim-82-only)
Details
@github-actions
test_ale (--neovim-02-only)
Details
@github-actions
test_ale (--neovim-04-only)
Details
@github-actions
test_ale (--linters-only)
Details
@appveyor
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants