[RFC] TUI cursor fixes #9079

bfredl · 2018-10-02T12:12:40Z

Fixes the true root cause of #7861 (which #8806 avoided on BCE terminals and hack-fixed on non-BCE terminals).

I would expect this to fix #8936, as now it always does a safe cursor move after resize. A plausible case that could have caused this crash (though I'm not 100% it is exactly what happened):

the previous cursor position was row 50 since before, on a grid larger than 50 rows.
grid_resize changes the grid height to 40, and invalidly assumes the resize moved the physical cursor to row 0
some event used a operation that moved the cursor (such as a clear or scroll), and then reset the cursor to the "true" position row 50 (pointless after [RDY] UI grid protocol revision: line based updates and semantic highlights #8221, but I forgot to remove it)
raw_line/cheap_to_print is invoked, and tries to inspect the grid at row 50 (memory error)
grid_cursor_move would have been called at this point, and set a valid cursor position 0-39.

This PR changes two wrongs to two rights (don't reset to a "stale" input cursor location + don't assume anything about the physical cursor after resize), and is thus hopefully robust to similar conditions caused by slightly different event orderings.

justinmk · 2018-10-02T20:25:08Z

Nice cleanup!

Would be good to include the plausible Case in the commit message.

I will try to repro and check with asan.

bfredl · 2018-10-06T08:41:05Z

[ RUN      ] tui 't_Co' (terminal colors) TERM=interix uses 8 colors: ERR
test/functional/ui/screen.lua:431: Row 2 did not match.
Expected:
  |{1: }                                                 |
  |*~                                                 |
  |~                                                 |
  |~                                                 |
  |~                                                 |
  |8                                                 |
  |{3:-- TERMINAL --}                                    |
Actual:
  |{1: }                                                 |
  |*                                                  |
  |~                                                 |
  |~                                                 |
  |~                                                 |
  |8                                                 |
  |{3:-- TERMINAL --}                                    |
To print the expect() call that would assert the current screen state, use
screen:snapshot_util(). In case of non-deterministic failures, use
screen:redraw_debug() to show all intermediate screen states.  
stack traceback:
	test/functional/ui/screen.lua:431: in function 'wait'
	test/functional/ui/screen.lua:307: in function 'expect'
	...uild/neovim/neovim/test/functional/terminal/tui_spec.lua:484: in function 'assert_term_colors'
	...uild/neovim/neovim/test/functional/terminal/tui_spec.lua:708: in function <...uild/neovim/neovim/test/functional/terminal/tui_spec.lua:707>

not sure if it only "worked by accident" originally, running TERM=interrix nvim (both master and this branch) in pangoterm (libvterm just like :term) gives a lot of glitches, especially with vsplit+scroll.

bfredl · 2018-10-06T08:50:32Z

reverting the second commit fixes it, we could keep the workaround only for interix. But then again, it might not be needed in "real" interix, only fake TERM=interix with libvterm...

bfredl · 2018-10-06T09:00:14Z

Replacing magic wait() invocation with pre-assert (of the condition of actual user interaction) fixes the issue at least locally.

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.

This was caused by cursor position being invalid right after tui_grid_resize, which is now fixed

justinmk · 2018-10-08T12:51:45Z

Reported an ASan trace in #8936 (comment). Will try those steps with this PR later.

Update: Success! Did not repro the issue with this PR.

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 mentioned this pull request Oct 2, 2018

crash: tui_raw_line/cheap_to_print #8936

Closed

marvim added the RFC label Oct 2, 2018

bfredl force-pushed the tui_cursor branch 2 times, most recently from 1798f92 to c002778 Compare October 2, 2018 20:19

justinmk approved these changes Oct 2, 2018

View reviewed changes

bfredl force-pushed the tui_cursor branch from c002778 to 9aecb8a Compare October 6, 2018 08:59

bfredl added 3 commits October 6, 2018 11:29

TUI: delete "first-row" workaround after resize

1bf83ea

This was caused by cursor position being invalid right after tui_grid_resize, which is now fixed

test: replace wait() with pre-assertion in assert_term_colors

075dc42

bfredl force-pushed the tui_cursor branch from 9aecb8a to 075dc42 Compare October 6, 2018 09:38

bfredl merged commit f4b2b66 into neovim:master Oct 9, 2018

justinmk removed the RFC label Oct 9, 2018

justinmk mentioned this pull request Nov 4, 2018

Certain colorschemes + set number + tmux cause bleed on first line #9201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RFC] TUI cursor fixes #9079

[RFC] TUI cursor fixes #9079

Uh oh!

bfredl commented Oct 2, 2018 •

edited

Loading

Uh oh!

justinmk commented Oct 2, 2018 •

edited

Loading

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

justinmk commented Oct 8, 2018 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[RFC] TUI cursor fixes #9079

[RFC] TUI cursor fixes #9079

Uh oh!

Conversation

bfredl commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

bfredl commented Oct 6, 2018

Uh oh!

justinmk commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bfredl commented Oct 2, 2018 •

edited

Loading

justinmk commented Oct 2, 2018 •

edited

Loading

justinmk commented Oct 8, 2018 •

edited

Loading