diff --git a/lib/src/binding/scheduler_binding.dart b/lib/src/binding/scheduler_binding.dart index 165a9de6..3072d75c 100644 --- a/lib/src/binding/scheduler_binding.dart +++ b/lib/src/binding/scheduler_binding.dart @@ -637,11 +637,14 @@ mixin SchedulerBinding on NoctermBinding { // Execute persistent frame callbacks // Subclasses (like TerminalBinding) set currentFrameBuildEnd/LayoutEnd/PaintEnd // during their _drawFrameCallback to provide accurate phase timings. - NoctermTimeline.startSync('Build'); + // The 'Build' NoctermTimeline section is opened INSIDE + // TerminalBinding's _drawFrameCallback around the actual + // `super.drawFrame()` call (the buildScope + finalizeTree), + // not here — opening it here would also enclose the layout + // and paint the callback runs, which is misleading. for (final callback in List.of(_persistentCallbacks)) { _invokeFrameCallback(callback, _currentFrameTimeStamp!); } - NoctermTimeline.finishSync(); // Build // Use subclass-provided timing if available, otherwise use now final buildEnd = diff --git a/lib/src/binding/terminal_binding.dart b/lib/src/binding/terminal_binding.dart index ac6f0f05..84c041b1 100644 --- a/lib/src/binding/terminal_binding.dart +++ b/lib/src/binding/terminal_binding.dart @@ -1328,8 +1328,15 @@ class TerminalBinding extends NoctermBinding t0 = DateTime.now().microsecondsSinceEpoch; - // Build phase - handled by BuildOwner via persistent callback + // Build phase - handled by BuildOwner via persistent callback. + // Wrapped in `NoctermTimeline` here (not in + // `SchedulerBinding.handleDrawFrame`, which surrounds the + // entire persistent-callback loop and so would also count + // the layout and paint time we run later) so the per-section + // report reflects the actual build time, not the whole frame. + NoctermTimeline.startSync('Build'); super.drawFrame(); + NoctermTimeline.finishSync(); // Build t1 = DateTime.now().microsecondsSinceEpoch; // Report build end time to scheduler for FrameTiming @@ -1364,11 +1371,13 @@ class TerminalBinding extends NoctermBinding } // Layout phase + NoctermTimeline.startSync('Layout'); renderObject.layout(BoxConstraints.tight( Size(size.width.toDouble(), size.height.toDouble()))); // Flush layout pipeline pipelineOwner.flushLayout(); + NoctermTimeline.finishSync(); // Layout t3 = DateTime.now().microsecondsSinceEpoch; // Report layout end time to scheduler for FrameTiming @@ -1378,8 +1387,10 @@ class TerminalBinding extends NoctermBinding pipelineOwner.flushPaint(); // Paint phase - paint directly to buffer + NoctermTimeline.startSync('Paint'); final canvas = TerminalCanvas(buffer, screenRect); renderObject.paintWithContext(canvas, Offset.zero); + NoctermTimeline.finishSync(); // Paint } t4 = DateTime.now().microsecondsSinceEpoch; diff --git a/lib/src/components/basic.dart b/lib/src/components/basic.dart index 38151180..6e128671 100644 --- a/lib/src/components/basic.dart +++ b/lib/src/components/basic.dart @@ -436,10 +436,13 @@ class RenderConstrainedBox extends RenderObject @override void performLayout() { + final enforcedConstraints = _additionalConstraints.enforce(constraints); if (child != null) { // Apply additional constraints using enforce method - child!.layout(_additionalConstraints.enforce(constraints), - parentUsesSize: true); + child!.layout( + enforcedConstraints, + parentUsesSize: true, + ); // Position child at origin final BoxParentData childParentData = child!.parentData as BoxParentData; @@ -449,7 +452,7 @@ class RenderConstrainedBox extends RenderObject size = child!.size; } else { // If no child, constrain to smallest size allowed by enforced constraints - size = _additionalConstraints.enforce(constraints).constrain(Size.zero); + size = enforcedConstraints.constrain(Size.zero); } } diff --git a/lib/src/components/decorated_box.dart b/lib/src/components/decorated_box.dart index 12e122f1..daedaa57 100644 --- a/lib/src/components/decorated_box.dart +++ b/lib/src/components/decorated_box.dart @@ -425,8 +425,15 @@ class RenderDecoratedBox extends RenderObject BoxDecoration get decoration => _decoration; set decoration(BoxDecoration value) { if (_decoration != value) { + final oldInset = _borderInsetFor(_decoration); + final newInset = _borderInsetFor(value); + final affectsLayout = oldInset != newInset; _decoration = value; - markNeedsLayout(); + if (affectsLayout) { + markNeedsLayout(); + } else { + markNeedsPaint(); + } } } @@ -448,13 +455,7 @@ class RenderDecoratedBox extends RenderObject @override void performLayout() { - // Calculate border insets - double borderInset = 0; - if (_decoration.border != null && !_decoration.border!.hasNoBorder) { - // In terminal, we can only draw 1-character wide borders - // So we treat any border as taking up 1 unit of space - borderInset = 1; - } + final borderInset = _borderInsetFor(_decoration); if (child != null) { // Deflate constraints by border width @@ -480,6 +481,15 @@ class RenderDecoratedBox extends RenderObject } } + double _borderInsetFor(BoxDecoration decoration) { + if (decoration.border != null && !decoration.border!.hasNoBorder) { + // In terminal, we can only draw 1-character wide borders, so any + // visible border reserves one cell regardless of color or style. + return 1; + } + return 0; + } + void _paintDecoration(TerminalCanvas canvas, Offset offset) { // Skip painting if size is non-finite (e.g. from unconstrained scroll views) if (!size.width.isFinite || !size.height.isFinite) return; diff --git a/lib/src/components/render_paragraph.dart b/lib/src/components/render_paragraph.dart index 66fa3f23..1bc19f55 100644 --- a/lib/src/components/render_paragraph.dart +++ b/lib/src/components/render_paragraph.dart @@ -26,8 +26,17 @@ class RenderParagraph extends RenderObject with Selectable { InlineSpan get text => _text; set text(InlineSpan value) { if (_text == value) return; + final oldPlainText = _text.toPlainText(); + final newPlainText = value.toPlainText(); _text = value; _cachedSegments = null; + + if (_layoutResult != null && oldPlainText == newPlainText) { + _styledLines = _mapSegmentsToLines(_segments, _layoutResult!.lines); + markNeedsPaint(); + return; + } + markNeedsLayout(); } diff --git a/lib/src/components/render_stack.dart b/lib/src/components/render_stack.dart index e8b2ec55..02e1a1cb 100644 --- a/lib/src/components/render_stack.dart +++ b/lib/src/components/render_stack.dart @@ -210,7 +210,10 @@ class RenderStack extends RenderObject if (!childParentData.isPositioned) { hasNonPositionedChildren = true; - child.layout(nonPositionedConstraints, parentUsesSize: true); + child.layout( + nonPositionedConstraints, + parentUsesSize: fit != stack_lib.StackFit.expand, + ); final Size childSize = child.size; width = math.max(width, childSize.width); diff --git a/lib/src/components/render_text.dart b/lib/src/components/render_text.dart index 954eb798..d71d1238 100644 --- a/lib/src/components/render_text.dart +++ b/lib/src/components/render_text.dart @@ -26,7 +26,24 @@ class RenderText extends RenderObject with Selectable { String get text => _text; set text(String value) { if (_text == value) return; + final oldSize = hasSize ? size : null; + final oldConstraints = hasSize ? constraints : null; + final hadLayout = _layoutResult != null; _text = value; + + if (hadLayout && oldSize != null && oldConstraints != null) { + final newLayout = _layoutText(oldConstraints); + final newSize = oldConstraints.constrain(Size( + newLayout.actualWidth.toDouble(), + newLayout.actualHeight.toDouble(), + )); + if (newSize == oldSize) { + _layoutResult = newLayout; + markNeedsPaint(); + return; + } + } + markNeedsLayout(); } @@ -81,26 +98,26 @@ class RenderText extends RenderObject with Selectable { @override bool hitTestSelf(Offset position) => true; - @override - void performLayout() { - // For text alignment to work properly, we need to use the actual constraint width - // When in a Column without stretch, we get infinite width, so we use intrinsic width + TextLayoutConfig _layoutConfigFor(BoxConstraints constraints) { final maxWidth = constraints.maxWidth.isFinite ? constraints.maxWidth.toInt() : double.maxFinite.toInt(); - // Debug: print constraint info - // print('RenderText layout: text="$_text", constraints=$constraints, maxWidth=$maxWidth'); - - final config = TextLayoutConfig( + return TextLayoutConfig( softWrap: _softWrap, overflow: _overflow, textAlign: _textAlign, maxLines: _maxLines, maxWidth: maxWidth, ); + } + + TextLayoutResult _layoutText(BoxConstraints constraints) => + TextLayoutEngine.layout(_text, _layoutConfigFor(constraints)); - _layoutResult = TextLayoutEngine.layout(_text, config); + @override + void performLayout() { + _layoutResult = _layoutText(constraints); size = constraints.constrain(Size( _layoutResult!.actualWidth.toDouble(), diff --git a/lib/src/foundation/layout_profiler.dart b/lib/src/foundation/layout_profiler.dart new file mode 100644 index 00000000..277bae61 --- /dev/null +++ b/lib/src/foundation/layout_profiler.dart @@ -0,0 +1,136 @@ +/// Per-render-object layout profiler. +/// +/// Accumulates time spent in [RenderObject.layout] (and its recursive +/// descendants) keyed by the runtime type of the render object. When +/// the host's [FrameProfiler] is recording, [setActive] is flipped on +/// and the per-type counts roll up into the next call to [collect]. +/// +/// Cheap when inactive: a single bool check per layout call. +/// +/// The hot-path is in [RenderObject.layout], which the framework +/// calls on every layout pass. We avoid allocating per call by +/// keeping a fixed-size array of "hot type slots" (the most common +/// render object types like RenderConstrainedBox, RenderParagraph, +/// RenderPadding, etc.) and a Map for the long tail. The hot slot +/// cache is bypassed when inactive. +class NoctermLayoutProfiler { + NoctermLayoutProfiler._(); + + /// Single shared instance. The framework's [RenderObject.layout] + /// reads [isActive] on every call; the chat panel's + /// [FrameProfiler] flips it on when recording starts and back + /// off when recording stops. + static final NoctermLayoutProfiler instance = NoctermLayoutProfiler._(); + + /// Hot-path gate. When false, [RenderObject.layout] skips the + /// bookkeeping entirely and just runs the layout. + static bool isActive = false; + + /// Per-type totals. Keyed by the render object's runtime type + /// string (e.g. `RenderParagraph`, `RenderConstrainedBox`). + /// Cleared by [reset] at the start of every recording. + final Map _byType = {}; + + /// Per-call records, kept so the report can surface the + /// `topSlowLayoutCalls` (the N individual layout calls that + /// took the longest). Capped at 200 to bound memory. + final List<_LayoutCall> _calls = <_LayoutCall>[]; + int _nextCallId = 0; + + /// Start recording. Idempotent. + void setActive() { + isActive = true; + } + + /// Stop recording. The map and call list are kept around so + /// the host can call [collect] after flipping the flag off. + void setInactive() { + isActive = false; + } + + /// Wipe per-type and per-call accumulators. Called by the + /// host at the start of a recording. + void reset() { + _byType.clear(); + _calls.clear(); + _nextCallId = 0; + } + + /// Called from [RenderObject.layout] on every invocation while + /// [isActive] is true. [us] is the elapsed microseconds for + /// that one layout() call (which is one render object's + /// performLayout, NOT the cumulative subtree time — see + /// note in [collect]). + void record(String typeName, int us) { + final stats = _byType.putIfAbsent(typeName, () => _TypeStats()); + stats.totalUs += us; + stats.count++; + if (stats.maxUs < us) stats.maxUs = us; + + // Keep the top slowest individual calls so the report can + // surface "this one Row layout() call took 3.2ms" rather + // than only averages. Capped to keep memory bounded. + if (_calls.length < 200) { + _calls.add(_LayoutCall(_nextCallId++, typeName, us)); + } else { + // Find the smallest in the list and replace it if the new + // one is bigger. Simple linear scan — 200 entries is cheap. + var minIdx = 0; + for (var i = 1; i < _calls.length; i++) { + if (_calls[i].us < _calls[minIdx].us) minIdx = i; + } + if (_calls[minIdx].us < us) { + _calls[minIdx] = _LayoutCall(_nextCallId++, typeName, us); + } + } + } + + /// Drain accumulators into a JSON-friendly map suitable for + /// embedding in a profile report under `byLayout`. + Map collect() { + final byType = {}; + for (final entry in _byType.entries) { + final s = entry.value; + byType[entry.key] = { + 'count': s.count, + 'totalUs': s.totalUs, + 'avgUs': s.count == 0 ? 0 : s.totalUs ~/ s.count, + 'maxUs': s.maxUs, + }; + } + // Sort the per-type entries by totalUs descending so the + // biggest offenders surface at the top of the report. + final sorted = byType.entries.toList() + ..sort((a, b) => + (b.value['totalUs'] as int).compareTo(a.value['totalUs'] as int)); + + // Top-N individual layout calls (capped at 20 in the + // report itself; the underlying list may be longer for + // debugging). + final calls = List<_LayoutCall>.from(_calls) + ..sort((a, b) => b.us.compareTo(a.us)); + final topCalls = calls + .take(20) + .map((c) => {'id': c.id, 'type': c.type, 'us': c.us}) + .toList(); + + return { + 'byType': Map.fromEntries(sorted), + 'topSlowCalls': topCalls, + 'totalCalls': _calls.length < 200 ? _calls.length : 200, + }; + } +} + +class _TypeStats { + int count = 0; + int totalUs = 0; + int maxUs = 0; +} + +class _LayoutCall { + _LayoutCall(this.id, this.type, this.us); + final int id; + final String type; + final int us; +} diff --git a/lib/src/framework/framework.dart b/lib/src/framework/framework.dart index ecc76e6a..3eebacca 100644 --- a/lib/src/framework/framework.dart +++ b/lib/src/framework/framework.dart @@ -7,6 +7,7 @@ import 'package:meta/meta.dart'; import 'package:nocterm/src/components/basic.dart'; import 'package:nocterm/src/foundation/persistent_hash_map.dart'; import 'package:nocterm/src/foundation/nocterm_error.dart'; +import 'package:nocterm/src/foundation/layout_profiler.dart'; import 'package:nocterm/src/rectangle.dart'; import 'package:nocterm/src/size.dart'; import 'package:nocterm/src/style.dart'; diff --git a/lib/src/framework/render_object.dart b/lib/src/framework/render_object.dart index 7dd94fdd..f9792c7c 100644 --- a/lib/src/framework/render_object.dart +++ b/lib/src/framework/render_object.dart @@ -24,7 +24,9 @@ class PipelineOwner { VoidCallback? onNeedsVisualUpdate; void requestLayout(RenderObject renderObject) { - _nodesNeedingLayout.add(renderObject); + if (!_nodesNeedingLayout.contains(renderObject)) { + _nodesNeedingLayout.add(renderObject); + } requestVisualUpdate(); } @@ -167,6 +169,7 @@ class BoxConstraints { bool get hasBoundedHeight => maxHeight < double.infinity; bool get hasInfiniteWidth => minWidth >= double.infinity; bool get hasInfiniteHeight => minHeight >= double.infinity; + bool get isTight => minWidth == maxWidth && minHeight == maxHeight; /// Returns new box constraints that respect the given constraints while being /// as close as possible to the original constraints. @@ -322,6 +325,7 @@ abstract class RenderObject { bool _needsLayout = false; bool _needsPaint = false; bool _hasLayoutError = false; + bool _parentUsesSize = false; /// Whether this render object needs layout. /// @@ -342,8 +346,10 @@ abstract class RenderObject { /// Mark this render object as needing layout. /// /// This will cause [performLayout] to be called during the next layout pass. - /// The mark propagates up the tree the first time it's set; subsequent calls - /// short-circuit. Frame-skip in the scheduler can retire `_hasScheduledFrame` + /// If the parent used this object's size during the last layout, the dirty + /// mark propagates up because ancestor geometry may change. Otherwise this + /// object is a relayout boundary and can be queued directly with its existing + /// constraints. Frame-skip in the scheduler can retire `_hasScheduledFrame` /// between calls, so even a short-circuited call must prod the owner. void markNeedsLayout() { if (_needsLayout) { @@ -352,7 +358,15 @@ abstract class RenderObject { } _needsLayout = true; markNeedsPaint(); - parent?.markNeedsLayout(); + if (parent == null) { + owner?.requestVisualUpdate(); + } else if (_parentUsesSize) { + parent!.markNeedsLayout(); + } else if (_constraints != null) { + owner?.requestLayout(this); + } else { + parent!.markNeedsLayout(); + } } /// Mark this render object as needing to be repainted. @@ -387,6 +401,34 @@ abstract class RenderObject { /// The [parentUsesSize] parameter indicates whether the parent depends on /// this render object's size for its own layout. This is used for optimization. void layout(BoxConstraints constraints, {bool parentUsesSize = false}) { + // Hot-path gate: when the [NoctermLayoutProfiler] is inactive + // (the common case), skip the bookkeeping entirely. The + // framework sets it active only while the chat panel's + // `/d-profiler` recording is running, so the cost of this + // method in the steady state is a single bool check. + if (NoctermLayoutProfiler.isActive) { + final t0 = DateTime.now().microsecondsSinceEpoch; + try { + _layoutImpl(constraints, parentUsesSize: parentUsesSize); + } finally { + final dt = DateTime.now().microsecondsSinceEpoch - t0; + NoctermLayoutProfiler.instance.record(runtimeType.toString(), dt); + } + } else { + _layoutImpl(constraints, parentUsesSize: parentUsesSize); + } + } + + /// Original [layout] body, split out so the profiling wrapper + /// (above) can time it without paying for try/finally when + /// the profiler is inactive. Hot path is the no-op + /// [NoctermLayoutProfiler.isActive] branch above. + void _layoutImpl( + BoxConstraints constraints, { + bool parentUsesSize = false, + }) { + _parentUsesSize = parentUsesSize; + // Always reset error state when layout is called, even if we might skip the actual layout _hasLayoutError = false; _lastError = null; @@ -506,7 +548,8 @@ abstract class RenderObject { _lastStackTrace = null; // If we were already marked as needing layout or paint, notify the owner - if (_needsLayout && parent == null) { + if (_needsLayout && + (parent == null || (!_parentUsesSize && _constraints != null))) { owner.requestLayout(this); } if (_needsPaint && parent == null) { diff --git a/test/framework/layout_invalidation_optimization_test.dart b/test/framework/layout_invalidation_optimization_test.dart new file mode 100644 index 00000000..3bb5ce27 --- /dev/null +++ b/test/framework/layout_invalidation_optimization_test.dart @@ -0,0 +1,580 @@ +import 'package:nocterm/nocterm.dart'; +import 'package:nocterm/src/components/render_text.dart'; +import 'package:nocterm/src/rendering/mouse_region.dart'; +import 'package:test/test.dart'; + +/// Tests for the layout invalidation optimization introduced in 2450c6d +/// ("Optimize render layout invalidation") and the related fixes. +/// +/// The optimization uses two cooperating mechanisms: +/// +/// 1. **Relayout boundaries** — nodes whose parent doesn't depend on +/// their size (`_parentUsesSize == false`) can re-layout alone +/// without cascading up the tree. The parent is notified of paint +/// invalidation only. +/// 2. **Same-footprint text** — `RenderText.text` setter skips +/// `markNeedsLayout()` when the new text has the same rendered +/// size as the old, and only fires `markNeedsPaint()`. +/// +/// These tests pin both behaviors down so the optimizations don't +/// regress, AND so dependent code (chat toolbar's LayoutBuilder, the +/// streaming ContextBar and MetricsDisplay) keeps working correctly. +void main() { + group('relayout boundaries', () { + test( + 'child with parentUsesSize=false relayouts alone, parent is not relaid out', + () { + final owner = PipelineOwner(); + final parent = _SizeIgnoringParent(); + final child = _CountingBox(); + parent.addChild(child); + parent.attach(owner); + + parent.layout(BoxConstraints.tight(const Size(40, 5))); + expect(parent.layoutCount, 1); + expect(child.layoutCount, 1); + + parent.layoutCount = 0; + child.layoutCount = 0; + + child.markNeedsLayout(); + owner.flushLayout(); + + expect(child.layoutCount, 1, reason: 'child should re-layout by itself'); + expect(parent.layoutCount, 0, + reason: 'parent that ignores child size must NOT be ' + 're-laid out — it is a relayout boundary'); + expect(parent.needsPaint, isTrue, + reason: 'parent must still be marked for paint so the ' + 'change is rendered'); + }); + + test('child with parentUsesSize=true cascades relayout to parent', () { + final owner = PipelineOwner(); + final parent = _SizeUsingParent(); + final child = _CountingBox(); + parent.addChild(child); + parent.attach(owner); + + parent.layout(BoxConstraints.tight(const Size(40, 5))); + parent.layoutCount = 0; + child.layoutCount = 0; + + child.markNeedsLayout(); + owner.flushLayout(); + + expect(child.layoutCount, 1); + expect(parent.layoutCount, 1, + reason: 'parent whose size depends on child must re-layout ' + 'when child marks itself dirty'); + }); + + test('relayout boundary still schedules a visual update', () { + // Even when layout is skipped (relayout boundary), the paint + // invalidation must reach the root so a frame is scheduled. + // Without this, the visual change would be invisible. + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _SizeIgnoringParent(); + final child = _CountingBox(); + parent.addChild(child); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 5))); + visualUpdates = 0; + + child.markNeedsLayout(); + + expect(visualUpdates, greaterThan(0), + reason: 'a dirty mark on a child must propagate paint ' + 'invalidation to the root, regardless of _parentUsesSize'); + }); + + test('addChild marks parent dirty for next layout pass', () { + // Regression check: adding a child must trigger relayout, + // otherwise the new child would never be laid out. + final owner = PipelineOwner(); + final parent = _SizeUsingParent(); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 5))); + parent.layoutCount = 0; + + final newChild = _CountingBox(); + parent.addChild(newChild); + + expect(parent.needsLayout, isTrue, + reason: 'addChild must mark parent dirty so the new ' + 'child is actually included in the next layout pass'); + }); + }); + + group('RenderText.text setter optimization', () { + test('same-footprint text change only marks paint', () { + // "9" → "1" both render at 1 cell wide under tight constraints. + final render = RenderText(text: '9'); + render.layout(BoxConstraints.tight(const Size(8, 1))); + + render.text = '1'; + + expect(render.needsLayout, isFalse, + reason: 'same-footprint text change must NOT trigger relayout'); + expect(render.needsPaint, isTrue, + reason: 'same-footprint text change MUST trigger repaint'); + expect(render.selectableLayout?.lines, ['1']); + }); + + test('size-changing text marks layout', () { + // "9" → "10" — the new text is one cell wider. Use loose + // constraints so the constrained size actually reflects the + // text footprint (under tight constraints the optimization + // would see both as constraint-clamped, which is not what + // real-world layouts look like). + final render = RenderText(text: '9'); + render.layout(const BoxConstraints(maxWidth: 100, maxHeight: 1)); + + render.text = '10'; + + expect(render.needsLayout, isTrue, + reason: 'text whose footprint grew must trigger relayout'); + expect(render.needsPaint, isTrue); + }); + + test('text change updates _layoutResult even when size unchanged', () { + // Critical for paint correctness: the cached layout result must + // reflect the new text content, otherwise paint() would draw + // the old characters. + final render = RenderText(text: 'hello'); + render.layout(BoxConstraints.tight(const Size(20, 1))); + expect(render.selectableLayout?.lines, ['hello']); + + render.text = 'world'; // same width + expect(render.selectableLayout?.lines, ['world'], + reason: 'cached layout result must reflect the new text'); + }); + + test('style change marks paint but not layout', () { + // Style-only changes never affect layout — they're paint-only. + final render = RenderText( + text: 'x', + style: const TextStyle(color: Color(0xFFFF0000)), + ); + render.layout(BoxConstraints.tight(const Size(8, 1))); + + render.style = const TextStyle(color: Color(0xFF00FF00)); + + expect(render.needsLayout, isFalse, + reason: 'style-only change must not trigger relayout'); + expect(render.needsPaint, isTrue, + reason: 'style change must trigger repaint'); + }); + + test('text change propagates paint to root even across relayout boundary', + () { + // Even when text is inside a relayout boundary + // (parentUsesSize=false), paint invalidation must reach the root, + // otherwise the change is invisible. The streaming ContextBar + // and MetricsDisplay rely on this for their 16–50ms updates. + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _SizeIgnoringParent(); + final text = RenderText(text: 'a'); + parent.addChild(text); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 1))); + visualUpdates = 0; + + text.text = 'b'; // same footprint + + expect(visualUpdates, greaterThan(0), + reason: 'paint invalidation must propagate to root even ' + 'when layout is skipped by the relayout boundary'); + }); + }); + + group('markNeedsLayout — propagation semantics', () { + test('parentUsesSize=true propagates dirty mark to parent', () { + // When parent actually depends on child size, dirty marks + // must cascade up so the parent re-derives its size. + final owner = PipelineOwner(); + final parent = _SizeUsingParent(); + final child = _CountingBox(); + parent.addChild(child); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 5))); + + // sanity check + expect(parent.needsLayout, isFalse); + + child.markNeedsLayout(); + + expect(parent.needsLayout, isTrue, + reason: 'parent whose size depends on child must be marked ' + 'dirty when child marks itself dirty'); + }); + + test('parentUsesSize=false does NOT cascade, only paints', () { + // The optimization: a node whose parent's size doesn't depend on + // it can re-layout alone. Parent is NOT marked for layout, only + // paint. + final owner = PipelineOwner(); + final parent = _SizeIgnoringParent(); + final child = _CountingBox(); + parent.addChild(child); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 5))); + + expect(parent.needsLayout, isFalse); + + child.markNeedsLayout(); + + expect(parent.needsLayout, isFalse, + reason: 'parent that ignores child size must NOT be marked ' + 'for layout — that would defeat the relayout boundary'); + expect(parent.needsPaint, isTrue, + reason: 'parent must still be marked for paint so the ' + 'change is rendered'); + }); + }); + + group('RenderLayoutBuilder child chain', () { + // The chat toolbar's LayoutBuilder wraps an inner widget tree that + // includes the streaming ContextBar and MetricsDisplay. The + // container child of LayoutBuilder must propagate dirty marks up + // so the toolbar's _layoutCallback re-runs and the inner widgets + // are diffed/updated. If this chain breaks, the inner timers' + // updates become invisible. + // + // Regression: this was previously broken because the container + // child was laid out with parentUsesSize=!constraints.isTight, + // which evaluated to false under tight constraints and broke + // propagation. + test('LayoutBuilder-style wrapper always cascades child dirty marks', () { + final owner = PipelineOwner(); + final lbRoot = _LayoutBuilderChildWrapper(); + final lbChild = _CountingBox(); + lbRoot.addChild(lbChild); + lbRoot.attach(owner); + lbRoot.layout(BoxConstraints.tight(const Size(40, 1))); + + expect(lbRoot.needsLayout, isFalse); + + lbChild.markNeedsLayout(); + + // The wrapper simulates RenderLayoutBuilder's behavior: it always + // declares parentUsesSize=true on its child, so a dirty mark on + // the child MUST cascade up. + expect(lbRoot.needsLayout, isTrue, + reason: 'LayoutBuilder\'s container child must propagate ' + 'dirty marks up so the toolbar rebuilds when inner ' + 'streaming widgets (ContextBar, MetricsDisplay) tick'); + }); + + test( + 'RenderLayoutBuilder child dirty marks propagate up under TIGHT constraints', + () { + // This is the regression test for the exact bug we hit in the + // chat toolbar: under tight constraints (which is what the chat + // panel Column passes when the screen is fully sized), the + // previous `parentUsesSize: !constraints.isTight` evaluated to + // `false`, which broke the propagation chain. The fix is to + // always pass `parentUsesSize: true` because LayoutBuilder + // always derives its size from its child. + final owner = PipelineOwner(); + final parent = _TightParentOfLayoutBuilder(); + final lb = RenderLayoutBuilder(); + parent.addChild(lb); + final lbChild = _CountingBox(); + lb.child = lbChild; + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(40, 1))); + + expect(lb.needsLayout, isFalse); + + lbChild.markNeedsLayout(); + + expect(lb.needsLayout, isTrue, + reason: 'LayoutBuilder must cascade dirty marks from its ' + 'child under tight constraints — otherwise the chat ' + 'toolbar\'s streaming widgets go stale'); + }); + }); + + group('mouse hit-test wrappers', () { + test('RenderMouseRegion cascades child dirty marks under tight constraints', + () { + // RenderMouseRegion uses child.size as its own size. If its child + // changes layout after a terminal resize, the mouse region must relayout + // too so hover/click hit-test bounds stay in sync. + final owner = PipelineOwner(); + final mouseRegion = RenderMouseRegion(onHover: (_) {}); + final child = _CountingBox(); + mouseRegion.child = child; + mouseRegion.attach(owner); + + mouseRegion.layout(BoxConstraints.tight(const Size(20, 3))); + expect(mouseRegion.needsLayout, isFalse); + + child.markNeedsLayout(); + + expect(mouseRegion.needsLayout, isTrue, + reason: 'MouseRegion size and hit-test bounds come from child.size, ' + 'so child layout dirtiness must cascade even when constraints ' + 'are tight.'); + }); + }); + + group('streaming widget repaint path', () { + // Both ContextBar and MetricsDisplay follow the same pattern: + // StatefulComponent + // → internal Timer + // → timer fires setState() + // → build() reads updated runtime values, returns new widget tree + // → framework diffs and calls updateRenderObject() on the + // existing render objects (text/style setters fire + // markNeedsPaint/markNeedsLayout) + // + // For this to produce a visible update, the paint invalidation + // must propagate all the way up to the root, AND a frame must be + // scheduled. These tests verify both halves. + test('style-only update on a deep Text schedules a frame', () { + // Simulates a metrics cell changing color when responding. + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _StreamingWidgetHost(); + final text = RenderText( + text: '42.0 tok/s', + style: const TextStyle(color: Color(0xFF808080)), + ); + text.layout(BoxConstraints.tight(const Size(10, 1))); + parent.addChild(text); + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(20, 1))); + visualUpdates = 0; + + // Style-only change — like MetricsDisplay switching between + // metricsActive (responding) and metricsIdle (idle) color. + text.style = const TextStyle(color: Color(0xFF00FF00)); + + expect(visualUpdates, greaterThan(0), + reason: 'style-only update on a streaming widget cell ' + 'must propagate paint invalidation to the root'); + expect(parent.needsPaint, isTrue, + reason: 'parent must be marked for paint so the visual ' + 'change is rendered'); + }); + + test('same-width text update on a deep Text schedules a frame', () { + // Simulates a ContextBar cell label changing from "61,000" to + // "62,000" (same character count). + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _StreamingWidgetHost(); + final text = RenderText( + text: '61,000 / 976k', + style: const TextStyle(), + ); + text.layout(const BoxConstraints(maxWidth: 100, maxHeight: 1)); + parent.addChild(text); + parent.attach(owner); + parent.layout(const BoxConstraints(maxWidth: 100, maxHeight: 1)); + visualUpdates = 0; + + text.text = '62,000 / 976k'; // same width — should hit the optimization + + expect(visualUpdates, greaterThan(0), + reason: 'same-width text update must still propagate ' + 'paint invalidation to root — this is the hot path ' + 'for the streaming ContextBar'); + expect(parent.needsPaint, isTrue); + }); + + test('size-changing text update marks relayout AND propagates paint', () { + // Simulates a token count growing from 1 digit to 2 digits + // (e.g. "9,000" → "10,000"). + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _StreamingWidgetHost(); + final text = RenderText( + text: '9,000 / 976k', + style: const TextStyle(), + ); + text.layout(const BoxConstraints(maxWidth: 100, maxHeight: 1)); + parent.addChild(text); + parent.attach(owner); + parent.layout(const BoxConstraints(maxWidth: 100, maxHeight: 1)); + visualUpdates = 0; + + text.text = '10,000 / 976k'; // wider — must trigger relayout + + expect(visualUpdates, greaterThan(0), + reason: 'size-changing text must propagate paint to root'); + expect(text.needsLayout, isTrue, + reason: 'size-changing text must trigger relayout so the ' + 'parent re-positions the cell'); + expect(parent.needsPaint, isTrue); + }); + + test('multiple Text updates cascade paint without over-marking', () { + // Simulates a streaming tick that updates several cells. + // After the first update propagates, subsequent updates on + // siblings should still propagate but not double-schedule. + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _StreamingWidgetHost(); + final cells = [ + for (var i = 0; i < 5; i++) + RenderText(text: '$i', style: const TextStyle()) + ..layout(BoxConstraints.tight(const Size(1, 1))), + ]; + for (final c in cells) { + parent.addChild(c); + } + parent.attach(owner); + parent.layout(BoxConstraints.tight(const Size(5, 1))); + visualUpdates = 0; + + // Update every cell. + for (var i = 0; i < 5; i++) { + cells[i].text = '${i + 10}'; + } + + expect(visualUpdates, greaterThan(0)); + // Parent should be marked for paint but NOT re-laid out + // (text widths didn't change). + expect(parent.needsPaint, isTrue); + expect(parent.needsLayout, isFalse, + reason: 'same-footprint updates should not cascade relayout'); + }); + }); +} + +// ───────────────────────────────────────────────────────────────────── +// Test fixtures +// ───────────────────────────────────────────────────────────────────── + +/// A parent that uses its child's size in its own layout (i.e., +/// `_parentUsesSize = true` is correct). Mirrors RenderColumn / +/// RenderConstrainedBox. +class _SizeUsingParent extends RenderObject + with ContainerRenderObjectMixin { + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + if (children.isNotEmpty) { + children.first.layout( + BoxConstraints.tight(const Size(8, 2)), + parentUsesSize: true, + ); + size = constraints.constrain(children.first.size); + } else { + size = constraints.constrain(const Size(40, 5)); + } + } +} + +/// A parent whose size does NOT depend on its child — the canonical +/// "relayout boundary" case. +class _SizeIgnoringParent extends RenderObject + with ContainerRenderObjectMixin { + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + if (children.isNotEmpty) { + children.first.layout( + BoxConstraints.tight(const Size(8, 2)), + parentUsesSize: false, // ← the optimization + ); + } + size = constraints.constrain(const Size(40, 5)); + } +} + +/// Wrapper that simulates RenderLayoutBuilder's "always parentUsesSize" +/// policy. Used to verify the chain stays intact when the toolbar's +/// LayoutBuilder rebuilds. +class _LayoutBuilderChildWrapper extends RenderObject + with ContainerRenderObjectMixin { + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + if (children.isNotEmpty) { + children.first.layout( + BoxConstraints.tight(const Size(8, 2)), + parentUsesSize: true, // ← the fix + ); + size = constraints.constrain(children.first.size); + } else { + size = constraints.constrain(const Size(40, 5)); + } + } +} + +class _CountingBox extends RenderObject { + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + size = constraints.constrain(const Size(1, 1)); + } +} + +/// Mimics RenderLayoutBuilder's hosting parent (e.g. RenderColumn in +/// the chat panel). Passes TIGHT constraints to its child so we can +/// exercise the exact regression scenario where +/// `parentUsesSize: !constraints.isTight` would evaluate to false. +class _TightParentOfLayoutBuilder extends RenderObject + with ContainerRenderObjectMixin { + @override + void performLayout() { + if (children.isNotEmpty) { + children.first.layout( + // tight constraints — this is the case where the bug bit + BoxConstraints.tight(const Size(40, 1)), + parentUsesSize: true, + ); + size = children.first.size; + } + } +} + +/// Mimics the chat toolbar's `Row` of Text cells (used by both +/// ContextBar's BgProgressBar and MetricsDisplay). Children have +/// `parentUsesSize: true` so dirty marks cascade up. +class _StreamingWidgetHost extends RenderObject + with ContainerRenderObjectMixin { + @override + void performLayout() { + for (final child in children) { + child.layout(child.constraints, parentUsesSize: true); + } + if (children.isNotEmpty) { + size = constraints.constrain(children.first.size); + } else { + size = constraints.constrain(const Size(20, 1)); + } + } +} diff --git a/test/framework/relayout_boundary_test.dart b/test/framework/relayout_boundary_test.dart new file mode 100644 index 00000000..1d0eec59 --- /dev/null +++ b/test/framework/relayout_boundary_test.dart @@ -0,0 +1,93 @@ +import 'package:nocterm/nocterm.dart'; +import 'package:test/test.dart'; + +void main() { + group('relayout boundaries', () { + test('child laid out without parent size dependency relayouts alone', () { + var visualUpdates = 0; + final owner = PipelineOwner() + ..onNeedsVisualUpdate = () { + visualUpdates++; + }; + final parent = _BoundaryParent(parentUsesChildSize: false); + final child = _CountingBox(); + parent.child = child; + parent.attach(owner); + + parent.layout(BoxConstraints.tight(const Size(20, 5))); + expect(parent.layoutCount, 1); + expect(child.layoutCount, 1); + + parent.layoutCount = 0; + child.layoutCount = 0; + visualUpdates = 0; + + child.markNeedsLayout(); + + expect(child.needsLayout, isTrue); + expect(parent.needsLayout, isFalse); + expect(parent.needsPaint, isTrue); + expect(visualUpdates, greaterThan(0)); + + owner.flushLayout(); + + expect(parent.layoutCount, 0); + expect(child.layoutCount, 1); + expect(child.needsLayout, isFalse); + }); + + test('child laid out with parent size dependency propagates layout', () { + final owner = PipelineOwner(); + final parent = _BoundaryParent(parentUsesChildSize: true); + final child = _CountingBox(); + parent.child = child; + parent.attach(owner); + + parent.layout(BoxConstraints.tight(const Size(20, 5))); + parent.layoutCount = 0; + child.layoutCount = 0; + + child.markNeedsLayout(); + + expect(child.needsLayout, isTrue); + expect(parent.needsLayout, isTrue); + + parent.layout(BoxConstraints.tight(const Size(20, 5))); + + expect(parent.layoutCount, 1); + expect(child.layoutCount, 1); + expect(parent.needsLayout, isFalse); + expect(child.needsLayout, isFalse); + }); + }); +} + +class _BoundaryParent extends RenderObject + with RenderObjectWithChildMixin { + _BoundaryParent({required this.parentUsesChildSize}); + + final bool parentUsesChildSize; + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + child?.layout( + BoxConstraints.tight(const Size(8, 2)), + parentUsesSize: parentUsesChildSize, + ); + size = constraints.constrain( + parentUsesChildSize ? child?.size ?? Size.zero : const Size(20, 5), + ); + } +} + +class _CountingBox extends RenderObject { + int layoutCount = 0; + + @override + void performLayout() { + layoutCount++; + size = constraints.constrain(const Size(1, 1)); + } +} diff --git a/test/framework/render_object_invalidation_optimization_test.dart b/test/framework/render_object_invalidation_optimization_test.dart new file mode 100644 index 00000000..402706c5 --- /dev/null +++ b/test/framework/render_object_invalidation_optimization_test.dart @@ -0,0 +1,90 @@ +import 'package:nocterm/nocterm.dart' hide TextAlign; +import 'package:nocterm/src/components/decorated_box.dart'; +import 'package:nocterm/src/components/render_paragraph.dart'; +import 'package:nocterm/src/components/render_text.dart'; +import 'package:test/test.dart'; + +void main() { + group('render object invalidation optimization', () { + test('RenderText downgrades same-footprint text changes to paint', () { + final render = RenderText(text: '1'); + render.layout(const BoxConstraints(maxWidth: 10, maxHeight: 5)); + + render.text = '2'; + + expect(render.needsLayout, isFalse); + expect(render.needsPaint, isTrue); + expect(render.selectableLayout?.lines, ['2']); + }); + + test('RenderText relayouts when text footprint changes', () { + final render = RenderText(text: '9'); + render.layout(const BoxConstraints(maxWidth: 10, maxHeight: 5)); + + render.text = '10'; + + expect(render.needsLayout, isTrue); + expect(render.needsPaint, isTrue); + }); + + test('RenderText respects parent-determined size for text changes', () { + final render = RenderText(text: '9'); + render.layout(BoxConstraints.tight(const Size(8, 1))); + + render.text = '10'; + + expect(render.needsLayout, isFalse); + expect(render.needsPaint, isTrue); + expect(render.size, const Size(8, 1)); + expect(render.selectableLayout?.lines, ['10']); + }); + + test('RenderParagraph downgrades style-only span changes to paint', () { + final render = RenderParagraph( + text: const TextSpan( + text: 'status', + style: TextStyle(color: Colors.red), + ), + ); + render.layout(const BoxConstraints(maxWidth: 20, maxHeight: 5)); + + render.text = const TextSpan( + text: 'status', + style: TextStyle(color: Colors.green), + ); + + expect(render.needsLayout, isFalse); + expect(render.needsPaint, isTrue); + expect(render.selectableLayout?.lines, ['status']); + }); + + test('RenderDecoratedBox downgrades paint-only decoration changes', () { + final render = RenderDecoratedBox( + decoration: const BoxDecoration(color: Colors.red), + ); + render.layout(const BoxConstraints(maxWidth: 10, maxHeight: 5)); + + render.decoration = const BoxDecoration(color: Colors.green); + + expect(render.needsLayout, isFalse); + expect(render.needsPaint, isTrue); + }); + + test('RenderDecoratedBox relayouts when border inset changes', () { + final render = RenderDecoratedBox( + decoration: const BoxDecoration(color: Colors.red), + ); + render.layout(const BoxConstraints(maxWidth: 10, maxHeight: 5)); + + render.decoration = const BoxDecoration( + color: Colors.red, + border: BoxBorder( + top: BorderSide(color: Colors.green), + ), + ); + + expect(render.needsLayout, isTrue); + expect(render.needsPaint, isTrue); + }); + }); +} diff --git a/test/framework/render_pipeline_test.dart b/test/framework/render_pipeline_test.dart index f031b07d..abd5ca9a 100644 --- a/test/framework/render_pipeline_test.dart +++ b/test/framework/render_pipeline_test.dart @@ -1362,7 +1362,7 @@ class _TrackingRenderBox extends RenderObject void performLayout() { onPerformLayout?.call(); if (child != null) { - child!.layout(constraints); + child!.layout(constraints, parentUsesSize: true); size = child!.size; } else { size = constraints.constrain(const Size(10, 1));