From 4676940ca05f81d55fa703223060fa10125a38ea Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Wed, 27 May 2026 16:26:52 -0500 Subject: [PATCH 1/8] Standardize NeedsDrawRect on viewport-local coordinates (#5359) 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 #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) --- .../ViewBase/View.Drawing.Adornments.cs | 5 +- Terminal.Gui/ViewBase/View.Layout.cs | 10 +- Terminal.Gui/ViewBase/View.NeedsDraw.cs | 74 ++++++++++++--- .../ViewBase/Draw/NeedsDrawTests.cs | 12 +-- plans/5359-needsdraw-coord-normalization.md | 91 +++++++++++++++++++ 5 files changed, 173 insertions(+), 19 deletions(-) create mode 100644 plans/5359-needsdraw-coord-normalization.md diff --git a/Terminal.Gui/ViewBase/View.Drawing.Adornments.cs b/Terminal.Gui/ViewBase/View.Drawing.Adornments.cs index ebabfe0c2f..179ed4c54c 100644 --- a/Terminal.Gui/ViewBase/View.Drawing.Adornments.cs +++ b/Terminal.Gui/ViewBase/View.Drawing.Adornments.cs @@ -137,9 +137,12 @@ internal void DoDrawAdornments (Region? originalClip) // methods (DoClearViewport, DoDrawText, DoDrawContent) are now gated on the // needsDrawSelf snapshot captured in Draw() *before* this escalation, so this no // longer forces a full parent redraw when only a child was dirty (issue #5358). + // + // Issue #5359: NeedsDrawRect is viewport-LOCAL, so set (0, 0, W, H) instead of + // Viewport (which carries the scroll offset and would corrupt the convention). if (NeedsDrawRect == Rectangle.Empty) { - NeedsDrawRect = Viewport; + NeedsDrawRect = new (Point.Empty, Viewport.Size); } if (OnDrawingAdornments ()) diff --git a/Terminal.Gui/ViewBase/View.Layout.cs b/Terminal.Gui/ViewBase/View.Layout.cs index 0db07b69a4..a740587f30 100644 --- a/Terminal.Gui/ViewBase/View.Layout.cs +++ b/Terminal.Gui/ViewBase/View.Layout.cs @@ -117,9 +117,17 @@ private bool SetFrame (in Rectangle frame) // new frames on the SuperView so its region-aware ClearViewport repaints just that area. // SetFrame is the single source of truth for this invalidation for both direct Frame // assignment and layout-driven frame updates. + // + // Issue #5359: Frames are in SuperView CONTENT coords, but SetNeedsDraw expects + // SuperView VIEWPORT-LOCAL coords. Translate by subtracting the SuperView's scroll + // offset (Viewport.Location). if (oldFrame is { } prev && SuperView is { }) { - SuperView.SetNeedsDraw (Rectangle.Union (prev, frame)); + Rectangle invalidationContent = Rectangle.Union (prev, frame); + Point superScroll = SuperView.Viewport.Location; + Rectangle invalidationViewport = invalidationContent; + invalidationViewport.Offset (-superScroll.X, -superScroll.Y); + SuperView.SetNeedsDraw (invalidationViewport); } // BUGBUG: When SetFrame is called from Frame_set, this event gets raised BEFORE OnResizeNeeded. Is that OK? diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index 5303d20106..bf91bbdf2d 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -2,14 +2,19 @@ public partial class View { - // NOTE: NeedsDrawRect is not currently used to clip drawing to only the invalidated region. - // It is only used within SetNeedsDraw to propagate redraw requests to subviews. + // NOTE: NeedsDrawRect is in viewport-LOCAL coordinates: (0, 0) is the top-left visible + // cell of the View's Viewport (inside Padding, after any scroll). The rect is + // independent of Viewport.Location, so scrolling and negative viewport locations do + // not bleed into the dirty rect. See SetNeedsDraw(Rectangle) for the cascade + // convention used to propagate dirty rects into SubViews. // NOTE: Consider changing NeedsDrawRect from Rectangle to Region for more precise invalidation // NeedsDraw is already efficiently cached via NeedsDrawRect. It checks: // 1. NeedsDrawRect (cached by SetNeedsDraw/ClearNeedsDraw) // 2. Adornment NeedsDraw flags (each cached separately) /// - /// INTERNAL: Gets the viewport-relative region that needs to be redrawn. + /// INTERNAL: Gets the viewport-local region that needs to be redrawn. (0, 0) is the + /// top-left of ; the rect is independent of 's + /// (scroll offset). /// internal Rectangle NeedsDrawRect { get; private set; } = Rectangle.Empty; @@ -41,20 +46,32 @@ public void SetNeedsDraw () return; } - SetNeedsDraw (viewport); + // Pass a viewport-LOCAL rect: (0, 0, W, H) covers the whole visible viewport regardless + // of scroll. Passing Viewport here would leak Viewport.Location (the scroll offset, which + // can also be negative under AllowNegativeX/Y) into NeedsDrawRect, breaking the + // viewport-local convention NeedsDrawRect is supposed to honor. + SetNeedsDraw (new (Point.Empty, viewport.Size)); } /// Expands the area of this view needing to be redrawn to include . /// /// - /// The location of is relative to the View's . + /// is in viewport-LOCAL coordinates: + /// (0, 0) is the top-left visible cell of the View's . + /// The rect does NOT include 's + /// (scroll offset). + /// + /// + /// The cascade to intersecting SubViews translates the region into each SubView's + /// own viewport-local coordinates, accounting for the parent's scroll, the SubView's + /// adornments, and the SubView's own scroll. /// /// /// If the view has not been initialized ( is ), the area to be /// redrawn will be the . /// /// - /// The relative region that needs to be redrawn. + /// The viewport-local region that needs to be redrawn. public void SetNeedsDraw (Rectangle viewPortRelativeRegion) { // Invalidate the cached drawn region used for TransparentMouse hit-testing. @@ -112,16 +129,51 @@ public void SetNeedsDraw (Rectangle viewPortRelativeRegion) adornment.Adornment?.Parent?.SetSubViewNeedsDrawDownHierarchy (); } + // Cascade the dirty region into intersecting SubViews. Coordinate conversion chain + // (issue #5359 — every step is needed for correctness under scroll/adornments): + // 1. viewPortRelativeRegion is in THIS view's viewport-local coords; translate to + // this view's content coords by adding Viewport.Location (the scroll offset). + // 2. subview.Frame is in this view's content coords; intersect there. + // 3. Translate the intersection to subview-frame-local (subtract Frame.Location). + // 4. Translate to subview-VIEWPORT-local by also subtracting the subview's adornment + // offset (Padding inset) and the subview's own Viewport.Location. + // 5. Clip to the subview's visible viewport bounds; anything outside is in adornment + // or scrolled-off territory. If nothing is left, fall back to a full subview + // invalidation so the subview at least redraws itself safely. + Point thisScroll = Viewport.Location; + Rectangle contentRegion = viewPortRelativeRegion; + contentRegion.Offset (thisScroll.X, thisScroll.Y); + foreach (View subview in InternalSubViews.Snapshot ()) { - if (!subview.Frame.IntersectsWith (viewPortRelativeRegion)) + if (!subview.Frame.IntersectsWith (contentRegion)) + { + continue; + } + + Rectangle subviewFrameRegion = Rectangle.Intersect (subview.Frame, contentRegion); + subviewFrameRegion.Offset (-subview.Frame.X, -subview.Frame.Y); + + Point subviewAdornmentOffset = subview.GetViewportOffsetFromFrame (); + Point subviewScroll = subview.Viewport.Location; + subviewFrameRegion.Offset (-subviewAdornmentOffset.X - subviewScroll.X, + -subviewAdornmentOffset.Y - subviewScroll.Y); + + Rectangle subviewViewportBounds = new (Point.Empty, subview.Viewport.Size); + Rectangle subviewViewportRegion = Rectangle.Intersect (subviewViewportBounds, subviewFrameRegion); + + if (subviewViewportRegion.IsEmpty) { + // Dirty region overlaps the subview's frame but only its adornment/scrolled-off + // area. The subview's own viewport isn't dirty; a no-arg SetNeedsDraw is the + // safe fallback (also flags adornments so any border/padding the dirty region + // touched gets repainted). + subview.SetNeedsDraw (); + continue; } - Rectangle subviewRegion = Rectangle.Intersect (subview.Frame, viewPortRelativeRegion); - subviewRegion.X -= subview.Frame.X; - subviewRegion.Y -= subview.Frame.Y; - subview.SetNeedsDraw (subviewRegion); + + subview.SetNeedsDraw (subviewViewportRegion); } } diff --git a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs index 3039b9ec94..a5b800a9d9 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs @@ -296,16 +296,16 @@ public void NeedsDrawRect_Is_Viewport_Relative () view.Frame = new Rectangle (3, 3, 6, 6); // Grow right/bottom 1 Assert.Equal (new Rectangle (3, 3, 6, 6), view.Frame); Assert.Equal (new Rectangle (1, 1, 6, 6), view.Viewport); - // Union((0,0,6,6),(1,1,6,6)) — Viewport now has scroll offset (1,1). - // SetFrame's SetNeedsDraw passes the current Viewport = (1,1,6,6). - // Rectangle.Union((0,0,6,6),(1,1,6,6)) = (0,0,7,7). - Assert.Equal (new Rectangle (0, 0, 7, 7), view.NeedsDrawRect); + // Issue #5359: NeedsDrawRect is viewport-LOCAL — independent of Viewport.Location + // (the scroll offset). SetFrame's SetNeedsDraw passes (0, 0, 6, 6), not (1, 1, 6, 6). + // Union((0,0,6,6),(0,0,6,6)) = (0,0,6,6). + Assert.Equal (new Rectangle (0, 0, 6, 6), view.NeedsDrawRect); view.Frame = new Rectangle (3, 3, 5, 5); Assert.Equal (new Rectangle (3, 3, 5, 5), view.Frame); Assert.Equal (new Rectangle (1, 1, 5, 5), view.Viewport); - // Union((0,0,7,7),(1,1,5,5)) = (0,0,7,7). - Assert.Equal (new Rectangle (0, 0, 7, 7), view.NeedsDrawRect); + // Union((0,0,6,6),(0,0,5,5)) = (0,0,6,6). + Assert.Equal (new Rectangle (0, 0, 6, 6), view.NeedsDrawRect); } [Fact] diff --git a/plans/5359-needsdraw-coord-normalization.md b/plans/5359-needsdraw-coord-normalization.md new file mode 100644 index 0000000000..94c783d7df --- /dev/null +++ b/plans/5359-needsdraw-coord-normalization.md @@ -0,0 +1,91 @@ +# Issue #5359 — `NeedsDrawRect` Coordinate-System Normalization + +**Branch:** `fix-5359-needsdraw-coord-normalization` (based on `fix-5358-stop-child-redraws-escalating`, which will be retargeted to `develop` after #5431 merges) + +**Parent issue:** [#4973](https://github.com/gui-cs/Terminal.Gui/issues/4973) +**Sibling PR:** [#5431](https://github.com/gui-cs/Terminal.Gui/pull/5431) — covers AC1 (union math) +**This PR covers:** AC2 (subview cascade) and AC3 (viewport movement / negative locations). + +--- + +## Background + +`NeedsDrawRect` is currently treated inconsistently across the codebase: + +- `SetNeedsDraw()` no-arg passes the *current* `Viewport` rectangle (i.e., `Viewport.Location` is the scroll offset; can be non-zero or negative). That stores content-coord rects in `NeedsDrawRect`. +- `DoDrawAdornments` escalation does the same: `NeedsDrawRect = Viewport`. +- The subview cascade in `SetNeedsDraw(Rectangle)` intersects `subview.Frame` (in `this`'s content coords) directly with `viewPortRelativeRegion`. Only works when `this.Viewport.Location == (0, 0)`. +- `CanNarrowClearToNeedsDrawRect` (added in #5431) consumes `NeedsDrawRect` via `ViewportToScreen`, which expects **viewport-local** coords (`(0, 0)` = top-left visible cell). That's why #5431 had to add a `Viewport.Location == Point.Empty` guard. + +The deferred follow-up called out in PR #5431's description is exactly this: *normalize the convention*. + +## Convention + +> **`NeedsDrawRect` and the `viewPortRelativeRegion` parameter of `SetNeedsDraw(Rectangle)` are in viewport-LOCAL coordinates: `(0, 0)` = top-left visible cell of the View's Viewport (inside `Padding`, after any scroll).** + +This matches what `ViewportToScreen(in Rectangle)` already expects and is the meaning documented for `ViewportToScreen` itself. + +## Changes + +### 1. `View.NeedsDraw.cs` + +- Update the XML doc on `NeedsDrawRect`, `SetNeedsDraw()`, and `SetNeedsDraw(Rectangle)` to explicitly state "viewport-local; `(0, 0)` = top-left visible cell." +- Fix `SetNeedsDraw()` no-arg to pass `new Rectangle(Point.Empty, Viewport.Size)` — never propagate the scroll offset into the dirty rect. +- Fix the subview cascade: + - Translate `viewPortRelativeRegion` → `this`'s content coords by adding `Viewport.Location`. + - Intersect with `subview.Frame` (already in `this`'s content coords). + - Translate the intersection to subview-frame-local by subtracting `subview.Frame.Location`. + - Translate to subview-viewport-local by subtracting `subview.GetViewportOffsetFromFrame()` and `subview.Viewport.Location`. + - Clip to the subview's viewport bounds (`new Rectangle(Point.Empty, subview.Viewport.Size)`); if empty, fall back to `subview.SetNeedsDraw()` no-arg (the subview still needs *some* redraw since the parent invalidation touched its frame area, but it's all in adornment / scrolled-off territory — let it do a safe full redraw). + +### 2. `View.Drawing.Adornments.cs` + +- `DoDrawAdornments` escalation: `NeedsDrawRect = new Rectangle(Point.Empty, Viewport.Size)` instead of `Viewport`. + +### 3. `View.Layout.cs` + +- `SetFrame`'s `SuperView.SetNeedsDraw(Rectangle.Union(prev, frame))` passes a rect in SuperView's content coords. Translate to SuperView's viewport-local by subtracting `SuperView.Viewport.Location` before passing. + +### 4. `View.Drawing.cs` + +- `CanNarrowClearToNeedsDrawRect`: remove the `Viewport.Location != Point.Empty` early-return. Update the comment block to reflect that narrowing is now safe regardless of scroll because `NeedsDrawRect` is consistently viewport-local. + +### 5. Tests + +- `Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs`: + - `NeedsDrawRect_Is_Viewport_Relative` — update the scrolled-frame assertions. The post-scroll `view.Frame = (3, 3, 6, 6)` step previously asserted `NeedsDrawRect = (0, 0, 7, 7)` because the broken `SetNeedsDraw` passed `Viewport = (1, 1, 6, 6)`. With the fix it passes `(0, 0, 6, 6)`, and accumulating union with prior `(0, 0, 6, 6)` stays `(0, 0, 6, 6)`. + +- New file `Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs`: + - `SetNeedsDraw_NoArg_OnScrolledView_StoresViewportLocalRect` — Viewport at `(5, 3, 10, 8)`; `SetNeedsDraw()` → `NeedsDrawRect == (0, 0, 10, 8)`. + - `SetNeedsDraw_NoArg_OnNegativeViewportLocation_StoresViewportLocalRect` — `AllowNegativeX`, `Viewport.X = -3`; `SetNeedsDraw()` → `NeedsDrawRect == (0, 0, W, H)`. + - `SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview` — parent scrolled by `(5, 0)`; invalidate content-area rect that overlaps a subview; subview's `NeedsDrawRect` matches the expected viewport-local slice. + - `SetNeedsDraw_SubViewWithPadding_CascadesViewportLocalRectIntoSubviewViewport` — subview has `Padding.Thickness = (1)`; parent invalidation that covers subview's full frame should land as `(0, 0, viewport.W, viewport.H)` in subview-viewport-local, not as the frame-local rect. + - `SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAccountingForScroll` — subview is itself scrolled; cascade produces correct viewport-local rect. + - `SetNeedsDraw_DirtyRegionInSubviewAdornmentOnly_FallsBackToFullSubviewRedraw` — invalidation overlaps only subview's padding/border; subview's `NeedsDrawRect` ends up as full viewport (safe fallback) and the subview's adornments are flagged. + - `FrameworkNarrowing_NowWorks_OnScrolledView` — after removing the guard, a scrolled view with a partial `NeedsDrawRect` narrows the clear correctly. + - `SetNeedsDraw_ZeroSizeViewport_IsNoOp` — edge case. + +## Acceptance Criteria Mapping + +| AC | Coverage | +|----|----------| +| AC1 (repeated small invalidations → correct union) | Already in PR #5431 (`Rectangle.Union`). Existing test `SetNeedsDraw_MultipleRectangles_Expands` covers it. | +| AC2 (parent → subview translation correct) | Fixed cascade + new tests `SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview`, `…_SubViewWithPadding_…`, `…_ScrolledSubview_…`, `…_DirtyRegionInSubviewAdornmentOnly_…`. | +| AC3 (viewport movement / negative locations) | `SetNeedsDraw()` no-arg fix + new tests `…_OnScrolledView_…`, `…_OnNegativeViewportLocation_…`. | + +## Out of Scope (per issue's "Non-Functional Requirements" / "do not expand") + +- Region-based dirty rect (stay with `Rectangle`). +- Per-cell partial rendering / clipping at the IOutput level. +- Removing the `NeedsDrawRect = Viewport.Size` escalation in `DoDrawAdornments` entirely (that's tied to the `LayoutAndDraw force=true` fan-out tracked separately in #5434). + +## Verification + +```bash +dotnet build --no-restore +dotnet test --project Tests/UnitTestsParallelizable --no-build +dotnet test --project Tests/UnitTests.NonParallelizable --no-build +dotnet test --project Tests/IntegrationTests --no-build +``` + +Expect: all green; no new warnings; existing test count + new tests added. From cc9829dd31d9b43cf75127c0a9e32a4198260bd9 Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Wed, 27 May 2026 16:27:00 -0500 Subject: [PATCH 2/8] Remove scrolled-view narrowing guard now that NeedsDrawRect coords are normalized PR #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 #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) --- Terminal.Gui/ViewBase/View.Drawing.cs | 31 ++++++++------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/Terminal.Gui/ViewBase/View.Drawing.cs b/Terminal.Gui/ViewBase/View.Drawing.cs index 3417168b93..6eda6390dc 100644 --- a/Terminal.Gui/ViewBase/View.Drawing.cs +++ b/Terminal.Gui/ViewBase/View.Drawing.cs @@ -241,18 +241,11 @@ internal void DoClearViewport (DrawContext? context = null) } // Issue #5358: narrow the framework's clear to NeedsDrawRect when it's a true - // partial region AND this view is not itself scrolled. Narrowing in the public - // ClearViewport API would silently change the contract for direct callers - // (Code.OnClearingViewport, MarkdownCodeBlock, direct test calls) that expect - // a full fill of the viewport background — see review feedback items 1, 3. - // - // The "this view is not itself scrolled" guard sidesteps a separate coordinate- - // space inconsistency: SetNeedsDraw(Rectangle) cascades to subviews using - // frame-local coordinates (subtracts subview.Frame.X/Y), while the no-arg - // SetNeedsDraw passes Viewport (content-coord). For an unscrolled view those - // coincide; for a scrolled view they don't, and the narrowing math would shift - // the clear off-screen. Until that convention is normalized (out of scope here), - // only narrow when Viewport.Location is the origin. + // partial region. Narrowing in the public ClearViewport API would silently change the + // contract for direct callers (Code.OnClearingViewport, MarkdownCodeBlock, direct test + // calls) that expect a full fill of the viewport background — see review feedback + // items 1, 3 on PR #5431. Issue #5359 normalized NeedsDrawRect on viewport-local + // coordinates, so narrowing is now safe regardless of scroll state. if (CanNarrowClearToNeedsDrawRect (out Rectangle narrowedScreen)) { Driver?.FillRect (narrowedScreen); @@ -289,15 +282,9 @@ private bool CanNarrowClearToNeedsDrawRect (out Rectangle narrowedScreen) Rectangle viewport = Viewport; - // Only narrow when this view is NOT itself scrolled — see DoClearViewport comment. - if (viewport.Location != Point.Empty) - { - return false; - } - // Only narrow when NeedsDrawRect is strictly smaller than the viewport. SetNeedsDraw() - // (no-arg) sets NeedsDrawRect to the current Viewport, meaning "everything is dirty"; - // we don't want to narrow in that case. + // (no-arg) sets NeedsDrawRect to (Point.Empty, Viewport.Size), meaning "everything is + // dirty"; we don't want to narrow in that case. if (NeedsDrawRect.Width >= viewport.Width && NeedsDrawRect.Height >= viewport.Height) { return false; @@ -310,8 +297,8 @@ private bool CanNarrowClearToNeedsDrawRect (out Rectangle narrowedScreen) return false; } - // NeedsDrawRect is in this view's coords; for an unscrolled view those equal viewport- - // local coords (origin (0,0) = top-left of visible area). Convert to screen. + // NeedsDrawRect is viewport-LOCAL — (0, 0) is the top-left visible cell — so + // ViewportToScreen converts it correctly regardless of any scroll on this view. Rectangle dirtyScreen = ViewportToScreen (NeedsDrawRect); Rectangle toClear = ViewportToScreen (viewport with { Location = Point.Empty }); narrowedScreen = Rectangle.Intersect (toClear, dirtyScreen); From b9ab21edc427554465122b63671a8b049073ae38 Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Wed, 27 May 2026 16:27:09 -0500 Subject: [PATCH 3/8] Add NeedsDrawCoordTests covering #5359 acceptance criteria 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) --- .../ViewBase/Draw/NeedsDrawCoordTests.cs | 354 ++++++++++++++++++ 1 file changed, 354 insertions(+) create mode 100644 Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs diff --git a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs new file mode 100644 index 0000000000..b43d708be7 --- /dev/null +++ b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs @@ -0,0 +1,354 @@ +#nullable disable +using System.Text; +using UnitTests; + +namespace ViewBaseTests.Draw; + +// Claude - Opus 4.7 +/// +/// Issue #5359: NeedsDrawRect and the viewPortRelativeRegion parameter of SetNeedsDraw are in +/// viewport-LOCAL coordinates — (0, 0) is the top-left visible cell of the View's Viewport, +/// independent of Viewport.Location (scroll offset, possibly negative). +/// +/// These tests pin down the convention across: +/// * SetNeedsDraw() no-arg on a scrolled or negative-viewport-location view +/// * Subview cascade when parent is scrolled +/// * Subview cascade when subview has adornments +/// * Subview cascade when subview is itself scrolled +/// * Subview cascade when dirty region only overlaps subview's adornments +/// * Framework ClearViewport narrowing now working on scrolled views +/// * Zero-size viewport edge case +/// +public class NeedsDrawCoordTests : TestDriverBase +{ + /// + /// SetNeedsDraw() no-arg on a scrolled view stores a viewport-LOCAL rect — (0, 0, W, H), + /// NOT the current Viewport (which carries the scroll offset). + /// + [Fact] + public void SetNeedsDraw_NoArg_OnScrolledView_StoresViewportLocalRect () + { + IDriver driver = CreateTestDriver (40, 20); + + View view = new () { Driver = driver, X = 0, Y = 0, Width = 10, Height = 8 }; + view.SetContentSize (new Size (100, 100)); + view.BeginInit (); + view.EndInit (); + view.LayoutSubViews (); + view.Draw (); + + // Scroll so Viewport.Location is non-zero. + view.Viewport = view.Viewport with { Location = new Point (5, 3) }; + Assert.Equal (new Point (5, 3), view.Viewport.Location); + + view.ClearNeedsDraw (); + view.SetNeedsDraw (); + + // NeedsDrawRect must be viewport-LOCAL: (0, 0, W, H). + Assert.Equal (new Rectangle (0, 0, 10, 8), view.NeedsDrawRect); + + view.Dispose (); + driver.Dispose (); + } + + /// + /// SetNeedsDraw() no-arg on a view with a negative Viewport.Location (allowed via + /// AllowNegativeX/Y) must still produce a viewport-local rect — negative locations must + /// not leak into NeedsDrawRect. + /// + [Fact] + public void SetNeedsDraw_NoArg_OnNegativeViewportLocation_StoresViewportLocalRect () + { + IDriver driver = CreateTestDriver (40, 20); + + View view = new () + { + Driver = driver, + X = 0, + Y = 0, + Width = 10, + Height = 5, + ViewportSettings = ViewportSettingsFlags.AllowNegativeLocation + | ViewportSettingsFlags.AllowLocationGreaterThanContentSize + }; + view.SetContentSize (new Size (100, 100)); + view.BeginInit (); + view.EndInit (); + view.LayoutSubViews (); + view.Draw (); + + view.Viewport = view.Viewport with { Location = new Point (-3, -2) }; + Assert.Equal (new Point (-3, -2), view.Viewport.Location); + + view.ClearNeedsDraw (); + view.SetNeedsDraw (); + + Assert.Equal (new Rectangle (0, 0, 10, 5), view.NeedsDrawRect); + + view.Dispose (); + driver.Dispose (); + } + + /// + /// When the parent is scrolled and a content-area region is invalidated, the cascade + /// translates the parent's viewport-local region into the subview's viewport-local + /// coordinates correctly. + /// + /// Setup: parent (20×10), scrolled to (5, 0). Subview at Frame (3, 1, 10, 6) in + /// parent's CONTENT coords — that means in the parent's viewport-local frame the + /// subview's visible portion starts at viewport-local X = (3 - 5) = -2. Invalidate a + /// parent viewport-local rect of (1, 2, 5, 2). That maps to parent CONTENT + /// (6, 2, 5, 2), which overlaps the subview at content (6, 2, 5, 2) ∩ Frame + /// (3, 1, 10, 6) = (6, 2, 5, 2). In subview-frame-local: (3, 1, 5, 2). The subview + /// has no adornments and isn't scrolled, so subview-viewport-local equals + /// subview-frame-local. + /// + [Fact] + public void SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview () + { + IDriver driver = CreateTestDriver (40, 20); + + View parent = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + parent.SetContentSize (new Size (40, 20)); + + View subview = new () { X = 3, Y = 1, Width = 10, Height = 6 }; + parent.Add (subview); + + parent.BeginInit (); + parent.EndInit (); + parent.LayoutSubViews (); + parent.Draw (); + + // Scroll parent. + parent.Viewport = parent.Viewport with { Location = new Point (5, 0) }; + parent.Draw (); + + parent.ClearNeedsDraw (); + + // Invalidate a parent viewport-local 5×2 rect at viewport (1, 2). + parent.SetNeedsDraw (new Rectangle (1, 2, 5, 2)); + + Assert.Equal (new Rectangle (3, 1, 5, 2), subview.NeedsDrawRect); + + parent.Dispose (); + driver.Dispose (); + } + + /// + /// When a subview has padding (adornment), the cascade lands the dirty region in the + /// subview's viewport-local coords (inside the padding), NOT subview-frame-local. + /// + /// Setup: parent (20×10), subview at Frame (2, 2, 12, 6) with Padding.Thickness = 1. + /// Subview Viewport size = (10, 4). Invalidate parent viewport-local (3, 3, 10, 4). + /// That intersects subview.Frame at content (3, 3, 10, 4) ∩ (2, 2, 12, 6) + /// = (3, 3, 10, 4). Subview-frame-local: (1, 1, 10, 4). Subtract padding offset + /// (1, 1): (0, 0, 10, 4). Clip to subview viewport (0, 0, 10, 4): (0, 0, 10, 4). + /// + [Fact] + public void SetNeedsDraw_SubViewWithPadding_CascadesViewportLocalRectIntoSubviewViewport () + { + IDriver driver = CreateTestDriver (40, 20); + + View parent = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + + View subview = new () { X = 2, Y = 2, Width = 12, Height = 6 }; + subview.Padding.Thickness = new Thickness (1); + parent.Add (subview); + + parent.BeginInit (); + parent.EndInit (); + parent.LayoutSubViews (); + parent.Draw (); + + Assert.Equal (new Size (10, 4), subview.Viewport.Size); + + parent.ClearNeedsDraw (); + + parent.SetNeedsDraw (new Rectangle (3, 3, 10, 4)); + + // Subview's NeedsDrawRect is in subview-VIEWPORT-local — adornment offset is removed. + Assert.Equal (new Rectangle (0, 0, 10, 4), subview.NeedsDrawRect); + + parent.Dispose (); + driver.Dispose (); + } + + /// + /// When the subview itself is scrolled, the cascade translates into subview-viewport- + /// local correctly (subtracting subview.Viewport.Location on top of the frame offset). + /// + /// Setup: parent (20×10), subview Frame (0, 0, 10, 5) with content (100, 100) and + /// subview is scrolled to (4, 1). Parent invalidation at viewport-local (2, 2, 4, 2). + /// Subview-frame-local: (2, 2, 4, 2). Subtract subview's scroll (4, 1): + /// (-2, 1, 4, 2). Clip to subview viewport bounds (0, 0, 10, 5): (0, 1, 2, 2). + /// + [Fact] + public void SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAccountingForScroll () + { + IDriver driver = CreateTestDriver (40, 20); + + View parent = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + + View subview = new () { X = 0, Y = 0, Width = 10, Height = 5 }; + subview.SetContentSize (new Size (100, 100)); + parent.Add (subview); + + parent.BeginInit (); + parent.EndInit (); + parent.LayoutSubViews (); + parent.Draw (); + + subview.Viewport = subview.Viewport with { Location = new Point (4, 1) }; + parent.Draw (); + parent.ClearNeedsDraw (); + + parent.SetNeedsDraw (new Rectangle (2, 2, 4, 2)); + + Assert.Equal (new Rectangle (0, 1, 2, 2), subview.NeedsDrawRect); + + parent.Dispose (); + driver.Dispose (); + } + + /// + /// When the dirty region only overlaps the subview's adornment area (not its viewport), + /// the cascade falls back to a no-arg SetNeedsDraw on the subview — the subview's + /// NeedsDrawRect becomes the full viewport AND its adornments are flagged for redraw. + /// + /// Setup: subview Frame (5, 5, 10, 6) with Padding.Thickness = 1; visible viewport + /// is (0, 0, 8, 4) in subview-local. Invalidate parent (5, 5, 1, 1) — only the + /// top-left padding cell of the subview. Subview-frame-local: (0, 0, 1, 1). Minus + /// padding offset (1, 1): (-1, -1, 1, 1). Clip with viewport bounds (0, 0, 8, 4): + /// empty. Cascade falls back to full subview invalidation. + /// + [Fact] + public void SetNeedsDraw_DirtyRegionInSubviewAdornmentOnly_FallsBackToFullSubviewRedraw () + { + IDriver driver = CreateTestDriver (40, 20); + + View parent = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + + View subview = new () { X = 5, Y = 5, Width = 10, Height = 6 }; + subview.Padding.Thickness = new Thickness (1); + subview.Padding.GetOrCreateView (); + parent.Add (subview); + + parent.BeginInit (); + parent.EndInit (); + parent.LayoutSubViews (); + parent.Draw (); + + Assert.Equal (new Size (8, 4), subview.Viewport.Size); + + parent.ClearNeedsDraw (); + + parent.SetNeedsDraw (new Rectangle (5, 5, 1, 1)); + + // Fallback: subview's NeedsDrawRect ends up as the full viewport, and adornments + // are flagged for redraw. + Assert.Equal (new Rectangle (0, 0, 8, 4), subview.NeedsDrawRect); + Assert.True (subview.Padding.View?.NeedsDraw); + + parent.Dispose (); + driver.Dispose (); + } + + /// + /// Issue #5359 follow-up to PR #5431: with the coord-system normalized, the framework's + /// region-aware ClearViewport now narrows correctly on scrolled views. The temporary + /// "Viewport.Location == Point.Empty" guard from PR #5431 is removed. + /// + [Fact] + public void FrameworkNarrowing_NowWorks_OnScrolledView () + { + IDriver driver = CreateTestDriver (40, 20); + driver.Clip = new Region (driver.Screen); + + View view = new () { Driver = driver, X = 5, Y = 5, Width = 10, Height = 5 }; + view.SetContentSize (new Size (100, 100)); + view.BeginInit (); + view.EndInit (); + view.LayoutSubViews (); + view.Draw (); + + // Scroll so Viewport.Location != (0, 0). + view.Viewport = view.Viewport with { Y = 5 }; + Assert.Equal (new Point (0, 5), view.Viewport.Location); + + view.Draw (); + driver.Clip = new Region (driver.Screen); + driver.FillRect (driver.Screen, new Rune ('X')); + + // Partial dirty rect in viewport-local coords. + view.ClearNeedsDraw (); + view.SetNeedsDraw (new Rectangle (1, 1, 3, 2)); + + view.Draw (); + + // Only the 3×2 dirty region in viewport-local should be cleared. viewport-local (1, 1) + // → screen (5+1, 5+1) = (6, 6) (the view's screen position is (5, 5), no adornments). + for (var y = 6; y < 8; y++) + { + for (var x = 6; x < 9; x++) + { + Assert.Equal (" ", driver.Contents [y, x].Grapheme); + } + } + + // Cells outside the dirty region (but inside the viewport) keep their 'X'. + Assert.Equal ("X", driver.Contents [5, 5].Grapheme); + Assert.Equal ("X", driver.Contents [9, 14].Grapheme); + + view.Dispose (); + driver.Dispose (); + } + + /// + /// SetNeedsDraw() no-arg on a view with a zero-size viewport is a no-op (early-return + /// guards: NeedsDrawRect already set + viewport empty). + /// + [Fact] + public void SetNeedsDraw_ZeroSizeViewport_IsNoOp () + { + View view = new () { Width = 0, Height = 0 }; + view.BeginInit (); + view.EndInit (); + + view.SetNeedsDraw (); + + Assert.False (view.NeedsDraw); + Assert.Equal (Rectangle.Empty, view.NeedsDrawRect); + + view.Dispose (); + } + + /// + /// Repeated small invalidations on a scrolled view still produce a precise accumulating + /// union (AC1 from #5359 — already enabled by #5431's union fix, but worth pinning to + /// verify the new coord convention doesn't regress accumulation). + /// + [Fact] + public void SetNeedsDraw_RepeatedSmallInvalidationsOnScrolledView_AccumulateAsViewportLocalUnion () + { + IDriver driver = CreateTestDriver (40, 20); + + View view = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + view.SetContentSize (new Size (100, 100)); + view.BeginInit (); + view.EndInit (); + view.LayoutSubViews (); + view.Draw (); + + view.Viewport = view.Viewport with { Location = new Point (4, 2) }; + view.ClearNeedsDraw (); + + view.SetNeedsDraw (new Rectangle (1, 1, 3, 2)); + view.SetNeedsDraw (new Rectangle (10, 5, 4, 3)); + + // Union((1,1,3,2),(10,5,4,3)) = (1,1,13,7). + Assert.Equal (new Rectangle (1, 1, 13, 7), view.NeedsDrawRect); + + view.Dispose (); + driver.Dispose (); + } +} From c49d07507ad709f0effd84b4281e1554f500f8ce Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Wed, 27 May 2026 17:10:56 -0500 Subject: [PATCH 4/8] Address review: drop scroll subtraction in subview cascade, fix bad tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CoPilot review of PR #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) --- Terminal.Gui/ViewBase/View.NeedsDraw.cs | 21 +++--- .../ViewBase/Draw/NeedsDrawCoordTests.cs | 69 +++++++++++++++++-- .../Draw/RegionAwareClearViewportTests.cs | 51 ++------------ 3 files changed, 79 insertions(+), 62 deletions(-) diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index bf91bbdf2d..b0afd0473a 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -135,11 +135,14 @@ public void SetNeedsDraw (Rectangle viewPortRelativeRegion) // this view's content coords by adding Viewport.Location (the scroll offset). // 2. subview.Frame is in this view's content coords; intersect there. // 3. Translate the intersection to subview-frame-local (subtract Frame.Location). - // 4. Translate to subview-VIEWPORT-local by also subtracting the subview's adornment - // offset (Padding inset) and the subview's own Viewport.Location. + // 4. Translate to subview-VIEWPORT-local by subtracting the subview's adornment + // offset (Padding inset). Do NOT also subtract the subview's Viewport.Location: + // viewport-local coords are scroll-INDEPENDENT — (0, 0) is always the top-left + // visible cell regardless of which content cell the scroll maps there. The dirty + // ON-SCREEN cells are what we propagate, not the content cells they're showing. // 5. Clip to the subview's visible viewport bounds; anything outside is in adornment - // or scrolled-off territory. If nothing is left, fall back to a full subview - // invalidation so the subview at least redraws itself safely. + // territory. If nothing is left, fall back to a full subview invalidation so the + // subview at least redraws itself safely (and flags its adornments). Point thisScroll = Viewport.Location; Rectangle contentRegion = viewPortRelativeRegion; contentRegion.Offset (thisScroll.X, thisScroll.Y); @@ -155,18 +158,16 @@ public void SetNeedsDraw (Rectangle viewPortRelativeRegion) subviewFrameRegion.Offset (-subview.Frame.X, -subview.Frame.Y); Point subviewAdornmentOffset = subview.GetViewportOffsetFromFrame (); - Point subviewScroll = subview.Viewport.Location; - subviewFrameRegion.Offset (-subviewAdornmentOffset.X - subviewScroll.X, - -subviewAdornmentOffset.Y - subviewScroll.Y); + subviewFrameRegion.Offset (-subviewAdornmentOffset.X, -subviewAdornmentOffset.Y); Rectangle subviewViewportBounds = new (Point.Empty, subview.Viewport.Size); Rectangle subviewViewportRegion = Rectangle.Intersect (subviewViewportBounds, subviewFrameRegion); if (subviewViewportRegion.IsEmpty) { - // Dirty region overlaps the subview's frame but only its adornment/scrolled-off - // area. The subview's own viewport isn't dirty; a no-arg SetNeedsDraw is the - // safe fallback (also flags adornments so any border/padding the dirty region + // Dirty region overlaps the subview's frame but only its adornment area. The + // subview's own viewport isn't dirty; a no-arg SetNeedsDraw is the safe + // fallback (also flags adornments so any border/padding the dirty region // touched gets repainted). subview.SetNeedsDraw (); diff --git a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs index b43d708be7..cf4334d4f3 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs @@ -174,16 +174,19 @@ public void SetNeedsDraw_SubViewWithPadding_CascadesViewportLocalRectIntoSubview } /// - /// When the subview itself is scrolled, the cascade translates into subview-viewport- - /// local correctly (subtracting subview.Viewport.Location on top of the frame offset). + /// When the subview itself is scrolled, the cascade lands the dirty rect at the + /// correct VIEWPORT-LOCAL position — viewport-local coords are scroll-INDEPENDENT + /// ((0, 0) is always the top-left visible cell regardless of which content cell the + /// scroll maps there). We propagate the dirty ON-SCREEN cells, not the content cells + /// they happen to be showing. /// /// Setup: parent (20×10), subview Frame (0, 0, 10, 5) with content (100, 100) and /// subview is scrolled to (4, 1). Parent invalidation at viewport-local (2, 2, 4, 2). - /// Subview-frame-local: (2, 2, 4, 2). Subtract subview's scroll (4, 1): - /// (-2, 1, 4, 2). Clip to subview viewport bounds (0, 0, 10, 5): (0, 1, 2, 2). + /// Subview-frame-local: (2, 2, 4, 2). No adornment offset, no scroll subtraction: + /// subview-viewport-local: (2, 2, 4, 2). Clip to (0, 0, 10, 5): (2, 2, 4, 2). /// [Fact] - public void SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAccountingForScroll () + public void SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAtCorrectOnScreenPosition () { IDriver driver = CreateTestDriver (40, 20); @@ -204,7 +207,7 @@ public void SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAccountingFo parent.SetNeedsDraw (new Rectangle (2, 2, 4, 2)); - Assert.Equal (new Rectangle (0, 1, 2, 2), subview.NeedsDrawRect); + Assert.Equal (new Rectangle (2, 2, 4, 2), subview.NeedsDrawRect); parent.Dispose (); driver.Dispose (); @@ -322,6 +325,60 @@ public void SetNeedsDraw_ZeroSizeViewport_IsNoOp () view.Dispose (); } + /// + /// When a subview's Frame changes and the SuperView is itself scrolled, SetFrame's + /// SuperView invalidation must translate the union(old, new) rect from SuperView + /// CONTENT coords to SuperView VIEWPORT-LOCAL coords. Without the translation, the + /// invalidation lands at the wrong on-screen position. + /// + /// Setup: superView (20×10) scrolled by (5, 3). subview at Frame (10, 8, 7, 4) in + /// superView's content coords. Shrink subview Frame to (10, 8, 3, 2). SetFrame + /// invalidates with union(old, new) = (10, 8, 7, 4) (content coords). Translation + /// to superView viewport-local: subtract (5, 3) → (5, 5, 7, 4). superView's + /// NeedsDrawRect must contain this rect (it may be larger due to union with previously + /// accumulated dirty regions, but must contain (5, 5, 7, 4)). + /// + [Fact] + public void SetFrame_ScrolledSuperView_TranslatesInvalidationToViewportLocal () + { + IDriver driver = CreateTestDriver (40, 20); + + View superView = new () { Driver = driver, X = 0, Y = 0, Width = 20, Height = 10 }; + superView.SetContentSize (new Size (100, 100)); + + View subview = new () { X = 10, Y = 8, Width = 7, Height = 4 }; + superView.Add (subview); + + superView.BeginInit (); + superView.EndInit (); + superView.LayoutSubViews (); + superView.Draw (); + + superView.Viewport = superView.Viewport with { Location = new Point (5, 3) }; + Assert.Equal (new Point (5, 3), superView.Viewport.Location); + + superView.Draw (); + superView.ClearNeedsDraw (); + Assert.Equal (Rectangle.Empty, superView.NeedsDrawRect); + + // Shrink subview's Frame. SetFrame invalidates SuperView with union(old, new) = + // (10, 8, 7, 4) in superView CONTENT coords; the translation subtracts the scroll + // (5, 3) to produce viewport-local (5, 5, 7, 4). + subview.Frame = new Rectangle (10, 8, 3, 2); + + Assert.True (superView.NeedsDrawRect.Contains (new Rectangle (5, 5, 7, 4)), + $"Expected superView.NeedsDrawRect to contain viewport-local (5, 5, 7, 4); got {superView.NeedsDrawRect}."); + + // Sanity: the rect must NOT have left the scroll offset baked in. If translation + // were skipped, NeedsDrawRect would contain content-coord (10, 8, ...) which has + // X >= 10. + Assert.True (superView.NeedsDrawRect.X < 10, + $"NeedsDrawRect.X = {superView.NeedsDrawRect.X}; if scroll wasn't subtracted it would be >= 10 (content-coord X of the subview)."); + + superView.Dispose (); + driver.Dispose (); + } + /// /// Repeated small invalidations on a scrolled view still produce a precise accumulating /// union (AC1 from #5359 — already enabled by #5431's union fix, but worth pinning to diff --git a/Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs b/Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs index 78b68f4576..e34b30346a 100644 --- a/Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs +++ b/Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs @@ -147,52 +147,11 @@ public void FrameworkDraw_FullViewportNeedsDrawRect_ClearsFullViewport () driver.Dispose (); } - /// - /// Review feedback item 1: narrowing must NOT fire when the view is itself scrolled, - /// because SetNeedsDraw(Rectangle) cascades to subviews using frame-local coordinates - /// while the no-arg version passes content-coord Viewport. Until that convention is - /// normalized, the framework falls back to a full clear for scrolled views. - /// - [Fact] - public void FrameworkDraw_ScrolledView_FallsBackToFullClear () - { - IDriver driver = CreateTestDriver (40, 20); - driver.Clip = new Region (driver.Screen); - - View view = new () { Driver = driver, X = 5, Y = 5, Width = 10, Height = 5 }; - view.SetContentSize (new Size (100, 100)); - view.BeginInit (); - view.EndInit (); - view.LayoutSubViews (); - view.Draw (); - - // Scroll so Viewport.Location is non-empty. - view.Viewport = view.Viewport with { Y = 5 }; - Assert.Equal (new Point (0, 5), view.Viewport.Location); - - view.Draw (); - driver.Clip = new Region (driver.Screen); - driver.FillRect (driver.Screen, new Rune ('X')); - - // Set a partial dirty rect (which would narrow if the view weren't scrolled). - view.ClearNeedsDraw (); - view.SetNeedsDraw (new Rectangle (1, 6, 3, 2)); - - view.Draw (); - - // Full viewport should be cleared (narrowing must not fire on scrolled view). - Rectangle screen = view.ViewportToScreen (view.Viewport with { Location = new Point (0, 0) }); - for (int y = screen.Y; y < screen.Y + screen.Height; y++) - { - for (int x = screen.X; x < screen.X + screen.Width; x++) - { - Assert.Equal (" ", driver.Contents [y, x].Grapheme); - } - } - - view.Dispose (); - driver.Dispose (); - } + // Note: a prior FrameworkDraw_ScrolledView_FallsBackToFullClear test was removed in #5359. + // It documented the temporary "Viewport.Location == Point.Empty" guard added in #5431 as a + // workaround for the cascade coord-system inconsistency. Issue #5359 normalized the + // convention and removed that guard. Narrowing on scrolled views is now correct and is + // covered by NeedsDrawCoordTests.FrameworkNarrowing_NowWorks_OnScrolledView. /// /// When a subview's Frame shrinks, View.Layout invalidates the SuperView for the union From 491f1904c0251b848e2d4f411b084feba7afddea Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Thu, 28 May 2026 11:28:50 -0500 Subject: [PATCH 5/8] Updated comment from code review feedback Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- Terminal.Gui/ViewBase/View.NeedsDraw.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index b0afd0473a..7ce763c390 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -62,9 +62,11 @@ public void SetNeedsDraw () /// (scroll offset). /// /// + /// /// The cascade to intersecting SubViews translates the region into each SubView's - /// own viewport-local coordinates, accounting for the parent's scroll, the SubView's - /// adornments, and the SubView's own scroll. + /// own viewport-local coordinates, accounting for the parent's scroll and the SubView's + /// adornments. Viewport-local coordinates are scroll-independent, so the SubView's own + /// does not affect the propagated rect. /// /// /// If the view has not been initialized ( is ), the area to be From 5cec61ee2eb396aa35dc4ac9b89fbe5ce9cf0181 Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Thu, 28 May 2026 11:38:34 -0500 Subject: [PATCH 6/8] Address review: clarify cascade adornment-offset comment, fix plan doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- Terminal.Gui/ViewBase/View.NeedsDraw.cs | 3 ++- plans/5359-needsdraw-coord-normalization.md | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index 7ce763c390..abce8f40ce 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -138,7 +138,8 @@ public void SetNeedsDraw (Rectangle viewPortRelativeRegion) // 2. subview.Frame is in this view's content coords; intersect there. // 3. Translate the intersection to subview-frame-local (subtract Frame.Location). // 4. Translate to subview-VIEWPORT-local by subtracting the subview's adornment - // offset (Padding inset). Do NOT also subtract the subview's Viewport.Location: + // offset (GetViewportOffsetFromFrame() — the combined Margin/Border/Padding inset). + // Do NOT also subtract the subview's Viewport.Location: // viewport-local coords are scroll-INDEPENDENT — (0, 0) is always the top-left // visible cell regardless of which content cell the scroll maps there. The dirty // ON-SCREEN cells are what we propagate, not the content cells they're showing. diff --git a/plans/5359-needsdraw-coord-normalization.md b/plans/5359-needsdraw-coord-normalization.md index 94c783d7df..a487f82de9 100644 --- a/plans/5359-needsdraw-coord-normalization.md +++ b/plans/5359-needsdraw-coord-normalization.md @@ -35,8 +35,8 @@ This matches what `ViewportToScreen(in Rectangle)` already expects and is the me - Translate `viewPortRelativeRegion` → `this`'s content coords by adding `Viewport.Location`. - Intersect with `subview.Frame` (already in `this`'s content coords). - Translate the intersection to subview-frame-local by subtracting `subview.Frame.Location`. - - Translate to subview-viewport-local by subtracting `subview.GetViewportOffsetFromFrame()` and `subview.Viewport.Location`. - - Clip to the subview's viewport bounds (`new Rectangle(Point.Empty, subview.Viewport.Size)`); if empty, fall back to `subview.SetNeedsDraw()` no-arg (the subview still needs *some* redraw since the parent invalidation touched its frame area, but it's all in adornment / scrolled-off territory — let it do a safe full redraw). + - Translate to subview-viewport-local by subtracting `subview.GetViewportOffsetFromFrame()` (the combined Margin/Border/Padding inset). Do **not** subtract `subview.Viewport.Location`: viewport-local coordinates are scroll-independent — `(0, 0)` is always the top-left visible cell regardless of which content cell the subview's scroll maps there. We propagate the dirty *on-screen* cells, not the content cells they're showing. + - Clip to the subview's viewport bounds (`new Rectangle(Point.Empty, subview.Viewport.Size)`); if empty, fall back to `subview.SetNeedsDraw()` no-arg (the subview still needs *some* redraw since the parent invalidation touched its frame area, but it's all in adornment territory — let it do a safe full redraw and flag its adornments). ### 2. `View.Drawing.Adornments.cs` From e982dde1f5cb9441d806890dc34ed979f72dccd6 Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Thu, 28 May 2026 11:40:01 -0500 Subject: [PATCH 7/8] Fix malformed duplicate in SetNeedsDraw XML doc The Copilot autofix in 491f1904c opened 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) --- Terminal.Gui/ViewBase/View.NeedsDraw.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Terminal.Gui/ViewBase/View.NeedsDraw.cs b/Terminal.Gui/ViewBase/View.NeedsDraw.cs index abce8f40ce..993da4da9a 100644 --- a/Terminal.Gui/ViewBase/View.NeedsDraw.cs +++ b/Terminal.Gui/ViewBase/View.NeedsDraw.cs @@ -62,7 +62,6 @@ public void SetNeedsDraw () /// (scroll offset). /// /// - /// /// The cascade to intersecting SubViews translates the region into each SubView's /// own viewport-local coordinates, accounting for the parent's scroll and the SubView's /// adornments. Viewport-local coordinates are scroll-independent, so the SubView's own From 907beac0456e8ee64b0f8b816c8d593949d6a8e8 Mon Sep 17 00:00:00 2001 From: Kevin Harder Date: Fri, 29 May 2026 11:17:12 -0500 Subject: [PATCH 8/8] removed plan file --- plans/5359-needsdraw-coord-normalization.md | 91 --------------------- 1 file changed, 91 deletions(-) delete mode 100644 plans/5359-needsdraw-coord-normalization.md diff --git a/plans/5359-needsdraw-coord-normalization.md b/plans/5359-needsdraw-coord-normalization.md deleted file mode 100644 index a487f82de9..0000000000 --- a/plans/5359-needsdraw-coord-normalization.md +++ /dev/null @@ -1,91 +0,0 @@ -# Issue #5359 — `NeedsDrawRect` Coordinate-System Normalization - -**Branch:** `fix-5359-needsdraw-coord-normalization` (based on `fix-5358-stop-child-redraws-escalating`, which will be retargeted to `develop` after #5431 merges) - -**Parent issue:** [#4973](https://github.com/gui-cs/Terminal.Gui/issues/4973) -**Sibling PR:** [#5431](https://github.com/gui-cs/Terminal.Gui/pull/5431) — covers AC1 (union math) -**This PR covers:** AC2 (subview cascade) and AC3 (viewport movement / negative locations). - ---- - -## Background - -`NeedsDrawRect` is currently treated inconsistently across the codebase: - -- `SetNeedsDraw()` no-arg passes the *current* `Viewport` rectangle (i.e., `Viewport.Location` is the scroll offset; can be non-zero or negative). That stores content-coord rects in `NeedsDrawRect`. -- `DoDrawAdornments` escalation does the same: `NeedsDrawRect = Viewport`. -- The subview cascade in `SetNeedsDraw(Rectangle)` intersects `subview.Frame` (in `this`'s content coords) directly with `viewPortRelativeRegion`. Only works when `this.Viewport.Location == (0, 0)`. -- `CanNarrowClearToNeedsDrawRect` (added in #5431) consumes `NeedsDrawRect` via `ViewportToScreen`, which expects **viewport-local** coords (`(0, 0)` = top-left visible cell). That's why #5431 had to add a `Viewport.Location == Point.Empty` guard. - -The deferred follow-up called out in PR #5431's description is exactly this: *normalize the convention*. - -## Convention - -> **`NeedsDrawRect` and the `viewPortRelativeRegion` parameter of `SetNeedsDraw(Rectangle)` are in viewport-LOCAL coordinates: `(0, 0)` = top-left visible cell of the View's Viewport (inside `Padding`, after any scroll).** - -This matches what `ViewportToScreen(in Rectangle)` already expects and is the meaning documented for `ViewportToScreen` itself. - -## Changes - -### 1. `View.NeedsDraw.cs` - -- Update the XML doc on `NeedsDrawRect`, `SetNeedsDraw()`, and `SetNeedsDraw(Rectangle)` to explicitly state "viewport-local; `(0, 0)` = top-left visible cell." -- Fix `SetNeedsDraw()` no-arg to pass `new Rectangle(Point.Empty, Viewport.Size)` — never propagate the scroll offset into the dirty rect. -- Fix the subview cascade: - - Translate `viewPortRelativeRegion` → `this`'s content coords by adding `Viewport.Location`. - - Intersect with `subview.Frame` (already in `this`'s content coords). - - Translate the intersection to subview-frame-local by subtracting `subview.Frame.Location`. - - Translate to subview-viewport-local by subtracting `subview.GetViewportOffsetFromFrame()` (the combined Margin/Border/Padding inset). Do **not** subtract `subview.Viewport.Location`: viewport-local coordinates are scroll-independent — `(0, 0)` is always the top-left visible cell regardless of which content cell the subview's scroll maps there. We propagate the dirty *on-screen* cells, not the content cells they're showing. - - Clip to the subview's viewport bounds (`new Rectangle(Point.Empty, subview.Viewport.Size)`); if empty, fall back to `subview.SetNeedsDraw()` no-arg (the subview still needs *some* redraw since the parent invalidation touched its frame area, but it's all in adornment territory — let it do a safe full redraw and flag its adornments). - -### 2. `View.Drawing.Adornments.cs` - -- `DoDrawAdornments` escalation: `NeedsDrawRect = new Rectangle(Point.Empty, Viewport.Size)` instead of `Viewport`. - -### 3. `View.Layout.cs` - -- `SetFrame`'s `SuperView.SetNeedsDraw(Rectangle.Union(prev, frame))` passes a rect in SuperView's content coords. Translate to SuperView's viewport-local by subtracting `SuperView.Viewport.Location` before passing. - -### 4. `View.Drawing.cs` - -- `CanNarrowClearToNeedsDrawRect`: remove the `Viewport.Location != Point.Empty` early-return. Update the comment block to reflect that narrowing is now safe regardless of scroll because `NeedsDrawRect` is consistently viewport-local. - -### 5. Tests - -- `Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs`: - - `NeedsDrawRect_Is_Viewport_Relative` — update the scrolled-frame assertions. The post-scroll `view.Frame = (3, 3, 6, 6)` step previously asserted `NeedsDrawRect = (0, 0, 7, 7)` because the broken `SetNeedsDraw` passed `Viewport = (1, 1, 6, 6)`. With the fix it passes `(0, 0, 6, 6)`, and accumulating union with prior `(0, 0, 6, 6)` stays `(0, 0, 6, 6)`. - -- New file `Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawCoordTests.cs`: - - `SetNeedsDraw_NoArg_OnScrolledView_StoresViewportLocalRect` — Viewport at `(5, 3, 10, 8)`; `SetNeedsDraw()` → `NeedsDrawRect == (0, 0, 10, 8)`. - - `SetNeedsDraw_NoArg_OnNegativeViewportLocation_StoresViewportLocalRect` — `AllowNegativeX`, `Viewport.X = -3`; `SetNeedsDraw()` → `NeedsDrawRect == (0, 0, W, H)`. - - `SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview` — parent scrolled by `(5, 0)`; invalidate content-area rect that overlaps a subview; subview's `NeedsDrawRect` matches the expected viewport-local slice. - - `SetNeedsDraw_SubViewWithPadding_CascadesViewportLocalRectIntoSubviewViewport` — subview has `Padding.Thickness = (1)`; parent invalidation that covers subview's full frame should land as `(0, 0, viewport.W, viewport.H)` in subview-viewport-local, not as the frame-local rect. - - `SetNeedsDraw_ScrolledSubview_CascadesIntoSubviewViewportAccountingForScroll` — subview is itself scrolled; cascade produces correct viewport-local rect. - - `SetNeedsDraw_DirtyRegionInSubviewAdornmentOnly_FallsBackToFullSubviewRedraw` — invalidation overlaps only subview's padding/border; subview's `NeedsDrawRect` ends up as full viewport (safe fallback) and the subview's adornments are flagged. - - `FrameworkNarrowing_NowWorks_OnScrolledView` — after removing the guard, a scrolled view with a partial `NeedsDrawRect` narrows the clear correctly. - - `SetNeedsDraw_ZeroSizeViewport_IsNoOp` — edge case. - -## Acceptance Criteria Mapping - -| AC | Coverage | -|----|----------| -| AC1 (repeated small invalidations → correct union) | Already in PR #5431 (`Rectangle.Union`). Existing test `SetNeedsDraw_MultipleRectangles_Expands` covers it. | -| AC2 (parent → subview translation correct) | Fixed cascade + new tests `SetNeedsDraw_ScrolledParent_CascadesViewportLocalRectToSubview`, `…_SubViewWithPadding_…`, `…_ScrolledSubview_…`, `…_DirtyRegionInSubviewAdornmentOnly_…`. | -| AC3 (viewport movement / negative locations) | `SetNeedsDraw()` no-arg fix + new tests `…_OnScrolledView_…`, `…_OnNegativeViewportLocation_…`. | - -## Out of Scope (per issue's "Non-Functional Requirements" / "do not expand") - -- Region-based dirty rect (stay with `Rectangle`). -- Per-cell partial rendering / clipping at the IOutput level. -- Removing the `NeedsDrawRect = Viewport.Size` escalation in `DoDrawAdornments` entirely (that's tied to the `LayoutAndDraw force=true` fan-out tracked separately in #5434). - -## Verification - -```bash -dotnet build --no-restore -dotnet test --project Tests/UnitTestsParallelizable --no-build -dotnet test --project Tests/UnitTests.NonParallelizable --no-build -dotnet test --project Tests/IntegrationTests --no-build -``` - -Expect: all green; no new warnings; existing test count + new tests added.