Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

make filling chars (and, thus, erase line/char) unset wrap#2831

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
DHowett-MSFT merged 4 commits intomasterfromdev/cazamor/bugfix-conhost-copy
Oct 1, 2019

Conversation

@carlos-zamora
Copy link
Member

@carlos-zamoracarlos-zamora commentedSep 20, 2019
edited
Loading

Summary of the Pull Request

EraseInLine callsFillConsoleOutputCharacterW(). In filling the row with chars, we were setting the wrap flag. We need to specifically not do this on ANYFILL 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 booleansetWrap 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 valueare we filling the last cell in the row?new wrap valueCOMMENTS
000
001IMPOSSIBLE
010
011THIS CASE WAS RIGHTFULLY HANDLED
100THIS CASE WAS UNHANDLED
101
110IMPOSSIBLE
111

To handle that special case (1-0-0), we need to UNSET the wrap. So now, we havesetWrapwrap 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

Validation Steps Performed

I feel like we should add a test for this case. But idk where or how. Help?

@carlos-zamoracarlos-zamora self-assigned thisSep 20, 2019
@carlos-zamoracarlos-zamora requested a review froma teamSeptember 20, 2019 22:52
Copy link
Member

@miniksaminiksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me. Especially considering we walked through it together. :D

@miniksaminiksa added the Needs-SecondIt's a PR that needs another sign-off labelSep 23, 2019
Copy link
Contributor

@DHowett-MSFTDHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For tests!

@ghostghost added the Needs-Author-FeedbackThe original author of the issue/PR needs to come back and respond to something labelSep 23, 2019
Copy link
Member

@zadjii-msftzadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I want tests too so I'm just going to comment

@ghostghost removed the Needs-Author-FeedbackThe original author of the issue/PR needs to come back and respond to something labelSep 24, 2019
Copy link
Contributor

@DHowett-MSFTDHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@miniksa mind looking at these new tests? They seem sound to me.

@miniksa
Copy link
Member

@miniksa Michael Niksa FTE mind looking at these new tests? They seem sound to me.

Sure.

Copy link
Member

@miniksaminiksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett-MSFTDHowett-MSFT changed the titleFilling chars (and, thus, erase line/char) should unset wrapmake filling chars (and, thus, erase line/char) unset wrapSep 30, 2019
@DHowett-MSFTDHowett-MSFT added the AutoMergeMarked for automatic merge by the bot when requirements are met labelSep 30, 2019
@ghost
Copy link

Hello@DHowett-MSFT!

Because this pull request has theAutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn morehere.

@DHowett-MSFTDHowett-MSFT removed the AutoMergeMarked for automatic merge by the bot when requirements are met labelSep 30, 2019
@DHowett-MSFTDHowett-MSFT merged commit4dd9f9c intomasterOct 1, 2019
@DHowett-MSFTDHowett-MSFT deleted the dev/cazamor/bugfix-conhost-copy branchOctober 1, 2019 01:16
DHowett-MSFT pushed a commit that referenced this pull requestOct 3, 2019
EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row withchars, we were setting the wrap flag. We need to specifically not do this onANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill tothe 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 valueNow we're making this bool a std::optional to allow for a ternary state. Thisallows for us to handle the following cases completely. Refer to the tablebelow:,- 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 isCloses#1126
@ghost
Copy link

🎉Windows Terminal Preview v0.5.2762.0 has been released which incorporates this pull request.:tada:

Handy links:

DHowett-MSFT pushed a commit that referenced this pull requestOct 17, 2019
EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row withchars, we were setting the wrap flag. We need to specifically not do this onANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill tothe 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 valueNow we're making this bool a std::optional to allow for a ternary state. Thisallows for us to handle the following cases completely. Refer to the tablebelow:,- 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 isCloses#1126(cherry picked from commit4dd9f9c)
DHowett-MSFT pushed a commit that referenced this pull requestOct 17, 2019
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705)fixesGH-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 commit6f4b98a)- Passthrough BEL in conpty (GH-2990)fixesGH-2952.(cherry picked from commit6831120)- make filling chars (and, thus, erase line/char) unset wrap (GH-2831)EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row withchars, we were setting the wrap flag. We need to specifically not do this onANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill tothe 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 UNHANDLEDTo 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 isClosesGH-1126(cherry picked from commit4dd9f9c)- When reverse indexing, preserve RGB/256 attributes (GH-2987)(cherry picked from commit847d6b5)- do not allow CUU and CUD to move "across" margins. (GH-2996)If we're moving the cursor up, its vertical movement should be stoppedat the top margin.Similarly, this applies to moving down and the bottom margin.FixesGH-2929.(cherry picked from commit0ce08af)- Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651)fixesGH-2319(cherry picked from commitb97db63)- Render the cursor state to VT (GH-2829)(cherry picked from commita9f3849)- Don't put NUL in the buffer in VT mode (GH-3015)(cherry picked from commita82d6b8)- Return to ground when we flush the last char (GH-2823)The InputStateMachineEngine was incorrectly not returning to the groundstate after flushing the last sequence. That means that something likealt+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, whichwas originally used for a theoretical time when we only open-sourced theparser. It's unnecessary now, and we can get rid of it.Also includes a small patch to bcz.cmd, to make sure bx works withprojects with a space in their name.(cherry picked from commit53c81a0)- 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@zadjii-msftzadjii-msftzadjii-msft left review comments

@miniksaminiksaminiksa approved these changes

+1 more reviewer

@DHowett-MSFTDHowett-MSFTDHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carlos-zamoracarlos-zamora

Labels

Needs-SecondIt's a PR that needs another sign-off

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Copying text from the command window behaves differently than previous versions

5 participants

@carlos-zamora@miniksa@DHowett-MSFT@zadjii-msft

[8]ページ先頭

©2009-2025 Movatter.jp