-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
When Conpty encounters an unknown string, flush immediately #4896
Conversation
…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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you kind making sure this works with split sequences like in my recent PR? It made it so we call FlushToTerminal two times instead of one
@@ -1894,6 +1887,15 @@ bool AdaptDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle) | |||
success = _pConApi->PrivateAllowCursorBlinking(fEnableBlinking); | |||
} | |||
|
|||
// If we're a conpty, always return false, so that this cursor state will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why after not before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this after to make the test work tbh. Doing this after means the conpty will still have this state in it the same as the Terminal, even if there's no way to query or use that information.
And finally: this will work okay, but not amazing, for things that manipulate how text is displayed — because it will happen in between the frames that render text? |
Pretty much exactly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval, assuming it DOES work with my split sequences PR #4870
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get()); | ||
|
||
auto& g = ServiceLocator::LocateGlobals(); | ||
// auto& renderer = *g.pRender; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray comment
Admin merge: I removed a comment, but our CI is in deep |
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