After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 731205 - Save/restore cursor should also effect charset
Save/restore cursor should also effect charset
Product: vte
Classification: Core
Component: general
Other Linux
: Normal normal
: ---
Assigned To: VTE Maintainers
VTE Maintainers
: 590367 (view as bug list)
Depends on: 732586
Reported: 2014-06-04 13:27 UTC by Egmont Koblinger
Modified: 2018-02-28 13:24 UTC
See Also:
GNOME target: ---
GNOME version: ---

Part 1: change cursor_current to be relative to insert_delta (29.53 KB, patch)
2014-07-25 20:57 UTC, Egmont Koblinger
rejected Details | Review
Part 2: make cursor and other variables per-terminal (73.43 KB, patch)
2014-07-25 21:25 UTC, Egmont Koblinger
rejected Details | Review
Part 3: save and restore the required attributes (8.08 KB, patch)
2014-07-25 22:02 UTC, Egmont Koblinger
rejected Details | Review
Fix (86.97 KB, patch)
2014-07-26 12:44 UTC, Egmont Koblinger
none Details | Review
Fix, v2 (87.24 KB, patch)
2014-07-27 14:30 UTC, Egmont Koblinger
rejected Details | Review
Fix, v3 (86.21 KB, patch)
2014-08-26 22:57 UTC, Egmont Koblinger
accepted-commit_now Details | Review
Fix v4 (86.59 KB, patch)
2014-11-22 18:00 UTC, Egmont Koblinger
committed Details | Review

Description Egmont Koblinger 2014-06-04 13:27:16 UTC
vttest -> screen features (2) -> SAVE/RESTORE CURSOR (last screen)

or this equivalent one-liner:
echo -e '\e(0qqqqq\e7\e(Bfoo\e8qqqqq'      

expected output: ──────────
actual output:   ─────qqqqq

Apparently xterm saves/restores the "alternate character set" state.  And some more (check xterm source code).
Comment 1 Egmont Koblinger 2014-07-25 19:05:10 UTC
There are quite a few per-screen variables that should be per-terminal instead: cursor_current, reverse_mode, origin_mode, sendrecv_mode, insert_mode, linefeed_mode, scrolling_region, scrolling_restricted, defaults, color_defaults, fill_defaults, alternate_charset (getting replaced by character_replacement* in bug 732586). [The list is to be double checked.]

[cursor_current is tricky, it is absolute according to the beginning of the scroll history, so it can't simply be made per-terminal. Either the whole code needs to be changed so that it's relative to insert_delta, or it needs to be adjusted on screen change.]

It seems to me that this list is exactly the list of variables that should be saved/restored on \e7 / \e8.

Interestingly, even though these values should be shared across the two screens, the normal and the alternate screens each have their separate save/restore slot. So we should end up having 3 versions of each variable: the actual runtime, the normal screen's saved and the alt screen's saved value.
Comment 2 Egmont Koblinger 2014-07-25 20:57:47 UTC
Created attachment 281728 [details] [review]
Part 1: change cursor_current to be relative to insert_delta

Offsetting by insert_delta was deleted 24 times and added 52 times.  I'm not super happy, but I guess it's better to go for the cleaner semantics and pay this price.
Comment 3 Egmont Koblinger 2014-07-25 21:25:55 UTC
Created attachment 281729 [details] [review]
Part 2: make cursor and other variables per-terminal

Pretty much a search-replace so taht the cursor position and some other variables are now correctly per-terminal, rather than per-screen.

Forgot to say: the patches apply on top of bug 732586.
Comment 4 Egmont Koblinger 2014-07-25 22:02:34 UTC
Created attachment 281733 [details] [review]
Part 3: save and restore the required attributes

This is the actual change I had in mind.  With this fix, VTE passes(*) vttest's 2nd ("Test of screen features") test.

(*)Well, almost passes... underlined line drawing doesn't work, see bug 708195.
Comment 5 Egmont Koblinger 2014-07-25 22:15:54 UTC
Further cleanup possibilities:

- Rename cursor_current to cursor, cursor_saved to saved_cursor for consistency.

- Create "pvt" variable as a shorthand for "terminal->pvt" in many functions for brevity (similarly to how we have "screen" at many places).

- I'm wondering if we should have a struct grouping all these variables that are saved. It'd make it easier to save/restore them and make that code nicer there. But I wouldn't want to refer to them differently because of this property (e.g. "cursor.color_defaults" rather than "color_defaults", etc.), I think it's better to have the current spaghetti save/restore methods rather than an additional indirection throughout the code.
Comment 6 Egmont Koblinger 2014-07-26 09:37:13 UTC
Comment on attachment 281728 [details] [review]
Part 1: change cursor_current to be relative to insert_delta

Not good.  There are problems with the vertical cursor position, and multiple attempts to fix it have failed.  For some weird reason it's much more complicated than it should be.

I'll redo the whole thing, keeping the old semantics (absolute cursor).
Comment 7 Egmont Koblinger 2014-07-26 12:44:55 UTC
Created attachment 281768 [details] [review]

Here's a better patch (all in one).
Comment 8 Egmont Koblinger 2014-07-27 13:55:06 UTC
The cursor position is not always kept correctly across screen changes.  It boils down to two reasons: 1. the ring is not extended to contain the cursor (will update the patch soon), and 2. vte_terminal_set_scrollback_lines() only works on the active screen so the alternate screen is only three lines high until you first switch to it (it's a separate bugfix that I'm submitting right now).
Comment 9 Egmont Koblinger 2014-07-27 14:30:45 UTC
Created attachment 281819 [details] [review]
Fix, v2
Comment 10 Egmont Koblinger 2014-08-24 00:49:50 UTC
Review of attachment 281819 [details] [review]:

Breaks scrolling upwards in mcview if mc is configured --with-screen=ncurses.
Comment 11 Egmont Koblinger 2014-08-26 22:57:10 UTC
Created attachment 284563 [details] [review]
Fix, v3

Fix the mc scrolling bug.  (Restricted scrolling is per-terminal, but is not saved/restored on cursor save/restore.)  Also a git merge.  Goes on top of bug 732586 comment 16.
Comment 12 Egmont Koblinger 2014-11-22 18:00:04 UTC
Created attachment 291249 [details] [review]
Fix v4

(Just a git merge. Goes on top of bug 732586 comment 17.)
Comment 13 Egmont Koblinger 2014-11-22 18:34:17 UTC
Comment 14 Egmont Koblinger 2018-02-28 13:24:58 UTC
*** Bug 590367 has been marked as a duplicate of this bug. ***