From ebeceddbe41b4576b3dea7c1d963b0e6992ea624 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 19 Aug 2026 13:15:23 -0400 Subject: [PATCH] LT-22714: Stop the IPA populate path duplicating phoneme features 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 --- DistFiles/Templates/BasicIPAInfo.xml | 8 +- Src/LexText/Morphology/BasicIPASymbolSlice.cs | 10 +- .../BasicIPASymbolSliceTests.cs | 266 ++++++++++++++++++ 3 files changed, 275 insertions(+), 9 deletions(-) create mode 100644 Src/LexText/Morphology/MorphologyEditorDllTests/BasicIPASymbolSliceTests.cs diff --git a/DistFiles/Templates/BasicIPAInfo.xml b/DistFiles/Templates/BasicIPAInfo.xml index 568b79a5d0..b31ddfe851 100644 --- a/DistFiles/Templates/BasicIPAInfo.xml +++ b/DistFiles/Templates/BasicIPAInfo.xml @@ -340,13 +340,11 @@ - - - - - + + + diff --git a/Src/LexText/Morphology/BasicIPASymbolSlice.cs b/Src/LexText/Morphology/BasicIPASymbolSlice.cs index a2b7c267db..71d699d95d 100644 --- a/Src/LexText/Morphology/BasicIPASymbolSlice.cs +++ b/Src/LexText/Morphology/BasicIPASymbolSlice.cs @@ -128,7 +128,8 @@ public void SetFeaturesBasedOnIPA() var sFeature = (string) feature.Attribute("feature"); var sValue = (string) feature.Attribute("value"); IFsFeatDefn featDefn = m_cache.LanguageProject.PhFeatureSystemOA.GetFeature(sFeature); - if (featDefn == null) + var closedFeat = featDefn as IFsClosedFeature; + if (closedFeat == null) continue; IFsSymFeatVal symVal = m_cache.LanguageProject.PhFeatureSystemOA.GetSymbolicValue(sValue); @@ -138,9 +139,10 @@ public void SetFeaturesBasedOnIPA() { phoneme.FeaturesOA = m_cache.ServiceLocator.GetInstance().Create(); } - IFsClosedValue value = m_cache.ServiceLocator.GetInstance().Create(); - phoneme.FeaturesOA.FeatureSpecsOC.Add(value); - value.FeatureRA = featDefn; + // Reuse any spec already held for this feature, so that repopulating a + // phoneme cannot leave it with two specs for one feature (LT-22714). + IFsClosedValue value = phoneme.FeaturesOA.GetOrCreateValue(closedFeat); + value.FeatureRA = closedFeat; value.ValueRA = symVal; m_justChangedFeatures = true; } diff --git a/Src/LexText/Morphology/MorphologyEditorDllTests/BasicIPASymbolSliceTests.cs b/Src/LexText/Morphology/MorphologyEditorDllTests/BasicIPASymbolSliceTests.cs new file mode 100644 index 0000000000..f2b8111b54 --- /dev/null +++ b/Src/LexText/Morphology/MorphologyEditorDllTests/BasicIPASymbolSliceTests.cs @@ -0,0 +1,266 @@ +// 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.Collections.Generic; +using System.IO; +using System.Linq; +using System.Xml.Linq; +using System.Xml.XPath; + +using NUnit.Framework; +using SIL.FieldWorks.Common.FwUtils; +using SIL.LCModel; +using SIL.LCModel.Core.Text; +using SIL.LCModel.Infrastructure; + +namespace SIL.FieldWorks.XWorks.MorphologyEditor +{ + /// + /// Tests that populating a phoneme's phonological features from its Basic IPA Symbol + /// leaves at most one feature specification per phonological feature (LT-22714). + /// + [TestFixture] + public class BasicIPASymbolSliceTests : MemoryOnlyBackendProviderTestBase + { + private IPhPhoneme m_phoneme; + + public override void TestSetup() + { + base.TestSetup(); + NonUndoableUnitOfWorkHelper.Do(m_actionHandler, () => + { + var langProject = Cache.LanguageProject; + if (langProject.PhonologicalDataOA == null) + { + langProject.PhonologicalDataOA = + Cache.ServiceLocator.GetInstance().Create(); + } + if (langProject.PhFeatureSystemOA == null) + { + langProject.PhFeatureSystemOA = + Cache.ServiceLocator.GetInstance().Create(); + } + var phonemeSet = Cache.ServiceLocator.GetInstance().Create(); + langProject.PhonologicalDataOA.PhonemeSetsOS.Add(phonemeSet); + m_phoneme = Cache.ServiceLocator.GetInstance().Create(); + phonemeSet.PhonemesOC.Add(m_phoneme); + }); + } + + public override void TestTearDown() + { + m_phoneme = null; + base.TestTearDown(); + } + + /// + /// The shipped IPA inventory must not name the same feature twice for one segment, + /// because each pair becomes a separate feature specification on the phoneme. + /// + [Test] + public void BasicIPAInfo_NoSegmentNamesTheSameFeatureTwice() + { + var offenders = new List(); + foreach (var segment in IpaInfoDocument().XPathSelectElements( + "/SegmentDefinitions/SegmentDefinition")) + { + var featureIds = segment.XPathSelectElements("Features/FeatureValuePair") + .Select(pair => (string)pair.Attribute("feature")) + .ToList(); + var duplicated = featureIds.GroupBy(id => id) + .Where(group => group.Count() > 1) + .Select(group => group.Key) + .ToList(); + if (duplicated.Count > 0) + { + var representation = segment.XPathSelectElement("Representations/Representation"); + offenders.Add(string.Format("{0}: {1}", + representation == null ? "?" : representation.Value.Trim(), + string.Join(", ", duplicated))); + } + } + Assert.That(offenders, Is.Empty, + "segments naming a feature more than once: " + string.Join("; ", offenders)); + } + + [Test] + public void SettingSymbol_AddsEachFeatureOnce() + { + CreateFeatureSystemFor("j"); + using (CreateSlice()) + { + SetSymbol("j"); + + AssertNoFeatureIsSpecifiedTwice(); + } + } + + /// + /// Reproduces the reported sequence: once a symbol has populated the features, the + /// slice re-enters its populate branch on every later call. + /// + [Test] + public void RepopulatingSameSymbol_DoesNotDuplicateFeatures() + { + CreateFeatureSystemFor("p"); + using (var slice = CreateSlice()) + { + SetSymbol("p"); + var countAfterFirstPopulate = m_phoneme.FeaturesOA.FeatureSpecsOC.Count; + + NonUndoableUnitOfWorkHelper.Do(m_actionHandler, () => + { + slice.SetFeaturesBasedOnIPA(); + slice.SetFeaturesBasedOnIPA(); + }); + + AssertNoFeatureIsSpecifiedTwice(); + Assert.That(m_phoneme.FeaturesOA.FeatureSpecsOC.Count, + Is.EqualTo(countAfterFirstPopulate)); + } + } + + /// + /// Correcting a symbol must not leave the phoneme carrying two specifications, with + /// contradictory values, for the features the two symbols share. + /// + [Test] + public void ChangingSymbol_DoesNotDuplicateSharedFeatures() + { + CreateFeatureSystemFor("p", "t"); + using (CreateSlice()) + { + SetSymbol("p"); + SetSymbol("t"); + + AssertNoFeatureIsSpecifiedTwice(); + foreach (var pair in FeaturePairsFor("t")) + { + var spec = m_phoneme.FeaturesOA.FeatureSpecsOC.OfType() + .SingleOrDefault(value => value.FeatureRA.CatalogSourceId == pair.Key); + Assert.That(spec, Is.Not.Null, "no specification for " + pair.Key); + Assert.That(spec.ValueRA.CatalogSourceId, Is.EqualTo(pair.Value), + "wrong value for " + pair.Key); + } + } + } + + /// + /// Features set through the chooser leave the slice unaware that the feature structure + /// is already populated, so a later symbol edit must still not duplicate anything. + /// + [Test] + public void SymbolEditAfterFeaturesAlreadySet_DoesNotDuplicateFeatures() + { + CreateFeatureSystemFor("p", "t"); + using (var slice = CreateSlice()) + { + SetSymbol("p"); + NonUndoableUnitOfWorkHelper.Do(m_actionHandler, () => + { + var voice = Cache.LanguageProject.PhFeatureSystemOA.GetFeature("fPAVoice"); + var closedValue = m_phoneme.FeaturesOA.GetOrCreateValue((IFsClosedFeature)voice); + closedValue.FeatureRA = voice; + closedValue.ValueRA = ((IFsClosedFeature)voice).ValuesOC.First(); + }); + + SetSymbol("t"); + + AssertNoFeatureIsSpecifiedTwice(); + } + } + + private void AssertNoFeatureIsSpecifiedTwice() + { + Assert.That(m_phoneme.FeaturesOA, Is.Not.Null, "no features were populated"); + var duplicated = m_phoneme.FeaturesOA.FeatureSpecsOC + .GroupBy(spec => spec.FeatureRA) + .Where(group => group.Count() > 1) + .Select(group => group.Key.CatalogSourceId) + .ToList(); + Assert.That(duplicated, Is.Empty, + "features specified more than once: " + string.Join(", ", duplicated)); + } + + private BasicIPASymbolSlice CreateSlice() + { + var slice = new BasicIPASymbolSlice(Cache, "customWithParams", + PhPhonemeTags.kflidBasicIPASymbol, null, m_phoneme, null, + Cache.DefaultPronunciationWs); + slice.Cache = Cache; + return slice; + } + + private void SetSymbol(string ipaSymbol) + { + NonUndoableUnitOfWorkHelper.Do(m_actionHandler, () => + { + m_phoneme.BasicIPASymbol = + TsStringUtils.MakeString(ipaSymbol, Cache.DefaultPronunciationWs); + }); + } + + private static XDocument IpaInfoDocument() + { + return XDocument.Load(Path.Combine(FwDirectoryFinder.TemplateDirectory, + PhPhonemeTags.ksBasicIPAInfoFile)); + } + + /// + /// Gets the feature-to-value catalog ids the shipped inventory lists for a symbol, + /// keeping the first value where a symbol names the same feature more than once. + /// + private static IDictionary FeaturePairsFor(string ipaSymbol) + { + var pairs = new Dictionary(); + var features = IpaInfoDocument().XPathSelectElement( + "/SegmentDefinitions/SegmentDefinition[Representations/Representation[.='" + + ipaSymbol + "']]/Features"); + Assert.That(features, Is.Not.Null, "no inventory entry for " + ipaSymbol); + foreach (var pair in features.Elements("FeatureValuePair")) + { + var featureId = (string)pair.Attribute("feature"); + if (!pairs.ContainsKey(featureId)) + pairs.Add(featureId, (string)pair.Attribute("value")); + } + return pairs; + } + + /// + /// Builds the closed features and symbolic values the given symbols refer to, since a + /// memory-only project starts with an empty phonological feature system. + /// + private void CreateFeatureSystemFor(params string[] ipaSymbols) + { + NonUndoableUnitOfWorkHelper.Do(m_actionHandler, () => + { + var featureSystem = Cache.LanguageProject.PhFeatureSystemOA; + foreach (var ipaSymbol in ipaSymbols) + { + foreach (var pair in FeaturePairsFor(ipaSymbol)) + { + var closedFeature = + featureSystem.GetFeature(pair.Key) as IFsClosedFeature; + if (closedFeature == null) + { + closedFeature = Cache.ServiceLocator + .GetInstance().Create(); + featureSystem.FeaturesOC.Add(closedFeature); + closedFeature.CatalogSourceId = pair.Key; + closedFeature.Name.SetAnalysisDefaultWritingSystem(pair.Key); + } + if (closedFeature.GetSymbolicValue(pair.Value) == null) + { + var symbolicValue = Cache.ServiceLocator + .GetInstance().Create(); + closedFeature.ValuesOC.Add(symbolicValue); + symbolicValue.CatalogSourceId = pair.Value; + symbolicValue.Name.SetAnalysisDefaultWritingSystem(pair.Value); + } + } + } + }); + } + } +}