Fix LT-22539: Loss of focus from gloss field#931
Conversation
Render comparison artifactsRender snapshot failures were reported in 3912e756d998 run 27293398351.1, but the latest run c9a9a778884f run 27444495541.1 is clean. This comment will be replaced if a future run produces render snapshot failures again. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Fixes interlinear Focus Box refresh behavior so edits aren’t lost and focus isn’t stolen when related lexicon/MSA data changes (LT-22539 / LT-22534 / LT-22541). The change replaces a “reselect + move focus box” refresh with a targeted field update that aims to preserve the secondary cache and user focus.
Changes:
- Route
InterlinDocForAnalysis.PropChangedrefreshes throughFocusBoxController.UpdateFieldinstead of reselecting the occurrence and repositioning the focus box. - Add
SandboxBase.UpdateFieldto refresh secondary-cache sense/MSA display viaEstablishDefaultSense. - Add plumbing (
IAnalysisControlInternal.UpdateField) and update test doubles/designer usings accordingly.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Src/LexText/Interlinear/SandboxBase.cs | Adds secondary-cache refresh helper (UpdateField) and a morph lookup utility to update displayed sense/MSA without clearing caches. |
| Src/LexText/Interlinear/ITextDllTests/InterlinDocForAnalysisTests.cs | Updates mock sandbox to satisfy new interface method. |
| Src/LexText/Interlinear/InterlinDocForAnalysis.Designer.cs | Adds (currently unused) using directive. |
| Src/LexText/Interlinear/InterlinDocForAnalysis.cs | Switches PropChanged refresh from “reselect/move” to FocusBox.UpdateField. |
| Src/LexText/Interlinear/FocusBoxController.cs | Adds focus-suppression flag and UpdateField forwarding; extends internal sandbox interface. |
Files not reviewed (1)
- Src/LexText/Interlinear/InterlinDocForAnalysis.Designer.cs: Language not supported
| if (hvoObject != null && hvoObject.Owner is IMoMorphSynAnalysis msa) | ||
| { |
| get { return m_sandbox.IsDirty; } | ||
| } | ||
|
|
||
| private bool suppressFocusChange = false; |
| public void UpdateField(int hvo, int flid) | ||
| { | ||
| throw new NotImplementedException(); | ||
| } |
| public void UpdateField(int hvo, int flid) | ||
| { | ||
| ICmObject hvoObject = Caches.MainCache.ServiceLocator.GetInstance<ICmObjectRepository>().GetObject(hvo); | ||
| if (hvoObject is ILexSense lexSense && lexSense.Owner is ILexEntry lexEntry) | ||
| { |
|
This wont' cover updates to Subsenses, which may or may not matter in practice. I think there is an 'Entry' property on ILexSense that will chase up the tree to an Entry if you think that edge case is worth covering. |
|
|
|
I defer to @jasonleenaylor for finishing the reviews on this PR. |
|
@jtmaxwell3 - don't feel you need to address this feedback - I had asked Claude about the PR and was going to review it more manually. Here is Claude's review if you are interested: Concern 2 — Part-of-speech changes made directly on the MSA might not refreshWhat it means: When something changes, FieldWorks broadcasts a PropChanged("object X, field Y changed"). The new code handles the part-of-speech case like this: if (hvoObject.Owner is IMoMorphSynAnalysis msa) { ... } That only fires when the changed object is something owned by an MSA (the grammatical-info record). But editing a part-of-speech is often a change on the MSA object itself (its PartOfSpeech is an atomic reference property). In that case the changed object is the MSA, and its owner is the LexEntry — so the is IMoMorphSynAnalysis test is false and the refresh is skipped. Realistic concern: Moderate, and the most substantive of the three. The author manually verified LT-22541, so the specific reported scenario clearly works — meaning some edit path does flow through an MSA-owned object. The risk is the adjacent cases: a different way of editing the part-of-speech (e.g., from a different dialog or a direct reference change) could silently fail to update the Focus Box display, reproducing exactly the stale-display symptom this PR set out to kill. It wouldn't lose data or crash — the worst case is "I changed the POS and the interlinear didn't refresh until I clicked away." Worth either confirming which PropChanged patterns FieldWorks actually emits for POS edits, or just broadening the check to hvoObject is MSA || hvoObject.Owner is MSA defensively. Cheap to fix, real to leave. Concern 6 — The test stand-in (MockSandbox.UpdateField) throws instead of doing nothingWhat it means: Tests use a fake sandbox, MockSandbox, in place of the real one. The new interface method was stubbed as: public void UpdateField(int hvo, int flid) { throw new NotImplementedException(); } Because InterlinDocForAnalysis.PropChanged now routes through FocusBox.UpdateField → m_sandbox.UpdateField, any test that drives a gloss/sense edit through this document will hit that throw. Realistic concern: Low today, latent landmine tomorrow. I confirmed the current suite (4,251 tests, 0 failures) never triggers a PropChanged against the mock, so nothing breaks right now. But it's a tripwire: the next person who writes a perfectly reasonable test exercising focus-box refresh gets a confusing NotImplementedException from deep inside production code, not an assertion failure. The fix is one line — make it a no-op ({ }). Leaving it is harmless until it isn't, and when it bites it wastes someone's afternoon. Low severity, near-zero fix cost. Concern 7 — No regression test for the new UpdateField logicWhat it means: SandboxBase.UpdateField is the heart of all three fixes (preserve focus, preserve edits, refresh sense/MSA), yet there's no automated test asserting "edit a gloss/sense → secondary cache refreshes and user edits survive." There's an established SandboxBaseTests suite where such a test would belong. Realistic concern: Low immediate, moderate long-term. The code works now (manually verified), so there's no live defect. The risk is durability: this is a subtle, cache-interaction bug that was already regressed once. With no test pinning the behavior, a future refactor of MoveFocusBoxIntoPlace, EstablishDefaultSense, or the PropChanged routing could silently reintroduce the exact "lost edits / stolen focus" symptom — and nobody would notice until a user reports it again. This is the classic "bug fixed without a test" situation: the cost isn't paid today, it's paid the next time someone touches this area. Given the bug's history (three JIRA tickets), the case for a pinning test here is stronger than usual. Bottom line: None of the three block correctness of the reported scenarios — the PR does what it claims. 2 is the only one with a plausible path to a real user-visible miss (a POS edit that doesn't refresh); 6 and 7 are test-hygiene debt that protect future-you rather than fix present behavior. If you address only one before merge, make it 2; 6 is a free one-liner worth grabbing alongside it. |
This fixes https://jira.sil.org/browse/LT-22539, https://jira.sil.org/browse/LT-22534, and https://jira.sil.org/browse/LT-22541. I discovered that MoveFocusBoxIntoPlace caused the secondary cache to be cleared, which meant that any user edits were lost. So I replaced it with UpdateField and EstablishDefaultSense. EstablishDefaultSense is an existing function which updates the sense name and msa in the secondary cache and the display. FocusBoxController.UpdateField suppresses the focus change to fix LT-22539. SandboxBase.UpdateField fixes LT-22534 and LT-22541. I put all of these fixes in the same commit to make sure that they all worked together.
This change is