From 7e704c3ba18ed904c795accb90541a8b6820f78b Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Mon, 17 Aug 2026 15:26:02 -0700 Subject: [PATCH] Add every new custom list to the Lists area, not just the last one The Lists area compared a cached count of ownerless lists against the current one and, when they differed, appended a single list picked by index. It now compares the full set of ownerless lists against the tool nodes already in the window configuration, matching on list guid, and adds only the ones that are missing. Tools whose list no longer exists are skipped when the display is filled, so a list deleted in one main window does not break the sidebar in another. Co-Authored-By: Claude Opus 5 --- Src/LexText/LexTextDll/AreaListener.cs | 146 +++++++----------- .../LexTextDllTests/AreaListenerTests.cs | 140 +++++++++++++++-- 2 files changed, 183 insertions(+), 103 deletions(-) diff --git a/Src/LexText/LexTextDll/AreaListener.cs b/Src/LexText/LexTextDll/AreaListener.cs index 1fd563f85f..dd2de3e5da 100644 --- a/Src/LexText/LexTextDll/AreaListener.cs +++ b/Src/LexText/LexTextDll/AreaListener.cs @@ -28,18 +28,6 @@ public class AreaListener : IxCoreColleague, IDisposable protected Mediator m_mediator; protected PropertyTable m_propertyTable; - /// - /// Keeps track of how many lists are loaded into List area - /// memory windowConfiguration XML (including Custom ones). - /// - private int m_ctotalLists; - - /// - /// Keeps track of how many Custom lists are loaded into List area - /// memory windowConfiguration XML. - /// - private int m_ccustomLists; - #endregion #region IDisposable & Co. implementation @@ -152,8 +140,6 @@ public void Init(Mediator mediator, PropertyTable propertyTable, XmlNode configu m_mediator = mediator; m_propertyTable = propertyTable; mediator.AddColleague(this); - m_ctotalLists = 0; - m_ccustomLists = 0; Subscriber.Subscribe(EventConstants.SetInitialContentObject, SetInitialContentObject, m_propertyTable.GetWindow()); Subscriber.Subscribe(EventConstants.SetToolFromName, SetToolFromName, m_propertyTable.GetWindow()); Subscriber.Subscribe(EventConstants.ReloadAreaTools, ReloadAreaTools, m_propertyTable.GetWindow()); @@ -326,56 +312,45 @@ public bool OnDisplayListsToolsList(object parameters, ref UIListDisplayProperti /// Lists area listener was getting too different from other areas, so I made a /// separate version of FillList just for the Lists area. /// - /// - /// private bool FillListAreaList(UIListDisplayProperties display) { - var customLists = GetListOfOwnerlessLists(); - var fcustomChanged = customLists.Count != m_ccustomLists; - // Since we're in the 'Lists' area, don't bother refreshing this - // list, unless the number of Custom lists has changed. This happens - // whenever someone adds one. - if (display.List.Count > 0 && !fcustomChanged) + var windowConfiguration = m_propertyTable.GetValue("WindowConfiguration"); + var ownerlessLists = GetListOfOwnerlessLists(); + var configuredGuids = GetConfiguredOwnerlessListGuids(windowConfiguration); + // Only additions are reconciled: deleting a Custom list rebuilds the window it was + // deleted in, discarding that window's copy of this configuration. + var newLists = ownerlessLists.Where(list => !configuredGuids.Contains(list.Guid)).ToList(); + // Since we're in the 'Lists' area, don't bother refreshing this list unless a Custom + // list has shown up that the window configuration doesn't know about yet. + if (display.List.Count > 0 && newLists.Count == 0) return true; - // We can get here in the following cases: - // Case 1: display.List is empty and m_ctotalLists == 0 - // Load 'windowConfiguration' with all Custom lists, - // Update both list counts, - // Load 'display' with ALL lists. - // Case 2: display.List is empty, but m_ctotalLists > 0 - // We MAY have a recent Custom list addition to add. - // If 'fcustomChanged', load the new Custom list into 'windowConfiguration', - // and update both list counts, - // Load 'display' with ALL lists. - // Case 3: display.List is loaded, but we have a recent Custom list addition to add. - // Load the new Custom list into 'windowConfiguration', - // Update both list counts, - // Only update 'display' with new Custom list. - // N.B. This may need changing if we allow the user to DELETE Custom lists someday. - var windowConfiguration = m_propertyTable.GetValue("WindowConfiguration"); - UpdateWinConfig(fcustomChanged, customLists, windowConfiguration); + if (newLists.Count > 0) + { + AddListsToWindowConfig(newLists, windowConfiguration); + UpdateMediatorConfig(windowConfiguration); + } + + var toolNodes = windowConfiguration.SelectNodes(XWindow.GetToolXPath("lists")); + if (toolNodes == null) + return true; - // Now update 'display' + // A display that already holds the other lists only wants the ones just added. + var wantNewListsOnly = display.List.Count > 0; + var newListGuids = new HashSet(newLists.Select(list => list.Guid)); + var liveGuids = new HashSet(ownerlessLists.Select(list => list.Guid)); var cache = m_propertyTable.GetValue("cache"); var possRepo = cache.ServiceLocator.GetInstance(); - if (display.List.Count > 0) - { - var node = windowConfiguration.SelectSingleNode(GetListToolsXPath()).LastChild; - if (node != null) - AddToolNodeToDisplay(possRepo, cache, display, node); - } - else + foreach (XmlNode toolNode in toolNodes) { - var nodes = windowConfiguration.SelectNodes(XWindow.GetToolXPath("lists")); - if (nodes == null) - { - return true; - } - foreach (XmlNode node in nodes) - { - AddToolNodeToDisplay(possRepo, cache, display, node); - } + var listGuid = GetOwnerlessListGuidFromToolNode(toolNode); + // A Custom list deleted in another main window leaves its tool in this window's + // configuration, where the guid no longer resolves to a list. + if (listGuid != Guid.Empty && !liveGuids.Contains(listGuid)) + continue; + if (wantNewListsOnly && !newListGuids.Contains(listGuid)) + continue; + AddToolNodeToDisplay(possRepo, cache, display, toolNode); } return true; } @@ -534,41 +509,38 @@ private static XmlNode FindClerkNode(XmlNode toolNode, string clerkId) return null; } - private void UpdateWinConfig(bool fcustomChanged, List customLists, XmlNode windowConfig) - { - // See caller FillListAreaList() for description of Case 1-3 - if (m_ctotalLists == 0) // Case 1 - LoadAllCustomLists(customLists, windowConfig); - else // Case 2 and 3 - if (fcustomChanged) // Case 3 is automatically true - AddACustomList(customLists[customLists.Count - 1], windowConfig); - } - - private void AddACustomList(ICmPossibilityList customList, XmlNode windowConfig) + /// + /// Gets the guids of the ownerless lists that the 'lists' area of the given window + /// configuration already has a tool for. + /// + private static HashSet GetConfiguredOwnerlessListGuids(XmlNode windowConfig) { - // Add 'customList' to windowConfig - AddListsToWindowConfig(new List {customList}, windowConfig); - - // We have to update this because other things besides 'tools' need to get set. - UpdateMediatorConfig(windowConfig); - - m_ccustomLists++; - m_ctotalLists++; + var configuredGuids = new HashSet(); + var toolNodes = windowConfig.SelectNodes(XWindow.GetToolXPath("lists")); + if (toolNodes == null) + return configuredGuids; + foreach (XmlNode toolNode in toolNodes) + { + var listGuid = GetOwnerlessListGuidFromToolNode(toolNode); + if (listGuid != Guid.Empty) + configuredGuids.Add(listGuid); + } + return configuredGuids; } - private void LoadAllCustomLists(List customLists, XmlNode windowConfig) + /// + /// Gets the guid of the ownerless list that the given tool edits, or Guid.Empty if the + /// tool edits an owned list or isn't wired to a list at all. + /// + private static Guid GetOwnerlessListGuidFromToolNode(XmlNode toolNode) { - - AddListsToWindowConfig(customLists, windowConfig); - - // We have to update this because other things besides 'tools' need to get set. - UpdateMediatorConfig(windowConfig); - - var nodes = windowConfig.SelectNodes(XWindow.GetToolXPath("lists")); - if (nodes != null) - m_ctotalLists = nodes.Count; - m_ccustomLists = customLists.Count; - m_ctotalLists += m_ccustomLists; + var recordListNode = GetClerkRecordListNodeFromToolNode(toolNode); + if (recordListNode == null || XmlUtils.GetAttributeValue(recordListNode, "owner") != "unowned") + return Guid.Empty; + // An unowned recordList carries the list's guid as its property. + Guid listGuid; + return Guid.TryParse(XmlUtils.GetAttributeValue(recordListNode, "property"), out listGuid) + ? listGuid : Guid.Empty; } private void UpdateMediatorConfig(XmlNode windowConfig) diff --git a/Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs b/Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs index 484649e487..6a6da8d10a 100644 --- a/Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs +++ b/Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs @@ -8,6 +8,7 @@ // // using System.Collections.Generic; +using System.Linq; using System.Xml; using NUnit.Framework; using SIL.LCModel; @@ -29,6 +30,11 @@ public class AreaListenerTests : MemoryOnlyBackendProviderRestoredForEachTestTes { #region Member Data + private const string ClerkXPath = "//item[@value='lists']/parameters/clerks/clerk"; + private const string CommandXPath = "//commands/command"; + private const string ContextMenuXPath = "//contextMenus/menu"; + private const string ToolXPath = "//item[@value='lists']/parameters/tools/tool"; + /// /// For testing. /// @@ -146,6 +152,29 @@ private static XmlNode SetupWindowConfigWithListTool(string clerkId, string tool return fakeWindowConfig.DocumentElement; } + private void UseTestWindowConfiguration() + { + m_propertyTable.SetProperty("WindowConfiguration", m_testWindowConfig, true); + m_propertyTable.SetPropertyPersistence("WindowConfiguration", false); + } + + private ICmPossibilityList CreateUnownedList(string name) + { + return Cache.ServiceLocator.GetInstance() + .CreateUnowned(name, WritingSystemServices.kwsAnals); + } + + /// + /// Refreshes the Lists area sidebar the way xCore does and returns the tool name of every + /// entry the refresh left in the display, in display order. + /// + private List RefreshListsToolsDisplay() + { + var display = new UIListDisplayProperties(new XCore.List(null)); + m_listener.OnDisplayListsToolsList(null, ref display); + return display.List.Cast().Select(item => item.value).ToList(); + } + #endregion ///-------------------------------------------------------------------------------------- @@ -158,24 +187,18 @@ public void AddListToXmlConfig() { // Setup var node = m_testWindowConfig; - const string clerkXPath = "//item[@value='lists']/parameters/clerks/clerk"; - const string commandXPath = "//commands/command"; - const string contextXPath = "//contextMenus/menu"; - const string toolXPath = "//item[@value='lists']/parameters/tools/tool"; - //var fakeUIDisplay = new UIListDisplayProperties(new XCore.List(node.SelectSingleNode(toolXPath), null)); + //var fakeUIDisplay = new UIListDisplayProperties(new XCore.List(node.SelectSingleNode(ToolXPath), null)); //var cdispNodesBefore = fakeUIDisplay.List.Count; - var contextNodes = node.SelectNodes(contextXPath); + var contextNodes = node.SelectNodes(ContextMenuXPath); var ccontextNodesBefore = contextNodes == null ? 0 : contextNodes.Count; - var commandNodes = node.SelectNodes(commandXPath); + var commandNodes = node.SelectNodes(CommandXPath); var ccommandNodesBefore = commandNodes == null ? 0 : commandNodes.Count; - var clerkNodes = node.SelectNodes(clerkXPath); + var clerkNodes = node.SelectNodes(ClerkXPath); var cclerkNodesBefore = clerkNodes == null ? 0 : clerkNodes.Count; - var toolNodes = node.SelectNodes(toolXPath); + var toolNodes = node.SelectNodes(ToolXPath); var ctoolNodesBefore = toolNodes == null ? 0 : toolNodes.Count; - const string listName = "testList1"; - var ws = WritingSystemServices.kwsAnals; - var testList = Cache.ServiceLocator.GetInstance().CreateUnowned(listName, ws); + var testList = CreateUnownedList("testList1"); // SUT m_listener.AddListsToWindowConfig(new List { testList }, node); @@ -183,16 +206,101 @@ public void AddListToXmlConfig() // Verify // The above routine no longer handles display nodes //Assert.That(fakeUIDisplay.List.Count, Is.EqualTo(cdispNodesBefore + 1), "Didn't add a display node."); - var ctoolNodesAfter = node.SelectNodes(toolXPath).Count; + var ctoolNodesAfter = node.SelectNodes(ToolXPath).Count; Assert.That(ctoolNodesAfter, Is.EqualTo(ctoolNodesBefore + 1), "Didn't add a tool node."); - var cclerkNodesAfter = node.SelectNodes(clerkXPath).Count; + var cclerkNodesAfter = node.SelectNodes(ClerkXPath).Count; Assert.That(cclerkNodesAfter, Is.EqualTo(cclerkNodesBefore + 1), "Didn't add a clerk node."); - var ccommandNodesAfter = node.SelectNodes(commandXPath).Count; + var ccommandNodesAfter = node.SelectNodes(CommandXPath).Count; Assert.That(ccommandNodesAfter, Is.EqualTo(ccommandNodesBefore + 1), "Didn't add a command node."); - var ccontextNodesAfter = node.SelectNodes(contextXPath).Count; + var ccontextNodesAfter = node.SelectNodes(ContextMenuXPath).Count; Assert.That(ccontextNodesAfter, Is.EqualTo(ccontextNodesBefore + 1), "Didn't add a context menu node."); } + ///-------------------------------------------------------------------------------------- + /// + /// Every ownerless list created since the previous refresh must reach the Lists area, not + /// just one of them; a single session can create several. + /// + ///-------------------------------------------------------------------------------------- + [Test] + public void FillListAreaList_AddsEveryListCreatedSinceLastRefresh() + { + UseTestWindowConfiguration(); + CreateUnownedList("Original List"); + var afterFirstRefresh = RefreshListsToolsDisplay(); + Assert.That(afterFirstRefresh, Does.Contain("OriginalListEdit")); + + CreateUnownedList("Second List"); + CreateUnownedList("Third List"); + + // SUT + var afterSecondRefresh = RefreshListsToolsDisplay(); + + Assert.That(afterSecondRefresh.Except(afterFirstRefresh), + Is.EquivalentTo(new[] { "SecondListEdit", "ThirdListEdit" }), "Missed a new list."); + Assert.That(afterSecondRefresh, Is.Unique, "Showed a list more than once."); + } + + ///-------------------------------------------------------------------------------------- + /// + /// A refresh that finds no new ownerless list must leave the window configuration and the + /// mediator command set alone. Adding a list a second time appends duplicate tool, clerk + /// and context menu nodes, and throws on its already registered command id. + /// + ///-------------------------------------------------------------------------------------- + [Test] + public void FillListAreaList_RepeatedRefresh_DoesNotDuplicateConfigNodes() + { + UseTestWindowConfiguration(); + CreateUnownedList("Original List"); + RefreshListsToolsDisplay(); + CreateUnownedList("Second List"); + CreateUnownedList("Third List"); + var afterListsAdded = RefreshListsToolsDisplay(); + var ctoolNodes = m_testWindowConfig.SelectNodes(ToolXPath).Count; + var cclerkNodes = m_testWindowConfig.SelectNodes(ClerkXPath).Count; + var ccommandNodes = m_testWindowConfig.SelectNodes(CommandXPath).Count; + var ccontextNodes = m_testWindowConfig.SelectNodes(ContextMenuXPath).Count; + + // SUT: a refresh with nothing new to pick up + var afterIdleRefresh = RefreshListsToolsDisplay(); + + Assert.That(afterIdleRefresh, Is.EquivalentTo(afterListsAdded)); + Assert.That(m_testWindowConfig.SelectNodes(ToolXPath).Count, + Is.EqualTo(ctoolNodes), "Duplicated a tool node."); + Assert.That(m_testWindowConfig.SelectNodes(ClerkXPath).Count, + Is.EqualTo(cclerkNodes), "Duplicated a clerk node."); + Assert.That(m_testWindowConfig.SelectNodes(CommandXPath).Count, + Is.EqualTo(ccommandNodes), "Duplicated a command node."); + Assert.That(m_testWindowConfig.SelectNodes(ContextMenuXPath).Count, + Is.EqualTo(ccontextNodes), "Duplicated a context menu node."); + } + + ///-------------------------------------------------------------------------------------- + /// + /// Deleting a Custom list rebuilds only the window it was deleted in, so any other main + /// window keeps a tool whose list is gone. Refreshing there must drop that tool rather + /// than fail to resolve its guid. + /// + ///-------------------------------------------------------------------------------------- + [Test] + public void FillListAreaList_ListDeletedInAnotherWindow_DropsItsTool() + { + UseTestWindowConfiguration(); + var doomedList = CreateUnownedList("Doomed List"); + CreateUnownedList("Surviving List"); + var afterFirstRefresh = RefreshListsToolsDisplay(); + Assert.That(afterFirstRefresh, Does.Contain("DoomedListEdit")); + + doomedList.Delete(); + + // SUT + var afterDeletion = RefreshListsToolsDisplay(); + + Assert.That(afterDeletion, Does.Not.Contain("DoomedListEdit")); + Assert.That(afterDeletion, Does.Contain("SurvivingListEdit"), "Dropped the wrong tool."); + } + ///-------------------------------------------------------------------------------------- /// /// Publishing EventConstants.GetToolForList for a list that is wired into the window