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 #6801

Closed
wants to merge 9 commits into from

Conversation

jdebp
Copy link
Contributor

@jdebp jdebp commented May 24, 2017

This is #6793, rebased.

@justinmk
Copy link
Member

justinmk commented May 25, 2017

LGTM, @jdebp are you ready for this to be merged?


For reference: This felt slower to me when scrolling, but all attempts to measure it show no significant difference.

bash:

time printf 'j%.0s' {1..500} | build/bin/nvim runtime/filetype.vim -u NORC -i NONE --cmd 'set cursorline' -c 'sleep 100m' -c redraw -c '0' -c 'vsplit' +qa

python RPC connection:

import time; s=time.time(); [n.input("j") for i in range(1,500)]; n.eval('1+1'); time.time()-s

@aktau
Copy link
Contributor

aktau commented May 25, 2017

LGTM, although I must admit that I'm not a VT protocol warrior, so my LGTM is worse than the author taking a second look. Would be great if @leonerd could take a look, but if this is tried out in a couple of terminals (tmux, xterm, urxvt, gnome-terminal, iTerm2, Terminal.app). I've tried the latter two + tmux and it seems fine (anything I should pay more attention to?).

@jdebp
Copy link
Contributor Author

jdebp commented May 25, 2017

iTerm2, Terminal.app are particuarly useful for you to test out @aktau, because they are two that I am unable to test. Look in particular for scrolling problems with vertically and horizontally split windows, and oddities in changing the cursor shape when entering and leaving insert mode; both with and without screen/tmux in the mix. With screen/tmux, check that only the current pane is affected by shape changes.

Check over SSH. Ensure that you transfer the XTERM_VERSION, COLORTERM, and KONSOLE_* environment variables across from client to server. Also try not doing that, of course.

Or go straight to #6816 and do this.

@aktau
Copy link
Contributor

aktau commented May 26, 2017

I've tested iTerm2 and Terminal.app locally here as to your suggestions (no tmux, no ssh). The result matrix:

term scrolling cursor
iterm2 OK OK
Terminal.app NOT OK OK

So, Terminal.app had issues scrolling. I split my window into 4 panes (one in each corner):

| 1 | 2 |
| 3 | 4 |

What seemed to happen is that scrolling in pane 1 also scrolled pane 2 (and vice versa). The same for panes 3 and 4. This causes corruption in the screen state. See the screenshots at the end [1].

This is macOS Sierra, and the $TERM variable when I'm insude Terminal.app is:

13:22:49 neovim/neovim git:(merge-pr-6801) » echo $TERM
xterm-256color

[1]: screen shot 2017-05-26 at 13 20 21
screen shot 2017-05-26 at 13 20 47

@jdebp
Copy link
Contributor Author

jdebp commented May 26, 2017

This is as #6802 leads me to expect. Now try with #6816. Your matrix should be success in all cases.

@aktau
Copy link
Contributor

aktau commented May 26, 2017

This is as #6802 leads me to expect. Now try with #6816. Your matrix should be success in all cases.

Indeed, Terminal.app works correctly with #6816 (and so does iTerm2). Though I do notice sluggishness when scrolling one of the panes in iTerm2 now. Terminal.app seems nice and responsive.

The fact that it's exactly the same Neovim version + configuration makes me think it has something to do with the terminal.

However, I stopwatch timed scrolling to the bottom of the LICENSE file and these are the results:

  • iTerm2/6816: 10.54s
  • iTerm2/master: 10.26s
  • iTerm2/6816: 10.27s
  • Terminal.app/6816: 10.36s

Basically saying that either I'm imagining things or that Neovim always scrolls equally fast, but doesn't uphold the same framerate while doing so.

Anyway, everything seems good, especially from a functionality point of view.

@justinmk
Copy link
Member

justinmk commented May 27, 2017

@aktau That's exactly the same observation (and measurement outcomes) I had :) #6801 (comment)

jdebp and others added 9 commits May 30, 2017 07:49
... rather than hardwiring the string and testing the terminal
type every time the screen is re-sized.
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.
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.
Per warnings about house style from automated tools.
The clear_screen capability moves the cursor position.
This needs to be accounted for.
@aktau
Copy link
Contributor

aktau commented May 31, 2017

@jdebp, any idea about the sluggishness on iTerm2 (but not Terminal.app)? If two people saw it, it may be a real thing. How do we properly debug this? Does your newest rebase change anything there?

Also, do we need to keep this PR open if #6816 seems to subsume it? (Since there's commits in #6816 that fix bugs introduced here, I wouldn't merge it separately in order to have a more bug-free history).

@jdebp
Copy link
Contributor Author

jdebp commented Jun 3, 2017

The rebase was just a rebase, and shouldn't have changed anything. At this point it seems wise to focus on #6816 because that has all of this and a lot more.

I suggest that you try the latest in #6816, and measure there. We seem to have finally pushed iTerm2 past the highly faulty stage, although I am still unhappy about some of the bizarre output that is in the syscall traces provided by @choco there that I do not yet have a satisfactory explanation for, and about @choco's one (I understand) remaining problem which is backspacing in command editing mode around the edge of the terminal.

I suspect that the two are related, but I have yet to grasp how. @choco's syscall output looks like malformed multibyte characters of some sort, following a cnorm sequence. I don't see a code path where anything can follow a cnorm sequence at the tail of a write() system call.

@gnachman
Copy link

gnachman commented Jun 6, 2017

If you're running into problems with iTerm2, I'd like to know if there are bugs or performance improvements I can make to help you. Especially if there are ways the terminal emulation differs from Terminal that aren't justifiable! Logs from script that produce incorrect output ought to be enough for me to track down the issue.

@justinmk
Copy link
Member

justinmk commented Jul 6, 2017

This PR is a subset of #6816, which was merged.

@justinmk justinmk closed this Jul 6, 2017
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

4 participants