Skip to content

Conversation

bfredl
Copy link
Member

@bfredl bfredl commented Apr 2, 2018

Some changes I think it will make sense to group together as a new revision of the "grid protocol". This refers mostly to the ui-grid section of ui.txt, other parts of the ui protocol is not expected to change much.

This branch is currently a mess, but I prefer presenting messy code than vaporwave... It will be cleaned up and test added etc after some discussion this is the direction we want to move in.

For backwards compatibility api/ui.c should translate back to the present protocol, unless clients opt-in. All internal structures such as bridge, TUI, the "compositor" in #6619 should use the new format.

  • For clients that opt-in to this revision, we can remove deprecated events, such as update_fg, eol_clear etc.
  • Nominally support multiple grids. Enabling this revision shouldn't per-se cause external clients to see any more grids. But grid events should be updated as needed for multi-grid support. Multi-grid is an abstraction which can be used by several concrete features (where floating windows which will have its own ext_float flag, will be the first)
  • support sending an entire updated screen line at a time. Or rather, the largest damaged region of each line. Also supports clearing to an arbitrary column with non-normal background (and thus replaces severely limited eol_clear).
  • define highlight groups in advance, and later just send numeric ids. Always sends both rgb and cterm colors, rgb flag thus has no effect.
  • (Separate) opt in: allow UIs to inspect the "semantic" highlight state. UI highlights like Number will be identified as such, and UIs need not guess it from color values or positions. (current representation is very detailed, we might not want this level of detail, but I wanted to see how far it could go...)
  • But if the UI doesn't opt in, it should only see highlight ids with unique colors (should also apply to TUI, to reduce spurious updates)
  • Changing the Normal color shouldn't cause clearing the screen. The UI already has all necessary information to repaint the screen. Not a very impactful optimization as such, but this convention will be used later to support efficient highlighting overlays.

It probably makes sense to mark this revision as experimental/unstable in the first PR that introduces it and use some unwieldy name as ext_multigrid_experimental, and recommend most external UIs to stick with the present format for now. A good reference point for stabilizing it is the same time as merging the floats PR #6619, as then there will be a strong reason for clients to actually use it. We should see this an opportunity to fix the issues/limitations of the UI protocol that has accumulated over time. Moving forward we only need to support the current format, and this new revision when we are happy with it.

Depends on: #7992

@bfredl
Copy link
Member Author

bfredl commented Apr 2, 2018

Ref #7723 (pre-registered colors)
#6459 (more efficient line representation)
#8006 (forward highlight groups)

@bfredl bfredl added refactor changes that are not features or bugfixes ui ui-extensibility UI extensibility, events, protocol, externalized UI labels Apr 2, 2018
@marvim marvim added the WIP label Apr 2, 2018
@bfredl bfredl force-pushed the hlstate branch 5 times, most recently from 7912dd6 to 1b85857 Compare April 8, 2018 09:53
@bfredl
Copy link
Member Author

bfredl commented Apr 8, 2018

Cleaned up the branch a little.

Concretely, here are a sketch new and changed events in the revised grid protocol. Names and such are meant to be bikeshedded (and not 100% consistent with the branches either).

hl_attr_define(integer id, Dictionary attr, Array info)
Tell UI that than a highlight with id was added to the highlight table. The attr dict has similar format to the old highlight_set, except that both rgb and cterm colors are always available. info is empty by default.

hl_attr_set(integer id)
Works like highlight_set, except clients should look up the id in an earlier hl_attr_define event.

grid_line(Integer grid_id, Integer row, Integer col_start, Array cells)
Tell the UI that a chuck of cells starting at col_start of row on grid grid_id was changed. cells is an array of [text, hl_id, repeat] each describing the UTF-8 text and highlight id (as above) of this cells. Right cells of doublewidth chars will be the emtpy string (unlike "empty" cells, which contains space char). If hl_id is not present the last sent hl_id should be used (but it is always sent for the first cell in a grid_line event). If repeat is present, the cell should be repeated repeat times including the first time. (Doublewith chars are never repeated.)

grid_resize(grid_id, rows, cols) (replacing resize)
Resize grid with grid_id. If grid_id wasn't seen by the client before, a new grid is being created with this size. Grid 1 is the "global" grid used by default. The purpose of grids beyond that will be defined by further extensions/events. Thus, just activating ext_multigrid will not cause the client to "see" any grids it cannot handle.

grid_destroy(grid_id)
grid_id will not be used anymore and the client can free all data associated with it.

