microsoft / microsoft/terminal

Double check how we reflow for a huge height delta

Open
#16,033 0 comments 1 reaction 0 assignees View on GitHub
Area-Output Issue-Task Product-Conhost
Dominant language
C++
Stars
105k
Forks
9.6k
Avg merge
3d 17h
Merged PRs (30d)
29

Description

Yes, exactly that. I did it for this test specifically:
https://github.com/microsoft/terminal/blob/fc4a37ee9189f2f7335ab942d50381a38a03acf2/src/buffer/out/ut_textbuffer/ReflowTests.cpp#L294-L315

`$` is the cursor. `false` in each line means that it has an explicit newline (`true` = force wrap).
It has a lot of comments about bugs in the old implementation, which is why the entire text past the cursor fits into the new buffer. In the new, fixed implementation the `false` = explicit newlines are correctly handled and so it doesn't fit anymore:
https://github.com/microsoft/terminal/blob/6374aece11edbf8b1329d56ab963e6ea2089f83d/src/buffer/out/ut_textbuffer/ReflowTests.cpp#L294-L315

Basically I had to make a choice over whether I:
* continue copying until all text in the old buffer has been consumed.
If the new cursor position is now somewhere at a negative Y, we just put it at (0,0).
* stop copying at some point to prevent the cursor from being lost.

I chose the latter. What do you think we should do?

_Originally posted by @lhecker in https://github.com/microsoft/terminal/pull/15701#discussion_r1332947703_


> > continue copying until all text in the old buffer has been consumed.
> > If the new cursor position is now somewhere at a negative Y, we just put it at (0,0).
>
> Probably that one, right? Like, the priority should be to keep the newest text, right?
>
> ... this is currently a 1.19 candidate. I'm okay to say merge as is and iterate in the first hotfix since
>
> 1. we're out of time and I'm about to log out for the day
> 2. this is a RARE edge case
> 3. this is more goodness than badness)

> This is an interesting case. I'm honestly not sure how to handle it -- we should look at how other terminals that do reflow handle it?

Contributor guide

Open the contributing guide

Research direction

Start with the reflow case in src/buffer/out/ut_textbuffer/ReflowTests.cpp at lines 294-315 and compare the two referenced revisions. Determine whether reflow should consume all old-buffer text or stop to preserve the cursor, then update the implementation and test so the chosen behavior is explicit.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
cli
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.