LT-22714: Stop the IPA populate path duplicating phoneme features - #1087
LT-22714: Stop the IPA populate path duplicating phoneme features#1087johnml1135 wants to merge 2 commits into
Conversation
The Basic IPA Symbol slice created a new closed value for every FeatureValuePair the IPA inventory lists and appended it, so a phoneme could end up holding several specifications for one phonological feature. It now upserts through IFsFeatStruc.GetOrCreateValue, updating the specification a feature already has rather than adding another. The lookup requires a closed feature, which also covers the null case, and skips a feature that has no symbolic value to assign. The shipped inventory also named fPAAnterior and fPACoronal twice for IPA j, which put two duplicates on a phoneme the first time that symbol populated its features. The surviving rows are the ones in canonical position, matching the feature order comparable segments use. The m_justChangedFeatures latch stays. It stands in for a view of how a phoneme's declared features diverge from the standard features of its IPA symbol, which there is currently nowhere to show. Five tests cover setting a symbol, repopulating the same symbol, changing a symbol, and editing a symbol after features were already set, plus a check that no segment in the shipped inventory names a feature twice. All five fail without this change. Phonemes corrupted before this change are not repaired by it and are tracked in LT-22716. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1087 +/- ##
==========================================
+ Coverage 38.07% 38.37% +0.30%
==========================================
Files 1499 1508 +9
Lines 350141 350653 +512
Branches 40238 40309 +71
==========================================
+ Hits 133304 134576 +1272
+ Misses 187558 186848 -710
+ Partials 29279 29229 -50
🚀 New features to boost your workflow:
|
NUnit Tests 1 files ± 0 1 suites ±0 8m 54s ⏱️ - 1m 23s Results for commit c48fbdd. ± Comparison against base commit 1cfe930. This pull request removes 20 and adds 97 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
On the existing-project problem deferred to LT-22716: that repair should be run by the If the repair is deterministic — if a duplicated pair always has an unambiguous winner, so If it is not deterministic — if the two specs can disagree and a person has to decide which That seam is five members ( string Label { get; }
UtilityDlg Dialog { set; }
void LoadUtilities();
void OnSelection();
void Process();The closest exemplar is var cache = m_dlg.PropTable.GetValue<LcmCache>("cache");
UndoableUnitOfWorkHelper.Do(ITextStrings.ksUndoMergeAnalyses, ITextStrings.ksRedoMergeAnalyses,
cache.ActionHandlerAccessor,
() => WfiWordformServices.MergeDuplicateAnalyses(cache, new ProgressBarWrapper(m_dlg.ProgressBar)));
Either route argues for extracting the populate loop out of This review was assisted by Claude Fable 5. |
Move the populate loop into PhonemeFeaturePopulator, a plain service taking (LcmCache, IPhPhoneme, XDocument). The LT-22716 repair for phonemes already carrying duplicates needs something to call, and neither a Tools > Utilities utility nor a FixData rule can call a UserControl, which every Slice is. The slice keeps the decision of WHEN to write, because that depends on slice state: the m_justChangedFeatures latch records that this slice populated the features, which is what lets clearing the symbol clear them again without discarding features a user set through the chooser. Only the writing moved. Also expose DuplicatedFeatureIds, so the repair can both find affected phonemes and report what it fixed, and ClearFeatures, which the slice's clear branch now calls. Add four service-level tests that drive the populator directly rather than constructing a UserControl, proving idempotence unconditionally instead of through the slice's gate. Removing the fix makes five tests red across both levels, so the extraction is behaviour-preserving and the new tests are sensitive rather than decorative. Give the fixture Apartment(STA). It builds a real Slice, whose rootsite creates apartment-threaded Views COM objects, and NUnit 3 defaults to MTA. Three existing slice fixtures in the repo omit this, so it is a latent problem there too rather than something this branch introduced. Correct the doc comment on SetFeaturesBasedOnIPA, which was the description method's summary copy-pasted onto it.
|
Both points taken, and the extraction is done in The extraction
Callers own the unit of work; nothing in there starts one. What did not move is the decision of when to write. That depends on slice Where I did not follow youTwo of your three reasons for retargeting the tests hold; one does not.
So the four stayed, and four service-level tests were added that drive the That is 9 tests where there were 5, and only the pre-existing four construct a Evidence the extraction did not change behaviour: with LT-22716Both routes are now written onto the ticket, with the determinism question One thing I added there that is worth flagging here: re-running VerificationDebug build clean, comment-hygiene clean. |
3 participants
Setting or correcting a phoneme's Basic IPA Symbol no longer leaves it holding
the same phonological feature twice. Changing a symbol from
ptotpreviously appended
t's entire standard feature set on top ofp's: 16duplicated features from one ordinary edit, many carrying contradictory values.
The unknown you are starting with is whether this is the whole cause or a
symptom. Every writer to
PhPhoneme.FeaturesOAwas audited. Both featurechoosers and the bulk-edit path clear or upsert before writing; the IPA
populate loop was the only one that appended blind. Two defects compound there
j, and a slice-locallatch short-circuits the "features are not already filled" gate and can never
reset while the symbol is non-empty. Review is better spent on whether an
upsert is the right containment than on hunting for other writers.
Where to look
BasicIPASymbolSlice.csupserts throughIFsFeatStruc.GetOrCreateValue, thehelper the choosers and bulk edit already use.
IFsClosedFeature, which subsumes the oldnull check and skips anything with no symbolic value to assign.
BasicIPAInfo.xmlthe retainedjrows are the ones in canonicalposition; its first eight features now match the order
kuses.passed for the wrong reason and was corrected before being trusted.
be reintroduced silently.
Deliberately not here
m_justChangedFeatureslatch stays.nothing at all (recorded on LT-22714).
Verification. Build clean, 0 warnings.
MorphologyEditorDllTests12/12.Red and green confirmed in both directions. The full suite was not run.
build.ps1 -CommentHygienecould not complete: it trips on an untrackedscratch file that predates this branch, and that run confirmed zero violations
in these three files.
Reading this a year from now - start here
This branch is the containment half of a two-part story. The duplication had
two independent causes, and only the cheap one is fixed here. The expensive one
still present by choice.
If you are here because the duplication came back, check first whether the
latch is still in
BasicIPASymbolSlice.SetFeaturesBasedOnIPA. If it is, thebug you are looking at is probably not a duplicate but a stale feature: one
the previous IPA symbol specified and the current one does not name, which the
upsert has no reason to remove.
Decisions, and why
Upsert rather than skip. The obvious guard - "if this feature already
exists, skip it" - is wrong in a way that is easy to miss. On a
ptotedit it prevents the duplicate but silently discards
t's values for everyshared feature, leaving the phoneme still described as
p. Any guard here hasto update the existing specification, not step over it.
The latch was kept deliberately.
m_justChangedFeaturesis a crude recordof "these features are mine, not the user's". Deleting it would make the
mechanism idempotent, but it would also mean an IPA symbol overwrites
hand-picked feature values, and it would force a decision about whether a
symbol change owns the whole feature set or only the features the new symbol
names. The durable answer is a view showing where a phoneme's declared features
diverge from the standard features of its IPA symbol. There is nowhere to
present that today, so the latch remains a placeholder for the missing view
rather than an oversight to clean up.
Which duplicate rows to delete from the inventory. The two duplicated pairs
for
jwere byte-identical, so the choice looked arbitrary. It was not:j'sfeature order was compared against neighbouring segments, and positions 1-8
match
kexactly, with Anterior, Coronal and High at 4, 5 and 6. Theoddly-indented block was therefore in canonical position and the later,
normally-indented pair was the intruder - the opposite of what the stray
indentation suggests.
Paths not taken
Three fixes were costed on LT-22714.
Categorical, in the slice. Remove the latch from the gate and upsert. This
closes the bug class rather than the instance and also fixes the chooser-first
defect. Rejected for now only because it forces the ownership-semantics
decision described above, which wants the divergence view first.
Model invariant in liblcm. Enforce one specification per feature inside
FsFeatStrucso no caller anywhere can duplicate. Attractive, and the onlyuniversal option, but it hits a concrete obstacle: every call site reviewed
adds the specification before setting its feature (
FeatureSpecsOC.Add(value); value.FeatureRA = featDefn;). AtAddtimeFeatureRAis still null, so anadd-time check cannot see the feature at all; the invariant would have to hook
the
FeatureRAsetter. Throwing there also risks breaking incremental objectconstruction during project load, XML import and undo replay. It is worth doing
later as a debug-only assertion, not a runtime invariant.
Surprising findings
Several plausible culprits were checked and cleared, which is worth recording
so they are not re-investigated:
BasicIPASymbolChangedis an event on thelong-lived model object subscribed from a UI slice, which is the classic leak
shape. It does not leak:
DataTree.ResetandDataTree.RemoveSlicebothdispose, and the unsubscribe runs before
base.Dispose.re-fire the side effect; the generated setter does a value-equality check.
features into a freshly created structure, and LIFT carries no phonemes.
PriorityUnionmatching. It reads as matching onFeatureRA.Name, butNameis an accessor cached per object with nooperator==overload, so thecomparison is really feature-object identity. It works, but it will not
collapse two distinct feature definitions that share a name.
PriorityUnioncannot repair. It resolves matches withmyFeatureValues.First(), so on an already-duplicated phoneme it updates onecopy and leaves the twin. This is why bulk edit will not clean up existing
corruption, and why LT-22716 exists.
Evidence
The regression tests were run against a reverted working tree to confirm they
are not vacuous. Pre-fix failures, which also quantify the defect:
SettingSymbol_AddsEachFeatureOncejduplicated 2 features on a fresh phoneme, from the data defect aloneRepopulatingSameSymbol_DoesNotDuplicateFeaturespduplicated 18 featuresChangingSymbol_DoesNotDuplicateSharedFeaturesptotduplicated 16 featuresSymbolEditAfterFeaturesAlreadySet_DoesNotDuplicateFeaturesBasicIPAInfo_NoSegmentNamesTheSameFeatureTwicej: fPAAnterior, fPACoronalRepopulatingSameSymbolinitially failed withNot in the right state to register a changerather than on the assertion, because it calledSetFeaturesBasedOnIPAoutside a unit of work. That also meant it passed greenfor the wrong reason: post-fix the upsert writes the same value back, registers
no change, and so raised nothing. It was wrapped in a unit of work and both
directions re-run before being trusted.
A scripted audit of the shipped inventory found exactly one affected segment
out of 245.
Preflight review details
See
.review/summary.mdon the branch author's working copy. The findings itrecords are reproduced in the pitch and accordions above: no Critical and no
Important findings; five Minor, of which three are the deliberate deferrals
listed under "Deliberately not here", one is a latent liblcm edge case that
this path cannot reach, and one is the comment-hygiene gate being blocked by an
untracked file predating the branch.
This change is