Skip to content

Add every new custom list to the Lists area, not just the last one #1075

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jasonleenaylor wants to merge 1 commit into
base: main
Choose a base branch
from
+183 −103
Open

Add every new custom list to the Lists area, not just the last one #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 59 additions & 87 deletions Src/LexText/LexTextDll/AreaListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,6 @@
protected Mediator m_mediator;
protected PropertyTable m_propertyTable;

/// <summary>
/// Keeps track of how many lists are loaded into List area
/// memory windowConfiguration XML (including Custom ones).
/// </summary>
private int m_ctotalLists;

/// <summary>
/// Keeps track of how many Custom lists are loaded into List area
/// memory windowConfiguration XML.
/// </summary>
private int m_ccustomLists;

#endregion

#region IDisposable & Co. implementation
Expand Down Expand Up @@ -152,8 +140,6 @@
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());
Expand Down Expand Up @@ -326,56 +312,45 @@
/// Lists area listener was getting too different from other areas, so I made a
/// separate version of FillList just for the Lists area.
/// </summary>
/// <param name="display"></param>
/// <returns></returns>
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<XmlNode>("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<XmlNode>("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<Guid>(newLists.Select(list => list.Guid));
var liveGuids = new HashSet<Guid>(ownerlessLists.Select(list => list.Guid));
var cache = m_propertyTable.GetValue<LcmCache>("cache");
var possRepo = cache.ServiceLocator.GetInstance<ICmPossibilityListRepository>();
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.

Check warning on line 348 in Src/LexText/LexTextDll/AreaListener.cs

View workflow job for this annotation

GitHub Actions / Build Debug and run tests

comment-hygiene (absence-narration) 

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;
}
Expand Down Expand Up @@ -534,41 +509,38 @@
return null;
}

private void UpdateWinConfig(bool fcustomChanged, List<ICmPossibilityList> 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)
/// <summary>
/// Gets the guids of the ownerless lists that the 'lists' area of the given window
/// configuration already has a tool for.
/// </summary>
private static HashSet<Guid> GetConfiguredOwnerlessListGuids(XmlNode windowConfig)
{
// Add 'customList' to windowConfig
AddListsToWindowConfig(new List<ICmPossibilityList> {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<Guid>();
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<ICmPossibilityList> customLists, XmlNode windowConfig)
/// <summary>
/// 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.
/// </summary>
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)
Expand Down
140 changes: 124 additions & 16 deletions Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// <remarks>
// </remarks>
using System.Collections.Generic;
using System.Linq;
using System.Xml;
using NUnit.Framework;
using SIL.LCModel;
Expand All @@ -29,6 +30,11 @@
{
#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";

/// <summary>
/// For testing.
/// </summary>
Expand Down Expand Up @@ -146,6 +152,29 @@
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<ICmPossibilityListFactory>()
.CreateUnowned(name, WritingSystemServices.kwsAnals);
}

/// <summary>
/// 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.
/// </summary>
private List<string> RefreshListsToolsDisplay()
{
var display = new UIListDisplayProperties(new XCore.List(null));
m_listener.OnDisplayListsToolsList(null, ref display);
return display.List.Cast<ListItem>().Select(item => item.value).ToList();
}

#endregion

///--------------------------------------------------------------------------------------
Expand All @@ -158,41 +187,120 @@
{
// 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));

Check warning on line 190 in Src/LexText/LexTextDll/LexTextDllTests/AreaListenerTests.cs

View workflow job for this annotation

GitHub Actions / Build Debug and run tests

comment-hygiene (comment-line-too-long) 

118 columns (max 98): 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<ICmPossibilityListFactory>().CreateUnowned(listName, ws);
var testList = CreateUnownedList("testList1");

// SUT
m_listener.AddListsToWindowConfig(new List<ICmPossibilityList> { testList }, node);

// 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.");
}

///--------------------------------------------------------------------------------------
/// <summary>
/// Every ownerless list created since the previous refresh must reach the Lists area, not
/// just one of them; a single session can create several.
/// </summary>
///--------------------------------------------------------------------------------------
[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.");
}

///--------------------------------------------------------------------------------------
/// <summary>
/// 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.
/// </summary>
///--------------------------------------------------------------------------------------
[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.");
}

///--------------------------------------------------------------------------------------
/// <summary>
/// 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.
/// </summary>
///--------------------------------------------------------------------------------------
[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.");
}

///--------------------------------------------------------------------------------------
/// <summary>
/// Publishing EventConstants.GetToolForList for a list that is wired into the window
Expand Down
Loading