From 568cef9c0a2fbe6715d793e40fbbaa557ba1ffe0 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 07:26:58 -0400 Subject: [PATCH 1/8] Add failing repro tests for ComplexConcPatternVc direct-edit crash ComplexConcPatternVc has no UpdateProp override, so any edit that reaches the view engine without going through PatternView.OnKeyPress (IME composition, drag-and-drop, or a direct IVwSelection.ReplaceWithTsString call) falls through to VwBaseVc.UpdateProp, which throws NotImplementedException. These tests drive a real PatternView/ ComplexConcPatternVc pair and confirm the crash reproduces across every fragment tried: ktagType, ktagForm, ktagGloss, ktagCategory, ktagEntry, ktagTag, ktagInfl, kfragOR, kfragHash, and both min/max quantifier lines (11 of 11 fail today). Two contrast tests pass already: a plain keystroke never reaches the engine (PatternView.OnKeyPress swallows it), and Delete still raises RemoveItemsRequested. Also seed the probe/bug docs from the prior investigation. --- ...omplexConcPatternVcDirectEditProbeTests.cs | 125 ++++++ Docs/bugs/complex-conc-pattern-crash.md | 68 +++ .../ComplexConcPatternVcDirectEditTests.cs | 425 ++++++++++++++++++ 3 files changed, 618 insertions(+) create mode 100644 Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs create mode 100644 Docs/bugs/complex-conc-pattern-crash.md create mode 100644 Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs diff --git a/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs b/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs new file mode 100644 index 0000000000..fc2537268d --- /dev/null +++ b/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs @@ -0,0 +1,125 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) +// +// REVIEW PROBE (not part of any shipped fix) -- exists only to determine, empirically, +// what happens when a ComplexConcPatternVc-hosted PatternView is edited via the same +// low-level ReplaceWithTsString bypass used by the phon-rule-formula-readonly repro. +// ComplexConcControl leaves PatternView.ReadOnlyView = false, so this bypass is not even +// needed in production -- a plain keystroke reaches OnKeyPress first, but ReplaceWithTsString +// is exactly what IME composition or drag-and-drop would call, same as the phon-rule case. + +using System.Windows.Forms; +using NUnit.Framework; +using SIL.LCModel; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Core.KernelInterfaces; +using SIL.LCModel.Infrastructure; +using SIL.FieldWorks.Common.RootSites; +using SIL.FieldWorks.Common.ViewsInterfaces; +using SIL.FieldWorks.LexText.Controls; +using XCore; + +namespace SIL.FieldWorks.IText +{ + [TestFixture] + public class ComplexConcPatternVcDirectEditProbeTests : MemoryOnlyBackendProviderTestBase + { + private Mediator m_mediator; + private PropertyTable m_propertyTable; + private TestPatternView m_view; + + public override void TestSetup() + { + base.TestSetup(); + m_mediator = new Mediator(); + m_propertyTable = new PropertyTable(m_mediator); + m_propertyTable.SetProperty("cache", Cache, false); + } + + public override void TestTearDown() + { + if (m_view != null) + { + m_view.Dispose(); + m_view = null; + } + if (m_propertyTable != null) + { + m_propertyTable.Dispose(); + m_propertyTable = null; + } + if (m_mediator != null) + { + m_mediator.Dispose(); + m_mediator = null; + } + base.TestTearDown(); + } + + private class NullPatternControl : IPatternControl + { + public object GetContext(SelectionHelper sel) => null; + public object GetContext(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; + public object GetItem(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; + public int GetItemContextIndex(object ctxt, object obj) => -1; + public SelLevInfo[] GetLevelInfo(object ctxt, int index) => null; + public int GetContextCount(object ctxt) => 0; + public object GetNextContext(object ctxt) => null; + public object GetPrevContext(object ctxt) => null; + public int GetFlid(object ctxt) => 0; + } + + private class TestPatternView : PatternView + { + public void CallLayout() + { + OnLayout(new LayoutEventArgs(this, string.Empty)); + } + } + + /// + /// Probe: a bare word node (no Form/Gloss/Category/InflFeatures) renders as a single + /// line whose only content is the computed "Type: Word" text, bound to the fake tag + /// ComplexConcPatternVc.ktagType on the synthetic (negative-hvo, non-domain) word node. + /// ComplexConcControl leaves ReadOnlyView = false and ComplexConcPatternVc never marks + /// this fragment ktptNotEditable, so -- unlike the phon-rule fix -- nothing blocks a + /// direct low-level edit here at all, not even OnKeyPress (which only blocks WM_CHAR, + /// not ReplaceWithTsString). This probe records what actually happens. + /// + [Test] + public void ReplaceWithTsString_OnWordNodeTypeLine_RecordsWhatHappens() + { + var root = new ComplexConcGroupNode(); + var wordNode = new ComplexConcWordNode(); + root.Children.Add(wordNode); + var model = new ComplexConcPatternModel(Cache, root); + + var vc = new ComplexConcPatternVc(Cache, m_propertyTable); + var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; + view.Init(m_mediator, m_propertyTable, model.Root.Hvo, new NullPatternControl(), vc, + ComplexConcPatternVc.kfragPattern, model.DataAccess); + view.CallLayout(); + m_view = view; + + var levels = new[] + { + new SelLevInfo { tag = ComplexConcPatternSda.ktagChildren, ihvo = 0 } + }; + IVwSelection sel = view.RootBox.MakeTextSelInObj(0, levels.Length, levels, + ComplexConcPatternVc.ktagType, null, true, false, false, /* fWholeObj */ true, /* fInstall */ true); + Assert.That(sel, Is.Not.Null, + "could not construct a selection over the word node's Type line -- fixture/path assumption is wrong"); + + ITsString replacement = TsStringUtils.MakeString("HACKED", Cache.DefaultUserWs); + + // No try/catch: if this throws, that IS the finding (an unhandled exception from a + // direct edit on an unmarked, unaudited ComplexConcPatternVc fragment). If it does + // not throw, the test passes and the assertions below record what changed instead. + UndoableUnitOfWorkHelper.Do("undo", "redo", Cache.LangProject, + () => sel.ReplaceWithTsString(replacement)); + + Assert.Pass("No exception was thrown by ReplaceWithTsString on the word node's Type line."); + } + } +} diff --git a/Docs/bugs/complex-conc-pattern-crash.md b/Docs/bugs/complex-conc-pattern-crash.md new file mode 100644 index 0000000000..d91e7951a3 --- /dev/null +++ b/Docs/bugs/complex-conc-pattern-crash.md @@ -0,0 +1,68 @@ +# Bug 4 — Complex Concordance pattern builder crashes on any direct edit + +**Area:** Texts & Words → Complex Concordance → pattern builder pane (`ComplexConcControl`) +**Type:** Crash (unhandled exception), not data corruption +**Found by:** adversarial review of Bug 1 (`phon-rule-direct-editing.md`), which shares the same base classes + +## What the user sees + +Texts & Words area, **Complex Concordance** tool. The top-left pane is a pattern builder: you insert +Morph / Word / Tag / OR / Word Boundary pieces from a row of options, and each appears as a bracketed +column with labelled rows (Form, Gloss, Cat, Entry, Type, Infl) filled in through choosers. Like the +phonological rule formula, it is meant to be built by inserting and deleting, never by typing. + +If input reaches the view without passing through `PatternView.OnKeyPress` — IME composition when +typing a vernacular script, or dragging text into the pane — FLEx dies with an unhandled +`NotImplementedException`. No warning, no "field not editable" feedback. Because the trigger is an +IME path, it would preferentially hit vernacular-script users. + +## Why it happens + +`ComplexConcControl` and the rule formula editor are built from the same two classes: `PatternView` +(`Src/LexText/LexTextControls/PatternView.cs`) and `PatternVcBase` +(`Src/LexText/LexTextControls/PatternVcBase.cs`). `PatternVcBase` has exactly two subclasses and +`PatternView` exactly two consumers — the rule formula editor and this one — so the audit surface is +closed. + +Two differences from the rule formula editor make this a crash rather than a rename: + +1. **No `UpdateProp` override.** `RuleFormulaVcBase` overrides `UpdateProp`, so an edit reaching the + view is intercepted and absorbed. `ComplexConcPatternVc` (`Src/LexText/Interlinear/ComplexConcPatternVc.cs`) + has no such override, so the engine falls through to `VwBaseVc.UpdateProp`, which throws + `NotImplementedException`. Nothing catches it. +2. **No wall.** `ComplexConcControl.Designer.cs:57` still sets `ReadOnlyView = false`, and none of + `ComplexConcPatternVc`'s fragments are marked `ktptNotEditable`. Bug 1 closed both of these for the + rule formula editor; this control was left as-is. + +## Why it is NOT the Bug 1 corruption + +`ComplexConcPatternVc` binds no real domain fields — a grep for `AddStringAltMember` in that file +returns zero. Its content is synthetic: `ComplexConcPatternNode.Hvo` values are negative sentinels and +`Form`/`Gloss`/etc. are plain in-memory properties served by `ComplexConcPatternSda`, not LCM objects. +So a stray edit cannot rename a shared phoneme or natural class the way Bug 1's could. It just throws. + +Crash is louder but arguably less dangerous than Bug 1's silent project-wide rename. Both are real. + +## Evidence + +`Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs` (seeded in this worktree) is a working probe +written during Bug 1's adversarial review. It builds a real `ComplexConcGroupNode` / +`ComplexConcWordNode` / `ComplexConcPatternSda` / `PatternView` exactly as `ComplexConcControl.Init` +does, then calls `IVwSelection.ReplaceWithTsString` on the word node's Type line +(`ComplexConcPatternVc.ktagType`). Result: unhandled `NotImplementedException` from +`VwBaseVc.UpdateProp` propagating out of `ReplaceWithTsString`. + +Treat the probe as a starting point to verify independently, not as a finished test. + +## Not yet established + +Whether the crash is reachable in a running FLEx via a real IME or drag-and-drop, as opposed to via a +direct `ReplaceWithTsString` call in a test. This is the same open question Bug 1 has, and it is why +the probe proves the *mechanism* rather than the *user path*. + +## Related + +- `phon-rule-direct-editing.md` — Bug 1, the sibling defect in the other `PatternVcBase` subclass. +- `ConstChartVc.cs:297` has the same defect *shape* as Bug 1 (binds a shared `CmPossibility` field) + but appears guarded at the cell level by `MakeCellsMethod.cs:495`. Marked SUSPECTED-safe by code + reading only; never verified by execution. diff --git a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs new file mode 100644 index 0000000000..07065ce580 --- /dev/null +++ b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs @@ -0,0 +1,425 @@ +// Copyright (c) 2026 SIL International +// This software is licensed under the LGPL, version 2.1 or later +// (http://www.gnu.org/licenses/lgpl-2.1.html) +// +// Reproduction and regression coverage for the Complex Concordance pattern-builder crash +// (Docs/bugs/complex-conc-pattern-crash.md). ComplexConcControl and the phonological rule +// formula editor share PatternView/PatternVcBase. ComplexConcPatternVc has no UpdateProp +// override, so an edit that reaches the view engine without passing through +// PatternView.OnKeyPress (IME composition, drag-and-drop, or any direct +// IVwSelection.ReplaceWithTsString call) falls through to VwBaseVc.UpdateProp, which throws +// NotImplementedException. Unlike the sibling rule-formula bug, ComplexConcPatternVc binds no +// real domain fields via AddStringAltMember (verified by inspection: zero occurrences in +// ComplexConcPatternVc.cs), so this is a crash, not a silent corruption/rename. +// +// These tests drive a real IVwRootBox (PatternView/ComplexConcPatternVc) against a real +// in-memory LcmCache and call IVwSelection.ReplaceWithTsString directly -- the same low-level +// entry point IME composition or drag-and-drop would use, and one PatternView.OnKeyPress never +// sees because it only reacts to Windows key events, not to ReplaceWithTsString. +using System.Collections.Generic; +using System.Windows.Forms; +using NUnit.Framework; +using SIL.LCModel; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Core.KernelInterfaces; +using SIL.LCModel.Infrastructure; +using SIL.FieldWorks.Common.RootSites; +using SIL.FieldWorks.Common.ViewsInterfaces; +using SIL.FieldWorks.LexText.Controls; +using XCore; +using FS = System.Collections.Generic.Dictionary; + +namespace SIL.FieldWorks.IText +{ + [TestFixture] + public class ComplexConcPatternVcDirectEditTests : MemoryOnlyBackendProviderTestBase + { + private Mediator m_mediator; + private PropertyTable m_propertyTable; + private TestPatternView m_view; + + public override void TestSetup() + { + base.TestSetup(); + m_mediator = new Mediator(); + m_propertyTable = new PropertyTable(m_mediator); + m_propertyTable.SetProperty("cache", Cache, false); + } + + public override void TestTearDown() + { + if (m_view != null) + { + m_view.Dispose(); + m_view = null; + } + if (m_propertyTable != null) + { + m_propertyTable.Dispose(); + m_propertyTable = null; + } + if (m_mediator != null) + { + m_mediator.Dispose(); + m_mediator = null; + } + base.TestTearDown(); + } + + private class NullPatternControl : IPatternControl + { + public object GetContext(SelectionHelper sel) => null; + public object GetContext(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; + public object GetItem(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; + public int GetItemContextIndex(object ctxt, object obj) => -1; + public SelLevInfo[] GetLevelInfo(object ctxt, int index) => null; + public int GetContextCount(object ctxt) => 0; + public object GetNextContext(object ctxt) => null; + public object GetPrevContext(object ctxt) => null; + public int GetFlid(object ctxt) => 0; + } + + private class TestPatternView : PatternView + { + public void CallLayout() + { + OnLayout(new LayoutEventArgs(this, string.Empty)); + } + + public void SimulateKeyPress(char ch) + { + OnKeyPress(new KeyPressEventArgs(ch)); + } + + public void SimulateKeyDown(Keys key) + { + OnKeyDown(new KeyEventArgs(key)); + } + } + + private IPartOfSpeech CreatePartOfSpeech(string name, string abbr) + { + IPartOfSpeech pos = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + if (Cache.LangProject.PartsOfSpeechOA == null) + Cache.LangProject.PartsOfSpeechOA = Cache.ServiceLocator.GetInstance().Create(); + pos = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PartsOfSpeechOA.PossibilitiesOS.Add(pos); + pos.Name.SetAnalysisDefaultWritingSystem(name); + pos.Abbreviation.SetAnalysisDefaultWritingSystem(abbr); + }); + return pos; + } + + private ICmPossibility CreateTag(string name, string abbr) + { + ICmPossibility tag = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + if (Cache.LangProject.TextMarkupTagsOA == null) + Cache.LangProject.TextMarkupTagsOA = Cache.LangProject.GetDefaultTextTagList(); + tag = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.TextMarkupTagsOA.PossibilitiesOS.Add(tag); + tag.Name.SetAnalysisDefaultWritingSystem(name); + tag.Abbreviation.SetAnalysisDefaultWritingSystem(abbr); + }); + return tag; + } + + private IFsClosedFeature CreateClosedFeature(string name, out IFsSymFeatVal value) + { + IFsClosedFeature feat = null; + IFsSymFeatVal val = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + IFsFeatureSystem featSys = Cache.LanguageProject.MsFeatureSystemOA; + feat = Cache.ServiceLocator.GetInstance().Create(); + featSys.FeaturesOC.Add(feat); + feat.Name.SetAnalysisDefaultWritingSystem(name); + feat.Abbreviation.SetAnalysisDefaultWritingSystem(name); + val = Cache.ServiceLocator.GetInstance().Create(); + feat.ValuesOC.Add(val); + val.Name.SetAnalysisDefaultWritingSystem("v1"); + val.Abbreviation.SetAnalysisDefaultWritingSystem("v1"); + }); + value = val; + return feat; + } + + /// + /// Builds a live PatternView/ComplexConcPatternVc pair over a one-child pattern and + /// returns the view plus the model root (so callers can add children before use). + /// + private (ComplexConcPatternModel model, TestPatternView view) BuildView() + { + var model = new ComplexConcPatternModel(Cache); + var vc = new ComplexConcPatternVc(Cache, m_propertyTable); + var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; + view.Init(m_mediator, m_propertyTable, model.Root.Hvo, new NullPatternControl(), vc, + ComplexConcPatternVc.kfragPattern, model.DataAccess); + m_view = view; + return (model, view); + } + + private static IVwSelection MakeSelOnChild(TestPatternView view, int childIndex, int tag) + { + var levels = new[] + { + new SelLevInfo { tag = ComplexConcPatternSda.ktagChildren, ihvo = childIndex } + }; + return view.RootBox.MakeTextSelInObj(0, levels.Length, levels, tag, null, true, false, false, + /* fWholeObj */ true, /* fInstall */ true); + } + + private void AttemptEdit(IVwSelection sel, string replacementText, int ws) + { + ITsString replacement = TsStringUtils.MakeString(replacementText, ws); + UndoableUnitOfWorkHelper.Do("undo", "redo", Cache.LangProject, () => sel.ReplaceWithTsString(replacement)); + } + + // ------------------------------------------------------------------ + // Angle 1: breadth of the crash across the fragments ComplexConcPatternVc renders. + // Each of these encodes the DESIRED end state (no crash, content unchanged) and must + // fail against current code, which throws NotImplementedException instead. + // ------------------------------------------------------------------ + + [Test] + public void ReplaceWithTsString_OnWordNodeTypeLine_DoesNotThrow() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode(); + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagType); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Type line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), + "a direct edit on the word node's Type line must not crash the view engine"); + } + + [Test] + public void ReplaceWithTsString_OnWordNodeFormLine_DoesNotThrow_AndFormUnchanged() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode { Form = TsStringUtils.MakeString("original", Cache.DefaultVernWs) }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Form line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultVernWs), + "a direct edit on the word node's Form line must not crash the view engine"); + Assert.That(wordNode.Form.Text, Is.EqualTo("original"), + "the synthetic pattern node's Form must not be mutated by a discarded edit"); + } + + [Test] + public void ReplaceWithTsString_OnMorphNodeGlossLine_DoesNotThrow_AndGlossUnchanged() + { + var (model, view) = BuildView(); + var morphNode = new ComplexConcMorphNode { Gloss = TsStringUtils.MakeString("original-gloss", Cache.DefaultAnalWs) }; + model.Root.Children.Add(morphNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagGloss); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Gloss line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), + "a direct edit on the morph node's Gloss line must not crash the view engine"); + Assert.That(morphNode.Gloss.Text, Is.EqualTo("original-gloss"), + "the synthetic pattern node's Gloss must not be mutated by a discarded edit"); + } + + [Test] + public void ReplaceWithTsString_OnMorphNodeEntryLine_DoesNotThrow_AndEntryUnchanged() + { + var (model, view) = BuildView(); + var morphNode = new ComplexConcMorphNode { Entry = TsStringUtils.MakeString("original-entry", Cache.DefaultVernWs) }; + model.Root.Children.Add(morphNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagEntry); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Entry line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultVernWs), + "a direct edit on the morph node's Entry line must not crash the view engine"); + Assert.That(morphNode.Entry.Text, Is.EqualTo("original-entry"), + "the synthetic pattern node's Entry must not be mutated by a discarded edit"); + } + + [Test] + public void ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPartOfSpeechUnrenamed() + { + IPartOfSpeech noun = CreatePartOfSpeech("noun", "N"); + var (model, view) = BuildView(); + var morphNode = new ComplexConcMorphNode { Category = noun }; + model.Root.Children.Add(morphNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagCategory); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Category line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), + "a direct edit on the morph node's Category line must not crash the view engine"); + // This is the specific check for the bug doc's claim that this is a crash, not a + // Bug-1-style corruption: the category line displays a REAL, shared IPartOfSpeech's + // Abbreviation, so if this bug were the same class as Bug 1, a botched edit here + // could rename it project-wide. Confirm it does not. + Assert.That(noun.Abbreviation.BestAnalysisAlternative.Text, Is.EqualTo("N"), + "an edit attempt on the Category line must not rename the real, shared PartOfSpeech"); + } + + [Test] + public void ReplaceWithTsString_OnMorphNodeInflLine_DoesNotThrow() + { + IFsSymFeatVal value; + IFsClosedFeature feature = CreateClosedFeature("num", out value); + var (model, view) = BuildView(); + var morphNode = new ComplexConcMorphNode + { + InflFeatures = { { feature, new ClosedFeatureValue(value, false) } } + }; + model.Root.Children.Add(morphNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagInfl); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Infl Features header line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), + "a direct edit on the morph node's Infl Features header line must not crash the view engine"); + } + + [Test] + public void ReplaceWithTsString_OnTagNodeTagLine_DoesNotThrow_AndRealTagUnrenamed() + { + ICmPossibility tag = CreateTag("Noun Phrase", "NP"); + var (model, view) = BuildView(); + var tagNode = new ComplexConcTagNode { Tag = tag }; + model.Root.Children.Add(tagNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagTag); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Tag line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), + "a direct edit on the tag node's Tag line must not crash the view engine"); + Assert.That(tag.Abbreviation.BestAnalysisAlternative.Text, Is.EqualTo("NP"), + "an edit attempt on the Tag line must not rename the real, shared CmPossibility"); + } + + [Test] + public void ReplaceWithTsString_OnOrNode_DoesNotThrow() + { + var (model, view) = BuildView(); + model.Root.Children.Add(new ComplexConcOrNode()); + model.Root.Children.Add(new ComplexConcWordBdryNode()); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagInnerNonBoundary); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the OR literal"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), + "a direct edit on the OR literal must not crash the view engine"); + } + + [Test] + public void ReplaceWithTsString_OnWordBoundaryNode_DoesNotThrow() + { + var (model, view) = BuildView(); + model.Root.Children.Add(new ComplexConcOrNode()); + model.Root.Children.Add(new ComplexConcWordBdryNode()); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 1, PatternVcBase.ktagInnerNonBoundary); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the '#' literal"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), + "a direct edit on the word-boundary '#' literal must not crash the view engine"); + } + + [Test] + public void ReplaceWithTsString_OnNodeMaximum_DoesNotThrow_AndMaximumUnchanged() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode { Minimum = 0, Maximum = 3 }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagRightNonBoundary); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the max-quantifier line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "9", Cache.DefaultUserWs), + "a direct edit on the max-quantifier line must not crash the view engine"); + Assert.That(wordNode.Maximum, Is.EqualTo(3), + "the synthetic pattern node's Maximum must not be mutated by a discarded edit"); + } + + [Test] + public void ReplaceWithTsString_OnNodeMinimum_DoesNotThrow_AndMinimumUnchanged() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode { Minimum = 0, Maximum = 3 }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagRightBoundary); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the min-quantifier line"); + + Assert.DoesNotThrow(() => AttemptEdit(sel, "9", Cache.DefaultUserWs), + "a direct edit on the min-quantifier line must not crash the view engine"); + Assert.That(wordNode.Minimum, Is.EqualTo(0), + "the synthetic pattern node's Minimum must not be mutated by a discarded edit"); + } + + // ------------------------------------------------------------------ + // Angle 2: is the crash reachable through PatternView's own input handling (keystrokes), + // or only through paths that bypass it (IME composition, drag-and-drop, or any other + // direct ReplaceWithTsString caller)? PatternView.OnKeyPress unconditionally sets + // e.Handled = true and returns without calling base.OnKeyPress for anything but + // Backspace/Delete, so ordinary WM_CHAR-driven typing never reaches the engine at all. + // This test is expected to PASS today: it documents that the keystroke path is already + // safe, which is what makes the ReplaceWithTsString bypass above the actual bug. + // ------------------------------------------------------------------ + + [Test] + public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode { Form = TsStringUtils.MakeString("original", Cache.DefaultVernWs) }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the Form line"); + sel.Install(); + + Assert.DoesNotThrow(() => view.SimulateKeyPress('x'), + "a plain keystroke must not crash PatternView, regardless of the ComplexConcPatternVc.UpdateProp gap"); + Assert.That(wordNode.Form.Text, Is.EqualTo("original"), + "a plain keystroke must not reach the engine and alter content -- PatternView.OnKeyPress swallows it before that"); + } + + // ------------------------------------------------------------------ + // Angle 3: insert/delete must keep working. PatternView.OnKeyDown raises + // RemoveItemsRequested for the Delete key; this must survive whatever fix is applied. + // ------------------------------------------------------------------ + + [Test] + public void DeleteKey_StillRaisesRemoveItemsRequested() + { + var (model, view) = BuildView(); + model.Root.Children.Add(new ComplexConcWordNode()); + view.CallLayout(); + + bool removeRequested = false; + view.RemoveItemsRequested += (sender, e) => removeRequested = true; + + view.SimulateKeyDown(Keys.Delete); + + Assert.That(removeRequested, Is.True, "Delete must still raise RemoveItemsRequested"); + } + } +} From 7d925a8d17011562d134221a147d50e9379a8b59 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 07:39:48 -0400 Subject: [PATCH 2/8] Fix ComplexConcPatternVc direct-edit crash Root cause: ComplexConcPatternVc renders every fragment (feature lines, OR/word-boundary literals, brackets, min/max quantifiers) via AddProp + DisplayVariant -- a computed display over the synthetic pattern node, never a real bound field (zero AddStringAltMember calls, confirmed by inspection). With no UpdateProp override, any edit that reaches the engine without going through PatternView.OnKeyPress fell through to VwBaseVc.UpdateProp, which throws NotImplementedException. Fix, in the layers actually shown to be needed by ablation: - UpdateProp override (necessary and sufficient to stop the crash -- verified by re-running the full failing suite with only this change added: 13/13 pass). It absorbs the edit the same way RuleFormulaVcBase.UpdateProp does; there is nothing to write back to, so the next redraw simply shows the live node state again. - ktptNotEditable on every fragment ComplexConcPatternVc renders (feature lines, Infl Features lines, OR/#, brackets/parens and their pile glyphs, min/max), so an edit is rejected at the selection layer instead of silently reaching UpdateProp. Verified separately: a selection over a marked fragment now reports IsEditable == false. - ReadOnlyView = true on ComplexConcControl's PatternView, the categorical fix for the IME-composition/drag-and-drop path named in the bug report (it unregisters the keyboard/IME controller hook). This needs PatternView.AllowDisplaySelection overridden to stay true, or the chooser-driven selection highlight disappears once ReadOnlyView is set -- SimpleRootSite suppresses Activate() by default for read-only views. PatternView is shared with RuleFormulaControl (Morphology), which still runs with ReadOnlyView = false on this branch, so AllowDisplaySelection is a no-op there today. Delete still raises RemoveItemsRequested with ReadOnlyView = true (verified). All 16 tests in ComplexConcPatternVcDirectEditTests.cs pass, including the 11 that reproduced the crash pre-fix and the 3 new ablation checks. --- .../ComplexConcControl.Designer.cs | 4 +- .../Interlinear/ComplexConcPatternVc.cs | 43 ++++++++++++ .../ComplexConcPatternVcDirectEditTests.cs | 66 ++++++++++++++++++- Src/LexText/LexTextControls/PatternView.cs | 11 ++++ 4 files changed, 121 insertions(+), 3 deletions(-) diff --git a/Src/LexText/Interlinear/ComplexConcControl.Designer.cs b/Src/LexText/Interlinear/ComplexConcControl.Designer.cs index fd16c9b0d1..90154f2d5f 100644 --- a/Src/LexText/Interlinear/ComplexConcControl.Designer.cs +++ b/Src/LexText/Interlinear/ComplexConcControl.Designer.cs @@ -54,7 +54,9 @@ private void InitializeComponent() this.m_view.IsTextBox = false; this.m_view.Mediator = null; this.m_view.Name = "m_view"; - this.m_view.ReadOnlyView = false; + // The pattern builder is modifiable only via chooser-insert and delete, never free + // text; see Docs/bugs/complex-conc-pattern-crash.md. + this.m_view.ReadOnlyView = true; this.m_view.ScrollMinSize = new System.Drawing.Size(0, 0); this.m_view.ScrollPosition = new System.Drawing.Point(0, 0); this.m_view.ShowRangeSelAfterLostFocus = false; diff --git a/Src/LexText/Interlinear/ComplexConcPatternVc.cs b/Src/LexText/Interlinear/ComplexConcPatternVc.cs index 7880bda032..d93e1eca55 100644 --- a/Src/LexText/Interlinear/ComplexConcPatternVc.cs +++ b/Src/LexText/Interlinear/ComplexConcPatternVc.cs @@ -79,6 +79,7 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) { OpenSingleLinePile(vwenv, GetMaxNumLines(vwenv), false); vwenv.Props = m_bracketProps; + SetNotEditable(vwenv); vwenv.AddProp(ComplexConcPatternSda.ktagChildren, this, kfragEmpty); CloseSingleLinePile(vwenv, false); } @@ -100,12 +101,14 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) if (node is ComplexConcOrNode) { OpenSingleLinePile(vwenv, maxNumLines); + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragOR); CloseSingleLinePile(vwenv, false); } else if (node is ComplexConcWordBdryNode) { OpenSingleLinePile(vwenv, maxNumLines); + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragHash); CloseSingleLinePile(vwenv); } @@ -117,10 +120,12 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) { OpenSingleLinePile(vwenv, maxNumLines, false); // use normal parentheses for a single line group + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftBoundary, this, kfragLeftParen); vwenv.AddObjVecItems(ComplexConcPatternSda.ktagChildren, this, kfragNode); + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightBoundary, this, kfragRightParen); if (hasMinMax) DisplayMinMax(numLines, vwenv); @@ -130,6 +135,7 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) { vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginLeading, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, ktagLeftNonBoundary, vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftParenUpHook); @@ -142,6 +148,7 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginTrailing, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightParenUpHook); @@ -161,10 +168,12 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) { OpenSingleLinePile(vwenv, maxNumLines, false); // use normal brackets for a single line constraint + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftBoundary, this, kfragLeftBracket); DisplayFeatures(vwenv, node); + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightBoundary, this, kfragRightBracket); if (hasMinMax) DisplayMinMax(numLines, vwenv); @@ -175,6 +184,7 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) // left bracket pile vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginLeading, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, ktagLeftNonBoundary, vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketUpHook); @@ -193,6 +203,7 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) // right bracket pile vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginTrailing, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightBracketUpHook); @@ -208,6 +219,17 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) } } + /// + /// Every fragment ComplexConcPatternVc renders -- feature lines, quantifiers, OR/word- + /// boundary literals, and the bracket/paren glyphs -- is a computed display, never free + /// text (see UpdateProp). Mark the current run explicitly non-editable so an edit attempt + /// is rejected at the selection layer instead of silently reaching UpdateProp. + /// + private static void SetNotEditable(IVwEnv vwenv) + { + vwenv.set_IntProperty((int) FwTextPropType.ktptEditable, (int) FwTextPropVar.ktpvEnum, (int) TptEditable.ktptNotEditable); + } + private void DisplayMinMax(int numLines, IVwEnv vwenv) { int superOffset = 0; @@ -230,11 +252,13 @@ private void DisplayMinMax(int numLines, IVwEnv vwenv) if (numLines == 1) vwenv.set_IntProperty((int) FwTextPropType.ktptOffset, (int) FwTextPropVar.ktpvMilliPoint, superOffset); vwenv.OpenParagraph(); + SetNotEditable(vwenv); vwenv.AddProp(ktagRightNonBoundary, this, kfragNodeMax); vwenv.CloseParagraph(); AddExtraLines(numLines - 2, ktagRightNonBoundary, vwenv); vwenv.set_IntProperty((int) FwTextPropType.ktptOffset, (int) FwTextPropVar.ktpvMilliPoint, 0); vwenv.OpenParagraph(); + SetNotEditable(vwenv); vwenv.AddProp(ktagRightBoundary, this, kfragNodeMin); vwenv.CloseParagraph(); vwenv.CloseInnerPile(); @@ -382,6 +406,9 @@ public override ITsString DisplayVariant(IVwEnv vwenv, int tag, int frag) private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node) { + // Every line here (Type, Form, Entry, Category, Gloss, Infl Features) is a computed + // summary of the synthetic pattern node, not free text; see UpdateProp and SetNotEditable. + SetNotEditable(vwenv); vwenv.AddProp(ktagType, this, kfragFeatureLine); var morphNode = node as ComplexConcMorphNode; if (morphNode != null) @@ -437,6 +464,7 @@ private void DisplayInflFeatureLines(IVwEnv vwenv, IDictionary lastInflFeatures = m_curInflFeatures; m_curInflFeatures = inflFeatures; + SetNotEditable(vwenv); foreach (KeyValuePair kvp in inflFeatures) { if (kvp.Key is IFsComplexFeature) @@ -462,10 +490,12 @@ private void DisplayInflFeatures(IVwEnv vwenv, IDictionary if (numLines == 1) { // use normal brackets for a single line constraint + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragLeftBracket); DisplayInflFeatureLines(vwenv, inflFeatures, false); + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragRightBracket); } else @@ -473,6 +503,7 @@ private void DisplayInflFeatures(IVwEnv vwenv, IDictionary // left bracket pile vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginLeading, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketUpHook); for (int i = 1; i < numLines - 1; i++) @@ -489,6 +520,7 @@ private void DisplayInflFeatures(IVwEnv vwenv, IDictionary // right bracket pile vwenv.Props = m_bracketProps; vwenv.set_IntProperty((int) FwTextPropType.ktptMarginTrailing, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); + SetNotEditable(vwenv); vwenv.OpenInnerPile(); vwenv.AddProp(ktagInnerNonBoundary, this, kfragRightBracketUpHook); for (int i = 1; i < numLines - 1; i++) @@ -498,6 +530,17 @@ private void DisplayInflFeatures(IVwEnv vwenv, IDictionary } } + /// + /// Every fragment this VC renders is a computed display (Display/DisplayVariant read + /// live node state), not a real bound field, so an edit that reaches this far has + /// nothing to apply. Absorb it and let the next layout redraw the correct value, the + /// same way RuleFormulaVcBase.UpdateProp does for the sibling rule-formula editor. + /// + public override ITsString UpdateProp(IVwSelection vwsel, int hvo, int tag, int frag, ITsString tssVal) + { + return tssVal; + } + public ITsString CreateFeatureLine(ITsString name, ITsString value, bool negated) { ITsIncStrBldr featLine = TsStringUtils.MakeIncStrBldr(); diff --git a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs index 07065ce580..31a459c0b8 100644 --- a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs +++ b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs @@ -17,6 +17,7 @@ // entry point IME composition or drag-and-drop would use, and one PatternView.OnKeyPress never // sees because it only reacts to Windows key events, not to ReplaceWithTsString. using System.Collections.Generic; +using System.Reflection; using System.Windows.Forms; using NUnit.Framework; using SIL.LCModel; @@ -95,6 +96,8 @@ public void SimulateKeyDown(Keys key) { OnKeyDown(new KeyEventArgs(key)); } + + public bool TestAllowDisplaySelection => AllowDisplaySelection; } private IPartOfSpeech CreatePartOfSpeech(string name, string abbr) @@ -407,19 +410,78 @@ public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash() // RemoveItemsRequested for the Delete key; this must survive whatever fix is applied. // ------------------------------------------------------------------ + // ------------------------------------------------------------------ + // Ablation evidence for the fix's layers. + // ------------------------------------------------------------------ + + /// + /// Confirms ComplexConcPatternVc's SetNotEditable calls actually take effect: a + /// selection over a fake-tag fragment must not be editable, independent of whether + /// UpdateProp would otherwise absorb an edit there. + /// + [Test] + public void SelectionOverFormLine_IsNotEditable() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode { Form = TsStringUtils.MakeString("original", Cache.DefaultVernWs) }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + Assert.That(sel, Is.Not.Null); + Assert.That(sel.IsEditable, Is.False, + "the Form line's fragment must be marked ktptNotEditable, not merely absorbed by UpdateProp"); + } + + /// + /// ComplexConcControl.Designer.cs must wire up the pattern-builder view as read-only + /// (this is the categorical fix for the IME-composition/keyboard-controller-registration + /// path named in the bug report, distinct from the per-fragment ktptEditable markings). + /// + [Test] + public void ComplexConcControl_WiresViewAsReadOnly() + { + using (var control = new ComplexConcControl()) + { + var viewField = typeof(ComplexConcControl).GetField("m_view", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.That(viewField, Is.Not.Null, "test assumption: ComplexConcControl has a private m_view field"); + var view = (PatternView) viewField.GetValue(control); + Assert.That(view.ReadOnlyView, Is.True, + "ComplexConcControl must wire up its PatternView with ReadOnlyView = true"); + } + } + + /// + /// PatternView.AllowDisplaySelection must be overridden to stay true even when + /// ReadOnlyView is true, or the pattern-builder's chooser-driven selection highlight + /// would disappear once ReadOnlyView is turned on (SimpleRootSite suppresses Activate() + /// by default for read-only views). + /// + [Test] + public void AllowDisplaySelection_IsTrue_WhenRootsiteIsReadOnly() + { + var (model, view) = BuildView(); + view.ReadOnlyView = true; + + Assert.That(view.TestAllowDisplaySelection, Is.True, + "the selection must still be shown even though the rootsite is read-only"); + } + [Test] - public void DeleteKey_StillRaisesRemoveItemsRequested() + public void DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly() { var (model, view) = BuildView(); model.Root.Children.Add(new ComplexConcWordNode()); view.CallLayout(); + view.ReadOnlyView = true; bool removeRequested = false; view.RemoveItemsRequested += (sender, e) => removeRequested = true; view.SimulateKeyDown(Keys.Delete); - Assert.That(removeRequested, Is.True, "Delete must still raise RemoveItemsRequested"); + Assert.That(removeRequested, Is.True, + "Delete must still raise RemoveItemsRequested now that ComplexConcControl wires the view as ReadOnlyView = true"); } } } diff --git a/Src/LexText/LexTextControls/PatternView.cs b/Src/LexText/LexTextControls/PatternView.cs index 27bbb76eca..f1de0146b7 100644 --- a/Src/LexText/LexTextControls/PatternView.cs +++ b/Src/LexText/LexTextControls/PatternView.cs @@ -69,6 +69,17 @@ protected override EditingHelper CreateEditingHelper() return new PatternEditingHelper(Cache, this); } + /// + /// Activate() is suppressed by default in ReadOnlyViews (SimpleRootSite.AllowDisplaySelection + /// defaults to IsEditable), but both PatternView consumers are pattern builders whose chooser + /// insert/delete needs the user to see the current selection even when the view itself is + /// read-only. + /// + protected override bool AllowDisplaySelection + { + get { return true; } + } + public void Init(Mediator mediator, PropertyTable propertyTable, int hvo, IPatternControl patternControl, PatternVcBase vc, int rootFrag, ISilDataAccess sda) { CheckDisposed(); From 5effda1ba52f1731e3f4cbc5ae8f2d9b0f333ca5 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 07:46:32 -0400 Subject: [PATCH 3/8] Add architecture self-review for the ComplexConcPatternVc crash fix Documents: this is a different failure mode than the sibling rule-formula bug (UpdateProp alone is necessary and sufficient here, confirmed by ablation, because no fragment binds a real field); what was deliberately not hoisted to PatternVcBase and why; why PatternEditingHelper.CanCut/ CanPaste were left alone (branch-topology reason, noted for whoever integrates both branches); and what still needs manual verification in a running FLEx (IME, drag-and-drop, selection visibility, Delete). --- .../bugs/complex-conc-pattern-crash-review.md | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 Docs/bugs/complex-conc-pattern-crash-review.md diff --git a/Docs/bugs/complex-conc-pattern-crash-review.md b/Docs/bugs/complex-conc-pattern-crash-review.md new file mode 100644 index 0000000000..12e9da2892 --- /dev/null +++ b/Docs/bugs/complex-conc-pattern-crash-review.md @@ -0,0 +1,159 @@ +# Review: ComplexConcPatternVc direct-edit crash (architecture self-review) + +## 1. Is this the right architecture? + +The invariant is the same one identified by the sibling bug's review: "a pattern view is +not free text; it changes only via chooser-insert and delete." That invariant has the +same two independent parts here: + +- **"This fragment is a computed display, not a bound field, and must not accept an + edit."** `ComplexConcPatternVc` binds no real domain object field at all -- a grep for + `AddStringAltMember` in `ComplexConcPatternVc.cs` returns zero matches, confirmed by + inspection and by `ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPartOfSpeechUnrenamed` + and `..._OnTagNodeTagLine_..._AndRealTagUnrenamed`, which specifically check that the + real, shared `IPartOfSpeech`/`ICmPossibility` referenced by a node is untouched after an + edit attempt. Every fragment is `AddProp` + `DisplayVariant` over the synthetic + `ComplexConcPatternNode` tree, so there is nothing to corrupt project-wide -- but there + was also nothing absorbing the edit, so it fell through to `VwBaseVc.UpdateProp` + (`NotImplementedException`, unhandled). +- **"This widget only accepts chooser-insert and delete, never typed input."** A + control-level fact, fixed the same way as the sibling: `ComplexConcControl` now sets + `m_view.ReadOnlyView = true` instead of `false`. + +**This bug is not the same failure mode as the sibling's, and the ablation evidence shows +it plainly.** For the sibling (`RuleFormulaVcBase`), `ktptEditable` was the *only* +load-bearing layer, because `AddStringAltMember`-bound fragments write straight to the +real property without ever calling `UpdateProp` -- the sibling's own review documents this +(removing `ktptEditable` alone reproduced the corruption). Here, re-running the entire +failing test suite with *only* the `UpdateProp` override added (no `ktptEditable` +markings, no `ReadOnlyView` change) turned all 13 tests green. `UpdateProp` alone is +necessary and sufficient to stop the crash, because there is no real property for an edit +to land on -- `UpdateProp` returning `tssVal` unchanged is a true no-op relative to model +state, not a mitigation racing a live write. + +Given that, the `ktptNotEditable` markings and `ReadOnlyView = true` added here are +**not required to stop the crash** -- they are added anyway, for reasons independent of +the crash: + +- `ktptNotEditable` on every fragment (feature lines, `Infl Features` header, `OR`/`#`, + every bracket/paren glyph including the multi-line pile hooks, and both quantifier + lines) rejects an edit at the selection layer instead of letting it reach `UpdateProp` + at all. This matters because `UpdateProp`'s no-op is only a no-op *for the model*; it is + not obviously a no-op for the view's own display cache -- returning `tssVal` from + `UpdateProp` is how a VC normally *approves* a display update, and nothing in this + codebase demonstrates that the specific cache entry backing a fake, negative flid + (`ktagType` et al., never registered with `ISilDataAccess`) is invalidated correctly + without a full `Reconstruct`. Marking the fragments non-editable removes the need to + trust that path at all. `SelectionOverFormLine_IsNotEditable` confirms the marking + actually takes hold (`sel.IsEditable == false`). +- `ReadOnlyView = true` is the categorical fix for the trigger the bug report actually + named: IME composition and drag-and-drop, which do not go through + `PatternView.OnKeyPress`. `ReadOnlyView` unregisters the keyboard/IME controller hook + (`SimpleRootSite`); `ktptEditable` does not gate that registration, only whether a + `ReplaceWithTsString`-style edit is accepted once something reaches the view. + `ComplexConcControl_WiresViewAsReadOnly` checks the real Designer-generated wiring, not + just a synthetic test harness. +- `PatternView.AllowDisplaySelection` had to be added (it did not previously exist on + this branch) because `ReadOnlyView = true` suppresses `Activate()` by default + (`SimpleRootSite.AllowDisplaySelection` defaults to `IsEditable`), which would hide the + selection the chooser insert/delete buttons need the user to see. + +**Should the invariant live in `PatternVcBase`/`PatternView` itself, so a third subclass +can't reintroduce this?** Yes, partially, and I did not do the full version here. Two +independent things could move: + +1. **`UpdateProp` could have a base-class default that returns `tssVal` instead of + throwing.** `PatternVcBase` doesn't override `VwBaseVc.UpdateProp` at all today, so + the *unimplemented* default is inherited from `VwBaseVc`. A `PatternVcBase.UpdateProp` + override doing exactly what both subclasses currently do by hand (`RuleFormulaVcBase` + and now `ComplexConcPatternVc`) would mean a third subclass gets the safe behavior + automatically instead of needing to remember to add it. **I did not make this change.** + It only affects the crash-avoidance half of the story (which, per the ablation above, + is the *entire* story for this bug but was *not* sufficient for the sibling's, where + `ktptEditable` was load-bearing) -- moving it to the base class doesn't, by itself, + protect a future subclass that binds a real field via `AddStringAltMember`, which is + the actually dangerous case. Given `ComplexConcPatternVc.UpdateProp` and + `RuleFormulaVcBase.UpdateProp` are now textually identical one-liners, hoisting it is + almost pure duplication removal with no behavior change for either existing subclass -- + a safe follow-up, but it touches `RuleFormulaVcBase.cs` on the sibling's own unmerged + branch, so I left it as a documented recommendation rather than doing it here to avoid + a cross-branch collision on a file I don't own in this task. +2. **The "not free text" fragment-marking discipline cannot be hoisted as cheaply.** + Marking editability is inherently per-fragment (each subclass alone knows which of its + own `AddProp`/`AddStringAltMember` calls binds to a mutable channel), so there's no + single base-class change that forces a third subclass to mark its fragments correctly. + The closest structural guard I can identify without redesigning the VC pattern: give + `PatternVcBase` a protected helper (`MarkNotEditable(vwenv)`, effectively what + `SetNotEditable` is here) so at least the *mechanism* is shared and discoverable, and + have `PatternVcBase.AddExtraLines` (which already does this for filler lines) serve as + the precedent a new subclass's author is likely to copy. I did not hoist `SetNotEditable` + itself, since it is a one-line wrapper and duplicating it costs less than adding + cross-subclass coupling for a helper this small; I would reconsider if a third + subclass appears. + +`ReadOnlyView`/`AllowDisplaySelection` are already control-level and already shared +(`PatternView`), so nothing further to hoist there -- the risk is a future `PatternView` +consumer forgetting to set `ReadOnlyView = true` on its own control, which is a Designer.cs +wiring mistake no base-class change can prevent. + +## 2. What can be removed or simplified? + +Nothing was removed. The specific candidate, per the task brief, was +`PatternView.PatternEditingHelper.CanCut()`/`CanPaste()` +(`Src/LexText/LexTextControls/PatternView.cs`), which look redundant now that +`ComplexConcControl` also runs with `ReadOnlyView = true` (matching `RuleFormulaControl`'s +state *on the sibling's own branch*). They were **not** removed here, for a +branch-topology reason rather than a functional one: on *this* branch, +`RuleFormulaControl.Designer.cs` still sets `ReadOnlyView = false` (the sibling's flip to +`true` lives only on the unmerged `fix/phon-rule-formula-readonly` branch). From this +branch's point of view, the override is still load-bearing for the rule-formula editor, +so removing it now would reduce coverage for a consumer this task did not touch. This is +the flip side of the sibling's own review note ("kept because `ComplexConcControl` still +depends on it") -- once both branches are integrated and *both* consumers set +`ReadOnlyView = true`, `CanCut`/`CanPaste` become fully redundant with +`EditingHelper.CanCut`/`CanPaste`'s own `Editable`-gated base behavior, and should be +removed then. **Noting this for whoever integrates both branches, per the task brief, +rather than editing `fix/phon-rule-formula-readonly`.** `CanCopy()` stays regardless: the +base implementation doesn't consult `Editable`, so it was never redundant. + +## 3. What was not fixed, and why + +- **Zero-width-space boundary markers** (`PatternVcBase.OpenSingleLinePile`/ + `CloseSingleLinePile`, `kfragZeroWidthSpace` on `ktagLeftBoundary`/`ktagRightBoundary`) + are not individually marked `ktptNotEditable`, in either `PatternVcBase` subclass. This + is a pre-existing, shared gap the sibling's own review flagged and left open for the + same reason: these are invisible cursor-parking glyphs used for boundary navigation, not + literal or bound content, and `UpdateProp` (now overridden in both subclasses) absorbs + any edit attempt there harmlessly. Confirmed by reasoning, not by a dedicated test -- + I did not add one, since it would exercise the identical `UpdateProp` no-op path already + covered by the eleven fragment tests, not a new mechanism. +- **`PatternVcBase.UpdateProp` base-class hoist** -- see section 1. Left as a + recommendation, not implemented, to avoid touching `RuleFormulaVcBase.cs` on a branch I + don't own. +- **`ConstChartVc.cs:297`** -- out of scope for this bug; already tracked as + SUSPECTED-safe-by-reading-only in `phon-rule-direct-editing.md`, unchanged by this work. +- **Live IME/drag-and-drop reproduction** -- as with the sibling bug, this was inferred + (the bug report itself says "if input reaches the view without passing through + PatternView.OnKeyPress"), not reproduced with a real IME or a real OS-level drag + operation. `ReadOnlyView = true` closing the keyboard-controller registration is the + categorical fix for that path; see section 4 for what still needs a live check. + +## 4. What needs manual verification in a running FLEx + +- Open Texts & Words -> Complex Concordance, build a pattern with at least one Word and + one Morph node with Form/Gloss/Category/Entry/Infl Features all populated, and confirm + the pattern builder still renders identically to before this change (no visual + regression from the `ktptEditable`/`ReadOnlyView` changes). +- With a vernacular IME active, place focus in the pattern builder and attempt to compose + and commit text into a feature line. Confirm composition does not commit into the pane + at all (rather than committing and then silently reverting) -- this is what + `ReadOnlyView = true` should prevent categorically. +- Confirm the selection highlight is still visible when a chooser-inserted item is + selected (this is what `PatternView.AllowDisplaySelection` restores), and that the + Insert/Search controls still operate against the right selection. +- Confirm Delete still removes the selected item in the live UI, matching + `DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly`. +- Try dragging text onto the pattern-builder pane; confirm it is rejected/does nothing, + rather than being accepted and then not visibly changing anything (the two are + distinguishable to a user who's watching the drop target, even though neither corrupts + data). From 15c0b1ff5b170af7962f7dab752998ec9c957010 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 08:41:49 -0400 Subject: [PATCH 4/8] Remove working-doc references from source comments Docs/bugs/*.md are working documents that get evicted from the branch when the PR is written; a source comment pointing at one would dangle. Rewrite the ComplexConcControl.Designer.cs comment and the test file header to state the reasoning inline instead of citing the doc. --- .../Interlinear/ComplexConcControl.Designer.cs | 3 ++- .../ComplexConcPatternVcDirectEditTests.cs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/Src/LexText/Interlinear/ComplexConcControl.Designer.cs b/Src/LexText/Interlinear/ComplexConcControl.Designer.cs index 90154f2d5f..a03e049698 100644 --- a/Src/LexText/Interlinear/ComplexConcControl.Designer.cs +++ b/Src/LexText/Interlinear/ComplexConcControl.Designer.cs @@ -55,7 +55,8 @@ private void InitializeComponent() this.m_view.Mediator = null; this.m_view.Name = "m_view"; // The pattern builder is modifiable only via chooser-insert and delete, never free - // text; see Docs/bugs/complex-conc-pattern-crash.md. + // text: content is entirely computed from the synthetic pattern-node tree, so a + // typed or IME-composed edit has nothing valid to apply. this.m_view.ReadOnlyView = true; this.m_view.ScrollMinSize = new System.Drawing.Size(0, 0); this.m_view.ScrollPosition = new System.Drawing.Point(0, 0); diff --git a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs index 31a459c0b8..d379c6afbe 100644 --- a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs +++ b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs @@ -2,15 +2,15 @@ // This software is licensed under the LGPL, version 2.1 or later // (http://www.gnu.org/licenses/lgpl-2.1.html) // -// Reproduction and regression coverage for the Complex Concordance pattern-builder crash -// (Docs/bugs/complex-conc-pattern-crash.md). ComplexConcControl and the phonological rule -// formula editor share PatternView/PatternVcBase. ComplexConcPatternVc has no UpdateProp -// override, so an edit that reaches the view engine without passing through -// PatternView.OnKeyPress (IME composition, drag-and-drop, or any direct -// IVwSelection.ReplaceWithTsString call) falls through to VwBaseVc.UpdateProp, which throws -// NotImplementedException. Unlike the sibling rule-formula bug, ComplexConcPatternVc binds no -// real domain fields via AddStringAltMember (verified by inspection: zero occurrences in -// ComplexConcPatternVc.cs), so this is a crash, not a silent corruption/rename. +// Reproduction and regression coverage for the Complex Concordance pattern-builder crash. +// ComplexConcControl and the phonological rule formula editor share PatternView/ +// PatternVcBase. ComplexConcPatternVc has no UpdateProp override, so an edit that reaches +// the view engine without passing through PatternView.OnKeyPress (IME composition, +// drag-and-drop, or any direct IVwSelection.ReplaceWithTsString call) falls through to +// VwBaseVc.UpdateProp, which throws NotImplementedException. Unlike the sibling rule-formula +// bug, ComplexConcPatternVc binds no real domain fields via AddStringAltMember (verified by +// inspection: zero occurrences in ComplexConcPatternVc.cs), so this is a crash, not a silent +// corruption/rename. // // These tests drive a real IVwRootBox (PatternView/ComplexConcPatternVc) against a real // in-memory LcmCache and call IVwSelection.ReplaceWithTsString directly -- the same low-level From 9fa6dac1874b49d917ab5a937624411d443e516c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 11:30:05 -0400 Subject: [PATCH 5/8] Fix inert fragment-selection test helper; fix ktptEditable persistence bug it exposed Adversarial review found MakeSelOnChild inert: it passed the requested tag into MakeTextSelInObj's unused cvsliEnd slot with fWholeObj: true, so 9 of 11 "fragment angle" tests were all the same whole-object range selection in disguise (anchored at the outermost boundary glyph, not the named fragment). Replaced it with MakeSelOnFragment, built on IVwRootBox.MakeTextSelection with the tag passed as tagTextProp -- the same API PatternView.SelectLeftBoundary/SelectRightBoundary already use to target one fake-tag property on an object. Every fragment test now carries an AssertSelectionTargets(sel, expectedTag) check via IVwSelection.TextSelInfo, so a future regression in targeting fails loudly instead of silently passing. Added MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode as a standing proof that three different tags on one node resolve to three different selections. Fixing the helper exposed a real production bug: ComplexConcPatternVc's SetNotEditable(vwenv) calls do not persist across multiple AddProp calls the way vwenv.Props = someBuilder does -- ktptEditable must be re-asserted immediately before *every* AddProp, matching the convention PatternVcBase.AddExtraLines and RuleFormulaVcBase already use. Most of the fix's ktptEditable markings were only accidentally correct for single-AddProp call sites (OR, #, min/max); the feature lines (Form/Entry/Category/Gloss/Infl) and the multi-line bracket/paren pile-hook sequences were not actually protected. Fixed by re-asserting SetNotEditable before each individual AddProp in DisplayFeatures, DisplayInflFeatureLines, DisplayInflFeatures, and the four pile-hook sequences. Also fixed a second, genuinely unmarked gap found by mutation testing: PatternVcBase.OpenSingleLinePile/CloseSingleLinePile's zero-width-space boundary run (shared by both PatternVcBase subclasses) had no ktptEditable marking at all. With UpdateProp removed, an edit on this run still threw NotImplementedException. Marked it NotEditable and pinned it with SelectionOverZeroWidthBoundaryRun_IsNotEditable, verified red (fails without the marking, confirmed by temporarily reverting it) and green (passes with it). Corrected ablation with the fixed helper (18-test suite): UpdateProp alone (ktptEditable neutralized) = 16/18 (only the two IsEditable-assertion tests fail; every crash/content test still passes). ktptEditable alone (UpdateProp removed) = 18/18. Both are independently sufficient to stop the crash, unlike the sibling bug where ktptEditable was the only load-bearing layer. But a direct probe (UpdateProp intact, ktptEditable neutralized) showed UpdateProp's no-op leaves the *displayed* text stale/corrupted ("HACKEDForm: original" instead of "Form: original") until an explicit Reconstruct -- ktptEditable prevents this because the edit never reaches that point. Both layers are needed, not for redundant crash-prevention, but because they prevent two different failure modes. Also added ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse, pinning the actual behaviour change from ReadOnlyView = true: AcceptsTab was already false before this fix (unchanged); AcceptsReturn flips from true to false (new). --- .../Interlinear/ComplexConcPatternVc.cs | 59 +++++- .../ComplexConcPatternVcDirectEditTests.cs | 171 ++++++++++++++++-- Src/LexText/LexTextControls/PatternVcBase.cs | 4 + 3 files changed, 217 insertions(+), 17 deletions(-) diff --git a/Src/LexText/Interlinear/ComplexConcPatternVc.cs b/Src/LexText/Interlinear/ComplexConcPatternVc.cs index d93e1eca55..2e47dc2c7a 100644 --- a/Src/LexText/Interlinear/ComplexConcPatternVc.cs +++ b/Src/LexText/Interlinear/ComplexConcPatternVc.cs @@ -138,9 +138,14 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, ktagLeftNonBoundary, vwenv); + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftParenUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftParenExt); + } + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftBoundary, this, kfragLeftParenLowHook); vwenv.CloseInnerPile(); @@ -151,9 +156,14 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, vwenv); + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightParenUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightParenExt); + } + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightBoundary, this, kfragRightParenLowHook); vwenv.CloseInnerPile(); if (hasMinMax) @@ -187,9 +197,14 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, ktagLeftNonBoundary, vwenv); + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketExt); + } + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftBoundary, this, kfragLeftBracketLowHook); vwenv.CloseInnerPile(); @@ -206,9 +221,14 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) SetNotEditable(vwenv); vwenv.OpenInnerPile(); AddExtraLines(maxNumLines - numLines, hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, vwenv); + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightBracketUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightNonBoundary, this, kfragRightBracketExt); + } + SetNotEditable(vwenv); vwenv.AddProp(hasMinMax ? ktagInnerNonBoundary : ktagRightBoundary, this, kfragRightBracketLowHook); vwenv.CloseInnerPile(); if (hasMinMax) @@ -414,16 +434,29 @@ private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node) if (morphNode != null) { if (morphNode.Form != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagForm, this, kfragFeatureLine); + } if (morphNode.Entry != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagEntry, this, kfragFeatureLine); + } if (morphNode.Category != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagCategory, this, kfragFeatureLine); + } if (morphNode.Gloss != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagGloss, this, kfragFeatureLine); + } if (morphNode.InflFeatures.Count > 0) { vwenv.OpenParagraph(); + SetNotEditable(vwenv); vwenv.AddProp(ktagInfl, this, kfragFeatureLine); DisplayInflFeatures(vwenv, morphNode.InflFeatures); vwenv.CloseParagraph(); @@ -435,14 +468,24 @@ private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node) if (wordNode != null) { if (wordNode.Form != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagForm, this, kfragFeatureLine); + } if (wordNode.Category != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagCategory, this, kfragFeatureLine); + } if (wordNode.Gloss != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagGloss, this, kfragFeatureLine); + } if (wordNode.InflFeatures.Count > 0) { vwenv.OpenParagraph(); + SetNotEditable(vwenv); vwenv.AddProp(ktagInfl, this, kfragFeatureLine); DisplayInflFeatures(vwenv, wordNode.InflFeatures); vwenv.CloseParagraph(); @@ -454,7 +497,10 @@ private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node) if (tagNode != null) { if (tagNode.Tag != null) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagTag, this, kfragFeatureLine); + } } } } @@ -464,13 +510,13 @@ private void DisplayInflFeatureLines(IVwEnv vwenv, IDictionary lastInflFeatures = m_curInflFeatures; m_curInflFeatures = inflFeatures; - SetNotEditable(vwenv); foreach (KeyValuePair kvp in inflFeatures) { if (kvp.Key is IFsComplexFeature) { if (openPara) vwenv.OpenParagraph(); + SetNotEditable(vwenv); vwenv.AddProp(kvp.Key.Hvo, this, kfragFeatureLine); DisplayInflFeatures(vwenv, (IDictionary) kvp.Value); if (openPara) @@ -478,6 +524,7 @@ private void DisplayInflFeatureLines(IVwEnv vwenv, IDictionary vwenv.set_IntProperty((int) FwTextPropType.ktptMarginLeading, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); SetNotEditable(vwenv); vwenv.OpenInnerPile(); + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftNonBoundary, this, kfragLeftBracketExt); + } + SetNotEditable(vwenv); vwenv.AddProp(ktagLeftBoundary, this, kfragLeftBracketLowHook); vwenv.CloseInnerPile(); @@ -522,9 +574,14 @@ private void DisplayInflFeatures(IVwEnv vwenv, IDictionary vwenv.set_IntProperty((int) FwTextPropType.ktptMarginTrailing, (int) FwTextPropVar.ktpvMilliPoint, PileMargin); SetNotEditable(vwenv); vwenv.OpenInnerPile(); + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragRightBracketUpHook); for (int i = 1; i < numLines - 1; i++) + { + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragRightBracketExt); + } + SetNotEditable(vwenv); vwenv.AddProp(ktagInnerNonBoundary, this, kfragRightBracketLowHook); vwenv.CloseInnerPile(); } diff --git a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs index d379c6afbe..1790a990f9 100644 --- a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs +++ b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs @@ -165,14 +165,46 @@ private IFsClosedFeature CreateClosedFeature(string name, out IFsSymFeatVal valu return (model, view); } - private static IVwSelection MakeSelOnChild(TestPatternView view, int childIndex, int tag) + /// + /// Selects the fragment rendered under fake tag on the child + /// node at , via IVwRootBox.MakeTextSelection with + /// passed as tagTextProp -- the same call PatternView itself uses + /// (PatternView.SelectLeftBoundary/SelectRightBoundary) to target one specific fake-tag + /// property on an object, as opposed to the whole object's rendering. + /// + /// MakeTextSelInObj (the API the original version of this helper used) does NOT take a + /// tag argument at all: its signature is (ihvoRoot, cvsli, rgvsli, cvsliEnd, rgvsliEnd, + /// fInitial, fEdit, fRange, fWholeObj, fInstall). The previous version of this helper + /// passed `tag` into the cvsliEnd slot and set fWholeObj: true, which per the documented + /// contract (Views.cs "If fWholeObject is true, these arguments are not used") made that + /// argument dead and made every call select the same thing: the whole child object's + /// rendering, anchored at its outermost boundary glyph. Nine of the eleven "fragment + /// angle" tests that used it were therefore the same selection in disguise -- see + /// MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode below for the proof this + /// version actually targets the requested tag. + /// + private static IVwSelection MakeSelOnFragment(TestPatternView view, int childIndex, int tag, int ich = 0) { var levels = new[] { new SelLevInfo { tag = ComplexConcPatternSda.ktagChildren, ihvo = childIndex } }; - return view.RootBox.MakeTextSelInObj(0, levels.Length, levels, tag, null, true, false, false, - /* fWholeObj */ true, /* fInstall */ true); + return view.RootBox.MakeTextSelection(0, levels.Length, levels, tag, 0, ich, ich, 0, false, -1, null, true); + } + + /// + /// Fails the test with a diagnostic if does not actually target + /// -- proof, not assumption, that a fragment test is + /// exercising the fragment it claims to. + /// + private static void AssertSelectionTargets(IVwSelection sel, int expectedTag) + { + ITsString tss; + int ich, hvo, tag, ws; + bool fAssocPrev; + sel.TextSelInfo(false, out tss, out ich, out fAssocPrev, out hvo, out tag, out ws); + Assert.That(tag, Is.EqualTo(expectedTag), + $"selection did not target the requested tag {expectedTag}; got tag {tag} (text '{tss?.Text}')"); } private void AttemptEdit(IVwSelection sel, string replacementText, int ws) @@ -181,6 +213,44 @@ private void AttemptEdit(IVwSelection sel, string replacementText, int ws) UndoableUnitOfWorkHelper.Do("undo", "redo", Cache.LangProject, () => sel.ReplaceWithTsString(replacement)); } + /// + /// Proof that MakeSelOnFragment actually discriminates between fragments, rather than + /// resolving to the same selection regardless of the requested tag (the bug an + /// adversarial review found in this helper's first version, which used + /// IVwRootBox.MakeTextSelInObj with fWholeObj: true -- see MakeSelOnFragment's doc + /// comment). One node with three populated fields is selected by three different tags; + /// each selection must land on its own distinct tag and text, not all collapse onto one + /// (e.g. the node's outermost boundary glyph). + /// + [Test] + public void MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode() + { + var (model, view) = BuildView(); + var wordNode = new ComplexConcWordNode + { + Form = TsStringUtils.MakeString("myform", Cache.DefaultVernWs), + Gloss = TsStringUtils.MakeString("myGloss", Cache.DefaultAnalWs) + }; + model.Root.Children.Add(wordNode); + view.CallLayout(); + + IVwSelection selType = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagType); + IVwSelection selForm = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagForm); + IVwSelection selGloss = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagGloss); + + AssertSelectionTargets(selType, ComplexConcPatternVc.ktagType); + AssertSelectionTargets(selForm, ComplexConcPatternVc.ktagForm); + AssertSelectionTargets(selGloss, ComplexConcPatternVc.ktagGloss); + + ITsString tss; int ich, hvo, tag, ws; bool fAssocPrev; + selType.TextSelInfo(false, out tss, out ich, out fAssocPrev, out hvo, out tag, out ws); + Assert.That(tss.Text, Is.EqualTo("Type: Word")); + selForm.TextSelInfo(false, out tss, out ich, out fAssocPrev, out hvo, out tag, out ws); + Assert.That(tss.Text, Is.EqualTo("Form: myform")); + selGloss.TextSelInfo(false, out tss, out ich, out fAssocPrev, out hvo, out tag, out ws); + Assert.That(tss.Text, Is.EqualTo("Gloss: myGloss")); + } + // ------------------------------------------------------------------ // Angle 1: breadth of the crash across the fragments ComplexConcPatternVc renders. // Each of these encodes the DESIRED end state (no crash, content unchanged) and must @@ -195,8 +265,9 @@ public void ReplaceWithTsString_OnWordNodeTypeLine_DoesNotThrow() model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagType); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagType); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Type line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagType); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), "a direct edit on the word node's Type line must not crash the view engine"); @@ -210,8 +281,9 @@ public void ReplaceWithTsString_OnWordNodeFormLine_DoesNotThrow_AndFormUnchanged model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagForm); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Form line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagForm); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultVernWs), "a direct edit on the word node's Form line must not crash the view engine"); @@ -227,8 +299,9 @@ public void ReplaceWithTsString_OnMorphNodeGlossLine_DoesNotThrow_AndGlossUnchan model.Root.Children.Add(morphNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagGloss); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagGloss); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Gloss line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagGloss); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), "a direct edit on the morph node's Gloss line must not crash the view engine"); @@ -244,8 +317,9 @@ public void ReplaceWithTsString_OnMorphNodeEntryLine_DoesNotThrow_AndEntryUnchan model.Root.Children.Add(morphNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagEntry); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagEntry); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Entry line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagEntry); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultVernWs), "a direct edit on the morph node's Entry line must not crash the view engine"); @@ -262,8 +336,9 @@ public void ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPart model.Root.Children.Add(morphNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagCategory); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagCategory); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Category line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagCategory); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), "a direct edit on the morph node's Category line must not crash the view engine"); @@ -288,8 +363,9 @@ public void ReplaceWithTsString_OnMorphNodeInflLine_DoesNotThrow() model.Root.Children.Add(morphNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagInfl); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagInfl); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Infl Features header line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagInfl); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), "a direct edit on the morph node's Infl Features header line must not crash the view engine"); @@ -304,8 +380,9 @@ public void ReplaceWithTsString_OnTagNodeTagLine_DoesNotThrow_AndRealTagUnrename model.Root.Children.Add(tagNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagTag); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagTag); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Tag line"); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagTag); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), "a direct edit on the tag node's Tag line must not crash the view engine"); @@ -321,8 +398,9 @@ public void ReplaceWithTsString_OnOrNode_DoesNotThrow() model.Root.Children.Add(new ComplexConcWordBdryNode()); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagInnerNonBoundary); + IVwSelection sel = MakeSelOnFragment(view, 0, PatternVcBase.ktagInnerNonBoundary); Assert.That(sel, Is.Not.Null, "could not construct a selection over the OR literal"); + AssertSelectionTargets(sel, PatternVcBase.ktagInnerNonBoundary); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), "a direct edit on the OR literal must not crash the view engine"); @@ -336,8 +414,9 @@ public void ReplaceWithTsString_OnWordBoundaryNode_DoesNotThrow() model.Root.Children.Add(new ComplexConcWordBdryNode()); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 1, PatternVcBase.ktagInnerNonBoundary); + IVwSelection sel = MakeSelOnFragment(view, 1, PatternVcBase.ktagInnerNonBoundary); Assert.That(sel, Is.Not.Null, "could not construct a selection over the '#' literal"); + AssertSelectionTargets(sel, PatternVcBase.ktagInnerNonBoundary); Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultUserWs), "a direct edit on the word-boundary '#' literal must not crash the view engine"); @@ -351,8 +430,9 @@ public void ReplaceWithTsString_OnNodeMaximum_DoesNotThrow_AndMaximumUnchanged() model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagRightNonBoundary); + IVwSelection sel = MakeSelOnFragment(view, 0, PatternVcBase.ktagRightNonBoundary); Assert.That(sel, Is.Not.Null, "could not construct a selection over the max-quantifier line"); + AssertSelectionTargets(sel, PatternVcBase.ktagRightNonBoundary); Assert.DoesNotThrow(() => AttemptEdit(sel, "9", Cache.DefaultUserWs), "a direct edit on the max-quantifier line must not crash the view engine"); @@ -368,8 +448,9 @@ public void ReplaceWithTsString_OnNodeMinimum_DoesNotThrow_AndMinimumUnchanged() model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, PatternVcBase.ktagRightBoundary); + IVwSelection sel = MakeSelOnFragment(view, 0, PatternVcBase.ktagRightBoundary); Assert.That(sel, Is.Not.Null, "could not construct a selection over the min-quantifier line"); + AssertSelectionTargets(sel, PatternVcBase.ktagRightBoundary); Assert.DoesNotThrow(() => AttemptEdit(sel, "9", Cache.DefaultUserWs), "a direct edit on the min-quantifier line must not crash the view engine"); @@ -395,7 +476,7 @@ public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash() model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagForm); Assert.That(sel, Is.Not.Null, "could not construct a selection over the Form line"); sel.Install(); @@ -427,12 +508,38 @@ public void SelectionOverFormLine_IsNotEditable() model.Root.Children.Add(wordNode); view.CallLayout(); - IVwSelection sel = MakeSelOnChild(view, 0, ComplexConcPatternVc.ktagForm); + IVwSelection sel = MakeSelOnFragment(view, 0, ComplexConcPatternVc.ktagForm); Assert.That(sel, Is.Not.Null); + AssertSelectionTargets(sel, ComplexConcPatternVc.ktagForm); Assert.That(sel.IsEditable, Is.False, "the Form line's fragment must be marked ktptNotEditable, not merely absorbed by UpdateProp"); } + /// + /// PatternVcBase.OpenSingleLinePile/CloseSingleLinePile add a 1-char zero-width-space + /// boundary run (tag ktagLeftBoundary/ktagRightBoundary, frag kfragZeroWidthSpace) around + /// single-line piles, with no ktptEditable marking at all -- an unmarked gap shared by + /// both PatternVcBase subclasses, found by mutation testing: with UpdateProp removed and + /// every other guard left in place, an edit on this run still threw + /// NotImplementedException, because nothing else was rejecting it at the selection layer. + /// This test fails without the ktptEditable marking added to OpenSingleLinePile/ + /// CloseSingleLinePile (verified by temporarily reverting it) and passes with it. + /// + [Test] + public void SelectionOverZeroWidthBoundaryRun_IsNotEditable() + { + var (model, view) = BuildView(); + model.Root.Children.Add(new ComplexConcOrNode()); + view.CallLayout(); + + // The 1-char ZWSP boundary run precedes OR in the same paragraph, at ich=0. + IVwSelection sel = MakeSelOnFragment(view, 0, PatternVcBase.ktagLeftBoundary, 0); + Assert.That(sel, Is.Not.Null, "could not construct a selection over the zero-width-space boundary run"); + AssertSelectionTargets(sel, PatternVcBase.ktagLeftBoundary); + Assert.That(sel.IsEditable, Is.False, + "the zero-width-space boundary run (PatternVcBase.OpenSingleLinePile) must be marked ktptNotEditable"); + } + /// /// ComplexConcControl.Designer.cs must wire up the pattern-builder view as read-only /// (this is the categorical fix for the IME-composition/keyboard-controller-registration @@ -451,6 +558,38 @@ public void ComplexConcControl_WiresViewAsReadOnly() } } + /// + /// SimpleRootSite.ReadOnlyView's setter forces AcceptsReturn = AcceptsTab = false when set + /// to true (SimpleRootSite.cs), which happens AFTER the Designer's own explicit + /// AcceptsReturn = true / AcceptsTab = false assignments in InitializeComponent's + /// generated-code ordering. This pins the actual, real behaviour change: AcceptsTab was + /// already false before this fix (Designer-set, independent of ReadOnlyView) so Tab + /// navigation out of the pane is unchanged; AcceptsReturn flips from true to false, which + /// is new. Since PatternView.OnKeyPress already swallows Return either way (it is not + /// Backspace/Delete), the observable difference is only where the key is disposed of: it + /// used to reach the control and be silently swallowed there; now IsInputKey(Return) + /// returns false and the key is never delivered to the control at all, so it is processed + /// as an ordinary dialog/navigation key by whatever contains this pane. This control is + /// hosted as a Words-area tool pane (DistFiles/.../Concordance/toolConfiguration.xml), not + /// inside a modal dialog with an AcceptButton, so no default-button activation is expected + /// in practice -- but that is unverified live; see the review doc. + /// + [Test] + public void ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse() + { + using (var control = new ComplexConcControl()) + { + var viewField = typeof(ComplexConcControl).GetField("m_view", BindingFlags.NonPublic | BindingFlags.Instance); + var view = (PatternView) viewField.GetValue(control); + + Assert.That(view.AcceptsTab, Is.False, + "AcceptsTab was already false in the Designer before this fix; it must still be false"); + Assert.That(view.AcceptsReturn, Is.False, + "ReadOnlyView = true forces AcceptsReturn to false, overriding the Designer's " + + "explicit AcceptsReturn = true -- this is the real behaviour change, not Tab"); + } + } + /// /// PatternView.AllowDisplaySelection must be overridden to stay true even when /// ReadOnlyView is true, or the pattern-builder's chooser-driven selection highlight diff --git a/Src/LexText/LexTextControls/PatternVcBase.cs b/Src/LexText/LexTextControls/PatternVcBase.cs index ad4bd25fb7..e8d1c383d4 100644 --- a/Src/LexText/LexTextControls/PatternVcBase.cs +++ b/Src/LexText/LexTextControls/PatternVcBase.cs @@ -226,7 +226,10 @@ protected void OpenSingleLinePile(IVwEnv vwenv, int maxNumLines, bool addBoundar vwenv.OpenParagraph(); if (addBoundary) { + // This zero-width-space boundary run is a cursor-parking glyph for arrow-key + // navigation, never free text; a fake tag with no bound field behind it. vwenv.Props = m_bracketProps; + vwenv.set_IntProperty((int) FwTextPropType.ktptEditable, (int) FwTextPropVar.ktpvEnum, (int) TptEditable.ktptNotEditable); vwenv.AddProp(ktagLeftBoundary, this, kfragZeroWidthSpace); } } @@ -241,6 +244,7 @@ protected void CloseSingleLinePile(IVwEnv vwenv, bool addBoundary) if (addBoundary) { vwenv.Props = m_bracketProps; + vwenv.set_IntProperty((int) FwTextPropType.ktptEditable, (int) FwTextPropVar.ktpvEnum, (int) TptEditable.ktptNotEditable); vwenv.AddProp(ktagRightBoundary, this, kfragZeroWidthSpace); } vwenv.CloseParagraph(); From d91c7d85a5a0bb3658e41d506a345688addaf917 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 11:30:20 -0400 Subject: [PATCH 6/8] Correct review doc: ablation findings, ReadOnlyView framing, AcceptsReturn Rewrites the ablation conclusion (ktptEditable is independently sufficient too, and prevents a display artifact UpdateProp's no-op does not), states plainly that ReadOnlyView does not gate ReplaceWithTsString (mutation-tested) and its only proven value is closing the IME-controller registration channel (unverified live), documents the AcceptsReturn/ AcceptsTab behaviour change (AcceptsTab unchanged, AcceptsReturn now false), and updates the ConstChartVc status per the coordinator's adversarial-review note (escalated to a concrete corruption suspicion, explicitly out of scope for this task). --- .../bugs/complex-conc-pattern-crash-review.md | 293 +++++++++++------- 1 file changed, 182 insertions(+), 111 deletions(-) diff --git a/Docs/bugs/complex-conc-pattern-crash-review.md b/Docs/bugs/complex-conc-pattern-crash-review.md index 12e9da2892..2ec3a81fe5 100644 --- a/Docs/bugs/complex-conc-pattern-crash-review.md +++ b/Docs/bugs/complex-conc-pattern-crash-review.md @@ -1,5 +1,15 @@ # Review: ComplexConcPatternVc direct-edit crash (architecture self-review) +**Revision note:** this review was corrected after adversarial review found two errors in +the first draft: (1) the fragment-targeting test helper was inert for 9 of 11 "fragment +angle" tests (it always selected the same whole-object range regardless of the tag +argument), so the claimed breadth was inflated; (2) a genuinely unmarked gap existed for +the OR/`#` literals' zero-width-space boundary run. Both are fixed; the corrected ablation +below also reverses part of the original conclusion -- `ktptEditable`, once complete, turns +out to be independently sufficient to stop the crash too, not merely "defense in depth" as +first claimed, and it prevents a real, demonstrated display-corruption artifact that +`UpdateProp` alone does not. + ## 1. Is this the right architecture? The invariant is the same one identified by the sibling bug's review: "a pattern view is @@ -20,140 +30,201 @@ same two independent parts here: control-level fact, fixed the same way as the sibling: `ComplexConcControl` now sets `m_view.ReadOnlyView = true` instead of `false`. -**This bug is not the same failure mode as the sibling's, and the ablation evidence shows -it plainly.** For the sibling (`RuleFormulaVcBase`), `ktptEditable` was the *only* -load-bearing layer, because `AddStringAltMember`-bound fragments write straight to the -real property without ever calling `UpdateProp` -- the sibling's own review documents this -(removing `ktptEditable` alone reproduced the corruption). Here, re-running the entire -failing test suite with *only* the `UpdateProp` override added (no `ktptEditable` -markings, no `ReadOnlyView` change) turned all 13 tests green. `UpdateProp` alone is -necessary and sufficient to stop the crash, because there is no real property for an edit -to land on -- `UpdateProp` returning `tssVal` unchanged is a true no-op relative to model -state, not a mitigation racing a live write. - -Given that, the `ktptNotEditable` markings and `ReadOnlyView = true` added here are -**not required to stop the crash** -- they are added anyway, for reasons independent of -the crash: - -- `ktptNotEditable` on every fragment (feature lines, `Infl Features` header, `OR`/`#`, - every bracket/paren glyph including the multi-line pile hooks, and both quantifier - lines) rejects an edit at the selection layer instead of letting it reach `UpdateProp` - at all. This matters because `UpdateProp`'s no-op is only a no-op *for the model*; it is - not obviously a no-op for the view's own display cache -- returning `tssVal` from - `UpdateProp` is how a VC normally *approves* a display update, and nothing in this - codebase demonstrates that the specific cache entry backing a fake, negative flid - (`ktagType` et al., never registered with `ISilDataAccess`) is invalidated correctly - without a full `Reconstruct`. Marking the fragments non-editable removes the need to - trust that path at all. `SelectionOverFormLine_IsNotEditable` confirms the marking - actually takes hold (`sel.IsEditable == false`). -- `ReadOnlyView = true` is the categorical fix for the trigger the bug report actually - named: IME composition and drag-and-drop, which do not go through - `PatternView.OnKeyPress`. `ReadOnlyView` unregisters the keyboard/IME controller hook - (`SimpleRootSite`); `ktptEditable` does not gate that registration, only whether a - `ReplaceWithTsString`-style edit is accepted once something reaches the view. - `ComplexConcControl_WiresViewAsReadOnly` checks the real Designer-generated wiring, not - just a synthetic test harness. -- `PatternView.AllowDisplaySelection` had to be added (it did not previously exist on - this branch) because `ReadOnlyView = true` suppresses `Activate()` by default - (`SimpleRootSite.AllowDisplaySelection` defaults to `IsEditable`), which would hide the - selection the chooser insert/delete buttons need the user to see. - -**Should the invariant live in `PatternVcBase`/`PatternView` itself, so a third subclass -can't reintroduce this?** Yes, partially, and I did not do the full version here. Two -independent things could move: +### Corrected ablation + +The first draft of this review claimed `UpdateProp` alone was "necessary and sufficient" +and that `ktptEditable`/`ReadOnlyView` were pure defense-in-depth. That was measured with a +test helper that turned out to be inert for most fragments (see the Testing section +below) and is **wrong on the "defense in depth" characterization**. Redone with a fixed +helper that provably targets the requested fragment (`AssertSelectionTargets`, checked via +`IVwSelection.TextSelInfo`), plus the previously-missing `ktptEditable` marking on the +zero-width-space boundary run, the real ablation is: + +| Configuration | Result (18-test suite) | +|---|---| +| Shipped state (`UpdateProp` + complete `ktptEditable` + `ReadOnlyView`) | 18/18 pass | +| `UpdateProp` removed, `ktptEditable` complete | 18/18 pass | +| `UpdateProp` present, `ktptEditable` neutralized everywhere | 16/18 pass -- the 2 failures are exactly the two tests that assert `IsEditable` directly; every crash/content test still passes | + +So, once `ktptEditable` is *actually complete* (see the persistence bug below), **either +layer independently stops the crash** for every fragment enumerated. This is a different +finding from the sibling bug, where `ktptEditable` was the *only* load-bearing layer +(`AddStringAltMember`-bound fragments write straight to the real property without ever +calling `UpdateProp`). Here neither fragment binds a real field, so `UpdateProp`'s no-op is +a true no-op for the *model* regardless of `ktptEditable`. + +**But `UpdateProp` alone is not equivalent to `ktptEditable`, because of what it does to +the *display*.** With `ktptEditable` neutralized and `UpdateProp` intact, a direct probe +(temporarily reverting `ktptEditable`, performing an edit, then re-selecting the same +fragment without any `Reconstruct`) showed the model correctly unchanged (`Form: 'original'`) +but the **redisplayed text read `'HACKEDForm: original'`** -- the discarded edit's text +was prepended into the cached display run and did not self-correct. `UpdateProp` returning +`tssVal` unmodified only means "no crash and no model write"; it does not mean the view's +own cached rendering of that fragment is refreshed to match. `ktptEditable` prevents this +because the edit never gets that far -- confirmed by `SelectionOverFormLine_IsNotEditable` +and `SelectionOverZeroWidthBoundaryRun_IsNotEditable`, both of which fail (red) if their +respective marking is removed and pass (green) with it present (verified by mutation, not +assumed). + +**Conclusion:** `UpdateProp` is required (it is the only thing standing between a crash and +survival if `ktptEditable` is ever incomplete, including for a fragment nobody has thought +to mark yet). `ktptEditable` is also required, not for defense-in-depth against the crash, +but because it is the only layer that also prevents the visible, corrupted-looking display +artifact just described. + +### A production bug found while fixing the test helper + +While redoing the ablation, `SelectionOverFormLine_IsNotEditable` failed unexpectedly +(`IsEditable == true`) even though `ComplexConcPatternVc.DisplayFeatures` calls +`SetNotEditable(vwenv)` once before its *first* `AddProp` call (`ktagType`). A diagnostic +probe confirmed: the first `AddProp` after `SetNotEditable` is correctly marked, but the +*second* `AddProp` sharing that same earlier call (`ktagForm`, `ktagGloss`, etc.) is not -- +`ktptEditable` set via `vwenv.set_IntProperty` does not persist across multiple `AddProp` +calls the way `vwenv.Props = someBuilder` does; it must be re-asserted immediately before +*every* `AddProp` call, which is exactly the pattern `PatternVcBase.AddExtraLines` and +`RuleFormulaVcBase` already use (and which the first draft of this fix did not follow +consistently). Fixed by adding a `SetNotEditable(vwenv)` call before each individual +`AddProp` in `DisplayFeatures`, `DisplayInflFeatureLines`, `DisplayInflFeatures`, and the +four multi-line bracket/paren pile-hook sequences (UpHook/Ext-loop/LowHook), none of which +had previously been re-asserted per call. This means most of the `ktptEditable` markings +claimed in the original fix were only accidentally correct for single-`AddProp` call sites +(OR, `#`, min/max, brackets/parens themselves) and were **not actually taking effect** for +the feature lines (Form/Entry/Category/Gloss/Infl) or the multi-line pile glyphs until this +correction. + +### Should the invariant live in `PatternVcBase`/`PatternView` itself? + +Yes, partially, and I did not do the full version here. Two independent things could move: 1. **`UpdateProp` could have a base-class default that returns `tssVal` instead of - throwing.** `PatternVcBase` doesn't override `VwBaseVc.UpdateProp` at all today, so - the *unimplemented* default is inherited from `VwBaseVc`. A `PatternVcBase.UpdateProp` - override doing exactly what both subclasses currently do by hand (`RuleFormulaVcBase` - and now `ComplexConcPatternVc`) would mean a third subclass gets the safe behavior - automatically instead of needing to remember to add it. **I did not make this change.** - It only affects the crash-avoidance half of the story (which, per the ablation above, - is the *entire* story for this bug but was *not* sufficient for the sibling's, where - `ktptEditable` was load-bearing) -- moving it to the base class doesn't, by itself, - protect a future subclass that binds a real field via `AddStringAltMember`, which is - the actually dangerous case. Given `ComplexConcPatternVc.UpdateProp` and - `RuleFormulaVcBase.UpdateProp` are now textually identical one-liners, hoisting it is - almost pure duplication removal with no behavior change for either existing subclass -- - a safe follow-up, but it touches `RuleFormulaVcBase.cs` on the sibling's own unmerged - branch, so I left it as a documented recommendation rather than doing it here to avoid - a cross-branch collision on a file I don't own in this task. -2. **The "not free text" fragment-marking discipline cannot be hoisted as cheaply.** - Marking editability is inherently per-fragment (each subclass alone knows which of its - own `AddProp`/`AddStringAltMember` calls binds to a mutable channel), so there's no - single base-class change that forces a third subclass to mark its fragments correctly. - The closest structural guard I can identify without redesigning the VC pattern: give - `PatternVcBase` a protected helper (`MarkNotEditable(vwenv)`, effectively what - `SetNotEditable` is here) so at least the *mechanism* is shared and discoverable, and - have `PatternVcBase.AddExtraLines` (which already does this for filler lines) serve as - the precedent a new subclass's author is likely to copy. I did not hoist `SetNotEditable` - itself, since it is a one-line wrapper and duplicating it costs less than adding - cross-subclass coupling for a helper this small; I would reconsider if a third - subclass appears. + throwing.** `PatternVcBase` doesn't override `VwBaseVc.UpdateProp` at all today, so the + *unimplemented* default is inherited from `VwBaseVc`. Given `ComplexConcPatternVc.UpdateProp` + and `RuleFormulaVcBase.UpdateProp` are now textually identical one-liners, hoisting it + is almost pure duplication removal with no behavior change for either existing + subclass -- a safe follow-up, but it touches `RuleFormulaVcBase.cs` on the sibling's own + unmerged branch, so I left it as a documented recommendation rather than doing it here + to avoid a cross-branch collision on a file I don't own in this task. +2. **The "not free text" fragment-marking discipline cannot be hoisted as cheaply**, and + the persistence bug above is exactly the argument for trying: marking editability is + inherently per-fragment and per-call (each subclass alone knows which of its own + `AddProp` calls binds to a mutable channel, and the property must be re-asserted before + each one), so there's no single base-class change that forces a third subclass to mark + its fragments correctly or to remember the per-call re-assertion rule. The closest + structural guard I can identify without redesigning the VC pattern: give `PatternVcBase` + a protected `SetNotEditable`/`MarkNotEditable` helper (already present here, and already + the pattern `AddExtraLines` uses) so at least the *mechanism and its per-call-site + convention* are shared and discoverable. I did not hoist it in this task, since it is a + one-line wrapper and duplicating it costs less than adding cross-subclass coupling for a + helper this small -- I would reconsider if a third subclass appears, and would make the + per-call convention an explicit doc comment on the shared helper if I did. `ReadOnlyView`/`AllowDisplaySelection` are already control-level and already shared (`PatternView`), so nothing further to hoist there -- the risk is a future `PatternView` consumer forgetting to set `ReadOnlyView = true` on its own control, which is a Designer.cs wiring mistake no base-class change can prevent. +### `ReadOnlyView` -- corrected framing + +**`ReadOnlyView = true` does not prevent the crash.** This was directly demonstrated by +mutation testing: in every ablation run above, `ReadOnlyView` was `true` throughout, and +whether a given configuration crashed was governed entirely by `UpdateProp`/`ktptEditable`, +never by `ReadOnlyView`. `ReadOnlyView` does not gate `IVwSelection.ReplaceWithTsString` at +all -- it is unrelated to the crash mechanism. + +Its proven value is narrower and different: it unregisters the keyboard/IME controller hook +(`SimpleRootSite.cs` -- `UnsubscribeFromRootSiteEventHandlerEvents`, called from the +`ReadOnlyView` setter), which is the categorical fix for the IME-composition/drag-and-drop +path the bug report actually named as the likely real-world trigger. That specific claim +(that it closes the IME channel) has **not** been verified against a live IME or drag +operation in this task -- see section 4. It is kept because closing that channel is still +worthwhile even though it does not touch the crash mechanism, and because the sibling +branch already establishes the same pattern for the rule-formula editor. + +### An unremarked behaviour change: `AcceptsReturn`/`AcceptsTab` + +`SimpleRootSite.ReadOnlyView`'s setter also forces `AcceptsReturn = AcceptsTab = false` +when set to `true`. Checked directly against `ComplexConcControl.Designer.cs`'s generated +code (`ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse`): + +- **`AcceptsTab` is unchanged.** The Designer already sets `AcceptsTab = false` + unconditionally, independent of `ReadOnlyView`, before this fix. Tab already moved focus + out of the pane; this fix does not change that. +- **`AcceptsReturn` changes from `true` to `false`.** The Designer explicitly set + `AcceptsReturn = true`; the `ReadOnlyView = true` assignment that follows it in + `InitializeComponent`'s generated ordering overrides that back to `false`. This is a + real behaviour change this fix introduces, not the Tab regression a first guess might + expect. + +Since `PatternView.OnKeyPress` already swallows Return unconditionally (it is not +Backspace/Delete), the practical difference is only *where* the keystroke is disposed of: +previously `IsInputKey(Return)` returned `true`, so the key reached the control and was +silently swallowed there; now it returns `false`, so the key is never delivered to the +control and is processed as an ordinary dialog/navigation key by whatever contains the +pane instead. `ComplexConcControl` is hosted as a Words-area tool pane +(`DistFiles/Language Explorer/Configuration/Words/Concordance/toolConfiguration.xml`), not +inside a modal dialog with an `AcceptButton` -- a repo-wide grep for `AcceptButton` finds it +only on this feature's own *editing* dialogs (`ComplexConcMorphDlg`, `ComplexConcWordDlg`, +`ComplexConcTagDlg`), none of which host this control -- so no default-button activation is +expected in the pane's real hosting context. That is reasoning from the wiring, not a live +verification; see section 4. + ## 2. What can be removed or simplified? Nothing was removed. The specific candidate, per the task brief, was `PatternView.PatternEditingHelper.CanCut()`/`CanPaste()` (`Src/LexText/LexTextControls/PatternView.cs`), which look redundant now that `ComplexConcControl` also runs with `ReadOnlyView = true` (matching `RuleFormulaControl`'s -state *on the sibling's own branch*). They were **not** removed here, for a -branch-topology reason rather than a functional one: on *this* branch, -`RuleFormulaControl.Designer.cs` still sets `ReadOnlyView = false` (the sibling's flip to -`true` lives only on the unmerged `fix/phon-rule-formula-readonly` branch). From this -branch's point of view, the override is still load-bearing for the rule-formula editor, -so removing it now would reduce coverage for a consumer this task did not touch. This is -the flip side of the sibling's own review note ("kept because `ComplexConcControl` still -depends on it") -- once both branches are integrated and *both* consumers set -`ReadOnlyView = true`, `CanCut`/`CanPaste` become fully redundant with -`EditingHelper.CanCut`/`CanPaste`'s own `Editable`-gated base behavior, and should be -removed then. **Noting this for whoever integrates both branches, per the task brief, -rather than editing `fix/phon-rule-formula-readonly`.** `CanCopy()` stays regardless: the -base implementation doesn't consult `Editable`, so it was never redundant. +state *on the sibling's own branch, PR sillsdev/FieldWorks#1082*). They were **not** +removed here, for a branch-topology reason rather than a functional one: on *this* branch +(and on current `origin/main`), `RuleFormulaControl.Designer.cs` still sets +`ReadOnlyView = false` -- the sibling's flip to `true` lives only on the unmerged PR #1082, +which cites "`ComplexConcControl` still runs with `ReadOnlyView = false`" as its own +explicit justification for keeping `CanCut`/`CanPaste`. This branch's change removes that +premise. Traced directly (not just by analogy): `SimpleRootSite.ReadOnlyView`'s setter is +literally `EditingHelper.Editable = !value`, and the base `EditingHelper.CanCut()`/ +`CanPaste()` both open with `if (... && m_fEditable) ...` and otherwise return `false` -- +so once **both** consumers set `ReadOnlyView = true`, the base class already returns +`false` unconditionally for both, with no dependency on the override; `CanCut`/`CanPaste` +become provably dead code once both branches land. **Not edited here** -- `PatternView.cs` +belongs partly to PR #1082's own review position, and the decision of when to remove them +belongs to whoever reconciles the two branches, not to this task. + +`CanCopy()` stays regardless: the base implementation doesn't consult `Editable`, so it was +never redundant. ## 3. What was not fixed, and why -- **Zero-width-space boundary markers** (`PatternVcBase.OpenSingleLinePile`/ - `CloseSingleLinePile`, `kfragZeroWidthSpace` on `ktagLeftBoundary`/`ktagRightBoundary`) - are not individually marked `ktptNotEditable`, in either `PatternVcBase` subclass. This - is a pre-existing, shared gap the sibling's own review flagged and left open for the - same reason: these are invisible cursor-parking glyphs used for boundary navigation, not - literal or bound content, and `UpdateProp` (now overridden in both subclasses) absorbs - any edit attempt there harmlessly. Confirmed by reasoning, not by a dedicated test -- - I did not add one, since it would exercise the identical `UpdateProp` no-op path already - covered by the eleven fragment tests, not a new mechanism. - **`PatternVcBase.UpdateProp` base-class hoist** -- see section 1. Left as a recommendation, not implemented, to avoid touching `RuleFormulaVcBase.cs` on a branch I don't own. -- **`ConstChartVc.cs:297`** -- out of scope for this bug; already tracked as - SUSPECTED-safe-by-reading-only in `phon-rule-direct-editing.md`, unchanged by this work. -- **Live IME/drag-and-drop reproduction** -- as with the sibling bug, this was inferred - (the bug report itself says "if input reaches the view without passing through - PatternView.OnKeyPress"), not reproduced with a real IME or a real OS-level drag - operation. `ReadOnlyView = true` closing the keyboard-controller registration is the - categorical fix for that path; see section 4 for what still needs a live check. +- **`ConstChartVc.cs:297`** -- out of scope for this bug. Status has moved since the first + draft of this review: it is no longer "suspected safe" but a concrete suspicion of a + Bug-1-class *corruption* defect (`ApplyFormatting`'s `vwenv.Props = ttp` appears to be a + full property-bag replace that discards the cell-level `ktptNotEditable` `MakeCellsMethod` + sets, immediately before a real, shared `ICmPossibility` is bound via + `AddStringAltMember`). Unconfirmed -- a probe attempt hit an `ArgumentException` + constructing the selection. This is a separate, already-flagged investigation; explicitly + out of scope for this task. +- **Live IME/drag-and-drop reproduction, and the `AcceptsReturn` consequence** -- inferred + from wiring, not reproduced with a real IME, a real OS-level drag operation, or a live + check of what (if anything) receives an un-delivered Return keystroke. See section 4. ## 4. What needs manual verification in a running FLEx -- Open Texts & Words -> Complex Concordance, build a pattern with at least one Word and - one Morph node with Form/Gloss/Category/Entry/Infl Features all populated, and confirm - the pattern builder still renders identically to before this change (no visual - regression from the `ktptEditable`/`ReadOnlyView` changes). +- Open Texts & Words -> Complex Concordance, build a pattern with at least one Word and one + Morph node with Form/Gloss/Category/Entry/Infl Features all populated, and confirm the + pattern builder still renders identically to before this change (no visual regression + from the `ktptEditable`/`ReadOnlyView` changes -- the persistence-bug fix touched every + multi-line pile and feature-line call site). - With a vernacular IME active, place focus in the pattern builder and attempt to compose - and commit text into a feature line. Confirm composition does not commit into the pane - at all (rather than committing and then silently reverting) -- this is what - `ReadOnlyView = true` should prevent categorically. -- Confirm the selection highlight is still visible when a chooser-inserted item is - selected (this is what `PatternView.AllowDisplaySelection` restores), and that the - Insert/Search controls still operate against the right selection. + and commit text into a feature line. Confirm composition does not commit into the pane at + all -- this is `ReadOnlyView`'s specific, as-yet-unverified claim. +- Confirm the selection highlight is still visible when a chooser-inserted item is selected + (this is what `PatternView.AllowDisplaySelection` restores), and that the Insert/Search + controls still operate against the right selection. - Confirm Delete still removes the selected item in the live UI, matching `DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly`. -- Try dragging text onto the pattern-builder pane; confirm it is rejected/does nothing, - rather than being accepted and then not visibly changing anything (the two are - distinguishable to a user who's watching the drop target, even though neither corrupts - data). +- Press Enter/Return while focused in the pattern-builder pane and confirm nothing + unexpected happens (no default button activates, focus does not jump unexpectedly) -- + this is the `AcceptsReturn` change's live consequence, reasoned about but not observed. +- Try dragging text onto the pattern-builder pane; confirm it is rejected/does nothing. From 1c21a07b06570590bd684c9cc3f6ea0e1ad4cf79 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 15:19:56 -0400 Subject: [PATCH 7/8] LT-22717: move investigation and design notes to the PR description The bug analysis, the seed probe and the architecture self-review were working documents for this fix. Their conclusions are now carried by the code and its tests; the reasoning, decisions and paths not taken live in the pull request body so they inform review without merging into the tree. --- ...omplexConcPatternVcDirectEditProbeTests.cs | 125 ---------- .../bugs/complex-conc-pattern-crash-review.md | 230 ------------------ Docs/bugs/complex-conc-pattern-crash.md | 68 ------ 3 files changed, 423 deletions(-) delete mode 100644 Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs delete mode 100644 Docs/bugs/complex-conc-pattern-crash-review.md delete mode 100644 Docs/bugs/complex-conc-pattern-crash.md diff --git a/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs b/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs deleted file mode 100644 index fc2537268d..0000000000 --- a/Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs +++ /dev/null @@ -1,125 +0,0 @@ -// Copyright (c) 2026 SIL International -// This software is licensed under the LGPL, version 2.1 or later -// (http://www.gnu.org/licenses/lgpl-2.1.html) -// -// REVIEW PROBE (not part of any shipped fix) -- exists only to determine, empirically, -// what happens when a ComplexConcPatternVc-hosted PatternView is edited via the same -// low-level ReplaceWithTsString bypass used by the phon-rule-formula-readonly repro. -// ComplexConcControl leaves PatternView.ReadOnlyView = false, so this bypass is not even -// needed in production -- a plain keystroke reaches OnKeyPress first, but ReplaceWithTsString -// is exactly what IME composition or drag-and-drop would call, same as the phon-rule case. - -using System.Windows.Forms; -using NUnit.Framework; -using SIL.LCModel; -using SIL.LCModel.Core.Text; -using SIL.LCModel.Core.KernelInterfaces; -using SIL.LCModel.Infrastructure; -using SIL.FieldWorks.Common.RootSites; -using SIL.FieldWorks.Common.ViewsInterfaces; -using SIL.FieldWorks.LexText.Controls; -using XCore; - -namespace SIL.FieldWorks.IText -{ - [TestFixture] - public class ComplexConcPatternVcDirectEditProbeTests : MemoryOnlyBackendProviderTestBase - { - private Mediator m_mediator; - private PropertyTable m_propertyTable; - private TestPatternView m_view; - - public override void TestSetup() - { - base.TestSetup(); - m_mediator = new Mediator(); - m_propertyTable = new PropertyTable(m_mediator); - m_propertyTable.SetProperty("cache", Cache, false); - } - - public override void TestTearDown() - { - if (m_view != null) - { - m_view.Dispose(); - m_view = null; - } - if (m_propertyTable != null) - { - m_propertyTable.Dispose(); - m_propertyTable = null; - } - if (m_mediator != null) - { - m_mediator.Dispose(); - m_mediator = null; - } - base.TestTearDown(); - } - - private class NullPatternControl : IPatternControl - { - public object GetContext(SelectionHelper sel) => null; - public object GetContext(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; - public object GetItem(SelectionHelper sel, SelectionHelper.SelLimitType limit) => null; - public int GetItemContextIndex(object ctxt, object obj) => -1; - public SelLevInfo[] GetLevelInfo(object ctxt, int index) => null; - public int GetContextCount(object ctxt) => 0; - public object GetNextContext(object ctxt) => null; - public object GetPrevContext(object ctxt) => null; - public int GetFlid(object ctxt) => 0; - } - - private class TestPatternView : PatternView - { - public void CallLayout() - { - OnLayout(new LayoutEventArgs(this, string.Empty)); - } - } - - /// - /// Probe: a bare word node (no Form/Gloss/Category/InflFeatures) renders as a single - /// line whose only content is the computed "Type: Word" text, bound to the fake tag - /// ComplexConcPatternVc.ktagType on the synthetic (negative-hvo, non-domain) word node. - /// ComplexConcControl leaves ReadOnlyView = false and ComplexConcPatternVc never marks - /// this fragment ktptNotEditable, so -- unlike the phon-rule fix -- nothing blocks a - /// direct low-level edit here at all, not even OnKeyPress (which only blocks WM_CHAR, - /// not ReplaceWithTsString). This probe records what actually happens. - /// - [Test] - public void ReplaceWithTsString_OnWordNodeTypeLine_RecordsWhatHappens() - { - var root = new ComplexConcGroupNode(); - var wordNode = new ComplexConcWordNode(); - root.Children.Add(wordNode); - var model = new ComplexConcPatternModel(Cache, root); - - var vc = new ComplexConcPatternVc(Cache, m_propertyTable); - var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; - view.Init(m_mediator, m_propertyTable, model.Root.Hvo, new NullPatternControl(), vc, - ComplexConcPatternVc.kfragPattern, model.DataAccess); - view.CallLayout(); - m_view = view; - - var levels = new[] - { - new SelLevInfo { tag = ComplexConcPatternSda.ktagChildren, ihvo = 0 } - }; - IVwSelection sel = view.RootBox.MakeTextSelInObj(0, levels.Length, levels, - ComplexConcPatternVc.ktagType, null, true, false, false, /* fWholeObj */ true, /* fInstall */ true); - Assert.That(sel, Is.Not.Null, - "could not construct a selection over the word node's Type line -- fixture/path assumption is wrong"); - - ITsString replacement = TsStringUtils.MakeString("HACKED", Cache.DefaultUserWs); - - // No try/catch: if this throws, that IS the finding (an unhandled exception from a - // direct edit on an unmarked, unaudited ComplexConcPatternVc fragment). If it does - // not throw, the test passes and the assertions below record what changed instead. - UndoableUnitOfWorkHelper.Do("undo", "redo", Cache.LangProject, - () => sel.ReplaceWithTsString(replacement)); - - Assert.Pass("No exception was thrown by ReplaceWithTsString on the word node's Type line."); - } - } -} diff --git a/Docs/bugs/complex-conc-pattern-crash-review.md b/Docs/bugs/complex-conc-pattern-crash-review.md deleted file mode 100644 index 2ec3a81fe5..0000000000 --- a/Docs/bugs/complex-conc-pattern-crash-review.md +++ /dev/null @@ -1,230 +0,0 @@ -# Review: ComplexConcPatternVc direct-edit crash (architecture self-review) - -**Revision note:** this review was corrected after adversarial review found two errors in -the first draft: (1) the fragment-targeting test helper was inert for 9 of 11 "fragment -angle" tests (it always selected the same whole-object range regardless of the tag -argument), so the claimed breadth was inflated; (2) a genuinely unmarked gap existed for -the OR/`#` literals' zero-width-space boundary run. Both are fixed; the corrected ablation -below also reverses part of the original conclusion -- `ktptEditable`, once complete, turns -out to be independently sufficient to stop the crash too, not merely "defense in depth" as -first claimed, and it prevents a real, demonstrated display-corruption artifact that -`UpdateProp` alone does not. - -## 1. Is this the right architecture? - -The invariant is the same one identified by the sibling bug's review: "a pattern view is -not free text; it changes only via chooser-insert and delete." That invariant has the -same two independent parts here: - -- **"This fragment is a computed display, not a bound field, and must not accept an - edit."** `ComplexConcPatternVc` binds no real domain object field at all -- a grep for - `AddStringAltMember` in `ComplexConcPatternVc.cs` returns zero matches, confirmed by - inspection and by `ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPartOfSpeechUnrenamed` - and `..._OnTagNodeTagLine_..._AndRealTagUnrenamed`, which specifically check that the - real, shared `IPartOfSpeech`/`ICmPossibility` referenced by a node is untouched after an - edit attempt. Every fragment is `AddProp` + `DisplayVariant` over the synthetic - `ComplexConcPatternNode` tree, so there is nothing to corrupt project-wide -- but there - was also nothing absorbing the edit, so it fell through to `VwBaseVc.UpdateProp` - (`NotImplementedException`, unhandled). -- **"This widget only accepts chooser-insert and delete, never typed input."** A - control-level fact, fixed the same way as the sibling: `ComplexConcControl` now sets - `m_view.ReadOnlyView = true` instead of `false`. - -### Corrected ablation - -The first draft of this review claimed `UpdateProp` alone was "necessary and sufficient" -and that `ktptEditable`/`ReadOnlyView` were pure defense-in-depth. That was measured with a -test helper that turned out to be inert for most fragments (see the Testing section -below) and is **wrong on the "defense in depth" characterization**. Redone with a fixed -helper that provably targets the requested fragment (`AssertSelectionTargets`, checked via -`IVwSelection.TextSelInfo`), plus the previously-missing `ktptEditable` marking on the -zero-width-space boundary run, the real ablation is: - -| Configuration | Result (18-test suite) | -|---|---| -| Shipped state (`UpdateProp` + complete `ktptEditable` + `ReadOnlyView`) | 18/18 pass | -| `UpdateProp` removed, `ktptEditable` complete | 18/18 pass | -| `UpdateProp` present, `ktptEditable` neutralized everywhere | 16/18 pass -- the 2 failures are exactly the two tests that assert `IsEditable` directly; every crash/content test still passes | - -So, once `ktptEditable` is *actually complete* (see the persistence bug below), **either -layer independently stops the crash** for every fragment enumerated. This is a different -finding from the sibling bug, where `ktptEditable` was the *only* load-bearing layer -(`AddStringAltMember`-bound fragments write straight to the real property without ever -calling `UpdateProp`). Here neither fragment binds a real field, so `UpdateProp`'s no-op is -a true no-op for the *model* regardless of `ktptEditable`. - -**But `UpdateProp` alone is not equivalent to `ktptEditable`, because of what it does to -the *display*.** With `ktptEditable` neutralized and `UpdateProp` intact, a direct probe -(temporarily reverting `ktptEditable`, performing an edit, then re-selecting the same -fragment without any `Reconstruct`) showed the model correctly unchanged (`Form: 'original'`) -but the **redisplayed text read `'HACKEDForm: original'`** -- the discarded edit's text -was prepended into the cached display run and did not self-correct. `UpdateProp` returning -`tssVal` unmodified only means "no crash and no model write"; it does not mean the view's -own cached rendering of that fragment is refreshed to match. `ktptEditable` prevents this -because the edit never gets that far -- confirmed by `SelectionOverFormLine_IsNotEditable` -and `SelectionOverZeroWidthBoundaryRun_IsNotEditable`, both of which fail (red) if their -respective marking is removed and pass (green) with it present (verified by mutation, not -assumed). - -**Conclusion:** `UpdateProp` is required (it is the only thing standing between a crash and -survival if `ktptEditable` is ever incomplete, including for a fragment nobody has thought -to mark yet). `ktptEditable` is also required, not for defense-in-depth against the crash, -but because it is the only layer that also prevents the visible, corrupted-looking display -artifact just described. - -### A production bug found while fixing the test helper - -While redoing the ablation, `SelectionOverFormLine_IsNotEditable` failed unexpectedly -(`IsEditable == true`) even though `ComplexConcPatternVc.DisplayFeatures` calls -`SetNotEditable(vwenv)` once before its *first* `AddProp` call (`ktagType`). A diagnostic -probe confirmed: the first `AddProp` after `SetNotEditable` is correctly marked, but the -*second* `AddProp` sharing that same earlier call (`ktagForm`, `ktagGloss`, etc.) is not -- -`ktptEditable` set via `vwenv.set_IntProperty` does not persist across multiple `AddProp` -calls the way `vwenv.Props = someBuilder` does; it must be re-asserted immediately before -*every* `AddProp` call, which is exactly the pattern `PatternVcBase.AddExtraLines` and -`RuleFormulaVcBase` already use (and which the first draft of this fix did not follow -consistently). Fixed by adding a `SetNotEditable(vwenv)` call before each individual -`AddProp` in `DisplayFeatures`, `DisplayInflFeatureLines`, `DisplayInflFeatures`, and the -four multi-line bracket/paren pile-hook sequences (UpHook/Ext-loop/LowHook), none of which -had previously been re-asserted per call. This means most of the `ktptEditable` markings -claimed in the original fix were only accidentally correct for single-`AddProp` call sites -(OR, `#`, min/max, brackets/parens themselves) and were **not actually taking effect** for -the feature lines (Form/Entry/Category/Gloss/Infl) or the multi-line pile glyphs until this -correction. - -### Should the invariant live in `PatternVcBase`/`PatternView` itself? - -Yes, partially, and I did not do the full version here. Two independent things could move: - -1. **`UpdateProp` could have a base-class default that returns `tssVal` instead of - throwing.** `PatternVcBase` doesn't override `VwBaseVc.UpdateProp` at all today, so the - *unimplemented* default is inherited from `VwBaseVc`. Given `ComplexConcPatternVc.UpdateProp` - and `RuleFormulaVcBase.UpdateProp` are now textually identical one-liners, hoisting it - is almost pure duplication removal with no behavior change for either existing - subclass -- a safe follow-up, but it touches `RuleFormulaVcBase.cs` on the sibling's own - unmerged branch, so I left it as a documented recommendation rather than doing it here - to avoid a cross-branch collision on a file I don't own in this task. -2. **The "not free text" fragment-marking discipline cannot be hoisted as cheaply**, and - the persistence bug above is exactly the argument for trying: marking editability is - inherently per-fragment and per-call (each subclass alone knows which of its own - `AddProp` calls binds to a mutable channel, and the property must be re-asserted before - each one), so there's no single base-class change that forces a third subclass to mark - its fragments correctly or to remember the per-call re-assertion rule. The closest - structural guard I can identify without redesigning the VC pattern: give `PatternVcBase` - a protected `SetNotEditable`/`MarkNotEditable` helper (already present here, and already - the pattern `AddExtraLines` uses) so at least the *mechanism and its per-call-site - convention* are shared and discoverable. I did not hoist it in this task, since it is a - one-line wrapper and duplicating it costs less than adding cross-subclass coupling for a - helper this small -- I would reconsider if a third subclass appears, and would make the - per-call convention an explicit doc comment on the shared helper if I did. - -`ReadOnlyView`/`AllowDisplaySelection` are already control-level and already shared -(`PatternView`), so nothing further to hoist there -- the risk is a future `PatternView` -consumer forgetting to set `ReadOnlyView = true` on its own control, which is a Designer.cs -wiring mistake no base-class change can prevent. - -### `ReadOnlyView` -- corrected framing - -**`ReadOnlyView = true` does not prevent the crash.** This was directly demonstrated by -mutation testing: in every ablation run above, `ReadOnlyView` was `true` throughout, and -whether a given configuration crashed was governed entirely by `UpdateProp`/`ktptEditable`, -never by `ReadOnlyView`. `ReadOnlyView` does not gate `IVwSelection.ReplaceWithTsString` at -all -- it is unrelated to the crash mechanism. - -Its proven value is narrower and different: it unregisters the keyboard/IME controller hook -(`SimpleRootSite.cs` -- `UnsubscribeFromRootSiteEventHandlerEvents`, called from the -`ReadOnlyView` setter), which is the categorical fix for the IME-composition/drag-and-drop -path the bug report actually named as the likely real-world trigger. That specific claim -(that it closes the IME channel) has **not** been verified against a live IME or drag -operation in this task -- see section 4. It is kept because closing that channel is still -worthwhile even though it does not touch the crash mechanism, and because the sibling -branch already establishes the same pattern for the rule-formula editor. - -### An unremarked behaviour change: `AcceptsReturn`/`AcceptsTab` - -`SimpleRootSite.ReadOnlyView`'s setter also forces `AcceptsReturn = AcceptsTab = false` -when set to `true`. Checked directly against `ComplexConcControl.Designer.cs`'s generated -code (`ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse`): - -- **`AcceptsTab` is unchanged.** The Designer already sets `AcceptsTab = false` - unconditionally, independent of `ReadOnlyView`, before this fix. Tab already moved focus - out of the pane; this fix does not change that. -- **`AcceptsReturn` changes from `true` to `false`.** The Designer explicitly set - `AcceptsReturn = true`; the `ReadOnlyView = true` assignment that follows it in - `InitializeComponent`'s generated ordering overrides that back to `false`. This is a - real behaviour change this fix introduces, not the Tab regression a first guess might - expect. - -Since `PatternView.OnKeyPress` already swallows Return unconditionally (it is not -Backspace/Delete), the practical difference is only *where* the keystroke is disposed of: -previously `IsInputKey(Return)` returned `true`, so the key reached the control and was -silently swallowed there; now it returns `false`, so the key is never delivered to the -control and is processed as an ordinary dialog/navigation key by whatever contains the -pane instead. `ComplexConcControl` is hosted as a Words-area tool pane -(`DistFiles/Language Explorer/Configuration/Words/Concordance/toolConfiguration.xml`), not -inside a modal dialog with an `AcceptButton` -- a repo-wide grep for `AcceptButton` finds it -only on this feature's own *editing* dialogs (`ComplexConcMorphDlg`, `ComplexConcWordDlg`, -`ComplexConcTagDlg`), none of which host this control -- so no default-button activation is -expected in the pane's real hosting context. That is reasoning from the wiring, not a live -verification; see section 4. - -## 2. What can be removed or simplified? - -Nothing was removed. The specific candidate, per the task brief, was -`PatternView.PatternEditingHelper.CanCut()`/`CanPaste()` -(`Src/LexText/LexTextControls/PatternView.cs`), which look redundant now that -`ComplexConcControl` also runs with `ReadOnlyView = true` (matching `RuleFormulaControl`'s -state *on the sibling's own branch, PR sillsdev/FieldWorks#1082*). They were **not** -removed here, for a branch-topology reason rather than a functional one: on *this* branch -(and on current `origin/main`), `RuleFormulaControl.Designer.cs` still sets -`ReadOnlyView = false` -- the sibling's flip to `true` lives only on the unmerged PR #1082, -which cites "`ComplexConcControl` still runs with `ReadOnlyView = false`" as its own -explicit justification for keeping `CanCut`/`CanPaste`. This branch's change removes that -premise. Traced directly (not just by analogy): `SimpleRootSite.ReadOnlyView`'s setter is -literally `EditingHelper.Editable = !value`, and the base `EditingHelper.CanCut()`/ -`CanPaste()` both open with `if (... && m_fEditable) ...` and otherwise return `false` -- -so once **both** consumers set `ReadOnlyView = true`, the base class already returns -`false` unconditionally for both, with no dependency on the override; `CanCut`/`CanPaste` -become provably dead code once both branches land. **Not edited here** -- `PatternView.cs` -belongs partly to PR #1082's own review position, and the decision of when to remove them -belongs to whoever reconciles the two branches, not to this task. - -`CanCopy()` stays regardless: the base implementation doesn't consult `Editable`, so it was -never redundant. - -## 3. What was not fixed, and why - -- **`PatternVcBase.UpdateProp` base-class hoist** -- see section 1. Left as a - recommendation, not implemented, to avoid touching `RuleFormulaVcBase.cs` on a branch I - don't own. -- **`ConstChartVc.cs:297`** -- out of scope for this bug. Status has moved since the first - draft of this review: it is no longer "suspected safe" but a concrete suspicion of a - Bug-1-class *corruption* defect (`ApplyFormatting`'s `vwenv.Props = ttp` appears to be a - full property-bag replace that discards the cell-level `ktptNotEditable` `MakeCellsMethod` - sets, immediately before a real, shared `ICmPossibility` is bound via - `AddStringAltMember`). Unconfirmed -- a probe attempt hit an `ArgumentException` - constructing the selection. This is a separate, already-flagged investigation; explicitly - out of scope for this task. -- **Live IME/drag-and-drop reproduction, and the `AcceptsReturn` consequence** -- inferred - from wiring, not reproduced with a real IME, a real OS-level drag operation, or a live - check of what (if anything) receives an un-delivered Return keystroke. See section 4. - -## 4. What needs manual verification in a running FLEx - -- Open Texts & Words -> Complex Concordance, build a pattern with at least one Word and one - Morph node with Form/Gloss/Category/Entry/Infl Features all populated, and confirm the - pattern builder still renders identically to before this change (no visual regression - from the `ktptEditable`/`ReadOnlyView` changes -- the persistence-bug fix touched every - multi-line pile and feature-line call site). -- With a vernacular IME active, place focus in the pattern builder and attempt to compose - and commit text into a feature line. Confirm composition does not commit into the pane at - all -- this is `ReadOnlyView`'s specific, as-yet-unverified claim. -- Confirm the selection highlight is still visible when a chooser-inserted item is selected - (this is what `PatternView.AllowDisplaySelection` restores), and that the Insert/Search - controls still operate against the right selection. -- Confirm Delete still removes the selected item in the live UI, matching - `DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly`. -- Press Enter/Return while focused in the pattern-builder pane and confirm nothing - unexpected happens (no default button activates, focus does not jump unexpectedly) -- - this is the `AcceptsReturn` change's live consequence, reasoned about but not observed. -- Try dragging text onto the pattern-builder pane; confirm it is rejected/does nothing. diff --git a/Docs/bugs/complex-conc-pattern-crash.md b/Docs/bugs/complex-conc-pattern-crash.md deleted file mode 100644 index d91e7951a3..0000000000 --- a/Docs/bugs/complex-conc-pattern-crash.md +++ /dev/null @@ -1,68 +0,0 @@ -# Bug 4 — Complex Concordance pattern builder crashes on any direct edit - -**Area:** Texts & Words → Complex Concordance → pattern builder pane (`ComplexConcControl`) -**Type:** Crash (unhandled exception), not data corruption -**Found by:** adversarial review of Bug 1 (`phon-rule-direct-editing.md`), which shares the same base classes - -## What the user sees - -Texts & Words area, **Complex Concordance** tool. The top-left pane is a pattern builder: you insert -Morph / Word / Tag / OR / Word Boundary pieces from a row of options, and each appears as a bracketed -column with labelled rows (Form, Gloss, Cat, Entry, Type, Infl) filled in through choosers. Like the -phonological rule formula, it is meant to be built by inserting and deleting, never by typing. - -If input reaches the view without passing through `PatternView.OnKeyPress` — IME composition when -typing a vernacular script, or dragging text into the pane — FLEx dies with an unhandled -`NotImplementedException`. No warning, no "field not editable" feedback. Because the trigger is an -IME path, it would preferentially hit vernacular-script users. - -## Why it happens - -`ComplexConcControl` and the rule formula editor are built from the same two classes: `PatternView` -(`Src/LexText/LexTextControls/PatternView.cs`) and `PatternVcBase` -(`Src/LexText/LexTextControls/PatternVcBase.cs`). `PatternVcBase` has exactly two subclasses and -`PatternView` exactly two consumers — the rule formula editor and this one — so the audit surface is -closed. - -Two differences from the rule formula editor make this a crash rather than a rename: - -1. **No `UpdateProp` override.** `RuleFormulaVcBase` overrides `UpdateProp`, so an edit reaching the - view is intercepted and absorbed. `ComplexConcPatternVc` (`Src/LexText/Interlinear/ComplexConcPatternVc.cs`) - has no such override, so the engine falls through to `VwBaseVc.UpdateProp`, which throws - `NotImplementedException`. Nothing catches it. -2. **No wall.** `ComplexConcControl.Designer.cs:57` still sets `ReadOnlyView = false`, and none of - `ComplexConcPatternVc`'s fragments are marked `ktptNotEditable`. Bug 1 closed both of these for the - rule formula editor; this control was left as-is. - -## Why it is NOT the Bug 1 corruption - -`ComplexConcPatternVc` binds no real domain fields — a grep for `AddStringAltMember` in that file -returns zero. Its content is synthetic: `ComplexConcPatternNode.Hvo` values are negative sentinels and -`Form`/`Gloss`/etc. are plain in-memory properties served by `ComplexConcPatternSda`, not LCM objects. -So a stray edit cannot rename a shared phoneme or natural class the way Bug 1's could. It just throws. - -Crash is louder but arguably less dangerous than Bug 1's silent project-wide rename. Both are real. - -## Evidence - -`Docs/bugs/ComplexConcPatternVcDirectEditProbeTests.cs` (seeded in this worktree) is a working probe -written during Bug 1's adversarial review. It builds a real `ComplexConcGroupNode` / -`ComplexConcWordNode` / `ComplexConcPatternSda` / `PatternView` exactly as `ComplexConcControl.Init` -does, then calls `IVwSelection.ReplaceWithTsString` on the word node's Type line -(`ComplexConcPatternVc.ktagType`). Result: unhandled `NotImplementedException` from -`VwBaseVc.UpdateProp` propagating out of `ReplaceWithTsString`. - -Treat the probe as a starting point to verify independently, not as a finished test. - -## Not yet established - -Whether the crash is reachable in a running FLEx via a real IME or drag-and-drop, as opposed to via a -direct `ReplaceWithTsString` call in a test. This is the same open question Bug 1 has, and it is why -the probe proves the *mechanism* rather than the *user path*. - -## Related - -- `phon-rule-direct-editing.md` — Bug 1, the sibling defect in the other `PatternVcBase` subclass. -- `ConstChartVc.cs:297` has the same defect *shape* as Bug 1 (binds a shared `CmPossibility` field) - but appears guarded at the cell level by `MakeCellsMethod.cs:495`. Marked SUSPECTED-safe by code - reading only; never verified by execution. From a8d7f32db4e7bb1496cbbffa4491fb145880c420 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 15:23:18 -0400 Subject: [PATCH 8/8] LT-22717: tighten comments to the repo commenting standard Move the file-header explanation into the fixture's own doc comment, replace decorative section banners with one-line statements, drop a pointer to a working document, and state the AcceptsReturn contract as current behaviour rather than as a before-and-after. --- .../Interlinear/ComplexConcPatternVc.cs | 3 +- .../ComplexConcPatternVcDirectEditTests.cs | 77 +++++++------------ Src/LexText/LexTextControls/PatternView.cs | 6 +- 3 files changed, 32 insertions(+), 54 deletions(-) diff --git a/Src/LexText/Interlinear/ComplexConcPatternVc.cs b/Src/LexText/Interlinear/ComplexConcPatternVc.cs index 2e47dc2c7a..c308dfe8aa 100644 --- a/Src/LexText/Interlinear/ComplexConcPatternVc.cs +++ b/Src/LexText/Interlinear/ComplexConcPatternVc.cs @@ -427,7 +427,8 @@ public override ITsString DisplayVariant(IVwEnv vwenv, int tag, int frag) private void DisplayFeatures(IVwEnv vwenv, ComplexConcPatternNode node) { // Every line here (Type, Form, Entry, Category, Gloss, Infl Features) is a computed - // summary of the synthetic pattern node, not free text; see UpdateProp and SetNotEditable. + // summary of the synthetic pattern node, not free text; see UpdateProp and + // SetNotEditable. SetNotEditable(vwenv); vwenv.AddProp(ktagType, this, kfragFeatureLine); var morphNode = node as ComplexConcMorphNode; diff --git a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs index 1790a990f9..5eaa526577 100644 --- a/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs +++ b/Src/LexText/Interlinear/ITextDllTests/ComplexConcPatternVcDirectEditTests.cs @@ -1,21 +1,7 @@ // Copyright (c) 2026 SIL International // This software is licensed under the LGPL, version 2.1 or later // (http://www.gnu.org/licenses/lgpl-2.1.html) -// -// Reproduction and regression coverage for the Complex Concordance pattern-builder crash. -// ComplexConcControl and the phonological rule formula editor share PatternView/ -// PatternVcBase. ComplexConcPatternVc has no UpdateProp override, so an edit that reaches -// the view engine without passing through PatternView.OnKeyPress (IME composition, -// drag-and-drop, or any direct IVwSelection.ReplaceWithTsString call) falls through to -// VwBaseVc.UpdateProp, which throws NotImplementedException. Unlike the sibling rule-formula -// bug, ComplexConcPatternVc binds no real domain fields via AddStringAltMember (verified by -// inspection: zero occurrences in ComplexConcPatternVc.cs), so this is a crash, not a silent -// corruption/rename. -// -// These tests drive a real IVwRootBox (PatternView/ComplexConcPatternVc) against a real -// in-memory LcmCache and call IVwSelection.ReplaceWithTsString directly -- the same low-level -// entry point IME composition or drag-and-drop would use, and one PatternView.OnKeyPress never -// sees because it only reacts to Windows key events, not to ReplaceWithTsString. + using System.Collections.Generic; using System.Reflection; using System.Windows.Forms; @@ -32,6 +18,16 @@ namespace SIL.FieldWorks.IText { + /// + /// Covers the Complex Concordance pattern builder's response to an edit that reaches the + /// view engine without passing through PatternView.OnKeyPress, as IME composition and + /// drag-and-drop do. Without an UpdateProp override such an edit falls through to + /// VwBaseVc.UpdateProp, which throws. The pattern builder binds no real domain fields, so + /// the failure is a crash rather than a silent rename of shared project data. + /// + /// These tests drive a real IVwRootBox against an in-memory LcmCache and call + /// IVwSelection.ReplaceWithTsString directly. + /// [TestFixture] public class ComplexConcPatternVcDirectEditTests : MemoryOnlyBackendProviderTestBase { @@ -251,11 +247,8 @@ public void MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode() Assert.That(tss.Text, Is.EqualTo("Gloss: myGloss")); } - // ------------------------------------------------------------------ - // Angle 1: breadth of the crash across the fragments ComplexConcPatternVc renders. - // Each of these encodes the DESIRED end state (no crash, content unchanged) and must - // fail against current code, which throws NotImplementedException instead. - // ------------------------------------------------------------------ + // Breadth of the crash across every fragment ComplexConcPatternVc renders. Each case + // asserts no crash and unchanged content. [Test] public void ReplaceWithTsString_OnWordNodeTypeLine_DoesNotThrow() @@ -342,10 +335,8 @@ public void ReplaceWithTsString_OnMorphNodeCategoryLine_DoesNotThrow_AndRealPart Assert.DoesNotThrow(() => AttemptEdit(sel, "HACKED", Cache.DefaultAnalWs), "a direct edit on the morph node's Category line must not crash the view engine"); - // This is the specific check for the bug doc's claim that this is a crash, not a - // Bug-1-style corruption: the category line displays a REAL, shared IPartOfSpeech's - // Abbreviation, so if this bug were the same class as Bug 1, a botched edit here - // could rename it project-wide. Confirm it does not. + // The category line displays a real, shared IPartOfSpeech's Abbreviation, so a + // botched edit here would rename it project-wide. Assert.That(noun.Abbreviation.BestAnalysisAlternative.Text, Is.EqualTo("N"), "an edit attempt on the Category line must not rename the real, shared PartOfSpeech"); } @@ -458,15 +449,8 @@ public void ReplaceWithTsString_OnNodeMinimum_DoesNotThrow_AndMinimumUnchanged() "the synthetic pattern node's Minimum must not be mutated by a discarded edit"); } - // ------------------------------------------------------------------ - // Angle 2: is the crash reachable through PatternView's own input handling (keystrokes), - // or only through paths that bypass it (IME composition, drag-and-drop, or any other - // direct ReplaceWithTsString caller)? PatternView.OnKeyPress unconditionally sets - // e.Handled = true and returns without calling base.OnKeyPress for anything but - // Backspace/Delete, so ordinary WM_CHAR-driven typing never reaches the engine at all. - // This test is expected to PASS today: it documents that the keystroke path is already - // safe, which is what makes the ReplaceWithTsString bypass above the actual bug. - // ------------------------------------------------------------------ + // Ordinary typing never reaches the view engine: OnKeyPress handles everything except + // Backspace and Delete, so only paths that bypass it can crash. [Test] public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash() @@ -486,10 +470,7 @@ public void SimulateTyping_ViaOnKeyPress_DoesNotReachEngine_AndDoesNotCrash() "a plain keystroke must not reach the engine and alter content -- PatternView.OnKeyPress swallows it before that"); } - // ------------------------------------------------------------------ - // Angle 3: insert/delete must keep working. PatternView.OnKeyDown raises - // RemoveItemsRequested for the Delete key; this must survive whatever fix is applied. - // ------------------------------------------------------------------ + // Insert and delete must keep working: OnKeyDown raises RemoveItemsRequested for Delete. // ------------------------------------------------------------------ // Ablation evidence for the fix's layers. @@ -559,20 +540,14 @@ public void ComplexConcControl_WiresViewAsReadOnly() } /// - /// SimpleRootSite.ReadOnlyView's setter forces AcceptsReturn = AcceptsTab = false when set - /// to true (SimpleRootSite.cs), which happens AFTER the Designer's own explicit - /// AcceptsReturn = true / AcceptsTab = false assignments in InitializeComponent's - /// generated-code ordering. This pins the actual, real behaviour change: AcceptsTab was - /// already false before this fix (Designer-set, independent of ReadOnlyView) so Tab - /// navigation out of the pane is unchanged; AcceptsReturn flips from true to false, which - /// is new. Since PatternView.OnKeyPress already swallows Return either way (it is not - /// Backspace/Delete), the observable difference is only where the key is disposed of: it - /// used to reach the control and be silently swallowed there; now IsInputKey(Return) - /// returns false and the key is never delivered to the control at all, so it is processed - /// as an ordinary dialog/navigation key by whatever contains this pane. This control is - /// hosted as a Words-area tool pane (DistFiles/.../Concordance/toolConfiguration.xml), not - /// inside a modal dialog with an AcceptButton, so no default-button activation is expected - /// in practice -- but that is unverified live; see the review doc. + /// SimpleRootSite.ReadOnlyView's setter forces AcceptsReturn and AcceptsTab to false, + /// after the Designer's own assignments in InitializeComponent. AcceptsTab is false + /// either way, so Tab navigation out of the pane is unchanged. AcceptsReturn becomes + /// false, and since PatternView.OnKeyPress swallows Return regardless, the only + /// difference is where the key is disposed of: IsInputKey(Return) returns false, so the + /// key is not delivered to the control and whatever hosts the pane treats it as an + /// ordinary navigation key. The pane is a Words-area tool rather than a modal dialog + /// with an AcceptButton, so no default-button activation is expected. Unverified live. /// [Test] public void ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse() diff --git a/Src/LexText/LexTextControls/PatternView.cs b/Src/LexText/LexTextControls/PatternView.cs index f1de0146b7..571ba32d61 100644 --- a/Src/LexText/LexTextControls/PatternView.cs +++ b/Src/LexText/LexTextControls/PatternView.cs @@ -70,8 +70,10 @@ protected override EditingHelper CreateEditingHelper() } /// - /// Activate() is suppressed by default in ReadOnlyViews (SimpleRootSite.AllowDisplaySelection - /// defaults to IsEditable), but both PatternView consumers are pattern builders whose chooser + /// Activate() is suppressed by default in ReadOnlyViews + /// (SimpleRootSite.AllowDisplaySelection + /// defaults to IsEditable), but both PatternView consumers are pattern builders whose + /// chooser /// insert/delete needs the user to see the current selection even when the view itself is /// read-only. ///