From a85ed5ad29a6d3e2a6d45df401e35bcb12408fc5 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Mon, 30 Mar 2026 14:36:39 -0700 Subject: [PATCH 1/3] Fix LT-22316 regression: persist hidden column labels The LT-22265 fix auto-added common columns missing from saved settings, but couldn't distinguish "new column" from "deliberately removed column" since only active columns were persisted. This caused Lexeme Form and other common columns to reappear after navigation. - Bump kBrowseViewVersion to 20 - Save hidden column labels as elements sorted for S/R - On load, skip auto-adding columns in the hidden list - Bootstrap: when no hidden tracking exists (pre-upgrade), skip all auto-adds to prevent removed columns from reappearing Co-Authored-By: Claude Opus 4.6 --- Src/Common/Controls/XMLViews/BrowseViewer.cs | 21 +++++++++- .../XMLViewsTests/TestXmlBrowseView.cs | 41 +++++++++++++++++++ .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 31 +++++++++++++- 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/Src/Common/Controls/XMLViews/BrowseViewer.cs b/Src/Common/Controls/XMLViews/BrowseViewer.cs index ae13740886..e6d149a1d7 100644 --- a/Src/Common/Controls/XMLViews/BrowseViewer.cs +++ b/Src/Common/Controls/XMLViews/BrowseViewer.cs @@ -23,6 +23,7 @@ using System.ComponentModel; using System.Diagnostics; using System.Drawing; +using System.Security; using System.Text; using System.Windows.Forms; using System.Xml; @@ -2968,7 +2969,7 @@ protected void InsertColumn(XmlNode colSpec, int position) // Note: often we also want to update LayoutCache.LayoutVersionNumber. // (last updated by Ariel Rorabaugh, Oct 28, 2025, for adding PictureLicense column to Browse view) - internal const int kBrowseViewVersion = 19; + internal const int kBrowseViewVersion = 20; /// /// Column has been added or removed, update all child windows. @@ -3042,6 +3043,24 @@ protected void UpdateColumnList() colList.Append(node.OuterXml); } } + // Save labels of hidden columns so we can distinguish "removed by user" from "new column" on load. + // Use SortedSet with Ordinal comparison for deterministic output (stable for Send/Receive). + var activeLabels = new HashSet(); + foreach (XmlNode node in ColumnSpecs) + { + var label = XmlUtils.GetOptionalAttributeValue(node, "label", ""); + if (!string.IsNullOrEmpty(label)) + activeLabels.Add(label); + } + var hiddenLabels = new SortedSet(StringComparer.Ordinal); + foreach (XmlNode node in m_xbv.Vc.PossibleColumnSpecs) + { + var label = XmlUtils.GetOptionalAttributeValue(node, "label", ""); + if (!string.IsNullOrEmpty(label) && !activeLabels.Contains(label)) + hiddenLabels.Add(label); + } + foreach (var label in hiddenLabels) + colList.Append(""); colList.Append(""); m_propertyTable.SetProperty(m_xbv.Vc.ColListId, colList.ToString(), PropertyTable.SettingsGroup.LocalSettings, true); } diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs index 37e441ca49..fb92cfcb75 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs @@ -133,6 +133,47 @@ public void MigrateBrowseColumns() } } + [Test] + public void MigrateBrowseColumns_Version19ToVersion20() + { + var input = + "" + + "" + + "" + + ""; + using (var mediator = new Mediator()) + using (var propertyTable = new PropertyTable(mediator)) + { + var output = XmlBrowseViewBaseVc.GetSavedColumns(input, mediator, propertyTable, "myKey"); + Assert.That(XmlUtils.GetOptionalAttributeValue(output.DocumentElement, "version"), Is.EqualTo(BrowseViewer.kBrowseViewVersion.ToString())); + Assert.That(XmlUtils.GetOptionalAttributeValue(output.DocumentElement, "version"), Is.EqualTo("20")); + // Columns should be preserved as-is + var headwordNode = output.SelectSingleNode("//column[@label='Headword']"); + Assert.That(headwordNode, Is.Not.Null); + Assert.That(XmlUtils.GetOptionalAttributeValue(headwordNode, "layout"), Is.EqualTo("EntryHeadwordForFindEntry")); + } + } + + [Test] + public void MigrateBrowseColumns_HiddenElementsPreservedThroughMigration() + { + // Version 19 with hidden elements (simulating a future scenario where hidden elements exist) + var input = + "" + + "" + + "" + + ""; + using (var mediator = new Mediator()) + using (var propertyTable = new PropertyTable(mediator)) + { + var output = XmlBrowseViewBaseVc.GetSavedColumns(input, mediator, propertyTable, "myKey"); + Assert.That(XmlUtils.GetOptionalAttributeValue(output.DocumentElement, "version"), Is.EqualTo("20")); + // Hidden elements should be preserved + var hiddenNode = output.SelectSingleNode("//hidden[@label='Lexeme Form']"); + Assert.That(hiddenNode, Is.Not.Null, "Hidden column labels should be preserved through migration"); + } + } + void VerifyColumn(XmlNode output, string layoutName, string attrName, string attrVal) { var node = output.SelectSingleNode("//column[@layout='" + layoutName + "']"); diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 2c18ba0d38..ebd2d06739 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -229,10 +229,34 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) ColumnSpecs.Add(node); } + // Collect labels of columns the user deliberately removed (hidden). + var hiddenNodes = doc.DocumentElement.SelectNodes("//hidden"); + var hiddenLabels = new HashSet(); + bool hasHiddenTracking = hiddenNodes.Count > 0; + if (hasHiddenTracking) + { + foreach (XmlNode hidden in hiddenNodes) + { + var label = XmlUtils.GetOptionalAttributeValue(hidden, "label", ""); + if (!string.IsNullOrEmpty(label)) + hiddenLabels.Add(label); + } + } + foreach (var node in newPossibleColumns) { - // add any possible columns that were not in the saved list and are common (and not custom) - if (!IsCustomField(node, out _) && XmlUtils.GetOptionalAttributeValue(node, "common", "false") == "true") + if (!hasHiddenTracking) + { + // Bootstrap: no hidden tracking yet (pre-upgrade save). + // Don't auto-add anything — all missing columns are presumed + // deliberately removed. They'll be properly tracked on next save. + continue; + } + // Add common non-custom columns that were not in the saved list and not deliberately hidden. + var label = XmlUtils.GetOptionalAttributeValue(node, "label", ""); + if (!hiddenLabels.Contains(label) + && !IsCustomField(node, out _) + && XmlUtils.GetOptionalAttributeValue(node, "common", "false") == "true") { ColumnSpecs.Add(node); } @@ -299,6 +323,9 @@ internal static XmlDocument GetSavedColumns(string savedCols, Mediator mediator, case 18: savedCols = FixVersion19Columns(savedCols); savedCols = savedCols.Replace("root version=\"18\"", "root version=\"19\""); + goto case 19; + case 19: + savedCols = savedCols.Replace("root version=\"19\"", "root version=\"20\""); propertyTable.SetProperty(colListId, savedCols, true); doc.LoadXml(savedCols); break; From 84978cf645e2cf5e135ad4f38c278602d3b3a542 Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Wed, 1 Apr 2026 16:25:54 -0700 Subject: [PATCH 2/3] Fix Phonological Features columns lost after MasterRefresh (LT-22447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IsValidColumnSpec checked GetPartFromParentNode before checking PossibleColumnSpecs, causing it to reject valid columns for views like Phonological Features where FsFeatDefn's class hierarchy (FsFeatDefn → CmObject) has no matching parts for Name/Abbreviation/Description. Reorder the checks so PossibleColumnSpecs label match takes priority, with GetPartFromParentNode as a fallback for generated columns. Co-Authored-By: Claude Opus 4.6 --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index ebd2d06739..2da482e1c3 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -640,23 +640,27 @@ internal int ListItemsClass /// internal bool IsValidColumnSpec(XmlNode colSpec) { - if (GetPartFromParentNode(colSpec, ListItemsClass) == null) - { - return false; - } // If it is a Custom Field, check that it is valid and has the correct label. if (IsCustomField(colSpec, out var isValid)) { return isValid; } - // In the simple case, `node`s label should match a label in PossibleColumnSpecs. There may be more complicated cases. - // ENHANCE (Hasso) 2025.11: 'layout' (mandatory?) and 'field' (optional) would be better attributes to match, but that would require more test setup. + // If the column matches a known PossibleColumnSpec by label, it is valid. + // Check this before GetPartFromParentNode because some views (e.g. Phonological Features + // with FsFeatDefn) have valid columns whose class hierarchy has no matching parts in + // the part inventory; GetPartFromParentNode would incorrectly reject those columns. var label = XmlUtils.GetLocalizedAttributeValue(colSpec, "label", null) ?? XmlUtils.GetMandatoryAttributeValue(colSpec, "label"); var originalLabel = XmlUtils.GetLocalizedAttributeValue(colSpec, "originalLabel", null) ?? XmlUtils.GetAttributeValue(colSpec, "originalLabel"); - return XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", label) != null || - XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", originalLabel) != null; + if (XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", label) != null || + XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", originalLabel) != null) + { + return true; + } + // For columns not in PossibleColumnSpecs (e.g. generated columns), fall back to + // checking whether the part/layout inventory can resolve the column. + return GetPartFromParentNode(colSpec, ListItemsClass) != null; } /// From 56e8120fbc2f736b6c106184fc82f610a268263a Mon Sep 17 00:00:00 2001 From: Jason Naylor Date: Thu, 2 Apr 2026 07:30:34 -0700 Subject: [PATCH 3/3] Write sentinel when no columns are hidden (LT-22447) When all columns are visible, UpdateColumnList wrote zero elements. On reload, hasHiddenTracking was false (hiddenNodes.Count==0), so the bootstrap path skipped auto-adding new common columns. Write a sentinel (no label attribute) so the load side correctly detects that hidden tracking is active. Also guard the GetPartFromParentNode fallback in IsValidColumnSpec to require a layout attribute, since columns without one trivially return non-null. Co-Authored-By: Claude Opus 4.6 --- Src/Common/Controls/XMLViews/BrowseViewer.cs | 13 ++++++- .../XMLViewsTests/TestXmlBrowseView.cs | 38 +++++++++++++++++++ .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 8 +++- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/Src/Common/Controls/XMLViews/BrowseViewer.cs b/Src/Common/Controls/XMLViews/BrowseViewer.cs index e6d149a1d7..cae0c518cc 100644 --- a/Src/Common/Controls/XMLViews/BrowseViewer.cs +++ b/Src/Common/Controls/XMLViews/BrowseViewer.cs @@ -3059,8 +3059,17 @@ protected void UpdateColumnList() if (!string.IsNullOrEmpty(label) && !activeLabels.Contains(label)) hiddenLabels.Add(label); } - foreach (var label in hiddenLabels) - colList.Append(""); + if (hiddenLabels.Count == 0) + { + // Write a sentinel so the load side knows hidden tracking is active + // (distinguishes "all columns visible" from "pre-upgrade save with no tracking"). + colList.Append(""); + } + else + { + foreach (var label in hiddenLabels) + colList.Append(""); + } colList.Append(""); m_propertyTable.SetProperty(m_xbv.Vc.ColListId, colList.ToString(), PropertyTable.SettingsGroup.LocalSettings, true); } diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs index fb92cfcb75..25be18618f 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/TestXmlBrowseView.cs @@ -3,6 +3,7 @@ // (http://www.gnu.org/licenses/lgpl-2.1.html) using System; +using System.Collections.Generic; using System.Xml; using NUnit.Framework; using SIL.FieldWorks.Common.Controls; @@ -174,6 +175,43 @@ public void MigrateBrowseColumns_HiddenElementsPreservedThroughMigration() } } + [Test] + public void HiddenSentinel_AllColumnsVisible_AllowsAutoAddOfNewCommonColumns() + { + // Simulate a version-20 save where all columns are visible: the sentinel + // element (no label) signals that hidden tracking is active, so new common columns + // should be auto-added rather than skipped by the bootstrap path. + var savedXml = + "" + + "" + + "" + + "" + // sentinel: hidden tracking active, but nothing hidden + ""; + var doc = new XmlDocument(); + doc.LoadXml(savedXml); + + var hiddenNodes = doc.DocumentElement.SelectNodes("//hidden"); + bool hasHiddenTracking = hiddenNodes.Count > 0; + Assert.That(hasHiddenTracking, Is.True, "Sentinel should enable hidden tracking"); + + // Collect hidden labels — sentinel has no label, so hiddenLabels should be empty + var hiddenLabels = new HashSet(); + foreach (XmlNode hidden in hiddenNodes) + { + var label = XmlUtils.GetOptionalAttributeValue(hidden, "label", ""); + if (!string.IsNullOrEmpty(label)) + hiddenLabels.Add(label); + } + Assert.That(hiddenLabels, Is.Empty, "Sentinel should not contribute a label"); + + // A new common column that is not in the saved list and not hidden should be auto-added + // (i.e. hasHiddenTracking is true AND label is not in hiddenLabels) + var newColumnLabel = "NewCommonColumn"; + Assert.That(hasHiddenTracking, Is.True); + Assert.That(hiddenLabels.Contains(newColumnLabel), Is.False, + "New column should not be blocked when hidden tracking is active with no hidden labels"); + } + void VerifyColumn(XmlNode output, string layoutName, string attrName, string attrVal) { var node = output.SelectSingleNode("//column[@layout='" + layoutName + "']"); diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 2da482e1c3..564183af35 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -658,8 +658,12 @@ internal bool IsValidColumnSpec(XmlNode colSpec) { return true; } - // For columns not in PossibleColumnSpecs (e.g. generated columns), fall back to - // checking whether the part/layout inventory can resolve the column. + // For columns not in PossibleColumnSpecs (e.g. generated columns with a layout + // attribute), fall back to checking whether the part/layout inventory can resolve + // the column. Columns without a layout attribute that aren't in PossibleColumnSpecs + // are invalid (GetPartFromParentNode would trivially return the node itself). + if (XmlUtils.GetOptionalAttributeValue(colSpec, "layout") == null) + return false; return GetPartFromParentNode(colSpec, ListItemsClass) != null; }