Skip to content

Fixes #5499. Skip SetNeedsLayout when Dim.Auto text recompute yields unchanged size#5549

Merged
harder merged 4 commits into
tui-cs:developfrom
harder:copilot/skip-setneedslayout-when-dim-auto-unchanged
Jun 28, 2026
Merged

Fixes #5499. Skip SetNeedsLayout when Dim.Auto text recompute yields unchanged size#5549
harder merged 4 commits into
tui-cs:developfrom
harder:copilot/skip-setneedslayout-when-dim-auto-unchanged

Conversation

@harder

@harder harder commented Jun 27, 2026

Copy link
Copy Markdown
Member

Fixes #5499.

Problem

The View.Text setter unconditionally called SetNeedsLayout(). That marks the view and its whole subtree as needing layout, sets _needsDrawAfterLayout, and propagates up the entire ancestor chain on every text change, even when the recomputed frame is identical to the current one (e.g. a once-per-second clock Label).

The layout machinery already no-ops the frame mutation when nothing changed (SetRelativeLayout only calls SetFrame when Frame != newFrame), but the layout pass, formatter setup, redraw scheduling, and ancestor invalidation were still being scheduled every tick.

Change

Terminal.Gui/ViewBase/View.Layout.cs

Extracted SetRelativeLayout's Pos/Dim -> absolute-Frame resolution into a private TryComputeRelativeFrame (Size, out Rectangle). SetRelativeLayout now calls it, and so does the Text setter's speculative check. This keeps frame prediction and real layout on the same computation path.

Also extracted SetRelativeLayout's post-frame TextFormatter constraint cleanup into FinalizeTextFormatterConstraints(). This matters because UpdateTextFormatterText() clears ConstrainToWidth/ConstrainToHeight; if the text setter skips layout, the fast path must still restore the same formatter constraints a full layout pass would produce so wrapped/clipped text formats correctly.

Terminal.Gui/ViewBase/View.Text.cs

The setter now calls TryRedrawWithoutLayout() after updating TextFormatter.Text.

When the view has already been laid out and both Width and Height are eligible, TryRedrawWithoutLayout() resolves the would-be Frame via TryComputeRelativeFrame. If the predicted frame equals the current Frame, it skips SetNeedsLayout() and ancestor propagation, then:

  • finalizes TextFormatter constraints with FinalizeTextFormatterConstraints();
  • sets TextFormatter.NeedsFormat = true;
  • calls SetNeedsDraw();
  • lets the setter raise TextChanged normally.

The comparison is the full Rectangle (position and size), not just dimensions. A Pos can depend on Text (e.g. Pos.Func reading view.Text), so a same-size text change can still move the view and must run layout.

Eligibility is exact by design:

static bool IsEagerlyResolvable (Dim dim) => dim is DimAbsolute or DimAuto { Style: DimAutoStyle.Text };
  • DimAbsolute is constant; a Text-only DimAuto depends on this view's TextFormatter.
  • DimAutoStyle.Content / DimAutoStyle.Auto can lay out subviews while calculating, so those fall back to SetNeedsLayout().
  • Composite/nested dims such as Dim.Auto(Text) + 2 are not bare DimAbsolute/DimAuto, so they also fall back.

This covers the intended safe cases: both-axis Text-auto, fixed-size text redraws, and one-axis-auto (Width = Auto(Text), Height = 1). The optimization only avoids setting NeedsLayout; it never clears a pre-existing pending layout request.

Tests

Added focused layout and render coverage:

  • Tests/UnitTestsParallelizable/ViewBase/Layout/Dim.AutoTests.TextOptimization.cs

    • ancestor diagnostic: same-size text change leaves superView.NeedsLayout == false while the view still redraws;
    • full-frame guard: Pos.Func depending on Text forces layout when same-size text moves the view;
    • exact guard: composite dims bypass the fast path;
    • fixed-size and one-axis-auto fast paths;
    • formatter constraint survival for fixed-size and one-axis-auto word-wrap cases;
    • clock-style repeated same-width updates with no frame drift or ancestor propagation;
    • reentrant TextChanged handling;
    • min/max, border adornment, uninitialized, DimAutoStyle.Auto, different-size, and pending-layout cases.
  • Tests/UnitTestsParallelizable/ViewBase/Layout/TextFastPathRenderTests.cs

    • compares the isolated redraw-only fast path against a forced full-layout control;
    • verifies rendered driver contents match for fixed-size wrap, fixed-size clip, single-row clipping, and max-constrained Text-auto wrapping.

Additional exploratory probes were run and removed after conversion of the high-value cases into permanent tests. They compared fast-path vs full-layout behavior across repeated text updates, fixed/min/max/Text-auto dimensions, word-wrap on/off, explicit content-size constraints, Button, CheckBox, and reentrant handlers.

