Updates IListDataSource.Render to rename the start parameter to viewportXOffset
#4392
+3,183
−2,166
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Implement 118 parallelizable unit tests for WindowsKeyConverter - Cover ToKey and ToKeyInfo methods with full bidirectional testing - Test basic characters, modifiers, special keys, function keys - Test VK_PACKET Unicode/IME input - Test OEM keys, NumPad keys, and lock states - Include round-trip conversion tests - All tests passing successfully Fixes gui-cs#4389
The `start` parameter in several methods and interfaces has been renamed to `viewportXOffset` to better reflect its purpose as the horizontal offset of the viewport during string rendering. - Updated method signatures in `ListViewWithSelection` to use `viewportXOffset` instead of `start`, including default values. - Modified the `RenderUstr` method in `ListViewWithSelection` to use `viewportXOffset` for calculating the starting index. - Renamed the `start` parameter to `viewportXOffset` in the `IListDataSource` interface and updated its documentation. - Replaced all occurrences of `start` with `viewportXOffset` in the `ListWrapper<T>` class, including method calls and logic. - Updated the `RenderUstr` method in `ListWrapper<T>` to use `viewportXOffset` for substring calculations. - Adjusted the test method in `ListViewTests.cs` to reflect the parameter name change. These changes improve code readability and make the parameter's role in rendering logic more explicit.
tig
changed the title
IListDataSource interface has been updated to rename the start parameter to viewportXOffsetIListDataSource interface to rename the start parameter to viewportXOffset
tig
changed the title
IListDataSource interface to rename the start parameter to viewportXOffsetIListDataSource.Render to rename the start parameter to viewportXOffset
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2_develop #4392 +/- ##
==============================================
+ Coverage 74.69% 74.74% +0.04%
==============================================
Files 387 388 +1
Lines 46709 46679 -30
Branches 6634 6640 +6
==============================================
Hits 34890 34890
+ Misses 9923 9895 -28
+ Partials 1896 1894 -2
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR renames the start parameter to viewportXOffset in the IListDataSource.Render method and all its implementations, improving code clarity by using a more descriptive name that better conveys the parameter's purpose as a horizontal scroll offset.
Key Changes:
- Renamed parameter from
starttoviewportXOffsetin interface and all implementations - Updated all call sites to use the new parameter name
- Updated XML documentation to reference the new parameter name
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Terminal.Gui/Views/IListDataSource.cs | Updated interface definition to rename start parameter to viewportXOffset in Render method signature and documentation |
| Terminal.Gui/Views/ListView.cs | Updated ListWrapper<T>.Render method and RenderUstr helper method to use renamed parameter, including all call sites |
| Tests/UnitTests/Views/ListViewTests.cs | Updated test stub implementation of IListDataSource.Render to use renamed parameter |
| Examples/UICatalog/Scenarios/ListViewWithSelection.cs | Updated example implementation of IListDataSource.Render and its RenderUstr helper method to use renamed parameter |
Collaborator
Author
|
@copilot rename viewportXOffset to viewportX. |
Refactored `ListView` and `IListDataSource` to improve readability, maintainability, and functionality. Introduced `ListWrapper<T>` as a default implementation of `IListDataSource` for easier integration with standard collections. Enhanced `ListView` with better handling of marking, selection, and scrolling. Replaced `viewportXOffset` with `viewportX` for horizontal scrolling. Added `EnsureSelectedItemVisible` to maintain visibility of the selected item. Updated `IListDataSource` with detailed XML documentation and added `SuspendCollectionChangedEvent` for bulk updates. Improved null safety with nullable reference types. Added comprehensive unit tests for `ListWrapper<T>` and `IListDataSource` to ensure robustness. Modernized the codebase with C# features like expression-bodied members and pattern matching. Fixed bugs related to `SelectedItem` validation and rendering artifacts.
Enhance robustness by adding stricter checks for valid indices in ComboBox and ListView. Updated conditions in the `_listview.SelectedItemChanged` event handler to ensure `e.Item` is non-negative before accessing `_searchSet`. Modified the `SetValue` method to use `e.Item` instead of `_listview.SelectedItem`. In ListView, updated the `OnSelectedChanged` method to validate that `SelectedItem` is non-negative (`>= 0`) before accessing the `Source` list. These changes prevent potential out-of-range errors and improve code safety.
Refactored and added new tests to improve coverage, readability, and consistency across multiple test files. Key changes include: - **ShortcutTests.cs**: Added tests for `BindKeyToApplication` and removed redundant tests. - **SourcesManagerTests.cs**: Renamed `Update_*` tests to `Load_*` for clarity. - **ArrangementTests.cs**: Reintroduced `MouseGrabHandler` tests, added `ViewArrangement` flag tests, and improved structure. - **NeedsDrawTests.cs**: Replaced `Application.Screen.Size` with fixed dimensions for better isolation. - **DimAutoTests.cs**: Updated layout tests to use fixed dimensions. - **FrameTests.cs**: Standardized object initialization and validated frame behavior. - **SubViewTests.cs**: Improved formatting and modernized event handling. - **NumericUpDownTests.cs**: Decoupled layout tests from screen size. General improvements: - Enhanced formatting and removed redundant tests. - Added comments for clarity. - Introduced `ITestOutputHelper` for better debugging in `ArrangementTests`.
Enabled nullable reference types across the codebase to improve null safety and prevent potential null reference issues. Refactored `SelectedItem` and related properties from `int` to `int?` to represent no selection with `null` instead of `-1`. Updated logic, event arguments, and method signatures to handle nullable values consistently. Simplified object initialization using modern C# syntax and improved code readability with interpolated strings. Added null checks and early returns to prevent runtime errors. Enhanced error handling by throwing `ArgumentOutOfRangeException` for invalid values. Updated tests to reflect the changes, replacing assertions for `-1` with `null` and ensuring proper handling of nullable values. Cleaned up redundant code and improved formatting for better maintainability.
…e `Shortcut` class.
Updated `Run-LocalCoverage.ps1` to increase `--blame-hang-timeout` from 10s to 60s. Improved null safety in `GuiTestContext` by adding null-conditional operators. Commented out problematic code in `SetupFakeApplicationAttribute.cs` to prevent test hangs. Excluded `ViewBase` files from `UnitTests.Parallelizable.csproj` and removed redundant folder declarations. Simplified event handling in `IListDataSourceTests.cs` and updated `ListViewTests.cs` to use nullable reference types. Enhanced documentation to emphasize the transition to an instance-based application architecture. Updated examples in `application.md`, `multitasking.md`, and `navigation.md` to reflect the use of `Application.Create()` and `View.App`. Clarified the obsolescence of the static `Application` class. Revised table of contents in `toc.yml` to include new sections like "Application Deep Dive" and "Scheme Deep Dive." Added `dotnet-tools.json` for tool configuration. These changes improve maintainability, testability, and alignment with modern C# practices.
The `ListViewTests` class has been refactored to replace the `AutoInitShutdown` attribute with explicit application lifecycle management using `IApplication` and `app.Init()` from the `Terminal.Gui` framework. Key changes include: - Rewriting tests to use `Terminal.Gui`'s application lifecycle. - Adding a private `_output` field for logging test output via `ITestOutputHelper`. - Updating `DriverAssert.AssertDriverContentsWithFrameAre` to include `app.Driver` for UI verification. - Rewriting tests like `Clicking_On_Border_Is_Ignored`, `EnsureSelectedItemVisible_SelectedItem`, and others to align with the new framework. - Adding explicit calls to `app.Shutdown()` for proper cleanup. - Enabling nullable reference types with `#nullable enable`. - Updating `using` directives and `namespace` to reflect the new structure. These changes improve test maintainability, compatibility, and diagnostics.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated 17 comments.
Comments suppressed due to low confidence (9)
Examples/UICatalog/Scenarios/DynamicStatusBar.cs:448
- The comparison
_lstItems.SelectedItem > _lstItems.Source.Count - 1will always be false when SelectedItem is null (null > int evaluates to false). This should check_lstItems.SelectedItem.HasValue && _lstItems.SelectedItem.Value > _lstItems.Source.Count - 1.
if (_lstItems.Source.Count > 0 && _lstItems.SelectedItem > _lstItems.Source.Count - 1)
{
_lstItems.SelectedItem = _lstItems.Source.Count - 1;
}
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:89
- This assignment to current is useless, since its value is never read.
Assert.Equal (strings.IndexOf ("$101.10"), current = n.GetNextMatchingItem (current, '2')); // Shouldn't move
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:114
- This assignment to current is useless, since its value is never read.
Assert.Equal (strings.IndexOf ("apricot"), current = n.GetNextMatchingItem (current, 'a'));
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:228
- This assignment to current is useless, since its value is never read.
Assert.Equal (strings.IndexOf ("can"), current = n.GetNextMatchingItem (current, 'z')); // Shouldn't move
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:286
- This assignment to current is useless, since its value is never read.
Assert.Equal (strings.IndexOf ("$$"), current = n.GetNextMatchingItem (current, '$'));
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:315
- This assignment to current is useless, since its value is never read.
Assert.Equal (strings.IndexOf ("apricot"), current = n.GetNextMatchingItem (current, 'a'));
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:340
- This assignment to current is useless, since its value is never read.
current = n.GetNextMatchingItem (current, ' ')
Tests/UnitTestsParallelizable/Text/CollectionNavigatorTests.cs:359
- This assignment to current is useless, since its value is never read.
Assert.Equal (0, current = n.GetNextMatchingItem (current, 't')); // no matches
Terminal.Gui/Views/ListView.cs:614
- Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
if (source is null && Source is not ListWrapper<T>)
{
Source = null;
}
else
{
Source = new ListWrapper<T> (source);
}
Terminal.Gui/Views/CollectionNavigation/CollectionNavigatorBase.cs
Outdated
Show resolved
Hide resolved
Terminal.Gui/Views/CollectionNavigation/CollectionNavigatorBase.cs
Outdated
Show resolved
Hide resolved
…e.cs Co-authored-by: Copilot <[email protected]>
…e.cs Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…Value)` for consistency and removed an outdated comment questioning its correctness. Enhanced the exception message in the `SelectedItem` property setter to provide clearer guidance when the value is out of range.
Added multiple test methods to validate `ListView` behavior: - `Vertical_ScrollBar_Hides_And_Shows_As_Needed`: Ensures the vertical scrollbar auto-hides/shows based on content height. - `Mouse_Wheel_Scrolls`: Verifies vertical scrolling with the mouse wheel updates `TopItem`. - `SelectedItem_With_Source_Null_Does_Nothing`: Confirms no exceptions occur when setting `SelectedItem` with a `null` source. - `Horizontal_Scroll`: Tests horizontal scrolling, including programmatic and mouse wheel interactions, ensuring `LeftItem` updates correctly. - `SetSourceAsync_SetsSource`: Validates the asynchronous `SetSourceAsync` method updates the source and item count. - `AllowsMultipleSelection_Set_To_False_Unmarks_All_But_Selected`: Ensures disabling multiple selection unmarks all but the selected item. - `Source_CollectionChanged_Remove`: Confirms `SelectedItem` and source count update correctly when items are removed from the source collection.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
IListDataSource.Renderto rename thestartparameter toviewportXOffsetListView