Immediately flush a frame when we encounter a string we don't understand #2665
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
The solution here is to have the parser call up to the host to have the host paint a frame, before the parser writes the string to the terminal. This way, any state from the text before the string we didn't understand will be sent to the terminal before the string we didn't understand, which should hopefully maintain state like this. Unsure if this works with a sequence we didn't understand that's at the end of a frame all by itself, something that doesn't trigger another frame. This is hard to test unfortunately.
zadjii-msft
added
Issue-Bug
DHowett-MSFT
reviewed
Sep 5, 2019
|
Is there a risk that this will insert a full redraw (like, redraw everything for some reason) before an unknown sequence? or, after one? Additionally, I'm concerned that a consumer of this will see the right behavior at first ( Do we know how much this will impact perf? How often do we even hit unknown sequences? |
|
|
I ACK your comment but have no actionable response. Thanks! |
…rings # Conflicts: # src/host/VtIo.cpp
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DHowett-MSFT
approved these changes
Oct 4, 2019
| // - TRUE always. | ||
| BOOL ConhostInternalGetSet::PrivatePassThroughString(const wchar_t* const rgwch, const size_t cch) const noexcept | ||
| { | ||
| // return SUCCEEDED(DoSrvPrivatePassThroughString(rgwch, cch)); |
| } | ||
| else | ||
| { | ||
| engine.SetTerminalConnection(nullptr, | ||
| nullptr); | ||
| engine.EnableFlushing(nullptr); |
There was a problem hiding this comment.
this seems less like enabling flushing and more like not enabling flushing
| @@ -2521,13 +2521,11 @@ void SCREEN_INFORMATION::SetTerminalConnection(_In_ ITerminalOutputConnection* c | |||
| OutputStateMachineEngine& engine = reinterpret_cast<OutputStateMachineEngine&>(_stateMachine->Engine()); | |||
| if (pTtyConnection) | |||
| { | |||
| engine.SetTerminalConnection(pTtyConnection, | |||
| std::bind(&StateMachine::FlushToTerminal, _stateMachine.get())); | |||
| engine.EnableFlushing(std::bind(&StateMachine::FlushToTerminal, _stateMachine.get())); | |||
There was a problem hiding this comment.
I wonder if we should file low-prio followup work to clear up the ownership here: the state machine engine shouldn't need a function bound into it from the state machine itself, especially not by an unrelated class.
zadjii-msft
added a commit
that referenced
this pull request
Mar 12, 2020
…h the frame
## Summary of the Pull Request
When Conpty encounters a string we don't understand, immediately flush the frame.
## References
This PR superceeds #2665. This solution is much simpler than what was proposed in that PR.
As mentioned in #2665: "This might have some long-term consequences for #1173."
## PR Checklist
* [x] Closes #2011
* [x] Closes #4106
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated
4 tasks
|
This PR was a far too complicated solution to this bug. I've got a better solution in #4896 |
DHowett-MSFT
pushed a commit
that referenced
this pull request
Mar 12, 2020
When ConPTY encounters a string we don't understand, immediately flush the frame. ## References This PR supersedes #2665. This solution is much simpler than what was proposed in that PR. As mentioned in #2665: "This might have some long-term consequences for #1173." ## PR Checklist * [x] Closes #2011 * [x] Closes #4106 * [x] I work here * [x] Tests added/passed
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
Whenever conpty encounters a VT sequence it doesn't understand, we'll make sure to flush the current frame before writing that string through to the connected terminal application.
References
This might have some long-term consequences for #1173. IIRC I wrote some code that was fairly similar for passthrough mode. This might be a subset of that code.
PR Checklist
Detailed Description of the Pull Request / Additional comments
We need to do this because some VT sequences might be dependent upon state that's currently buffered in the frame. So we'll flush the frame so the terminal also has that state, then we'll write the sequence we didn't understand/wanted to passthrough.
Validation Steps Performed
While testing this might be a bit trickier, I wrote a test script that changes the cursor color, in the middle of a big block of text. Changing the cursor color is a VT sequence we passthrough always. Before the change, the cursor color sequence would always appear before any of the related text. After the change, the text before the color sequence is always emitted before the sequence, and the text after the sequence is always emitted after.