From 838c57f3a17b1befc246b577bd1b174ce3823c2a Mon Sep 17 00:00:00 2001
From: leper <leper@wildfiregames.com>
Date: Mon, 11 Apr 2016 03:45:15 +0200
Subject: [PATCH] Fix some issues.

 * Do not specify Health twice for foundations.
 * Change merging behaviour slightly to fix the Identity case.
   Previously using merge="" would overwrite the value of that tag with the new value,
   this would lead to all elements below Identity having empty/undef values, causing
   the GUI to complain about the specific name not being defined.
 * Only keep elements that are actually defined in the parent if we are filtering. (merge fix #2)
 * Remove unused function.
 * Add tests for some of the above issues.
   TODO: Check if some of the tests are (partially) duplicated, and possibly fix the tags of the last test.
---
 .../templates/special_filter/foundation.xml        |  9 ++++-----
 source/simulation2/system/ParamNode.cpp            | 22 +++++-----------------
 source/simulation2/system/ParamNode.h              |  8 --------
 source/simulation2/tests/test_ParamNode.h          | 13 +++++++++++++
 4 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/binaries/data/mods/public/simulation/templates/special_filter/foundation.xml b/binaries/data/mods/public/simulation/templates/special_filter/foundation.xml
index e294934..633ccdc 100644
--- a/binaries/data/mods/public/simulation/templates/special_filter/foundation.xml
+++ b/binaries/data/mods/public/simulation/templates/special_filter/foundation.xml
@@ -8,15 +8,14 @@
     <PopulationBonus>0</PopulationBonus>
   </Cost>
   <Decay merge=""/>
-  <!-- Initialise health to 1 -->
-  <Health>
-    <Initial>1</Initial>
-  </Health>
   <Fogging merge=""/>
   <Footprint merge=""/>
   <!-- Add the Foundation component, to deal with the construction process -->
   <Foundation replace=""/>
-  <Health merge=""/>
+  <!-- Initialise health to 1 -->
+  <Health>
+    <Initial>1</Initial>
+  </Health>
   <Identity merge=""/>
   <!-- Foundations shouldn't initially block unit movement -->
   <Obstruction merge="">
diff --git a/source/simulation2/system/ParamNode.cpp b/source/simulation2/system/ParamNode.cpp
index 74ca203..e41df76 100644
--- a/source/simulation2/system/ParamNode.cpp
+++ b/source/simulation2/system/ParamNode.cpp
@@ -87,6 +87,7 @@ void CParamNode::ApplyLayer(const XMBFile& xmb, const XMBElement& element, const
 	} op = INVALID;
 	bool replacing = false;
 	bool filtering = false;
+	bool merging = false;
 	{
 		XERO_ITER_ATTR(element, attr)
 		{
@@ -108,6 +109,7 @@ void CParamNode::ApplyLayer(const XMBFile& xmb, const XMBElement& element, const
 			{
 				if (m_Childs.find(name) == m_Childs.end())
 					return;
+				merging = true;
 			}
 			else if (attr.Name == at_op)
 			{
@@ -183,7 +185,7 @@ void CParamNode::ApplyLayer(const XMBFile& xmb, const XMBElement& element, const
 		} 
 		hasSetValue = true; 
 	} 
-	if (!hasSetValue)
+	if (!hasSetValue && !merging)
 		node.m_Value = value;
 
 	// Recurse through the element's children
@@ -193,7 +195,8 @@ void CParamNode::ApplyLayer(const XMBFile& xmb, const XMBElement& element, const
 		if (filtering)
 		{
 			std::string childname = xmb.GetElementString(child.GetNodeName()); // TODO: is GetElementString inefficient? (especially if we call it twice)
-			childs[childname] = std::move(node.m_Childs[childname]);
+			if (node.m_Childs.find(childname) != node.m_Childs.end())
+				childs[childname] = std::move(node.m_Childs[childname]);
 		}
 	}
 
@@ -212,21 +215,6 @@ void CParamNode::ApplyLayer(const XMBFile& xmb, const XMBElement& element, const
 	}
 }
 
-void CParamNode::CopyFilteredChildrenOfChild(const CParamNode& src, const char* name, const std::set<std::string>& permitted)
-{
-	ResetScriptVal();
-
-	ChildrenMap::iterator dstChild = m_Childs.find(name);
-	ChildrenMap::const_iterator srcChild = src.m_Childs.find(name);
-	if (dstChild == m_Childs.end() || srcChild == src.m_Childs.end())
-		return; // error
-
-	ChildrenMap::const_iterator it = srcChild->second.m_Childs.begin();
-	for (; it != srcChild->second.m_Childs.end(); ++it)
-		if (permitted.count(it->first))
-			dstChild->second.m_Childs[it->first] = it->second;
-}
-
 const CParamNode& CParamNode::GetChild(const char* name) const
 {
 	ChildrenMap::const_iterator it = m_Childs.find(name);
diff --git a/source/simulation2/system/ParamNode.h b/source/simulation2/system/ParamNode.h
index 9ce530e..0b18e5e 100644
--- a/source/simulation2/system/ParamNode.h
+++ b/source/simulation2/system/ParamNode.h
@@ -182,14 +182,6 @@ public:
 	static PSRETURN LoadXMLString(CParamNode& ret, const char* xml, const wchar_t* sourceIdentifier = NULL);
 
 	/**
-	 * Finds the childs named @a name from @a src and from @a this, and copies the source child's children
-	 * which are in the @a permitted set into this node's child.
-	 * Intended for use as a filtered clone of XML files.
-	 * @a this and @a src must have childs named @a name.
-	 */
-	void CopyFilteredChildrenOfChild(const CParamNode& src, const char* name, const std::set<std::string>& permitted);
-
-	/**
 	 * Returns the (unique) child node with the given name, or a node with IsOk() == false if there is none.
 	 */
 	const CParamNode& GetChild(const char* name) const;
diff --git a/source/simulation2/tests/test_ParamNode.h b/source/simulation2/tests/test_ParamNode.h
index d329424..fc9a1ed 100644
--- a/source/simulation2/tests/test_ParamNode.h
+++ b/source/simulation2/tests/test_ParamNode.h
@@ -141,6 +141,11 @@ public:
 		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node, "<test> <a><b/></a> <c>toberemoved</c> <d><e/></d> </test>"), PSRETURN_OK);
 		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node, "<test filtered=\"\"> <a/> <d><f/></d> <g/> </test>"), PSRETURN_OK);
 		TS_ASSERT_WSTR_EQUALS(node.ToXML(), L"<test><a><b></b></a><d><e></e><f></f></d><g></g></test>");
