LT-22610: Fix randomly missing WS labels — RefreshDisplay must always reconstruct

[johnml1135]

Fixes LT-22610 — "Sometimes the WS labels are not visible (random?)" (regression in 9.3.9).

What happened

FieldWorks views are drawn from a "root box" — a native object that builds what you see on screen by running a view constructor. The render-speedup work in #724 taught the root box to remember whether anything had changed since it was last built (a NeedsReconstruct flag), and taught SimpleRootSite.RefreshDisplay() to skip rebuilding the view whenever that flag said "nothing changed."

The problem is that the flag only knows about changes the root box itself can see: edits to the data it displays, a stylesheet change, or being handed a new root object. But view constructors also read state the root box cannot see. The clearest example is the multi-string fields in the Lexicon Edit detail pane (Lexeme Form, Citation Form, etc.): the list of writing-system rows to show, and the little "Frn" / "Frn-Aud" / "FrIPA" labels next to them, come from writing-system lists held on the managed side, not from the root box's data.

When you change which writing systems a field displays — "Show all writing systems," the per-field Configure Writing Systems dialog, or the Set Up Vernacular Writing Systems dialog — the code updates those lists and then calls RefreshDisplay() to redraw the field. After #724, that call looked at NeedsReconstruct, saw "nothing changed" (no data edit had happened), and silently did nothing. The field kept showing the rows and labels from its previous state: labels missing, or one stale label sitting on the wrong row, with the audio-field sound button positioned against the stale layout.

This also explains every odd detail in the bug report:

• "I tried the Set Up Vernacular WS dialog and changing things, but that didn't help" — that dialog's refresh goes through exactly the code path that was being skipped.
• "Later on it seemed to correct itself" — the moment you actually edit data in the field, a real data-change notification sets the flag, and the next refresh genuinely rebuilds the view.
• Restarting FLEx fixed it — a fresh start rebuilds everything from scratch.
• "Random" — the skip path also cleared the "refresh pending" flag without doing the refresh, so a refresh queued while a field was scrolled out of view or being reused could be dropped permanently, depending on timing.

How this fixes it

RefreshDisplay() now always rebuilds the view, as it did in every release before 9.3.9. It is the deliberate "rebuild everything the drastic way" API, called rarely (master refresh, writing-system changes) and precisely when something outside the root box's knowledge has changed — so the root box is the wrong thing to ask about whether the rebuild is needed.

Scope notes:

• The now-unused m_fForceNextRefreshDisplay / ShouldReconstructDisplay() plumbing is removed; the NotifyDataAccessSemanticsChanged() / SetRootBoxDataAccess* API surface is kept.
• The native performance work from perf: Views engine render optimizations — warm 99.99% faster, cold 10% faster #724 is untouched: the layout guard (PATH-L1), the HFONT/color caches, and the Uniscribe shape/analysis caches all remain, so the warm-render wins are preserved. Only the managed refresh short-circuit — the one piece that made a correctness decision from incomplete information — is removed.
• The regression test that pinned the skip behavior now pins the opposite: RefreshDisplay must reconstruct even when the root box reports no pending changes.

Testing

• SimpleRootSiteTests: 116/116 pass (includes the updated RefreshDisplayNeedsReconstruct tests).
• WidgetsTests (LabeledMultiStringView): 19/19 pass.
• XMLViewsTests, DetailControlsTests (DataTree render baselines): pass.

Manual repro for verification: in Lexicon Edit, right-click a multi-string field → "Show All Writing Systems" (or Configure Writing Systems) and confirm the rows and WS labels update immediately, without editing data first.

🤖 Generated with Claude Code

---

This change is 

[github-actions]

NUnit Tests

    1 files  ±0      1 suites  ±0   11m 0s ⏱️ +10s
4 301 tests  - 1  4 228 ✅  - 1  73 💤 ±0  0 ❌ ±0 
4 310 runs   - 1  4 237 ✅  - 1  73 💤 ±0  0 ❌ ±0 

Results for commit 1283d29. ± Comparison against base commit d4e14ac.

This pull request removes 6 and adds 5 tests. Note that renamed tests count towards both.

SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ NotifyDataAccessSemanticsChanged_DefersUntilVisible
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ NotifyDataAccessSemanticsChanged_Reconstructs_WhenRootBoxDoesNotNeedReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ RefreshDisplay_Reconstructs_WhenRootBoxNeedsReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ RefreshDisplay_SkipsReconstruct_WhenRootBoxDoesNotNeedReconstruct
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ SetRootBoxDataAccessAndRefresh_Reconstructs_WhenSwapChangesDisplaySemantics
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayNeedsReconstructTests ‑ SetRootBoxDataAccess_DoesNotReconstruct_WhenSwapIsCheap

SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ NotifyDataAccessSemanticsChanged_DefersUntilVisible
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ NotifyDataAccessSemanticsChanged_Reconstructs
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ RefreshDisplay_AlwaysReconstructs
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ SetRootBoxDataAccessAndRefresh_Reconstructs_WhenSwapChangesDisplaySemantics
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RefreshDisplayReconstructTests ‑ SetRootBoxDataAccess_DoesNotReconstruct_WhenSwapIsCheap

♻️ This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 32.97%. Comparing base (d4e14ac) to head (1283d29).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
+ Coverage   32.96%   32.97%   +0.01%     
==========================================
  Files        1202     1202              
  Lines      278291   278282       -9     
  Branches    37166    37164       -2     
==========================================
+ Hits        91733    91768      +35     
+ Misses     158688   158649      -39     
+ Partials    27870    27865       -5     

Files with missing lines	Coverage Δ
Src/Common/SimpleRootSite/SimpleRootSite.cs	55.26% <ø> (-0.01%)	⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[johnml1135]

Self-review findings and disposition

Ran a recall-biased, high-effort self-review of this branch against main (8 finder angles + verification). The LT-22610 fix itself is correct — the confirmed findings were all about the blast radius of removing the NeedsReconstruct skip globally rather than bugs in the fix's core logic. Cleanups landed in 1283d295c; the rest is dispositioned below.

Fixed in 1283d295c

• Weakened test coverage — after the fix, two tests in SimpleRootSiteTests_RefreshDisplayNeedsReconstruct.cs asserted identical outcomes, and several still stubbed NeedsReconstruct, which production code no longer reads. Renamed the file to SimpleRootSiteTests_RefreshDisplayReconstruct.cs, merged the now-identical pair into RefreshDisplay_AlwaysReconstructs, and dropped the dead mock setups. 115/115 pass (was 116; the merge removed one redundant test).
• Docs drift — openspec/changes/render-speedup-benchmark/refresh-dirty-flag-audit.md still described the removed skip as shipped. Added a superseded note there and struck through the PATH-L5 line in tasks.md, both pointing at this PR.

Considered and deliberately not done: scoping the fix instead of removing the skip globally

The review flagged that removing the skip for all RefreshDisplay() callers costs a full native reconstruct per pane on master refresh (Undo/Redo across a save point, Send/Receive merge, style/WS change), plus a second reconstruct on reentrant refresh replay, and suggested restoring the skip everywhere except the known blind spot (InnerLabeledMultiStringView/MultiStringSlice's managed-only WS-list state).

I looked into this seriously rather than dismissing it. RefreshDisplay() has exactly 5 overrides in the whole tree. Auditing all 5: GhostStringSlice and SandboxBase never call base.RefreshDisplay() (unaffected either way); XmlBrowseViewBase calls it but only touches selection/index state, not view-constructor state (safe under the old skip). But TitleContentsPane.RefreshDisplay() also mutates managed-only VC state (m_vc.SetupWritingSystemsForTitle(), which rebuilds m_writingSystems/m_WsLabels) immediately before calling base.RefreshDisplay() — a second blind spot the review itself didn't surface.

That's why the skip stays out: the overrides only bound known blind spots. LT-22610 was already an intermittent, hard-to-reproduce bug caused by exactly one such unscoped optimization; a scoped fix is only as safe as the completeness of the audit behind it, and confirming there's no third call site that mutates VC-only state and calls RefreshDisplay() directly (not through an override) would require auditing every one of the ~20+ call sites, not just the 5 overrides. That's real, valuable follow-up work, but it deserves its own ticket and test pass rather than being folded into this bug-fix PR.

Deferred to the same follow-up ticket

• Dead native NeedsReconstruct flag (Src/views/VwRootBox.h) — no managed code reads it anymore. Removing it is a native C++ change requiring its own rebuild/verification, and it probably wants to happen alongside deciding whether a scoped skip is worth reintroducing at all. The follow-up starts from the two known blind spots (InnerLabeledMultiStringView, TitleContentsPane).

🤖 Generated with Claude Code