From d6d465658b199027265d7d4dd6d030eaa5e23431 Mon Sep 17 00:00:00 2001 From: Hasso Date: Thu, 30 Oct 2025 14:18:51 -0500 Subject: [PATCH 01/12] LT-22265: Hide Invalid Columns in Bulk Edit Phoneme Features and update .gitignore (unrelated) Change-Id: Id650c14534e898426dd0fc805315bd4ce09974ee --- .gitignore | 14 +++++++------- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 4 +--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index 654ec6bd86..636707842a 100644 --- a/.gitignore +++ b/.gitignore @@ -30,12 +30,16 @@ Bin/_setLatestBuildConfig.bat Bin/_setroot.bat Lib/debug/DebugProcs.lib Lib/debug/Generic.lib +Lib/debug/graphite2.lib Lib/debug/System.Buffers.dll Lib/debug/System.ValueTuple.dll Lib/debug/unit++.* Lib/release/DebugProcs.lib Lib/release/Generic.lib -Lib/release/unit++.lib +Lib/release/graphite2.lib +Lib/release/System.Buffers.dll +Lib/release/System.ValueTuple.dll +Lib/release/unit++.* DistFiles/Parts/Generated.fwlayout Lib/src/Ethnologue/GeneratedEthnologue.cs Src/FwParatextLexiconPlugin/GeneratedParatextLexiconPluginRegistryHelper.cs @@ -94,6 +98,8 @@ Lib/src/unit++/autom4te.cache/ Lib/src/unit++/configure Lib/src/unit++/VS/*/ Lib/src/Enchant/fieldworks-enchant-1.6.1/ +Src/Kernel/FwKernelTlb.idl +Src/Kernel/*.idh Src/Kernel/libFwKernel Src/views/libViews Src/views/libVwGraphics @@ -125,9 +131,6 @@ DistFiles/ReleaseData/ !Lib/Windows/Debug !Lib/Windows/Release -Lib/debug/graphite2.lib -Lib/release/graphite2.lib - Lib/icu*.lib DistFiles/Icu*/data DistFiles/Icu*/icudt*l @@ -139,6 +142,3 @@ DistFiles/Templates/GOLDEtic.xml DistFiles/Templates/NewLangProj.fwdata DistFiles/Templates/POS.xml DistFiles/Templates/SemDom.xml - -Src/Kernel/FwKernelTlb.idl -Src/Kernel/*.idh diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index d8ea71ba1c..4b5fc0d6d2 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -594,7 +594,7 @@ internal int ListItemsClass /// check to see if column spec is still valid and useable. /// /// - /// + /// True if we can't prove the column is invalid internal bool IsValidColumnSpec(XmlNode node) { List possibleColumns = this.PossibleColumnSpecs; @@ -656,8 +656,6 @@ private bool CheckForBadCustomField(List possibleColumns, XmlNode node) return false; } - - private string GetNewLabelFromMatchingCustomField(List possibleColumns, int flid) { foreach (XmlNode possibleColumn in possibleColumns) From 156e3985d22f5f4a69bba094db31559a12c1e322 Mon Sep 17 00:00:00 2001 From: Hasso Date: Mon, 3 Nov 2025 16:50:41 -0600 Subject: [PATCH 02/12] Break Tests Change-Id: I2c36f8e602bc3f6e029c0b5e940df8d5893635ac --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 4b5fc0d6d2..a064d1ad4b 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -597,19 +597,19 @@ internal int ListItemsClass /// True if we can't prove the column is invalid internal bool IsValidColumnSpec(XmlNode node) { - List possibleColumns = this.PossibleColumnSpecs; - // first, check to see if we can find some part or child node information - // to process. Eg. Custom field column nodes that refer to parts that no longer exist - // because the custom field has been removed so the parts cannot be generated - XmlNode partNode = this.GetPartFromParentNode(node, this.ListItemsClass); - if (partNode == null) - return false; // invalid node, don't add. - bool badCustomField = CheckForBadCustomField(possibleColumns, node); - if (badCustomField) - return false; // invalid custom field, don't add. - bool badReversalIndex = CheckForBadReversalIndex(possibleColumns, node); - if (badReversalIndex) - return false; + List possibleColumns = ComputePossibleColumns(); + //// first, check to see if we can find some part or child node information + //// to process. Eg. Custom field column nodes that refer to parts that no longer exist + //// because the custom field has been removed so the parts cannot be generated + //XmlNode partNode = this.GetPartFromParentNode(node, this.ListItemsClass); + //if (partNode == null) + // return false; // invalid node, don't add. + //bool badCustomField = CheckForBadCustomField(possibleColumns, node); + //if (badCustomField) + // return false; // invalid custom field, don't add. + //bool badReversalIndex = CheckForBadReversalIndex(possibleColumns, node); + //if (badReversalIndex) + // return false; return true; // valid as far as we can tell. } @@ -1906,15 +1906,16 @@ public override void AddObjProp(int tag, IVwViewConstructor vc, int frag) /// internal bool RemoveInvalidColumns() { - List invalidColumns = new List(); - for (int i = 0; i < m_columns.Count; ++i) - { - if (!IsValidColumnSpec(m_columns[i])) - invalidColumns.Add(m_columns[i]); - } - for (int i = 0; i < invalidColumns.Count; ++i) - m_columns.Remove(invalidColumns[i]); - return invalidColumns.Count > 0; + return false; + //List invalidColumns = new List(); + //for (int i = 0; i < m_columns.Count; ++i) + //{ + // if (!IsValidColumnSpec(m_columns[i])) + // invalidColumns.Add(m_columns[i]); + //} + //for (int i = 0; i < invalidColumns.Count; ++i) + // m_columns.Remove(invalidColumns[i]); + //return invalidColumns.Count > 0; } } } From 518160d00d6cc1b6777c0debfca58f2dfb1bad6e Mon Sep 17 00:00:00 2001 From: Hasso Date: Mon, 3 Nov 2025 17:29:23 -0600 Subject: [PATCH 03/12] 'Tis the fix to be simple (or is it?) TODO: - look at complicated cases in existing code - write unit tests - fix unit tests Change-Id: I2a612b43398e519539f37c20955fb4875fe8ce15 --- .../XMLViewsTests/XmlBrowseViewBaseVcTests.cs | 16 ++++++++++++++++ .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 12 ++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs index eb23bf1893..f16cf4be84 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs @@ -178,5 +178,21 @@ public void GetHeaderLabels_ReturnsColumnSpecLabels() CollectionAssert.AreEqual(new List { "Ref", "Occurrence" }, columnLabels); } + + [Test] + public void IsValidColumnSpec_ValidReturnsTrue() + { + // set up PossibleColumnSpecs + // Pass a valid node + Assert.IsTrue(false); + } + + [Test] + public void IsValidColumnSpec_InvalidReturnsFalse() + { + // set up PossibleColumnSpecs + // Pass an invalid node + Assert.IsFalse(true); + } } } diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index a064d1ad4b..63cda1cfdc 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -597,7 +597,15 @@ internal int ListItemsClass /// True if we can't prove the column is invalid internal bool IsValidColumnSpec(XmlNode node) { - List possibleColumns = ComputePossibleColumns(); + // In the simple case, `node`s label should match a label in PossibleColumnSpecs. There may be more complicated cases. + // The existing code sure seems complicated and is entirely untested. + var label = XmlUtils.GetLocalizedAttributeValue(node, "label", null) ?? + XmlUtils.GetMandatoryAttributeValue(node, "label"); + var oLabel = XmlUtils.GetAttributeValue(node, "originalLabel"); + //MenuItem mi = new MenuItem(label, ConfigItemClicked); + if (XmlViewsUtils.FindNodeWithAttrVal(ColumnSpecs, "label", label) != null || + XmlViewsUtils.FindNodeWithAttrVal(ColumnSpecs, "originalLabel", label) != null) + return PossibleColumnSpecs.Contains(node); //// first, check to see if we can find some part or child node information //// to process. Eg. Custom field column nodes that refer to parts that no longer exist //// because the custom field has been removed so the parts cannot be generated @@ -610,7 +618,7 @@ internal bool IsValidColumnSpec(XmlNode node) //bool badReversalIndex = CheckForBadReversalIndex(possibleColumns, node); //if (badReversalIndex) // return false; - return true; // valid as far as we can tell. + return true; // valid as far as we can tell. } /// From 47d7df1953605f213eed31a3028bd5ae14c2dddd Mon Sep 17 00:00:00 2001 From: Hasso Date: Thu, 20 Nov 2025 14:28:52 -0600 Subject: [PATCH 04/12] Accept automatic changes to DotSettings Change-Id: Idc60c55b8b05cd03d357aec2f7134cd887c2ae71 --- FW.sln.DotSettings | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/FW.sln.DotSettings b/FW.sln.DotSettings index b32adec6bf..0229294ec7 100644 --- a/FW.sln.DotSettings +++ b/FW.sln.DotSettings @@ -124,6 +124,21 @@ <Policy Inspect="True" Prefix="" Suffix="" Style="AaBb" /> <Policy Inspect="True" Prefix="T" Suffix="" Style="AaBb" /> <Policy Inspect="True" Prefix="" Suffix="" Style="AaBb" /> + <Policy><Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static readonly fields (private)"><ElementKinds><Kind Name="READONLY_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Private" Description="Constant fields (private)"><ElementKinds><Kind Name="CONSTANT_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb"><ExtraRule Prefix="m_" Suffix="" Style="AaBb" /><ExtraRule Prefix="k" Suffix="" Style="AaBb" /></Policy></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Type parameters"><ElementKinds><Kind Name="TYPE_PARAMETER" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="T" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Instance" AccessRightKinds="Private" Description="Instance fields (private)"><ElementKinds><Kind Name="FIELD" /><Kind Name="READONLY_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="m_" Suffix="" Style="AaBb"><ExtraRule Prefix="m_f" Suffix="" Style="AaBb" /><ExtraRule Prefix="m_" Suffix="" Style="aaBb" /></Policy></Policy> + <Policy><Descriptor Staticness="Instance" AccessRightKinds="Protected, ProtectedInternal, Internal, Public, PrivateProtected" Description="Instance fields (not private)"><ElementKinds><Kind Name="FIELD" /><Kind Name="READONLY_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="m_" Suffix="" Style="AaBb"><ExtraRule Prefix="m_" Suffix="" Style="aaBb" /><ExtraRule Prefix="" Suffix="" Style="AaBb" /><ExtraRule Prefix="s_" Suffix="" Style="AaBb" /><ExtraRule Prefix="s_" Suffix="" Style="aaBb" /></Policy></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Local variables"><ElementKinds><Kind Name="LOCAL_VARIABLE" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="aaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Protected, ProtectedInternal, Internal, Public, PrivateProtected" Description="Constant fields (not private)"><ElementKinds><Kind Name="CONSTANT_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Static" AccessRightKinds="Protected, ProtectedInternal, Internal, Public, PrivateProtected" Description="Static fields (not private)"><ElementKinds><Kind Name="FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="m_" Suffix="" Style="AaBb"><ExtraRule Prefix="m_" Suffix="" Style="aaBb" /><ExtraRule Prefix="" Suffix="" Style="AaBb" /><ExtraRule Prefix="s_" Suffix="" Style="AaBb" /><ExtraRule Prefix="s_" Suffix="" Style="aaBb" /></Policy></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Parameters"><ElementKinds><Kind Name="PARAMETER" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="aaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Enum members"><ElementKinds><Kind Name="ENUM_MEMBER" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Types and namespaces"><ElementKinds><Kind Name="NAMESPACE" /><Kind Name="CLASS" /><Kind Name="STRUCT" /><Kind Name="ENUM" /><Kind Name="DELEGATE" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Local constants"><ElementKinds><Kind Name="LOCAL_CONSTANT" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="aaBb" /></Policy> + <Policy><Descriptor Staticness="Any" AccessRightKinds="Any" Description="Interfaces"><ElementKinds><Kind Name="INTERFACE" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="I" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Static" AccessRightKinds="Protected, ProtectedInternal, Internal, Public, PrivateProtected" Description="Static readonly fields (not private)"><ElementKinds><Kind Name="READONLY_FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="" Suffix="" Style="AaBb" /></Policy> + <Policy><Descriptor Staticness="Static" AccessRightKinds="Private" Description="Static fields (private)"><ElementKinds><Kind Name="FIELD" /></ElementKinds></Descriptor><Policy Inspect="True" WarnAboutPrefixesAndSuffixes="False" Prefix="s_" Suffix="" Style="aaBb"><ExtraRule Prefix="g_" Suffix="" Style="aaBb" /><ExtraRule Prefix="s_" Suffix="" Style="AaBb" /></Policy></Policy> $object$_On$event$ <Policy Inspect="True" Prefix="" Suffix="" Style="AaBb" /> @@ -153,6 +168,7 @@ True TEMP_FOLDER True + True True True True @@ -162,6 +178,7 @@ True True True + True True True True From 632b2609fd882666421ee931e8c4ca9e534e3708 Mon Sep 17 00:00:00 2001 From: Hasso Date: Thu, 20 Nov 2025 16:15:45 -0600 Subject: [PATCH 05/12] Update comment Change-Id: Iefd92f0aa60c03c964743a8d23d592359eb7b1cc --- Src/Common/Controls/XMLViews/XmlVc.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlVc.cs b/Src/Common/Controls/XMLViews/XmlVc.cs index fce574499b..f1a7ace8aa 100644 --- a/Src/Common/Controls/XMLViews/XmlVc.cs +++ b/Src/Common/Controls/XMLViews/XmlVc.cs @@ -1878,11 +1878,10 @@ private IPicture GetComPicture(string imagePath) /// custom field identified during TryColumnForCustomField. Is not currently meaningful /// outside TryColumnForCustomField(). /// - XmlNode m_customFieldNode = null; + private XmlNode m_customFieldNode = null; // REVIEW (Hasso) 2025.11: this should be replaced by a passed local variable /// - /// Determine whether or not the given colSpec refers to a custom field, respective of - /// whether or not it is still valid. + /// Determine whether the given colSpec refers to a custom field, irrespective of whether it is still valid. /// Uses layout/parts to find custom field specifications. /// /// From 63e09588fd53538f5686c9703bf1e45a817d87aa Mon Sep 17 00:00:00 2001 From: Hasso Date: Thu, 20 Nov 2025 17:07:59 -0600 Subject: [PATCH 06/12] Refactor Custom Field checking Fix oversimplified method to catch most things, but it still reports renamed custom fields as "bad"; check those first (need to test) Change-Id: Ib601edfa1aa670e87c7252922597769a9e259297 --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 132 +++++++++--------- 1 file changed, 68 insertions(+), 64 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 63cda1cfdc..1cf9d6bfe4 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -591,94 +591,98 @@ internal int ListItemsClass } /// - /// check to see if column spec is still valid and useable. + /// check to see if column spec is still valid. /// - /// /// True if we can't prove the column is invalid - internal bool IsValidColumnSpec(XmlNode node) + internal bool IsValidColumnSpec(XmlNode colSpec) + { + if (GetPartFromParentNode(colSpec, ListItemsClass) == null) + return false; // invalid node, don't add. + // If it is a Custom Field, check that it is valid and has the correct label + if (IsCustomField(colSpec, out var isValid)) + { + return isValid; + } + return IsValidColumnSpec_Oversimplified(colSpec); + /*/ + //var possibleColumns = PossibleColumnSpecs; // REVIEW (Hasso) 2025.11: why do we need a local copy of this pointer? + // first, check to see if we can find some part or child node information + // to process. Eg. Custom field column nodes that refer to parts that no longer exist + // because the custom field has been removed so the parts cannot be generated + if (GetPartFromParentNode(colSpec, ListItemsClass) == null) + return false; // invalid node, don't add. + // If it is a Custom Field, check that it is valid and has the correct label + if (TryColumnForCustomField(colSpec, ListItemsClass, out var customFieldNode, out var propWs)) + { + return IsValidCustomField(colSpec, customFieldNode, propWs); + } + if (CheckForBadReversalIndex(colSpec)) + return false; + return true; // valid as far as we can tell. + /**/ + } + + private bool IsValidColumnSpec_Oversimplified(XmlNode colSpec) { // In the simple case, `node`s label should match a label in PossibleColumnSpecs. There may be more complicated cases. // The existing code sure seems complicated and is entirely untested. - var label = XmlUtils.GetLocalizedAttributeValue(node, "label", null) ?? - XmlUtils.GetMandatoryAttributeValue(node, "label"); - var oLabel = XmlUtils.GetAttributeValue(node, "originalLabel"); - //MenuItem mi = new MenuItem(label, ConfigItemClicked); - if (XmlViewsUtils.FindNodeWithAttrVal(ColumnSpecs, "label", label) != null || - XmlViewsUtils.FindNodeWithAttrVal(ColumnSpecs, "originalLabel", label) != null) - return PossibleColumnSpecs.Contains(node); - //// first, check to see if we can find some part or child node information - //// to process. Eg. Custom field column nodes that refer to parts that no longer exist - //// because the custom field has been removed so the parts cannot be generated - //XmlNode partNode = this.GetPartFromParentNode(node, this.ListItemsClass); - //if (partNode == null) - // return false; // invalid node, don't add. - //bool badCustomField = CheckForBadCustomField(possibleColumns, node); - //if (badCustomField) - // return false; // invalid custom field, don't add. - //bool badReversalIndex = CheckForBadReversalIndex(possibleColumns, node); - //if (badReversalIndex) - // return false; - return true; // valid as far as we can tell. + var label = XmlUtils.GetMandatoryAttributeValue(colSpec, "label"); + var originalLabel = XmlUtils.GetAttributeValue(colSpec, "originalLabel"); + return (XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", label) != null || + XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", originalLabel) != null); } /// - /// Check for a nonexistent custom field. (Custom fields can be deleted.) As a side-effect, - /// if the node refers to a valid custom field, the label attribute is adjusted to what we - /// want the user to see. + /// Check whether the column spec is a custom field, and if so, if it is still valid (Custom fields can be deleted). + /// If the node refers to a valid custom field, the label attribute is adjusted to what we want the user to see. /// - /// - /// - /// true if this node refers to a nonexistent custom field - private bool CheckForBadCustomField(List possibleColumns, XmlNode node) + private bool IsCustomField(XmlNode colSpec, out bool isValidCustomField) { - // see if this node is based on a layout. If so, get its part - PropWs propWs; - XmlNode columnForCustomField = null; - if (this.TryColumnForCustomField(node, this.ListItemsClass, out columnForCustomField, out propWs)) + if (!TryColumnForCustomField(colSpec, ListItemsClass, out var columnForCustomField, out var propWs)) { - if (columnForCustomField != null) - { - string fieldName = XmlUtils.GetAttributeValue(columnForCustomField, "field"); - string className = XmlUtils.GetAttributeValue(columnForCustomField, "class"); - if (!String.IsNullOrEmpty(fieldName) && !String.IsNullOrEmpty(className)) - { - if ((m_mdc as IFwMetaDataCacheManaged).FieldExists(className, fieldName, false)) - { - ColumnConfigureDialog.GenerateColumnLabel(node, m_cache); - return false; - } - } - return true; - } - else if (propWs != null) + isValidCustomField = false; + return false; + } + + if (columnForCustomField != null) + { + var fieldName = XmlUtils.GetAttributeValue(columnForCustomField, "field"); + var className = XmlUtils.GetAttributeValue(columnForCustomField, "class"); + if (!string.IsNullOrEmpty(fieldName) && !string.IsNullOrEmpty(className) && + ((IFwMetaDataCacheManaged)m_mdc).FieldExists(className, fieldName, false)) { - XmlUtils.AppendAttribute(node, "originalLabel", GetNewLabelFromMatchingCustomField(possibleColumns, propWs.flid)); - ColumnConfigureDialog.GenerateColumnLabel(node, m_cache); + ColumnConfigureDialog.GenerateColumnLabel(colSpec, m_cache); + isValidCustomField = true; } else { - // it's an invalid custom field. - return true; + isValidCustomField = false; } } - return false; + else if (propWs == null) + { + isValidCustomField = false; + } + else + { + XmlUtils.AppendAttribute(colSpec, "originalLabel", GetNewLabelFromMatchingCustomField(propWs.flid)); + ColumnConfigureDialog.GenerateColumnLabel(colSpec, m_cache); + isValidCustomField = true; + } + return true; } - private string GetNewLabelFromMatchingCustomField(List possibleColumns, int flid) + private string GetNewLabelFromMatchingCustomField(int flid) { - foreach (XmlNode possibleColumn in possibleColumns) + foreach (var possibleColumn in PossibleColumnSpecs) { // Desired node may be a child of a child... (See LT-6447.) - PropWs propWs; - XmlNode columnForCustomField; - if (TryColumnForCustomField(possibleColumn, ListItemsClass, out columnForCustomField, out propWs)) + if (TryColumnForCustomField(possibleColumn, ListItemsClass, out _, out var propWs)) { // the flid of the updated custom field node matches the given flid of the old node. if (propWs != null && propWs.flid == flid) { - string label = XmlUtils.GetLocalizedAttributeValue(possibleColumn, - "label", null); - return label; + return XmlUtils.GetLocalizedAttributeValue(possibleColumn, "label", null); } } } @@ -687,11 +691,11 @@ private string GetNewLabelFromMatchingCustomField(List possibleColumns, /// /// Check for an invalid reversal index. (Reversal indexes can be deleted.) + /// REVIEW (Hasso) 2025-11: when does a column spec refer to a reversal index? /// - /// /// /// true if this node refers to a nonexistent reversal index. - private bool CheckForBadReversalIndex(List possibleColumns, XmlNode node) + private bool CheckForBadReversalIndex(XmlNode node) { // Look for a child node which is similar to this (value of ws attribute may differ): // From 9b85770372217cb8979f29ab7e6ea311c6b6c89f Mon Sep 17 00:00:00 2001 From: Hasso Date: Tue, 25 Nov 2025 11:06:33 -0600 Subject: [PATCH 07/12] clean up Change-Id: I4724b1a06b79eaf8726bd30e5b3c7d4a9af52101 --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 67 +++---------------- 1 file changed, 8 insertions(+), 59 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 1cf9d6bfe4..17c3fa8654 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -1,4 +1,4 @@ -// Copyright (c) 2005-2017 SIL International +// Copyright (c) 2005-2025 SIL International // This software is licensed under the LGPL, version 2.1 or later // (http://www.gnu.org/licenses/lgpl-2.1.html) @@ -603,37 +603,17 @@ internal bool IsValidColumnSpec(XmlNode colSpec) { return isValid; } - return IsValidColumnSpec_Oversimplified(colSpec); - /*/ - //var possibleColumns = PossibleColumnSpecs; // REVIEW (Hasso) 2025.11: why do we need a local copy of this pointer? - // first, check to see if we can find some part or child node information - // to process. Eg. Custom field column nodes that refer to parts that no longer exist - // because the custom field has been removed so the parts cannot be generated - if (GetPartFromParentNode(colSpec, ListItemsClass) == null) - return false; // invalid node, don't add. - // If it is a Custom Field, check that it is valid and has the correct label - if (TryColumnForCustomField(colSpec, ListItemsClass, out var customFieldNode, out var propWs)) - { - return IsValidCustomField(colSpec, customFieldNode, propWs); - } - if (CheckForBadReversalIndex(colSpec)) - return false; - return true; // valid as far as we can tell. - /**/ - } - - private bool IsValidColumnSpec_Oversimplified(XmlNode colSpec) - { // In the simple case, `node`s label should match a label in PossibleColumnSpecs. There may be more complicated cases. - // The existing code sure seems complicated and is entirely untested. - var label = XmlUtils.GetMandatoryAttributeValue(colSpec, "label"); - var originalLabel = XmlUtils.GetAttributeValue(colSpec, "originalLabel"); - return (XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", label) != null || - XmlViewsUtils.FindNodeWithAttrVal(PossibleColumnSpecs, "label", originalLabel) != null); + 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; } /// - /// Check whether the column spec is a custom field, and if so, if it is still valid (Custom fields can be deleted). + /// Check whether the column spec is a custom field, and if so, if it is still valid (Custom Fields can be deleted). /// If the node refers to a valid custom field, the label attribute is adjusted to what we want the user to see. /// private bool IsCustomField(XmlNode colSpec, out bool isValidCustomField) @@ -688,37 +668,6 @@ private string GetNewLabelFromMatchingCustomField(int flid) } return ""; } - - /// - /// Check for an invalid reversal index. (Reversal indexes can be deleted.) - /// REVIEW (Hasso) 2025-11: when does a column spec refer to a reversal index? - /// - /// - /// true if this node refers to a nonexistent reversal index. - private bool CheckForBadReversalIndex(XmlNode node) - { - // Look for a child node which is similar to this (value of ws attribute may differ): - // - XmlNode child = XmlUtils.FindNode(node, "string"); - if (child != null && - XmlUtils.GetOptionalAttributeValue(child, "field") == "ReversalEntriesText") - { - string sWs = StringServices.GetWsSpecWithoutPrefix(child); - if (sWs != null && sWs != "reversal") - { - if (!m_cache.ServiceLocator.WritingSystemManager.Exists(sWs)) - return true; // invalid writing system - // Check whether we have a reversal index for the given writing system. - foreach (var idx in m_cache.LangProject.LexDbOA.ReversalIndexesOC) - { - if (idx.WritingSystem == sWs) - return false; - } - return true; - } - } - return false; - } #endregion Construction and initialization #region Properties From 01010f17d5c6c98b9554b038df692e0193f50d4a Mon Sep 17 00:00:00 2001 From: Hasso Date: Tue, 25 Nov 2025 16:31:33 -0600 Subject: [PATCH 08/12] Add Tests for the part that changed significantly. TODO: consider whether checking `field` instead of `label' would make more sense. Change-Id: I04290497ebc11dc625867ca33ce1b1d33027c7aa --- .../XMLViewsTests/XmlBrowseViewBaseVcTests.cs | 45 ++++++++++++++----- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 25 ++++------- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs index f16cf4be84..12f9ccf71e 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs @@ -1,12 +1,12 @@ -// Copyright (c) 2015 SIL International +// Copyright (c) 2015-2025 SIL International // This software is licensed under the LGPL, version 2.1 or later // (http://www.gnu.org/licenses/lgpl-2.1.html) +using NUnit.Framework; +using SIL.FieldWorks.Common.Controls; using System.Collections.Generic; using System.Linq; using System.Xml; -using NUnit.Framework; -using SIL.FieldWorks.Common.Controls; namespace XMLViewsTests { @@ -168,7 +168,6 @@ public void GetHeaderLabels_ReturnsColumnSpecLabels() var columnDoc = new XmlDocument(); columnDoc.LoadXml(testColumns); - columnDoc.SelectNodes("column"); var testVc = new XmlBrowseViewBaseVc { ColumnSpecs = new List(columnDoc.DocumentElement.GetElementsByTagName("column").OfType()) @@ -182,17 +181,41 @@ public void GetHeaderLabels_ReturnsColumnSpecLabels() [Test] public void IsValidColumnSpec_ValidReturnsTrue() { - // set up PossibleColumnSpecs - // Pass a valid node - Assert.IsTrue(false); + var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; + var possibleColumns = new XmlDocument(); + possibleColumns.LoadXml(""); + foreach (XmlNode node in possibleColumns.DocumentElement.GetElementsByTagName("column")) + { + vc.PossibleColumnSpecs.Add(node); + } + + var savedColumns = new XmlDocument(); + savedColumns.LoadXml(""); + + // SUT + foreach (XmlNode node in savedColumns.DocumentElement.GetElementsByTagName("column")) + { + Assert.IsTrue(vc.IsValidColumnSpec(node), $"Should have found this node to be valid: {node.OuterXml}"); + } } [Test] public void IsValidColumnSpec_InvalidReturnsFalse() { - // set up PossibleColumnSpecs - // Pass an invalid node - Assert.IsFalse(true); + var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; + var possibleColumns = new XmlDocument(); + possibleColumns.LoadXml(""); + foreach (XmlNode node in possibleColumns.DocumentElement.GetElementsByTagName("column")) + { + vc.PossibleColumnSpecs.Add(node); + } + + var savedColumns = new XmlDocument(); + savedColumns.LoadXml(""); + var invalidColumn = savedColumns.DocumentElement.SelectSingleNode("column"); + + // SUT + Assert.IsFalse(vc.IsValidColumnSpec(invalidColumn), $"Should have found this node to be invalid: {invalidColumn.OuterXml}"); } } -} +} \ No newline at end of file diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 17c3fa8654..d374180c17 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -56,10 +56,7 @@ public class XmlBrowseViewBaseVc : XmlVc /// Specifications of columns to display /// protected List m_columns = new List(); - /// - /// Specs of columns that COULD be displayed, but which have not been selected. - /// - protected List m_possibleColumns; + /// // Top-level fake property for list of objects. protected int m_fakeFlid = 0; /// // Controls appearance of column for check boxes. @@ -216,7 +213,7 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) if (doc == null) // nothing saved, or saved info won't parse { // default: the columns that have 'width' specified. - foreach(XmlNode node in m_possibleColumns) + foreach(XmlNode node in PossibleColumnSpecs) { if (XmlUtils.GetOptionalAttributeValue(node, "visibility", "always") == "always") m_columns.Add(node); @@ -224,7 +221,7 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) } else { - var newPossibleColumns = new List(m_possibleColumns); + var newPossibleColumns = new List(PossibleColumnSpecs); foreach (XmlNode node in doc.DocumentElement.SelectNodes("//column")) { @@ -550,9 +547,7 @@ public List ComputePossibleColumns() XmlVc vc = null; if (ListItemsClass != 0) vc = this; - m_possibleColumns = PartGenerator.GetGeneratedChildren(m_xnSpec.SelectSingleNode("columns"), - m_xbv.Cache, vc, (int)ListItemsClass); - return m_possibleColumns; + return PossibleColumnSpecs = PartGenerator.GetGeneratedChildren(m_xnSpec.SelectSingleNode("columns"), m_xbv.Cache, vc, (int)ListItemsClass); } int m_listItemsClass = 0; @@ -604,6 +599,7 @@ internal bool IsValidColumnSpec(XmlNode colSpec) return isValid; } // In the simple case, `node`s label should match a label in PossibleColumnSpecs. There may be more complicated cases. + // TODO (Hasso) 2025.11: 'layout' (madatory?) and 'field' (optional) would be better attributes to match. var label = XmlUtils.GetLocalizedAttributeValue(colSpec, "label", null) ?? XmlUtils.GetMandatoryAttributeValue(colSpec, "label"); var originalLabel = XmlUtils.GetLocalizedAttributeValue(colSpec, "originalLabel", null) ?? @@ -763,13 +759,10 @@ internal virtual List ColumnSpecs } } - internal List PossibleColumnSpecs - { - get - { - return m_possibleColumns; - } - } + /// + /// Specs of columns that COULD be displayed, but which have not been selected. + /// + protected internal List PossibleColumnSpecs { get; set; } internal ISortItemProvider SortItemProvider { From 31d0eb44647356cfe5678a96c762e3b125f63438 Mon Sep 17 00:00:00 2001 From: Hasso Date: Wed, 26 Nov 2025 10:37:43 -0600 Subject: [PATCH 09/12] Add Test with Real Data Doesn't work because the Layout hasn't been set up everywhere Change-Id: Ic7593a7f2a25749fb1f5aa3664f8266cc0ec8eca --- .../XMLViewsTests/XmlBrowseViewBaseVcTests.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs index 12f9ccf71e..59cf7ec9d8 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs @@ -178,6 +178,35 @@ public void GetHeaderLabels_ReturnsColumnSpecLabels() CollectionAssert.AreEqual(new List { "Ref", "Occurrence" }, columnLabels); } + [Test] + public void IsValidColumnSpec_HasLayout_TODO() + { + var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; + var possibleColumns = new XmlDocument(); + possibleColumns.LoadXml(@" + + + +"); + foreach (XmlNode node in possibleColumns.DocumentElement.GetElementsByTagName("column")) + { + vc.PossibleColumnSpecs.Add(node); + } + + var validColumns = new XmlDocument(); + validColumns.LoadXml(@" + + + +"); + + // SUT + foreach (XmlNode node in validColumns.DocumentElement.GetElementsByTagName("column")) + { + Assert.IsTrue(vc.IsValidColumnSpec(node), $"Should have found this node to be valid: {node.OuterXml}"); + } + } + [Test] public void IsValidColumnSpec_ValidReturnsTrue() { From 06799e9071dc39e2f959622f2e2442c95e56738c Mon Sep 17 00:00:00 2001 From: Hasso Date: Wed, 26 Nov 2025 10:50:08 -0600 Subject: [PATCH 10/12] Clean up Change-Id: Ia5fb27d9058f83e3ab1fcfa35ba7158e47bb8a88 --- .../XMLViewsTests/XmlBrowseViewBaseVcTests.cs | 33 ++++++++----------- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 12 +++---- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs index 59cf7ec9d8..a3bbf59952 100644 --- a/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs +++ b/Src/Common/Controls/XMLViews/XMLViewsTests/XmlBrowseViewBaseVcTests.cs @@ -178,7 +178,7 @@ public void GetHeaderLabels_ReturnsColumnSpecLabels() CollectionAssert.AreEqual(new List { "Ref", "Occurrence" }, columnLabels); } - [Test] + /// TODO (Hasso) 2025.11: This test needs further setup to find the layout XmlNode in the LayoutCache public void IsValidColumnSpec_HasLayout_TODO() { var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; @@ -207,8 +207,13 @@ public void IsValidColumnSpec_HasLayout_TODO() } } + /// + /// Tests that IsValidColumnSpec can identify valid columns based on their labels or originalLabels. + /// Artificially skips the layout check because that requires a more complex setup. + /// LT-22265: Invalid columns should not be displayed. + /// [Test] - public void IsValidColumnSpec_ValidReturnsTrue() + public void IsValidColumnSpec_MatchesLabels() { var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; var possibleColumns = new XmlDocument(); @@ -218,30 +223,18 @@ public void IsValidColumnSpec_ValidReturnsTrue() vc.PossibleColumnSpecs.Add(node); } - var savedColumns = new XmlDocument(); - savedColumns.LoadXml(""); + var validColumns = new XmlDocument(); + validColumns.LoadXml(""); // SUT - foreach (XmlNode node in savedColumns.DocumentElement.GetElementsByTagName("column")) + foreach (XmlNode node in validColumns.DocumentElement.GetElementsByTagName("column")) { Assert.IsTrue(vc.IsValidColumnSpec(node), $"Should have found this node to be valid: {node.OuterXml}"); } - } - - [Test] - public void IsValidColumnSpec_InvalidReturnsFalse() - { - var vc = new XmlBrowseViewBaseVc { PossibleColumnSpecs = new List(), ListItemsClass = -1 /* can't be 0 */ }; - var possibleColumns = new XmlDocument(); - possibleColumns.LoadXml(""); - foreach (XmlNode node in possibleColumns.DocumentElement.GetElementsByTagName("column")) - { - vc.PossibleColumnSpecs.Add(node); - } - var savedColumns = new XmlDocument(); - savedColumns.LoadXml(""); - var invalidColumn = savedColumns.DocumentElement.SelectSingleNode("column"); + var invalidColumns = new XmlDocument(); + invalidColumns.LoadXml(""); + var invalidColumn = invalidColumns.DocumentElement.SelectSingleNode("column"); // SUT Assert.IsFalse(vc.IsValidColumnSpec(invalidColumn), $"Should have found this node to be invalid: {invalidColumn.OuterXml}"); diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index d374180c17..d16e1dcb0a 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -224,7 +224,6 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) var newPossibleColumns = new List(PossibleColumnSpecs); foreach (XmlNode node in doc.DocumentElement.SelectNodes("//column")) { - // if there is a corresponding possible column, remove it from the newPossibleColumns list. var possible = newPossibleColumns.Find(n => XmlUtils.GetOptionalAttributeValue(n, "label", "") == @@ -586,20 +585,21 @@ internal int ListItemsClass } /// - /// check to see if column spec is still valid. + /// Check to see if column spec is still valid. If it is a custom field, update the label if necessary. /// - /// True if we can't prove the column is invalid internal bool IsValidColumnSpec(XmlNode colSpec) { if (GetPartFromParentNode(colSpec, ListItemsClass) == null) - return false; // invalid node, don't add. - // If it is a Custom Field, check that it is valid and has the correct label + { + 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. - // TODO (Hasso) 2025.11: 'layout' (madatory?) and 'field' (optional) would be better attributes to match. + // ENHANCE (Hasso) 2025.11: 'layout' (mandatory?) and 'field' (optional) would be better attributes to match, but that would require more test setup. var label = XmlUtils.GetLocalizedAttributeValue(colSpec, "label", null) ?? XmlUtils.GetMandatoryAttributeValue(colSpec, "label"); var originalLabel = XmlUtils.GetLocalizedAttributeValue(colSpec, "originalLabel", null) ?? From da5f942bcef76fdaeff9f14ef5af35d6db4ed7d5 Mon Sep 17 00:00:00 2001 From: Hasso Date: Wed, 26 Nov 2025 11:27:39 -0600 Subject: [PATCH 11/12] Convert some simple properties to autoproperties Change-Id: Iebbbd8da9411b209313024f5ac5b071203fc364b --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 55 ++++++------------- .../Controls/XMLViews/XmlRDEBrowseViewVc.cs | 10 ++-- 2 files changed, 21 insertions(+), 44 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index d16e1dcb0a..5e1b1a87f5 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -51,12 +51,6 @@ public class XmlBrowseViewBaseVc : XmlVc #endregion Constants #region Data Members - - /// - /// Specifications of columns to display - /// - protected List m_columns = new List(); - /// // Top-level fake property for list of objects. protected int m_fakeFlid = 0; /// // Controls appearance of column for check boxes. @@ -73,8 +67,7 @@ public class XmlBrowseViewBaseVc : XmlVc protected int m_dxmpCheckBorderWidth = 72000 / 96; /// protected XmlBrowseViewBase m_xbv; - /// - protected ISortItemProvider m_sortItemProvider; + IPicture m_PreviewArrowPic; IPicture m_PreviewRTLArrowPic; int m_icolOverrideAllowEdit = -1; // index of column to force allow editing in. @@ -216,7 +209,7 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) foreach(XmlNode node in PossibleColumnSpecs) { if (XmlUtils.GetOptionalAttributeValue(node, "visibility", "always") == "always") - m_columns.Add(node); + ColumnSpecs.Add(node); } } else @@ -231,14 +224,14 @@ public XmlBrowseViewBaseVc(XmlNode xnSpec, int fakeFlid, XmlBrowseViewBase xbv) if (possible != null) newPossibleColumns.Remove(possible); if (IsValidColumnSpec(node)) - m_columns.Add(node); + ColumnSpecs.Add(node); } foreach (var node in newPossibleColumns) { // add any possible columns that were not in the saved list and are common if (XmlUtils.GetOptionalAttributeValue(node, "common", "false") == "true") - m_columns.Add(node); + ColumnSpecs.Add(node); } } m_fakeFlid = fakeFlid; @@ -747,34 +740,18 @@ public static List GetHeaderLabels(XmlBrowseViewBaseVc vc) return headerLabelList; } - internal virtual List ColumnSpecs - { - get - { - return m_columns; - } - set - { - m_columns = value; - } - } + /// + /// Specifications of columns to display + /// + protected internal virtual List ColumnSpecs { get; set; } = new List(); /// /// Specs of columns that COULD be displayed, but which have not been selected. /// protected internal List PossibleColumnSpecs { get; set; } - internal ISortItemProvider SortItemProvider - { - get - { - return m_sortItemProvider; - } - set - { - m_sortItemProvider = value; - } - } + /// + protected internal ISortItemProvider SortItemProvider { get; set; } /// /// Use this to store a suitable preview arrow if we will be displaying previews. @@ -892,7 +869,7 @@ protected virtual void AddTableRow(IVwEnv vwenv, int hvo, int frag) // Make a table. VwLength[] rglength = m_xbv.GetColWidthInfo(); - int colCount = m_columns.Count; + int colCount = ColumnSpecs.Count; if (m_fShowSelected) colCount++; @@ -954,12 +931,12 @@ protected virtual void AddTableRow(IVwEnv vwenv, int hvo, int frag) int cAdjCol = m_fShowSelected ? 0 : 1; if (m_fShowColumnsRTL) { - for (int icol = m_columns.Count; icol > 0; --icol) + for (int icol = ColumnSpecs.Count; icol > 0; --icol) AddTableCell(vwenv, hvo, index, hvoRoot, icolActive, cAdjCol, icol); } else { - for (int icol = 1; icol <= m_columns.Count; ++icol) + for (int icol = 1; icol <= ColumnSpecs.Count; ++icol) AddTableCell(vwenv, hvo, index, hvoRoot, icolActive, cAdjCol, icol); } vwenv.CloseTableRow(); @@ -993,7 +970,7 @@ internal override int WsForce private void AddTableCell(IVwEnv vwenv, int hvo, int index, int hvoRoot, int icolActive, int cAdjCol, int icol) { - XmlNode node = m_columns[icol - 1]; + XmlNode node = ColumnSpecs[icol - 1]; // Figure out the underlying Right-To-Left value. bool fRightToLeft = false; // Figure out if this column's writing system is audio. @@ -1095,7 +1072,7 @@ private void AddTableCell(IVwEnv vwenv, int hvo, int index, int hvoRoot, int ico fParaOpened = true; } - if (m_sortItemProvider == null) + if (SortItemProvider == null) { try { @@ -1118,7 +1095,7 @@ private void AddTableCell(IVwEnv vwenv, int hvo, int index, int hvoRoot, int ico int hvoDum, tag, ihvo; vwenv.GetOuterObject(level - 2, out hvoDum, out tag, out ihvo); Debug.Assert(tag == m_fakeFlid); - IManyOnePathSortItem item = m_sortItemProvider.SortItemAt(ihvo); + IManyOnePathSortItem item = SortItemProvider.SortItemAt(ihvo); if (item != null) DisplayCell(item, node, hvo, vwenv); // (Original) cell contents } diff --git a/Src/Common/Controls/XMLViews/XmlRDEBrowseViewVc.cs b/Src/Common/Controls/XMLViews/XmlRDEBrowseViewVc.cs index bc450b9bac..c4b85f32be 100644 --- a/Src/Common/Controls/XMLViews/XmlRDEBrowseViewVc.cs +++ b/Src/Common/Controls/XMLViews/XmlRDEBrowseViewVc.cs @@ -398,7 +398,7 @@ private bool ShouldSuppressNoForOtherColumn(XmlNode frag) { // If the column that suppresses the "no" special behavior is present, we don't want the "no" child. // That includes if a ws-specific column which includes that label is present. - foreach (var col in m_columns) + foreach (var col in ColumnSpecs) { if (XmlUtils.GetOptionalAttributeValue(col, @"label").Contains(suppressNoForColumn)) return true; @@ -445,7 +445,7 @@ private void AddEditRow(IVwEnv vwenv, int hvo) // Make a table VwLength[] rglength = m_xbv.GetColWidthInfo(); - int colCount = m_columns.Count; + int colCount = ColumnSpecs.Count; if (m_fShowSelected) colCount++; @@ -476,12 +476,12 @@ private void AddEditRow(IVwEnv vwenv, int hvo) // Make the cells. if (m_fShowColumnsRTL) { - for (int i = m_columns.Count; i > 0; --i) + for (int i = ColumnSpecs.Count; i > 0; --i) AddEditCell(vwenv, cda, i); } else { - for (int i = 1; i <= m_columns.Count; ++i) + for (int i = 1; i <= ColumnSpecs.Count; ++i) AddEditCell(vwenv, cda, i); } vwenv.CloseTableRow(); @@ -491,7 +491,7 @@ private void AddEditRow(IVwEnv vwenv, int hvo) private void AddEditCell(IVwEnv vwenv, IVwCacheDa cda, int i) { - XmlNode node = m_columns[i - 1]; + XmlNode node = ColumnSpecs[i - 1]; // Make a cell and embed an editable virtual string for the column. var editable = XmlUtils.GetOptionalBooleanAttributeValue(node, "editable", true); if (!editable) From 11891048198926d5a05450f3b12ba192572b4d8b Mon Sep 17 00:00:00 2001 From: Hasso Date: Wed, 26 Nov 2025 11:34:33 -0600 Subject: [PATCH 12/12] Clean up and modernise Change-Id: I628fc99c77ea267cfcef4fda877c5c584b0c88ae --- .../Controls/XMLViews/XmlBrowseViewBaseVc.cs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs index 5e1b1a87f5..8dd512bff2 100644 --- a/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs +++ b/Src/Common/Controls/XMLViews/XmlBrowseViewBaseVc.cs @@ -8,6 +8,7 @@ using System.Collections.Generic; using System.Drawing; using System.Diagnostics; +using System.Linq; using System.Windows.Forms; using System.Reflection; // for check-box icons. using SIL.FieldWorks.Common.FwUtils; @@ -746,7 +747,7 @@ public static List GetHeaderLabels(XmlBrowseViewBaseVc vc) protected internal virtual List ColumnSpecs { get; set; } = new List(); /// - /// Specs of columns that COULD be displayed, but which have not been selected. + /// Specs of columns that COULD be displayed, regardless of whether they have been selected. /// protected internal List PossibleColumnSpecs { get; set; } @@ -1837,16 +1838,12 @@ public override void AddObjProp(int tag, IVwViewConstructor vc, int frag) /// internal bool RemoveInvalidColumns() { - return false; - //List invalidColumns = new List(); - //for (int i = 0; i < m_columns.Count; ++i) - //{ - // if (!IsValidColumnSpec(m_columns[i])) - // invalidColumns.Add(m_columns[i]); - //} - //for (int i = 0; i < invalidColumns.Count; ++i) - // m_columns.Remove(invalidColumns[i]); - //return invalidColumns.Count > 0; + var invalidColumns = ColumnSpecs.Where(colSpec => !IsValidColumnSpec(colSpec)).ToList(); + foreach (var colSpec in invalidColumns) + { + ColumnSpecs.Remove(colSpec); + } + return invalidColumns.Count > 0; } } }