Verification

Current local verification on ae2047abec598989a10bea28d858e01e6b6b2f5f:

  • Build Tests/UnitTestsParallelizable: passed
  • DimAutoTests text-focused tests: 67 passed
  • TextFastPathRenderTests: 5 passed
  • SetRelativeLayoutTests: 10 passed
  • Full UnitTestsParallelizable: 17,484 total / 0 failed / 17 skipped
  • Full UnitTests.NonParallelizable: 74 total / 0 failed / 2 skipped

Pull Request checklist

  • My code follows the style guidelines of Terminal.Gui
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

🤖 Generated with Claude Code

Copilot AI and others added 4 commits June 27, 2026 19:21
…ields unchanged size

The View.Text setter unconditionally called SetNeedsLayout(), which propagates
layout work up the entire ancestor chain and forces a redraw on every text
change - even when the recomputed Dim.Auto(DimAutoStyle.Text) size is identical
to the current Frame (e.g. a once-per-second clock Label).

When both Width and Height are exactly DimAutoStyle.Text and the view has been
laid out, the setter now predicts the new Frame size by invoking the exact same
Dim.Calculate path the layout engine uses (no reimplementation, so it cannot
disagree with a real layout pass). If the size is unchanged, it skips
SetNeedsLayout() and the ancestor propagation, marks TextFormatter.NeedsFormat
and calls SetNeedsDraw() so the new content is still reformatted and redrawn.

The optimization only ever avoids *setting* NeedsLayout; it never clears it, so
a pre-existing pending layout request survives. DimAutoStyle.Auto, fixed dims,
and not-yet-laid-out views all fall back to the original SetNeedsLayout() path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fixed/one-axis

Code review of tui-cs#5549 found two correctness gaps and one scope limitation in the
Text-change layout-skip optimization:

1. Position changes were ignored. The predictor compared only size to Frame, but
   a Pos can depend on Text (e.g. Pos.Func reading view.Text), so a same-size
   text change could still require a new Frame.X/Y and was wrongly skipped.

2. The "exactly Text auto" guard used dim.Has(...), which matches nested/composite
   dimensions (e.g. Dim.Auto(Text) + 2), letting location- or reference-sensitive
   composites into the simplified fast path.

3. The optimization was narrower than tui-cs#5499 intended - it only handled both axes
   being DimAutoStyle.Text and forced layout for fixed-size and one-axis-auto text
   changes that are safe to skip.

Fixes:

- Extracted SetRelativeLayout's Frame computation into TryComputeRelativeFrame so
  the speculative check and the real layout share one source of truth. The
  predictor now resolves and compares the full Rectangle (position included), so a
  text-dependent Pos that moves the view correctly triggers layout.

- The eligibility guard is now exact (`dim is DimAbsolute or DimAuto { Style:
  DimAutoStyle.Text }`). Composite/nested dims and DimAutoStyle.Content/.Auto
  (which lay out subviews while calculating) fall back to SetNeedsLayout.

- Broadened to fixed dimensions and one-axis-auto, the common safe cases: a fixed
  or Text-only-auto view whose resolved Frame is unchanged now skips layout and
  only redraws.

Tests updated/added: Pos-depends-on-Text moves view (regression for tui-cs#1), composite
dim bypasses fast path (tui-cs#2), one-axis-auto same/different size, and fixed-size text
redraw skips layout (tui-cs#3). Full UnitTestsParallelizable: 17458 passed / 0 failed;
UnitTests.NonParallelizable: 72 passed / 0 failed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A new review found the widened same-frame fast path could leave TextFormatter
unconstrained. UpdateTextFormatterText() clears ConstrainToWidth/Height, and the
fast path then skipped SetRelativeLayout() — including its post-resolve cleanup
that restores constraints from GetContentWidth()/GetContentHeight(). For a
fixed-size view with WordWrap, ConstrainToWidth stayed null (treated as
int.MaxValue by GetLines()), so wrapped/clipped text could render incorrectly
until some later layout restored the constraints.

- Extracted that finalization into FinalizeTextFormatterConstraints(), shared by
  SetRelativeLayout() and the fast path, so the fast path leaves TextFormatter in
  exactly the state a full layout pass would.
- Renamed the helper to TryRedrawWithoutLayout() to reflect that it now performs
  the redraw-only handling (finalize constraints, mark NeedsFormat, SetNeedsDraw)
  rather than just answering a question.

Deep review (temporary probe harnesses, since removed) compared the isolated fast
path against a full-layout control for Frame, both TextFormatter constraints, and
rendered driver contents across a sweep of width/height dims (Text-auto, min/max,
fixed) x WordWrap x a text-change sequence — all identical, and the sweep was
confirmed to catch the regression when the finalization was removed. Also probed
CheckBox (overrides UpdateTextFormatterText), Pos.Align sibling groups, fixed
parents with subviews, reentrant TextChanged, and a 100-tick clock simulation —
no unintended consequences.

Permanent coverage added: fixed-size and one-axis-auto constraint survival with
WordWrap, repeated-update frame-drift/ancestor-propagation, reentrancy, and
render-equivalence tests (TextFastPathRenderTests) covering fixed-size wrap/clip
and max-constrained auto wrapping.

Full UnitTestsParallelizable: 17467 passed / 0 failed; UnitTests.NonParallelizable:
72 passed / 0 failed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@harder

harder commented Jun 28, 2026

Copy link
Copy Markdown
Member Author

P1 (formatter constraints lost on the widened fast path) — resolved in ae2047a

Confirmed the bug with a probe: a View { Width = 5, Height = 2, WordWrap = true } had constraints 5×2 after layout, but after a fast-path Text change ConstrainToWidth/ConstrainToHeight were both null while NeedsLayout == false. Since GetLines() treats a null width as int.MaxValue, wrapped/clipped fixed-size text could render incorrectly until some later layout restored the constraints.

Root cause: UpdateTextFormatterText() clears the constraints, and the widened fast path then skipped SetRelativeLayout() — including its post-resolve cleanup that restores them from GetContentWidth()/GetContentHeight().

Fix:

  • Extracted that cleanup into a shared FinalizeTextFormatterConstraints(), called by both SetRelativeLayout() and the fast path, so the fast path leaves TextFormatter in exactly the state a full layout pass produces.
  • Renamed the helper to TryRedrawWithoutLayout() (it now owns the redraw-only handling: finalize constraints → mark NeedsFormatSetNeedsDraw).

Testing

Beyond the requested constraint-survival coverage (fixed-size and one-axis-auto, with WordWrap), I ran a deeper probe pass with temporary harnesses (since removed):

  • Render-equivalence sweep comparing the isolated fast path (laid out only when the setter requests it — no masking Layout()) against a full-layout control on Frame, both TextFormatter constraints, and rendered driver contents, across width dims (Text-auto / min / max / fixed) × height dims (Text-auto / fixed) × WordWrap × a multi-step text sequence — all identical. Verified non-vacuous (removing the finalization makes the fixed-dim cases diverge).
  • Also probed CheckBox (overrides UpdateTextFormatterText, now called eagerly — idempotent), Pos.Align sibling groups, fixed parents with subviews, reentrant TextChanged, and a 100-tick clock simulation (zero frame drift, no ancestor propagation). No unintended consequences.

The high-value probes were kept as permanent tests: TextFastPathRenderTests (render-equivalence for fixed-size wrap/clip and max-constrained auto wrapping), plus clock-drift, reentrancy, and constraint-survival cases.

Results: UnitTestsParallelizable 17467 passed / 0 failed; UnitTests.NonParallelizable 72 passed / 0 failed; no new warnings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Optimizes View.Text updates to avoid scheduling a full layout pass (and ancestor NeedsLayout propagation) when changing text would resolve to the same Frame, while still ensuring correct redraw and TextFormatter constraint behavior. This targets the performance issue described in #5499 (e.g., clock-like labels updating frequently without changing size/position).

Changes:

  • Refactors layout frame resolution into TryComputeRelativeFrame and extracts FinalizeTextFormatterConstraints so both normal layout and the new text fast path share the same computation and formatter finalization.
  • Updates View.Text setter to attempt a redraw-only fast path when a predicted frame matches the current frame and the dims are safely resolvable.
  • Adds focused diagnostics and end-to-end render comparison tests to validate redraw-only behavior and formatter constraint preservation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
Terminal.Gui/ViewBase/View.Layout.cs Extracts frame prediction and formatter constraint finalization to share logic with the text fast path.
Terminal.Gui/ViewBase/View.Text.cs Adds TryRedrawWithoutLayout to skip SetNeedsLayout() when predicted Frame is unchanged, while still forcing format+draw.
Tests/UnitTestsParallelizable/ViewBase/Layout/Dim.AutoTests.TextOptimization.cs Adds diagnostics covering same-frame text updates, propagation behavior, and guards for unsafe cases.
Tests/UnitTestsParallelizable/ViewBase/Layout/TextFastPathRenderTests.cs Adds end-to-end rendering parity tests comparing fast-path redraw vs forced full-layout control.

@harder harder marked this pull request as ready for review June 28, 2026 02:17
@harder harder requested a review from tig as a code owner June 28, 2026 02:17

@tig tig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice.

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.

Skip SetNeedsLayout when a Dim.Auto recompute yields an unchanged size

4 participants