From c64ce7a781bbdfd202207248d1c168d5b1c23453 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 04:19:39 -0400 Subject: [PATCH 1/6] LT-22710: add failing tests for direct editing of rule formula cells Adds the failing tests that demonstrate Docs/bugs/phon-rule-direct-editing.md: - RuleFormulaVcBaseEditabilityTests: RuleFormulaVcBase.Display never sets ktptEditable=NotEditable before AddStringAltMember for the natural-class abbreviation (kfragNC) or the terminal-unit name (kfragTerminalUnit), across RegRuleFormulaVc, MetaRuleFormulaVc, and AffixRuleFormulaVc. - RuleFormulaDirectEditReproTests: drives a real IVwRootBox/PatternView and calls IVwSelection.ReplaceWithTsString directly (bypassing PatternView.OnKeyPress entirely, the same low-level path IME composition or drag-and-drop would use) and shows it actually renames the live PhPhoneme.Name. All 5 tests fail against current code, confirming the defect by direct reproduction rather than code reading alone. --- Docs/bugs/phon-rule-direct-editing.md | 69 +++++ .../RuleFormulaDirectEditReproTests.cs | 165 ++++++++++ .../RuleFormulaVcBaseEditabilityTests.cs | 283 ++++++++++++++++++ 3 files changed, 517 insertions(+) create mode 100644 Docs/bugs/phon-rule-direct-editing.md create mode 100644 Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs create mode 100644 Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs diff --git a/Docs/bugs/phon-rule-direct-editing.md b/Docs/bugs/phon-rule-direct-editing.md new file mode 100644 index 0000000000..96535539eb --- /dev/null +++ b/Docs/bugs/phon-rule-direct-editing.md @@ -0,0 +1,69 @@ +# Bug 1 — Rule formula cells are directly editable, and edits rename the underlying phoneme / natural class + +**Area:** Grammar → Phonological Rules / Affix Processes (rule formula slices) +**Type:** Data corruption +**Related prior work:** LT-21888 (the keystroke filter this report argues is insufficient) + +## Symptom + +The rule formula view is specified to be modifiable only by (a) inserting an item through a chooser and (b) deleting an item. In practice users are sometimes able to modify the content of a cell directly. Because the view renders the *referenced object's own* text field, an edit that lands does not merely corrupt the rule — it renames the phoneme or natural class project-wide, affecting every other rule that references it. + +## Root cause + +Editability is never denied at the view level. It is only filtered at the control level, and the filter covers exactly one input path. + +### The fragments are editable + +`RuleFormulaVcBase.Display` renders the substantive parts of a rule with plain string-alternative calls against the referenced object's real multistring property: + +- `RuleFormulaVcBase.cs:287-303` — `kfragNC` calls `AddStringAltMember` on the natural class's `Abbreviation` / `Name`. +- `RuleFormulaVcBase.cs:305-312` — `kfragTerminalUnit` calls `AddStringAltMember(PhTerminalUnitTags.kflidName, ...)`, i.e. the phoneme's or boundary marker's live `Name`. + +None of `kfragNC`, `kfragTerminalUnit`, `kfragFeatureLine`, or `kfragFeats` sets `ktptEditable = TptEditable.ktptNotEditable`. The **only** place that property is set anywhere in the `RuleFormulaVcBase` / `PatternVcBase` chain is on the blank filler lines: `PatternVcBase.cs:209` (`AddExtraLines`). + +### The rootsite is not read-only + +`RuleFormulaControl.cs:1153` sets `m_view.ReadOnlyView = false`. + +### The only guard is a WM_CHAR filter + +- `PatternView.cs:135-149` — `OnKeyPress` swallows every character except Backspace and Delete. The comment cites LT-21888, i.e. this was itself added as a bug fix. +- `PatternView.cs:41-54` — a custom `PatternEditingHelper` hard-codes `CanCopy` / `CanCut` / `CanPaste` to `false`. + +Anything that reaches the root box without going through `OnKeyPress` or the clipboard helper is unguarded. Candidates, in rough order of likelihood for FLEx users: + +1. **IME composition.** Vernacular-script keyboards commit text through IME messages rather than plain WM_CHAR. This is the most probable real-world trigger and matches the "sometimes" in the report. +2. Drag-and-drop text onto the view. +3. Any other rootsite entry point that mutates the selection's string property directly. + +**Status: CONFIRMED by code reading.** The `ReadOnlyView = false` setting, the absence of `ktptNotEditable` on substantive fragments, and the single-path keystroke filter are all directly verified. The specific IME mechanism is **inferred, not reproduced** — see Verification below. + +## Proposed fix + +Enforce the invariant where it belongs, in the view constructor, rather than patching input paths one at a time. + +1. In `RuleFormulaVcBase`, wrap the substantive fragments in `ktptEditable = TptEditable.ktptNotEditable` before the `AddStringAltMember` calls at `RuleFormulaVcBase.cs:287-312`, and likewise for the feature-line and feature fragments. +2. Consider setting `m_view.ReadOnlyView = true` at `RuleFormulaControl.cs:1153`. This needs checking against the delete path — `PatternView.OnKeyDown` (`PatternView.cs:120-149`) intercepts Delete/Backspace and raises `RemoveItemsRequested` rather than editing text, so a read-only rootsite may still be compatible with deletion, but this must be verified, not assumed. +3. Keep the `OnKeyPress` filter as defence in depth. Do not remove it as part of this fix. + +Option 1 alone is likely sufficient and is the lower-risk change. + +## Verification required before closing + +- Reproduce the original defect with an IME / vernacular keyboard against an unpatched build. Without a repro we are fixing an inferred mechanism. +- Confirm delete still works on all three rule kinds after the change: regular phonological rules, metathesis rules, affix processes. +- Confirm the fix covers metathesis rules. `MetaRuleFormulaControl` / `MetaRuleFormulaVc` were not read line by line; they share `RuleFormulaVcBase` and reuse `CmdCtxtSetFeatures`, so they are expected to share the defect, but this is unverified. + +## Scope + +Independent of Bug 2 (natural class vs. phonological features) and Bug 3 (affix process clone on sense split). No shared code paths beyond both Bug 1 and Bug 2 living in the rule formula UI. + +## Key files + +| Path:line | Role | +|---|---| +| `Src/LexText/Morphology/RuleFormulaVcBase.cs:287-312` | Renders NC abbreviation and phoneme name as editable strings | +| `Src/LexText/LexTextControls/PatternVcBase.cs:209` | The only `ktptNotEditable` in the chain (filler lines only) | +| `Src/LexText/LexTextControls/PatternView.cs:41-54` | Clipboard guard | +| `Src/LexText/LexTextControls/PatternView.cs:120-149` | Keystroke filter and delete interception | +| `Src/LexText/Morphology/RuleFormulaControl.cs:1153` | `ReadOnlyView = false` | diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs new file mode 100644 index 0000000000..b65da24ea1 --- /dev/null +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs @@ -0,0 +1,165 @@ +// 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) + +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.XWorks.MorphologyEditor +{ + /// + /// Reproduces the "phonological-rule formula cells are directly editable" bug + /// (Docs/bugs/phon-rule-direct-editing.md) at the level that matters: does an edit that + /// lands via the rootsite's own selection/text-replacement API (i.e. NOT via + /// PatternView.OnKeyPress, which only filters WM_CHAR) actually corrupt the referenced + /// PhPhoneme's real, project-wide Name? + /// + /// This drives a real IVwRootBox (the managed Views engine) hosted by a real + /// RegRuleFormulaControl-equivalent PatternView/RegRuleFormulaVc pair against a real + /// in-memory LcmCache, then calls IVwSelection.ReplaceWithTsString directly -- the same + /// low-level entry point IME composition or drag-and-drop would use, and one that + /// PatternView.OnKeyPress never sees because it only reacts to Windows key events. + /// + [TestFixture] + public class RuleFormulaDirectEditReproTests : MemoryOnlyBackendProviderTestBase + { + private Mediator m_mediator; + private PropertyTable m_propertyTable; + + 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_propertyTable != null) + { + m_propertyTable.Dispose(); + m_propertyTable = null; + } + if (m_mediator != null) + { + m_mediator.Dispose(); + m_mediator = null; + } + base.TestTearDown(); + } + + /// Minimal no-op IPatternControl -- sufficient because we never drive + /// selection through the chooser/insert/delete UI in this test; we only need + /// PatternView's selection-changed handler not to crash when we install a + /// selection directly. + 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; + } + + /// Exposes the protected layout hook so the view can be laid out headlessly, + /// exactly like RootSiteTests' DummyBasicView.CallLayout(). + private class TestPatternView : PatternView + { + public void CallLayout() + { + OnLayout(new LayoutEventArgs(this, string.Empty)); + } + } + + private IPhPhoneme CreatePhoneme(string name) + { + IPhPhoneme p = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + Cache.LangProject.PhonologicalDataOA.PhonemeSetsOS.Add( + Cache.ServiceLocator.GetInstance().Create()); + p = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.PhonemeSetsOS[0].PhonemesOC.Add(p); + p.Name.SetVernacularDefaultWritingSystem(name); + }); + return p; + } + + /// + /// Builds a real regular-rule RHS whose left context is a single phoneme, hosts it in a + /// live PatternView/RegRuleFormulaVc pair, and returns the phoneme plus the live view. + /// + private (IPhPhoneme phoneme, TestPatternView view) BuildLiveRuleFormulaView(string phonemeName) + { + IPhPhoneme phoneme = CreatePhoneme(phonemeName); + IPhSegRuleRHS rhs = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + var rule = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.PhonRulesOS.Add(rule); + rhs = Cache.ServiceLocator.GetInstance().Create(); + rule.RightHandSidesOS.Add(rhs); + var segCtxt = Cache.ServiceLocator.GetInstance().Create(); + rhs.LeftContextOA = segCtxt; + segCtxt.FeatureStructureRA = phoneme; + }); + + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; + view.Init(m_mediator, m_propertyTable, rhs.Hvo, new NullPatternControl(), vc, RegRuleFormulaVc.kfragRHS, + Cache.MainCacheAccessor); + view.CallLayout(); + return (phoneme, view); + } + + /// + /// Selects the whole displayed phoneme (via its object path from the RHS root, bypassing + /// any WM_CHAR-level filtering entirely -- PatternView.OnKeyPress is never invoked here) + /// and replaces its text directly through IVwSelection.ReplaceWithTsString, exactly the + /// kind of call an IME composition commit or a drag-and-drop would make. + /// + [Test] + public void ReplaceWithTsString_OnPhonemeTerminalUnit_BypassesOnKeyPress_AndShouldNotRenameThePhoneme() + { + var (phoneme, view) = BuildLiveRuleFormulaView("p"); + + var levels = new[] + { + new SelLevInfo { tag = PhSimpleContextSegTags.kflidFeatureStructure, ihvo = 0 }, + new SelLevInfo { tag = PhSegRuleRHSTags.kflidLeftContext, ihvo = 0 } + }; + IVwSelection sel = view.RootBox.MakeTextSelInObj(0, levels.Length, levels, 0, null, + true, false, false, /* fWholeObj */ true, /* fInstall */ true); + Assert.That(sel, Is.Not.Null, + "could not construct a selection over the phoneme's terminal-unit display -- fixture/path assumption is wrong"); + + ITsString corrupted = TsStringUtils.MakeString("CORRUPTED", Cache.DefaultVernWs); + + // This call never goes through PatternView.OnKeyPress -- it is the rootsite's own + // low-level text-replacement API, exactly what bypasses the WM_CHAR filter. It is + // wrapped in a UOW only because the change-tracking infrastructure requires one for + // any edit to commit at all; IME composition and drag-and-drop land inside a UOW + // supplied by the real editing helper, not by PatternView.OnKeyPress. + UndoableUnitOfWorkHelper.Do("undo", "redo", phoneme, () => sel.ReplaceWithTsString(corrupted)); + + string nameAfter = phoneme.Name.VernacularDefaultWritingSystem.Text; + Assert.That(nameAfter, Is.EqualTo("p"), + "an edit that bypassed PatternView.OnKeyPress altered the real PhPhoneme.Name " + + "(got '" + nameAfter + "') -- this is the project-wide-rename data corruption " + + "described in Docs/bugs/phon-rule-direct-editing.md"); + } + } +} diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs new file mode 100644 index 0000000000..9edbae0ff6 --- /dev/null +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs @@ -0,0 +1,283 @@ +// 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) + +using System; +using System.Collections.Generic; +using System.Runtime.InteropServices; +using NUnit.Framework; +using SIL.LCModel; +using SIL.LCModel.Core.KernelInterfaces; +using SIL.LCModel.Infrastructure; +using SIL.FieldWorks.Common.ViewsInterfaces; +using XCore; + +namespace SIL.FieldWorks.XWorks.MorphologyEditor +{ + /// + /// Reproduces the "phonological-rule formula cells are directly editable" bug + /// (Docs/bugs/phon-rule-direct-editing.md). The rule formula view is meant to be + /// modifiable only by chooser-insert and delete; these tests demonstrate that the + /// view constructor never marks the substantive fragments -- the natural class + /// abbreviation and the terminal unit (phoneme/boundary) name -- as non-editable, + /// which is the structural hole that lets anything bypassing the WM_CHAR filter in + /// PatternView.OnKeyPress (e.g. IME composition) rename the referenced object. + /// + [TestFixture] + public class RuleFormulaVcBaseEditabilityTests : MemoryOnlyBackendProviderTestBase + { + private Mediator m_mediator; + private PropertyTable m_propertyTable; + + public override void TestSetup() + { + base.TestSetup(); + m_mediator = new Mediator(); + m_propertyTable = new PropertyTable(m_mediator); + } + + public override void TestTearDown() + { + if (m_propertyTable != null) + { + m_propertyTable.Dispose(); + m_propertyTable = null; + } + if (m_mediator != null) + { + m_mediator.Dispose(); + m_mediator = null; + } + base.TestTearDown(); + } + + private IPhNaturalClass CreateNaturalClass(string abbr) + { + IPhNaturalClass nc = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + nc = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.NaturalClassesOS.Add(nc); + nc.Name.SetAnalysisDefaultWritingSystem("Test Class"); + nc.Abbreviation.SetAnalysisDefaultWritingSystem(abbr); + }); + return nc; + } + + private IPhPhoneme CreatePhoneme(string name) + { + IPhPhoneme p = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + Cache.LangProject.PhonologicalDataOA.PhonemeSetsOS.Add( + Cache.ServiceLocator.GetInstance().Create()); + p = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.PhonemeSetsOS[0].PhonemesOC.Add(p); + p.Name.SetVernacularDefaultWritingSystem(name); + }); + return p; + } + + /// + /// The natural-class abbreviation fragment (kfragNC) is rendered via AddStringAltMember + /// directly against the natural class's own Abbreviation field. Per the bug report, no + /// ktptEditable=NotEditable is ever set before that call. This test currently FAILS, + /// demonstrating the hole; RuleFormulaVcBase.cs:287-303. + /// + [Test] + public void Display_NaturalClassAbbreviationFragment_IsMarkedNotEditable() + { + IPhNaturalClass nc = CreateNaturalClass("Stp"); + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, nc.Hvo, RuleFormulaVcBase.kfragNC); + + Assert.That(env.StringAltMemberCalls, Is.Not.Empty, + "expected AddStringAltMember to be called for the NC abbreviation fragment"); + foreach (var call in env.StringAltMemberCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "the natural class abbreviation must not be directly editable in the rule formula view"); + } + } + + /// + /// The terminal-unit (phoneme/boundary) fragment (kfragTerminalUnit) is rendered via + /// AddStringAltMember directly against the terminal unit's own Name field -- a live + /// write channel into the phoneme's real, project-wide Name. No ktptEditable=NotEditable + /// is ever set. This test currently FAILS; RuleFormulaVcBase.cs:305-312. + /// + [Test] + public void Display_TerminalUnitNameFragment_IsMarkedNotEditable() + { + IPhPhoneme phoneme = CreatePhoneme("p"); + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, phoneme.Hvo, RuleFormulaVcBase.kfragTerminalUnit); + + Assert.That(env.StringAltMemberCalls, Is.Not.Empty, + "expected AddStringAltMember to be called for the terminal unit name fragment"); + foreach (var call in env.StringAltMemberCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "the phoneme/boundary name must not be directly editable in the rule formula view"); + } + } + + /// Same defect, exercised through the metathesis-rule view constructor. + [Test] + public void Display_TerminalUnitNameFragment_ViaMetaRuleFormulaVc_IsMarkedNotEditable() + { + IPhPhoneme phoneme = CreatePhoneme("t"); + var vc = new MetaRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, phoneme.Hvo, RuleFormulaVcBase.kfragTerminalUnit); + + Assert.That(env.StringAltMemberCalls, Is.Not.Empty); + foreach (var call in env.StringAltMemberCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "metathesis rule formula view shares the same defect as the base class"); + } + } + + /// Same defect, exercised through the affix-process view constructor. + [Test] + public void Display_TerminalUnitNameFragment_ViaAffixRuleFormulaVc_IsMarkedNotEditable() + { + IPhPhoneme phoneme = CreatePhoneme("k"); + var vc = new AffixRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, phoneme.Hvo, RuleFormulaVcBase.kfragTerminalUnit); + + Assert.That(env.StringAltMemberCalls, Is.Not.Empty); + foreach (var call in env.StringAltMemberCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "affix process rule formula view shares the same defect as the base class"); + } + } + + /// + /// Records enough of IVwEnv's calls to observe, at the moment AddStringAltMember binds a + /// fragment to a real domain-object field, whether the view constructor had most recently + /// set the ktptEditable property to NotEditable. All other members are unused by the + /// fragments under test and throw if hit, so a future change that routes through a + /// different IVwEnv member will fail loudly rather than silently pass. + /// + private class EditabilityRecordingEnv : IVwEnv + { + public struct Call + { + public int Tag; + public int Ws; + public int EditableAtCallTime; + } + + public List StringAltMemberCalls = new List(); + + private int m_currentEditable = int.MinValue; // sentinel: never set + + public void AddStringAltMember(int tag, int ws, IVwViewConstructor _vwvc) + { + StringAltMemberCalls.Add(new Call { Tag = tag, Ws = ws, EditableAtCallTime = m_currentEditable }); + } + + public void set_IntProperty(int tpt, int tpv, int nValue) + { + if (tpt == (int)FwTextPropType.ktptEditable) + m_currentEditable = nValue; + } + + public ITsTextProps Props + { + set { /* not relevant to editability of these two fragments */ } + } + + public void get_StringWidth(ITsString _tss, ITsTextProps _ttp, out int dmpx, out int dmpy) + { + dmpx = 0; + dmpy = 0; + } + + public int OpenObject + { + get { throw new NotImplementedException(); } + } + + public int EmbeddingLevel + { + get { return 0; } + } + + public ISilDataAccess DataAccess + { + get { throw new NotImplementedException(); } + } + + public void AddObjProp(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddObjVec(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddObjVecItems(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddReversedObjVecItems(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddObj(int hvo, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddLazyVecItems(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddLazyItems(int[] _rghvo, int chvo, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddProp(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void AddDerivedProp(int[] _rgtag, int ctag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } + public void NoteDependency(int[] _rghvo, int[] _rgtag, int chvo) { } + public void NoteStringValDependency(int hvo, int tag, int ws, ITsString _tssVal) { throw new NotImplementedException(); } + public void AddStringProp(int tag, IVwViewConstructor _vwvc) { throw new NotImplementedException(); } + public void AddUnicodeProp(int tag, int ws, IVwViewConstructor _vwvc) { throw new NotImplementedException(); } + public void AddIntProp(int tag) { throw new NotImplementedException(); } + public void AddIntPropPic(int tag, IVwViewConstructor _vc, int frag, int nMin, int nMax) { throw new NotImplementedException(); } + public void AddStringAlt(int tag) { throw new NotImplementedException(); } + public void AddStringAltSeq(int tag, int[] _rgenc, int cws) { throw new NotImplementedException(); } + public void AddString(ITsString _ss) { throw new NotImplementedException(); } + public void AddTimeProp(int tag, uint flags) { throw new NotImplementedException(); } + public int CurrentObject() { throw new NotImplementedException(); } + public void GetOuterObject(int ichvoLevel, out int _hvo, out int _tag, out int _ihvo) { throw new NotImplementedException(); } + public void AddWindow(IVwEmbeddedWindow _ew, int dmpAscent, bool fJustifyRight, bool fAutoShow) { throw new NotImplementedException(); } + public void AddSeparatorBar() { throw new NotImplementedException(); } + public void AddSimpleRect(int rgb, int dmpWidth, int dmpHeight, int dmpBaselineOffset) { throw new NotImplementedException(); } + public void OpenDiv() { throw new NotImplementedException(); } + public void CloseDiv() { throw new NotImplementedException(); } + public void OpenParagraph() { throw new NotImplementedException(); } + public void OpenTaggedPara() { throw new NotImplementedException(); } + public void OpenMappedPara() { throw new NotImplementedException(); } + public void OpenMappedTaggedPara() { throw new NotImplementedException(); } + public void OpenConcPara(int ichMinItem, int ichLimItem, VwConcParaOpts cpoFlags, int dmpAlign) { throw new NotImplementedException(); } + public void OpenOverridePara(int cOverrideProperties, DispPropOverride[] _rgOverrideProperties) { throw new NotImplementedException(); } + public void CloseParagraph() { throw new NotImplementedException(); } + public void OpenInnerPile() { throw new NotImplementedException(); } + public void CloseInnerPile() { throw new NotImplementedException(); } + public void OpenSpan() { throw new NotImplementedException(); } + public void CloseSpan() { throw new NotImplementedException(); } + public void OpenTable(int cCols, VwLength vlWidth, int mpBorder, VwAlignment vwalign, VwFramePosition frmpos, VwRule vwrule, int mpSpacing, int mpPadding, bool fSelectOneCol) { throw new NotImplementedException(); } + public void CloseTable() { throw new NotImplementedException(); } + public void OpenTableRow() { throw new NotImplementedException(); } + public void CloseTableRow() { throw new NotImplementedException(); } + public void OpenTableCell(int nRowSpan, int nColSpan) { throw new NotImplementedException(); } + public void CloseTableCell() { throw new NotImplementedException(); } + public void OpenTableHeaderCell(int nRowSpan, int nColSpan) { throw new NotImplementedException(); } + public void CloseTableHeaderCell() { throw new NotImplementedException(); } + public void MakeColumns(int nColSpan, VwLength vlWidth) { throw new NotImplementedException(); } + public void MakeColumnGroup(int nColSpan, VwLength vlWidth) { throw new NotImplementedException(); } + public void OpenTableHeader() { throw new NotImplementedException(); } + public void CloseTableHeader() { throw new NotImplementedException(); } + public void OpenTableFooter() { throw new NotImplementedException(); } + public void CloseTableFooter() { throw new NotImplementedException(); } + public void OpenTableBody() { throw new NotImplementedException(); } + public void CloseTableBody() { throw new NotImplementedException(); } + public void set_StringProperty(int sp, string bstrValue) { throw new NotImplementedException(); } + public void AddPictureWithCaption(IPicture _pict, int tag, ITsTextProps _ttpCaption, int hvoCmFile, int ws, int dxmpWidth, int dympHeight, IVwViewConstructor _vwvc) { throw new NotImplementedException(); } + public void AddPicture(IPicture _pict, int tag, int dxmpWidth, int dympHeight) { throw new NotImplementedException(); } + public void SetParagraphMark(VwBoundaryMark boundaryMark) { throw new NotImplementedException(); } + public void EmptyParagraphBehavior(int behavior) { throw new NotImplementedException(); } + public bool IsParagraphOpen() { throw new NotImplementedException(); } + } + } +} From 4f9701998f7380fa6c87b38817b9dfca32ff718a Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 04:42:33 -0400 Subject: [PATCH 2/6] LT-22710: make rule formula fragments and rootsite non-editable Enforces "a rule cell is not free text" structurally instead of patching another input path: - RuleFormulaVcBase.Display now sets ktptEditable=NotEditable before every AddStringAltMember/AddProp call that binds a fragment to a real domain object's field (natural class abbreviation/name, terminal unit name) or to a computed feature/variable line. This is shared by RegRuleFormulaVc, MetaRuleFormulaVc, and AffixRuleFormulaVc, so all three rule kinds are covered by one change. - RuleFormulaControl now sets m_view.ReadOnlyView = true. This also unregisters the keyboard/IME controller hook for the view (see SimpleRootSite.ReadOnlyView), closing the IME-composition bypass, not just the WM_CHAR path PatternView.OnKeyPress already filtered. - PatternView.AllowDisplaySelection now always returns true (the established pattern also used by InterlinPrintView/InterlinTaggingChild), so the now-read-only rootsite still shows a visible selection for chooser insert/delete to act on. - PatternView.OnKeyPress is left in place as defence in depth. Adds a test confirming Delete still raises RemoveItemsRequested with the rootsite read-only, and disposes the test view to avoid a finalizer-thread COM cleanup race. All 6 reproduction/fix tests pass; MorphologyEditorDllTests (13), LexTextControlsTests (356), and ITextDllTests (208) show no regressions. --- Src/LexText/LexTextControls/PatternView.cs | 9 ++++ .../RuleFormulaDirectEditReproTests.cs | 54 ++++++++++++++----- .../RuleFormulaVcBaseEditabilityTests.cs | 18 +++---- Src/LexText/Morphology/RuleFormulaControl.cs | 3 +- Src/LexText/Morphology/RuleFormulaVcBase.cs | 8 +++ 5 files changed, 65 insertions(+), 27 deletions(-) diff --git a/Src/LexText/LexTextControls/PatternView.cs b/Src/LexText/LexTextControls/PatternView.cs index 27bbb76eca..5337604c6b 100644 --- a/Src/LexText/LexTextControls/PatternView.cs +++ b/Src/LexText/LexTextControls/PatternView.cs @@ -69,6 +69,15 @@ protected override EditingHelper CreateEditingHelper() return new PatternEditingHelper(Cache, this); } + /// + /// Activate() is disabled by default in ReadOnlyViews, but a pattern editor does want to + /// show selections so the user can see what a chooser insert/delete will act on. + /// + 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(); diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs index b65da24ea1..fdcbd7a559 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs @@ -16,23 +16,18 @@ namespace SIL.FieldWorks.XWorks.MorphologyEditor { /// - /// Reproduces the "phonological-rule formula cells are directly editable" bug - /// (Docs/bugs/phon-rule-direct-editing.md) at the level that matters: does an edit that - /// lands via the rootsite's own selection/text-replacement API (i.e. NOT via - /// PatternView.OnKeyPress, which only filters WM_CHAR) actually corrupt the referenced - /// PhPhoneme's real, project-wide Name? - /// - /// This drives a real IVwRootBox (the managed Views engine) hosted by a real - /// RegRuleFormulaControl-equivalent PatternView/RegRuleFormulaVc pair against a real - /// in-memory LcmCache, then calls IVwSelection.ReplaceWithTsString directly -- the same - /// low-level entry point IME composition or drag-and-drop would use, and one that - /// PatternView.OnKeyPress never sees because it only reacts to Windows key events. + /// Drives a real IVwRootBox (the managed Views engine), hosted by a live + /// PatternView/RegRuleFormulaVc pair against a real in-memory LcmCache, and calls + /// 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. /// [TestFixture] public class RuleFormulaDirectEditReproTests : MemoryOnlyBackendProviderTestBase { private Mediator m_mediator; private PropertyTable m_propertyTable; + private TestPatternView m_view; public override void TestSetup() { @@ -44,6 +39,11 @@ public override void TestSetup() public override void TestTearDown() { + if (m_view != null) + { + m_view.Dispose(); + m_view = null; + } if (m_propertyTable != null) { m_propertyTable.Dispose(); @@ -82,6 +82,12 @@ public void CallLayout() { OnLayout(new LayoutEventArgs(this, string.Empty)); } + + public void SimulateKeyDown(Keys key) + { + var e = new KeyEventArgs(key); + OnKeyDown(e); + } } private IPhPhoneme CreatePhoneme(string name) @@ -121,7 +127,9 @@ private IPhPhoneme CreatePhoneme(string name) var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; view.Init(m_mediator, m_propertyTable, rhs.Hvo, new NullPatternControl(), vc, RegRuleFormulaVc.kfragRHS, Cache.MainCacheAccessor); + view.ReadOnlyView = true; view.CallLayout(); + m_view = view; return (phoneme, view); } @@ -157,9 +165,27 @@ public void ReplaceWithTsString_OnPhonemeTerminalUnit_BypassesOnKeyPress_AndShou string nameAfter = phoneme.Name.VernacularDefaultWritingSystem.Text; Assert.That(nameAfter, Is.EqualTo("p"), - "an edit that bypassed PatternView.OnKeyPress altered the real PhPhoneme.Name " + - "(got '" + nameAfter + "') -- this is the project-wide-rename data corruption " + - "described in Docs/bugs/phon-rule-direct-editing.md"); + "an edit that bypassed PatternView.OnKeyPress altered the real, project-wide " + + "PhPhoneme.Name (got '" + nameAfter + "')"); + } + + /// + /// A read-only rootsite must not prevent PatternView's own Delete-key handling, which + /// removes items through RemoveItemsRequested rather than by editing text. + /// + [Test] + public void DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly() + { + var (_, view) = BuildLiveRuleFormulaView("p"); + Assert.That(view.ReadOnlyView, Is.True, "fixture assumption: the rootsite is read-only"); + + bool removeRequested = false; + view.RemoveItemsRequested += (sender, e) => removeRequested = true; + + view.SimulateKeyDown(Keys.Delete); + + Assert.That(removeRequested, Is.True, + "Delete must still raise RemoveItemsRequested when the rootsite is read-only"); } } } diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs index 9edbae0ff6..f5216c68e3 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs @@ -15,13 +15,10 @@ namespace SIL.FieldWorks.XWorks.MorphologyEditor { /// - /// Reproduces the "phonological-rule formula cells are directly editable" bug - /// (Docs/bugs/phon-rule-direct-editing.md). The rule formula view is meant to be - /// modifiable only by chooser-insert and delete; these tests demonstrate that the - /// view constructor never marks the substantive fragments -- the natural class - /// abbreviation and the terminal unit (phoneme/boundary) name -- as non-editable, - /// which is the structural hole that lets anything bypassing the WM_CHAR filter in - /// PatternView.OnKeyPress (e.g. IME composition) rename the referenced object. + /// The rule formula view is modifiable only by chooser-insert and delete, so + /// RuleFormulaVcBase must mark the natural-class abbreviation and terminal-unit + /// (phoneme/boundary) name fragments non-editable; both bind directly to the + /// referenced object's own live string field. /// [TestFixture] public class RuleFormulaVcBaseEditabilityTests : MemoryOnlyBackendProviderTestBase @@ -80,9 +77,7 @@ private IPhPhoneme CreatePhoneme(string name) /// /// The natural-class abbreviation fragment (kfragNC) is rendered via AddStringAltMember - /// directly against the natural class's own Abbreviation field. Per the bug report, no - /// ktptEditable=NotEditable is ever set before that call. This test currently FAILS, - /// demonstrating the hole; RuleFormulaVcBase.cs:287-303. + /// directly against the natural class's own Abbreviation field. /// [Test] public void Display_NaturalClassAbbreviationFragment_IsMarkedNotEditable() @@ -105,8 +100,7 @@ public void Display_NaturalClassAbbreviationFragment_IsMarkedNotEditable() /// /// The terminal-unit (phoneme/boundary) fragment (kfragTerminalUnit) is rendered via /// AddStringAltMember directly against the terminal unit's own Name field -- a live - /// write channel into the phoneme's real, project-wide Name. No ktptEditable=NotEditable - /// is ever set. This test currently FAILS; RuleFormulaVcBase.cs:305-312. + /// write channel into the phoneme's real, project-wide name. /// [Test] public void Display_TerminalUnitNameFragment_IsMarkedNotEditable() diff --git a/Src/LexText/Morphology/RuleFormulaControl.cs b/Src/LexText/Morphology/RuleFormulaControl.cs index 88035e5afa..c433231c86 100644 --- a/Src/LexText/Morphology/RuleFormulaControl.cs +++ b/Src/LexText/Morphology/RuleFormulaControl.cs @@ -1150,7 +1150,8 @@ private void InitializeComponent() this.m_view.Location = new System.Drawing.Point(0, 0); this.m_view.Mediator = null; this.m_view.Name = "m_view"; - this.m_view.ReadOnlyView = false; + // A rule formula cell is modifiable only via chooser-insert and delete, never free text. + 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/Morphology/RuleFormulaVcBase.cs b/Src/LexText/Morphology/RuleFormulaVcBase.cs index b316717761..22d8083d2c 100644 --- a/Src/LexText/Morphology/RuleFormulaVcBase.cs +++ b/Src/LexText/Morphology/RuleFormulaVcBase.cs @@ -285,6 +285,8 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) break; case kfragNC: + // This renders the referenced natural class's own Abbreviation/Name; a rule cell is never free text. + vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); int ncWs = WritingSystemServices.ActualWs(m_cache, WritingSystemServices.kwsFirstAnal, hvo, PhNaturalClassTags.kflidAbbreviation); if (ncWs != 0) @@ -303,6 +305,8 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) break; case kfragTerminalUnit: + // This renders the referenced phoneme's or boundary marker's own live Name. + vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); int tuWs = WritingSystemServices.ActualWs(m_cache, WritingSystemServices.kwsFirstVern, hvo, PhTerminalUnitTags.kflidName); if (tuWs != 0) @@ -320,14 +324,18 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) break; case kfragFeature: + // This is a computed "abbreviation value" line, not free text. + vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); vwenv.AddProp(ktagFeature, this, kfragFeatureLine); break; case kfragPlusVariable: + vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); vwenv.AddProp(ktagVariable, this, kfragPlusVariableLine); break; case kfragMinusVariable: + vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); vwenv.AddProp(ktagVariable, this, kfragMinusVariableLine); break; } From 7434d6f013ce7dcce5e1b3b15cf4b3d36400b39d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 04:50:05 -0400 Subject: [PATCH 3/6] LT-22710: add architecture self-review for the direct-editing fix Documents which layer owns the invariant, an experiment isolating that the view-constructor ktptEditable marking (not ReadOnlyView) is what actually blocks IVwSelection.ReplaceWithTsString, what was deliberately left unremoved (PatternEditingHelper's CanCut/CanPaste, shared with ComplexConcControl) and unfixed (ComplexConcPatternVc, unaudited fake-tag/literal spans elsewhere in the VC family), and manual verification still needed in a running FLEx. --- Docs/bugs/phon-rule-direct-editing-review.md | 100 +++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 Docs/bugs/phon-rule-direct-editing-review.md diff --git a/Docs/bugs/phon-rule-direct-editing-review.md b/Docs/bugs/phon-rule-direct-editing-review.md new file mode 100644 index 0000000000..314a81c842 --- /dev/null +++ b/Docs/bugs/phon-rule-direct-editing-review.md @@ -0,0 +1,100 @@ +# Review: rule formula cells are directly editable (architecture self-review) + +## 1. Is this the right architecture for the invariant? + +The invariant is "a rule cell is not free text; it only changes via chooser-insert and +delete." That invariant actually has two independent parts, and each belongs at a +different layer: + +- **"This span of text is bound to a real domain object's field and must not accept an + edit."** This is a per-fragment fact that only the view constructor knows, because only + it knows which `AddStringAltMember`/`AddProp` calls bind to a real, shared field + (`PhNaturalClass.Abbreviation`, `PhTerminalUnit.Name`) versus a fake tag or a filler + line. `RuleFormulaVcBase.Display` is exactly the place this was missing, and it is where + the fix went (five call sites: the natural-class abbreviation, the terminal-unit name, + and the three computed feature/variable lines). +- **"This whole widget only accepts chooser-insert and delete, never typed input."** This + is a fact about the control, not about any one fragment, and it belongs on the rootsite. + `RuleFormulaControl` now sets `m_view.ReadOnlyView = true` instead of `false`. + +Both were previously expressed only as a third thing: an input-path filter +(`PatternView.OnKeyPress` swallowing everything but Backspace/Delete). An input-path +filter is the wrong layer for either fact above -- it has to be re-derived and +re-applied for every new input path (keyboard, IME, drag-and-drop, programmatic paste), +and it says nothing about which content is actually safe to bind live. Fixing at the two +layers above is categorical: a new fragment added to any of the three rule-kind view +constructors is safe by default only if its author remembers to mark it, which is still +not perfect, but a new *input path* into an already-correctly-marked view is safe with no +further action, which is the property the old design did not have. + +**An experiment confirms which half is load-bearing.** Temporarily removing the +`ktptEditable` line for `kfragTerminalUnit` while leaving `ReadOnlyView = true` in place +reproduces the corruption again (`ReplaceWithTsString` still renames the phoneme). +Removing it back and only relying on the view-constructor fix, with `ReadOnlyView` never +touched, was already proven sufficient in the first fix iteration. So: the +`ktptEditable` marking is the control that actually stops a direct `ReplaceWithTsString` +call; `ReadOnlyView` does not gate that low-level API at all. `ReadOnlyView = true` earns +its place for a different reason -- it unregisters the keyboard/IME controller hook +(`SimpleRootSite.UnsubscribeFromRootSiteEventHandlerEvents`), which is the categorical fix +for the IME-composition bypass the original report flagged as the most likely real-world +trigger, and it disables `EditingHelper.CanCut`/`CanPaste` so cut/paste menu commands +stop offering to mutate the view. Both layers are necessary; neither is sufficient alone. + +## 2. What can be removed or simplified? + +Nothing was safely removable. The natural candidate was +`PatternView.PatternEditingHelper.CanCut()`/`CanPaste()`, which look redundant now that +`EditingHelper.CanCut`/`CanPaste` already return `false` whenever `Editable` is `false` +(which it now always is for `RuleFormulaControl`'s view). They were **not** removed, +because `PatternView` is also instantiated directly by `ComplexConcControl` (the complex +concordance pattern builder), which leaves `ReadOnlyView = false` and depends on this same +override to keep cut/paste disabled while still being interactively editable in other +respects. Removing the override would silently enable clipboard paste into that unrelated +feature. `CanCopy()` also stays for a different reason: the base implementation does not +consult `Editable` at all, so it is not made redundant by `ReadOnlyView` -- it is a +deliberate, independent restriction that this fix does not touch. + +`PatternView.OnKeyPress` also stays (see LT-21888 in the class's existing comment). It is +not dead: the audit below found several literal separator glyphs and fake-tag-bound +"index"/boundary strings across `MetaRuleFormulaVc`/`AffixRuleFormulaVc` that were never +audited for `ktptEditable` and are outside this bug's scope (see next section). Those +spans do not corrupt real data if edited -- they are not bound to a real field -- but +`OnKeyPress` is the only thing currently stopping a keystroke from reaching them. Removing +it would trade one narrow, already-fixed hole for a wider, unaudited one. + +## 3. What was not fixed, and why + +- **`ComplexConcPatternVc`** (the complex-concordance pattern builder) extends the same + `PatternVcBase` and is built on the same "chooser insert/delete only" premise, but was + not inspected fragment-by-fragment or fixed. It is a different feature with no test + coverage in this session's reach, and changing its rootsite's editability was + deliberately left alone (see section 2). It is worth a follow-up audit using the same + method used here. +- **Literal/fake-tag spans elsewhere in the same VC family** -- bracket glyphs + (`kfragLeftBracket`/`kfragRightBracket`, via `m_bracketProps`, which never sets + `ktptEditable`), zero-width boundary markers (`ktagLeftBoundary`/`ktagRightBoundary`), + and several `MetaRuleFormulaVc`/`AffixRuleFormulaVc` fields (`m_inputCtxtProps`-rendered + content, `ktagIndex`, `ktagLeftEmpty`/`ktagRightEmpty`) were not individually marked. + None of them bind to a real, shared domain field the way `kfragNC`/`kfragTerminalUnit` + do, so an edit landing there cannot rename a phoneme or natural class -- the actual bug + in scope -- but an edit attempt against a fake tag is unverified territory (it may throw, + or silently no-op, depending on how the data access layer handles an unknown flid). This + is exactly the gap `OnKeyPress` is still covering. +- **The IME repro itself.** The bug report's own "Verification required" section already + flagged this: the IME mechanism was inferred, not reproduced, because it requires a live + IME/vernacular keyboard. That remains true after this fix; see section 4. + +## 4. What still needs manual verification in a running FLEx + +- Open a phonological rule (or metathesis rule, or affix process) with an IME or + vernacular keyboard active, place the insertion point in a rule cell, and confirm that + IME composition can no longer commit text into the cell (it should behave as if the + control has no keyboard focus for typing at all, since the keyboard controller no longer + has this control registered). +- Confirm the selection highlight is still visible when clicking into a rule cell (this + is what `PatternView.AllowDisplaySelection` restores), and that the chooser + insert/delete buttons still operate against the right selection. +- Confirm Delete and Backspace still remove the selected item in a live UI for all three + rule kinds, matching the headless test added here. +- Try drag-and-drop of text onto a rule cell; confirm it either does nothing or is + rejected, rather than landing. From 38213cc15d50fa22116d18076bc1287283ad5315 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 06:16:03 -0400 Subject: [PATCH 4/6] LT-22710: close mutation-testing coverage gaps - Add RuleFormulaControlWiringTests: constructs the real Reg/Meta/Affix rule formula controls and asserts ReadOnlyView on the shipped rootsite, so the production RuleFormulaControl.cs wiring is covered rather than only the test-only view built in RuleFormulaDirectEditReproTests. - Extend EditabilityRecordingEnv to record AddProp calls (previously it threw NotImplementedException on the exact call kfragFeature/kfragPlusVariable/ kfragMinusVariable make) and add one test per fragment. - Add AllowDisplaySelection_IsTrue_WhenRootsiteIsReadOnly. - Add the natural-class equivalent of the phoneme ReplaceWithTsString repro: builds a real PhSimpleContextNC special-cased to display only its abbreviation, selects it through the real rootbox, and confirms a direct ReplaceWithTsString cannot rename PhNaturalClass.Abbreviation -- the same end-to-end standard already applied to the phoneme path. Each addition was confirmed by ablation: flipping RuleFormulaControl's ReadOnlyView back to false, deleting the three feature/variable ktptEditable lines, removing AllowDisplaySelection, and removing kfragNC's ktptEditable line each turn exactly the new, targeted test(s) red and nothing else; ablating kfragNC's marking also reproduces "CORRUPTED" in PhNaturalClass.Abbreviation end-to-end, mirroring the phoneme case. Also corrects the review doc's characterization of ComplexConcPatternVc: it has no real domain-field bindings to corrupt, so a direct edit throws NotImplementedException out of UpdateProp rather than renaming anything -- a crash risk, not a data-corruption risk. Notes the audit surface is closed (PatternVcBase has exactly two subclasses, PatternView exactly two consumers) and flags ConstChartVc's apparent cell-level guard as unverified/SUSPECTED-safe rather than confirmed clean. --- Docs/bugs/phon-rule-direct-editing-review.md | 31 +++++-- .../RuleFormulaControlWiringTests.cs | 47 ++++++++++ .../RuleFormulaDirectEditReproTests.cs | 89 +++++++++++++++++++ .../RuleFormulaVcBaseEditabilityTests.cs | 73 ++++++++++++++- 4 files changed, 231 insertions(+), 9 deletions(-) create mode 100644 Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs diff --git a/Docs/bugs/phon-rule-direct-editing-review.md b/Docs/bugs/phon-rule-direct-editing-review.md index 314a81c842..5f03d41102 100644 --- a/Docs/bugs/phon-rule-direct-editing-review.md +++ b/Docs/bugs/phon-rule-direct-editing-review.md @@ -64,12 +64,22 @@ it would trade one narrow, already-fixed hole for a wider, unaudited one. ## 3. What was not fixed, and why -- **`ComplexConcPatternVc`** (the complex-concordance pattern builder) extends the same - `PatternVcBase` and is built on the same "chooser insert/delete only" premise, but was - not inspected fragment-by-fragment or fixed. It is a different feature with no test - coverage in this session's reach, and changing its rootsite's editability was - deliberately left alone (see section 2). It is worth a follow-up audit using the same - method used here. +The audit surface here is closed, not open-ended: `PatternVcBase` has exactly two +subclasses (`RuleFormulaVcBase`, fixed here, and `ComplexConcPatternVc`) and `PatternView` +has exactly two consumers (`RuleFormulaControl` and `ComplexConcControl`). Both are +accounted for below. + +- **`ComplexConcPatternVc`/`ComplexConcControl`** (the complex-concordance pattern + builder) share `PatternVcBase`/`PatternView` and the same "chooser insert/delete only" + premise, but were deliberately left alone -- this is a different feature with no test + coverage in this session's reach. This is confirmed to be a *different* defect than the + one fixed here: `ComplexConcPatternVc` never overrides `UpdateProp`, so a direct edit that + bypasses `OnKeyPress` (the same `ReplaceWithTsString` path used above) throws an unhandled + `NotImplementedException` out of the Views engine rather than renaming anything. It cannot + reproduce this bug's corruption, because it binds no real domain fields the way + `kfragNC`/`kfragTerminalUnit` do -- so the "keep `CanCut`/`CanPaste` for + `ComplexConcControl`" reasoning in section 2 still stands. The crash risk is its own, + separate issue. - **Literal/fake-tag spans elsewhere in the same VC family** -- bracket glyphs (`kfragLeftBracket`/`kfragRightBracket`, via `m_bracketProps`, which never sets `ktptEditable`), zero-width boundary markers (`ktagLeftBoundary`/`ktagRightBoundary`), @@ -78,8 +88,13 @@ it would trade one narrow, already-fixed hole for a wider, unaudited one. None of them bind to a real, shared domain field the way `kfragNC`/`kfragTerminalUnit` do, so an edit landing there cannot rename a phoneme or natural class -- the actual bug in scope -- but an edit attempt against a fake tag is unverified territory (it may throw, - or silently no-op, depending on how the data access layer handles an unknown flid). This - is exactly the gap `OnKeyPress` is still covering. + the same way `ComplexConcPatternVc` does, or silently no-op). This is exactly the gap + `OnKeyPress` is still covering. +- **`ConstChartVc`** (`ConstChartVc.cs:297`) has the same defect shape (a fragment bound to + a real field with no visible `ktptEditable` marking), but is reported to be guarded at the + cell level by `MakeCellsMethod.cs:495`. That guard was checked by reading the code only, + not by mutation or a live reproduction attempt, so it is unverified/SUSPECTED-safe, not + confirmed clean. - **The IME repro itself.** The bug report's own "Verification required" section already flagged this: the IME mechanism was inferred, not reproduced, because it requires a live IME/vernacular keyboard. That remains true after this fix; see section 4. diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs new file mode 100644 index 0000000000..0ea12f0bf4 --- /dev/null +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs @@ -0,0 +1,47 @@ +// 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) + +using NUnit.Framework; + +namespace SIL.FieldWorks.XWorks.MorphologyEditor +{ + /// + /// Proves the shipped wiring, not a test-only substitute: constructing each real rule + /// formula control must produce a read-only rootsite, since a rule cell is modifiable only + /// by chooser-insert and delete. + /// + [TestFixture] + public class RuleFormulaControlWiringTests + { + [Test] + public void RegRuleFormulaControl_RootSiteIsReadOnly() + { + using (var control = new RegRuleFormulaControl(null)) + { + Assert.That(control.RootSite.ReadOnlyView, Is.True, + "RegRuleFormulaControl must wire up a read-only rootsite"); + } + } + + [Test] + public void MetaRuleFormulaControl_RootSiteIsReadOnly() + { + using (var control = new MetaRuleFormulaControl(null)) + { + Assert.That(control.RootSite.ReadOnlyView, Is.True, + "MetaRuleFormulaControl must wire up a read-only rootsite"); + } + } + + [Test] + public void AffixRuleFormulaControl_RootSiteIsReadOnly() + { + using (var control = new AffixRuleFormulaControl(null)) + { + Assert.That(control.RootSite.ReadOnlyView, Is.True, + "AffixRuleFormulaControl must wire up a read-only rootsite"); + } + } + } +} diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs index fdcbd7a559..2411e661b0 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs @@ -88,6 +88,8 @@ public void SimulateKeyDown(Keys key) var e = new KeyEventArgs(key); OnKeyDown(e); } + + public bool TestAllowDisplaySelection => AllowDisplaySelection; } private IPhPhoneme CreatePhoneme(string name) @@ -133,6 +135,79 @@ private IPhPhoneme CreatePhoneme(string name) return (phoneme, view); } + /// + /// Builds a real regular-rule RHS whose left context is a natural class special-cased to + /// display only its abbreviation ("C" or "V", per RuleFormulaVcBase's kfragNC branch), + /// hosts it in a live PatternView/RegRuleFormulaVc pair, and returns the natural class + /// plus the live view. + /// + private (IPhNaturalClass naturalClass, TestPatternView view) BuildLiveRuleFormulaViewWithNaturalClass(string abbr) + { + IPhNaturalClass nc = null; + IPhSegRuleRHS rhs = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, () => + { + nc = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.NaturalClassesOS.Add(nc); + nc.Name.SetAnalysisDefaultWritingSystem("Test Class"); + nc.Abbreviation.SetAnalysisDefaultWritingSystem(abbr); + + var rule = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.PhonRulesOS.Add(rule); + rhs = Cache.ServiceLocator.GetInstance().Create(); + rule.RightHandSidesOS.Add(rhs); + var ncCtxt = Cache.ServiceLocator.GetInstance().Create(); + rhs.LeftContextOA = ncCtxt; + ncCtxt.FeatureStructureRA = nc; + // GetNumLines(ncCtxt) must be exactly 1 to hit the "C"/"V" abbreviation-only + // branch; one plus-constraint variable is the cheapest way to make that so. + var constraint = Cache.ServiceLocator.GetInstance().Create(); + Cache.LangProject.PhonologicalDataOA.FeatConstraintsOS.Add(constraint); + ncCtxt.PlusConstrRS.Add(constraint); + }); + + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var view = new TestPatternView { Cache = Cache, Visible = false, Width = 300, Height = 60 }; + view.Init(m_mediator, m_propertyTable, rhs.Hvo, new NullPatternControl(), vc, RegRuleFormulaVc.kfragRHS, + Cache.MainCacheAccessor); + view.ReadOnlyView = true; + view.CallLayout(); + m_view = view; + return (nc, view); + } + + /// + /// Selects the whole displayed natural-class abbreviation (via its object path from the + /// RHS root, bypassing PatternView.OnKeyPress entirely) and replaces its text directly + /// through IVwSelection.ReplaceWithTsString. A natural class is shared by every rule that + /// references it, so an edit landing here is a project-wide rename, exactly like the + /// phoneme case. + /// + [Test] + public void ReplaceWithTsString_OnNaturalClassAbbreviation_BypassesOnKeyPress_AndShouldNotRenameTheClass() + { + var (naturalClass, view) = BuildLiveRuleFormulaViewWithNaturalClass("C"); + + var levels = new[] + { + new SelLevInfo { tag = PhSimpleContextNCTags.kflidFeatureStructure, ihvo = 0 }, + new SelLevInfo { tag = PhSegRuleRHSTags.kflidLeftContext, ihvo = 0 } + }; + IVwSelection sel = view.RootBox.MakeTextSelInObj(0, levels.Length, levels, 0, null, + true, false, false, /* fWholeObj */ true, /* fInstall */ true); + Assert.That(sel, Is.Not.Null, + "could not construct a selection over the natural class's abbreviation display -- fixture/path assumption is wrong"); + + ITsString corrupted = TsStringUtils.MakeString("CORRUPTED", Cache.DefaultAnalWs); + + UndoableUnitOfWorkHelper.Do("undo", "redo", naturalClass, () => sel.ReplaceWithTsString(corrupted)); + + string abbrAfter = naturalClass.Abbreviation.AnalysisDefaultWritingSystem.Text; + Assert.That(abbrAfter, Is.EqualTo("C"), + "an edit that bypassed PatternView.OnKeyPress altered the real, project-wide " + + "PhNaturalClass.Abbreviation (got '" + abbrAfter + "')"); + } + /// /// Selects the whole displayed phoneme (via its object path from the RHS root, bypassing /// any WM_CHAR-level filtering entirely -- PatternView.OnKeyPress is never invoked here) @@ -187,5 +262,19 @@ public void DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly() Assert.That(removeRequested, Is.True, "Delete must still raise RemoveItemsRequested when the rootsite is read-only"); } + + /// + /// A read-only rootsite suppresses Activate() by default (SimpleRootSite.AllowDisplaySelection), + /// which would hide the selection a chooser insert/delete needs the user to see. + /// + [Test] + public void AllowDisplaySelection_IsTrue_WhenRootsiteIsReadOnly() + { + var (_, view) = BuildLiveRuleFormulaView("p"); + Assert.That(view.ReadOnlyView, Is.True, "fixture assumption: the rootsite is read-only"); + + Assert.That(view.TestAllowDisplaySelection, Is.True, + "the selection must still be shown even though the rootsite is read-only"); + } } } diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs index f5216c68e3..2c42fda99e 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs @@ -156,6 +156,65 @@ public void Display_TerminalUnitNameFragment_ViaAffixRuleFormulaVc_IsMarkedNotEd } } + /// + /// The feature-value line (kfragFeature) is a computed "abbreviation value" string bound + /// to a fake tag, not free text. + /// + [Test] + public void Display_FeatureLineFragment_IsMarkedNotEditable() + { + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, 0, RuleFormulaVcBase.kfragFeature); + + Assert.That(env.AddPropCalls, Is.Not.Empty, + "expected AddProp to be called for the feature-line fragment"); + foreach (var call in env.AddPropCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "the feature-value line must not be directly editable in the rule formula view"); + } + } + + /// The plus-variable line (kfragPlusVariable) is a computed string bound to a + /// fake tag, not free text. + [Test] + public void Display_PlusVariableLineFragment_IsMarkedNotEditable() + { + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, 0, RuleFormulaVcBase.kfragPlusVariable); + + Assert.That(env.AddPropCalls, Is.Not.Empty, + "expected AddProp to be called for the plus-variable fragment"); + foreach (var call in env.AddPropCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "the plus-variable line must not be directly editable in the rule formula view"); + } + } + + /// The minus-variable line (kfragMinusVariable) is a computed string bound to a + /// fake tag, not free text. + [Test] + public void Display_MinusVariableLineFragment_IsMarkedNotEditable() + { + var vc = new RegRuleFormulaVc(Cache, m_propertyTable); + var env = new EditabilityRecordingEnv(); + + vc.Display(env, 0, RuleFormulaVcBase.kfragMinusVariable); + + Assert.That(env.AddPropCalls, Is.Not.Empty, + "expected AddProp to be called for the minus-variable fragment"); + foreach (var call in env.AddPropCalls) + { + Assert.That(call.EditableAtCallTime, Is.EqualTo((int)TptEditable.ktptNotEditable), + "the minus-variable line must not be directly editable in the rule formula view"); + } + } + /// /// Records enough of IVwEnv's calls to observe, at the moment AddStringAltMember binds a /// fragment to a real domain-object field, whether the view constructor had most recently @@ -172,7 +231,15 @@ public struct Call public int EditableAtCallTime; } + public struct PropCall + { + public int Tag; + public int Frag; + public int EditableAtCallTime; + } + public List StringAltMemberCalls = new List(); + public List AddPropCalls = new List(); private int m_currentEditable = int.MinValue; // sentinel: never set @@ -181,6 +248,11 @@ public void AddStringAltMember(int tag, int ws, IVwViewConstructor _vwvc) StringAltMemberCalls.Add(new Call { Tag = tag, Ws = ws, EditableAtCallTime = m_currentEditable }); } + public void AddProp(int tag, IVwViewConstructor _vwvc, int frag) + { + AddPropCalls.Add(new PropCall { Tag = tag, Frag = frag, EditableAtCallTime = m_currentEditable }); + } + public void set_IntProperty(int tpt, int tpv, int nValue) { if (tpt == (int)FwTextPropType.ktptEditable) @@ -220,7 +292,6 @@ public ISilDataAccess DataAccess public void AddObj(int hvo, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } public void AddLazyVecItems(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } public void AddLazyItems(int[] _rghvo, int chvo, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } - public void AddProp(int tag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } public void AddDerivedProp(int[] _rgtag, int ctag, IVwViewConstructor _vwvc, int frag) { throw new NotImplementedException(); } public void NoteDependency(int[] _rghvo, int[] _rgtag, int chvo) { } public void NoteStringValDependency(int hvo, int tag, int ws, ITsString _tssVal) { throw new NotImplementedException(); } From 0eb983c50998e97e017f8c778adb8e176c774fdf Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 08:08:51 -0400 Subject: [PATCH 5/6] LT-22710: move investigation and design notes to the PR description The bug analysis and 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. --- Docs/bugs/phon-rule-direct-editing-review.md | 115 ------------------- Docs/bugs/phon-rule-direct-editing.md | 69 ----------- 2 files changed, 184 deletions(-) delete mode 100644 Docs/bugs/phon-rule-direct-editing-review.md delete mode 100644 Docs/bugs/phon-rule-direct-editing.md diff --git a/Docs/bugs/phon-rule-direct-editing-review.md b/Docs/bugs/phon-rule-direct-editing-review.md deleted file mode 100644 index 5f03d41102..0000000000 --- a/Docs/bugs/phon-rule-direct-editing-review.md +++ /dev/null @@ -1,115 +0,0 @@ -# Review: rule formula cells are directly editable (architecture self-review) - -## 1. Is this the right architecture for the invariant? - -The invariant is "a rule cell is not free text; it only changes via chooser-insert and -delete." That invariant actually has two independent parts, and each belongs at a -different layer: - -- **"This span of text is bound to a real domain object's field and must not accept an - edit."** This is a per-fragment fact that only the view constructor knows, because only - it knows which `AddStringAltMember`/`AddProp` calls bind to a real, shared field - (`PhNaturalClass.Abbreviation`, `PhTerminalUnit.Name`) versus a fake tag or a filler - line. `RuleFormulaVcBase.Display` is exactly the place this was missing, and it is where - the fix went (five call sites: the natural-class abbreviation, the terminal-unit name, - and the three computed feature/variable lines). -- **"This whole widget only accepts chooser-insert and delete, never typed input."** This - is a fact about the control, not about any one fragment, and it belongs on the rootsite. - `RuleFormulaControl` now sets `m_view.ReadOnlyView = true` instead of `false`. - -Both were previously expressed only as a third thing: an input-path filter -(`PatternView.OnKeyPress` swallowing everything but Backspace/Delete). An input-path -filter is the wrong layer for either fact above -- it has to be re-derived and -re-applied for every new input path (keyboard, IME, drag-and-drop, programmatic paste), -and it says nothing about which content is actually safe to bind live. Fixing at the two -layers above is categorical: a new fragment added to any of the three rule-kind view -constructors is safe by default only if its author remembers to mark it, which is still -not perfect, but a new *input path* into an already-correctly-marked view is safe with no -further action, which is the property the old design did not have. - -**An experiment confirms which half is load-bearing.** Temporarily removing the -`ktptEditable` line for `kfragTerminalUnit` while leaving `ReadOnlyView = true` in place -reproduces the corruption again (`ReplaceWithTsString` still renames the phoneme). -Removing it back and only relying on the view-constructor fix, with `ReadOnlyView` never -touched, was already proven sufficient in the first fix iteration. So: the -`ktptEditable` marking is the control that actually stops a direct `ReplaceWithTsString` -call; `ReadOnlyView` does not gate that low-level API at all. `ReadOnlyView = true` earns -its place for a different reason -- it unregisters the keyboard/IME controller hook -(`SimpleRootSite.UnsubscribeFromRootSiteEventHandlerEvents`), which is the categorical fix -for the IME-composition bypass the original report flagged as the most likely real-world -trigger, and it disables `EditingHelper.CanCut`/`CanPaste` so cut/paste menu commands -stop offering to mutate the view. Both layers are necessary; neither is sufficient alone. - -## 2. What can be removed or simplified? - -Nothing was safely removable. The natural candidate was -`PatternView.PatternEditingHelper.CanCut()`/`CanPaste()`, which look redundant now that -`EditingHelper.CanCut`/`CanPaste` already return `false` whenever `Editable` is `false` -(which it now always is for `RuleFormulaControl`'s view). They were **not** removed, -because `PatternView` is also instantiated directly by `ComplexConcControl` (the complex -concordance pattern builder), which leaves `ReadOnlyView = false` and depends on this same -override to keep cut/paste disabled while still being interactively editable in other -respects. Removing the override would silently enable clipboard paste into that unrelated -feature. `CanCopy()` also stays for a different reason: the base implementation does not -consult `Editable` at all, so it is not made redundant by `ReadOnlyView` -- it is a -deliberate, independent restriction that this fix does not touch. - -`PatternView.OnKeyPress` also stays (see LT-21888 in the class's existing comment). It is -not dead: the audit below found several literal separator glyphs and fake-tag-bound -"index"/boundary strings across `MetaRuleFormulaVc`/`AffixRuleFormulaVc` that were never -audited for `ktptEditable` and are outside this bug's scope (see next section). Those -spans do not corrupt real data if edited -- they are not bound to a real field -- but -`OnKeyPress` is the only thing currently stopping a keystroke from reaching them. Removing -it would trade one narrow, already-fixed hole for a wider, unaudited one. - -## 3. What was not fixed, and why - -The audit surface here is closed, not open-ended: `PatternVcBase` has exactly two -subclasses (`RuleFormulaVcBase`, fixed here, and `ComplexConcPatternVc`) and `PatternView` -has exactly two consumers (`RuleFormulaControl` and `ComplexConcControl`). Both are -accounted for below. - -- **`ComplexConcPatternVc`/`ComplexConcControl`** (the complex-concordance pattern - builder) share `PatternVcBase`/`PatternView` and the same "chooser insert/delete only" - premise, but were deliberately left alone -- this is a different feature with no test - coverage in this session's reach. This is confirmed to be a *different* defect than the - one fixed here: `ComplexConcPatternVc` never overrides `UpdateProp`, so a direct edit that - bypasses `OnKeyPress` (the same `ReplaceWithTsString` path used above) throws an unhandled - `NotImplementedException` out of the Views engine rather than renaming anything. It cannot - reproduce this bug's corruption, because it binds no real domain fields the way - `kfragNC`/`kfragTerminalUnit` do -- so the "keep `CanCut`/`CanPaste` for - `ComplexConcControl`" reasoning in section 2 still stands. The crash risk is its own, - separate issue. -- **Literal/fake-tag spans elsewhere in the same VC family** -- bracket glyphs - (`kfragLeftBracket`/`kfragRightBracket`, via `m_bracketProps`, which never sets - `ktptEditable`), zero-width boundary markers (`ktagLeftBoundary`/`ktagRightBoundary`), - and several `MetaRuleFormulaVc`/`AffixRuleFormulaVc` fields (`m_inputCtxtProps`-rendered - content, `ktagIndex`, `ktagLeftEmpty`/`ktagRightEmpty`) were not individually marked. - None of them bind to a real, shared domain field the way `kfragNC`/`kfragTerminalUnit` - do, so an edit landing there cannot rename a phoneme or natural class -- the actual bug - in scope -- but an edit attempt against a fake tag is unverified territory (it may throw, - the same way `ComplexConcPatternVc` does, or silently no-op). This is exactly the gap - `OnKeyPress` is still covering. -- **`ConstChartVc`** (`ConstChartVc.cs:297`) has the same defect shape (a fragment bound to - a real field with no visible `ktptEditable` marking), but is reported to be guarded at the - cell level by `MakeCellsMethod.cs:495`. That guard was checked by reading the code only, - not by mutation or a live reproduction attempt, so it is unverified/SUSPECTED-safe, not - confirmed clean. -- **The IME repro itself.** The bug report's own "Verification required" section already - flagged this: the IME mechanism was inferred, not reproduced, because it requires a live - IME/vernacular keyboard. That remains true after this fix; see section 4. - -## 4. What still needs manual verification in a running FLEx - -- Open a phonological rule (or metathesis rule, or affix process) with an IME or - vernacular keyboard active, place the insertion point in a rule cell, and confirm that - IME composition can no longer commit text into the cell (it should behave as if the - control has no keyboard focus for typing at all, since the keyboard controller no longer - has this control registered). -- Confirm the selection highlight is still visible when clicking into a rule cell (this - is what `PatternView.AllowDisplaySelection` restores), and that the chooser - insert/delete buttons still operate against the right selection. -- Confirm Delete and Backspace still remove the selected item in a live UI for all three - rule kinds, matching the headless test added here. -- Try drag-and-drop of text onto a rule cell; confirm it either does nothing or is - rejected, rather than landing. diff --git a/Docs/bugs/phon-rule-direct-editing.md b/Docs/bugs/phon-rule-direct-editing.md deleted file mode 100644 index 96535539eb..0000000000 --- a/Docs/bugs/phon-rule-direct-editing.md +++ /dev/null @@ -1,69 +0,0 @@ -# Bug 1 — Rule formula cells are directly editable, and edits rename the underlying phoneme / natural class - -**Area:** Grammar → Phonological Rules / Affix Processes (rule formula slices) -**Type:** Data corruption -**Related prior work:** LT-21888 (the keystroke filter this report argues is insufficient) - -## Symptom - -The rule formula view is specified to be modifiable only by (a) inserting an item through a chooser and (b) deleting an item. In practice users are sometimes able to modify the content of a cell directly. Because the view renders the *referenced object's own* text field, an edit that lands does not merely corrupt the rule — it renames the phoneme or natural class project-wide, affecting every other rule that references it. - -## Root cause - -Editability is never denied at the view level. It is only filtered at the control level, and the filter covers exactly one input path. - -### The fragments are editable - -`RuleFormulaVcBase.Display` renders the substantive parts of a rule with plain string-alternative calls against the referenced object's real multistring property: - -- `RuleFormulaVcBase.cs:287-303` — `kfragNC` calls `AddStringAltMember` on the natural class's `Abbreviation` / `Name`. -- `RuleFormulaVcBase.cs:305-312` — `kfragTerminalUnit` calls `AddStringAltMember(PhTerminalUnitTags.kflidName, ...)`, i.e. the phoneme's or boundary marker's live `Name`. - -None of `kfragNC`, `kfragTerminalUnit`, `kfragFeatureLine`, or `kfragFeats` sets `ktptEditable = TptEditable.ktptNotEditable`. The **only** place that property is set anywhere in the `RuleFormulaVcBase` / `PatternVcBase` chain is on the blank filler lines: `PatternVcBase.cs:209` (`AddExtraLines`). - -### The rootsite is not read-only - -`RuleFormulaControl.cs:1153` sets `m_view.ReadOnlyView = false`. - -### The only guard is a WM_CHAR filter - -- `PatternView.cs:135-149` — `OnKeyPress` swallows every character except Backspace and Delete. The comment cites LT-21888, i.e. this was itself added as a bug fix. -- `PatternView.cs:41-54` — a custom `PatternEditingHelper` hard-codes `CanCopy` / `CanCut` / `CanPaste` to `false`. - -Anything that reaches the root box without going through `OnKeyPress` or the clipboard helper is unguarded. Candidates, in rough order of likelihood for FLEx users: - -1. **IME composition.** Vernacular-script keyboards commit text through IME messages rather than plain WM_CHAR. This is the most probable real-world trigger and matches the "sometimes" in the report. -2. Drag-and-drop text onto the view. -3. Any other rootsite entry point that mutates the selection's string property directly. - -**Status: CONFIRMED by code reading.** The `ReadOnlyView = false` setting, the absence of `ktptNotEditable` on substantive fragments, and the single-path keystroke filter are all directly verified. The specific IME mechanism is **inferred, not reproduced** — see Verification below. - -## Proposed fix - -Enforce the invariant where it belongs, in the view constructor, rather than patching input paths one at a time. - -1. In `RuleFormulaVcBase`, wrap the substantive fragments in `ktptEditable = TptEditable.ktptNotEditable` before the `AddStringAltMember` calls at `RuleFormulaVcBase.cs:287-312`, and likewise for the feature-line and feature fragments. -2. Consider setting `m_view.ReadOnlyView = true` at `RuleFormulaControl.cs:1153`. This needs checking against the delete path — `PatternView.OnKeyDown` (`PatternView.cs:120-149`) intercepts Delete/Backspace and raises `RemoveItemsRequested` rather than editing text, so a read-only rootsite may still be compatible with deletion, but this must be verified, not assumed. -3. Keep the `OnKeyPress` filter as defence in depth. Do not remove it as part of this fix. - -Option 1 alone is likely sufficient and is the lower-risk change. - -## Verification required before closing - -- Reproduce the original defect with an IME / vernacular keyboard against an unpatched build. Without a repro we are fixing an inferred mechanism. -- Confirm delete still works on all three rule kinds after the change: regular phonological rules, metathesis rules, affix processes. -- Confirm the fix covers metathesis rules. `MetaRuleFormulaControl` / `MetaRuleFormulaVc` were not read line by line; they share `RuleFormulaVcBase` and reuse `CmdCtxtSetFeatures`, so they are expected to share the defect, but this is unverified. - -## Scope - -Independent of Bug 2 (natural class vs. phonological features) and Bug 3 (affix process clone on sense split). No shared code paths beyond both Bug 1 and Bug 2 living in the rule formula UI. - -## Key files - -| Path:line | Role | -|---|---| -| `Src/LexText/Morphology/RuleFormulaVcBase.cs:287-312` | Renders NC abbreviation and phoneme name as editable strings | -| `Src/LexText/LexTextControls/PatternVcBase.cs:209` | The only `ktptNotEditable` in the chain (filler lines only) | -| `Src/LexText/LexTextControls/PatternView.cs:41-54` | Clipboard guard | -| `Src/LexText/LexTextControls/PatternView.cs:120-149` | Keystroke filter and delete interception | -| `Src/LexText/Morphology/RuleFormulaControl.cs:1153` | `ReadOnlyView = false` | From 4fe7c4c38f426a71222d5de9fc9aeae8e399737d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 15:06:19 -0400 Subject: [PATCH 6/6] LT-22710: tighten comments to the repo commenting standard State why a fragment is not editable rather than restating what the next line renders, drop a pointer to a test class and one to another view constructor's internals, and cut an over-long implementation comment to the sentence that matters. Re-wrap lines past the width limit. --- .../RuleFormulaControlWiringTests.cs | 5 ++--- .../RuleFormulaDirectEditReproTests.cs | 17 ++++++++--------- .../RuleFormulaVcBaseEditabilityTests.cs | 3 ++- Src/LexText/Morphology/RuleFormulaControl.cs | 3 ++- Src/LexText/Morphology/RuleFormulaVcBase.cs | 6 ++++-- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs index 0ea12f0bf4..abfecba4a7 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaControlWiringTests.cs @@ -7,9 +7,8 @@ namespace SIL.FieldWorks.XWorks.MorphologyEditor { /// - /// Proves the shipped wiring, not a test-only substitute: constructing each real rule - /// formula control must produce a read-only rootsite, since a rule cell is modifiable only - /// by chooser-insert and delete. + /// Constructing each real rule formula control must produce a read-only rootsite, since a + /// rule cell is modifiable only by chooser-insert and delete. /// [TestFixture] public class RuleFormulaControlWiringTests diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs index 2411e661b0..a3ffc56c84 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaDirectEditReproTests.cs @@ -74,8 +74,8 @@ private class NullPatternControl : IPatternControl public int GetFlid(object ctxt) => 0; } - /// Exposes the protected layout hook so the view can be laid out headlessly, - /// exactly like RootSiteTests' DummyBasicView.CallLayout(). + /// Exposes the protected layout hook so the view can be laid out + /// headlessly. private class TestPatternView : PatternView { public void CallLayout() @@ -137,7 +137,7 @@ private IPhPhoneme CreatePhoneme(string name) /// /// Builds a real regular-rule RHS whose left context is a natural class special-cased to - /// display only its abbreviation ("C" or "V", per RuleFormulaVcBase's kfragNC branch), + /// display only its abbreviation ("C" or "V"), /// hosts it in a live PatternView/RegRuleFormulaVc pair, and returns the natural class /// plus the live view. /// @@ -231,11 +231,9 @@ public void ReplaceWithTsString_OnPhonemeTerminalUnit_BypassesOnKeyPress_AndShou ITsString corrupted = TsStringUtils.MakeString("CORRUPTED", Cache.DefaultVernWs); - // This call never goes through PatternView.OnKeyPress -- it is the rootsite's own - // low-level text-replacement API, exactly what bypasses the WM_CHAR filter. It is - // wrapped in a UOW only because the change-tracking infrastructure requires one for - // any edit to commit at all; IME composition and drag-and-drop land inside a UOW - // supplied by the real editing helper, not by PatternView.OnKeyPress. + // The rootsite's own low-level text-replacement API, which bypasses the WM_CHAR + // filter. The unit of work is required for any edit to commit, not part of the + // bypass. UndoableUnitOfWorkHelper.Do("undo", "redo", phoneme, () => sel.ReplaceWithTsString(corrupted)); string nameAfter = phoneme.Name.VernacularDefaultWritingSystem.Text; @@ -264,7 +262,8 @@ public void DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly() } /// - /// A read-only rootsite suppresses Activate() by default (SimpleRootSite.AllowDisplaySelection), + /// A read-only rootsite suppresses Activate() by default + /// (SimpleRootSite.AllowDisplaySelection), /// which would hide the selection a chooser insert/delete needs the user to see. /// [Test] diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs index 2c42fda99e..79a381efa1 100644 --- a/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/RuleFormulaVcBaseEditabilityTests.cs @@ -120,7 +120,8 @@ public void Display_TerminalUnitNameFragment_IsMarkedNotEditable() } } - /// Same defect, exercised through the metathesis-rule view constructor. + /// Same defect, exercised through the metathesis-rule view + /// constructor. [Test] public void Display_TerminalUnitNameFragment_ViaMetaRuleFormulaVc_IsMarkedNotEditable() { diff --git a/Src/LexText/Morphology/RuleFormulaControl.cs b/Src/LexText/Morphology/RuleFormulaControl.cs index c433231c86..b2dfafdc97 100644 --- a/Src/LexText/Morphology/RuleFormulaControl.cs +++ b/Src/LexText/Morphology/RuleFormulaControl.cs @@ -1150,7 +1150,8 @@ private void InitializeComponent() this.m_view.Location = new System.Drawing.Point(0, 0); this.m_view.Mediator = null; this.m_view.Name = "m_view"; - // A rule formula cell is modifiable only via chooser-insert and delete, never free text. + // A rule formula cell is modifiable only via chooser-insert and delete, never free + // text. 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/Morphology/RuleFormulaVcBase.cs b/Src/LexText/Morphology/RuleFormulaVcBase.cs index 22d8083d2c..b3f526bb3d 100644 --- a/Src/LexText/Morphology/RuleFormulaVcBase.cs +++ b/Src/LexText/Morphology/RuleFormulaVcBase.cs @@ -285,7 +285,8 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) break; case kfragNC: - // This renders the referenced natural class's own Abbreviation/Name; a rule cell is never free text. + // The text belongs to the referenced natural class, not to the rule, so an + // edit here would rename it for every rule that uses it. vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); int ncWs = WritingSystemServices.ActualWs(m_cache, WritingSystemServices.kwsFirstAnal, hvo, PhNaturalClassTags.kflidAbbreviation); @@ -305,7 +306,8 @@ public override void Display(IVwEnv vwenv, int hvo, int frag) break; case kfragTerminalUnit: - // This renders the referenced phoneme's or boundary marker's own live Name. + // The text belongs to the referenced phoneme or boundary marker, so an edit + // here would rename it for every rule that uses it. vwenv.set_IntProperty((int)FwTextPropType.ktptEditable, (int)FwTextPropVar.ktpvEnum, (int)TptEditable.ktptNotEditable); int tuWs = WritingSystemServices.ActualWs(m_cache, WritingSystemServices.kwsFirstVern, hvo, PhTerminalUnitTags.kflidName);