GNOME Bugzilla – Bug 731205
Save/restore cursor should also effect charset
Last modified: 2018-02-28 13:24:58 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).
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.
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.
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.
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.
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 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).
Created attachment 281768 [details] [review] Fix Here's a better patch (all in one).
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).
Created attachment 281819 [details] [review] Fix, v2
Review of attachment 281819 [details] [review]: Breaks scrolling upwards in mcview if mc is configured --with-screen=ncurses.
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.
Created attachment 291249 [details] [review] Fix v4 (Just a git merge. Goes on top of bug 732586 comment 17.)
Committed.
*** Bug 590367 has been marked as a duplicate of this bug. ***