[Scenario] 2 Resize 2 Reflow #5800
Labels
Area-CodeHealth
Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc.
Issue-Scenario
Product-Conpty
For console issues specifically related to conpty
Milestone
Comments
ghost
added
Needs-Triage
DHowett-MSFT
removed
the
Needs-Triage
zadjii-msft
added
Area-CodeHealth
I'm pretty sure we're at least on |
DHowett-MSFT
pushed a commit
that referenced
this issue
May 12, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in #5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by #5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno #5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in #5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at #5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes #5839
DHowett-MSFT
pushed a commit
that referenced
this issue
May 12, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in #5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by #5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno #5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in #5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, #5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at #5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes #5839 (cherry picked from commit b4c33dd)
jelster
pushed a commit
to jelster/terminal
that referenced
this issue
May 28, 2020
This PR reverts a relatively minor change that was made incorrectly to ConPTY in microsoft#5771. In that PR, I authored two tests. One of them actually caught the bug that was supposed to be fixed by microsoft#5771. The other test was simply authored during the investigation. I believed at the time that the test revealed a bug in conpty that was fixed by _removing_ this block of code. However, an investigation itno microsoft#5839 revealed that this code was actually fairly critical. So, I'm also _skipping_ this buggy test for now. I'm also adding a specific test case to this bug. The problem in the bugged case of `WrapNewLineAtBottom` is that `WriteCharsLegacy` is wrapping the bottom row of the ConPTY buffer, which is causing the cursor to automatically move to the next line in the buffer. This is because `WriteCharsLegacy` isn't being called with the `WC_DELAY_EOL_WRAP` flag. So, in that test case, * The client emits a wrapped line to conpty * conpty fills the bottom line with that text, then dutifully increments the buffer to make space for the cursor on a _new_ bottom line. * Conpty reprints the last `~` of the wrapped line * Then it gets to the next line, which is being painted _before_ the client emits the rest of the line of text to fill that row. * Conpty thinks this row is empty, (it is) and manually breaks the row. However, the test expects this row to be emitted as wrapped. The problem comes from the torn state in the middle of these frames - the original line probably _should_ remain wrapped, but this is a sufficiently rare case that the fix is being punted into the next release. It's possible that improving how we handle line wrapping might also fix this case - currently we're only marking a row as wrapped when we print the last cell of a row, but we should probably mark it as wrapped instead when we print the first char of the _following_ row. That work is being tracked in microsoft#5800 ### The real bug in this PR The problem in the `DeleteWrappedWord` test is that the first line is still being marked as wrapped. So when we get to painting the line below it, we'll see that there are no characters to be printed (only spaces), we emit a `^[20X^[20C`, but the cursor is still at the end of the first line. Because it's there, we don't actually clear the text we want to clear. So DeleteWrappedWord, microsoft#5839 needs the `_wrappedRow = std::nullopt;` statement here. ## References * I guess just look at microsoft#5800, I put everything in there. ## Validation Steps Performed * Tested manually that this was fixed for the Terminal * ran tests Closes microsoft#5839
This was referenced Aug 2, 2021
This was referenced Nov 16, 2021
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
During 1.0, we discovered a ton of bugs in our resize with reflow implementation, and now that we need to keep both the conpty and the terminal in sync on these things, it's more important than ever to do these things right.
I'm collecting up all the straggling tasks I've found that might possibly be related.
This is a combo of "Make Reflow work correctly", and "Get rid of the spaghetti in the VtEngine related to line wrapping"
(see also #4200, the original reflow mega thread)
Issues
ScrollFrameoptimizations #5228 Conpty: AdditionalScrollFrameoptimizationsTextBuffer::ReflowTerminal::_WriteBuffer#12440, which might just be Implement stream writing mechanism on buffer (a.k.a. WriteCharsLegacy2ElectricBoogaloo) #780Things I think will all be fixed by #8000
PRs
Many of these are 1.0 PRs, but they've all got important context.
1.0 PRs
lessin Git for Windows Fix wrapped lines inlessin Git for Windows #5771vim.execorrupts buffer #5428 Add a test to cover #5428 #5449lessin Git for Windows #5771 Fix an accidental regression from #5771 #5870The text was updated successfully, but these errors were encountered: