diff --git a/Src/Common/Controls/XMLViews/BrowseViewer.cs b/Src/Common/Controls/XMLViews/BrowseViewer.cs index ae13740886..cae0c518cc 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,33 @@ 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); + } + 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 37e441ca49..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; @@ -133,6 +134,84 @@ 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"); + } + } + + [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 2c18ba0d38..564183af35 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; @@ -613,23 +640,31 @@ 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 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; } ///