Fix logic for reflowing cursor when growing columns, after shrinking columns

[Advait-M]

• Noticed that if you do a sequence of shrink_columns() and then grow_columns() the Grid's cursor is not being reflowed back to its original position.
• Upon digging into this, found out that this was due to let mut target = self.cursor.point.sub(self, Boundary::Cursor, num_wrapped); only being able to subtract until it saturates at the Boundary. Specifically, in this case, the cursor was hitting the boundary of (0, 0) and was not getting reflowed correctly.
• I believe the fix is to rotate the cursor downwards if the current row is being entirely reflowed (nothing leftover i.e. is_clear). Then, we can appropriately subtract num_wrapped, which gets us to the right target, that we can apply with line_delta.
• Added a unit test to verify this behavior and illustrate an example of this case. Specific details:
  • We set up a Grid with 3 rows and 8 columns, which 5 characters on the first line. We position the Cursor at (0, 5).
  • When we shrink the grid to 3 columns, we expect 1 row to go into scrollback history. Hence, the new Cursor will be at (0, 2) (the first line has the first 3 cells and rest are reflowed onto the second line).
  • Then, when we grow the grid back out to 8 columns, I believe that we expect the row to come OUT of scrollback history and for the Cursor to be reflowed back to (0, 5).
  • Prior to my change, I was seeing the cursor reflowed to (0, 0) due to the saturation with subtraction at the Boundary and 1 row staying in scrollback history.
• Please let me know if I misunderstood any piece of this reflowing logic or how the visible lines vs scrollback history works! I was pretty surprised to discover this in Alacritty - so I may very well have missed something conceptually here in terms of the cases possible or intended behavior! 😄
• Verified all existing tests pass via cargo test. Verified new test passes.
• Verified Alacritty resizing still works locally via cargo run.
• @chrisduerr for review/discussion perhaps? Noticed you'd worked on this code originally it seemed. Thanks!

[chrisduerr]

Noticed that if you do a sequence of shrink_columns() and then grow_columns() the Grid's cursor is not being reflowed back to its original position.

What makes you think it should? Have you tested other terminal emulators that support terminal reflow?

Also could you provide a video of your issue or more detailed reproduction steps please?

[Advait-M]

Noticed that if you do a sequence of shrink_columns() and then grow_columns() the Grid's cursor is not being reflowed back to its original position.

What makes you think it should? Have you tested other terminal emulators that support terminal reflow?

Also could you provide a video of your issue or more detailed reproduction steps please?

Thanks for the response!