grid_cursor_goto(grid_id, row, column) (replacing cursor_goto)
Makes grid grid_id current and row,column the cursor position. scrolling will be done on the current grid. (changed) This event will only be sent at the end of aredraw batch and indicate the focused grid (unlike how the cursor is managed now)

The following events are redundant with these additions and will not be sent in the new mode:
eol_clear (line can do this more generally)
update_fg (use default_colors_set)
update_bg
update_sp
highlight_set
put

@justinmk justinmk added this to the 0.3.1 milestone Apr 8, 2018
@justinmk
Copy link
Member

justinmk commented Apr 8, 2018

Would this also be the right time to separate the "pen" from the "edit cursor", i.e. disambiguate cursor_goto.

@bfredl
Copy link
Member Author

bfredl commented Apr 8, 2018

@justinmk That would be done by never using ui_put and always use ui_line. Currently, use of ui_put means code that haven't been refactored yet.

The lazyredraw-y approach would be to keep track of largest changed region of each line, and transmit that only at ui_flush. That could be messy with scrolling though. I will first try to convert most stuff to ui_line in a "semantic" way. (which also will need keep track of dirty regions, but only for a single line in a well defined context, and not globally)

@bfredl
Copy link
Member Author

bfredl commented Apr 8, 2018

Though then scroll will still depend on the cursor grid, but we could change it to grid_scroll(grid_id, row, col, width, height, scroll_rows, scroll_cols) as scrolling region is always defined prior to scroll anyway.

@bfredl bfredl force-pushed the hlstate branch 2 times, most recently from 6b2a2db to 7aed5c6 Compare April 9, 2018 17:51
@bfredl
Copy link
Member Author

bfredl commented Apr 9, 2018

@justinmk This is now 85% done. cursor_goto is only sent once at the end of a batch, and indicates the final position. The remaining 15% is to make sure the cursor position is actually correct, especially with messages as cursor position is almost never set explicitly but rather implicitly by the last put char. Thankfully we happen to have a large body of screen tests checking the intended position...

In addition, ui_put and ui_set_highlight are removed from the internal protocol as they are now redundant. api/ui.c is still emulating these events for existing remote UIs, which includes the present screen.lua implementation (which thus serves as test for the translation).

@bfredl bfredl force-pushed the hlstate branch 5 times, most recently from e40d27f to 158d351 Compare April 11, 2018 18:01
@bfredl
Copy link
Member Author

bfredl commented Apr 11, 2018

Now stateful cursor should work. Though one place it works slightly different is with ext_cmdline, it now didn't move the cursor to the cmdline row in some cases, but I think this is an actual improvement. I'd try make sure this new behavior is consistent, even with redrawing screen in cmdline mode.

At least functional/ui tests now pass (with the change of ext_cmdline behavior)

@bfredl
Copy link
Member Author

bfredl commented Apr 11, 2018

Also the behavior of writedelay is changed, the delay is now per line update, which at least makes sense while developing this branch. The same UI component should ideally only use one line update per line. At least window line, statusline-ish things and pum now follow this.

Messages are more tricky, as messages do treat the screen as a "dumb" terminal semantically. But I think we can live with that for now. (it is still not one line update per char, more like one per logical item)

@bfredl bfredl force-pushed the hlstate branch 3 times, most recently from a185ad4 to a3fbf63 Compare April 11, 2018 18:50
@@ -1562,3 +1563,15 @@ Object nvim_get_proc(Integer pid, Error *err)
#endif
return rvobj;
}

Array nvim__inspect(Integer row, Integer col)
Copy link
Member

Choose a reason for hiding this comment

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

though this is "private", it could still benefit from a more intuitive name, maybe nvim__screen_inspect or nvim__ui_inspect

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it is mostly for my own debugging, it will be made redundant or replaced with something better.

@bfredl
Copy link
Member Author

bfredl commented Apr 13, 2018

Scrolling is now stateless. Stateful scrolling regions was never actually used for any good, and more an arbitrary artefact of how the code in screen.c was organized.

bfredl added 3 commits July 21, 2018 13:21
Add ext_newgrid and ext_hlstate extensions. These use predefined
highlights and line-segment based updates, for efficiency and
simplicity.. The ext_hlstate extension in addition allows semantic
identification of builtin and syntax highlights.

Reimplement the old char-based updates in the remote UI layer, for
compatibility. For the moment, this is still the default. The bulitin
TUI uses the new line-based protocol.

cmdline uses curwin cursor position when ext_cmdline is active.
@blueyed
Copy link
Contributor

blueyed commented Jul 26, 2018

I am seeing a segfault together with Neomake due to this: 989b585#commitcomment-29838744

