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

Resize with Reflow loses leading spaces on resize operation #797

Closed
miniksa opened this issue May 14, 2019 · 0 comments · Fixed by #15783 or #15701
Closed

Resize with Reflow loses leading spaces on resize operation #797

miniksa opened this issue May 14, 2019 · 0 comments · Fixed by #15783 or #15701
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Milestone

Comments

@miniksa
Copy link
Member

miniksa commented May 14, 2019

To do this, we need to do #796 first.

This was a customer feedback bug MSFT:16934245. I didn't feel confident touching it because Resize with Reflow is sorta sketchy and was done before we really hit our stride on this project.

Here's the original Repro Steps:

Repro:

  1. Create a new C# console application.

  2. Use Console.WriteLine() to ask the user a question.

  3. Then use the ReadFromConsole_YesNo() method below to ask for a yes or no answer.

  4. Before pressing any key, maximize the console window that gets launched by VS.

ER: Insertion point remains indented by 4 spaces

AR: Insertion point jumps back to start of line

Once we have some actual tests around this, we might be able to dial in the resize with reflow algorithm to handle this better.

   /// <summary>
   /// Continuously reads lines of input from the console until a yes or no answer is received.
   /// Returns true if the read answer was "yes".  Otherwise, false is returned for "no".
   /// </summary>
   static bool ReadFromConsole_YesNo()
   {
       // Keep reading answers from the console until we get a "yes" or "no".
       while (true)
       {
           Console.Write("    "); // Indent input answers for readability
           Console.Out.Flush();
   
           string line = Console.ReadLine().Trim().ToLower(); // Get next answer
   
           // Check if it's a "yes" or "no".
           if      (line.Equals("yes")) {return true;}
           else if (line.Equals("y"))   {return true;}
           else if (line.Equals("no"))  {return false;}
           else if (line.Equals("n"))   {return false;}
       }
   }
@miniksa miniksa added Product-Conhost For issues in the Console codebase Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) labels May 14, 2019
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@miniksa miniksa added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Mass-Chaos labels May 17, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label May 12, 2020
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone May 12, 2020
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Backlog Jul 5, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 15, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 25, 2023
DHowett pushed a commit that referenced this issue Sep 26, 2023
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅
DHowett pushed a commit that referenced this issue Sep 26, 2023
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
  (No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
  future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
  (At 120x9001 with a full buffer I get 60ms -> 2ms.)

Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.

Closes #797
Closes #3088
Closes #4968
Closes #6546
Closes #6901
Closes #15964
Closes MSFT:19446208

Related to #5800 and #8000

## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
  (Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅

(cherry picked from commit 7474839)
Service-Card-Id: 90642727
Service-Version: 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants