Skip to content

Patch fix for #1360 until WriteStream (#780) can be implemented. - #2924

Merged
4 commits merged into
masterfrom
dev/miniksa/1360
Oct 1, 2019
Merged

Patch fix for #1360 until WriteStream (#780) can be implemented.#2924
4 commits merged into
masterfrom
dev/miniksa/1360

Conversation

@miniksa

@miniksa Michael Niksa (miniksa) commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Writing can enter an infinite loop with full-width characters being inserted into the last column of the buffer.

References

#780

PR Checklist

Detailed Description of the Pull Request / Additional comments

This is because the "WriteCharsLegacy2ElectricBoogaloo" implementation in the Terminal (Terminal::_WriteBuffer) is a hacky thing that we put together knowing full well that we'd have to do #780 but we had to get this working enough for now. Turns out, it's not super complete and fragile. Surprise.

Validation Steps Performed

  • Manual run of Mike Treit (@Treit)'s sample against the Terminal.
  • Attempting to author automated test with Mike Treit (@Treit)'s sample
  • I need to run it against conhost and make sure it's not failing too (and fix it there too if it is)

@miniksa

Michael Niksa (miniksa) commented Sep 26, 2019

Copy link
Copy Markdown
Contributor Author

Same code snippet against conhost doesn't crash, so I don't believe there's anything to fix there right now. This makes sense since it should hit the OG WriteCharsLegacy which has provisions to auto-newline when we pass the right side and not bisect characters.

zadjii-msft
Mike Griese (zadjii-msft) reviewed Sep 27, 2019
View reviewed changes

@zadjii-msft Mike Griese (zadjii-msft) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the record, I'm on board shipping this. I can't imagine blocking over something in test code, but I'll patiently wait for the test to sign off

@miniksa
Add a test that hangs in the broken state and passes in the success s…
f92666f 
…tat. Writes a bisecting character to the right most cell in the window.
@DHowett-MSFT

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Sep 27, 2019

Copy link
Copy Markdown
Contributor

image

@skyline75489 Chester Liu (skyline75489) mentioned this pull request Sep 29, 2019
Merged
2 of 5 tasks
@miniksa
Michael Niksa (miniksa) marked this pull request as ready for review September 30, 2019 03:57
@zadjii-msft Mike Griese (zadjii-msft) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 30, 2019
@ghost

Deleted user (ghost) commented Sep 30, 2019

Copy link
Copy Markdown

Hello Mike Griese (@zadjii-msft)!

Because this pull request has the AutoMerge 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 more here.

@DHowett-MSFT

Dustin L. Howett (MSFT) (DHowett-MSFT) commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Sep 30, 2019

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

HBelusca
Hermès BÉLUSCA - MAÏTO (HBelusca) reviewed Oct 1, 2019
View reviewed changes
Comment thread src/cascadia/TerminalCore/Terminal.cpp Outdated
}

// If we're about to try to place the cursor past the right edge of the buffer, move it down a row
// This is another patch that GH#780 should supercede. This is really correcting for other bad situations

@HBelusca Hermès BÉLUSCA - MAÏTO (HBelusca) Oct 1, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

supersede*

@ghost
ghost merged commit 5d906d9 into master Oct 1, 2019
@ghost
ghost deleted the dev/miniksa/1360 branch October 1, 2019 01:45
Dustin L. Howett (MSFT) (DHowett-MSFT) pushed a commit that referenced this pull request Oct 3, 2019
@miniksa @DHowett
Patch fix for #1360 until WriteStream (#780) can be implemented. (#2924)
776b9a5 
* Patch fix for #1360 until WriteStream (#780) can be implemented.

* Add a test that hangs in the broken state and passes in the success stat. Writes a bisecting character to the right most cell in the window.

* Code format! *shakes fist at sky*

* Update src/cascadia/TerminalCore/Terminal.cpp
Dustin L. Howett (MSFT) (DHowett-MSFT) pushed a commit that referenced this pull request Oct 4, 2019
@DHowett
Revert "Patch fix for #1360 until WriteStream (#780) can be implement…
351d326 
…ed. (#2924)"

This reverts commit 776b9a5.
Closed
@sc0ttwad3 Scott Wade (sc0ttwad3) mentioned this pull request Oct 9, 2019
Closed
Dustin L. Howett (MSFT) (DHowett-MSFT) pushed a commit that referenced this pull request Oct 15, 2019
@DHowett
Revert "Patch fix for #1360 until WriteStream (#780) can be implement…
6a35f5f 
…ed. (#2924)"

This reverts commit 5d906d9.
Merged
Dustin L. Howett (MSFT) (DHowett-MSFT) pushed a commit that referenced this pull request Oct 16, 2019
@DHowett
Revert "Patch fix for #1360 (#2924)" (#3212)
01c0736 
This reverts commit 5d906d9.
@ghost

Deleted user (ghost) commented Oct 23, 2019

Copy link
Copy Markdown

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

Handy links:

@ghost ghost mentioned this pull request Oct 23, 2019
Closed
@zadjii-msft Mike Griese (zadjii-msft) mentioned this pull request Jan 8, 2020
Merged
2 of 5 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zadjii-msft Mike Griese (zadjii-msft) zadjii-msft approved these changes
+2 more reviewers
@HBelusca Hermès BÉLUSCA - MAÏTO (HBelusca) HBelusca left review comments
@DHowett-MSFT Dustin L. Howett (MSFT) (DHowett-MSFT) DHowett-MSFT approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows terminal hangs after running ripgrep in a loop

5 participants

@miniksa @DHowett-MSFT @HBelusca @zadjii-msft @DHowett