New issue
Advanced search Search tips

Issue 765254 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

hterm: DECSET 1048 (Save Cursor State) is per-terminal, but for other terminals it's per-screen

Reported by judo...@gmail.com, Sep 14 2017

Issue description

What steps will reproduce the problem?
Interpret the following text in hterm: "\x1b[?1049h \n\n\n\n \x1b[s \x1b[?1049l world \n"

The commands corresponds to this sequence of actions:
- Save cursor (row=1)
- Activate alternate screen
- Clear screen
- Insert four newlines
- Save cursor (row=5, col=1)
- Activate primary screen
- Restore cursor

What is the expected result?
The cursor should now be at row=1 (the primary screen's cursor should be restored).

What happens instead?
The cursor is at row=5 (the alternate screen's cursor i restored).

 

Comment 1 Deleted

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/84301d071762440c33633825fe2d6a3c1539c46b

commit 84301d071762440c33633825fe2d6a3c1539c46b
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Dec 05 00:12:37 2017

hterm: separate terminal full & soft reset code paths

The current terminal full reset code tails into the soft terminal reset
function which in turn re-runs some of the reset logic.  The calls with
VT reset are also a bit confusing to keep track of due to the nesting.

BUG= chromium:765254 

Change-Id: I689769519438d9e28ffc991f6b57329b110d35bc
Reviewed-on: https://chromium-review.googlesource.com/797991
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/84301d071762440c33633825fe2d6a3c1539c46b/hterm/js/hterm_terminal.js

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/7e42f63bd655beb9afe3318e0365bc87706eab26

commit 7e42f63bd655beb9afe3318e0365bc87706eab26
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Dec 05 00:12:52 2017

hterm: move vt reset calls into the terminal layer

Our RIS (full reset) code path was resetting the VT itself, then calling
the terminal reset which would turn around and reset the VT again.  So
drop the VT reset from the RIS handler.

Our DECSTR (soft reset) code path wouldn't reset the VT twice, but anyone
using the terminal.softReset API wouldn't have the VT reset.  So move the
VT reset from the VT layer into the terminal layer.

Now the terminal reset API is consistent: users of the JS API will get
the right behavior, and the VT callbacks will follow the same codepaths.

We move the VT reset calls in the terminal functions to the top just to
be consistent code-wise for the readers.  It shouldn't matter here when
the VT was reset relative to the other features since there isn't any
overlap.

BUG= chromium:765254 

Change-Id: I17b8312f1382f455eeb5faa6f3f43ba7d906ddac
Reviewed-on: https://chromium-review.googlesource.com/797992
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/7e42f63bd655beb9afe3318e0365bc87706eab26/hterm/js/hterm_terminal.js
[modify] https://crrev.com/7e42f63bd655beb9afe3318e0365bc87706eab26/hterm/js/hterm_vt.js

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/5f8971526252c241430cc9d57ab145e3bab92437

commit 5f8971526252c241430cc9d57ab145e3bab92437
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Dec 05 00:14:14 2017

hterm: reset SGR attributes during soft resets

This looks like an oversight due to the confusing code paths we had here
previously, but the DECSTR docs are clear:
  Select graphic rendition   SGR   Normal rendition
And this matches xterm behavior.

BUG= chromium:765254 

Change-Id: I26bbbab6bbb5175d19cdb7e2fea4fc4e8ed294f3
Reviewed-on: https://chromium-review.googlesource.com/797993
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/5f8971526252c241430cc9d57ab145e3bab92437/hterm/js/hterm_terminal.js

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/a2cacaa2eb22fe38149ded7fa98fa81b1d118383

commit a2cacaa2eb22fe38149ded7fa98fa81b1d118383
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Dec 05 00:14:49 2017

hterm: move cursor save/restore state from per-terminal to per-screen

Currently when you save/restore the cursor state (a confusing phrase as
the set of attributes are a bit more than just the cursor), it's done on
a per-terminal basis.  The active screen (primary vs alternative) is not
taken into consideration.  This is unlike xterm (where the screen concept
comes from) which has separate cursor states for each screen.

Refactor the CursorState class so it's no longer tracked in the VT layers
directly, but is part of the screen class, and the terminal class helps
mediate access to the active one (which is what mediates the screens and
associated content now too).

BUG= chromium:765254 

Change-Id: I8043edc95c0bc7670ec480a39c64cf742f357398
Reviewed-on: https://chromium-review.googlesource.com/797994
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/a2cacaa2eb22fe38149ded7fa98fa81b1d118383/hterm/js/hterm_terminal.js
[modify] https://crrev.com/a2cacaa2eb22fe38149ded7fa98fa81b1d118383/hterm/js/hterm_vt.js
[modify] https://crrev.com/a2cacaa2eb22fe38149ded7fa98fa81b1d118383/hterm/js/hterm_screen.js
[modify] https://crrev.com/a2cacaa2eb22fe38149ded7fa98fa81b1d118383/hterm/js/hterm_terminal_tests.js

Owner: vapier@chromium.org
Status: Fixed (was: Available)
this should be fixed in the next hterm-1.76 and nassh-0.8.41 releases

Sign in to add a comment