From c40725392e61f624d99e51d2b00bbd685ab6d7b6 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 10 Jul 2026 19:42:05 -0400 Subject: [PATCH 1/3] LT-22610: Always reconstruct the view in RefreshDisplay The render optimizations in #724 made SimpleRootSite.RefreshDisplay() skip Reconstruct() unless the root box's NeedsReconstruct flag was set (PATH-L5). That flag only tracks mutations the root box can see (PropChanged, SetRootObjects, OnStylesheetChange), but view constructors also read state the root box cannot see, such as the writing-system lists behind the labels in LabeledMultiStringView. MultiStringSlice mutates that state and calls RefreshDisplay() to show it; the skip turned those refreshes into silent no-ops, leaving stale or missing writing-system rows and labels in the Lexicon Edit detail pane until a real data edit or an application restart forced a rebuild. The skip also cleared m_fRefreshPending without doing the work, dropping refreshes queued while a view was hidden. Remove the skip so RefreshDisplay() always reconstructs, as it did before 9.3.9, and drop the now-unused force flag. The native layout guard (PATH-L1) and the font/shape caches from #724 are unaffected. Co-Authored-By: Claude Fable 5 --- Src/Common/SimpleRootSite/SimpleRootSite.cs | 26 +++++-------------- ...iteTests_RefreshDisplayNeedsReconstruct.cs | 8 ++++-- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/Src/Common/SimpleRootSite/SimpleRootSite.cs b/Src/Common/SimpleRootSite/SimpleRootSite.cs index 3f6e898759..a10a2b41d2 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,13 @@ 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 here, even if the root box reports NeedsReconstruct == false. + // That flag only tracks mutations the root box itself can see (PropChanged, + // SetRootObjects, OnStylesheetChange). View constructors also read state the + // root box cannot see — e.g. the writing-system lists behind the labels in + // LabeledMultiStringView — and RefreshDisplay is the only way such changes + // reach the screen. Skipping based on NeedsReconstruct left stale writing + // system rows/labels in the Lexicon Edit detail pane (LT-22610). // Rebuild the display... the drastic way. SelectionRestorer restorer = CreateSelectionRestorer(); @@ -2560,7 +2554,6 @@ public virtual bool RefreshDisplay() { m_rootb.Reconstruct(); m_fRefreshPending = false; - m_fForceNextRefreshDisplay = false; } if (reconstructStopwatch != null) { @@ -2605,11 +2598,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_RefreshDisplayNeedsReconstruct.cs index 4c524ccdf8..6459fada65 100644 --- a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs +++ b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs @@ -70,13 +70,17 @@ public void TearDown() } [Test] - public void RefreshDisplay_SkipsReconstruct_WhenRootBoxDoesNotNeedReconstruct() + public void RefreshDisplay_Reconstructs_EvenWhenRootBoxReportsNoPendingChanges() { + // LT-22610: view constructors read state the root box cannot see (e.g. the + // writing-system lists behind the labels in LabeledMultiStringView), so + // RefreshDisplay must always reconstruct, even when NeedsReconstruct is false. m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); + m_rootbMock.Setup(rb => rb.Reconstruct()); Assert.That(m_site.RefreshDisplay(), Is.False); Assert.That(m_site.RefreshPending, Is.False); - m_rootbMock.Verify(rb => rb.Reconstruct(), Times.Never); + m_rootbMock.Verify(rb => rb.Reconstruct(), Times.Once); } [Test] From a01f260a927cc9cf437c88f8a429dc8c4bee1f5f Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 10 Jul 2026 20:01:58 -0400 Subject: [PATCH 2/3] Shorten LT-22610 fix comments to two lines Keep the constraint and ticket ID in the source; the full rationale lives in the original commit message and the PR description. Co-Authored-By: Claude Fable 5 --- Src/Common/SimpleRootSite/SimpleRootSite.cs | 9 ++------- ...SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs | 5 ++--- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/Src/Common/SimpleRootSite/SimpleRootSite.cs b/Src/Common/SimpleRootSite/SimpleRootSite.cs index a10a2b41d2..54d8a71d6b 100644 --- a/Src/Common/SimpleRootSite/SimpleRootSite.cs +++ b/Src/Common/SimpleRootSite/SimpleRootSite.cs @@ -2533,13 +2533,8 @@ public virtual bool RefreshDisplay() return false; } - // Always reconstruct here, even if the root box reports NeedsReconstruct == false. - // That flag only tracks mutations the root box itself can see (PropChanged, - // SetRootObjects, OnStylesheetChange). View constructors also read state the - // root box cannot see — e.g. the writing-system lists behind the labels in - // LabeledMultiStringView — and RefreshDisplay is the only way such changes - // reach the screen. Skipping based on NeedsReconstruct left stale writing - // system rows/labels in the Lexicon Edit detail pane (LT-22610). + // 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(); diff --git a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs index 6459fada65..1f0c42eeae 100644 --- a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs +++ b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs @@ -72,9 +72,8 @@ public void TearDown() [Test] public void RefreshDisplay_Reconstructs_EvenWhenRootBoxReportsNoPendingChanges() { - // LT-22610: view constructors read state the root box cannot see (e.g. the - // writing-system lists behind the labels in LabeledMultiStringView), so - // RefreshDisplay must always reconstruct, even when NeedsReconstruct is false. + // LT-22610: view constructors read state the root box can't see, so + // RefreshDisplay must reconstruct even when NeedsReconstruct is false. m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_rootbMock.Setup(rb => rb.Reconstruct()); From 1283d295c4441aed428ea13491cfdc0d939abb8d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Mon, 13 Jul 2026 12:54:18 -0400 Subject: [PATCH 3/3] Clean up dead NeedsReconstruct test/doc references from LT-22610 RefreshDisplay() no longer reads IVwRootBox.NeedsReconstruct, so the mock setups and near-duplicate test in SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs documented a decision point that no longer exists. Renamed the file, merged the two RefreshDisplay tests into one, and dropped the dead NeedsReconstruct stubs from the other tests. Also annotated the PATH-L5 render-speedup-benchmark openspec docs, which still described the removed managed skip as shipped, so they no longer contradict the code. Co-Authored-By: Claude Sonnet 5 --- ...ootSiteTests_RefreshDisplayReconstruct.cs} | 24 ++++--------------- .../refresh-dirty-flag-audit.md | 14 ++++++++--- .../changes/render-speedup-benchmark/tasks.md | 2 +- 3 files changed, 16 insertions(+), 24 deletions(-) rename Src/Common/SimpleRootSite/SimpleRootSiteTests/{SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs => SimpleRootSiteTests_RefreshDisplayReconstruct.cs} (79%) diff --git a/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs b/Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayReconstruct.cs similarity index 79% rename from Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs rename to Src/Common/SimpleRootSite/SimpleRootSiteTests/SimpleRootSiteTests_RefreshDisplayReconstruct.cs index 1f0c42eeae..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,11 +70,10 @@ public void TearDown() } [Test] - public void RefreshDisplay_Reconstructs_EvenWhenRootBoxReportsNoPendingChanges() + public void RefreshDisplay_AlwaysReconstructs() { // LT-22610: view constructors read state the root box can't see, so - // RefreshDisplay must reconstruct even when NeedsReconstruct is false. - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); + // RefreshDisplay must always reconstruct, regardless of NeedsReconstruct. m_rootbMock.Setup(rb => rb.Reconstruct()); Assert.That(m_site.RefreshDisplay(), Is.False); @@ -83,20 +82,8 @@ public void RefreshDisplay_Reconstructs_EvenWhenRootBoxReportsNoPendingChanges() } [Test] - public void RefreshDisplay_Reconstructs_WhenRootBoxNeedsReconstruct() + public void NotifyDataAccessSemanticsChanged_Reconstructs() { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(true); - m_rootbMock.Setup(rb => rb.Reconstruct()); - - Assert.That(m_site.RefreshDisplay(), Is.False); - Assert.That(m_site.RefreshPending, Is.False); - m_rootbMock.Verify(rb => rb.Reconstruct(), Times.Once); - } - - [Test] - public void NotifyDataAccessSemanticsChanged_Reconstructs_WhenRootBoxDoesNotNeedReconstruct() - { - m_rootbMock.SetupGet(rb => rb.NeedsReconstruct).Returns(false); m_rootbMock.Setup(rb => rb.Reconstruct()); m_site.NotifyDataAccessSemanticsChangedForTest(); @@ -108,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; @@ -128,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); @@ -142,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