Fixes #5359. Normalize NeedsDrawRect to viewport-local coordinates by harder · Pull Request #5435 · gui-cs/Terminal.Gui

harder · 2026-05-27T21:40:59Z

Summary

Fixes Fix NeedsDrawRect accumulation so partial invalidations stay partial #5359

Note

#5431 has merged to develop. This branch has been rebased onto the post-merge develop tip, so the diff now contains only the #5359 commits.

Standardizes NeedsDrawRect and the viewPortRelativeRegion parameter of SetNeedsDraw on viewport-local coordinates: (0, 0) is the top-left visible cell of the View's Viewport, independent of Viewport.Location (scroll offset, possibly negative under AllowNegativeX/Y). This matches the convention ViewportToScreen(in Rectangle) already expects.

This is the deferred coordinate-system normalization called out in #5431's description:

the cascade in View.NeedsDraw.cs produces frame-local rects while SetNeedsDraw()-no-arg passes content-coord Viewport; normalizing that convention is deferred.

Split from #4973.

Root causes

SetNeedsDraw() no-arg passed Viewport directly — so the scroll offset (Viewport.Location, possibly negative) leaked into NeedsDrawRect.
DoDrawAdornments escalation did the same: NeedsDrawRect = Viewport.
SetNeedsDraw(Rectangle) subview cascade compared subview.Frame (in parent's content coords) directly to viewPortRelativeRegion (intended as viewport-local). Only correct when parent.Viewport.Location == (0, 0), and it ignored the subview's own adornment offset.
SetFrame's SuperView invalidation (added in Fixes #5358. Narrow draw pipeline to skip parent on child-only redraws #5431) passed Rectangle.Union(prev, frame) — in SuperView's CONTENT coords — to SuperView.SetNeedsDraw, which expects viewport-local.

PR #5431 worked around (3) by gating its region-aware ClearViewport narrowing on Viewport.Location == Point.Empty. With the convention normalized, that guard is removed.

Changes

View.NeedsDraw.cs
- NeedsDrawRect doc comment now explicitly states viewport-LOCAL semantics.
- SetNeedsDraw() no-arg passes new (Point.Empty, Viewport.Size) — never leaks the scroll offset.
- SetNeedsDraw(Rectangle) subview cascade translates through the chain: parent's viewport-local → parent's content coords (add parent Viewport.Location) → intersect with subview.Frame → subview-frame-local (subtract Frame.Location) → subview-viewport-local (subtract GetViewportOffsetFromFrame()) → clip to the subview's viewport bounds. The subview's own Viewport.Location is not subtracted: viewport-local coords are scroll-independent — (0, 0) is always the top-left visible cell, and we propagate the dirty on-screen cells, not the content cells they happen to be showing. If the clipped result is empty (dirty area is only in the subview's adornments), the cascade falls back to a no-arg subview SetNeedsDraw so the subview redraws safely and its adornments are flagged.
View.Drawing.Adornments.cs — DoDrawAdornments escalation: NeedsDrawRect = new (Point.Empty, Viewport.Size) instead of Viewport.
View.Layout.cs — SetFrame translates Rectangle.Union(prev, frame) from SuperView content coords to SuperView viewport-local before invalidating.
View.Drawing.cs — CanNarrowClearToNeedsDrawRect no longer early-returns when Viewport.Location != Point.Empty. Updates the DoClearViewport comment block to reflect the new contract.

Testing

dotnet build --no-restore — succeeds, no new warnings.
dotnet test --project Tests/UnitTestsParallelizable --no-build — 17270 pass / 0 fail / 17 skip after rebase onto post-merge develop.

New tests — Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs (10 tests):

SetNeedsDraw_NoArg_OnScrolledView_StoresViewportLocalRect
SetNeedsDraw_NoArg_OnNegativeViewportLocation_StoresViewportLocalRect
SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview
SetNeedsDraw_SubViewWithPadding_CascadesViewportLocalRectIntoSubviewViewport
SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAtCorrectOnScreenPosition
SetNeedsDraw_DirtyRegionInSubviewAdornmentOnly_FallsBackToFullSubviewRedraw
FrameworkNarrowing_NowWorks_OnScrolledView
SetNeedsDraw_ZeroSizeViewport_IsNoOp
SetFrame_ScrolledSuperView_TranslatesInvalidationToViewportLocal
SetNeedsDraw_RepeatedSmallInvalidationsOnScrolledView_AccumulateAsViewportLocalUnion

Updated tests:

Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs — NeedsDrawRect_Is_Viewport_Relative scrolled-frame assertions updated to reflect the new convention (post-scroll SetFrame no longer accumulates a scroll-offset rect).
Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs — removed the now-obsolete FrameworkDraw_ScrolledView_FallsBackToFullClear test (it documented Fixes #5358. Narrow draw pipeline to skip parent on child-only redraws #5431's temporary Viewport.Location == Point.Empty guard, now removed); replaced with a pointer comment to NeedsDrawCoordTests.FrameworkNarrowing_NowWorks_OnScrolledView.

Acceptance criteria mapping

AC	Coverage
AC1 (repeated small invalidations → correct union)	Already in #5431 (`Rectangle.Union` fix). Existing `SetNeedsDraw_MultipleRectangles_Expands` test covers; new `…_RepeatedSmallInvalidationsOnScrolledView_…` test pins it across the new coord convention.
AC2 (parent → subview translation correct)	Fixed cascade + new `_ScrolledParent_`, `_SubViewWithPadding_`, `_ScrolledSubview_`, `_DirtyRegionInSubviewAdornmentOnly_` tests.
AC3 (viewport movement / negative locations)	`SetNeedsDraw()` no-arg fix + new `_OnScrolledView_`, `_OnNegativeViewportLocation_` tests.

…ests CoPilot review of PR gui-cs#5435 caught three issues: 1. (Blocking) The subview cascade in View.NeedsDraw.cs subtracted subview.Viewport.Location on top of the adornment offset when converting to subview-viewport-local. That's wrong — viewport-local coords are scroll-INDEPENDENT ((0, 0) is always the top-left visible cell regardless of scroll; scroll changes which content cell appears there, not the viewport-local coords themselves). Subtracting the scroll shifted the dirty rect to a different on-screen position than the cells that were actually dirtied. ViewportToScreen treats input as scroll-independent, so the bug would also corrupt the framework's narrowing in CanNarrowClearToNeedsDrawRect for the subview's later self-redraw. Fix: only subtract the adornment offset. 2. (Medium) The RegionAwareClearViewportTests.FrameworkDraw_ScrolledView_FallsBackToFullClear test documented the workaround guard removed by this PR. With the guard gone the test was passing accidentally because its dirty rect (1, 6, 3, 2) was outside the 5-row viewport visible area (Y=6 > 4), so the narrowing intersected to empty and fell through for an unrelated reason. Delete the test — NeedsDrawCoordTests's FrameworkNarrowing_NowWorks_OnScrolledView covers the correct behavior. Replace with a comment block explaining the removal. 3. (Low) SetFrame's scroll translation in View.Layout.cs was only exercised by tests with SuperView.Viewport.Location == Point.Empty (where the translation is a no-op). Add SetFrame_ScrolledSuperView_TranslatesInvalidationToViewportLocal to cover the scrolled case. Also update SetNeedsDraw_ScrolledSubview_... test: the previous assertion locked in the buggy shifted rect. With the cascade corrected, parent dirty viewport-local (2, 2, 4, 2) → subview viewport-local (2, 2, 4, 2) (same on-screen cells, just rendered by the subview now). Full suites still green: 17285 / 74 / 433. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Normalizes NeedsDrawRect (and SetNeedsDraw(Rectangle)’s viewPortRelativeRegion) to viewport-local coordinates so dirty-rect accumulation, parent→subview propagation, and framework clear narrowing behave consistently under scrolling (including negative Viewport.Location).

Changes:

Standardize invalidation to (0,0)-based viewport-local rects (no scroll-offset leakage) and translate dirty regions correctly when cascading into subviews.
Update draw/layout plumbing to use the normalized contract (adornment escalation, region-aware clear narrowing on scrolled views, and SetFrame → SuperView invalidation translation).
Add/adjust unit + integration tests to pin the coordinate convention and fan-out behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
Tests/UnitTestsParallelizable/Views/TabView/TabsFanOutDiagnosticTests.cs	Switch diagnostics to track `DrawingText` (NeedsDraw-gated) to validate reduced inactive-tab fan-out.
Tests/UnitTestsParallelizable/ViewBase/Draw/SubViewOnlyRedrawTests.cs	New tests ensuring child-only invalidations don’t trigger parent clear/text/content redraw; includes TransparentMouse regression coverage.
Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs	New tests covering public full-clear contract vs framework narrowing behavior, plus frame-shrink invalidation semantics.
Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs	Update expectations to reflect true-union accumulation and viewport-local semantics after scroll/frame changes.
Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs	New tests pinning viewport-local invalidation across scroll/negative locations, subview adornments, and scrolled subviews.
Tests/IntegrationTests/TabsFanOutIntegrationTests.cs	Track `DrawingText` to better isolate NeedsDraw-gated fan-out vs remaining `LayoutAndDraw(force=true)` behavior.
Terminal.Gui/ViewBase/View.NeedsDraw.cs	Enforce viewport-local invalidation; fix no-arg `SetNeedsDraw`; correct parent→subview dirty-rect translation chain.
Terminal.Gui/ViewBase/View.Layout.cs	Translate `SetFrame`’s superview invalidation from content coords to superview viewport-local coords.
Terminal.Gui/ViewBase/View.Drawing.cs	Gate self-clear/text/content on a pre-adornment `NeedsDraw` snapshot; enable clear narrowing on scrolled views using normalized rects.
Terminal.Gui/ViewBase/View.Drawing.Adornments.cs	Ensure adornment escalation sets viewport-local full-viewport rect (no scroll offset).
Terminal.Gui/ViewBase/Adornment/AdornmentView.cs	Remove mid-pass `SetNeedsDraw()` churn from `OnClearingViewport`; update migration commentary.
Terminal.Gui/App/ApplicationImpl.Screen.cs	Document remaining draw fan-out source via `LayoutAndDraw`’s `force=true` path.
plans/5359-needsdraw-coord-normalization.md	Add design/verification notes documenting the new coordinate convention and test mapping.

NeedsDrawRect and the viewPortRelativeRegion parameter of SetNeedsDraw are now in viewport-LOCAL coords: (0, 0) is the top-left visible cell of the View's Viewport, independent of Viewport.Location (scroll offset, possibly negative under AllowNegativeX/Y). Previously the no-arg SetNeedsDraw() passed Viewport (with the scroll offset baked in), DoDrawAdornments escalation did the same, and the subview cascade compared parent's viewport-relative region to subview.Frame (in parent's content coords) without accounting for the parent's scroll. PR gui-cs#5431 worked around the resulting inconsistency by gating its region-aware ClearViewport narrowing on "Viewport.Location == Point.Empty"; this commit normalizes the convention so that workaround can be removed in a follow-up commit. Changes: - SetNeedsDraw() no-arg now passes new (Point.Empty, Viewport.Size). - DoDrawAdornments NeedsDrawRect escalation does the same. - SetNeedsDraw(Rectangle) subview cascade translates the viewport-local region through: parent scroll -> content coords -> intersect with subview.Frame -> subview frame-local -> subtract subview adornment offset and subview scroll -> clip to subview viewport bounds. If the result is empty (dirty area is only in subview's adornments/scrolled- off content), fall back to a no-arg subview SetNeedsDraw so the subview redraws safely. - View.Layout.SetFrame translates the union(old, new) frame from SuperView content coords to SuperView viewport-local before passing to SuperView.SetNeedsDraw. - Update NeedsDrawRect_Is_Viewport_Relative test scrolled-frame assertions to reflect the new convention (NeedsDrawRect no longer acquires scroll-position offsets). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…e normalized PR gui-cs#5431 added a "Viewport.Location == Point.Empty" early-return to CanNarrowClearToNeedsDrawRect because the cascade in SetNeedsDraw produced rects in a different coordinate space than ViewportToScreen expects. Issue gui-cs#5359 normalized NeedsDrawRect on viewport-local coordinates, so the conversion via ViewportToScreen is now safe regardless of the view's scroll state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Nine tests pin the viewport-local coordinate convention: - SetNeedsDraw() no-arg on a scrolled view stores (0, 0, W, H). - SetNeedsDraw() no-arg on a view with negative Viewport.Location stores (0, 0, W, H). - Subview cascade from a scrolled parent produces correct subview- viewport-local rect (parent scroll translated out). - Subview-with-padding cascade produces subview-viewport-local coords (adornment offset subtracted), not subview-frame-local. - Scrolled-subview cascade subtracts the subview's own scroll on top of the frame and adornment offsets. - Dirty region overlapping only the subview's adornment area falls back to a full subview SetNeedsDraw (the safe fallback). - Framework ClearViewport narrowing now works on scrolled views (the scrolled-view guard removal from this PR). - Zero-size viewport is a no-op. - Repeated small invalidations on a scrolled view accumulate as a viewport-local union. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ests CoPilot review of PR gui-cs#5435 caught three issues: 1. (Blocking) The subview cascade in View.NeedsDraw.cs subtracted subview.Viewport.Location on top of the adornment offset when converting to subview-viewport-local. That's wrong — viewport-local coords are scroll-INDEPENDENT ((0, 0) is always the top-left visible cell regardless of scroll; scroll changes which content cell appears there, not the viewport-local coords themselves). Subtracting the scroll shifted the dirty rect to a different on-screen position than the cells that were actually dirtied. ViewportToScreen treats input as scroll-independent, so the bug would also corrupt the framework's narrowing in CanNarrowClearToNeedsDrawRect for the subview's later self-redraw. Fix: only subtract the adornment offset. 2. (Medium) The RegionAwareClearViewportTests.FrameworkDraw_ScrolledView_FallsBackToFullClear test documented the workaround guard removed by this PR. With the guard gone the test was passing accidentally because its dirty rect (1, 6, 3, 2) was outside the 5-row viewport visible area (Y=6 > 4), so the narrowing intersected to empty and fell through for an unrelated reason. Delete the test — NeedsDrawCoordTests's FrameworkNarrowing_NowWorks_OnScrolledView covers the correct behavior. Replace with a comment block explaining the removal. 3. (Low) SetFrame's scroll translation in View.Layout.cs was only exercised by tests with SuperView.Viewport.Location == Point.Empty (where the translation is a no-op). Add SetFrame_ScrolledSuperView_TranslatesInvalidationToViewportLocal to cover the scrolled case. Also update SetNeedsDraw_ScrolledSubview_... test: the previous assertion locked in the buggy shifted rect. With the cascade corrected, parent dirty viewport-local (2, 2, 4, 2) → subview viewport-local (2, 2, 4, 2) (same on-screen cells, just rendered by the subview now). Full suites still green: 17285 / 74 / 433. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Correct the step-4 cascade comment to say GetViewportOffsetFromFrame() is the combined Margin/Border/Padding inset (not just Padding), and update the plan to match the implemented contract — the cascade does not subtract the subview's Viewport.Location because viewport-local coordinates are scroll-independent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The Copilot autofix in 491f190 opened <para> twice with a single closing tag. Remove the stray opening tag so the doc comment is well-formed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

tig

LGTM;clean fix, solid tests. Please remove plans/5359-needsdraw-coord-normalization.md before merge.

One question: in the subview cascade fallback (line 174 in View.NeedsDraw.cs), when the dirty region overlaps the subview's frame but lands entirely in its adornment area, you call the no-arg SetNeedsDraw() which marks the entire subview viewport dirty. Was a more targeted adornment-only invalidation considered, or is full-subview the intentional safe default here?

harder · 2026-05-29T16:31:12Z

Removed the plan file - thanks for noticing that.

Full-subview was the intentional safe default. At that point in the cascade, all we know is that the parent dirty region intersected the subview’s frame. But after translating into the subview’s viewport there’s no viewport-local region left, it’s all adornment territory.

I asked my agent to consider a narrower adornment-only invalidation, but we don’t have a separate targeted dirty region path there for border/padding, and the current draw contract treats adornment dirtiness as part of the subview redraw path anyway. So kept subview.SetNeedsDraw () as the conservative fallback, and the SetNeedsDraw_DirtyRegionInSubviewAdornmentOnly_FallsBackToFullSubviewRedraw test is meant to pin that behavior.

We could do a follow-up issue to try to optimize that case though.
I created #5434 earlier which isn't exactly the same, but adjacent - it deals with precise invalidation around adornment thickness.

harder requested a review from Copilot May 27, 2026 22:22

Copilot started reviewing on behalf of harder May 27, 2026 22:22 View session

Copilot AI reviewed May 27, 2026

View reviewed changes

Comment thread Terminal.Gui/ViewBase/View.NeedsDraw.cs Outdated

harder and others added 4 commits May 28, 2026 10:36

harder force-pushed the fix-5359-needsdraw-coord-normalization branch from 676a2c2 to c49d075 Compare May 28, 2026 15:45

harder requested a review from Copilot May 28, 2026 15:57

Copilot started reviewing on behalf of harder May 28, 2026 15:57 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

Comment thread Terminal.Gui/ViewBase/View.NeedsDraw.cs

Comment thread plans/5359-needsdraw-coord-normalization.md Outdated

Comment thread Terminal.Gui/ViewBase/View.NeedsDraw.cs Outdated

harder and others added 3 commits May 28, 2026 11:28

Updated comment from code review feedback

491f190

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix malformed duplicate <para> in SetNeedsDraw XML doc

e982dde

The Copilot autofix in 491f190 opened <para> twice with a single closing tag. Remove the stray opening tag so the doc comment is well-formed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

harder marked this pull request as ready for review May 28, 2026 16:44

harder requested a review from tig as a code owner May 28, 2026 16:44

harder mentioned this pull request May 28, 2026

Fixes #5433. Make View.Draw self-draw context flow explicit #5440

Merged

tig approved these changes May 28, 2026

View reviewed changes

removed plan file

907beac

harder merged commit 751e247 into gui-cs:develop May 29, 2026
13 checks passed

harder deleted the fix-5359-needsdraw-coord-normalization branch May 29, 2026 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5359. Normalize NeedsDrawRect to viewport-local coordinates#5435

Fixes #5359. Normalize NeedsDrawRect to viewport-local coordinates#5435
harder merged 8 commits into
gui-cs:developfrom
harder:fix-5359-needsdraw-coord-normalization

harder commented May 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tig left a comment •

edited

Loading

Uh oh!

harder commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

harder commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root causes

Changes

Testing

Acceptance criteria mapping

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harder commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

harder commented May 27, 2026 •

edited

Loading

tig left a comment •

edited

Loading