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
Use NotifyEndApplication
to re-enable VT mode instead of doing it in InputLoop.Run
#16612
Conversation
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
if (_runStandAlone) | ||
{ | ||
this.Command.Context.EngineHostInterface.NotifyBeginApplication(); | ||
_hasNotifiedBeginApplication = true; |
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.
If a native process tweaks the window title, it will do so no matter it's running StandAlone
or in a pipeline. So, we should actually always call NotifyBeginApplication
and NotifyEndApplication
before and after running a native command. Therefore, I moved this code out of the if (_runStandAlone)
block.
if (outputMode.HasFlag(ConsoleControl.ConsoleModes.VirtualTerminal)) | ||
{ | ||
return true; | ||
} |
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.
If the outputMode
already has VirtualTerminal
enabled, no need to set the mode again.
if (ui.SupportsVirtualTerminal) | ||
{ | ||
// Re-enable VT mode if it was previously enabled, as a native command may have turned it off. | ||
ui.TryTurnOnVirtualTerminal(); |
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.
Nit: Interesting, is such console operations expensive?
In line 1105 we have already requested a handle and set mode, now TryTurnOnVirtualTerminal does the operations again. It seems we could do perf optimization.
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.
Very likely not expensive, especially if this is done only after native command execution. The handle is already cached, GetActiveScreenBufferHandle()
returns a cached output handle.
@SteveL-MSFT and @anmenaga Can you please review when you have time? Thanks! |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@SteveL-MSFT and @anmenaga gentle ping :) Please review when you have time, thanks! |
@SteveL-MSFT and @anmenaga Gentle ping again :) Please take a look when you have time, thanks! |
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.
LGTM.
Interesting scenario.
Final notices:
|
Use NotifyEndApplication to re-enable VT mode instead of doing it in InputLoop.Run, so that we run this code only after invoking native commands
🎉 Handy links: |
PR Summary
This is a refactor of the changes in #14413 -- use
NotifyEndApplication
to re-enable VT mode instead of doing it inInputLoop.Run
, so that we run this code only after invoking native commands.PR Context
I was reviewing the use of
NotifyBeginApplication
andNotifyEndApplication
for some related issues in VS Code extension, and .NET Interactive Notebook, and found that the code to re-enable VT mode inConsoleHost
is not done inNotifyEndApplication
, but inInputLoop.Run
, so the code runs for every command execution instead of only after executable invocation. This PR attempts to refactor the fix in #14413 to only do it after execution of a native command.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.