From 647bda25155d199a61e66c20eb0129cc59251502 Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:31:22 -0400 Subject: [PATCH] Move property sets out of units of work and to sequence ends This clears one of the conditions that keep PropertyTable change notifications on the deferred Mediator queue: exception ownership. A set inside a unit of work, or in the middle of a multi-step sequence, cannot safely be delivered synchronously, which the Pub/Sub conversion will do, because a handler that throws would roll back data or cut the sequence short. The sets below now sit after their unit of work or at the end of their sequence. Each still broadcasts through the queue, and the other conditions on these sites, such as being set before the reacting view exists, re-entry, and the slot among a workflow's other delayed paths, remain for later changes. The WritingSystemHvo change clears a different condition, ordering, for that one set. Under today's deferred delivery nothing observable changes: - ReversalIndexGuid is set after ReversalListener.Init's non-undoable task ends. - DictionaryPublicationLayout and ReversalIndexPublicationLayout are collected during the configuration migration and written after each of its units of work returns. A pending selection shadows the table for later reads in the same migration, so a second layout type of the same name still cannot match. They are written even when the migration throws, because the converted files are already saved. - ActiveClerk and ActiveClerkSelectedObject are the last sets in the record bar activation sequence, in their old order, so no notification passes another. - ShowRecordList is set in XmlDocView after the clerk is active and before UpdateList reads it while populating the record bar. - The WritingSystemHvo suppress flag is set before the property, so the handler finds it however the change is delivered. One ReversalIndexGuid set no longer broadcasts: ReversalClerk's fallback to the first index when no valid index is selected. The only listener is a ReversalClerk, one clerk is a message target at a time, and that clerk is already changing to the index it just chose, so the deferred second call was a no-op. Co-Authored-By: Claude Fable 5.1 --- Src/Common/SimpleRootSite/EditingHelper.cs | 4 +- .../LexEdDllTests/ReversalListenerTests.cs | 39 +++++++++ Src/LexText/Lexicon/ReversalListener.cs | 13 +-- .../PreHistoricMigrator.cs | 82 ++++++++++++++++--- Src/xWorks/RecordClerk.cs | 8 +- Src/xWorks/XmlDocView.cs | 5 +- .../PreHistoricMigratorTests.cs | 57 +++++++++++++ 7 files changed, 186 insertions(+), 22 deletions(-) create mode 100644 Src/LexText/Lexicon/LexEdDllTests/ReversalListenerTests.cs diff --git a/Src/Common/SimpleRootSite/EditingHelper.cs b/Src/Common/SimpleRootSite/EditingHelper.cs index 56f14d304f..b688ab2730 100644 --- a/Src/Common/SimpleRootSite/EditingHelper.cs +++ b/Src/Common/SimpleRootSite/EditingHelper.cs @@ -2842,9 +2842,11 @@ private void SetWritingSystemPropertyFromSelection(IVwRootBox rootbox, int oldWritingSystemHvo = int.Parse(s); if (oldWritingSystemHvo != ws) { + // Flag before the set: the handler must find it however the + // change is delivered. + m_fSuppressNextWritingSystemHvoChanged = true; rs.PropTable.SetProperty("WritingSystemHvo", ws.ToString(), true); rs.PropTable.SetPropertyPersistence("WritingSystemHvo", false); - m_fSuppressNextWritingSystemHvoChanged = true; } } } diff --git a/Src/LexText/Lexicon/LexEdDllTests/ReversalListenerTests.cs b/Src/LexText/Lexicon/LexEdDllTests/ReversalListenerTests.cs new file mode 100644 index 0000000000..217d52626b --- /dev/null +++ b/Src/LexText/Lexicon/LexEdDllTests/ReversalListenerTests.cs @@ -0,0 +1,39 @@ +// 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; +using SIL.FieldWorks.XWorks.LexEd; +using SIL.LCModel; +using SIL.LCModel.Infrastructure; +using XCore; + +namespace LexEdDllTests +{ + [TestFixture] + public class ReversalListenerTests : MemoryOnlyBackendProviderTestBase + { + [Test] + public void Init_NoReversalIndexSelected_SelectsTheFirstIndexOnceItsTaskHasEnded() + { + var repository = Cache.ServiceLocator.GetInstance(); + IReversalIndex index = null; + NonUndoableUnitOfWorkHelper.Do(Cache.ActionHandlerAccessor, + () => index = repository.FindOrCreateIndexForWs(Cache.DefaultAnalWs)); + using (var mediator = new Mediator()) + using (var propertyTable = new PropertyTable(mediator)) + using (var listener = new ReversalListener()) + { + propertyTable.SetProperty("cache", Cache, false); + propertyTable.SetPropertyDispose("cache", false); + + listener.Init(mediator, propertyTable, null); + + Assert.That(propertyTable.GetStringProperty("ReversalIndexGuid", null), + Is.EqualTo(index.Guid.ToString())); + Assert.That(Cache.ActionHandlerAccessor.CurrentDepth, Is.EqualTo(0), + "Init ends the task it opened"); + } + } + } +} diff --git a/Src/LexText/Lexicon/ReversalListener.cs b/Src/LexText/Lexicon/ReversalListener.cs index f4733f29bd..ec5def8e48 100644 --- a/Src/LexText/Lexicon/ReversalListener.cs +++ b/Src/LexText/Lexicon/ReversalListener.cs @@ -194,21 +194,20 @@ public virtual void Init(Mediator mediator, PropertyTable propertyTable, XmlNode // Set up for the reversal index combo box or dropdown menu. var reversalIndexGuid = ReversalIndexEntryUi.GetObjectGuidIfValid(m_propertyTable, "ReversalIndexGuid"); + Guid firstGuid = Guid.Empty; if (reversalIndexGuid == Guid.Empty) { // We haven't established the reversal index yet. Choose the first one available. - Guid firstGuid = Guid.Empty; List reversalIds = cache.LanguageProject.LexDbOA.CurrentReversalIndices; if (reversalIds.Count > 0) firstGuid = reversalIds[0].Guid; else if (cache.LanguageProject.LexDbOA.ReversalIndexesOC.Count > 0) firstGuid = cache.LanguageProject.LexDbOA.ReversalIndexesOC.ToGuidArray()[0]; - if (firstGuid != Guid.Empty) - { - SetReversalIndexGuid(firstGuid); - } } cache.DomainDataByFlid.EndNonUndoableTask(); + // Set after the task so the change notification can never run inside it. + if (firstGuid != Guid.Empty) + SetReversalIndexGuid(firstGuid); } /// @@ -422,7 +421,9 @@ public override void ChangeOwningObject(Guid newGuid, bool updateAndNotify, Dict newGuid = Cache.ServiceLocator.GetInstance().AllInstances().First().Guid; if (updateAndNotify) { - m_propertyTable.SetProperty("ReversalIndexGuid", newGuid.ToString(), true); + // No broadcast: the only listener is a ReversalClerk, and this one + // is already changing to that index. + m_propertyTable.SetProperty("ReversalIndexGuid", newGuid.ToString(), false); } } diff --git a/Src/xWorks/DictionaryConfigurationMigrators/PreHistoricMigrator.cs b/Src/xWorks/DictionaryConfigurationMigrators/PreHistoricMigrator.cs index 0873cb2623..793343d12e 100644 --- a/Src/xWorks/DictionaryConfigurationMigrators/PreHistoricMigrator.cs +++ b/Src/xWorks/DictionaryConfigurationMigrators/PreHistoricMigrator.cs @@ -56,6 +56,14 @@ public class PreHistoricMigrator : IDictionaryConfigurationMigrator, ILayoutConv /// internal string m_configDirSuffixBeingMigrated; + /// + /// Layout selections (property name, converted configuration path) found while + /// migrating, written to the property table once the migration's unit of work + /// has ended. + /// + private readonly List> m_pendingLayoutSelections = + new List>(); + /// /// The mediator is needed to retrieve information about the prehistoric configurations /// @@ -97,17 +105,33 @@ public void MigrateIfNeeded(SimpleLogger logger, PropertyTable propertyTable, st m_logger.WriteLine("Migrating dictionary configurations"); m_configDirSuffixBeingMigrated = DictionaryConfigurationListener.DictConfigDirName; Directory.CreateDirectory(Path.Combine(projectPath, m_configDirSuffixBeingMigrated)); - UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW( - "Undo Migrate old Dictionary Configurations", "Redo Migrate old Dictionary Configurations", - Cache.ActionHandlerAccessor, PerformMigrationUOW); + MigrateInUnitOfWork("Undo Migrate old Dictionary Configurations", + "Redo Migrate old Dictionary Configurations"); m_logger.WriteLine(string.Format("Migrating Reversal Index configurations, if any - {0}", DateTime.Now.ToLongTimeString())); m_configDirSuffixBeingMigrated = DictionaryConfigurationListener.RevIndexConfigDirName; Directory.CreateDirectory(Path.Combine(projectPath, m_configDirSuffixBeingMigrated)); - UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW( - "Undo Migrate old Reversal Configurations", "Redo Migrate old Reversal Configurations", + MigrateInUnitOfWork("Undo Migrate old Reversal Configurations", + "Redo Migrate old Reversal Configurations"); + } + } + + /// + /// Runs PerformMigrationUOW in a unit of work, then writes the layout + /// selections it found. They are written even when the migration throws: + /// the converted files are already saved. + /// + private void MigrateInUnitOfWork(string undoText, string redoText) + { + try + { + UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW(undoText, redoText, Cache.ActionHandlerAccessor, PerformMigrationUOW); } + finally + { + ApplyPendingLayoutSelections(); + } } /// Perform the migration for Dictionary or Reversal (depending on m_configDirSuffixBeingMigrated. @@ -1028,16 +1052,54 @@ private bool LayoutOptionsMatchStyle(BaseStyleInfo style, XmlDocConfigureDlg.Lay return wsLabel.Split(',').Select(item => new DictionaryNodeListOptions.DictionaryNodeOption { Id = item.Trim(), IsEnabled = true }).ToList(); } - private void MigratePublicationLayoutSelection(string oldLayout, string newPath) + /// + /// Records that the configuration at replaces + /// as the selected layout, if that layout is + /// selected. + /// + /// Internal only for tests; production entry point is + /// MigrateIfNeeded() + internal void MigratePublicationLayoutSelection(string oldLayout, string newPath) { - if (oldLayout.Equals(m_propertyTable.GetStringProperty("DictionaryPublicationLayout", string.Empty))) + if (oldLayout.Equals(CurrentLayoutSelection("DictionaryPublicationLayout"))) + { + m_pendingLayoutSelections.Add( + new KeyValuePair("DictionaryPublicationLayout", newPath)); + } + else if (oldLayout.Equals(CurrentLayoutSelection("ReversalIndexPublicationLayout"))) { - m_propertyTable.SetProperty("DictionaryPublicationLayout", newPath, true); + m_pendingLayoutSelections.Add( + new KeyValuePair("ReversalIndexPublicationLayout", newPath)); } - else if (oldLayout.Equals(m_propertyTable.GetStringProperty("ReversalIndexPublicationLayout", string.Empty))) + } + + /// + /// The selected layout as the migration sees it: a pending selection shadows + /// the table. + /// + private string CurrentLayoutSelection(string propertyName) + { + var pending = m_pendingLayoutSelections.LastOrDefault( + selection => selection.Key == propertyName); + return pending.Key == propertyName + ? pending.Value + : m_propertyTable.GetStringProperty(propertyName, string.Empty); + } + + /// + /// Writes the recorded layout selections to the property table. Called outside + /// the migration's unit of work, so that a change notification can never run + /// inside it. + /// + /// Internal only for tests; production entry point is + /// MigrateIfNeeded() + internal void ApplyPendingLayoutSelections() + { + foreach (var selection in m_pendingLayoutSelections) { - m_propertyTable.SetProperty("ReversalIndexPublicationLayout", newPath, true); + m_propertyTable.SetProperty(selection.Key, selection.Value, true); } + m_pendingLayoutSelections.Clear(); } #region trivial portions of the ILayoutConverter implementation diff --git a/Src/xWorks/RecordClerk.cs b/Src/xWorks/RecordClerk.cs index 8c6370d299..5786008ce4 100644 --- a/Src/xWorks/RecordClerk.cs +++ b/Src/xWorks/RecordClerk.cs @@ -1994,16 +1994,18 @@ virtual public bool IsControllingTheRecordTreeBar oldActiveClerk?.BecomeInactive(); m_propertyTable.SetProperty("OldActiveClerk", oldActiveClerk, false); m_propertyTable.SetPropertyPersistence("OldActiveClerk", false); - m_propertyTable.SetProperty("ActiveClerk", this, true); - m_propertyTable.SetPropertyPersistence("ActiveClerk", false); // We are adding this property so that EntryDlgListener can get access to the owning object // without first getting a RecordClerk, since getting a RecordClerk at that level causes a // circular dependency in compilation. m_propertyTable.SetProperty("ActiveClerkOwningObject", OwningObject, false); m_propertyTable.SetPropertyPersistence("ActiveClerkOwningObject", false); + Cache.DomainDataByFlid.AddNotification(this); + // Both broadcasting sets come last, ActiveClerk first, so the + // activation is complete before a change notification runs. + m_propertyTable.SetProperty("ActiveClerk", this, true); + m_propertyTable.SetPropertyPersistence("ActiveClerk", false); m_propertyTable.SetProperty("ActiveClerkSelectedObject", CurrentObject, true); m_propertyTable.SetPropertyPersistence("ActiveClerkSelectedObject", false); - Cache.DomainDataByFlid.AddNotification(this); } } get diff --git a/Src/xWorks/XmlDocView.cs b/Src/xWorks/XmlDocView.cs index 9ef9829522..9dc589094a 100644 --- a/Src/xWorks/XmlDocView.cs +++ b/Src/xWorks/XmlDocView.cs @@ -565,6 +565,9 @@ protected override void SetupDataContext() //m_flid = RecordClerk.GetFlidOfVectorFromName(m_vectorName, Cache, out m_owningObject); RecordClerk clerk = Clerk; clerk.ActivateUI(false); + // This view never shows the record bar. Set once the clerk is active, + // before UpdateList, which reads it while populating the bar. + m_propertyTable.SetProperty("ShowRecordList", false, true); // Enhance JohnT: could use logic similar to RecordView.InitBase to load persisted list contents (filtered and sorted). if (clerk.RequestedLoadWhileSuppressed) clerk.UpdateList(false); @@ -757,8 +760,6 @@ protected void InitBase(Mediator mediator, PropertyTable propertyTable, XmlNode m_mediator.AddColleague(this); - m_propertyTable.SetProperty("ShowRecordList", false, true); - SetupDataContext(); ShowRecord(); } diff --git a/Src/xWorks/xWorksTests/DictionaryConfigurationMigrators/PreHistoricMigratorTests.cs b/Src/xWorks/xWorksTests/DictionaryConfigurationMigrators/PreHistoricMigratorTests.cs index 33e466a6f7..040c7715a8 100644 --- a/Src/xWorks/xWorksTests/DictionaryConfigurationMigrators/PreHistoricMigratorTests.cs +++ b/Src/xWorks/xWorksTests/DictionaryConfigurationMigrators/PreHistoricMigratorTests.cs @@ -2580,7 +2580,64 @@ public void TestMigrateCustomFieldNode() Assert.That(newTypeNode1.Label, Is.EqualTo("Single Sense"), "A custom field copies its label properly during conversion"); } + [Test] + public void MigratePublicationLayoutSelection_IsWrittenOnlyWhenApplied() + { + const string dictionary = "DictionaryPublicationLayout"; + const string newPath = "C:\\config\\Stem.fwdictconfig"; + try + { + m_propertyTable.SetProperty(dictionary, "publishStem", false); + + m_migrator.MigratePublicationLayoutSelection("publishStem", newPath); + Assert.That(SelectedLayout(dictionary), Is.EqualTo("publishStem"), + "The selection waits until the migration's unit of work has ended"); + + m_migrator.ApplyPendingLayoutSelections(); + Assert.That(SelectedLayout(dictionary), Is.EqualTo(newPath)); + } + finally + { + m_propertyTable.RemoveProperty(dictionary); + } + } + + [Test] + public void MigratePublicationLayoutSelection_PendingSelectionShadowsTheTable() + { + const string dictionary = "DictionaryPublicationLayout"; + const string reversal = "ReversalIndexPublicationLayout"; + const string stemPath = "C:\\config\\Stem.fwdictconfig"; + const string stemCopyPath = "C:\\config\\Stem-copy.fwdictconfig"; + const string reversalPath = "C:\\config\\en.fwdictconfig"; + try + { + m_propertyTable.SetProperty(dictionary, "publishStem", false); + m_propertyTable.SetProperty(reversal, "publishReversal-en", false); + + m_migrator.MigratePublicationLayoutSelection("publishStem", stemPath); + // A second layout type of the same name compares against the pending + // path, not the table. + m_migrator.MigratePublicationLayoutSelection("publishStem", stemCopyPath); + m_migrator.MigratePublicationLayoutSelection("publishReversal-en", reversalPath); + m_migrator.ApplyPendingLayoutSelections(); + + Assert.That(SelectedLayout(dictionary), Is.EqualTo(stemPath)); + Assert.That(SelectedLayout(reversal), Is.EqualTo(reversalPath)); + } + finally + { + m_propertyTable.RemoveProperty(dictionary); + m_propertyTable.RemoveProperty(reversal); + } + } + #region Helper + private string SelectedLayout(string property) + { + return m_propertyTable.GetStringProperty(property, null); + } + private void DeleteStyleSheet(string styleName) { var style = m_styleSheet.FindStyle(styleName);