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

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

Merged
4 commits merged into from
Oct 1, 2019

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Sep 26, 2019

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 @Treit's sample against the Terminal.
  • Attempting to author automated test with @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
Copy link
Member Author

miniksa commented Sep 26, 2019

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.

Copy link
Member

@zadjii-msft zadjii-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 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

…tat. Writes a bisecting character to the right most cell in the window.
@DHowett-MSFT
Copy link
Contributor

image

@miniksa miniksa marked this pull request as ready for review September 30, 2019 03:57
@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 30, 2019
@ghost
Copy link

ghost commented Sep 30, 2019

Hello @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
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -381,6 +381,18 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView)
}
}

// 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
Copy link
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
DHowett-MSFT pushed a commit that referenced this pull request Oct 3, 2019
* 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
DHowett-MSFT pushed a commit that referenced this pull request Oct 4, 2019
DHowett-MSFT pushed a commit that referenced this pull request Oct 15, 2019
DHowett-MSFT pushed a commit that referenced this pull request Oct 16, 2019
@ghost
Copy link

ghost commented Oct 23, 2019

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows terminal hangs after running ripgrep in a loop
5 participants