Fix LT-22316 regression: persist hidden column labels (#790)

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 <hidden> 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
- Reorder the checks so PossibleColumnSpecs label match takes priority,
with GetPartFromParentNode as a fallback for generated columns.
- Write sentinel <hidden/> when no columns are hidden (LT-22447)

Author: jasonleenaylor

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;
 
 		/// <summary>
 		/// 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<string>();
+			foreach (XmlNode node in ColumnSpecs)
+			{
+				var label = XmlUtils.GetOptionalAttributeValue(node, "label", "");
+				if (!string.IsNullOrEmpty(label))
+					activeLabels.Add(label);
+			}
+			var hiddenLabels = new SortedSet<string>(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("<hidden/>");
+			}
+			else
+			{
+				foreach (var label in hiddenLabels)
+					colList.Append("<hidden label=\"" + SecurityElement.Escape(label) + "\"/>");
+			}
 			colList.Append("</root>");
 			m_propertyTable.SetProperty(m_xbv.Vc.ColListId, colList.ToString(), PropertyTable.SettingsGroup.LocalSettings, true);
 		}

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 =
+				"<root version=\"19\">" +
+				"<column layout=\"EntryHeadwordForFindEntry\" label=\"Headword\" sortmethod=\"FullSortKey\" ws=\"$ws=vernacular\" editable=\"false\" width=\"96000\"/>" +
+				"<column layout=\"Unknown Test\"/>" +
+				"</root>";
+			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 =
+				"<root version=\"19\">" +
+				"<column layout=\"EntryHeadwordForFindEntry\" label=\"Headword\"/>" +
+				"<hidden label=\"Lexeme Form\"/>" +
+				"</root>";
+			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 <hidden/>
+			// 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 =
+				"<root version=\"20\">" +
+				"<column layout=\"Name\" label=\"Name\" ws=\"$ws=best analysis\" field=\"Name\"/>" +
+				"<column layout=\"Abbreviation\" label=\"Abbreviation\" ws=\"$ws=best analysis\" field=\"Abbreviation\"/>" +
+				"<hidden/>" +  // sentinel: hidden tracking active, but nothing hidden
+				"</root>";
+			var doc = new XmlDocument();
+			doc.LoadXml(savedXml);
+
+			var hiddenNodes = doc.DocumentElement.SelectNodes("//hidden");
+			bool hasHiddenTracking = hiddenNodes.Count > 0;
+			Assert.That(hasHiddenTracking, Is.True, "Sentinel <hidden/> should enable hidden tracking");
+
+			// Collect hidden labels — sentinel has no label, so hiddenLabels should be empty
+			var hiddenLabels = new HashSet<string>();
+			foreach (XmlNode hidden in hiddenNodes)
+			{
+				var label = XmlUtils.GetOptionalAttributeValue(hidden, "label", "");
+				if (!string.IsNullOrEmpty(label))
+					hiddenLabels.Add(label);
+			}
+			Assert.That(hiddenLabels, Is.Empty, "Sentinel <hidden/> 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 + "']");

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<string>();
+				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
 		/// </summary>
 		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;
 		}
 
 		/// <summary>