UI protocol: scrolling up sent as grid_line events instead of grid_scroll

[glacambre]

• nvim --version: NVIM v0.5.0-519-g74fed7d50
• UI extensions: ext_linegrid=true, ext_multigrid=false, ext_messages=false, rgb=true

Steps to reproduce using nvim -u NORC

Create a 20 lines x 80 columns UI showing a buffer containing ~1000 lines and :set nu. Go to the bottom of the screen and press k to scroll up.

Actual behaviour

Neovim sends the following events:

["grid_line",[1,29,71,[["g",1],["k"]]]]
["grid_line",[1,29,71,[[" ",1,2]]]]
["grid_line",[1,1,0,[[" ",67],["9"],["7"],["2"],[" "],[" ",0,77]]],[1,2,0,[[" ",67],["9"],["7"],["3"],[" "],[" ",0,77]]],[1,3,0,[[" ",67],["9"],["7"],["4"],[" "],[" ",0,77]]],[1,4,0,[[" ",67],["9"],["7"],["5"],[" "],[" ",0,77]]],[1,5,0,[[" ",67],["9"],["7"],["6"],[" "],[" ",0,77]]],[1,6,0,[[" ",67],["9"],["7",67,2],[" "],[" ",0,77]]],[1,7,0,[[" ",67],["9"],["7"],["8"],[" "],[" ",0,77]]],[1,8,0,[[" ",67],["9"],["7"],["9"],[" "],[" ",0,77]]],[1,9,0,[[" ",67],["9"],["8"],["0"],[" "],[" ",0,77]]],[1,10,0,[[" ",67],["9"],["8"],["1"],[" "],[" ",0,77]]],[1,11,0,[[" ",67],["9"],["8"],["2"],[" "],[" ",0,77]]],[1,12,0,[[" ",67],["9"],["8"],["3"],[" "],[" ",0,77]]],[1,13,0,[[" ",67],["9"],["8"],["4"],[" "],[" ",0,77]]],[1,14,0,[[" ",67],["9"],["8"],["5"],[" "],[" ",0,77]]],[1,15,0,[[" ",67],["9"],["8"],["6"],[" "],[" ",0,77]]],[1,16,0,[[" ",67],["9"],["8"],["7"],[" "],[" ",0,77]]],[1,17,0,[[" ",67],["9"],["8",67,2],[" "],[" ",0,77]]],[1,18,0,[[" ",67],["9"],["8"],["9"],[" "],[" ",0,77]]],[1,19,0,[[" ",67],["9",67,2],["0"],[" "],[" ",0,77]]],[1,20,0,[[" ",67],["9",67,2],["1"],[" "],[" ",0,77]]],[1,21,0,[[" ",67],["9",67,2],["2"],[" "],[" ",0,77]]],[1,22,0,[[" ",67],["9",67,2],["3"],[" "],[" ",0,77]]],[1,23,0,[[" ",67],["9",67,2],["4"],[" "],[" ",0,77]]],[1,24,0,[[" ",67],["9",67,2],["5"],[" "],[" ",0,77]]],[1,25,0,[[" ",67],["9",67,2],["6"],[" "],[" ",0,77]]],[1,26,0,[[" ",67],["9",67,2],["7"],[" "],[" ",0,77]]],[1,27,0,[[" ",67],["9",67,2],["8"],[" "],[" ",0,77]]],[1,28,0,[[" ",67],["9",67,3],[" "],[" ",0,77]]],[1,0,0,[[" ",67],["9"],["7"],["1"],[" "],[" ",0,77]]]]

Expected behaviour

Neovim sends the following events:

["grid_line",[1,29,71,[["g",1],["k"]]]]
["grid_line",[1,29,71,[[" ",1,2]]]]
["grid_scroll",[1,0,29,0,82,-1,0]]
["grid_line",[1,28,0,[["1",67],["0",67,3],[" "],[" ",0,77]]]]

Since UI bugs aren't easy to reproduce I'll investigate myself. I'm just opening this here so that I don't forget.

[glacambre]

Here is a minimal reproducer:

import time
from pynvim import attach
from threading import Thread