@bfredl
Copy link
Member Author

bfredl commented Jul 26, 2018

Yeah a null check is needed, tough one was needed even before this PR (for anyone clearing the cursorline syntax group). Or maybe it should just be refactored to per value already, there is nothing wrong with returning a struct.

@aktau
Copy link
Contributor

aktau commented Jul 26, 2018

I think I'm seeing what @blueyed is seeing too. Here's my backtrace: https://gist.githubusercontent.com/aktau/8b97030d5385087ea5ee249d9082a2b5/raw/3d8595dbd8be585ef78019366880731419b085ab/backtrace.log. Which looks similar to @blueyed's backtrace, although mine is much (much) larger (because of what some script doest). I'm not sure which script is causing it. It's got something to do with split screens and signs/columns I think.

@bfredl
Copy link
Member Author

bfredl commented Jul 26, 2018

#8789 might fix it. I will try to reproduce tomorrow, at least something similar (:sign right after hi clear Statusline might be enough, but it might be a bit contrived)

bfredl added a commit to bfredl/neovim that referenced this pull request Aug 2, 2018
bfredl added a commit to bfredl/neovim that referenced this pull request Aug 10, 2018
bfredl added a commit to bfredl/neovim that referenced this pull request Aug 14, 2018
bfredl added a commit to bfredl/neovim that referenced this pull request Aug 26, 2018
Make sure cmdline updates will receive highlight specifications the same
way as screen cells. This is controlled by the ext_newgrid option so
nothing is changed by default (as screen cells are also not changed by
default).  This was already done for the cmdline itself in neovim#8221, this
extends it to cmdline_block. Which currently doesn't store highlights,
but the placeholder should be one that makes sense for future use.
bfredl added a commit to bfredl/neovim that referenced this pull request Aug 27, 2018
Make sure cmdline updates will receive highlight specifications the same
way as screen cells. This is controlled by the ext_newgrid option so
nothing is changed by default (as screen cells are also not changed by
default).  This was already done for the cmdline itself in neovim#8221, this
extends it to cmdline_block. Which currently doesn't store highlights,
but the placeholder should be one that makes sense for future use.
@bfredl bfredl mentioned this pull request Oct 2, 2018
bfredl added a commit to bfredl/neovim that referenced this pull request Oct 6, 2018
The old code could lead to a memory error in the following situation:

0. The previous cursor position was row 50 since before, on a grid
larger than 50 rows.
1. grid_resize changes the grid height to 40, and invalidly assumes the
resize moved the physical cursor to row 0
2. Some event used a operation that could move the cursor (such as clear), and
then reset the cursor to the "true" position row 50 (pointless after neovim#8221, but
I forgot to remove it)
3. raw_line/cheap_to_print is invoked, and tries to inspect the grid at
row 50 (memory error)
4. grid_cursor_goto would have been called at this point, and set a
valid cursor position 0-39.
justinmk added a commit to justinmk/neovim that referenced this pull request Dec 30, 2018
Besides the "visible" improvements, this release features numerous
internal improvements to the UI/screen code and test infrastructure.

Numerous patches were merged from Vim, which are not mentioned below.

FEATURES:

