Fixes #4361 - Consolidate FakeDriver into library and refactor driver architecture#4362
Conversation
…fer initialization Co-authored-by: tig <585482+tig@users.noreply.github.com>
Co-authored-by: tig <585482+tig@users.noreply.github.com>
Co-authored-by: tig <585482+tig@users.noreply.github.com>
|
@copilot continue |
The PR is complete and ready for review! All 12,891 tests pass (3,097 UnitTests + 9,794 UnitTestsParallelizable), build succeeds with no errors, and all acceptance criteria have been met. The [WIP] tag has been removed from the PR title. Key accomplishments:
The PR is ready to merge whenever you're ready. |
Replaced `ScreenChanged` with `SizeChanged` across the codebase to standardize naming and improve clarity. Updated event handling logic, including subscriptions, unsubscriptions, and raising methods. Removed deprecated `ScreenChanged` event and backward compatibility code. Refactored driver initialization to handle legacy `IConsoleDriver` types separately. Updated tests and mock implementations to align with the new `SizeChanged` event. Improved documentation and comments to reflect these changes. These updates enhance maintainability, consistency, and modernize the architecture.
…Monitor Renamed `IWindowSizeMonitor` to `IConsoleSizeMonitor` across the codebase, updating all references, method signatures, and event handlers. Replaced the `WindowSizeMonitor` class with the new `ConsoleSizeMonitor` implementation, which includes improved terminal size change handling via the `Poll` method. Enabled nullable reference types in several files to enhance code safety. Updated test cases to reflect the new `IConsoleSizeMonitor` interface. Removed redundant code, simplified null checks, and corrected minor typos in comments. Streamlined the codebase by removing the obsolete `WindowSizeMonitor` class and its interface.
Updated ShadowView.cs to use null-conditional operators and added null checks for safer access to `ScreenContents`. Refined XML documentation in View.Layout.cs for clarity and consistency. Refactored FakeDriverTests.cs to leverage modern C# features, including shorthand object instantiation, inline lambdas, and tuple-like syntax for `Size` and `Rectangle`. Removed redundant tests and improved test readability and reliability. Enhanced error handling with null checks and ensured backward compatibility for deprecated events. Improved test coverage for resizing, clipboard operations, and invalid coordinates. Verified buffer integrity and screen updates after resizing. General improvements include replacing explicit type declarations with `var`, removing unused imports, and aligning code formatting for better readability. Refactor and improve code quality and test coverage Updated `ShadowView` for null safety using null-conditional operators. Simplified object initializations and modernized syntax across the codebase, including shorthand initializations and inline lambdas. Enhanced event handling logic and ensured compatibility with obsolete members. Refactored `FakeDriverTests` by removing redundant code, standardizing formatting, and improving test setup. Suppressed obsolete warnings where necessary. Improved XML documentation in `View.Layout.cs` for clarity and removed outdated references. Performed general cleanup, including removing unused namespaces, redundant comments, and ensuring consistent formatting. These changes enhance readability, maintainability, and runtime safety.
…ub.com/gui-cs/Terminal.Gui into copilot/consolidate-fakedriver-library
Refactor TimedEventsTests for readability and consistency Improved code readability and maintainability: - Enabled nullable reference types with `#nullable enable`. - Removed unused `using System.Diagnostics;`. - Updated namespace to `UnitTests.ApplicationTests`. - Replaced `Terminal.Gui.App.TimedEvents` with `TimedEvents`. - Reformatted XML documentation comments for alignment. - Used `var` and target-typed new expressions for consistency. - Reformatted `Parallel.For` loops and lambdas for clarity. - Added `Thread.Sleep(10)` to prevent excessive CPU usage in tests. - Improved assertions and event handler formatting in tests. Aligned with modern C# coding practices.
Refactor codebase and introduce FakeClipboard - Adjusted `.editorconfig` to change severity levels for CS0612, CS0618, and CS0672 diagnostics. - Replaced `FakeDriver.FakeClipboard` with a new `FakeClipboard` class for testing purposes, supporting exception handling and clipboard data manipulation. - Removed redundant methods (`MakeColor`, `MapKey`) and unused classes (`MockConsoleDriver`) to streamline the codebase. - Refactored `ConsoleDriverFacade` and `FakeDriver` to simplify logic and improve maintainability. - Updated tests to use `CreateFakeDriver` and removed or commented out obsolete tests. - Reformatted and cleaned up code for readability across multiple files.
Standardized console size management by replacing `WindowSizeMonitor` with `ConsoleSizeMonitor` across the codebase. Updated methods `GetWindowSize` and `SetWindowSize` to `GetSize` and `SetSize` for consistency. Refactored `FakeDriver` to use `SetScreenSize` and removed redundant methods. Simplified driver initialization by removing legacy `InternalInit` logic. Standardized ANSI escape sequences by replacing `CSI_ReportTerminalSizeInChars` with `CSI_ReportWindowSizeInChars`. Updated test cases to align with the new `ConsoleSizeMonitor` and `SetScreenSize` methods. Removed obsolete test utilities like `FakeSizeMonitor` and `FakeWindowSizeMonitor`. Performed general code cleanup, including removing unused classes, redundant code, and improving formatting. Fixed resizing logic issues and improved exception handling in driver methods.
FakeDriver into library and refactor driver architecture
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates FakeDriver into the main Terminal.Gui library and refactors the driver architecture to improve testability. The changes include fixing a critical buffer initialization bug, renaming the resize API from SizeChanged to ScreenChanged, adding a new SetScreenSize() method for test scenarios, and ensuring 100% backward compatibility through deprecation shims.
Key Changes:
- Fixed critical buffer initialization bug in
FakeDriver.Init()by callingClearContents() - Renamed resize API to
ScreenChangedwith clean deprecation ofSizeChanged - Added
SetScreenSize()method for test scenarios (FakeDriver only) - Consolidated test infrastructure by removing duplicate
MockConsoleDriverand related classes - Renamed interfaces and methods for consistency (
IWindowSizeMonitor→IConsoleSizeMonitor,GetWindowSize()→GetSize())
Reviewed Changes
Copilot reviewed 106 out of 108 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Terminal.Gui/Drivers/FakeDriver/FakeDriver.cs | Core FakeDriver implementation with buffer initialization fix and SetScreenSize support |
| Terminal.Gui/Drivers/ConsoleDriver.cs | Base driver class with new SetScreenSize API and updated documentation |
| Terminal.Gui/Drivers/IConsoleDriver.cs | Interface updates for SetScreenSize and renamed event documentation |
| Terminal.Gui/App/Application.Lifecycle.cs | Driver event subscription updates to use renamed ScreenChanged event |
| Tests/UnitTests/ConsoleDrivers/FakeDriverTests.cs | Comprehensive test coverage for new functionality (+148 lines) |
| Tests/UnitTestsParallelizable/MockConsoleDriver.cs | Removed obsolete MockConsoleDriver implementation (-203 lines) |
Comments suppressed due to low confidence (1)
Terminal.Gui/App/Application.Screen.cs:1
- Missing spaces around comma in method call. Should be
SetScreenSize(120, 30)for consistency with C# style guidelines.
#nullable enable
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ub.com/gui-cs/Terminal.Gui into copilot/consolidate-fakedriver-library
BDisp
left a comment
There was a problem hiding this comment.
Very good and hard work. Only some suggestions.
Enhanced `Ruler` and `Thickness` classes by adding an optional `IConsoleDriver? driver` parameter to decouple rendering from the default `Application.Driver`, improving flexibility and testability. Updated `DriverAssert` to support nullable drivers and added defensive checks. Refactored and expanded test cases for `Ruler`, `Thickness`, `LineCanvas`, and `StraightLineExtensions` to ensure comprehensive coverage, including zero-length intersections, line rendering, and exclusion logic. Migrated rendering-dependent tests to a parallelizable namespace. Removed redundant tests, improved naming conventions, and updated documentation for better maintainability and clarity.
|
I'm going to stop here. I was going to prove that you could do mouse / keyboard input into an IApplication instance in the Parallel tests, but I think we can do that in another PR. |
Refactor: Simplify driver logic and update SetSize methods Removed FakeDriver safeguard in unit tests to simplify CreateDriver logic. Updated SetSize methods in NetOutput, UnixOutput, and WindowsOutput to do nothing instead of throwing NotImplementedException. Modified SizeChanged event in ConsoleDriverFacade to call SetScreenSize directly. Commented out unnecessary debug validation in DimAuto. These changes improve maintainability and reduce complexity.
|
Looks like I'm not done. Still some intermittent test failures. |
Refactored `_cachedRunStateToplevel` to `CachedRunStateToplevel` as an internal static property, delegating its management to `ApplicationImpl` for improved encapsulation. Updated all references to use the new property and centralized its handling in `ApplicationImpl`. Removed the `MouseGrabHandler` property from `ApplicationImpl` and simplified driver-related assignments by replacing `Application.ForceDriver` and `Application.Screen` with direct references. Reset `CachedRunStateToplevel` during cleanup to ensure proper state management. Updated the `Invoke` method to use `Top` directly, aligning with the refactored design. Improved debug assertions and performed general cleanup to enhance code readability and maintainability.
|
Nope. Still failing. |
Refactor and enhance codebase for maintainability - Applied null-conditional operator (`!`) to improve null-safety. - Refactored tests for clarity, added new cases, and removed redundancies. - Introduced `FakeApplicationFactory` and `FakeSizeMonitor` for testing. - Removed unused code, including legacy `DecodeEscSeq` logic. - Reformatted code for readability and consistency. - Updated assertions for more accurate validation in tests. - Fixed potential null reference issues across multiple files. - Improved event handling with proper null checks. - Enhanced documentation for new classes and methods. - Modernized code with C# features like `record struct` and `nullable enable`.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 156 out of 158 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
Tests/UnitTestsParallelizable/Views/CheckBoxTests.cs:1
- The
CheckBoxTestsclass constructor removes theITestOutputHelper outputparameter that was present in the original code. This parameter is essential for test output in several test methods that useDriverAssert.AssertDriverContentsWithFrameAre(which requires an output parameter). Removing this parameter will cause compilation errors in the test methods that depend on it.
Tests/UnitTestsParallelizable/Views/TextFieldTests.cs:1 - [nitpick] The class now inherits from
ParallelizableBasewhich already provides access to a driver creation method. Consider using the inheritedCreateFakeDriver()method from the base class instead of creating a new driver instance in each test to maintain consistency with the parallelizable test infrastructure.
Tests/UnitTestsParallelizable/ParallelizableBase.cs:1 - The method should call
driver.Init()afterSetScreenSize()to ensure the driver buffers are properly initialized. According to the PR description, one of the key fixes was ensuringClearContents()is called during init to initialize buffers, preventing null reference exceptions.
…r driver architecture (tui-cs#4362) * Initial plan * Add ScreenChanged event, SetScreenSize method, and fix FakeDriver buffer initialization Co-authored-by: tig <585482+tig@users.noreply.github.com> * Add comprehensive tests for ScreenChanged event and buffer integrity Co-authored-by: tig <585482+tig@users.noreply.github.com> * Replace obsolete SizeChanged usage with ScreenChanged in core and tests Co-authored-by: tig <585482+tig@users.noreply.github.com> * Refactor terminal size event handling Replaced `ScreenChanged` with `SizeChanged` across the codebase to standardize naming and improve clarity. Updated event handling logic, including subscriptions, unsubscriptions, and raising methods. Removed deprecated `ScreenChanged` event and backward compatibility code. Refactored driver initialization to handle legacy `IConsoleDriver` types separately. Updated tests and mock implementations to align with the new `SizeChanged` event. Improved documentation and comments to reflect these changes. These updates enhance maintainability, consistency, and modernize the architecture. * Refactor & Code Cleanup: Replace IWindowSizeMonitor with IConsoleSizeMonitor Renamed `IWindowSizeMonitor` to `IConsoleSizeMonitor` across the codebase, updating all references, method signatures, and event handlers. Replaced the `WindowSizeMonitor` class with the new `ConsoleSizeMonitor` implementation, which includes improved terminal size change handling via the `Poll` method. Enabled nullable reference types in several files to enhance code safety. Updated test cases to reflect the new `IConsoleSizeMonitor` interface. Removed redundant code, simplified null checks, and corrected minor typos in comments. Streamlined the codebase by removing the obsolete `WindowSizeMonitor` class and its interface. * Code cleanup - Refactor and enhance ShadowView and FakeDriverTests Updated ShadowView.cs to use null-conditional operators and added null checks for safer access to `ScreenContents`. Refined XML documentation in View.Layout.cs for clarity and consistency. Refactored FakeDriverTests.cs to leverage modern C# features, including shorthand object instantiation, inline lambdas, and tuple-like syntax for `Size` and `Rectangle`. Removed redundant tests and improved test readability and reliability. Enhanced error handling with null checks and ensured backward compatibility for deprecated events. Improved test coverage for resizing, clipboard operations, and invalid coordinates. Verified buffer integrity and screen updates after resizing. General improvements include replacing explicit type declarations with `var`, removing unused imports, and aligning code formatting for better readability. Refactor and improve code quality and test coverage Updated `ShadowView` for null safety using null-conditional operators. Simplified object initializations and modernized syntax across the codebase, including shorthand initializations and inline lambdas. Enhanced event handling logic and ensured compatibility with obsolete members. Refactored `FakeDriverTests` by removing redundant code, standardizing formatting, and improving test setup. Suppressed obsolete warnings where necessary. Improved XML documentation in `View.Layout.cs` for clarity and removed outdated references. Performed general cleanup, including removing unused namespaces, redundant comments, and ensuring consistent formatting. These changes enhance readability, maintainability, and runtime safety. * Code cleanup Refactor TimedEventsTests for readability and consistency Improved code readability and maintainability: - Enabled nullable reference types with `#nullable enable`. - Removed unused `using System.Diagnostics;`. - Updated namespace to `UnitTests.ApplicationTests`. - Replaced `Terminal.Gui.App.TimedEvents` with `TimedEvents`. - Reformatted XML documentation comments for alignment. - Used `var` and target-typed new expressions for consistency. - Reformatted `Parallel.For` loops and lambdas for clarity. - Added `Thread.Sleep(10)` to prevent excessive CPU usage in tests. - Improved assertions and event handler formatting in tests. Aligned with modern C# coding practices. * Code Cleanup - No more driver warnings. Refactor codebase and introduce FakeClipboard - Adjusted `.editorconfig` to change severity levels for CS0612, CS0618, and CS0672 diagnostics. - Replaced `FakeDriver.FakeClipboard` with a new `FakeClipboard` class for testing purposes, supporting exception handling and clipboard data manipulation. - Removed redundant methods (`MakeColor`, `MapKey`) and unused classes (`MockConsoleDriver`) to streamline the codebase. - Refactored `ConsoleDriverFacade` and `FakeDriver` to simplify logic and improve maintainability. - Updated tests to use `CreateFakeDriver` and removed or commented out obsolete tests. - Reformatted and cleaned up code for readability across multiple files. * Refactor FakeDriver - Code Cleanup Standardized console size management by replacing `WindowSizeMonitor` with `ConsoleSizeMonitor` across the codebase. Updated methods `GetWindowSize` and `SetWindowSize` to `GetSize` and `SetSize` for consistency. Refactored `FakeDriver` to use `SetScreenSize` and removed redundant methods. Simplified driver initialization by removing legacy `InternalInit` logic. Standardized ANSI escape sequences by replacing `CSI_ReportTerminalSizeInChars` with `CSI_ReportWindowSizeInChars`. Updated test cases to align with the new `ConsoleSizeMonitor` and `SetScreenSize` methods. Removed obsolete test utilities like `FakeSizeMonitor` and `FakeWindowSizeMonitor`. Performed general code cleanup, including removing unused classes, redundant code, and improving formatting. Fixed resizing logic issues and improved exception handling in driver methods. * Update Terminal.Gui/Drivers/OutputBuffer.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Terminal.Gui/Drivers/MouseButtonStateEx.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Terminal.Gui/App/MainLoop/IApplicationMainLoop.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Tests/UnitTests/Views/ToplevelTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update Terminal.Gui/ViewBase/View.Layout.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Moved all Drawing tests to Paralleizable - proving Fakedriver works Enhanced `Ruler` and `Thickness` classes by adding an optional `IConsoleDriver? driver` parameter to decouple rendering from the default `Application.Driver`, improving flexibility and testability. Updated `DriverAssert` to support nullable drivers and added defensive checks. Refactored and expanded test cases for `Ruler`, `Thickness`, `LineCanvas`, and `StraightLineExtensions` to ensure comprehensive coverage, including zero-length intersections, line rendering, and exclusion logic. Migrated rendering-dependent tests to a parallelizable namespace. Removed redundant tests, improved naming conventions, and updated documentation for better maintainability and clarity. * Fixed Run<T> startup hang. Refactor: Simplify driver logic and update SetSize methods Removed FakeDriver safeguard in unit tests to simplify CreateDriver logic. Updated SetSize methods in NetOutput, UnixOutput, and WindowsOutput to do nothing instead of throwing NotImplementedException. Modified SizeChanged event in ConsoleDriverFacade to call SetScreenSize directly. Commented out unnecessary debug validation in DimAuto. These changes improve maintainability and reduce complexity. * Fixed intermittent unit test bug. Refactored `_cachedRunStateToplevel` to `CachedRunStateToplevel` as an internal static property, delegating its management to `ApplicationImpl` for improved encapsulation. Updated all references to use the new property and centralized its handling in `ApplicationImpl`. Removed the `MouseGrabHandler` property from `ApplicationImpl` and simplified driver-related assignments by replacing `Application.ForceDriver` and `Application.Screen` with direct references. Reset `CachedRunStateToplevel` during cleanup to ensure proper state management. Updated the `Invoke` method to use `Top` directly, aligning with the refactored design. Improved debug assertions and performed general cleanup to enhance code readability and maintainability. * Fixed intermittent bug an massive code cleanup of warnings. Refactor and enhance codebase for maintainability - Applied null-conditional operator (`!`) to improve null-safety. - Refactored tests for clarity, added new cases, and removed redundancies. - Introduced `FakeApplicationFactory` and `FakeSizeMonitor` for testing. - Removed unused code, including legacy `DecodeEscSeq` logic. - Reformatted code for readability and consistency. - Updated assertions for more accurate validation in tests. - Fixed potential null reference issues across multiple files. - Improved event handling with proper null checks. - Enhanced documentation for new classes and methods. - Modernized code with C# features like `record struct` and `nullable enable`. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tig <585482+tig@users.noreply.github.com> Co-authored-by: Tig <tig@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Consolidate FakeDriver into library and refactor driver architecture
✅ All Tasks Complete - Ready for Review
This PR successfully addresses all requirements from issue #4361, completing the FakeDriver consolidation and architecture refinement that was blocked in PR #4347.
Summary
Successfully implemented all required tasks:
Test Results
Key Changes
1. Fixed Critical Bug
FakeDriver.Init()now callsClearContents()to initialize buffersFakeDriver_Can_Fill_Rectangletest2. Improved API Design
ScreenChangedevent (replacesSizeChanged)SetScreenSize()method for test scenarios3. Comprehensive Testing
FakeDriver_Can_Fill_Rectangle- validates buffer initializationFakeDriver_Buffer_Integrity_After_Multiple_Resizes- tests buffer cleanupScreenChanged_Event_Fires_On_SetScreenSize- verifies new eventFakeResize_Triggers_ScreenChanged_And_Updates_Application_Screen- integration testSizeChanged_Event_Still_Fires_For_Compatibility- backward compatibilityScreen_Changes_OnSizeChanged_Without_Call_Application_Init→Screen_Changes_OnScreenChanged_Without_Call_Application_Init4. Documentation
API Changes
New APIs:
IConsoleDriver.ScreenChangedeventIConsoleDriver.SetScreenSize(int, int)methodConsoleDriver.OnScreenChanged(SizeChangedEventArgs)methodDeprecated APIs (still functional):
IConsoleDriver.SizeChanged→ useScreenChangedConsoleDriver.OnSizeChanged()→ useOnScreenChanged()Exception Message:
"SetScreenSize is only supported by FakeDriver for test scenarios."Files Changed
Core Library (5 files):
Terminal.Gui/Drivers/ConsoleDriver.csTerminal.Gui/Drivers/ConsoleDriverFacade.csTerminal.Gui/Drivers/FakeDriver/FakeDriver.csTerminal.Gui/Drivers/IConsoleDriver.csTerminal.Gui/App/Application.Lifecycle.csTest Infrastructure (4 files):
Tests/TerminalGuiFluentTesting/FakeDriver/FakeConsoleDriver.csTests/TerminalGuiFluentTesting/FakeDriver/IFakeConsoleDriver.csTests/UnitTests/AutoInitShutdownAttribute.csTests/UnitTestsParallelizable/MockConsoleDriver.csTests (3 files):
Tests/UnitTests/ConsoleDrivers/FakeDriverTests.cs(+148 lines)Tests/UnitTests/Application/ApplicationScreenTests.csTests/UnitTests/ConsoleDrivers/ConsoleDriverTests.csAcceptance Criteria Met
✅ All FakeDriver tests pass (including previously failing rectangle fill test)
✅ New ScreenChanged event surfaces correctly; old event deprecated with shim
✅ AutoInitShutdownAttribute.FakeResize uses SetScreenSize and triggers layout
✅ No null reference exceptions when calling driver rendering APIs after Init()
✅ Consistent exception message across non-Fake drivers
✅ Documentation updated; no stale references to OnSizeChanged
✅ Build succeeds with 0 errors, 0 warnings (obsolete APIs)
✅ 100% backward compatible - all existing code works without modification
Backward Compatibility
This PR maintains 100% backward compatibility:
SizeChangedevent still fires via compatibility shimOnSizeChanged()method forwards to new implementationSecurity
AI Model: GitHub Copilot Chat (gpt-4o)
Fixes #4361
Continues work from #4347
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.