LT-22717: Stop the Complex Concordance pattern builder crashing on a bypassing edit - #1088
Draft
johnml1135 wants to merge 8 commits into
Draft
LT-22717: Stop the Complex Concordance pattern builder crashing on a bypassing edit#1088johnml1135 wants to merge 8 commits into
johnml1135 wants to merge 8 commits into
Conversation
ComplexConcPatternVc has no UpdateProp override, so any edit that reaches the view engine without going through PatternView.OnKeyPress (IME composition, drag-and-drop, or a direct IVwSelection.ReplaceWithTsString call) falls through to VwBaseVc.UpdateProp, which throws NotImplementedException. These tests drive a real PatternView/ ComplexConcPatternVc pair and confirm the crash reproduces across every fragment tried: ktagType, ktagForm, ktagGloss, ktagCategory, ktagEntry, ktagTag, ktagInfl, kfragOR, kfragHash, and both min/max quantifier lines (11 of 11 fail today). Two contrast tests pass already: a plain keystroke never reaches the engine (PatternView.OnKeyPress swallows it), and Delete still raises RemoveItemsRequested. Also seed the probe/bug docs from the prior investigation.
Root cause: ComplexConcPatternVc renders every fragment (feature lines, OR/word-boundary literals, brackets, min/max quantifiers) via AddProp + DisplayVariant -- a computed display over the synthetic pattern node, never a real bound field (zero AddStringAltMember calls, confirmed by inspection). With no UpdateProp override, any edit that reaches the engine without going through PatternView.OnKeyPress fell through to VwBaseVc.UpdateProp, which throws NotImplementedException. Fix, in the layers actually shown to be needed by ablation: - UpdateProp override (necessary and sufficient to stop the crash -- verified by re-running the full failing suite with only this change added: 13/13 pass). It absorbs the edit the same way RuleFormulaVcBase.UpdateProp does; there is nothing to write back to, so the next redraw simply shows the live node state again. - ktptNotEditable on every fragment ComplexConcPatternVc renders (feature lines, Infl Features lines, OR/#, brackets/parens and their pile glyphs, min/max), so an edit is rejected at the selection layer instead of silently reaching UpdateProp. Verified separately: a selection over a marked fragment now reports IsEditable == false. - ReadOnlyView = true on ComplexConcControl's PatternView, the categorical fix for the IME-composition/drag-and-drop path named in the bug report (it unregisters the keyboard/IME controller hook). This needs PatternView.AllowDisplaySelection overridden to stay true, or the chooser-driven selection highlight disappears once ReadOnlyView is set -- SimpleRootSite suppresses Activate() by default for read-only views. PatternView is shared with RuleFormulaControl (Morphology), which still runs with ReadOnlyView = false on this branch, so AllowDisplaySelection is a no-op there today. Delete still raises RemoveItemsRequested with ReadOnlyView = true (verified). All 16 tests in ComplexConcPatternVcDirectEditTests.cs pass, including the 11 that reproduced the crash pre-fix and the 3 new ablation checks.
Documents: this is a different failure mode than the sibling rule-formula bug (UpdateProp alone is necessary and sufficient here, confirmed by ablation, because no fragment binds a real field); what was deliberately not hoisted to PatternVcBase and why; why PatternEditingHelper.CanCut/ CanPaste were left alone (branch-topology reason, noted for whoever integrates both branches); and what still needs manual verification in a running FLEx (IME, drag-and-drop, selection visibility, Delete).
Docs/bugs/*.md are working documents that get evicted from the branch when the PR is written; a source comment pointing at one would dangle. Rewrite the ComplexConcControl.Designer.cs comment and the test file header to state the reasoning inline instead of citing the doc.
…e bug it exposed
Adversarial review found MakeSelOnChild inert: it passed the requested tag
into MakeTextSelInObj's unused cvsliEnd slot with fWholeObj: true, so 9 of
11 "fragment angle" tests were all the same whole-object range selection
in disguise (anchored at the outermost boundary glyph, not the named
fragment). Replaced it with MakeSelOnFragment, built on
IVwRootBox.MakeTextSelection with the tag passed as tagTextProp -- the
same API PatternView.SelectLeftBoundary/SelectRightBoundary already use
to target one fake-tag property on an object. Every fragment test now
carries an AssertSelectionTargets(sel, expectedTag) check via
IVwSelection.TextSelInfo, so a future regression in targeting fails
loudly instead of silently passing. Added
MakeSelOnFragment_DiscriminatesBetweenFragments_OnTheSameNode as a
standing proof that three different tags on one node resolve to three
different selections.
Fixing the helper exposed a real production bug: ComplexConcPatternVc's
SetNotEditable(vwenv) calls do not persist across multiple AddProp calls
the way vwenv.Props = someBuilder does -- ktptEditable must be
re-asserted immediately before *every* AddProp, matching the convention
PatternVcBase.AddExtraLines and RuleFormulaVcBase already use. Most of
the fix's ktptEditable markings were only accidentally correct for
single-AddProp call sites (OR, #, min/max); the feature lines
(Form/Entry/Category/Gloss/Infl) and the multi-line bracket/paren
pile-hook sequences were not actually protected. Fixed by re-asserting
SetNotEditable before each individual AddProp in DisplayFeatures,
DisplayInflFeatureLines, DisplayInflFeatures, and the four pile-hook
sequences.
Also fixed a second, genuinely unmarked gap found by mutation testing:
PatternVcBase.OpenSingleLinePile/CloseSingleLinePile's zero-width-space
boundary run (shared by both PatternVcBase subclasses) had no
ktptEditable marking at all. With UpdateProp removed, an edit on this
run still threw NotImplementedException. Marked it NotEditable and
pinned it with SelectionOverZeroWidthBoundaryRun_IsNotEditable, verified
red (fails without the marking, confirmed by temporarily reverting it)
and green (passes with it).
Corrected ablation with the fixed helper (18-test suite): UpdateProp
alone (ktptEditable neutralized) = 16/18 (only the two IsEditable-assertion
tests fail; every crash/content test still passes). ktptEditable alone
(UpdateProp removed) = 18/18. Both are independently sufficient to stop
the crash, unlike the sibling bug where ktptEditable was the only
load-bearing layer. But a direct probe (UpdateProp intact, ktptEditable
neutralized) showed UpdateProp's no-op leaves the *displayed* text
stale/corrupted ("HACKEDForm: original" instead of "Form: original")
until an explicit Reconstruct -- ktptEditable prevents this because the
edit never reaches that point. Both layers are needed, not for
redundant crash-prevention, but because they prevent two different
failure modes.
Also added ComplexConcControl_AcceptsTabUnchanged_AcceptsReturnNowFalse,
pinning the actual behaviour change from ReadOnlyView = true:
AcceptsTab was already false before this fix (unchanged); AcceptsReturn
flips from true to false (new).
…eturn Rewrites the ablation conclusion (ktptEditable is independently sufficient too, and prevents a display artifact UpdateProp's no-op does not), states plainly that ReadOnlyView does not gate ReplaceWithTsString (mutation-tested) and its only proven value is closing the IME-controller registration channel (unverified live), documents the AcceptsReturn/ AcceptsTab behaviour change (AcceptsTab unchanged, AcceptsReturn now false), and updates the ConstChartVc status per the coordinator's adversarial-review note (escalated to a concrete corruption suspicion, explicitly out of scope for this task).
The bug analysis, the seed probe and the architecture self-review were working documents for this fix. Their conclusions are now carried by the code and its tests; the reasoning, decisions and paths not taken live in the pull request body so they inform review without merging into the tree.
Move the file-header explanation into the fixture's own doc comment, replace decorative section banners with one-line statements, drop a pointer to a working document, and state the AcceptsReturn contract as current behaviour rather than as a before-and-after.
|
commit 9fa6dac187: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1088 +/- ##
==========================================
+ Coverage 38.07% 38.43% +0.36%
==========================================
Files 1499 1499
Lines 350141 350314 +173
Branches 40238 40256 +18
==========================================
+ Hits 133304 134656 +1352
+ Misses 187558 186489 -1069
+ Partials 29279 29169 -110
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Complex Concordance pattern builder no longer takes FLEx down. Any edit that reached the view without going through the keystroke filter — IME composition on a vernacular keyboard, or dragging text into the pane — fell through to
VwBaseVc.UpdatePropand threw an unhandledNotImplementedException, losing whatever was in the window.The question on opening this is why four layers when one of them fixes the crash. Because they do different jobs, established by ablation rather than assumed.
UpdatePropstops the throw.ktptEditablestops a different failure: withUpdatePropalone absorbing the edit, the model readsoriginalwhile the view displaysHACKEDForm: originaluntil something forces a redraw.ReadOnlyViewstops neither — its only value is closing the IME and keyboard-controller registration channel, which is the originally reported trigger and remains unverified against a real IME. The review worth your time is whether the read-only rootsite costs anything in a pane users navigate by keyboard.Where to look
ReadOnlyView = trueforcesAcceptsReturnfalse.AcceptsTabwas already false, Designer-set, so Tab navigation is unchanged; Return was already swallowed byOnKeyPress, so only its disposal point moves. Pinned by test, reasoned rather than verified for the hosting case.SetNotEditabledoes not persist acrossAddPropcalls — it must be re-asserted before each one. Several markings were only accidentally correct before this was found.PatternVcBasehad no editability marking at all. That fix lands in the shared base and benefits the rule formula editor too.OnKeyDownraisesRemoveItemsRequested, pinned by test.PatternViewis shared. This branch and LT-22710 both add an identicalAllowDisplaySelectionoverride and will conflict on that line; the resolution is mechanical, either side.Deliberately not here
PatternEditingHelper.CanCut/CanPaste, which become dead once LT-22710 also lands. Left to whoever merges second.ConstChartVc, a suspected corruption-class defect in a third view constructor. Separate investigation.Verification.
ComplexConcPatternVcDirectEditTests19/19,ITextDllTests226 passed / 1 pre-existing skip,MorphologyEditorDllTests7/7,LexTextControlsTests353 passed / 3 pre-existing skips. Draft pending the LT-22710 merge order.Reading this a year from now — start here
The investigation notes, the seed probe and the architecture review were deliberately deleted from this branch rather than merged; their content is synthesised into these sections.
The short version:
ComplexConcControland the phonological rule formula editor are the only two consumers ofPatternView, and the only two subclasses ofPatternVcBase. Both are chooser-driven surfaces where the user is never meant to type. Both lacked a structural guarantee of that, and each expressed the failure differently — the rule editor renamed shared project data, this one crashed — because the rule editor binds real domain fields and this one does not.Why it crashes here but corrupts there
ComplexConcPatternVcrenders synthetic content:ComplexConcPatternSdaserves in-memory values against negative sentinel ids, and a grep forAddStringAltMemberin that file returns zero. So no edit can rename a shared phoneme, natural class or part of speech — verified additionally by tests asserting a realIPartOfSpeechandICmPossibilityare untouched after an edit attempt.RuleFormulaVcBasehas always overriddenUpdateProp, so its edits were absorbed rather than throwing — and landed on realNameandAbbreviationfields. Same missing invariant, opposite symptom. Loud here, silent there.Decisions, and why
Four layers, each doing separate work.
UpdatePropprevents the throw.ktptEditableprevents the edit reaching the point where the display and the model diverge.ReadOnlyViewcloses the input-channel registration.AllowDisplaySelectionrestores the selection highlight that a read-only rootsite suppresses, which a chooser-driven pane needs so the user can see what insert and delete will act on.ReadOnlyViewwas kept despite not fixing the crash. It is the only thing that addresses the originally reported trigger, and dropping it to avoid a one-line merge conflict would trade a real protection for a trivial convenience. The PR says plainly that it does not prevent the crash, so nobody later mistakes it for the fix.The boundary-run marking went into
PatternVcBase, not into this subclass, because the gap belongs to the shared base and the sibling editor has it too.Surprising findings
The test helper was inert.
MakeSelOnChildpassed itstagintoMakeTextSelInObjwithfWholeObj: true, where the documented contract says the end arguments are unused. Nine of eleven "fragment angle" tests were constructing one identical selection anchored at the left bracket glyph. SelectingktagFormandktagGlosson the same node returned byte-identical results. Rebuilt onMakeTextSelection, every fragment is now genuinely targeted and a standing test proves three tags on one node give three distinct selections.SetNotEditabledoes not persist acrossAddPropcalls. Found only because fixing the helper made the difference observable. Several markings had been correct by accident, and the feature lines and multi-line pile hooks were not protected at all.The zero-width-space boundary run was never marked, for either subclass. It was assessed as harmless by reasoning; mutation testing showed it is exactly why
UpdatePropis load-bearing rather than a backstop for the OR and word-boundary fragments.A Tab regression was hypothesised and disproved.
AcceptsTabwas already false, set by the Designer independently ofReadOnlyView. The real change isAcceptsReturn.Evidence
Ablation on the 18-test suite, run rather than argued:
UpdatePropremoved,ktptEditablecompleteUpdatePropintact,ktptEditableneutralisedIsEditableassertions failUpdatePropremoved,ktptEditablecomplete, boundary run unmarkedktptEditableneutralised, direct probeoriginal, displayHACKEDForm: originalAll eleven fragments — Type, Form, Gloss, Category, Entry, Infl, Tag, OR, word boundary, min, max — crash against unfixed code, each individually targeted after the helper fix.
Deferred, and what would unblock it
ConstChartVc.ApplyFormattingdoesvwenv.Props = ttpwith a format-only property set, which theIVwEnv.Propscontract describes as applied in one operation — apparently displacing the cell-levelktptEditablethatMakeCellsMethodset, immediately before a real sharedICmPossibilityis bound viaAddStringAltMember. The Constituent Chart body is editable (ForEditing = true). That is the rule editor's corruption shape in a third view constructor. Unconfirmed: building a valid multi-level selection through the chart's custom cell rendering failed with anArgumentException. Needs someone fluent in Views selection plumbing, or a pixel-basedMakeSelAt.PatternEditingHelper.CanCut/CanPaste.ReadOnlyView's setter drivesEditingHelper.Editable, which both base implementations gate on, and no other assignment touches this control's editing helper. Dead once both consumers are read-only; removable in a follow-up.This change is