GNOME Bugzilla – Bug 708213
zsh - lots of blank space upon resizing VTE based terminals
Last modified: 2014-04-06 18:28:00 UTC
Overview: It's about zsh and its behaviour when being run inside a VTE based terminal (e.g. gnome-terminal, xfce4-terminal, roxterm, sakura, ...). lots of blank space upon resizing VTE based terminals original thread about this bug was here https://bbs.archlinux.org/viewtopic.php?pid=1246865 Steps to Reproduce: run zsh with theme adam2 or change bash PS1 to this one PS1='\033[J\u@\h:\w\$ ' then increase/decrease window height Actual Results: When I increase the terminal height, if the buffer exceeds the window height, the content above won't get correctly shifted down, but simply blank space is added at the bottom. On the contrary, when I decrease the terminal height, it does not simply remove the blank space at the bottom, but instead shifts up the buffer, keeping the blank space. Expected Results: when window height is decreased, last terminal line must be always at the bottom of the window. when window height is increased, last terminal line must be on same line (from the top line) as it was before resizing. patch that resolve this bug is in attachment.
Created attachment 255079 [details] [review] fix resizing
Duplicate of bug 707592 ?
Created attachment 255155 [details] [review] fix resizing v2 fix blank lines when increasing fix resizing v2 fix blank lines when increasing window height, problem described there https://bbs.archlinux.org/viewtopic.php?pid=1247188#p1247188
Christian Persch, i'm not sure, Egmont Koblinger talking about scrolling, while my problem is about resizing. my problem,as i know, appear only with "\033[J" escape sequence which is hardcoded in zsh prompt.
To be more clear, here is a video file with the problem and the result of the patch. ftp://linvinus.ru/forum/vte/vte_bug.ogv (12Mb)
The lines of code you're touching are definitely fishy :) The patch does indeed fix my problem there, thanks! I'm yet to take a closer look at what you're doing :) Denis, in the mean time, could you please try the patch from bug 336238 (patch version "7b")? I heavily re-wrote this part of the code. Please let me know if that behavior matches your expectations.
In the middle of bug 336238 comment 26 I mention a strange behavior: if your scrollbar is somewhere at middle-ish position and you keep making the terminal taller/shorter (e.g. maximize/unmaximize) then new lines appear at the bottom while old ones are removed from the top. I don't think this should be this way. My patch over there addresses this problem too, your one doesn't. I'm looking at your patch, and while I haven't found any bugs yet, I can't say I'm confident. Honestly there are way too many expressions without semantical meaning to me, I don't understand what's going on, and way too many -1's (especiall comparision like "x < y-1") make me heavily suspect that there's an off-by-one error somewhere, I just haven't found where. But maybe it's just me being bad in understanding others' code. :) I'll try to come up with a code (based on 336238's patch 7b) that's hopefully easier to understand, we'll see...
Created attachment 255247 [details] [review] egmont's first patch This is my first proposal. It's essentially the aforementioned patch with the rewrap stuff taken out. It's way more verbose, hence (hopefully) much more easy to read, understand, debug, and modify if required. If there's more than a screenful of text, I believe the behavior is what you're looking for. Unfortunately, if there's less than a screenful of text, the behavior of this patch is different from yours. I guess we should better understand the situation and the desired behavior. The story is: the \e[J stuff makes vte create the ring all the way to the bottom of the screen and fill up with empty lines. (As opposed to without the \e[J the bottom part of the screen is not covered by the ring.) So this leads to the observations: problems arise when the cursor is not in the bottommost row of the ring, and you shrink the terminal. To test: have a shell+prompt that does _not_ output \e[J, create some output (either less than a screenful or more, these could behave differently), then execute something like echo $'\e[3;0H' and then start resizing the terminal. What should happen? I guess current vte, your patch, my patch and xterm all behave differently :P
- What shall happen if you resize the window while the cursor is not in the bottommost line that's ever been used? Current vte and my patch: Lines are never dropped from the bottom of the logical buffer (the end of the ring). Hence after a \e[J is printed and the window is shrunk, the content gets pushed upwards. Note that current vte does this incorrectly: bug 707572 comment 1. xterm: Shrinking always cuts from the bottom of the buffer. Enlarging adds from the top (the scrollback buffer) if has data, otherwise an empty row from the bottom. So if there is scrollback data and you keep enlarging and shrinking back and forth by a single line, the contents start to drift away which is a bit silly. Denis's patch: Seems to be xterm-like at first glimpse, but on multiple cursor repositioning and multiple resizes with scrollback buffer present, it easily starts going crazy where I don't have a clue what happens :) With Denis's patch the problem might be that vte cannot shrink the ring at it's end, but that's what you'd need. So you end up pointing insert_delta to a position that's lower than ring_next-rows. I'm not sure if this is (or should be) handled consistently all across vte, including subsequent resizes and such. Another perhaps reasonable behavior, similar to xterm's or Denis's would be to follow this concept, and also keep the contents for those rows scrolled out at the bottom, just as vte currently remembers the content cut off at the right when you narrow the window. And bring them back when the window is enlarged. - What should \e[J do? Fill up the screen with actually used but empty lines? This is what it currently does, but together with not eating lines from the bottom at the resize the end result is not nice in zsh. Pretend that all the lines below the cursor were never used? Might sound reasonable, but then with scrollback buffer it'd need to snap the cursor to the bottom of the screen and bring back some contents from the scrollback, which might not be what you want. Or might keep you at an invalid scroll position where you can never get back to once you scroll away, sounds dangerous. Anyway, in order to implement in VTE we'd need to drop the end of the ring, or hide it by maintaining insert_delta properly. Clear the lines below the cursor that are currently used, but don't add new ones? Sounds easy to implement, not sure if the end result is user friendly in all use cases. Maybe shall we simply go for xterm's behavior saying it's a safe bet? :)
Created attachment 255253 [details] [review] case study: what if \e[J doesn't create new lines
Denis, could you please test with - only my "egmont's first patch", - and with both that and the "case study" patches attached here? Please test them both with and without the scrollbar present. I do hope that the second combo would provide the behavior you'd expect, although I'm not fully confident.
Egmont Koblinger, sorry both patches are buggy. 1) ftp://linvinus.ru/forum/vte/egmonts_first_patch.ogv 2) ftp://linvinus.ru/forum/vte/egmonts_first_patch_and_case_study.ogv on second video i compare your patches with mine. also i try to use only "case study" patch, but with no luck.
Created attachment 255292 [details] [review] fix resizing v2, with comments, some variables was renamed I hope now the source code easier to read
Created attachment 255293 [details] [review] 255292: fix resizing v2, with comments, some variables was renamed final version =) replace (screen->cursor_current.row+delta-last_index_in_ring) with "delta" as described in comments :)
Created attachment 255294 [details] [review] 255293: 255292: fix resizing v2, with comments, some variables was renamed final version v2 delta sign was wrong described
so basically, the differences is only in more strict conditions, when we should adjust cursor position on visible screen. that is all
Thanks for your continuous effort! Videos are really cool, I'll also figure out how to create them and upload some, they're a really good way of demonstrating bugs. Unfortunately your patch is not free of bugs (or undesired behavior) either, I'll show some. You concentrate on certain use cases that I did not think of previously, while I concentrate on some that you don't think of. Moreover, I'm working towards rewrapping which sets up another set of assumption and makes the code more complicated in nontrivial ways (e.g. currently the ring also changes, ring_next, ring_delta etc. all become different). I'd like us to clearly understand the requirements and come up with a solution that suffices all known use cases and is easily adjustable for rewrapping. (I encourage you to subscribe to the rewrapping bug 336238 an keep an eye on that to make sure that we don't accidentally break a use case in the future that's important to you.) In the mean time, could you please clarify whether the following is true or not? If I understand your videos correctly, I believe that you expect the following behavior: "Whenever the window size is shrunk by 1 row, and the cursor was not in the bottom row, the behavior should be that the bottom row is cut off, the rest stays where they were. If the bottom row contained any data, that data is lost forever." Is this true or not? (This pretty much matches xterm's behavior, although I'm not entirely sure this is the best possibility.)
Egmont Koblinger , you are right my patch working until ring overflow(first time). i will answer on your questions later. i use recordmydesktop for records.
Created attachment 255338 [details] [review] egmont's second patch How about this one? I hope this one's okay. I have only tested without the "case study" patch, but I think it shouldn't make a difference to apply that too.
Technical note: I haven't studied your new patch, but if you drop lines from the bottom (looks that's the behavior you're looking for) then I'm pretty sure you need a corresponding _vte_ring_shrink() to drop from the end of the ring as well.
My patch is not okay, after a while (outputting '\e[3;0H' and resizing a couple of times, and repeating these two) it starts to forget the contents of some lines near the top of the screen. Sometimes it forgets a part of a line only. The lines sometimes disappear on focus loss. Not sure if my bug or perhaps a bug in _vte_ring_shrink? Anyway, Denis, despite this bug I think you can already tell me whether you generally like the behavior or not :)
Perhaps it's time to bite the bullet and understand what the code is supposed to do and write it to do that, instead of trial-and-error fixing it? :D No one understands that code anymore...
Behdad, could you please take a look if you happen to have some time? Either I shouldn't use _vte_ring_shink() the way I do, or it is buggy. My most recent ("second") patch brings the ring into an inconsistent state where you execute "seq 1 50" and numbers 19-21 are missing. Output one more line and 20-22 will be missing, and so on. Maybe the filesystem row_stream goes out of sync with in-memory VteRowData? The missing lines are above the bottom 30 lines, maybe this is related to ring->mask (31)? Video: https://docs.google.com/file/d/0B9iCZ18clVMiSEEycTZrdEQzZUU/edit?usp=sharing I'll continue debugging tomorrow...
Ok, I'll find some time to dig this.
Re comment 22: <imho>We shouldn't understand what that code was meant to do. It probably didn't think of Denis's use case, and didn't think of the requirements that rewrapping introduces. We should drop that code, understand the requirements from scratch and implement them in a clean, well-documented way :) I believe we're already quite close to this.</imho>
Created attachment 255394 [details] [review] fix blank lines on resizing v3 seems now all is ok. Please test it. video ftp://linvinus.ru/forum/vte/fix_resizing_v3.ogv
(In reply to comment #17) > Is this true or not? (This pretty much matches xterm's behavior, although I'm > not entirely sure this is the best possibility.) yes that is true. we should meet following cases: if cursor at bottom line prompt with and without "\033[J" (bash and zsh prompt style) 1) on decreasing window height, scroll content up 2) on increasing window height, scroll content down until reach back lines buffer limit (ring maximum) if cursor at the middle of the window prompt with and without "\033[J" (bash and zsh prompt style) 1) on decreasing window height drop lines until cursor will be on last row then, scroll content up 2) on increasing window height, scroll content down (relative to current cursor position) until reach back lines buffer limit (ring maximum)
Problems with your v3: https://docs.google.com/file/d/0B9iCZ18clVMiRnpjSFh6UTVkQU0/edit?usp=sharing The behavior you described and I described are I believe the same. I also think that the behavior presented by your recent patch and my recent patch are basically the same (apart from bugs in both of them :)), implementing this described behavior. So I think we agree on what should happen, and that's a good point. You rely on scroll_delta when calculating the new values of insert_delta and such. This is incorrect. Whether or not the view is currently scrolled back should no effect on what the terminal's contents will be. The old value of scroll_delta should only influence the new value of scroll_delta and no other variable. I also demonstrate a long-standing vte scrolling problem that I've mentioned before and my patch addresses while your one doesn't. The rewrap feature requires that to be fixed. Both when shrinking or enlarging, the bottom line should stay (if possible). Dropping lines at the bottom does something that vte never did before: the ring continues further than (insert_delta+terminal->rows). Before accepting any patch (whether yours or mine) I believe we should perfectly understand whether: - this is allowed and is handled correctly all over in vte, or if we can adjust the rest of vte accordingly - or we should shrink the ring, and why that doesn't work with my patch
A side thought about ring_next: For the sake of easy explanation, let's assume that the scrollbar is not touched by the user, i.e. scroll_delta always stays the same as insert_delta. In current vte, ring_next points to somewhere between (inclusive) the first (or second?) row of the screen, and the row below the bottom one. With Denis's approach it can be an even higher number (pointing to many lines below the bottom). ring_next introduces the concept of whether a certain line was already used, or not. I.e. when the terminal starts and the shell displays its prompt, ring_next points to the second line, implying that most of the screen is "unused". If a command outputs a screenful of newlines and moves the cursor back to the top-left corner, ring_next starts pointinig to below the bottom of the screen, implying that every row is "used" now, while in the mean time no user-visible change occurred. I have a feeling that there are no sequence of events (stuff printed to the screen, or user-initiated events such resize or mouse highlight) that should be able to differentiate between these two cases. I mean, if any action behaves differently in these two cases, that's probably against the user's expectation. I have a guts feeling that probably other terminal emulators don't even have this concept of "used" vs "not yet used" rows. This suggests to me that forcing ring_next to point to the row below the bottom would decrease the number of independent variables and hence could make the code simpler. Or, this (along with ring_len) could be made a technical value private to ring.c, the rest of vte shouldn't have access to this.
(In reply to comment #29) > Or, this (along with ring_len) could be made a technical value > private to ring.c, the rest of vte shouldn't have access to this. This sounds right to me.
Denis, see the video from comment 23, your patch produces exactly the same (lines are disappearing, then seq's output misses some numbers). I'll try to catch the core problem here. Since we're both writing our own codes on our own, this pretty much looks like an existing problem somewhere in vte that we're both hitting. And (as opposed to my first thought) it's not related to _vte_ring_shrink() because you're not using that.
Created attachment 255434 [details] [review] fix blank lines on resizing v4 thanks! there was wrong cursor_row calculation, instead scroll_delta we should use insert_delta, now working whith scrolling
Created attachment 255439 [details] [review] fix blank lines on resizing v5 insert_delta must be limited too
(In reply to comment #31) > Denis, see the video from comment 23, your patch produces exactly the same > (lines are disappearing, then seq's output misses some numbers). i can't reproduce this.
Created attachment 255441 [details] ls output for reproducing comment 23 The exact output might matter. Please make sure to use vte-0.34.8 with either your patch or mine, and start it from vte source's root directory. I'm not sure if the username or colors matter, so I attach the color output of my "ls -l", please replace the "ls -l; ls -l" command with "cat lsoutput.txt; cat lsoutput.txt". The exact parameters (number of rows, etc.) might matter too, please take care to make the exact same steps.
If still doesn't work, try setting the prompt (bash) to the same as mine: PS1=$'\\[\E[0;3;38;5;34m\\]egmont@foo:\\w$\\[\E[0m\\] ' I'm really not sure where this bug lies :)
If you don't explicitly shrink the ring (as I did in my patch) then it'll be vte_terminal_set_scrollback_lines() that calls _vte_ring_shrink(). This means: - I was wrong assuming your approach would leave ring_next pointing to further than the bottom of the screen. (However, I'd still prefer to see an explicit _vte_ring_shrink in the resizing method rather than some magic after I-don't-know-how-many function calls that might easily break one day.) - Once again I suspect _vte_ring_shrink() being the culprit for this bug.
ok, i confirm, bug from comment 23 is present with my patches too. (does not depends on prompt) except that,my latest patch working good for me
We might be seeing two bugs actually. I'm not sure how much they depend on each other. I've made _vte_ring_shrink() immediately return without doing anything. The disappearing lines bug remains. The numbers missing from seq's output bug is gone. I have no idea... probably it's time for me to "shut up and hack" :)
Egmont, shouldn't be hard to debug why _vte_ring_shrink() is returning immediately, right?
The disappearing lines bug is independent from this, reproducible with mainstream vte. Moved to a separate bug 708496 because this one already got too complex.
Behdad: I meant I explicitly put a "return;" at the beginning of _vte_ring_shrink to see what happens. The disappearing lines problem remained, which is an independent bug present in current vte. The "seq print 18 19 21 22..." bug disappeared, which suggests to me that most likely _vte_ring_shrink is the culprit here.
Both the "suddenly disappearing lines" and the "numbers are missing from seq's output" are present in current official VTE, and are caused by the same ring corruption triggered by colors. So the good news is that it's one bug and not two :) Let's track in bug 708496 and get back here once that's solved :)
When it starts being buggy, in _vte_ring_thaw_row() in the do_truncate branch the "records[0].text_offset < ring->last_attr.text_offset" condition is true, but the next one is false. So ring->last_attr.whatever are not updated, and this seems to be strongly related. Within the do_truncate branch, swapping the order of this "if" stuff, and actually truncating the three streams looks good at the very first glimpse. It's 2am and I can hardly keep my eyes open and I have no idea what I'm doing :D
E.g. I'm commenting to the wrong bug. Sorry.
Created attachment 255546 [details] [review] fix blank lines on resizing v6 now, firstly removes rows below cursor, then adjust cursor position i hope it simple enough and easy to understand
(In reply to comment #43) > Both the "suddenly disappearing lines" and the "numbers are missing from seq's > output" are present in current official VTE, and are caused by the same ring > corruption triggered by colors. So the good news is that it's one bug and not > two :) Let's track in bug 708496 and get back here once that's solved :) good job! but we should divide bugs and patches. my original bug (about resizing and zsh prompt) completely resolved, now patch should be reviewed
Created attachment 255550 [details] [review] fix blank lines on resizing v7 now, firstly removes rows below cursor, then adjust cursor position i hope it simple enough and easy to understand (previous patch was tested only with interactively resizing, so actually is wasn't working, idea was correct but calculations was wrong )
video with latest patch ftp://linvinus.ru/forum/vte/93_fix_resizing_v7.ogv
Denis, with the greatest respect and appreciation towards the work you're doing here: - Solving the ZSH issue (your patch or mine, doesn't matter) makes VTE trigger bug 708496 way more often than it used to, because it became a much more frequent event that the ring is truncated. So fixing this bug without fixing the other would cause a severe regression in some use cases. This is why from a practical point of view I'd say that this bug depends on that other one. (I'm quite close to fixing the other one and I think both issues should be resolved by the next vte release. It's just much easier to verify this one if the other one's already fixed.) - I have a feeling that you focus on your particular ZSH problem only and nothing else. I have shown you another bug with scrolling that's addressed by my patch but not yours, you just ignored that. I have mentioned that I'm working on rewrapping bug 336238 and that bug is going to need heavy changes to the code you're touching now, in ways that you don't foresee now but I do. I dont feel confident touching your code, but I would have to; whereas my patch is essentially a backport of some work I had already done there shortly before you opened this ticket. - I believe that my patch is better documented and more readable than yours (of course YMMV), and definitely much easier to debug. - I believe that my (second) patch perfectly fixes the ZSH issue you reported, but I haven't heard back from you on this one. I'm still going to believe this, unless you prove me wrong. With all these in mind, I'd really prefer if: - First we waited until bug 708496 is fixed. (I'll probably do it today.) - Then VTE developers would prefer my approach over your one, unless of course they have a good reason not to. Denis please really don't take this an offense because I don't mean to, it's just that I already see the next step to be taken once this bug is fixed and I'm already working towards that goal.
(In reply to comment #50) > Denis, with the greatest respect and appreciation towards the work you're doing > here: > > - Solving the ZSH issue (your patch or mine, doesn't matter) makes VTE trigger > bug 708496 way more often than it used to, because it became a much more > frequent event that the ring is truncated. So fixing this bug without fixing > the other would cause a severe regression in some use cases. This is why from a > practical point of view I'd say that this bug depends on that other one. (I'm > quite close to fixing the other one and I think both issues should be resolved > by the next vte release. It's just much easier to verify this one if the other > one's already fixed.) ok i agree. > > - I have a feeling that you focus on your particular ZSH problem only and > nothing else. I have shown you another bug with scrolling that's addressed by > my patch but not yours, you just ignored that. not true! i have make some tests in this reply https://bugzilla.gnome.org/show_bug.cgi?id=708213#c12 after that i saw only new egmonts patch https://bugzilla.gnome.org/show_bug.cgi?id=708213#c19 which was described later as "My patch is not okay, after a while " https://bugzilla.gnome.org/show_bug.cgi?id=708213#c21 So I decided not to test it, that is all. I'm sorry if I offended you. now, i can confirm that https://bugzilla.gnome.org/attachment.cgi?id=255338 also resolve 708213 bug. >I have mentioned that I'm > working on rewrapping bug 336238 and that bug is going to need heavy changes to > the code you're touching now, in ways that you don't foresee now but I do. my previous bug report (663612) and patch was waiting for two years to be resolved. so, i believe that your rewrapping work is great, but needs a lot of time before be merged in to upstream. also some of your bugs i have never saw in my practice, so i can't say is they present or not. problem with zsh is related to me and some users that uses my version of drop-down terminal. I'm using my personal patсh for a while, but didn't create a bug report about it, because my previous bug report was not resolved, and actually i lose hope that it will be resolved in a future. But some time ago i get message that it probably was resolved in latest version. That is why i create new bug report about another known problem, and share my solution. > I dont feel confident touching your code, but I would have to; whereas my patch > is essentially a backport of some work I had already done there shortly before > you opened this ticket. > > - I believe that my patch is better documented and more readable than yours (of > course YMMV), and definitely much easier to debug. > > - I believe that my (second) patch perfectly fixes the ZSH issue you reported, > but I haven't heard back from you on this one. I'm still going to believe this, > unless you prove me wrong. > > With all these in mind, I'd really prefer if: > > - First we waited until bug 708496 is fixed. (I'll probably do it today.) > > - Then VTE developers would prefer my approach over your one, unless of course > they have a good reason not to. Denis please really don't take this an offense > because I don't mean to, it's just that I already see the next step to be taken > once this bug is fixed and I'm already working towards that goal. i have no plans to be a vte developer, if you wish resolve bugs i will not stop you i just wish that bugs resolves quickly ;) by the way, yours patch, as mine, have same problem :) if line length in bash history is longer than free cols in current line then content will printed in same line but from the begging. check out this video ftp://linvinus.ru/forum/vte/bug_line_length.ogv
(In reply to comment #51) > not true! i have make some tests in this reply > https://bugzilla.gnome.org/show_bug.cgi?id=708213#c12 > after that i saw only new egmonts patch > https://bugzilla.gnome.org/show_bug.cgi?id=708213#c19 > which was described later as "My patch is not okay, after a while " > https://bugzilla.gnome.org/show_bug.cgi?id=708213#c21 > So I decided not to test it, that is all. > I'm sorry if I offended you. No, you didn't offend at all! :) In fact, it was that I hit that other bug and thought it was a bug in my patch. > now, i can confirm that https://bugzilla.gnome.org/attachment.cgi?id=255338 > also resolve 708213 bug. Thanks! > my previous bug report (663612) and patch was waiting for two years to be > resolved. VTE developers are essentially 1.5 people with zillions of other things to do. A patch gets much better attention than a simple bug report. Btw that bug was fixed by me, I'm sorry that I didn't find your bug report and filed my duplicate one :D > so, i believe that your rewrapping work is great, but needs a lot of time > before be merged in to upstream. I really can't tell. It's a feature many people request, Behdad seems to be eager to have it in the code, and my current patch is essentially working great, the main drawback being the slowdown starting at around 100.000 scrollback lines. I happen to have some free time now and I can spend a couple of days making it faster. I do have hopes for this work to hit mainstream in a reasonably short time. > I'm using my personal patсh for a while, but didn't create a bug report about > it, because my previous bug report was not resolved, and actually i lose hope > that it will be resolved in a future. Yes sometimes it takes months (or even longer) for your work to get merged, even if you have a patch. Still, I encourage everyone to file bugs and submit patches/workarounds as soon as you have them, who knows, at some point maybe VTE developers will have some time when you don't. > if line length in bash history is longer than free cols in current line then > content will printed in same line but from the begging. bash requires that any non-printable characters in your prompt are surrounded by \[ and \]. Your prompt should be PS1='\[\033[J\]\u@\h:\w\$ '
(In reply to comment #52) > I really can't tell. It's a feature many people request, Behdad seems to be > eager to have it in the code, and my current patch is essentially working > great, the main drawback being the slowdown starting at around 100.000 > scrollback lines. I happen to have some free time now and I can spend a couple > of days making it faster. I do have hopes for this work to hit mainstream in a > reasonably short time. usually i change width less often than height, so, as for me, it is not so big problem. is it possible to make it optionally? i'd prefer more performance than rewrapping. in my application usually opened about 20 terminal tabs (https://github.com/linvinus/AltYo) so memory usage and cpu usage is critical for me. > bash requires that any non-printable characters in your prompt are surrounded > by \[ and \]. Your prompt should be > PS1='\[\033[J\]\u@\h:\w\$ ' thanks! working.
Created attachment 255607 [details] [review] fix blank lines on resizing v8 no changes required in vteseq.c, now my version of the patch is finished.
> is it possible to make it optionally? i'd prefer more performance than > rewrapping. Let me suggest move the discussion to that bug :)
Egmont Koblinger, ok, i will. i carefully compare your patch with mine the main differences is: 1) you are use strict calculation of insert_delta screen->insert_delta = _vte_ring_next(ring) - terminal->row_count; i think it is good idea 2) you are always scroll content (call vte_terminal_queue_adjustment_value_changed) others things looks similar. I don't understand for what reason you separate was_scrolled_to_bottom,was_scrolled_to_top, "was_scrolled_to_the_middle" all of this cases, should use the same formula for calculation, but in your patch it uses ton of code
(In reply to comment #56) > 2) you are always scroll content (call > vte_terminal_queue_adjustment_value_changed) Check the implementation of this method. It does nothing if called with the old value. I see no point in duplicating this check. > I don't understand for what reason you separate > was_scrolled_to_bottom,was_scrolled_to_top, "was_scrolled_to_the_middle" "was_scrolled_to_top" introduces a behavior that I find quite pleasant: once you scroll back to the top, it "snaps" there and you remain scrolled to the top after a resize. This special case is not necessary but I think it's nice to have. "was_scrolled_to_bottom" is stronger than "was_scrolled_to_the_middle" in a sense that it has to win even if "was_scrolled_to_top" is also true at the same time. I guess we could if (was_scrolled_to_top && !was_scrolled_to_bottom) /* special case for top */ else /* generic case for the rest */ but the rewrap stuff would unfortunately also need an inner "if" for the generic case, the code would be less straightforward, much harder to understand the intent, and we'd also use separate debugging info. My code follows the logic I'd use when describing the desired behavior in English text. I'd say something like "The strongest rule is that if the stuff fits, it should be aligned to top. If not, the second strongest rule is that if we were scrolled to the bottom then let's keep it this way. If not, the third nice-to-have behavior should be that if we were scrolled to top then keep this way. If none of these, the fourth case would probably be trying to keep the bottom visible row at the bottom (as per rewrap experiments, note that neither current vte nor your code does this)." The first two are clear rules, the next two are nice to have behaviors that I arrived to after experimenting with possible behaviors. Someday someone might come up with something even better and want to modify these without breaking the rest. You seem to be more thinking in terms of "deltas" (just like the current code does), i.e. how much the insert/scroll offset changes. Unfortunately I can't see how this way of thinking can be kept with rewrapping. That's why I completely threw out this code and came up with a brand new approach thinking in "old state" and "new state" much rather than the diff between them. > all of this cases, should use the same formula for calculation, Why? Four different cases as I outlined above. Two of them (2nd and 4th) might actually use the same formula right now but the 2nd this is clearly the right thing to do, whereas for the 4th it's disputable. > but in your patch it uses ton of code If you strip off comments and debug stuff, you'll see that our patches use pretty much the same amount of code.
ok, thanks for clarification. i hope that necessary changes will be merged soon. status of this bug is still UNCONFIRMED :(
Review of attachment 255607 [details] [review]: my patch still have a bugs :(
Review of attachment 255338 [details] [review]: working for me, i will testing it on my workstation.
Created attachment 255688 [details] [review] egmont's third patch Cleanup: Fix a misindented closing brace in debug.c in my previos patch, and use official spelling for XTerm. No functional change at all compared to my previous (second) patch.
*** Bug 707572 has been marked as a duplicate of this bug. ***
Simple reproducer from bug 707572: - ./configure --enable-debug - start vte - execute: echo -e '\e[J' - execute: sleep 1000000 - press enter ~8-10 times (the cursor is about mid-screen) - slowly shrink the window to ~5 lines - slowly enlarge the window to ~30 lines. Note that you'd expect the prompt and command to re-appear but they don't - slowly shrink the window. At about the original size suddenly the prompts and commands re-appear and the cursor jumps to almost the bottom. Keep shrinking until the cursor reaches the very bottom, and then keep shrinking by at least 1 more. The terminal is now ~15-20 lines tall. - press space - Vte-2.90:ERROR:vte.c:4368:vte_terminal_process_incoming: assertion failed: (screen->cursor_current.row >= screen->insert_delta) Aborted
Pushed Egmont's third patch to vte-0.34.
*** Bug 707592 has been marked as a duplicate of this bug. ***
Behdad, please don't forget about bug 708496 - with this ZSH fix we'll be hitting that bug much more often.