Prevent TerminalLogger corruption during terminal resize#14370
Prevent TerminalLogger corruption during terminal resize#14370AlesProkop with Copilot wants to merge 7 commits into
Conversation
|
Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
Co-authored-by: AlesProkop <276576870+AlesProkop@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves TerminalLogger’s live node display rendering stability by making refreshes always consult the current terminal dimensions so a terminal resize triggers the existing erase-and-redraw path immediately, preventing corrupted/wrapped output during active builds.
Changes:
- Simplified the refresher thread loop to call a new
Refresh()method each frame. - Introduced
TerminalLogger.Refresh()to centralize refresh locking and ensure terminal size is checked per refresh. - Added a regression unit test that simulates shrinking the terminal width and asserts the logger emits the erase sequence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Build/Logging/TerminalLogger/TerminalLogger.cs | Refactors refresher loop to refresh each frame via Refresh(), ensuring terminal resize is detected promptly. |
| src/Build.UnitTests/TerminalLogger_Tests.cs | Adds a resizable terminal test double and a regression test for resize-triggered redraw behavior. |
| { | ||
| if (_currentFrame.NodesCount > 0) | ||
| { | ||
| // Querying the terminal for it's dimensions is expensive, so we only do it every 30 frames e.g. once a second. |
There was a problem hiding this comment.
It seems to me that this change undoes this optimization (which is actually pretty important, check the historical ticket that introduced this).
There was a problem hiding this comment.
You are right. Changed it so the optimization is preserved. Now, dimensions are queried once every 30 frames. However, after detecting a resize, querying increases to 30 hz until dimensions are stable for three frames. I think this is a good trade-off between optimization and not having the terminal "ugly".
…logger-resizing-issue
|
TerminalLogger already remembers how many rows its live node block occupies and moves the cursor up by that amount before repainting. The problem is that this count becomes invalid after a resize: the terminal reflows previously written lines, so one old physical row may become two or more new rows. So, we cannot effectively (without second buffer) determine how many of the rows can be rewritten without potentionally corrupting the terminal. We have effectively two options:
By my micro banchmark, these are the times that are needed for the approaches:
So, the outcome if this is:
So the question is: Do we want to keep performance in the cost of "ugly" resizing or do we want to improve the UX in cost of some time? |
|
what impact does this have on a reasonably-sized build, like OrchardCore? |
Fixes: #14344
Summary
Customer Impact
Shrinking a terminal during a build no longer leaves wrapped duration fragments and corrupted status output.
Regression?
No. Unchanged dimensions retain the existing delta-rendering behavior.
Testing
Added regression coverage for shrinking an active terminal from 120 to 40 columns.
Risk
Low. The change adds inexpensive terminal dimension reads to the existing 30 Hz refresh loop.