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

Alternate buffer improvement #1764

Merged
merged 4 commits into from
Nov 23, 2018
Merged

Alternate buffer improvement #1764

merged 4 commits into from
Nov 23, 2018

Conversation

whydoubt
Copy link
Contributor

Alternate buffer handling in xterm.js is lacking in a few areas compared with most other terminals. One is how the cursor color and position is saved and restored. Another is how clearing is done when entering the alternate buffer.

These changes help xterm.js better replicate the behavior of other terminals.

@Tyriar
Copy link
Member

Tyriar commented Oct 20, 2018

Do you have an example program that illustrates the bad behavior inside xterm.js?

@whydoubt
Copy link
Contributor Author

alt_test.txt
Since there are a several variations I am checking for, I created a script that sleeps between each operation. I open up all terminals I want to check, determine tty for each, then in a separate terminal run something like:
. alt_test.txt > /dev/pts/7 & . alt_test.txt > /dev/pts/8
...to make it run in all terminals practically simultaneously. Each terminal treats it a bit different, but my changes most closely follow xterm.

@whydoubt
Copy link
Contributor Author

whydoubt commented Nov 8, 2018

Some smaller tests...

Fix cursor movement when swapping buffers

I made xterm.js follow xterm behavior in this test on a cleared buffer:

for x in 47 1047 1049 1048 9999; do
echo -e "\E[0m== ${x} ==\n\E[?${x}h\n\E[31mJUNK\E[?${x}lTEST\E[0m\n"; done

If 1049 comes back with TEST being red, it means that the attributes of the normal buffer are affected when I enter and exit vim. The result of this can be unpleasant.

Each buffer needs its own saved cursor attributes

On running the following in a bash shell, TEST should not be red.

echo -e '\E[0m\E[?1049h\E[31m\E[s\E[?1049lTEST\E[0m'

Clear alt buffer with erase attributes upon entering

In most terminals, this will cause the entire buffer to flash green for a second:

echo -ne '\E[0m\E[42m\E[?1049h'; sleep 1; echo -ne '\E[?1049l\E[0m'

@jerch
Copy link
Member

jerch commented Nov 8, 2018

@whydoubt Thanks alot for fixing this, LGTM 👍 . I think we should have a few test cases that cover the alt buffer switch behavior rather than relying on external scripts, maybe you could add those under BufferSet.test.ts?

@jerch jerch added this to the 3.9.0 milestone Nov 9, 2018
@whydoubt
Copy link
Contributor Author

I have updated the commits to include relevant tests.

@Tyriar
Copy link
Member

Tyriar commented Nov 18, 2018

The changes seem good, just want some clarification on what the first test is meant to do:

for x in 47 1047 1049 1048 9999; do echo -e "\033[0m== ${x} ==\n\033[?${x}h\n\033[31mJUNK\033[?${x}lTEST\033[0m\n"; done

This screenshot shows macOS programs in this order (left to right): This PR, iTerm2, Terminal.app, xterm.js based on master:

screen shot 2018-11-18 at 2 44 37 pm

@whydoubt
Copy link
Contributor Author

whydoubt commented Nov 19, 2018

The key parts in pseudo-code: {print header} {enter alt} {print newline} {text red} {print 'JUNK'} {leave alt} {print 'TEST'}.

  • If you see JUNKTEST in red, it means the alt control code had no effect (9999 is the control case for that).
  • Otherwise, if you see JUNK on the second line, it means the buffer did not clear on leaving the alt buffer (expected with 1048).
  • If the cursor (position and attributes) is saved on entry / restored on exit, TEST should start at the first row, first column, with default attributes (1048 / 1049).
  • If the cursor (position and attributes) are NOT restored on exit, TEST should be on the second line, indented, in red (47 / 1047).

@whydoubt
Copy link
Contributor Author

term_alt_tests
On Linux, left to right: Konsole, XTerm, Putty, GNOME Terminal (vte-based)

@jerch
Copy link
Member

jerch commented Nov 23, 2018

@whydoubt Wow nice test cases! About correctness - only "spec" I can find about those higher set mode numbers is from xterm and this clearly states when to store/restore cursor attrs and when not. Guess the other emulators do it wrong? Maybe @egmontkob also wants to have a look at this.

Since it now works as stated in the xterm doc I am fine with adding it as it is.

@jerch
Copy link
Member

jerch commented Nov 23, 2018

@whydoubt Many thanks for this PR!

We gonna add it as it is since it is line with xterm doc/behavior. Also we can easily switch to different behavior if we find it not suitable for whatever reason.

@jerch jerch merged commit 76e7057 into xtermjs:master Nov 23, 2018
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.

3 participants