07ad5d7 clipboard: Support custom VimL functions neovim#9304
725da1f neovim#9401 win/TUI: Improve terminal/console support
7a8dadb neovim#9077 startup: Use $XDG_CONFIG_DIRS/nvim/sysinit.vim if it exists
feec926 neovim#9299 support <cmd> mapping in more places
0653ed6 neovim#9028 diff/highlight: Show underline for low-priority CursorLine
bddcbbb signs: Add "numhl" argument neovim#9113
05f9c7c clipboard: support Wayland (neovim#9230)
14ae394 neovim#9052 TUI: add support for undercurl and underline color
4fa3492 neovim#9023 man.vim: soft (dynamic) wrap neovim#9023

API:

8b39e4e neovim#6920 API: implement object namespaces
b1aaa0a API: Implement nvim_win_set_buf() neovim#9100
8de87c7 neovim#8180 API: virtual text annotations (nvim_buf_set_virtual_text)
2b9fc9a neovim#8660 API: add nvim_buf_is_loaded()
    API: buf_get_lines, buf_line_count handle unloaded buffers
88f77c2 API: nvim_buf_get_offset_for_line
94841e5 API/UI: neovim#8221 ext_newgrid, ext_hlstate
    (use line-based rather than char-based updates)

UI

b5cfac0 neovim#8806 TUI: use BCE again more often, (smoother resizes/scrolling)
77b5e9a neovim#9315 screen: add missing status redraw when redraw_later(CLEAR) was used
5f15788 TUI: clip invalid regions on resize (neovim#8779), fixes neovim#8774
c936ae0 neovim#9193 TUI: improvements for scrolling and clearing
f204274 neovim#9143 UI: disable clearing almost everywhere
f4b2b66 neovim#9079 TUI: always use safe cursor movement after resize
d36afaf neovim#9211 ui_options: also send when starting or from OptionSet
67f80d4 TUI: Avoid reset_cursor_color in old VTE neovim#9191
e55ebae neovim#9021 don't erase screen on `:hi Normal` during startup
c5790d9 neovim#8915 TUI: Hint wrapped lines to terminals.

FIXES:

231de72 RPC: turn errors from async calls into notifications
907ad92 TUI: Restore terminal title via "title stacking" (neovim#9407)
cb76a8a genappimage: Unset $ARGV0 at invocation neovim#9376
b48efd9 neovim#9347 TUI: FreeBSD: Improve support for BSD vt console
c16529a TUI: Konsole 18.07.70 supports DECSCUSR (neovim#9364)
aec096f os/lang: use the correct LC_NUMERIC also for OS X
5fee0be provider: improve error message (neovim#9344)
3c42d7a TUI: alacritty supports set_cursor_color neovim#9353
7bff9a5 TUI: Alacritty supports DECSCUSR (neovim#9048)
57acfce macOS: infer primary language if $LANG is empty neovim#9345
bc132ae runtime/syntax: Fix highlighting of augroup contents (neovim#9328)
715fdfe neovim#9297 VimL/confirm(): Show dialog even if :silent
799d9c3 clipboard: Prefer xclip (neovim#9302)
6dae777 provider/nodejs: fix npm,yarn detection
16bc1e9 neovim#9218 channel: avoid buffering output when only terminal and no callbacks are active
72fecad neovim#8804 Fix crash in lang_init() on macOS if lang_region = NULL
d581398 ruby: detect rbenv shims for other versions (neovim#8733)
e568ac7 neovim#9123 third-party/unibilium: Fix parsing of extended capability entries
c4c74c3 jobstart(): Fix hang on non-executable cwd neovim#9204
1cf50cb provider/nodejs: Simultaneously query npm and yarn neovim#9054
6c496db undo: Fix infinite loop if undo_read_byte returns EOF neovim#2880
f8f8357 neovim#9034 'swapfile: always show dialog'

CHANGES:

c236e80 neovim#9024 --embed: wait for UI unless --headless
180b50d neovim#9248 python: 'neovim' module was renamed to 'pynvim'
2000b6a neovim#8589 VimL: Remove legacy aliases "v:errmsg", "v:shell_error", "v:this_session"
deb18a0 defaults: background=dark neovim#2894 (neovim#9205)
c1187d4 defaults: win: 'shellpipe' for cmd.exe (neovim#8827)
justinmk added a commit to justinmk/neovim that referenced this pull request Dec 31, 2018
Besides the "visible" improvements, this release features numerous
internal improvements to the UI/screen code and test infrastructure.

Numerous patches were merged from Vim, which are not mentioned below.

FEATURES:

07ad5d7 clipboard: Support custom VimL functions neovim#9304
725da1f neovim#9401 win/TUI: Improve terminal/console support
7a8dadb neovim#9077 startup: Use $XDG_CONFIG_DIRS/nvim/sysinit.vim if it exists
feec926 neovim#9299 support <cmd> mapping in more places
0653ed6 neovim#9028 diff/highlight: Show underline for low-priority CursorLine
bddcbbb signs: Add "numhl" argument neovim#9113
05f9c7c clipboard: support Wayland (neovim#9230)
14ae394 neovim#9052 TUI: add support for undercurl and underline color
4fa3492 neovim#9023 man.vim: soft (dynamic) wrap neovim#9023

API:

8b39e4e neovim#6920 API: implement object namespaces
b1aaa0a API: Implement nvim_win_set_buf() neovim#9100
8de87c7 neovim#8180 API: virtual text annotations (nvim_buf_set_virtual_text)
2b9fc9a neovim#8660 API: add nvim_buf_is_loaded()
    API: buf_get_lines, buf_line_count handle unloaded buffers
88f77c2 API: nvim_buf_get_offset_for_line
94841e5 API/UI: neovim#8221 ext_newgrid, ext_hlstate
    (use line-based rather than char-based updates)

UI

b5cfac0 neovim#8806 TUI: use BCE again more often, (smoother resizes/scrolling)
77b5e9a neovim#9315 screen: add missing status redraw when redraw_later(CLEAR) was used
5f15788 TUI: clip invalid regions on resize (neovim#8779), fixes neovim#8774
c936ae0 neovim#9193 TUI: improvements for scrolling and clearing
f204274 neovim#9143 UI: disable clearing almost everywhere
f4b2b66 neovim#9079 TUI: always use safe cursor movement after resize
d36afaf neovim#9211 ui_options: also send when starting or from OptionSet
67f80d4 TUI: Avoid reset_cursor_color in old VTE neovim#9191
e55ebae neovim#9021 don't erase screen on `:hi Normal` during startup
c5790d9 neovim#8915 TUI: Hint wrapped lines to terminals.

FIXES:

231de72 RPC: turn errors from async calls into notifications
907ad92 TUI: Restore terminal title via "title stacking" (neovim#9407)
cb76a8a genappimage: Unset $ARGV0 at invocation neovim#9376
b48efd9 neovim#9347 TUI: FreeBSD: Improve support for BSD vt console
c16529a TUI: Konsole 18.07.70 supports DECSCUSR (neovim#9364)
aec096f os/lang: use the correct LC_NUMERIC also for OS X
5fee0be provider: improve error message (neovim#9344)
3c42d7a TUI: alacritty supports set_cursor_color neovim#9353
7bff9a5 TUI: Alacritty supports DECSCUSR (neovim#9048)
57acfce macOS: infer primary language if $LANG is empty neovim#9345
bc132ae runtime/syntax: Fix highlighting of augroup contents (neovim#9328)
715fdfe neovim#9297 VimL/confirm(): Show dialog even if :silent
799d9c3 clipboard: Prefer xclip (neovim#9302)
6dae777 provider/nodejs: fix npm,yarn detection
16bc1e9 neovim#9218 channel: avoid buffering output when only terminal and no callbacks are active
72fecad neovim#8804 Fix crash in lang_init() on macOS if lang_region = NULL
d581398 ruby: detect rbenv shims for other versions (neovim#8733)
e568ac7 neovim#9123 third-party/unibilium: Fix parsing of extended capability entries
c4c74c3 jobstart(): Fix hang on non-executable cwd neovim#9204
1cf50cb provider/nodejs: Simultaneously query npm and yarn neovim#9054
6c496db undo: Fix infinite loop if undo_read_byte returns EOF neovim#2880
f8f8357 neovim#9034 'swapfile: always show dialog'

CHANGES:

c236e80 neovim#9024 --embed: wait for UI unless --headless
180b50d neovim#9248 python: 'neovim' module was renamed to 'pynvim'
2000b6a neovim#8589 VimL: Remove legacy aliases "v:errmsg", "v:shell_error", "v:this_session"
deb18a0 defaults: background=dark neovim#2894 (neovim#9205)
c1187d4 defaults: win: 'shellpipe' for cmd.exe (neovim#8827)
bfredl added a commit to bfredl/neovim that referenced this pull request Oct 11, 2019
PR neovim#8221 took a short-cut when implemententing the tests: screen.lua would
translate the linegrid highlight ids back into the old per-cell
attribute description.

Apart from cleaning up technichal dept, this enables to check both rgb
and cterm colors in the same expect(), which previously was needlessly
restricted to ext_hlstate tests only.
bfredl added a commit to bfredl/neovim that referenced this pull request Oct 11, 2019
PR neovim#8221 took a short-cut when implementing the tests: screen.lua would
translate the linegrid highlight ids back into the old per-cell
attribute description.

Apart from cleaning up technical dept, this enables to check both rgb
and cterm colors in the same expect(), which previously was needlessly
restricted to ext_hlstate tests only.
bfredl added a commit to bfredl/neovim that referenced this pull request Oct 11, 2019
PR neovim#8221 took a short-cut when implementing the tests: screen.lua would
translate the linegrid highlight ids back into the old per-cell
attribute description.

Apart from cleaning up technical debt, this enables to check both rgb
and cterm colors in the same expect(), which previously was needlessly
restricted to ext_hlstate tests only.
bfredl added a commit to bfredl/neovim that referenced this pull request Oct 13, 2019
PR neovim#8221 took a short-cut when implementing the tests: screen.lua would
translate the linegrid highlight ids back into the old per-cell
attribute description.

Apart from cleaning up technical debt, this enables to check both rgb
and cterm colors in the same expect(), which previously was needlessly
restricted to ext_hlstate tests only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor changes that are not features or bugfixes ui ui-extensibility UI extensibility, events, protocol, externalized UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants