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

TUI cursor motion, SGR, and scrolling optimizations #6793

Closed
wants to merge 20 commits into from

Conversation

jdebp
Copy link
Contributor

@jdebp jdebp commented May 23, 2017

The TUI sends a lot of extraneous stuff to the terminal. It repeats SGR sequences unnecessarily. It uses inefficient cursor motions. It does not shortcut repainting the cleared area in scrolls for terminals announced as BCE. It does not take advantage of terminal scrolling margin capabilities.

This builds upon the work in #6792 (née #6685) to address these, and reduce the amount of data that is sent to update and to repaint the screen. Yes, this is still important to people using SSH and the like.

Some of these optimizations I remember from the days of stevie.

jdebp and others added 17 commits May 23, 2017 02:05
This limits the use of dtterm's extension to DECSLPP to only those
terminal types where it is known to be supported.
Because it can be potentially understood as genuine DECSLPP
sequence, setting the number of lines to a number larger than 25,
which of course can cause confusion (especially if it is the width
parameter that results in this) only use it on terminals that are
known to support the dtterm extension.
Also moved the commentary out of a block, to in-line.
... rather than hardwiring the string and testing the terminal
type every time the screen is re-sized.
This limits the use of dtterm's extension to DECSLPP to only those
terminal types where it is known to be supported.
Because it can be potentially understood as genuine DECSLPP
sequence, setting the number of lines to a number larger than 25,
which of course can cause confusion (especially if it is the width
parameter that results in this) only use it on terminals that are
known to support the dtterm extension.
Also moved the commentary out of a block, to in-line.
... rather than hardwiring the string and testing the terminal
type every time the screen is re-sized.
DECSLPP is explicitly documented as not affecting the scroll region.
The dtterm extension is not as well documented, but it is safer than not to assume that it operates similarly.

This also eliminates a pointlessly repeated test from tui_scroll().
It additionally uses a non-parameterized DECSTBM sequence when attempting to reset back to whole-screen scrolling.
Track whether the terminal is in no attribute mode, assuming that it starts
this way, and do not attempt to reset back to that mode if already in it.
Respect the BGE flag from terminfo rather than guessing that it is always off.
Emit DECLRMM and DECSLRM (or equivalent) to properly define the scroll rectangle.
Instead of emitting CUP in several places each with their own poor local
optimizations, funnel all cursor motion through a central place.

This central function performs the same optimization for every place that
needs to move the cursor, and implements a better set of optimizations:

* Emit CUU/CUD/CUF/CUB instad of CUP when they are likely shorter.
* Use BS and LF when they are shorter than CUB and CUD.
* Use CR for quick returns to column zero.
* If printing the next few characters is shorter than a rightwards motion,
  then just write out the characters.
PuTTY does not implement DECLRMM or DECSLRM, but it does implement DECSTBM.
So allow using PuTTY terminal scrolling when the scroll rectangle is the
full width of the terminal.
A slight improvement on the CR optimization for some edge cases.
@jdebp
Copy link
Contributor Author

jdebp commented May 23, 2017

Running the test suite on FreeBSD here does not produce the failure that is shown by the automatic tests. It was showing some other errors, caused by error message spelling differences that the test harnesses were not expecting. I have fixed them in #6791.

@justinmk
Copy link
Member

The QB failure may be a timing issue or may just require a test adjustment, I'll take a closer look.

22:23:14,464 INFO  - # test/functional/terminal/tui_spec.lua @ 282: tui focus event handling can handle focus events in terminal mode
22:23:14,464 INFO  - not ok 2156 - tui focus event handling can handle focus events in terminal mode
22:23:14,464 INFO  - # test/functional/terminal/tui_spec.lua @ 282
22:23:14,464 INFO  - # Failure message: ./test/functional/helpers.lua:252:
22:23:14,464 INFO  - # retry() attempts: 2
22:23:14,464 INFO  - # ./test/functional/ui/screen.lua:301: Row 1 did not match.
22:23:14,464 INFO  - # Expected:
22:23:14,464 INFO  - # |*ready $ |
22:23:14,464 INFO  - # |[Process exited 0]{1: } |
22:23:14,464 INFO  - # | |
22:23:14,464 INFO  - # | |
22:23:14,464 INFO  - # | |
22:23:14,464 INFO  - # |gained |
22:23:14,464 INFO  - # |{3:-- TERMINAL --} |
22:23:14,464 INFO  - # Actual:
22:23:14,464 INFO  - # |*nal |
22:23:14,464 INFO  - # |{1: } |
22:23:14,464 INFO  - # |{4:~ }|
22:23:14,464 INFO  - # |{4:~ }|
22:23:14,464 INFO  - # |{4:~ }|
22:23:14,464 INFO  - # |gained |
22:23:14,464 INFO  - # |{3:-- TERMINAL --} |
22:23:14,464 INFO  - #
22:23:14,464 INFO  - # To print the expect() call that would assert the current screen state, use
22:23:14,465 INFO  - # screen:snaphot_util(). In case of non-deterministic failures, use
22:23:14,465 INFO  - # screen:redraw_debug() to show all intermediate screen states.
22:23:14,465 INFO  - # stack traceback:
22:23:14,465 INFO  - #     ./test/functional/helpers.lua:252: in function 'retry'
22:23:14,465 INFO  - #     test/functional/terminal/tui_spec.lua:286: in function <test/functional/terminal/tui_spec.lua:282>

Regarding the changes in general, I'll take your word for it, you clearly have deep experience with terminal programming. Maybe @leonerd could take a look :)

jdebp added 2 commits May 23, 2017 15:38
Per warnings about house style from automated tools.
The clear_screen capability moves the cursor position.
This needs to be accounted for.
@ZyX-I
Copy link
Contributor

ZyX-I commented May 23, 2017

By the way, I remember Vim having a hack (workaround for terminal emulators bugs) that sends cursor goto even if it should not be necessary after some characters with “ambiguous” width (specifically those for which terminal emulator may have other idea regarding width then Vim has (newest emoji?)). Does Neovim has something like this?

@jdebp
Copy link
Contributor Author

jdebp commented May 23, 2017

That's ambiwidth. It's dealt with at a level above the TUI, in the UI and screen layer. I suspect that there was a pre-existing problem here; because the UI layer hides this from lower layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants