Skip to content

Fixes #5434. Track adornment thickness deltas so LayoutAndDraw can stop force-drawing#5464

Open
harder wants to merge 2 commits into
gui-cs:developfrom
harder:copilot/issue-5434-adornment-layout-deltas
Open

Fixes #5434. Track adornment thickness deltas so LayoutAndDraw can stop force-drawing#5464
harder wants to merge 2 commits into
gui-cs:developfrom
harder:copilot/issue-5434-adornment-layout-deltas

Conversation

@harder
Copy link
Copy Markdown
Collaborator

@harder harder commented Jun 1, 2026

Summary

Follow-up to #5431 (#5358). ApplicationImpl.LayoutAndDraw previously force-redrew the
entire runnable tree whenever any view needed layout (force = neededLayout || forceRedraw),
cascading SetNeedsDraw to all overlapping subviews. That blanket force existed only because
adornment thickness changes alter rendered coverage without changing the View's Frame, so
nothing invalidated the affected region precisely.

This PR tracks adornment thickness deltas and invalidates the precise SuperView region on
change, allowing the force to be dropped (force = forceRedraw).

Changes

  • ApplicationImpl.Screen.csLayoutAndDraw no longer forces a full redraw just because
    layout ran. It now forces only when explicitly requested (forceRedraw).
  • View.Adornments.cs — the Margin/Border/Padding ThicknessChanged handlers now route
    through HandleAdornmentThicknessChanged, which:
    • tracks each adornment's previous thickness,
    • performs the existing layout/draw invalidation,
    • precisely invalidates the SuperView region the view occupies when the adornment's inner frame
      changed (InvalidateAdornmentThicknessChange), mirroring the existing SetFrame invalidation,
      and falling back to a full-screen clear for top-level (no SuperView) views, and
    • raises ViewportChanged when the viewport actually changed.
  • TabsFanOutIntegrationTests — flipped from documenting residual fan-out
    (Assert.True(inactiveTextDraws > 0, ...)) to asserting zero fan-out
    (Assert.Equal(0, inactiveTextDraws)).

This completes a consistent pattern: every mutation that changes rendered coverage without a
redraw request now self-invalidates the SuperView — Frame move/shrink (SetFrame), Visible
toggle (Visible setter), and now adornment thickness (this PR).

⚠️ Behavioral change to note

ViewportChanged now fires when an adornment's Thickness changes the viewport size. Previously,
setting Border/Margin/Padding.Thickness directly changed Viewport.Size silently; it now
raises ViewportChanged, consistent with SetFrame (which already raises it on any viewport
change). The cursor-adjustment branch in RaiseViewportChangedEvent is a no-op for this case
because the viewport location is unchanged (delta == 0), so there are no cursor jumps. Downstream
ViewportChanged subscribers will now be notified on thickness-driven size changes. This is
considered a latent-bug fix, but is called out explicitly for reviewer sign-off.

Acceptance criteria

  • Layout detects adornment-thickness changes affecting rendered coverage without frame changes
  • Adornment changes invalidate the precise SuperView region
  • LayoutAndDraw no longer force-redraws solely because layout ran
  • No stale-content regressions in adornment-rebalance paths
  • Integration test asserts zero fan-out (was: documents fan-out)
  • No regressions for overlapping views, shadows, transparent content, or clipping

Testing

  • Build clean, 0 warnings.
  • TabsFanOutIntegrationTests (3) and TabsFanOutDiagnosticTests pass with zero-fan-out assertions.
  • Regression-risk suites green: ShadowTests (35), BorderViewTests (78), AdornmentTests (116),
    BorderTests (9), PaddingTests (8), MarginTests (17), ViewportTests (143).
  • Full UnitTestsParallelizable (17,300) and UnitTests.NonParallelizable (72, +2 skipped) pass.

🤖 Generated with Claude Code

harder and others added 2 commits June 1, 2026 11:41
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix indentation of the Margin.ThicknessChanged handler body and factor
the duplicated viewport-size formula out of
GetViewportForAdornmentThickness and GetViewportFrameForAdornmentThickness
into a shared GetViewportSizeForThickness helper.

No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@harder harder requested a review from tig as a code owner June 1, 2026 22:51
@harder harder requested a review from Copilot June 1, 2026 22:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Follow-up to #5431 that removes the blanket force=true redraw in ApplicationImpl.LayoutAndDraw when layout ran. To make that safe, adornment ThicknessChanged handlers now track the previous thickness so that when an adornment's inner frame changes (without the View's outer Frame changing), the SuperView region is invalidated precisely (mirroring SetFrame's pattern), and ViewportChanged is raised when the viewport actually changed.

Changes:

  • LayoutAndDraw now passes only forceRedraw to View.Draw, dropping neededLayout from the force condition.
  • Margin/Border/Padding ThicknessChanged handlers are routed through a new HandleAdornmentThicknessChanged local that captures pre/post viewport state, calls InvalidateAdornmentThicknessChange on the SuperView, and raises ViewportChanged.
  • TabsFanOutIntegrationTests flipped from documenting fan-out to asserting inactiveTextDraws == 0.

Reviewed changes

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

File Description
Terminal.Gui/App/ApplicationImpl.Screen.cs Drops neededLayout from force arg to View.Draw.
Terminal.Gui/ViewBase/View.Adornments.cs Tracks per-adornment previous thickness, invalidates SuperView precisely on viewport-frame change, raises ViewportChanged.
Tests/IntegrationTests/TabsFanOutIntegrationTests.cs Asserts zero inactive-tab fan-out and updates the doc comment.

Copy link
Copy Markdown
Member

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean and well-scoped. The approach is correct: track previous adornment thickness in closures, compare old vs new viewport frame on change, and invalidate precisely at the SuperView instead of force-drawing the entire tree. Removing neededLayout from the force condition in LayoutAndDraw is the payoff, and the integration test flipping from "documents remaining fan-out" to Assert.Equal(0, inactiveTextDraws) proves it works.

The closure pattern (local function returning the new thickness, reassigned to the captured variable) is a bit unusual but valid and keeps the event wiring concise. The null-SuperView fallback to NeedsClearScreenNextIteration() handles the top-level case correctly.

I did not have time to do a bunch of user testing. If you feel like you've played hard with these Scenarios I'm good: Adornments, Shadow Styles, Menus, Arrangements...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants