Fixes #3823 - Upgrades TextView to use View's modern scrolling infrastructure
#4407
Conversation
- Add SetContentSize/GetContentSize integration - Configure VerticalScrollBar with AutoShow=true by default - Configure HorizontalScrollBar to track WordWrap property - Keep _topRow/_leftColumn in sync with Viewport for backward compatibility - Update content size on model changes - All parallelizable tests pass (163/163) Co-authored-by: tig <[email protected]>
- Create TextViewScrollingTests with 7 new tests - Test SetContentSize integration - Test VerticalScrollBar AutoShow behavior - Test HorizontalScrollBar AutoShow tracking WordWrap - Test Viewport syncing with TopRow/LeftColumn - Test content size updates on text changes - Test ScrollTo updates Viewport correctly - All 7 tests pass (7/7) Co-authored-by: tig <[email protected]>
- Add guards in ViewportChanged handler to prevent infinite loops - Remove unnecessary early return in UpdateContentSize - Improve UpdateContentSize documentation - All tests still pass (170/170 parallelizable) Co-authored-by: tig <[email protected]>
|
@copilot write new tests that test the same things as the broken tests and the delete the old ones. |
- Created 11 new parallelizable tests in TextViewNavigationTests.cs - Tests cover tab/backtab navigation, cursor movement, scrolling, word wrap, and unwrapped cursor position - Deleted 9 old non-parallelizable tests that failed due to ScrollBar layout changes - Old tests: BackTab_Test_Follow_By_Tab, KeyBindings_Command, Tab_Test_Follow_By_BackTab, Tab_Test_Follow_By_BackTab_With_Text, Tab_Test_Follow_By_CursorLeft_And_Then_Follow_By_CursorRight, Tab_Test_Follow_By_CursorLeft_And_Then_Follow_By_CursorRight_With_Text, Tab_Test_Follow_By_Home_And_Then_Follow_By_End_And_Then_Follow_By_BackTab_With_Text, TabWidth_Setting_To_Zero_Keeps_AllowsTab, UnwrappedCursorPosition_Event - All 181 parallelizable tests pass (170 existing + 11 new) - All 93 non-parallelizable tests pass (97 total with 4 skipped) Co-authored-by: tig <[email protected]>
Done! Created 11 new parallelizable tests in New tests cover:
All 181 parallelizable tests now pass (commit 114a271). |
- Add bounds check to prevent infinite loop when finding last tab - Improve loop condition and add safety break - All tests still pass Co-authored-by: tig <[email protected]>
|
@copilot Unit tests that don't Draw do not need View.Driver set. |
- Tests that don't call Draw() don't need View.Driver set - Removed CreateFakeDriver() and Driver assignment from all 11 tests - Simplified UnwrappedCursorPosition test to not use SetRelativeLayout - All 181 parallelizable tests still pass Co-authored-by: tig <[email protected]>
Done! Removed unnecessary |
tig
changed the title
TextView to use View's modern scrolling infrastructure
There was a problem hiding this comment.
Pull request overview
This pull request upgrades TextView to use View's modern scrolling infrastructure, transitioning from manual scroll management (_topRow/_leftColumn fields) to the built-in Viewport, SetContentSize(), and ScrollBar system.
Key changes:
- Integrated TextView with View's modern scrolling system via
SetContentSize()andViewportsynchronization - Configured ScrollBars with
AutoShowbehavior (vertical always enabled, horizontal tracks WordWrap) - Replaced 9 non-parallelizable tests with 11 new parallelizable tests covering navigation, scrolling, and tab handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Terminal.Gui/Views/TextInput/TextView.cs | Implemented bidirectional sync between _topRow/_leftColumn and Viewport, added UpdateContentSize() method, configured ScrollBar AutoShow properties, and subscribed to ViewportChanged event |
| Tests/UnitTestsParallelizable/Views/TextViewScrollingTests.cs | Added 7 new tests verifying content size calculation, scrollbar behavior, and viewport synchronization |
| Tests/UnitTestsParallelizable/Views/TextViewNavigationTests.cs | Added 11 new tests for tab navigation, cursor movement, scrolling, and word wrap behavior |
| Tests/UnitTests/Views/TextViewTests.cs | Removed 9 old non-parallelizable tests that relied on hard-coded viewport dimensions |
| // Update content size when content changes | ||
| if (IsInitialized) | ||
| { | ||
| UpdateContentSize (); | ||
| } |
There was a problem hiding this comment.
Performance concern: UpdateContentSize() is called on every content change (each keystroke), which triggers _model.GetMaxVisibleLine(0, _model.Count, TabWidth) - a potentially expensive O(n) operation that scans all lines. For large documents, this could cause noticeable lag during typing. Consider throttling or debouncing this call, or caching the max line width and only updating it when necessary.
| IDriver driver = CreateFakeDriver (); | ||
|
|
||
| var textView = new TextView | ||
| { | ||
| Width = 10, | ||
| Height = 5, | ||
| Text = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7", | ||
| Driver = driver |
There was a problem hiding this comment.
Unnecessary Driver assignments in tests that don't call Draw(). According to best practices, tests that don't call Draw() should not assign a Driver. This test (and all other tests in this file) should remove the Driver = driver assignment and the IDriver driver = CreateFakeDriver (); variable declaration.
| // Safety check to prevent infinite loop | ||
| if (col == 0) | ||
| { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Dead code: this safety check is unreachable. The loop condition col > 1 ensures col never reaches 0 inside the loop body. When col becomes 1, the loop exits due to the condition col > 1 being false. Remove this redundant check.
| // Safety check to prevent infinite loop | |
| if (col == 0) | |
| { | |
| break; | |
| } |
| else | ||
| { | ||
| // No word wrap: calculate max line width | ||
| // Cache the current value to avoid recalculating on every call |
There was a problem hiding this comment.
Misleading comment: the comment claims to "cache the current value to avoid recalculating on every call" but no caching is actually implemented. The method calls _model.GetMaxVisibleLine() on every invocation. Either implement caching or remove the misleading comment.
| // Cache the current value to avoid recalculating on every call |
| int contentHeight = Math.Max (_model.Count, 1); | ||
|
|
||
| // For horizontal size: if word wrap is enabled, content width equals viewport width | ||
| // Otherwise, calculate the maximum line width (but only if we have a reasonable viewport) |
There was a problem hiding this comment.
Incomplete or outdated comment: the comment mentions "(but only if we have a reasonable viewport)" but there's no conditional check for a "reasonable viewport" in the code. Remove this misleading phrase from the comment.
| // Otherwise, calculate the maximum line width (but only if we have a reasonable viewport) | |
| // Otherwise, calculate the maximum line width |
| if (_wordWrap) | ||
| { | ||
| // Word wrap: content width follows viewport width | ||
| contentWidth = Math.Max (Viewport.Width, 1); | ||
| } | ||
| else | ||
| { | ||
| // No word wrap: calculate max line width | ||
| // Cache the current value to avoid recalculating on every call | ||
| contentWidth = Math.Max (_model.GetMaxVisibleLine (0, _model.Count, TabWidth), 1); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (_wordWrap) | |
| { | |
| // Word wrap: content width follows viewport width | |
| contentWidth = Math.Max (Viewport.Width, 1); | |
| } | |
| else | |
| { | |
| // No word wrap: calculate max line width | |
| // Cache the current value to avoid recalculating on every call | |
| contentWidth = Math.Max (_model.GetMaxVisibleLine (0, _model.Count, TabWidth), 1); | |
| } | |
| contentWidth = _wordWrap | |
| ? Math.Max (Viewport.Width, 1) | |
| : Math.Max (_model.GetMaxVisibleLine (0, _model.Count, TabWidth), 1); |
Move TextView, ContentsChangedEventArgs, WordWrapManager to TextView/ subfolder Move HistoryText, HistoryTextItemEventArgs, TextEditingLineStatus to History/ subfolder Move TextValidation files to TextValidation/ subfolder Build and all 163 tests pass
Move TextViewAutocomplete class from TextView.cs to TextViewAutocomplete.cs Build and all 163 tests pass
Event handlers moved from TextView.EventHandlers.cs to their logical locations: - Initialization handlers (TextView_Initialized, TextView_SuperViewChanged, TextView_ViewportChanged, TextView_LayoutComplete, Model_LinesLoaded) -> TextView.Core.cs - History handler (HistoryText_ChangeText) -> TextView.Insert.cs - Context menu handler (ContextMenu_KeyChanged) -> TextView.ContextMenu.cs - Removed TextView.EventHandlers.cs Build successful, all 163 tests pass
All private utility methods now have XML docs with 'INTERNAL:' prefix: - Adjust: Scroll position and cursor visibility management - DoNeededAction: Redraw determination and cursor positioning - OffSetBackground: Viewport offset calculation - UpdateContentSize: Content size management for scrolling - ResetPosition: Document position reset - ResetColumnTrack: Column tracking state reset Build successful, all 163 tests pass
The method calculates viewport clipping caused by the view extending beyond the SuperView's boundaries, not anything related to 'background'. - Renamed OffSetBackground() -> GetViewportClipping() - Updated XML documentation to accurately describe clipping behavior - Fixed incorrect terminology (background -> viewport clipping) - Updated TextView.Drawing.cs to use renamed method This is a legacy naming issue. The term 'background' was misleading as it has nothing to do with background colors or rendering. The method returns negative offsets when the viewport extends beyond the SuperView, representing how much is clipped. Build successful, all 163 tests pass
Fixes
TextView- Upgrade to use Upgrade to use Content Area for scrolling #3823Proposed Changes/Todos
TextView manually managed scrolling via
_topRow/_leftColumnfields instead of using View's built-inViewport,SetContentSize(), andScrollBarsystem.Core scrolling integration:
SetContentSize()with text model dimensions (line count × max line width)_topRow/_leftColumn withViewport.X/YbidirectionallyViewportChangedto keep internal fields synchronizedScrollBar configuration (per issue requirements):
VerticalScrollBar.AutoShow = trueby defaultHorizontalScrollBar.AutoShow = !WordWrap(disabled when wrapping)Backward compatibility:
_topRow/_leftColumn` fields to avoid refactoring all usage sitesTopRow,LeftColumn,ScrollTo()) unchangedTests:
TextViewNavigationTests.cscovering tab/backtab navigation, cursor movement, scrolling, word wrap, and unwrapped cursor positionDriverassignments from tests that don't callDraw()Example
Breaking Changes (Intended)
Pull Request checklist:
dotnet testbefore commit///style comments)Original prompt
TextView- Upgrade to use Upgrade to use Content Area for scrolling #3823💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.