From 9deba58b9e0f85ec290e9bf3beddbe7df7b9dc49 Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Wed, 19 Aug 2026 16:17:35 -0400 Subject: [PATCH 1/2] Document the minimum commit body length gitlint enforces Gitlint's B5 rule rejects a body under 20 characters. The guidelines listed the other body rules but not this one, so a short body failed CI with no documented reason. --- .github/commit-guidelines.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/commit-guidelines.md b/.github/commit-guidelines.md index e5a7db099c..31daeb2c97 100644 --- a/.github/commit-guidelines.md +++ b/.github/commit-guidelines.md @@ -12,6 +12,7 @@ These align with the gitlint rules run in CI. ## Body (optional) - Blank line after the subject. +- At least 20 characters, if present. - Wrap lines at 80 characters. - Explain what and why over how; link issues like "Fixes #1234" when applicable. - No hard tabs, no trailing whitespace. From 643d4bb4fed14e1414c3de448676da1c7cdbc25a Mon Sep 17 00:00:00 2001 From: Zachary Burnham Date: Mon, 24 Aug 2026 18:00:08 -0400 Subject: [PATCH 2/2] Fix LT-22642: Make reversal configuration switching update the view Switching between two reversal configurations of the same writing system left the view and the pane bar label unchanged, and choosing a configuration in the Configure Reversal Index dialog left the label, the dropdown checkmark and the entry list disagreeing. The two properties describing the current reversal are kept in sync across a many-to-one mapping: the index guid comes from the configuration's writing system, so two configurations of one writing system select the same index. Arm m_updateContentLater only when the index actually changes. Otherwise no clerk reload follows, ActiveClerkSelectedObject never arrives, and the deferred UpdateContent never runs. Validate the configuration against ReversalIndexGuid rather than the clerk's current entry, which can still belong to the index being left during a reload, so an explicit choice survives. Sync the guid in RefreshAllContent. The Configure dialog saves the chosen configuration without broadcasting, so nothing else points the guid at the index it names. Co-Authored-By: Claude Opus 5 --- Src/FdoUi/ReversalIndexEntryUi.cs | 14 ++ Src/xWorks/XhtmlDocView.cs | 94 +++++++- Src/xWorks/xWorksTests/XhtmlDocViewTests.cs | 224 ++++++++++++++++++++ 3 files changed, 321 insertions(+), 11 deletions(-) diff --git a/Src/FdoUi/ReversalIndexEntryUi.cs b/Src/FdoUi/ReversalIndexEntryUi.cs index 5b4f03250c..dda8c2e380 100644 --- a/Src/FdoUi/ReversalIndexEntryUi.cs +++ b/Src/FdoUi/ReversalIndexEntryUi.cs @@ -46,6 +46,20 @@ protected override DummyCmObject GetMergeinfo(WindowParams wp, List + /// PropertyTable key naming the reversal index whose entries the clerk loads. + /// + public const string ReversalIndexGuidProperty = "ReversalIndexGuid"; + + /// + /// Fetches the GUID of the reversal index named by the ReversalIndexGuid property. + /// + /// The reversal index GUID, or empty GUID if there is a problem + public static Guid GetReversalIndexGuid(PropertyTable propertyTable) + { + return GetObjectGuidIfValid(propertyTable, ReversalIndexGuidProperty); + } + /// /// Fetches the GUID value of the given property, having checked it is a valid object. /// If it is not a valid object, the property is removed. diff --git a/Src/xWorks/XhtmlDocView.cs b/Src/xWorks/XhtmlDocView.cs index a74e556a3d..a460ce9374 100644 --- a/Src/xWorks/XhtmlDocView.cs +++ b/Src/xWorks/XhtmlDocView.cs @@ -9,10 +9,12 @@ using SIL.FieldWorks.Common.FwUtils; using SIL.FieldWorks.Common.RootSites; using SIL.FieldWorks.Common.Widgets; +using SIL.FieldWorks.FdoUi; using SIL.FieldWorks.FwCoreDlgControls; using SIL.FieldWorks.FwCoreDlgs; using SIL.IO; using SIL.LCModel; +using SIL.LCModel.Core.WritingSystems; using SIL.LCModel.DomainServices; using SIL.LCModel.Utils; using SIL.Progress; @@ -45,6 +47,10 @@ internal class XhtmlDocView : XWorksViewBase, IFindAndReplaceContext, IPostLayou internal string m_configObjectName; internal const string CurrentSelectedEntryClass = "currentSelectedEntry"; private const string ClassifiedDictConfig = "SemanticDomainSenses.fwdictconfig"; + /// + /// Id of the reversal clerk, declared in Configuration/Lexicon/areaConfiguration.xml. + /// + private const string AllReversalEntriesClerkId = "AllReversalEntries"; private const string FieldWorksPrintLimitEnv = "FIELDWORKS_PRINT_LIMIT"; private bool m_updateContentLater = false; // Whether we should postpone calling UpdateContent private string m_loadedConfig = null; @@ -73,7 +79,7 @@ public override void Init(Mediator mediator, PropertyTable propertyTable, XmlNod if (m_mainView.NativeBrowser is GeckoWebBrowser browser) { var clerk = XmlUtils.GetOptionalAttributeValue(configurationParameters, "clerk"); - if (clerk == "entries" || clerk == "AllReversalEntries" || clerk == "SemanticDomainList") + if (clerk == "entries" || clerk == AllReversalEntriesClerkId || clerk == "SemanticDomainList") { browser.DomClick += OnDomClick; browser.DomKeyPress += OnDomKeyPress; @@ -1125,9 +1131,10 @@ public void OnPropertyChanged(string name) var currentConfig = GetCurrentConfiguration(false); if (name == "ReversalIndexPublicationLayout") { - DictionaryConfigurationUtils.SetReversalIndexGuidBasedOnReversalIndexConfiguration(m_propertyTable, Cache); - // Wait until SetActiveSelectedEntryOnView to call UpdateContent. - m_updateContentLater = true; + // Sync ReversalIndexGuid to the chosen configuration. True means a + // different index, so let the clerk's reload refresh the view; false + // means no reload is coming, so refresh below. + m_updateContentLater = SyncReversalIndexGuidToConfiguration(); } var currentPublication = GetCurrentPublication(); var validPublication = GetValidPublicationForConfiguration(currentConfig) ?? xWorksStrings.AllEntriesPublication; @@ -1191,6 +1198,67 @@ private void RemoveStyleFromPreviousSelectedEntryOnView(GeckoWebBrowser browser) } } + /// + /// Points the ReversalIndexGuid property at the reversal index described by the current + /// ReversalIndexPublicationLayout. The index comes from the configuration's writing + /// system, so two configurations of one writing system select the same index. + /// + /// true if the selected index changed. Setting the property has + /// already told the clerk to reload, so ActiveClerkSelectedObject will follow on + /// Idle; the caller can defer refreshing until then. + internal bool SyncReversalIndexGuidToConfiguration() + { + var oldGuid = ReversalIndexEntryUi.GetReversalIndexGuid(m_propertyTable); + DictionaryConfigurationUtils.SetReversalIndexGuidBasedOnReversalIndexConfiguration(m_propertyTable, Cache); + return ReversalIndexEntryUi.GetReversalIndexGuid(m_propertyTable) != oldGuid; + } + + /// + /// Gets the writing system id of the reversal index named by the ReversalIndexGuid + /// property, or null if no valid index is selected. + /// + private string GetSelectedReversalWritingSystemId() + { + var reversalIndexGuid = ReversalIndexEntryUi.GetReversalIndexGuid(m_propertyTable); + if (reversalIndexGuid.Equals(Guid.Empty)) + return null; + var reversalIndex = Cache.ServiceLocator.GetObject(reversalIndexGuid) as IReversalIndex; + CoreWritingSystemDefinition writingSystem; + if (reversalIndex == null || !Cache.ServiceLocator.WritingSystemManager.TryGet(reversalIndex.WritingSystem, out writingSystem)) + return null; + return writingSystem.Id; + } + + /// + /// Decides whether ReversalIndexPublicationLayout still describes the reversal index + /// that is selected, and if not, works out what it should be replaced with. + /// + /// Writing system of the clerk's current + /// entry, used only when no valid index is selected. + /// Folder holding this project's reversal + /// configurations. + /// The configuration to switch to, or + /// null when the expected writing system has no configuration file. + /// true if the property needs to be rewritten. + internal bool TryGetReplacementReversalConfiguration(string currentEntryWritingSystemId, + string projectConfigurationDirectory, out string replacementConfiguration) + { + // Take the writing system from ReversalIndexGuid, the selection the user made; the + // clerk's current entry can still belong to the index we are leaving (LT-22642). + var expectedWritingSystemId = GetSelectedReversalWritingSystemId() ?? currentEntryWritingSystemId; + var currentConfiguration = m_propertyTable.GetStringProperty("ReversalIndexPublicationLayout", string.Empty); + if (File.Exists(currentConfiguration) + && new DictionaryConfigurationModel(currentConfiguration, Cache).WritingSystem == expectedWritingSystemId) + { + replacementConfiguration = null; + return false; + } + var newConfiguration = Path.Combine(projectConfigurationDirectory, + expectedWritingSystemId + DictionaryConfigurationModel.FileExtension); + replacementConfiguration = File.Exists(newConfiguration) ? newConfiguration : null; + return true; + } + /// /// Set the style attribute on the current entry to color the background. /// @@ -1198,7 +1266,7 @@ private void SetActiveSelectedEntryOnView(GeckoWebBrowser browser) { if (Clerk.CurrentObject == null) { - if (Clerk.Id == "AllReversalEntries" && m_updateContentLater) + if (Clerk.Id == AllReversalEntriesClerkId && m_updateContentLater) { // There are no entries, but we still need to clear the pane and update the title. var currentConfig = m_propertyTable.GetStringProperty("ReversalIndexPublicationLayout", string.Empty); @@ -1209,7 +1277,7 @@ private void SetActiveSelectedEntryOnView(GeckoWebBrowser browser) return; } - if (Clerk.Id == "AllReversalEntries") + if (Clerk.Id == AllReversalEntriesClerkId) { var reversalentry = Clerk.CurrentObject as IReversalIndexEntry; if (reversalentry == null) @@ -1219,13 +1287,12 @@ private void SetActiveSelectedEntryOnView(GeckoWebBrowser browser) return; var currReversalWs = writingSystem.Id; var currentConfig = m_propertyTable.GetStringProperty("ReversalIndexPublicationLayout", string.Empty); - var configuration = File.Exists(currentConfig) ? new DictionaryConfigurationModel(currentConfig, Cache) : null; var currentPage = GetTopCurrentPageButton(browser.Document.Body); - if (configuration == null || configuration.WritingSystem != currReversalWs) + string newConfig; + if (TryGetReplacementReversalConfiguration(currReversalWs, + DictionaryConfigurationListener.GetProjectConfigurationDirectory(m_propertyTable, reversalentry), out newConfig)) { - var newConfig = Path.Combine(DictionaryConfigurationListener.GetProjectConfigurationDirectory(m_propertyTable, reversalentry), - writingSystem.Id + DictionaryConfigurationModel.FileExtension); - m_propertyTable.SetProperty("ReversalIndexPublicationLayout", File.Exists(newConfig) ? newConfig : null, true); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", newConfig, true); } else if (m_updateContentLater) { // Force the content to be updated once (LT-21702). @@ -1350,6 +1417,11 @@ private void RefreshCurrentView(object obj) private void RefreshAllContent(object _) { var currentConfig = GetCurrentConfiguration(false); + if (Clerk.Id == AllReversalEntriesClerkId) + { + // The Configure dialog sets the layout silently, so sync the guid. + SyncReversalIndexGuidToConfiguration(); + } var currentPublication = GetCurrentPublication(); var validPublication = GetValidPublicationForConfiguration(currentConfig) ?? xWorksStrings.AllEntriesPublication; if (currentPublication != xWorksStrings.AllEntriesPublication && currentPublication != validPublication) diff --git a/Src/xWorks/xWorksTests/XhtmlDocViewTests.cs b/Src/xWorks/xWorksTests/XhtmlDocViewTests.cs index d63191bf35..2019234ac0 100644 --- a/Src/xWorks/xWorksTests/XhtmlDocViewTests.cs +++ b/Src/xWorks/xWorksTests/XhtmlDocViewTests.cs @@ -8,7 +8,9 @@ using System.Xml; using NUnit.Framework; using SIL.FieldWorks.Common.RootSites; +using SIL.FieldWorks.FdoUi; using SIL.LCModel.Core.Text; +using SIL.LCModel.Core.WritingSystems; using SIL.IO; using SIL.FieldWorks.Common.FwUtils; using SIL.LCModel; @@ -431,6 +433,228 @@ public void GetValidConfigurationForPublication_ConfigurationContainingPubIsPick } } + #region Reversal configuration switching (LT-22642) + + private const string ReversalConfigDirName = "XhtmlDocViewReversalConfigs"; + + /// + /// Writes a minimal reversal configuration file for . + /// + private static void WriteReversalConfiguration(string path, string label, string writingSystemId) + { + new DictionaryConfigurationModel + { + FilePath = path, + Label = label, + WritingSystem = writingSystemId, + Parts = new List(), + SharedItems = new List(), + Publications = new List() + }.Save(); + } + + /// + /// Makes an analysis writing system and returns + /// its reversal index, creating the index if the project does not have one yet. + /// + private IReversalIndex CreateReversalIndex(string writingSystemId) + { + CoreWritingSystemDefinition writingSystem; + Cache.ServiceLocator.WritingSystemManager.GetOrSet(writingSystemId, out writingSystem); + if (!Cache.ServiceLocator.WritingSystems.AnalysisWritingSystems.Contains(writingSystem)) + Cache.ServiceLocator.WritingSystems.AnalysisWritingSystems.Add(writingSystem); + return Cache.ServiceLocator.GetInstance().FindOrCreateIndexForWs(writingSystem.Handle); + } + + private static string CreateReversalConfigDirectory() + { + var directory = Path.Combine(Path.GetTempPath(), ReversalConfigDirName); + if (Directory.Exists(directory)) + Directory.Delete(directory, true); + Directory.CreateDirectory(directory); + return directory; + } + + [Test] + public void SyncReversalIndexGuidToConfiguration_SecondConfigurationOfSameWritingSystem_ReportsNoIndexChange() + { + var configDirectory = CreateReversalConfigDirectory(); + try + { + using (new UndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor, "doit", "undoit")) + using (var docView = new TestXhtmlDocView()) + { + CreateReversalIndex("en"); + var printConfig = Path.Combine(configDirectory, "en-print" + DictionaryConfigurationModel.FileExtension); + var webonaryConfig = Path.Combine(configDirectory, "en-webonary" + DictionaryConfigurationModel.FileExtension); + WriteReversalConfiguration(printConfig, "English-print", "en"); + WriteReversalConfiguration(webonaryConfig, "English-Webonary", "en"); + docView.SetPropertyTable(m_propertyTable); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", printConfig, false); + Assert.That(docView.SyncReversalIndexGuidToConfiguration(), Is.True, + "Choosing the first configuration should select its reversal index"); + var guidAfterFirstChoice = m_propertyTable.GetStringProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, string.Empty); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", webonaryConfig, false); + + // SUT + var indexChanged = docView.SyncReversalIndexGuidToConfiguration(); + + Assert.That(indexChanged, Is.False, + "Both configurations use 'en', so the index does not change, no clerk reload follows, and the view must refresh right away"); + Assert.That(m_propertyTable.GetStringProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, string.Empty), Is.EqualTo(guidAfterFirstChoice)); + } + } + finally + { + Directory.Delete(configDirectory, true); + } + } + + [Test] + public void SyncReversalIndexGuidToConfiguration_ConfigurationOfOtherWritingSystem_ReportsIndexChange() + { + var configDirectory = CreateReversalConfigDirectory(); + try + { + using (new UndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor, "doit", "undoit")) + using (var docView = new TestXhtmlDocView()) + { + CreateReversalIndex("en"); + var spanishIndex = CreateReversalIndex("es"); + var englishConfig = Path.Combine(configDirectory, "en" + DictionaryConfigurationModel.FileExtension); + var spanishConfig = Path.Combine(configDirectory, "es" + DictionaryConfigurationModel.FileExtension); + WriteReversalConfiguration(englishConfig, "English-print", "en"); + WriteReversalConfiguration(spanishConfig, "Spanish", "es"); + docView.SetPropertyTable(m_propertyTable); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", englishConfig, false); + docView.SyncReversalIndexGuidToConfiguration(); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", spanishConfig, false); + + // SUT + var indexChanged = docView.SyncReversalIndexGuidToConfiguration(); + + Assert.That(indexChanged, Is.True, "Switching writing systems selects a different reversal index"); + Assert.That(m_propertyTable.GetStringProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, string.Empty), + Is.EqualTo(spanishIndex.Guid.ToString())); + } + } + finally + { + Directory.Delete(configDirectory, true); + } + } + + [Test] + public void TryGetReplacementReversalConfiguration_ConfigurationMatchesSelectedIndex_KeepsUserChoice() + { + var configDirectory = CreateReversalConfigDirectory(); + try + { + using (new UndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor, "doit", "undoit")) + using (var docView = new TestXhtmlDocView()) + { + CreateReversalIndex("en"); + var spanishIndex = CreateReversalIndex("es"); + var englishConfig = Path.Combine(configDirectory, "en" + DictionaryConfigurationModel.FileExtension); + var spanishConfig = Path.Combine(configDirectory, "es" + DictionaryConfigurationModel.FileExtension); + WriteReversalConfiguration(englishConfig, "English-print", "en"); + WriteReversalConfiguration(spanishConfig, "Spanish", "es"); + docView.SetPropertyTable(m_propertyTable); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", spanishConfig, false); + m_propertyTable.SetProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, spanishIndex.Guid.ToString(), false); + + // SUT: the clerk still holds an English entry while the switch to Spanish + // is in flight + string replacement; + var needsRewrite = docView.TryGetReplacementReversalConfiguration("en", configDirectory, out replacement); + + Assert.That(needsRewrite, Is.False, + "The configuration matches the selected index, so the choice the user made must not be overwritten"); + Assert.That(replacement, Is.Null); + } + } + finally + { + Directory.Delete(configDirectory, true); + } + } + + [Test] + public void TryGetReplacementReversalConfiguration_ConfigurationOfOtherIndex_IsReplaced() + { + var configDirectory = CreateReversalConfigDirectory(); + try + { + using (new UndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor, "doit", "undoit")) + using (var docView = new TestXhtmlDocView()) + { + CreateReversalIndex("en"); + var spanishIndex = CreateReversalIndex("es"); + var englishConfig = Path.Combine(configDirectory, "en" + DictionaryConfigurationModel.FileExtension); + var spanishConfig = Path.Combine(configDirectory, "es" + DictionaryConfigurationModel.FileExtension); + WriteReversalConfiguration(englishConfig, "English-print", "en"); + WriteReversalConfiguration(spanishConfig, "Spanish", "es"); + docView.SetPropertyTable(m_propertyTable); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", englishConfig, false); + m_propertyTable.SetProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, spanishIndex.Guid.ToString(), false); + + // SUT: following a link into a Spanish entry must still pull + // the configuration across (FWR-1105) + string replacement; + var needsRewrite = docView.TryGetReplacementReversalConfiguration("es", configDirectory, out replacement); + + Assert.That(needsRewrite, Is.True); + Assert.That(replacement, Is.EqualTo(spanishConfig)); + } + } + finally + { + Directory.Delete(configDirectory, true); + } + } + + [Test] + public void SilentLayoutChange_SyncingGuidThenValidating_KeepsTheChosenConfiguration() + { + var configDirectory = CreateReversalConfigDirectory(); + try + { + using (new UndoableUnitOfWorkHelper(Cache.ActionHandlerAccessor, "doit", "undoit")) + using (var docView = new TestXhtmlDocView()) + { + CreateReversalIndex("en"); + var spanishIndex = CreateReversalIndex("es"); + var englishConfig = Path.Combine(configDirectory, "en" + DictionaryConfigurationModel.FileExtension); + var spanishConfig = Path.Combine(configDirectory, "es" + DictionaryConfigurationModel.FileExtension); + WriteReversalConfiguration(englishConfig, "English-print", "en"); + WriteReversalConfiguration(spanishConfig, "Spanish", "es"); + docView.SetPropertyTable(m_propertyTable); + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", englishConfig, false); + docView.SyncReversalIndexGuidToConfiguration(); + // The Configure dialog saves the Spanish choice with the broadcast + // suppressed, + // so the guid still names the English index at this point + m_propertyTable.SetProperty("ReversalIndexPublicationLayout", spanishConfig, false); + + // SUT: what RefreshAllContent does when the dialog closes + var indexChanged = docView.SyncReversalIndexGuidToConfiguration(); + + Assert.That(indexChanged, Is.True, "The dialog's choice must move the selection to the Spanish index"); + Assert.That(m_propertyTable.GetStringProperty(ReversalIndexEntryUi.ReversalIndexGuidProperty, string.Empty), + Is.EqualTo(spanishIndex.Guid.ToString())); + string replacement; + Assert.That(docView.TryGetReplacementReversalConfiguration("en", configDirectory, out replacement), Is.False, + "The English entry the clerk still holds must not pull the configuration back to English"); + } + } + finally + { + Directory.Delete(configDirectory, true); + } + } + + #endregion + private class TestXhtmlDocView : XhtmlDocView { internal void SetConfigObjectName(string name)