make sure console is restored #270
Conversation
tomlm
commented
Jan 22, 2025
tomlm
commented
- Always dispose console driver
- Implment CursorsConsole .PrepareConsole() and RestoreConsole() correctly.
📝 WalkthroughWalkthroughThe pull request introduces modifications to the In the Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Consolonia.PlatformSupport/CursesConsole.cs (1)
248-418: Refactor complex key handling logic for maintainabilityThe key handling logic within the
ProcessInput()method, particularly for ESC sequences, is complex and may be difficult to maintain or debug.Consider refactoring this code by:
- Breaking it into smaller helper methods with descriptive names.
- Adding comments to explain non-obvious logic.
- Using a state machine or parsing library to handle key sequences more elegantly.
src/Consolonia.Core/ConsoloniaLifetime.cs (1)
95-105: Handle exceptions within the main loop appropriatelyWhile the
try-finallyblock ensuresDispose()is called, any exceptions thrown withinDispatcher.UIThread.MainLoopmay cause abrupt termination without proper logging or user feedback.Consider catching exceptions inside the
tryblock to log them or provide user-friendly error messages:try { - Dispatcher.UIThread.MainLoop(_cts.Token); + try + { + Dispatcher.UIThread.MainLoop(_cts.Token); + } + catch (Exception ex) + { + // Log the exception or handle it accordingly + Console.Error.WriteLine($"An error occurred: {ex.Message}"); + } Environment.ExitCode = _exitCode; return _exitCode; } finally { Dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/ConsoloniaLifetime.cs(2 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/Consolonia.PlatformSupport/CursesConsole.cs (1)
248-418: Review the timeout setting for input processingSetting
Curses.timeout(200);introduces a 200ms delay when processing input, which may impact the responsiveness of the application, especially for quick key presses.Confirm if the 200ms timeout is necessary for processing ESC sequences. If not, consider reducing the timeout or exploring alternative approaches that do not compromise responsiveness.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Consolonia.PlatformSupport/CursesConsole.cs (1)
145-169: Consider implementing IDisposable pattern.The console resource management would benefit from proper implementation of the IDisposable pattern to ensure deterministic cleanup.
-public class CursesConsole : ConsoleBase +public class CursesConsole : ConsoleBase, IDisposable { private bool _disposed; private Curses.Window _cursesWindow; + protected virtual void Dispose(bool disposing) + { + if (_disposed) return; + if (disposing) + { + RestoreConsole(); + } + _disposed = true; + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + ~CursesConsole() + { + Dispose(false); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/ConsoloniaLifetime.cs(2 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/ConsoloniaLifetime.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/Consolonia.PlatformSupport/CursesConsole.cs (4)
111-111: Initialize_cursesWindowin constructor or add null checks.The field is used in multiple methods but lacks proper initialization guarantees.
128-129: Avoid calling virtual methods from constructors.Calling
PrepareConsole()(virtual) indirectly from the constructor viaStartEventLoop()can lead to unexpected behavior if a subclass overrides this method.
145-157: Add error handling for Curses initialization.The
PrepareConsole()method should handle potential failures from Curses functions.Additionally, consider adding debug logging to track console state transitions:
public override void PrepareConsole() { + Debug.WriteLine("Preparing Curses console..."); _cursesWindow = Curses.initscr(); Curses.raw(); Curses.noecho(); _cursesWindow.keypad(true); Curses.cbreak(); Curses.mousemask( Curses.Event.AllEvents | Curses.Event.ReportMousePosition, out Curses.Event _); base.PrepareConsole(); + Debug.WriteLine("Curses console prepared successfully"); }
159-169: Add null check and error handling inRestoreConsole().The method should safely handle cases where
_cursesWindowis null.Additionally, consider adding debug logging for console cleanup:
public override void RestoreConsole() { + Debug.WriteLine("Restoring Curses console..."); base.RestoreConsole(); + if (_cursesWindow == null) + { + Debug.WriteLine("Warning: Curses window was null during restore"); + return; + } Curses.mousemask(0, out Curses.Event _); Curses.nocbreak(); _cursesWindow.keypad(false); Curses.echo(); Curses.noraw(); Curses.endwin(); + Debug.WriteLine("Curses console restored successfully"); }
