make filling chars (and, thus, erase line/char) unset wrap #2831
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miniksa
approved these changes
Sep 20, 2019
DHowett-MSFT
approved these changes
Sep 23, 2019
ghost
added
the
Needs-Author-Feedback
zadjii-msft
reviewed
Sep 24, 2019
ghost
removed
the
Needs-Author-Feedback
DHowett-MSFT
approved these changes
Sep 24, 2019
There was a problem hiding this comment.
@miniksa mind looking at these new tests? They seem sound to me.
Sure. |
miniksa
approved these changes
Sep 26, 2019
There was a problem hiding this comment.
I'm good with the tests, but a few things made me cringe a bit.
Please at least fix the VERIFY_WIN32_BOOL_SUCCEEDED and the fill-allocator for the std::wstring.
If you want to 'Won't Fix' using the string comparators instead of character by character, that's fine.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DHowett-MSFT
changed the title
DHowett-MSFT
added
the
AutoMerge
|
Hello @DHowett-MSFT! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
DHowett-MSFT
removed
the
AutoMerge
DHowett-MSFT
pushed a commit
that referenced
this pull request
Oct 3, 2019
EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value Now we're making this bool a std::optional to allow for a ternary state. This allows for us to handle the following cases completely. Refer to the table below: ,- current wrap value | ,- are we filling the last cell in the row? | | ,- new wrap value | | | ,- comments |-- |-- |-- | | 0 | 0 | 0 | | 0 | 1 | 0 | | 0 | 1 | 1 | THIS CASE WAS HANDLED CORRECTLY | 1 | 0 | 0 | THIS CASE WAS UNHANDLED | 1 | 0 | 1 | | 1 | 1 | 1 | To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes #1126
|
🎉 Handy links: |
ghost
mentioned this pull request
5 tasks
DHowett-MSFT
pushed a commit
that referenced
this pull request
Oct 17, 2019
EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value Now we're making this bool a std::optional to allow for a ternary state. This allows for us to handle the following cases completely. Refer to the table below: ,- current wrap value | ,- are we filling the last cell in the row? | | ,- new wrap value | | | ,- comments |-- |-- |-- | | 0 | 0 | 0 | | 0 | 1 | 0 | | 0 | 1 | 1 | THIS CASE WAS HANDLED CORRECTLY | 1 | 0 | 0 | THIS CASE WAS UNHANDLED | 1 | 0 | 1 | | 1 | 1 | 1 | To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes #1126 (cherry picked from commit 4dd9f9c)
DHowett-MSFT
pushed a commit
that referenced
this pull request
Oct 17, 2019
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705) fixes GH-1222 PSReadline calls SetConsoleCursorPosition on each character. We try to snap. However, we'd only ever do this with the visible viewport, the viewport defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in Terminal Scrolling mode, we actually need to snap the virtual viewport, so that this behavior looks more regular. (cherry picked from commit 6f4b98a) - Passthrough BEL in conpty (GH-2990) fixes GH-2952. (cherry picked from commit 6831120) - make filling chars (and, thus, erase line/char) unset wrap (GH-2831) EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line. Originally, we had a boolean `setWrap` that would mean... - **true**: if writing to the end of the row, SET the wrap value to true - **false**: if writing to the end of the row, DON'T CHANGE the wrap value ,- current wrap value | ,- fill last cell? | | ,- new wrap value | | | ,- comments |-|-|-| |1|0|0| THIS CASE WAS UNHANDLED To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have ~setWrap~ `wrap` mean the following: - **true**: if writing to the end of the row, SET the wrap value to TRUE - **false**: if writing to the end of the row, SET the wrap value to FALSE - **nullopt**: leave the wrap value as it is Closes GH-1126 (cherry picked from commit 4dd9f9c) - When reverse indexing, preserve RGB/256 attributes (GH-2987) (cherry picked from commit 847d6b5) - do not allow CUU and CUD to move "across" margins. (GH-2996) If we're moving the cursor up, its vertical movement should be stopped at the top margin. Similarly, this applies to moving down and the bottom margin. Fixes GH-2929. (cherry picked from commit 0ce08af) - Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651) fixes GH-2319 (cherry picked from commit b97db63) - Render the cursor state to VT (GH-2829) (cherry picked from commit a9f3849) - Don't put NUL in the buffer in VT mode (GH-3015) (cherry picked from commit a82d6b8) - Return to ground when we flush the last char (GH-2823) The InputStateMachineEngine was incorrectly not returning to the ground state after flushing the last sequence. That means that something like alt+backspace would leave us in the Escape state, not the ground state. This makes sure we return to ground. Additionally removes the "Parser.UnitTests-common.vcxproj" file, which was originally used for a theoretical time when we only open-sourced the parser. It's unnecessary now, and we can get rid of it. Also includes a small patch to bcz.cmd, to make sure bx works with projects with a space in their name. (cherry picked from commit 53c81a0) - Add support for passing through extended text attributes, like… (GH-2917) ... Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
EraseInLine calls
FillConsoleOutputCharacterW(). In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANY FILL operation. Now a fill operation UNSETS the wrap flag if we fill to the end of the line.PR Checklist
Detailed Description of the Pull Request / Additional comments
Originally, we had a boolean
setWrapthat would mean...Now we're making this bool a std::optional to allow for a ternary state. This allows for us to handle the following cases completely. Refer to the table below:
001110To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have
setWrapwrapmean the following:Validation Steps Performed
I feel like we should add a test for this case. But idk where or how. Help?