Skip to content

[RFC] Floating windows in TUI and Remote UI#6619

Merged
bfredl merged 1 commit into
neovim:masterfrom
bfredl:floating
Mar 2, 2019
Merged

[RFC] Floating windows in TUI and Remote UI#6619
bfredl merged 1 commit into
neovim:masterfrom
bfredl:floating

Conversation

@bfredl

@bfredl bfredl commented Apr 29, 2017

Copy link
Copy Markdown
Member

From the drawer of very naive and crazy stuff (one does not simply mess with window positioning and expect stuff to work). Partially based on @dzhou121 external ui stuff (crazy/naiveness mine)

do this in a script:

fnew
call nvim_win_float_set_pos(0,5,10,20,5)
hi Floating guibg=#000044
set winhl=Normal:Floating
set nonumber

@justinmk justinmk added this to the 0.3 milestone Apr 29, 2017
@marvim marvim added the WIP label Apr 29, 2017
Comment thread src/nvim/ex_cmds.lua Outdated
@bfredl bfredl changed the title [WIP] (very WIP) Floating windows in TUI [WIP] Floating windows in TUI Apr 30, 2017
@johnzeng

johnzeng commented Jun 5, 2017

Copy link
Copy Markdown

hi, will this PR be merged? or, still wip?

@bfredl

bfredl commented Jun 5, 2017

Copy link
Copy Markdown
Member Author

Is still WIP, many window commands can easily segfault, and the mouse is broken. Also we need a better redrawing strategy than "redraw the float everytime anything else is redrawn". Probabaly should use some buffering logic from #5686

@johnzeng

johnzeng commented Jun 5, 2017

Copy link
Copy Markdown

Good to know that. I am waiting for it.

@bfredl

bfredl commented Aug 13, 2017

Copy link
Copy Markdown
Member Author

Update: replaced fnew with api command:

Window nvim_open_floating_window(Buffer buffer,
                                 Integer x, Integer y, Integer w, Integer h,
                                 Integer mode, Boolean enter, Error *err)

mode supports anchor modes (will be relevant for UIs with different grid sizes in floats) as well making the window "unfocusable". It is still possible to make it current with api or number, but the user selecting it "by accident" with mouse or ctrl-Wctrl-W should be avoided.

Also added very basic mouse support.

@justinmk

justinmk commented Aug 13, 2017

Copy link
Copy Markdown
Member

I'd suggest "new" as the standard verb for creating things. Also in API functions we always spell "window" as win.

nvim_new_floating_win

@bfredl

bfredl commented Aug 13, 2017

Copy link
Copy Markdown
Member Author

new isn't verb though. The alternative would be add, but open signals something more heavy (as a window or channel is) than something you just add to a set like a line or a highlight. create might also work and is slightly more general (like nvim_create_namespace )

@justinmk

Copy link
Copy Markdown
Member

new acts like a verb in many programming languages. It has wide recognition, it's short, strong, and familiar.

@bfredl

bfredl commented Aug 13, 2017

Copy link
Copy Markdown
Member Author

My point was primarily about operation strength, not grammar. Opening/Creating a new window or buffer (or channel) is a lot heavier than adding a new line (we call it [o]pen only because [n] was taken for [n]ext) or highlight, or allocating a new object in "many programming languages". These are all different verbs for a reason. (and if we're discussing common languages, new string() vs string() in javascript is just madness, while in modern c++ well-typed pointers is preferred to new/delete)

@bfredl

bfredl commented Sep 2, 2017

Copy link
Copy Markdown
Member Author

Added nvim_add_buf(). Note it only adds a buffer to the buffer list, they are not opened/allocated until they are displayed in some window.

let b = nvim_add_buf("")
let w =  nvim_open_floating_window(b,5,10,20,5,0,0)
hi Floating guibg=#000044
call setwinvar(w, '&winhl', 'Normal:Floating')
call setwinvar(w, '&number', 0)

@wsdjeg

wsdjeg commented Sep 2, 2017

Copy link
Copy Markdown
Contributor

This PR is awesome, and I have a question about the floating window, I just see the doc what dese Integer x, Integer y, Integer w, Integer h, base on? computer's screen or vim's screen?

Comment thread src/nvim/api/vim.c Outdated

Window nvim_open_floating_window(Buffer buffer,
Integer x, Integer y, Integer w, Integer h,
Integer mode, Boolean enter, Error *err)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nvim_open_floating_win

@justinmk

justinmk commented Sep 2, 2017

Copy link
Copy Markdown
Member

nvim_add_buf

Here "new" as the verb makes sense to me: nvim_new_buf(). We're creating an object. The buffer list is like a vtable (an implementation detail) so "add" isn't the characteristic operation.

@bfredl

bfredl commented Sep 2, 2017

Copy link
Copy Markdown
Member Author

The buffer list most definitely not an implementation detail: it is well known and used vim functionality that a buffer can be "listed" without actually being "opened". The API functions current operation is only to add/return a number in the buffer list, but "add" might not be the best either, as it can return an existing buffer.

However, that is not necessarily the best operation, we could have a function that only creates a new (unnamed) buffer and fully allocates it (this special case might not need to make it temporarily displayed, as there is no swapfile, encoding errors, and probably we want no autocmds either) The current behavior makes sense in the "floating window" case given that it will immediately opened in a window anyway, at which point all the vim stuff happens.

Comment thread src/nvim/buffer_defs.h Outdated
Comment thread src/nvim/buffer_defs.h Outdated
Comment thread src/nvim/screen.c Outdated
@ZyX-I

ZyX-I commented Sep 2, 2017

Copy link
Copy Markdown
Contributor

I have a suggestion: either make x and y Float and allow negative and fractional values, or add another argument which specifies what x and y are related to and what they actually mean. Neither makes sense for TUI, but GUI may open windows outside of the window.

Or do the other thing: open_floating_window already contains lots of arguments. Make that dict:

/// Open floating window
///
/// @param[in]  buffer  Buffer to display in that window.
/// @param[in]  position  Window position, pair of two integers which may be 
/// negative.
/// @param[in]  options  Additional options:
///                      Option | Type | Description
///                      ------ | ---- | -----------
///                      anchor | String | Make position be relative to the 
///                             |        | given anchor: "type-pos" where "type"
///                             |        | is one of "screen", "editor_area",
///                             |        | "wm_window" and "pos" is one of "NW",
///                             |        | "NE", "SW" or "SE"; additionally
///                             |        | "cursor" position is supported.
///                             |        |
///                             |        | Defaults to "cursor".
///                             |        |
///                             |        | TUI only supports "editor_area-…" and
///                             |        | "cursor".
///                      dimensions | pair of Integers | Dimensions in screen
///                                 |                  | cells, width x height.
///                                 |                  |
///                                 |                  | Defaults to either just
///                                 |                  | enough to show the
///                                 |                  | whole buffer contents
///                                 |                  | (not updated when
///                                 |                  | buffer contents
///                                 |                  | changes, call
///                                 |                  | `nvim_reconfigure_floating_window({dimensions: NIL})`
///                                 |                  | then), or just enough
///                                 |                  | to occupy all available
///                                 |                  | space, whichever is
///                                 |                  | lesser.
///                      standalone | Boolean | Make window standalone.
///                                 |         |
///                                 |         | Defaults to false, may be
///                                 |         | ignored in TUI.
///                      focusable | Boolean | Make window not focusable, and
///                                |         | also ignore mouse events.
///                                |         |
///                                |         | Defaults to false.
///                      position_type | String | Position type: either "cells"
///                                    |        | (position in display cells) or
///                                    |        | "pixels".
///                                    |        |
///                                    |        | Defaults to "cells" for
///                                    |        | "editor_area-*" and "cursor"
///                                    |        | anchors, "pixels" otherwise.
Window nvim_open_floating_window(Buffer buffer, ArrayOf(Integer) position,
                                 Dictionary options, Error *const err)

/// Reconfigure floating window
///
/// @param[in]  window  Window to reconfigure.
/// @param[in]  options  @see nvim_open_floating_window()
///                      Missing values mean “do not change option”, use NIL to 
///                      force defaults.
void nvim_reconfigure_floating_window(Window window, Dictionary options,
                                      Error *const err)

@justinmk

justinmk commented Sep 2, 2017

Copy link
Copy Markdown
Member

The buffer list most definitely not an implementation detail: it is well known and used vim functionality

It's an irrelevant detail in the context of the API. When one creates a buffer one does not think "I want to add a buffer to the buffer list", only "I want a new buffer".

@bfredl

bfredl commented Sep 2, 2017

Copy link
Copy Markdown
Member Author

Again, the name describes what the function does right now, which was the simplest thing to implement. The relevant change would be to change what the function actually does (fully allocate a buffer), which will be more involved but probably worth it in the long run.

@jamessan

jamessan commented Feb 4, 2019

Copy link
Copy Markdown
Member
* Window could have a single dict option like this:
  ```
  :setlocal float={'unfocusable':0,'standalone':0,...}
  ```

Wouldn't this be the first dict option? What's wrong with using the canonical comma-separated, colon-delimited format for other complex options?

Typing a dict for an option is going to be error-prone, since people are likely to want to insert whitespace but that's not allowed (without going \-happy) for :set

@justinmk

justinmk commented Feb 4, 2019

Copy link
Copy Markdown
Member

Wouldn't this be the first dict option? What's wrong with using the canonical comma-separated, colon-delimited format for other complex options?

Doesn't matter too much in this context, unless it turns out to be easier to implement. Anyways, let's save that discussion for later.

@bfredl

bfredl commented Feb 5, 2019

Copy link
Copy Markdown
Member Author

Do floats deal with overlap explicitly or only accidentally? Is it feasible to deal with overlap in future PR (if so I'll create a ticket)? Something like a z-index?

There is a z-index internally, but it is not updated for anything yet. Bringing focused float to front makes sense to start with, I will add it right away.

Window could have a single dict option like this ... or comma separated list:

This means that the plugin has to use string interpolation to configure the float, rather than construct the dict using the native language construct i e {anchor='NW', relative='buffer', lnum=pos[1], col=pos[2], unfocusable=true} in lua. Also the semantics would be weird with setl; we could disable setl for the option i e window-only local option, but that would be yet a new option scope kind[1]. Possibly there could be single option for quick checking the mode like float / nofloat, or maybe even float=standalone (or whatever we call it), but the concrete configured position is not an option, just as the current split layout is not an option (though there is some options that constrain it).

[1] Unless it already exists some option like that, wouldn't surprise me anymore.

@justinmk

justinmk commented Feb 9, 2019

Copy link
Copy Markdown
Member

but the concrete configured position is not an option, just as the current split layout is not an option

That's true, window layout is not a "window option". Though it could be viewed as a "tab option" (where "option" is understood as Vim-parlance for "property").

But it may be awkward to force "layout" it into the "options" model, and it shouldn't block this.

At the very least I hope we can avoid treating "floating windows", "flex windows", etc., as different classes of things, and instead stick with just windows which have layout properties. So nvim_win_config would operate on any window, we wouldn't have a separate nvim_win_config_float, nvim_win_config_xx, etc.

Does this seem feasible?

@bfredl

bfredl commented Feb 10, 2019

Copy link
Copy Markdown
Member Author

Yes, general nvim_open_win and nvim_win_config functions as we discussed make sense, but only supporting floats initially in this PR.

Comment thread src/nvim/api/window.c Outdated
/// See documentation at |nvim_open_float_win|, for the meaning of parameters.
///
/// When reconfiuring an existing float, absent option keys will not be
/// changed. The following restriction apply though: `x`, `y` and `relative`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is row and col instead of x, y at documentation of nvim_open_float_win, so what should be used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

row, col. I'm in the process of updating it

@chemzqm

chemzqm commented Feb 18, 2019

Copy link
Copy Markdown
Contributor

Can't move floating window without change content of buffer:

let s:buf = nvim_create_buf(1)

let s:win = nvim_open_float_win(s:buf, v:false, 5, 5, {
      \ 'col': 10,
      \ 'row': 1,
      \ 'unfocusable': v:true,
      \ 'standalone': v:true,
      \})
call nvim_buf_set_lines(s:buf, 0, -1, v:false, ['a', 'b', 'c', 'd', 'e'])
call setwinvar(s:win, '&signcolumn', 'no')
call setwinvar(s:win, '&number', 0)
call setwinvar(s:win, '&relativenumber', 0)
call setwinvar(s:win, '&winhl', 'NormalNC:Pmenu')

function! s:config()
  "call nvim_buf_set_lines(s:buf, 0, -1, v:false, ['a', 'b', 'c', '', ''])
  call nvim_win_config_float(s:win, 5, 5, {
        \ 'col': 11,
        \ 'row': 1,
        \})
endfunction

call timer_start(1000, {-> s:config()})

source these script with nvim -u NORC, the first column would disappear, it works as expected when content of buffer changed.

@bfredl

bfredl commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

@chemzqm Thanks for testing! Redrawing is still a bit unreliable, I probably messed up something in latest rebase. Hopefully I will have some time in the weekend to patch things up.

@bfredl

bfredl commented Feb 24, 2019

Copy link
Copy Markdown
Member Author

Updated the API (and docs) to use non-float specific names. Added 'window' relative positioning, which is needed for multigrid support. The end goal is to support lnum+col positioning, though that looks a bit messy, so it will be a follow up PR. For now cursor relative positioning works (i e one can use row=1 and col=0 to show float right below cursor), and multigrid client will receive grid-relative position when using relative=cursor.

Tests now use proper multigrid representation. Still a few failures due to leaks and missing checks (external window should only be valid with multigrid).

Comment thread src/nvim/api/vim.c
/// Exactly one of `external` and `relative` must be specified.
///
/// @param buffer handle of buffer to be displayed in the window
/// @param enter whether the window should be entered (made the current window)

@justinmk justinmk Feb 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

focus (used in FocusGained/Lost)is more common.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

focus as opposed to what? "current" is our jargon for current window (nvim_get/set_current_xx). "enter" is our jargon for entering a window/buffer (BufEnter / WinEnter).

Comment thread src/nvim/api/vim.c Outdated
/// "SW" south-west
/// "SE" south-east
/// `unfocusable`: The window will not be focused by wincmds and
/// mouse events. It can still be made current by API calls.

@justinmk justinmk Feb 24, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Negative logic is confusing, so focusable is a better name. It's ok if the default is true.

It can still be made current by API calls.

Does "made current" mean "focused"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

"made current" means "made the current window", "entered" would be established verb for this. "focused" means entered due to user input being directed towards the window, i e by clicking inside the window.

Comment thread src/nvim/api/window.c Outdated
/// floating and external windows (including changing a split window to these
/// types).
///
/// See documentation at |nvim_open_window|, for the meaning of parameters.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tag |nvim_open_window| won't exist, should be |nvim_open_window()|

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// See documentation at |nvim_open_window|, for the meaning of parameters.
/// See documentation at |nvim_open_win()|, for the meaning of parameters.

s/window/win/

Comment thread src/nvim/screen.c Outdated

FOR_ALL_WINDOWS_IN_TAB(wp, curtab) {
if (wp->w_redr_type == CLEAR && wp->w_floating && wp->w_grid.chars) {
// TODO(bfredl): this is overly expensive. The solution is to CLEAR less

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could add a field to g_stats so we have a counter of CLEARs which could be (eventually) tested.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It could be this TODO was from before the clearmaggedon. Now basically only resize and user intervention should lead to a clear.

assert(method == 'redraw')
for _, update in ipairs(args) do
print(require('inspect')(update))
if update[1] ~= "mode_info_set" then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this because mode_info_set is noisy? Would be good to add a comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

indeed.

@Shougo

Shougo commented Feb 27, 2019

Copy link
Copy Markdown
Contributor

Floating window support is here in denite.nvim.

Shougo/denite.nvim#580

@wsdjeg

wsdjeg commented Feb 27, 2019

Copy link
Copy Markdown
Contributor

I will also add floating windows support in flygrep :), but the api seems has been changed.

wsdjeg/SpaceVim#2216

@chemzqm

chemzqm commented Feb 28, 2019

Copy link
Copy Markdown
Contributor

Could floating window support optional border?
I can draw the border as buffer content, but it doesn't looks well when the buffer contains more lines than window height?

@bfredl

bfredl commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

Yes, but that would be a follow-up PR. winhl=Normal:GroupWithBG works for now to make the float stand out.

@bfredl

bfredl commented Feb 28, 2019

Copy link
Copy Markdown
Member Author

Implemented basic z-order: if float becomes current window, it will be put on top, with the exception of popupmenu which always is on top if shown. It can be made more fancy later on.

@chemzqm

chemzqm commented Mar 2, 2019

Copy link
Copy Markdown
Contributor

I think a function for close preview window or close window without focus is needed.

I can close the preview window by wincmd c but it requries focus the preview window, the problem is when the user have selected region at this time the change of focused window break current selection.

@bfredl

bfredl commented Mar 2, 2019

Copy link
Copy Markdown
Member Author

:[winnr]close can be used, but it is not very convenient. We could add nvim_win_close([winid]) which works the same but with winid.

@bfredl

bfredl commented Mar 2, 2019

Copy link
Copy Markdown
Member Author

Working on the last issues, ci build is aalmost passing now. I plan to merge this today or tomorrow. It is usable enough as a MVP (as demonstrated by plugins adding support already). The API/functionality (and tests with them) can be expanded in follow-up PRs.

Co-Author: Dongdong Zhou <dzhou121@gmail.com>
@bfredl

bfredl commented Mar 2, 2019

Copy link
Copy Markdown
Member Author

Merged. Sorry for taking so long. I will lock this thread, as dealing with overlong github threads is a mess. Requests for additions/improvements (or bugs too minor to deserve their own issue) can be in #9421. Feel free to repeat stuff that is hidden somewhere in the middle of this thread and still are a concern.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tui termcodes, terminfo, termcap ui-extensibility UI extensibility, events, protocol, externalized UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.