Restore terminal cursor visibility on exit in collect-linux#5707
Restore terminal cursor visibility on exit in collect-linux#5707
Conversation
- Add finally block to restore cursor visibility on all exit paths - Check IsOutputRedirected before restoring cursor - Add unit tests to verify cursor restoration behavior Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>
Remove manual cursor state manipulation in test to properly verify that the finally block respects IsOutputRedirected flag Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>
Co-authored-by: hoyosjs <19413848+hoyosjs@users.noreply.github.com>
|
I tested and this seems to be working correctly. |
There was a problem hiding this comment.
Pull request overview
Fixes terminal state cleanup for dotnet-trace collect-linux by ensuring the cursor is restored after trace collection, and adds functional tests around cursor visibility behavior.
Changes:
- Restore cursor visibility in
CollectLinuxCommandHandler.CollectLinuxvia afinallyblock. - Add functional tests for cursor visibility on success, error, and redirected-output scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs | Adds cursor restoration logic in finally for collect-linux execution paths. |
| src/tests/dotnet-trace/CollectLinuxCommandFunctionalTests.cs | Adds tests intended to validate cursor visibility restoration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| finally | ||
| { | ||
| if (!Console.IsOutputRedirected) |
There was a problem hiding this comment.
The cursor is always restored here even on paths where it was never hidden (e.g., --probe early return, or early returns before Console.CursorVisible = false), which can unexpectedly change the user's cursor state. Also, cursor visibility is still set to false unconditionally earlier, so with output redirection the cursor can be hidden but never restored. Consider only toggling cursor visibility when !Console.IsOutputRedirected, and track whether/what value you changed so the finally restores only when it was modified (ideally restoring the previous value).
| if (!Console.IsOutputRedirected) | |
| if (!Console.IsOutputRedirected && !Console.CursorVisible) |
| console.CursorVisible = true; | ||
|
|
||
| int exitCode = Run(TestArgs(), console); | ||
|
|
There was a problem hiding this comment.
exitCode is assigned but never used, which will produce a compiler warning that becomes an error in this repo (TreatWarningsAsErrors=true). Either assert on the exit code (e.g., Ok) or discard the value (e.g., _ = Run(...)).
| // Command should succeed | |
| Assert.Equal((int)ReturnCode.Ok, exitCode); |
| int exitCode = Run(TestArgs(), console); | ||
|
|
There was a problem hiding this comment.
exitCode is assigned but never used, which will produce a compiler warning that becomes an error in this repo (TreatWarningsAsErrors=true). Either assert on the exit code or discard the value.
| int exitCode = Run(TestArgs(), console); | ||
|
|
||
| // When output is redirected, the cursor restoration code in the finally block | ||
| // should not execute, so the cursor should remain in the state set by line 104 (false) | ||
| Assert.False(console.CursorVisible, "Cursor should remain hidden when output is redirected"); |
There was a problem hiding this comment.
This test asserts the cursor remains hidden when output is redirected, but the command currently also hides the cursor unconditionally earlier. If the intent is to avoid interfering with redirected/piped output, it would be better for the command to not change cursor visibility at all when IsOutputRedirected is true; then the test should assert the cursor remains in its original state. Also, the comment referencing a specific source line number is brittle and will go stale as the file changes.
| int exitCode = Run(TestArgs(), console); | |
| // When output is redirected, the cursor restoration code in the finally block | |
| // should not execute, so the cursor should remain in the state set by line 104 (false) | |
| Assert.False(console.CursorVisible, "Cursor should remain hidden when output is redirected"); | |
| bool initialCursorVisible = console.CursorVisible; | |
| int exitCode = Run(TestArgs(), console); | |
| // When output is redirected, the command should not change cursor visibility, | |
| // so the cursor should remain in its original state. | |
| Assert.Equal(initialCursorVisible, console.CursorVisible); |
dotnet-trace collect-linuxhides the cursor during trace collection but never restores it, leaving users with an invisible cursor after the tool exits.Changes
CollectLinuxCommand.cs: Add cursor restoration in
finallyblock, matching the pattern inCollectCommand.cs!Console.IsOutputRedirectedto avoid interfering with piped outputCollectLinuxCommandFunctionalTests.cs: Add tests verifying cursor state after command exits
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
securitytools.pkgs.visualstudio.com/opt/hostedtoolcache/CodeQL/2.24.0/x64/codeql/csharp/tools/linux64/Semmle.Autobuild.CSharp /opt/hostedtoolcache/CodeQL/2.24.0/x64/codeql/csharp/tools/linux64/Semmle.Autobuild.CSharp(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
dotnet-trace collect-linuxleaves terminal cursor hidden after tracing completes #5706💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.