From 610e43e14bef2bad6d306c6ada2b7ff42e5801de Mon Sep 17 00:00:00 2001 From: mark-sil <83427558+mark-sil@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:28:08 -0400 Subject: [PATCH] Stop NavBarAdapter's own selections from re-setting properties NavBarAdapter now reflects an areaChoice or ToolForAreaNamed_ change in the side pane without the TabClicked and ItemClicked events that the selection raises running the tab's or item's choice. Until now those events, raised by SidePane.SelectTab and SelectItem exactly as a user click raises them, re-ran ChoiceGroup's property sets from inside the OnPropertyChanged handler, including a set for the first item of a tab on its first visit, which the remembered tool's set then overwrote. The sets the re-run made, which no longer happen: * areaChoiceParameters * areaChoice * currentContentControlParameters * currentContentControl Every value was the one already in place, except areaChoiceParameters, which the first programmatic SelectTab of a session created with no listener alive. A user click on a tab or item still runs its choice. Co-Authored-By: Claude Fable 5.1 --- Src/XCore/FlexUIAdapter/NavBarAdapter.cs | 34 +++- .../SidebarAdapterSelectionTests.cs | 189 ++++++++++++++++++ 2 files changed, 218 insertions(+), 5 deletions(-) create mode 100644 Src/XCore/xCoreTests/SidebarAdapterSelectionTests.cs diff --git a/Src/XCore/FlexUIAdapter/NavBarAdapter.cs b/Src/XCore/FlexUIAdapter/NavBarAdapter.cs index 17e24df45c..8fab2c4bc0 100644 --- a/Src/XCore/FlexUIAdapter/NavBarAdapter.cs +++ b/Src/XCore/FlexUIAdapter/NavBarAdapter.cs @@ -26,6 +26,7 @@ public class SidebarAdapter : AdapterBase, IxCoreColleague, IDisposable private ChoiceGroupCollection m_choiceGroupCollectionCache; // cache of what was received by CreateUIForChoiceGroupCollection() private List m_choiceGroupCache = new List(); // cache of what was received by CreateUIForChoiceGroup() private string areasLabel; // label of the special case ChoiceGroup for Areas + private bool m_selectingInSidePane; // true while the adapter's own SelectTab/SelectItem runs #region Disposable stuff #if DEBUG @@ -141,10 +142,13 @@ public void Init(Mediator mediator, PropertyTable propertyTable, XmlNode configu } /// - /// Handle when an item on the sidepane is clicked. + /// Handle when an item on the sidepane is clicked. A selection the adapter itself makes + /// while reflecting a property change raises the same event and is ignored here. /// private void SidePaneItemClickedHandler(Item itemClicked) { + if (m_selectingInSidePane) + return; var choice = itemClicked.Tag as ChoiceBase; Debug.Assert(choice != null, "SidePaneItemClickedHandler received an Item with an invalid Tag."); if (choice != null) @@ -152,16 +156,36 @@ private void SidePaneItemClickedHandler(Item itemClicked) } /// - /// Handle when a tab on the sidepane is clicked. + /// Handle when a tab on the sidepane is clicked. A selection the adapter itself makes + /// while reflecting a property change raises the same event and is ignored here. /// private void SidePaneTabClickedHandler(Tab tabClicked) { + if (m_selectingInSidePane) + return; var choice = tabClicked.Tag as ChoiceBase; Debug.Assert(choice != null, "SidePaneTabClickedHandler received a Tab with an invalid Tag."); if (choice != null) choice.OnClick(this, null); } + /// + /// Runs one of the side pane's selection calls with the click handlers silenced, so + /// that showing a property's value does not set that property again. + /// + private bool SelectInSidePane(Func select) + { + m_selectingInSidePane = true; + try + { + return select(); + } + finally + { + m_selectingInSidePane = false; + } + } + public override void PersistLayout() { // use MyPanel @@ -555,12 +579,12 @@ public void OnPropertyChanged(string propertyName) // overwriting the property table value. See FWR-2004. string propToolForArea = "ToolForAreaNamed_" + areaName; string toolForArea = m_propertyTable.GetStringProperty(propToolForArea, null); - m_sidepane.SelectTab(tab); + SelectInSidePane(() => m_sidepane.SelectTab(tab)); // FWR-2895 Deleting a Custom list could result in needing to // update the PropertyTable. if (!String.IsNullOrEmpty(toolForArea) && !toolForArea.StartsWith("?")) { - var fsuccess = m_sidepane.SelectItem(tab, toolForArea); + var fsuccess = SelectInSidePane(() => m_sidepane.SelectItem(tab, toolForArea)); if (!fsuccess) { m_propertyTable.SetProperty(propToolForArea, null, true); @@ -605,7 +629,7 @@ private void SetToolForCurrentArea(string tool) // calling the SelectItem if it is already the CurrentItem. if(m_sidepane.CurrentItem != item) { - m_sidepane.SelectItem(tab, item.Name); + SelectInSidePane(() => m_sidepane.SelectItem(tab, item.Name)); } break; } diff --git a/Src/XCore/xCoreTests/SidebarAdapterSelectionTests.cs b/Src/XCore/xCoreTests/SidebarAdapterSelectionTests.cs new file mode 100644 index 0000000000..67f0a76075 --- /dev/null +++ b/Src/XCore/xCoreTests/SidebarAdapterSelectionTests.cs @@ -0,0 +1,189 @@ +// 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.Linq; +using System.Threading; +using System.Windows.Forms; +using System.Xml; +using NUnit.Framework; +using SIL.SilSidePane; + +namespace XCore +{ + /// + /// How reflects areaChoice and ToolForAreaNamed_ changes in + /// the side pane without setting those properties again. + /// + [TestFixture] + [Apartment(ApartmentState.STA)] + public class SidebarAdapterSelectionTests + { + /// + /// Two areas with two tools each, in the shape of the sidebar section of Main.xml. + /// + private const string SidebarXml = + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + "" + + ""; + + private Form m_window; + private Mediator m_mediator; + private PropertyTable m_propertyTable; + private ImageCollection m_smallImages; + private ImageCollection m_largeImages; + private SidebarAdapter m_adapter; + private Control m_panel; + private ChoiceGroupCollection m_groups; + + [SetUp] + public void SetUp() + { + m_window = new Form(); + m_mediator = new Mediator(); + m_propertyTable = new PropertyTable(m_mediator); + m_smallImages = new ImageCollection(false); + m_largeImages = new ImageCollection(true); + m_adapter = new SidebarAdapter(); + m_panel = m_adapter.Init(m_window, m_smallImages, m_largeImages, m_mediator, m_propertyTable); + m_window.Controls.Add(m_panel); + + var doc = new XmlDocument(); + doc.LoadXml(SidebarXml); + m_groups = new ChoiceGroupCollection(m_mediator, m_propertyTable, m_adapter, + doc.DocumentElement.SelectSingleNode("sidebar")); + m_groups.Init(); + m_adapter.FinishInit(); + CreateHandles(m_window); + } + + [TearDown] + public void TearDown() + { + m_adapter.Dispose(); + m_window.Dispose(); + m_smallImages.Dispose(); + m_largeImages.Dispose(); + m_propertyTable.Dispose(); + m_mediator.Dispose(); + } + + /// + /// The side pane's list views raise their selection events only once they have a + /// window handle, which a hidden child of an unshown form never gets on its own. + /// + private static void CreateHandles(Control control) + { + var unused = control.Handle; + foreach (Control child in control.Controls) + CreateHandles(child); + } + + private static IEnumerable AllControls(Control root) + { + yield return root; + foreach (Control child in root.Controls) + foreach (var descendant in AllControls(child)) + yield return descendant; + } + + /// + /// Every side-pane item widget, across all the tabs' item lists. The form is never + /// shown, so a list's Visible flag cannot tell which tab is current. + /// + private IEnumerable AllItemWidgets() + { + return AllControls(m_panel).OfType().SelectMany(list => list.Items.Cast()); + } + + /// + /// The tool selected in the item list that holds the named tool, or null when none is. + /// Each tab's list keeps its own selection, so the list is named by one of its tools. + /// + private string SelectedToolInTheListHolding(string tool) + { + var list = AllItemWidgets().First(widget => ((Item)widget.Tag).Name == tool).ListView; + if (list.SelectedItems.Count == 0) + return null; + return ((ListPropertyChoice)((Item)list.SelectedItems[0].Tag).Tag).Value; + } + + private void ShowArea(string area, string rememberedTool) + { + m_propertyTable.SetProperty("areaChoice", area, false); + m_propertyTable.SetProperty("ToolForAreaNamed_" + area, rememberedTool, false); + m_adapter.OnPropertyChanged("areaChoice"); + } + + [Test] + public void AreaChoiceChange_SelectsTheRememberedToolWithoutSettingAnyProperty() + { + ShowArea("grammar", "posBrowse"); + + Assert.That(SelectedToolInTheListHolding("posBrowse"), Is.EqualTo("posBrowse")); + Assert.That(m_mediator.JobItems, Is.EqualTo(0), "the selection must not set a property"); + Assert.That(m_propertyTable.PropertyExists("currentContentControl"), Is.False); + Assert.That(m_propertyTable.PropertyExists("areaChoiceParameters"), Is.False); + } + + [Test] + public void ReturnToAnArea_SelectsTheToolRememberedSinceTheLastVisit() + { + ShowArea("lexicon", "lexiconEdit"); + ShowArea("grammar", "posEdit"); + + ShowArea("lexicon", "lexiconBrowse"); + + Assert.That(SelectedToolInTheListHolding("lexiconBrowse"), Is.EqualTo("lexiconBrowse")); + Assert.That(m_mediator.JobItems, Is.EqualTo(0), "the selection must not set a property"); + Assert.That(m_propertyTable.PropertyExists("currentContentControl"), Is.False); + } + + [Test] + public void ToolForAreaChange_SelectsThatToolWithoutSettingAnyProperty() + { + ShowArea("lexicon", "lexiconEdit"); + m_propertyTable.SetProperty("ToolForAreaNamed_lexicon", "lexiconBrowse", false); + + m_adapter.OnPropertyChanged("ToolForAreaNamed_lexicon"); + + Assert.That(SelectedToolInTheListHolding("lexiconBrowse"), Is.EqualTo("lexiconBrowse")); + Assert.That(m_mediator.JobItems, Is.EqualTo(0), "the selection must not set a property"); + Assert.That(m_propertyTable.PropertyExists("currentContentControl"), Is.False); + } + + [Test] + public void ItemSelectedOutsideTheAdapter_StillSetsTheProperty() + { + ShowArea("lexicon", "lexiconEdit"); + + AllItemWidgets().First(widget => ((Item)widget.Tag).Name == "lexiconBrowse").Selected = true; + + Assert.That(m_propertyTable.GetStringProperty("currentContentControl", null), Is.EqualTo("lexiconBrowse")); + Assert.That(m_mediator.JobItems, Is.GreaterThan(0), "a selection the adapter did not make is a click"); + } + } +}