diff --git a/Src/Common/SimpleRootSite/SimpleRootSite.cs b/Src/Common/SimpleRootSite/SimpleRootSite.cs index 3f6e898759..54d8a71d6b 100644 --- a/Src/Common/SimpleRootSite/SimpleRootSite.cs +++ b/Src/Common/SimpleRootSite/SimpleRootSite.cs @@ -252,7 +252,6 @@ public void OnInputLanguageChanged(IKeyboardDefinition previousKeyboard, IKeyboa /// True if we are waiting to do a refresh on the view (will be done when the view /// becomes visible); false otherwise protected bool m_fRefreshPending = false; - private bool m_fForceNextRefreshDisplay; private RefreshPhase m_refreshPhase; private bool m_fRefreshReplayRequested; @@ -1054,7 +1053,6 @@ protected void NotifyDataAccessSemanticsChanged() if (m_rootb?.Site == null) return; - m_fForceNextRefreshDisplay = true; if (!Visible || FindForm() == null) { m_fRefreshPending = true; @@ -2535,17 +2533,8 @@ public virtual bool RefreshDisplay() return false; } - // PATH-L5: Skip the expensive selection save/restore and drawing - // suspension when the VwRootBox reports no pending changes. - // The root box's NeedsReconstruct flag is set by PropChanged, - // OnStylesheetChange, and other mutation paths. When false, - // Reconstruct() would be a no-op (PATH-R1), so we can avoid - // the managed overhead entirely. - if (!ShouldReconstructDisplay()) - { - m_fRefreshPending = false; - return false; - } + // Always reconstruct: view constructors read state the root box can't see + // (e.g. WS lists), so skipping on NeedsReconstruct left stale views (LT-22610). // Rebuild the display... the drastic way. SelectionRestorer restorer = CreateSelectionRestorer(); @@ -2560,7 +2549,6 @@ public virtual bool RefreshDisplay() { m_rootb.Reconstruct(); m_fRefreshPending = false; - m_fForceNextRefreshDisplay = false; } if (reconstructStopwatch != null) { @@ -2605,11 +2593,6 @@ private void QueueRefreshDisplay() }); } - private bool ShouldReconstructDisplay() - { - return m_fForceNextRefreshDisplay || m_rootb.NeedsReconstruct; - } - /// ------------------------------------------------------------------------------------ /// /// Creates a new selection restorer. diff --git a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayReconstruct.cs similarity index 81% rename from Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs rename to Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayReconstruct.cs index 4c524ccdf8..6343c3cd32 100644 --- a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs +++ b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayReconstruct.cs @@ -36,7 +36,7 @@ protected override SelectionRestorer CreateSelectionRestorer() } [TestFixture] - public class RefreshDisplayNeedsReconstructTests + public class RefreshDisplayReconstructTests { private RefreshDisplayDummyRootSite m_site; private Mock m_rootbMock; @@ -70,19 +70,10 @@ public void TearDown() } [Test] - public void RefreshDisplay_SkipsReconstruct_WhenRootBoxDoesNotNeedReconstruct() + public void RefreshDisplay_AlwaysReconstructs() { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); - - Assert.That(m_site.RefreshDisplay(), Is.False); - Assert.That(m_site.RefreshPending, Is.False); - m_rootbMock.Verify(rb => rb.Reconstruct(), Times.Never); - } - - [Test] - public void RefreshDisplay_Reconstructs_WhenRootBoxNeedsReconstruct() - { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(true); + // LT-22610: view constructors read state the root box can't see, so + // RefreshDisplay must always reconstruct, regardless of NeedsReconstruct. m_rootbMock.Setup(rb => rb.Reconstruct()); Assert.That(m_site.RefreshDisplay(), Is.False); @@ -91,9 +82,8 @@ public void RefreshDisplay_Reconstructs_WhenRootBoxNeedsReconstruct() } [Test] - public void NotifyDataAccessSemanticsChanged_Reconstructs_WhenRootBoxDoesNotNeedReconstruct() + public void NotifyDataAccessSemanticsChanged_Reconstructs() { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_rootbMock.Setup(rb => rb.Reconstruct()); m_site.NotifyDataAccessSemanticsChangedForTest(); @@ -105,7 +95,6 @@ public void NotifyDataAccessSemanticsChanged_Reconstructs_WhenRootBoxDoesNotNeed [Test] public void NotifyDataAccessSemanticsChanged_DefersUntilVisible() { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_rootbMock.Setup(rb => rb.Reconstruct()); m_site.Visible = false; @@ -125,7 +114,6 @@ public void SetRootBoxDataAccess_DoesNotReconstruct_WhenSwapIsCheap() { var replacementDataAccess = Mock.Of(); m_rootbMock.SetupSet(rb => rb.DataAccess = replacementDataAccess); - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_site.SetRootBoxDataAccessForTest(replacementDataAccess); @@ -139,7 +127,6 @@ public void SetRootBoxDataAccessAndRefresh_Reconstructs_WhenSwapChangesDisplaySe { var replacementDataAccess = Mock.Of(); m_rootbMock.SetupSet(rb => rb.DataAccess = replacementDataAccess); - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_rootbMock.Setup(rb => rb.Reconstruct()); m_site.SetRootBoxDataAccessAndRefreshForTest(replacementDataAccess); diff --git a/openspec/changes/render-speedup-benchmark/refresh-dirty-flag-audit.md b/openspec/changes/render-speedup-benchmark/refresh-dirty-flag-audit.md index 6455bd4274..4ffbaeef46 100644 --- a/openspec/changes/render-speedup-benchmark/refresh-dirty-flag-audit.md +++ b/openspec/changes/render-speedup-benchmark/refresh-dirty-flag-audit.md @@ -2,6 +2,14 @@ Date: 2026-03-10 +> **Superseded 2026-07-10 (LT-22610, PR #1006):** the managed PATH-L5 gate +> described below (`SimpleRootSite.RefreshDisplay()` skipping on +> `NeedsReconstruct`) was removed — it missed managed-only view-constructor +> state (e.g. writing-system display lists) invisible to the native flag, +> causing stale views. `RefreshDisplay()` now always reconstructs. The native +> PATH-R1 guard and dirtying paths documented here are unaffected and still +> accurate. + ## Purpose This document audits the parts of FieldWorks that can require a visual refresh after the PATH-L5 / PATH-R1 optimization work. @@ -15,7 +23,7 @@ The recent `SetRootObjects()` fix in [Src/views/VwRootBox.cpp](../../../../Src/v ## Executive Summary - The native source of truth for whether a box tree needs rebuilding is `VwRootBox::m_fNeedsReconstruct` in [Src/views/VwRootBox.h](../../../../Src/views/VwRootBox.h). -- The managed fast path is [SimpleRootSite.RefreshDisplay()](../../../../Src/Common/SimpleRootSite/SimpleRootSite.cs), which now skips managed overhead when `m_rootb.NeedsReconstruct == false`. +- The managed fast path was [SimpleRootSite.RefreshDisplay()](../../../../Src/Common/SimpleRootSite/SimpleRootSite.cs) skipping managed overhead when `m_rootb.NeedsReconstruct == false`; this gate was removed in LT-22610 (see superseded note above) and `RefreshDisplay()` now always reconstructs. - The native fast path is `VwRootBox::Reconstruct()`, which now returns early when `m_fConstructed && !m_fNeedsReconstruct`. - The architecture is correct only if every semantic mutation that can stale the view either: - sets `m_fNeedsReconstruct = true` directly in native code, or @@ -50,11 +58,11 @@ Confirmed native clearing paths: ### Managed refresh gate -[SimpleRootSite.RefreshDisplay()](../../../../Src/Common/SimpleRootSite/SimpleRootSite.cs) does this: +[SimpleRootSite.RefreshDisplay()](../../../../Src/Common/SimpleRootSite/SimpleRootSite.cs) did this (removed in LT-22610; step 3 no longer exists — see superseded note above): 1. If a decorator is installed as `m_rootb.DataAccess`, call `decorator.Refresh()`. 2. If the control is not visible, defer refresh. -3. If `!m_rootb.NeedsReconstruct`, return without selection save/restore or drawing suspension. +3. ~~If `!m_rootb.NeedsReconstruct`, return without selection save/restore or drawing suspension.~~ 4. Otherwise call `m_rootb.Reconstruct()`. This means the system has two layered assumptions: diff --git a/openspec/changes/render-speedup-benchmark/tasks.md b/openspec/changes/render-speedup-benchmark/tasks.md index c16e22ca67..fda57bc25e 100644 --- a/openspec/changes/render-speedup-benchmark/tasks.md +++ b/openspec/changes/render-speedup-benchmark/tasks.md @@ -85,6 +85,6 @@ - [x] PATH-L1-VERIFY Run full benchmark suite and compare before/after timing evidence. Result: **99.99% warm render reduction** (153.00ms → 0.01ms). All 15 scenarios pass with 0% pixel variance. Cold render unaffected (62.33ms → 62.95ms). **Deferred** (future iterations): -- [x] PATH-L5 Skip Reconstruct when data unchanged — gate `SimpleRootSite.RefreshDisplay()` on `VwRootBox.NeedsReconstruct` and cover it with focused tests. +- [x] ~~PATH-L5 Skip Reconstruct when data unchanged — gate `SimpleRootSite.RefreshDisplay()` on `VwRootBox.NeedsReconstruct` and cover it with focused tests.~~ **Reverted 2026-07-10 (LT-22610, PR #1006):** the gate missed managed-only view-constructor state, causing stale views. `RefreshDisplay()` now always reconstructs. - [ ] PATH-L3 Per-paragraph layout caching — dirty-flag line-breaking in `VwParagraphBox::DoLayout()`. - [ ] PATH-L2 Deferred layout in Reconstruct — remove internal `Layout()` call from `Reconstruct()` (blocked: `RootBoxSizeChanged` callback needs dimensions immediately). \ No newline at end of file