GNOME Bugzilla – Bug 761097
Some lines dissapears on gnome-terminal resize
Last modified: 2016-02-11 19:13:32 UTC
Bug present in 0.39 0.40 0.41 0.42 0.43. 1. Execute something that output multiple lines like #ls -l /usr/lib 2. Open vim/vi/nano/etc. 3. Resize terminal. Decrease height. 4. Close vim/vi/nano/etc. Some lines removed from previous output.
Crap... I took special care to make sure this works as expected. No clue when and how it could broke. I can _often_ reproduce the bug, but not always. In order to git bisect, it would be good to reliably reproduce it.
Thanks a lot for the bug report, tenor! I don't understand how this bug could remain unnoticed for such a long time.
5a434e6c4457bdfe182a13213396e7a66a08f767 is the first bad commit commit 5a434e6c4457bdfe182a13213396e7a66a08f767 Author: Egmont Koblinger <egmont@gmail.com> Date: Sat Nov 22 19:20:35 2014 +0100 emulation: Save/restore alternate charset https://bugzilla.gnome.org/show_bug.cgi?id=731205 :040000 040000 502d0935608322ed31322b21c1de0ccd4a1bd649 a47458549f60e2c39d06ca53f93ffa69a815944c M src
Before breaking: Setting PTY size to 80x23. Resizing normal screen Old insert_delta=229 scroll_delta=229 cursor_current (absolute) row=252 (visual line 24) col=0 cursor_saved (relative to insert_delta) row=23 col=0 Dropping 0 [== MIN(230, 0, 1)] rows at the bottom Scroll to bottom New insert_delta=230 scroll_delta=230 cursor_current (absolute) row=252 (visual line 23) col=0 cursor_saved (relative to insert_delta) row=22 col=0 After breaking: Setting PTY size to 80x23. Resizing normal screen Old insert_delta=229 scroll_delta=229 cursor (absolute) row=50 (visual line -178) col=0 cursor_saved (relative to insert_delta) row=23 col=0 Dropping 1 [== MIN(230, 202, 1)] rows at the bottom Scroll to bottom New insert_delta=229 scroll_delta=229 cursor (absolute) row=50 (visual line -178) col=0 cursor_saved (relative to insert_delta) row=23 col=0 (The "Dropping 0..." is not logged by default, I just modified it now.) Row and visual line numbers are reported incorrectly, and drop2 is miscalculated.
(The above message were about resizing the normal screen while the alternate screen was active.) Before the change, we had per-screen current and saved cursors. After the change we have a per-terminal current cursor and per-screen saved cursors. So even attempting to log the absolute cursor position when we're not operating on the active screen doesn't make any sense. It's not just miscomputed, it's conceptually wrong. As for drop2, I think below_current_paragraph needs to use the saved cursor rather than the current cursor when operating on the non-active screen. Let me think about it a bit more...
The tricky part is... Resizing the normal screen while the visible screen is active relies on the concept that there is a cursor position on the (currently invisible) normal screen. This, however, is not true per se. There is a slot to save a cursor position, and there is an active cursor position that's shared by the two screens. You can switch between the screens with DECSET 47/1047 which does not save the cursor, or with 1049 which does. If switching with 1049, the normal screen's saved cursor position should be used in order to resize the normal screen. However, if switching with 47/1047... well, what should happen then? Dunno. Using the current cursor position (as seen on the alternate screen) doesn't make any sense. I'm temped to introduce another cursor position (the last seen per-screen value; at least for normal screen, may not be needed for the alternate one, not sure yet) which is always saved on switching screens (no matter if 47/1047 or 1049) and is used solely for positioning the cursor upon a resize. Another possibility could be to revive my first attempt in bug 731205 comments 2-6 (which I abandoned for a good reason) and try to have per-screen current cursors which are updated/synchronized as needed.
Created attachment 319709 [details] [review] Fix for 0-42, v1 Could you please test this against the 0.42 branch? Checklist: - This particular bug; - vttest's 2nd menu, esp. its last screen; - Important for porting to master! In vte_terminal_process_incoming() "screen" can deviate from "terminal->pvt->screen" and it's important to always use the proper one (which I've no clue about, so I just reverted these bits from the broken change).
Created attachment 319763 [details] [review] Fix for 0-42, v2
Created attachment 319764 [details] [review] Fix for master, v2 Please test both patches thoroughly!
All of that patches work for me.
Christian, please review (especially the C++ism).
Is this just a straight s/m_cursor/m_screen->cursor/ sed job, or are there any actual changes?
Actual ones, too. Although nothing C++-specific there. I'm not even sure if that #define'd m_ prefix is preferred or not. (In reply to Egmont Koblinger from comment #7) > - Important for porting to master! In vte_terminal_process_incoming() > "screen" can deviate from "terminal->pvt->screen" and it's important to > always use the proper one (which I've no clue about, so I just reverted > these bits from the broken change). Here I can see a small chance for an intermittent display corruption. The code has changed from 0.42 to 0.43, but I believe both of them potentially suffer from this issue. I'll file another bug for this.
Review of attachment 319764 [details] [review]: Using m_ prefixed vars is correct (the #define's just exist until all the code is ported over to the C++ class and then I'll rename the vars in the class to have the m_ prefix). Just a couple nits: ::: src/vte.cc @@ +4006,3 @@ /* The cursor shouldn't be above or below the addressable * part of the display buffer. */ + g_assert(screen->cursor.row >= screen->insert_delta); m_screen ::: src/vteseq.cc @@ +387,2 @@ /* cursor.row includes insert_delta, adjust accordingly */ + long cr = m_screen->cursor.row - m_screen->insert_delta; auto cr = ...
Submitted. Christian, I'd say let's wait for maybe a week to see if this change indeed proves stable, and after that could you please ask someone to create a new 0.42 tarball? It's a bit unfortunate that it's not a straightforward cherry-pick but quite a rewrite of the change between the two branches. I'll use 0-42 for the time being to potentially catch any regression, although I'm using too much g-t nowadays.
... although I'm not* using too much g-t nowadays.
Just FYI: 0.42.4 tarball is available, hooray/thanks! I've stress-tested this version a couple of times during the last 2 weeks, no problem occurred. I'll try to get this version into forthcoming Ubuntu Xenial LTS, it would be awesome :)