+
+		CParamNode node2;
+		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node2, "<test> <a><b>b</b><c>c</c><d>d</d><e>e</e></a> <f/> </test>"), PSRETURN_OK);
+		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node2, "<test filtered=\"\"> <a filtered=\"\"><b merge=\"\"/><c>c2</c><d/></a> </test>"), PSRETURN_OK);
+		TS_ASSERT_WSTR_EQUALS(node2.ToXML(), L"<test><a><b>b</b><c>c2</c><d></d></a></test>");
 	}
 
 	void test_overlay_merge()
@@ -151,6 +156,14 @@ public:
 		TS_ASSERT_WSTR_EQUALS(node.ToXML(), L"<test><a><b>test</b><c>bar</c><d>baz</d></a><x><w>more text</w><y><v>text</v><z>foo</z></y></x></test>");
 	}
 
+	void test_overlay_filtered_merge()
+	{
+		CParamNode node;
+		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node, "<test> <a><b/></a> <c><x/></c> <Health><Max>1200</Max></Health> </test>"), PSRETURN_OK);
+		TS_ASSERT_EQUALS(CParamNode::LoadXMLString(node, "<test filtered=\"\"> <c merge=\"\"/> <d>bar</d> <e merge=\"\"/> <Health><Initial>1</Initial></Health> </test>"), PSRETURN_OK);
+		TS_ASSERT_WSTR_EQUALS(node.ToXML(), L"<test><Health><Initial>1</Initial><Max>1200</Max></Health><c><x></x></c><d>bar</d></test>");
+	}
+
 	void test_types()
 	{
 		CParamNode node;
-- 
2.8.0

