LT-22710: Make phonological rule formula cells non-editable - #1082
LT-22710: Make phonological rule formula cells non-editable#1082johnml1135 wants to merge 7 commits into
Conversation
Comment hygiene (advisory)No comment-style violations in the lines this branch adds since |
This comment has been minimized.
This comment has been minimized.
NUnit Tests 1 files ± 0 1 suites ±0 11m 11s ⏱️ +32s Results for commit 7bb08f6. ± Comparison against base commit c373152. This pull request removes 20 and adds 104 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1082 +/- ##
==========================================
+ Coverage 38.28% 38.54% +0.25%
==========================================
Files 1499 1507 +8
Lines 350230 350649 +419
Branches 40261 40304 +43
==========================================
+ Hits 134096 135164 +1068
+ Misses 186915 186274 -641
+ Partials 29219 29211 -8
🚀 New features to boost your workflow:
|
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.
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.
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.
- 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.
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.
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.
b2832bf to
4fe7c4c
Compare
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
The diagnosis here is proven rather than argued, and the ablation discipline — removing
each layer and reporting what still corrupts — is better than this repo usually gets. You
also rejected the input-filter approach for the right reason ("that is how the bug got
here"), and enforcement belongs exactly where you put it. The three product comments state
why the text is dangerous rather than what the code does, which is the standard working
as intended.
1. Verify the user-facing paths by hand, and report it here.
LT-22710's "Not covered" section already names this: "Live IME and drag-and-drop
verification in a running FLEx. The mechanism is proven by test; the specific user input
path is not." That is the right call to have made explicitly, and it is also the thing I
want closed before this merges. ReplaceWithTsString corrupting the phoneme is the
mechanism; IME composition and drag-and-drop are the reported bug.
Please run it in a real FLEx and put the result in the PR body:
- The bug paths. Type into a formula cell through an IME in a vernacular script, and
drag text into a cell. The edit must not land, and the referenced phoneme or natural
class must keep its Name/Abbreviation. - The regression paths. Insert through the chooser and delete with the keyboard, and
confirm the correct item is affected in each of the rule kinds the base VC covers.
The second half is not busywork, and it is the part I am least confident about. See item 2.
2. Insert and delete now depend on an accident, and nothing pins it.
PatternView.cs:343, inside GetSelectionInfo:
if (!sel.Selection.IsEditable)
return false;
For an insertion point that early return gates the whole selection-normalisation path
(AdjustSelection -> MakeRangeSelection -> Install) that makes a click select that
item. And RuleFormulaControl.cs:1093 restores the cursor after every insert and every
delete with editable: true, reaching MakeTextSelInObj(..., fEditable: true, ...) at
PatternView.cs:445; SelectCell at :450 does the same.
After this change the phoneme name, the natural class abbreviation and the
feature/variable lines no longer offer an editable position. As far as I can tell the only
editable spans left in a cell are the zero-width-space boundary runs, because
PatternVcBase.cs:78-86 builds m_bracketProps without any ktptEditable value. So the
behaviour probably survives — by landing the cursor on a ZWSP that an unrelated props
builder happens to leave editable. That is a coincidence, not an invariant, and if anyone
ever marks those bracket spans (which your own "Paths not taken" contemplates), insert and
delete break with no exception and no failing test.
DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnly does not cover this. It
proves PatternView.OnKeyDown still raises an event, but it installs no selection, and
NullPatternControl returns null from GetContext/GetItem and -1 from
GetItemContextIndex — so RuleFormulaControl.RemoveItems (:480), the consumer that
actually resolves a selection to a cell and an index, never runs. Chooser-insert has no
test at all.
I am asking for manual verification rather than an end-to-end automated test because a
real IPatternControl fixture is a lot of scaffolding for this branch to carry. But the
manual result needs to be in the body, and the risk should be named there too, so the next
person to touch m_bracketProps knows what depends on it.
3. Add an acceptance test to LT-22710.
The ticket has steps to reproduce but no acceptance criteria. Right now the only record of
what "fixed" means lives in a PR description, which is not where someone verifying this in
a release build will look. Please add one covering both directions from item 1 — the bug
paths blocked, the chooser and delete paths still working — so the fix is verifiable
without reconstructing it from here.
4. Set ReadOnlyView in the constructor, not in the designer region.
RuleFormulaControl.cs:1153 sits inside #region Component Designer generated code
(opened at :1124). Any future round-trip through the WinForms designer drops your comment
and can revert the literal. Every other deliberately read-only rootsite in the tree avoids
this: InterlinRibbon.cs:56 and TitleContentsPane.cs:99 set it in the constructor, and
InterlinPrintView / InterlinTaggingChild use a separate .Designer.cs.
RuleFormulaControl has two constructors (:112, :117), both calling
InitializeComponent() — one line after each, or a single override, is designer-proof.
5. Note the MaxParasToScan ordering dependency.
SimpleRootSite.cs:923-926: the ReadOnlyView setter forces m_rootb.MaxParasToScan = 0
whenever the rootbox already exists. PatternView.MakeRoot (:102-105) deliberately sets
it to 10, with a comment that the default "isn't high enough when using the arrow keys to
move the cursor between items in a rule".
This PR is safe only because InitializeComponent runs before MakeRoot, so the setter
takes its early branch. That means nobody may ever set ReadOnlyView on a live
PatternView again without silently reverting the arrow-key fix — and nothing says so.
A comment at the assignment, or a re-assert of MaxParasToScan after the flag is set,
would keep that from being rediscovered the hard way.
6. RuleFormulaDirectEditReproTests needs [Apartment(ApartmentState.STA)].
It calls MakeRoot, MakeTextSelInObj and ReplaceWithTsString on a real IVwRootBox,
and NUnit 3 defaults to MTA. Every comparable fixture in the repo marks itself STA
(StVcTests.cs:28, RenderVerifyTests.cs:26, RenderTimingSuiteTests.cs:19), and
RequiresSTAOnWindowsAttribute exists for exactly this reason in a project
MorphologyEditorDllTests.csproj:39 already references. Passing in CI is not evidence of
apartment safety.
7. Two comments this branch falsifies or outgrows.
RuleFormulaVcBase.cs:338— "we use display variant to display literal strings that are
editable". This PR markskfragFeature,kfragPlusVariableandkfragMinusVariable
— theDisplayVariant-backed fragments — not editable, so the comment is now wrong. It
is in a file you are already editing, on a branch whose last commit is "tighten comments
to the repo commenting standard".EditabilityRecordingEnv.Props— the inline note says "not relevant to editability of
these two fragments", but the fixture now covers five viaAddProp.
Two questions, not change requests.
- Should
CanCopystill returnfalse? (PatternView.cs:41) You reasoned carefully
aboutCanCutandCanPasteand correctly deferred their removal until the Complex
Concordance branch lands. But a view whose whole justification is now "this content is
read-only" that still refuses Ctrl+C is a usability and accessibility gap. The body says
"CanCopyis unaffected either way" without asking whether it should be. Not a
regression from this PR — just the natural moment to notice it. - Would one marking beat five?
ktptEditableinherits down the box tree, so a single
set_IntPropertybefore theOpenTablein thekfragRHScase, or inside
PatternVcBase.OpenSingleLinePile, would cover every fragment including the
bracket/ZWSP spans your "Paths not taken" concedes are still unguarded — and would make
item 2 a designed invariant instead of an accident.MakeCellsMethod.cs:495is the
repo's cell-level precedent, and you already citeConstChartVcapprovingly for that
shape. The cost is needing an explicitktptIsEditableopt-back-in anywhere a pattern
view ever wants real typing. I am not asking you to change it in this branch — you have
already flagged the base-class question as open — but if the answer is "yes, later",
that is worth stating on the ticket so the follow-up is not five more markings.
Each rule formula now marks its outermost table not editable and lets ktptEditable inherit down the box tree, replacing the five per-fragment markings. The three cases marked are the three root fragments the product actually renders: RegRuleFormulaVc.kfragRHS, MetaRuleFormulaVc.kfragRule and AffixRuleFormulaVc.kfragRule. One marking covers what five could not reach: the bracket glyphs, the fake tag spans and the zero-width-space boundaries. It also makes the cursor's landing place deliberate. Insert, delete and click all restore the cursor with fEditable true, so a cell has to keep one editable position; the boundary spans are now put back to editable on purpose rather than surviving because the shared bracket props happened to omit the property. Leaving them editable reaches no data, since ktagLeftBoundary and ktagRightBoundary are fake tags and UpdateProp returns the value unchanged. Stop overriding CanCopy. Cut and paste stay refused because they would change the rule, but a read-only view still has to let the formula be copied out. Set ReadOnlyView from the constructors rather than the designer region, where a designer round-trip could rewrite it, and record why it must run before MakeRoot: the setter forces MaxParasToScan to 0 once the rootbox exists, which would silently undo the arrow-key fix. Add STA to the fixture that drives a real IVwRootBox, since the Views COM objects are apartment-threaded and NUnit 3 defaults to MTA. Retarget the editability tests. The per-fragment assertions described a mechanism that no longer exists, so they now assert only that those fragments bind straight to a referenced object's live field. Three new cases pin the table marking, one pins the boundary opt-in, and one drives a live rootbox to require that an editable position still exists. Removing the opt-in fails the last two, which is how the cursor invariant is now held.
|
Thank you — all seven items and both questions checked out against the code, and 2 (and the second question). Insert and delete no longer depend on an accident. You were right that the surviving editable span was a coincidence, and the code So I took your second question rather than the follow-up route. The five Two things made me confident this is safe rather than merely tidier:
And the invariant is now a red test rather than an argument. Removing the opt-in
4. 5. The 6. 7. Both comments. The First question — 3. Acceptance criteria are on the ticket. LT-22710 now has them, covering 1. Manual verification — still outstanding, and why the draft stays a draft. I have not done it, and I would rather say so than submit weaker evidence dressed Two consequences worth naming plainly:
Happy to run the parts that do not need an IME if you would rather have partial |
|
Filed the base-class question rather than leaving it in the body's "Deferred" It records that this branch's shape — one The manual IME and drag-and-drop verification stays on |
3 participants
Typing into a phonological rule formula no longer changes anything. Until now a rule cell rendered the referenced phoneme's or natural class's own
Name/Abbreviationas editable text, so an edit that reached the view renamed that object across the entire project — every other rule referencing it changed with it.The invariant was previously held by a keystroke filter —
PatternView.OnKeyPress, itself added for LT-21888 — which covers exactly one input path. This branch moves enforcement to two places nothing can route around: the rootsite is read-only, and each formula marks its outermost tablektptNotEditableso the property inherits down the whole box tree.The marking is one call, not five. An earlier revision marked five individual fragments. Review pointed out that insert and delete would then depend on an accident — the cursor has to land on something editable, and the only span left was the zero-width-space boundary, editable purely because the shared bracket props happened to omit the property. Marking the table instead covers every fragment plus the bracket glyphs and fake-tag spans, and the boundary spans are now returned to editable deliberately, with a test that fails if that opt-in is removed. The review worth your time is whether a read-only rootsite breaks an interaction users still need.
Where to look
DeleteKey_StillRaisesRemoveItemsRequested_WhenRootsiteIsReadOnlypins it.ReadOnlyView = trueis a broad switch (selection, focus, IME registration, cut/paste).AllowDisplaySelectionrestores visible selection; three per-control tests pin the shipped wiring.PatternViewandPatternVcBaseare shared with the Complex Concordance pattern builder, and this branch does reach it in two small ways, both intended:MarkBoundaryEditablenow runs in its piles (inert there — it never marks its table non-editable, so those spans were already editable), and dropping theCanCopyoverride enables Ctrl+C in that view too.ITextDllTestspattern/Complex Concordance fixtures stay green.RuleFormula_StillOffersAnEditablePosition_WhenMarkedNotEditabledrives a live rootbox and fails if the boundary opt-in is removed.RuleFormulaVcBase; regular, metathesis and affix-process each have their own test.OnKeyPressandPatternEditingHelper.CanCut/CanPasteare deliberately kept — still load-bearing for the other consumer.Deliberately not here
ReplaceWithTsString; the user-facing input path is not.ComplexConcPatternVchas a sibling defect — a crash, not corruption. Separate branch.ConstChartVcappears guarded at cell level. Assessed by reading only.One side effect worth knowing.
ReadOnlyView = truealso forcesAcceptsReturnto false (SimpleRootSite.cs:942).PatternViewships Tab-false / Return-true, so Tab is unaffected and Return changes only in where it is disposed of —OnKeyPressalready swallowed it, and it now propagates to the host instead of being trapped. The rule formula is a DataTree slice, not a dialog with anAcceptButton, so this should be inert. Reasoned, not verified.Verification. Debug build clean, comment hygiene clean.
MorphologyEditorDllTests26/26,LexTextControlsTests353 passed / 4 not executed (all pre-existing ignores, unrelated),ITextDllTestspattern fixtures 7/7. Not stacked.Still outstanding before merge: the live IME and drag-and-drop verification, and the chooser-insert / keyboard-delete regression pass in a real FLEx. Acceptance criteria for both directions are now on LT-22710. This is the reason the PR is still a draft.
Reading this a year from now — start here
The investigation notes and architecture review that produced this fix were deliberately deleted from the branch rather than merged; their content is synthesised into these sections. They were working documents — a one-time analysis whose conclusions are now carried by the code and its tests.
The short version: a rule formula cell is not free text, and never was meant to be. That invariant lived in exactly one place — a
WM_CHARfilter — and anything reaching the root box by another route (IME composition being the realistic case for vernacular keyboards) walked straight past it into a shared domain object's name field.The layer cake
Rule formula editing, model outward:
PhSegmentRule/PhRegularRule→PhSegRuleRHScells →PhPhonContexttreesRuleFormulaVcBase←PatternVcBase←FwBaseVcPatternViewRuleFormulaControl+RegRule/AffixRule/MetaRulesubclassesInsertionControl— a hotlink strip, not a menuRuleFormulaSliceA "cell" is
StrucDesc/StrucChange/LeftContext/RightContexton aPhSegRuleRHS.kfragNCandkfragTerminalUnitrender the referencedPhNaturalClass's andPhTerminalUnit's real multistring properties — which is why an edit renamed the object rather than corrupting the rule.PatternVcBasehas exactly two subclasses andPatternViewexactly two consumers, so the audit surface here is closed, not open-ended.Decisions, and why
Enforce in the view constructor, not with another input filter. The obvious cheap fix is to intercept whatever new path let the edit through. That is how the bug got here:
OnKeyPresswas already such a fix. Marking fragmentsktptNotEditabledenies the edit at the layer that owns the question, so a future input path inherits the guard instead of needing its own.Both layers were kept because both are load-bearing, established by ablation rather than assumed. Removing just the
kfragTerminalUnitmarking while leavingReadOnlyView = truereproduces the corruption. So the fragment markings — notReadOnlyView— are what actually blockReplaceWithTsString.ReadOnlyViewdoes separate work: it unregisters the keyboard/IME controller hook and disables cut/paste. Neither alone is sufficient.AllowDisplaySelectionfollows the existingInterlinPrintView/InterlinTaggingChildpattern. A read-only rootsite suppresses selection display by default, but a pattern editor needs the user to see what an insert or delete will act on.Paths not taken
Removing
OnKeyPressas now-redundant. Kept. The bracket-glyph and fake-tag spans that used to rely on it are now covered, because the table-level marking inherits to them — so this is no longer the only guard. It stays as defence in depth, and because it is what makes Backspace and Delete mean "remove the item" rather than "edit text".Removing
PatternEditingHelper.CanCut/CanPaste. They look dead once the rootsite is read-only, butPatternViewis shared withComplexConcControl, which as of this branch still runs withReadOnlyView = false. Deleting them here would silently enable paste there.Update: the Complex Concordance branch does set
ReadOnlyView = true, so once both land these two overrides become provably dead —SimpleRootSite'sReadOnlyViewsetter isEditingHelper.Editable = !value(SimpleRootSite.cs:907-944), and baseEditingHelper.CanCut/CanPaste(EditingHelper.cs:3672,3789) already return false whenm_fEditableis false, with no dependence on the override.CanCopyis a different case and this branch does remove it: a view whose justification is "this content is read-only" should still let the formula be copied out, and blocking Ctrl+C was an accessibility gap rather than a safety measure. RemovingCanCut/CanPasteis still left to a follow-up after both branches are in.Fixing
ComplexConcPatternVcin the same branch. Different feature area, and a different failure mode: it binds no real domain fields, so it cannot reproduce this rename. It throws instead.Surprising findings
The corruption is real and reproducible, not theoretical. The original report described it as "sometimes the user can modify the underlying forms." A test now drives
IVwSelection.ReplaceWithTsStringon a phoneme terminal unit and the realPhPhoneme.Namecomes back as"CORRUPTED".ReadOnlyViewdoes not gate the low-level edit API. Setting it alone leavesReplaceWithTsStringworking. This was measured, not reasoned about, and it is the reason the fix has two layers.Three of the five fragment markings were initially untested — they could be deleted with the suite green. Found by mutation testing after the first implementation and closed since; the current suite catches each one individually.
Evidence
Each claim in "Where to look" is pinned by a mutation, run rather than argued:
MarkBoundaryEditablefrom both boundary spansRuleFormula_StillOffersAnEditablePosition_WhenMarkedNotEditablered —MakeSimpleSel(fEdit: true)returns null, i.e. the formula has no position for the cursor;OpenAndCloseSingleLinePile_MarkTheBoundarySpansEditablered. The other 17 stay green, including both rename-prevention tests, which is what shows the table marking alone blocks the corruptionReadOnlyViewback tofalseAllowDisplaySelectionThe last mutation is the one that matters most, because it is the failure review predicted: with the whole table marked and no deliberate opt-in, nothing in a cell is editable and insert/delete would break with no exception and no red test. That is now a red test.
The
EditabilityRecordingEnvspy recordsOpenTableand the editable state at each call. The five former per-fragment editability assertions were removed rather than adapted — they described a mechanism that no longer exists. What remains of them asserts the still-true and still-useful part: that those fragments bind straight to a referenced object's live field, which is the write channel this bug was about.Deferred, and what would unblock it
ComplexConcPatternVc— sibling defect in the otherPatternVcBasesubclass, confirmed by probe: noUpdatePropoverride, so any bypassing edit throws an unhandledNotImplementedException. Own branch.ConstChartVc:297binds a sharedCmPossibilityin a chooser-driven cell — same defect shape — but appears pre-guarded at the cell level byMakeCellsMethod.cs:495. Assessed by code reading only; never exercised.PatternVcBasegained the boundary opt-in that makes the cursor's landing place explicit. What is still open is whetherPatternVcBaseshould mark itself non-editable so a third subclass inherits the invariant without opting in — that would change Complex Concordance, so it belongs with that branch.This change is