Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double check how we reflow for a huge height delta #16033

Open
zadjii-msft opened this issue Sep 26, 2023 · 0 comments
Open

Double check how we reflow for a huge height delta #16033

zadjii-msft opened this issue Sep 26, 2023 · 0 comments
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Sep 26, 2023

Yes, exactly that. I did it for this test specifically:

TestBuffer{
{ 6, 5 },
{
{ L"ABCDEF", false },
{ L"$ ", false },
{ L"GHIJKL", false },
{ L"MNOPQR", false },
{ L"STUVWX", false },
},
{ 0, 1 } // cursor on $
},
TestBuffer{
{ 5, 5 }, // reduce width by 1
{
{ L"F$ ", false },
{ L"GHIJK", true }, // [BUG] We should see GHIJK\n L\n MNOPQ\n R\n
{ L"LMNOP", true }, // The wrapping here is irregular
{ L"QRSTU", true },
{ L"VWX ", false },
},
{ 1, 1 } // [BUG] cursor moves to 1,1 instead of sticking with the $
},

$ 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:

TestBuffer{
{ 6, 5 },
{
{ L"ABCDEF", false },
{ L"$ ", false },
{ L"GHIJKL", false },
{ L"MNOPQR", false },
{ L"STUVWX", false },
},
{ 0, 1 }, // cursor on $
},
TestBuffer{
{ 5, 5 }, // reduce width by 1
{
{ L"$ ", false },
{ L"GHIJK", true },
{ L"L ", false },
{ L"MNOPQ", true },
{ L"R ", false },
},
{ 0, 0 },
},

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 #15701 (comment)

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?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 26, 2023
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. labels Sep 26, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed Needs-Tag-Fix Doesn't match tag requirements labels Sep 26, 2023
@carlos-zamora carlos-zamora added this to the Backlog milestone Sep 27, 2023
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 27, 2023
@lhecker lhecker removed their assignment May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

3 participants