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 707592 - screen positioned incorrectly after multiple resizes
screen positioned incorrectly after multiple resizes
Status: RESOLVED DUPLICATE of bug 708213
Product: vte
Classification: Core
Component: general
0.34.x
Other Linux
: Normal normal
: ---
Assigned To: VTE Maintainers
VTE Maintainers
Depends on:
Blocks: 336238
 
 
Reported: 2013-09-05 20:11 UTC by Egmont Koblinger
Modified: 2013-09-27 20:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Screenshot (120.21 KB, image/jpeg)
2013-09-05 20:11 UTC, Egmont Koblinger
  Details
Fix (886 bytes, patch)
2013-09-05 23:57 UTC, Egmont Koblinger
none Details | Review
Simple test program (759 bytes, text/plain)
2013-09-16 21:22 UTC, Egmont Koblinger
  Details

Description Egmont Koblinger 2013-09-05 20:11:06 UTC
Created attachment 254208 [details]
Screenshot

Run the joe (*) text editor inside vte, editing a file.  Resize the vte window many times in a row (using the window manager's opaque resize method, dragging the corner of the window crazily for a second or two).

Vte sometimes (1 out of ~5-10 cases) enters a state where the topmost row is unused, the 2nd row contains joe's header (which should be in the 1st row), and the last line of joe is invisible, under the visible area. The cursor can be moved down there and then it's invisible too (out of vte's window).

Exactly as if you scrolled back in the history buffer by 1 line, except that you can't scroll to the correct position with the scrollbar, nor jumps it there at keypress.

Might be related to bug 707572, although vte doesn't crash on that assertion in debug mode, not even when the cursor is moved out of the window.

(*) joe-editor.sf.net, version 3.7 using default settings when it uses the normal screen, not the alternate one.
Comment 1 Egmont Koblinger 2013-09-05 23:57:15 UTC
Created attachment 254218 [details] [review]
Fix

This patch seems to fix it (although it fixes the symptom rather than the actual inner bug, so is probably not the right thing to do).

The three lines of code in vte_terminal_emit_adjustment_changed() commented as "little dance" manually alter the value of scroll_delta, and then gtk_adjustment_set_value() reverts it back to its previous value if there are no multiple resize events queued. However, when multiple such events occurred in a short time, the last call doesn't exactly revert the value of scroll_delta but assigns it a different value (typically 1 lower) resulting in the off-by-one bug.

E.g. here delta is 100, so gtk_adjustment_set_value(terminal->adjustment, 100) is executed, which triggers the signal "value-changed" connected to vte_terminal_handle_scroll(). At the beginning of this method gtk_adjustment_get_value(terminal->adjustment) returns 99 instead of 100.

I can't figure out what happens between setting the value and invoking the signal handler.
Comment 2 Egmont Koblinger 2013-09-10 16:38:37 UTC
I'm totally stuck with this. My only guess is (and sorry if I'm totally wrong) is that before calling the value-changed handler, Gtk processes the change of the scrollbar adjustment (due to change of window height) which propagates back to scroll_delta. If this is the case then probably my patch above _is_ the right fix: i.e. let vte_terminal_handle_scroll() work with the new value, and then vte_terminal_emit_adjustment_changed() do its own "dance" and revert to the old delta. I'm really not sure though.
Comment 3 Egmont Koblinger 2013-09-16 21:22:32 UTC
Created attachment 255067 [details]
Simple test program

We might be seeing multiple bugs or the weird combination of them. Here's a simple test script (a simplified version of what joe does). The behavior is clearly buggy, 100% reproducible by a single resize, and is not fixed by my previous patch.

The program outputs some text just to make sure the scrollbar appears, then prints a screenful of text enumerating the rows. The second step is repeated whenever the number of terminal rows changes.

Start the program, it outputs the screenful of text, numbering the lines correctly. Drag the scrollbar back and forth and notice that everything is fine.

Make the terminal taller. The screenful of text, numbering all the lines is redrawn correctly. Start dragging the scrollbar. Notice that you cannot scroll back to the bottom.
Comment 4 Egmont Koblinger 2013-09-16 22:48:20 UTC
Even simpler test case:
  seq 1 50; sleep 100000
Then make the terminal taller, and drag the scrollbar back and forth.

New empty lines appear on the bottom, whereas old ones as still scrolled out at the top. In the mean time, terminal->adjustment's upper bound is not updated. (It's unclear to me why it's not updated even when those lines become filled up with data.)

One of the possible solutions is to make sure that new lines are added at the top, not at the bottom when the screen is made taller, so that it never happens that there are unused lines at the bottom and scrolled out ones on the top. This is actually part of my rewrap patch in bug #336238 (but that one's still going to take some time to complete).

Another possible direction is to make the scrollbar's upper bound grow to the visual bottom of the screen, something like this in vte.c:

                /* The upper value is the number of rows which might be visible.  (Add
                 * one to the cursor offset because it's zero-based.) */
                v = MAX(_vte_ring_next(screen->row_data),
                                screen->cursor_current.row + 1);
+               v = MAX(v, screen->insert_delta + terminal->row_count);

(I'm not sure if any of the two old values are still necessary in that 3-way MAX.)
Comment 5 Denis 2013-09-18 05:46:23 UTC
Egmont Koblinger, could you test patch from this bug report? 
https://bugzilla.gnome.org/show_bug.cgi?id=708213
Comment 6 Egmont Koblinger 2013-09-18 17:04:55 UTC
Yup, seems to fix it. Definitely touches the lines of code that were the most suspicious to me :) I'll take a closer look at the patch.
Comment 7 Behdad Esfahbod 2013-09-27 19:38:50 UTC
Is it right to mark this a dup now?  I've committed the latest patch from bug 708213.

*** This bug has been marked as a duplicate of bug 708213 ***
Comment 8 Egmont Koblinger 2013-09-27 20:39:44 UTC
I still don't understand the issue at the end of comment 1 which is still present. Nevertheless, it might be harmless and I couldn't reproduce the bug anymore. Instead of dupe I'd rather say "obsoleted by", but whatever :)