class UIThread(Thread):
    def __init__(self, nvim):
        Thread.__init__(self)
        self.nvim = nvim
        self.do_print = False
    def run(self):
        self.nvim.run_loop(self.callback, self.callback)
    def callback(self, event_name, events):
        if self.do_print:
            [print(event) for event in events if event[0] == "grid_line" or event[0] == "grid_scroll"]

nvim = attach('child', argv=["/home/me/bin/nvim", "--embed", "-u", "NORC", "-i", "NONE"])
ui = UIThread(nvim)
ui.start()
nvim.async_call(nvim.ui_attach, 80, 20, ext_linegrid = True, rgb = True)
nvim.async_call(nvim.command, "set laststatus=1")
nvim.async_call(nvim.command, "set nu")
nvim.async_call(nvim.input, "1000o")
nvim.async_call(nvim.input, "<Esc>")
nvim.async_call(nvim.input, "H")
time.sleep(1)
ui.do_print = True
time.sleep(1)
nvim.async_call(nvim.input, "k")
time.sleep(1)
ui.do_print = False
time.sleep(1)
nvim.async_call(nvim.input, ":qa!<Enter>")
time.sleep(1)

Running this script will print:

['grid_line', [1, 19, 52, [['k', 1]]]]
['grid_line', [1, 19, 52, [[' ', 1]]]]
['grid_line', [1, 1, 0, [[' ', 24], ['9'], ['8'], ['4'], [' '], [' ', 0, 75]]], [1, 2, 0, [[' ', 24], ['9'
], ['8'], ['5'], [' '], [' ', 0, 75]]], [1, 3, 0, [[' ', 24], ['9'], ['8'], ['6'], [' '], [' ', 0, 75]]],
[1, 4, 0, [[' ', 24], ['9'], ['8'], ['7'], [' '], [' ', 0, 75]]], [1, 5, 0, [[' ', 24], ['9'], ['8', 24, 2
], [' '], [' ', 0, 75]]], [1, 6, 0, [[' ', 24], ['9'], ['8'], ['9'], [' '], [' ', 0, 75]]], [1, 7, 0, [['
', 24], ['9', 24, 2], ['0'], [' '], [' ', 0, 75]]], [1, 8, 0, [[' ', 24], ['9', 24, 2], ['1'], [' '], [' '
, 0, 75]]], [1, 9, 0, [[' ', 24], ['9', 24, 2], ['2'], [' '], [' ', 0, 75]]], [1, 10, 0, [[' ', 24], ['9',
 24, 2], ['3'], [' '], [' ', 0, 75]]], [1, 11, 0, [[' ', 24], ['9', 24, 2], ['4'], [' '], [' ', 0, 75]]],
[1, 12, 0, [[' ', 24], ['9', 24, 2], ['5'], [' '], [' ', 0, 75]]], [1, 13, 0, [[' ', 24], ['9', 24, 2], ['
6'], [' '], [' ', 0, 75]]], [1, 14, 0, [[' ', 24], ['9', 24, 2], ['7'], [' '], [' ', 0, 75]]], [1, 15, 0,
[[' ', 24], ['9', 24, 2], ['8'], [' '], [' ', 0, 75]]], [1, 16, 0, [[' ', 24], ['9', 24, 3], [' '], [' ',
0, 75]]], [1, 17, 0, [['1', 24], ['0', 24, 3], [' '], [' ', 0, 75]]], [1, 18, 0, [['1', 24], ['0', 24, 2],
 ['1'], [' '], [' ', 0, 75]]], [1, 0, 0, [[' ', 24], ['9'], ['8'], ['3'], [' '], [' ', 0, 75]]], [1, 19, 6
4, [['3', 1]]]]

But set laststatus to 2 instead and you get a nice scroll event:

['grid_line', [1, 19, 69, [['k', 1]]]]
['grid_line', [1, 19, 69, [[' ', 1]]]]
['grid_scroll', [1, 0, 18, 0, 80, -1, 0]]
['grid_line', [1, 0, 0, [[' ', 24], ['9'], ['8'], ['3'], [' '], [' ', 0, 75]]], [1, 18, 64, [['3', 9], [',
'], ['0'], ['-'], ['1'], [' ', 9, 8], ['9', 9, 2], ['%']]]]

Now let's see if it's a regression.

[glacambre]

It is indeed a regression, Neovim 0.3.7 doesn't have this issue. Now let's bisect…

[glacambre]

This regression was introduced in b51ba12 (written by @bfredl). The problem with this patch is not obvious to me, I'll revert it hunk by hunk until I find out what's wrong.

[glacambre]

I couldn't understand what was wrong from reading b51ba12 so I looked at what differed between executing the reproducer with laststatus=1 and laststatus=2 and I found two reasons for this missed optimization.

The first one is in ui_comp_grid_scroll:

neovim/src/nvim/ui_compositor.c

Lines 613 to 647 in dbc8ec9

	static void ui_comp_grid_scroll(UI *ui, Integer grid, Integer top,
	Integer bot, Integer left, Integer right,
	Integer rows, Integer cols)
	{
	if (!ui_comp_should_draw() || !ui_comp_set_grid((int)grid)) {
	return;
	}
	top += curgrid->comp_row;
	bot += curgrid->comp_row;
	left += curgrid->comp_col;
	right += curgrid->comp_col;
	bool covered = curgrid_covered_above((int)(bot - MAX(rows, 0)));
	if (covered || curgrid->blending) {
	// TODO(bfredl):
	// 1. check if rectangles actually overlap
	// 2. calculate subareas that can scroll.
	compose_debug(top, bot, left, right, dbghl_recompose, true);
	for (int r = (int)(top + MAX(-rows, 0)); r < bot - MAX(rows, 0); r++) {
	// TODO(bfredl): workaround for win_update() performing two scrolls in a
	// row, where the latter might scroll invalid space created by the first.
	// ideally win_update() should keep track of this itself and not scroll
	// the invalid space.
	if (curgrid->attrs[curgrid->line_offset[r-curgrid->comp_row]
	+left-curgrid->comp_col] >= 0) {
	compose_line(r, left, right, 0);
	}
	}
	} else {
	ui_composed_call_grid_scroll(1, top, bot, left, right, rows, cols);
	if (rdb_flags & RDB_COMPOSITOR) {
	debug_delay(2);
	}
	}
	}

As explained by the TODOs, neovim doesn't try to compute a grid_scroll when there is potential overlap between two rectangles. Fixing this would make neovim send grid_scroll events again.

The second one is in curgrid_covered_above:

neovim/src/nvim/ui_compositor.c

Lines 606 to 611 in dbc8ec9

	static bool curgrid_covered_above(int row)
	{
	bool above_msg = (kv_A(layers, kv_size(layers)-1) == &msg_grid
	&& row < msg_current_row-(msg_was_scrolled?1:0));
	return kv_size(layers)-(above_msg?1:0) > curgrid->comp_index+1;
	}

The row < msg_current_row should actually be msg_current_row + 1 if the statusline is not being displayed. A patch like this fixes the problem when only one window is present:

diff --git a/src/nvim/ui_compositor.c b/src/nvim/ui_compositor.c
index e582d8f..951b667 100644
--- a/src/nvim/ui_compositor.c
+++ b/src/nvim/ui_compositor.c
@@ -605,8 +605,9 @@ static void ui_comp_msg_set_pos(UI *ui, Integer grid, Integer row,
 /// TODO(bfredl): currently this only handles message row
 static bool curgrid_covered_above(int row)
 {
+  int statusline_height = p_ls == 2 ? 1 : 0;
   bool above_msg = (kv_A(layers, kv_size(layers)-1) == &msg_grid
-                    && row < msg_current_row-(msg_was_scrolled?1:0));
+                    && row - statusline_height <= msg_current_row-(msg_was_scrolled?1:0));
   return kv_size(layers)-(above_msg?1:0) > curgrid->comp_index+1;
 }

Unfortunately this isn't entirely correct, statusline_height should actually be (p_ls == 2) || (p_ls == 1 && win_count() > 0). And on top of that, the compositor currently reasons in terms of grids, it's probably a bad idea to get windows and statuslines involved.

So I'll try to fix the TODOs in ui_comp_grid_scroll instead.

[teto]

thanks for the intel, keep up the good work :)