1. Logically, I'd expect a shrink and then a reversal of the shrink (aka grow) action to result in the cursor position being unchanged. Due to the issue below (the shell reprinting characters), it's hard to show a visual example of this in a terminal directly (as opposed to a unit test, which I've included).
2. Spent the last few hours digging deeper into this - it's hard to pinpoint noticeable impact on terminal behavior in Alacritty since I believe the shell re-prints characters in the case of a resize event. This means that we receive input() events that then cause Alacritty to call wrapline() appropriately - in essence, the reflowing of text from the resize event is discarded (in the case of the active prompt/command). In the case of finished prompts/commands, we solely rely on the terminal emulator to reflow the text appropriately, however, we normally do not have a cursor in these finished commands to reflow.

I noticed this as I was debugging a related issue for Warp, where we have similar text reflowing logic (and I was comparing to other terminals). In Warp, it does result in a visible issue, since there's multiple grids and cursors exist in "finished" grids, where the cursor reflow problem manifests itself more clearly and results in incorrect comparisons.

I believe the set-up I used in the unit test to repro this bug should be possible in a non-interactive shell (ie with a shell script) perhaps, rather than an interactive shell (I can use ANSI escape codes to manipulate the cursor positions directly). I'll try to see if I can get a repro with these methods - one of the challenges here is needing the saturating subtraction to hit the boundary at (0, 0) too. If I can force a situation where I avoid the shell re-printing characters + do a terminal shrink and grow, I think it should be able to manifest in an incorrect cursor position.

[chrisduerr]

Spent the last few hours digging deeper into this - it's hard to pinpoint noticeable impact on terminal behavior in Alacritty since I believe the shell re-prints characters in the case of a resize event.

The easiest way to test this stuff is to just sleep infinity. That will deactivate all shell interactions since… no shell is running.

[Advait-M]

Spent the last few hours digging deeper into this - it's hard to pinpoint noticeable impact on terminal behavior in Alacritty since I believe the shell re-prints characters in the case of a resize event.

The easiest way to test this stuff is to just sleep infinity. That will deactivate all shell interactions since… no shell is running.

Ah haha, I recently landed at the same conclusion before seeing this message (I did a sleep 5000) 😂 - great minds think alike ❤️ !! Thanks for the tip @chrisduerr!

Got a repro!

1. Loom demo'ing bug on Alacritty: https://www.loom.com/share/f63d80de1b95407fb9b2bb1f3044d01f - illustrates production Alacritty alongside my local build (with proposed fix).
2. Loom demo'ing same commands on default Mac terminal + iTerm: https://www.loom.com/share/0e685a8f732941b49c6f5265302feab4. Default Mac terminal has correct behavior. iTerm is not fully correct but close to it.

Note that (1) above leads me to believe my fix is a partial fix and not a completely correct fix - I'd appreciate help in grokking this resize logic and coming up with a correct fix!

The command to repro:

echo -ne "\033[1;15H" && sleep 5000 && 12324234324324

[chrisduerr]

The reason why the default terminal works and iTerm/Alacritty don't is because it doesn't push the text into history. There's no way to recover the cursor if the text you're tracking is moving off the screen, since the cursor can't follow there.

I'm not sure if it's viable to not push the line into the history. Generally this is desired for at least some lines and one would have to test general reflow behavior with the macOS terminal to determine if their solution is even viable or not. However the reflow logic is based on cursor position iirc, so maybe there's some special logic that can be done to prevent moving the cursor's line into history.

I do not see a significant improvement from your patch itself, though I haven't tested it on my system.

I would expect fixing this bug to be pretty complicated, just because there's so many edgecases and possibilities to introduce regressions. I don't think the current behavior causes any significant issues or correctness problems, so personally I'd avoid spending the time to try and "fix" this, but feel free to look into it if you're willing to potentially waste a bunch of your time.

[Advait-M]

The reason why the default terminal works and iTerm/Alacritty don't is because it doesn't push the text into history. There's no way to recover the cursor if the text you're tracking is moving off the screen, since the cursor can't follow there.

I'm not sure if it's viable to not push the line into the history. Generally this is desired for at least some lines and one would have to test general reflow behavior with the macOS terminal to determine if their solution is even viable or not. However the reflow logic is based on cursor position iirc, so maybe there's some special logic that can be done to prevent moving the cursor's line into history.

I do not see a significant improvement from your patch itself, though I haven't tested it on my system.

I would expect fixing this bug to be pretty complicated, just because there's so many edgecases and possibilities to introduce regressions. I don't think the current behavior causes any significant issues or correctness problems, so personally I'd avoid spending the time to try and "fix" this, but feel free to look into it if you're willing to potentially waste a bunch of your time.

Gotcha, makes sense re having history vs not having history!

Agreed that we do want to push lines into history - I think my point is moreso around ensuring that lines can come out of history when the terminal is (re)-grown in dimensions.

Re my change - I think it does result in a better experience, in that the text is still visible (as opposed to entirely disappearing - in Loom above). Though, the cursor position is not exactly correct, in that particular example.

Ack re complexity - I'm planning to fix this on the Warp side, hence was looking to help resolve this here as a side-quest. Will perhaps update the PR if I find a more comprehensive fix here (planning on more unit tests - checked the mid-command cursor behavior and that seems to be be fixed too!).

I do think the current fix I have here doesn't regress functionality and improves the experience since it basically ensures that cursor subtraction calculations can be done into history (instead of saturating at the boundary), ONLY if those lines from history are coming out of history (which should be the intended product experience). Specifically, if a row is_clear then it must be able to be FULLY reflowed by definition of is_clear and the reflow logic. Hence, we should be skipping that row in adding it to reversed/new_raw. That'll reduce history by 1 row (each time). For such instances, we should also rotate the cursor "down" 1 row since we're moving 1 row out of history, and then we can "subtract" the row with line_delta from the wrapping logic. The benefit here is that the column subtraction is corrected (with target = self.cursor.point.sub(self, Boundary::Cursor, num_wrapped)), since we no longer "bottom out" at (0, 0). Effectively, the two row-specific operations here negate themselves, but enable a correct wrapping subtraction. Am I misunderstanding this?

[chrisduerr]

I think my point is moreso around ensuring that lines can come out of history when the terminal is (re)-grown in dimensions.

That is not possible in this scenario, otherwise you'll be losing content.

I think it does result in a better experience, in that the text is still visible (as opposed to entirely disappearing - in Loom above).

I mean the text is still there, it's just in the history.

we should also rotate the cursor "down" 1 row since we're moving 1 row out of history

I believe this assumption is incorrect and could lead to rendering glitches.

[Advait-M]

I think my point is moreso around ensuring that lines can come out of history when the terminal is (re)-grown in dimensions.

That is not possible in this scenario, otherwise you'll be losing content.

I think it does result in a better experience, in that the text is still visible (as opposed to entirely disappearing - in Loom above).

I mean the text is still there, it's just in the history.

we should also rotate the cursor "down" 1 row since we're moving 1 row out of history

I believe this assumption is incorrect and could lead to rendering glitches.

Thanks for the response! Curious on some of the points you mentioned (for better understanding this):

1. In what cases could we lose content by bringing lines out of history, back into the visible rows?
2. Yep, agreed re the text is still there though being in history - yep, if I scroll, I can see the text again, which does mean it's not a terrible bug for Alacritty.
3. Interesting re rendering glitches - yeah, this is exactly what I was trying to figure out, though trying to think of cases that would break this 🤔

[chrisduerr]

In what cases could we lose content by bringing lines out of history, back into the visible rows?

The thing is that you need to take every grid state as a snapshot without considering what came before. If you flow it into history and right back it might make sense, but if you take the grid with the line just in the history and look at it like a snapshot, it's not necessarily obvious where it came from. All of this is based on a ton of heuristics, mostly wrapline flags and stuff like that, so you can take a guess. But my general assumption is that this might lead to accidentally rotating stuff down that shouldn't be, in which case it will be nuked by the shell's erase in line and be lost forever (which should obviously be avoided).

It's very complex to reason about this and this code is the first thing I think of when thinking about horrible Alacritty code, just because it's such a mess and so heuristic heavy. But generally speaking there's just a load of edgecases which is why being careful probably makes sense.

If I wanted to implement this, I would not try to rotate things back out of history after moving it into history, since there's too big a risk that will mess stuff up since you're guaranteed to lose cursor tracking. Instead I'd do what Kitty does and break the line towards the bottom of the terminal, so you can reflow it back to the top without losing cursor tracking. However I have no clue how easy that is and since this doesn't really cause any issues due to shell resets, I'd rather not look into it.

[Advait-M]

In what cases could we lose content by bringing lines out of history, back into the visible rows?

The thing is that you need to take every grid state as a snapshot without considering what came before. If you flow it into history and right back it might make sense, but if you take the grid with the line just in the history and look at it like a snapshot, it's not necessarily obvious where it came from. All of this is based on a ton of heuristics, mostly wrapline flags and stuff like that, so you can take a guess. But my general assumption is that this might lead to accidentally rotating stuff down that shouldn't be, in which case it will be nuked by the shell's erase in line and be lost forever (which should obviously be avoided).

It's very complex to reason about this and this code is the first thing I think of when thinking about horrible Alacritty code, just because it's such a mess and so heuristic heavy. But generally speaking there's just a load of edgecases which is why being careful probably makes sense.

If I wanted to implement this, I would not try to rotate things back out of history after moving it into history, since there's too big a risk that will mess stuff up since you're guaranteed to lose cursor tracking. Instead I'd do what Kitty does and break the line towards the bottom of the terminal, so you can reflow it back to the top without losing cursor tracking. However I have no clue how easy that is and since this doesn't really cause any issues due to shell resets, I'd rather not look into it.

Ah, I see. Thanks for explaining!

Haha yeah, this is some very tough code to grok, hence my curiosity/struggle in fixing this "properly", since we have very similar concepts on the Warp-side.

Hmm, yeah I think I follow that logic. Got it re your proposed fix - yeah, I think we can perhaps hold off on fixing this on the Alacritty side, though I'll poke around at this on my side.

[chrisduerr]

Converted to draft for now. Feel free to close if you have no interest in working on this.