Fix [special_note] duplication when unstoring units

The movetype's special notes were becoming a new note for the individual unit.

Clean up the documentation on some of movetype's functions, as they had
documentation in both the .hpp and the .cpp.

changelog_entries/special_note_duplication.md

@@ -0,0 +1,2 @@
+ ### Miscellaneous and Bug Fixes
+   * Fixed special notes being duplicated when storing units (issue #7153).

data/test/scenarios/special_note_from_movetype.cfg

@@ -0,0 +1,43 @@
+# wmllint: no translatables
+
+#####
+# API(s) being tested: [movetype][special_note],[store_unit],[unstore_unit]
+##
+# Actions:
+# Alice is a Spectre, a unit whose 'undeadspirit' movetype includes a special note.
+# Store Alice
+# Unstore Alice
+# Store Alice
+##
+# Expected end state:
+# Both stored copies of Alice have no special notes, because they are a part of the movetype
+# rather than a part of the unit.
+#####
+{GENERIC_UNIT_TEST "special_note_from_movetype" (
+    [event]
+        name=start
+
+        # The copies are called alice1 and alice2 so it's easy to work out which assert failed.
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=alice1
+        [/store_unit]
+        {ASSERT ({VARIABLE_CONDITIONAL alice1.special_note.length equals 0})}
+
+        [unstore_unit]
+            variable=alice1
+        [/unstore_unit]
+
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=alice2
+        [/store_unit]
+        {ASSERT ({VARIABLE_CONDITIONAL alice2.special_note.length equals 0})}
+
+        {SUCCEED}
+    [/event]
+) SIDE1_LEADER="Spectre"}

data/test/scenarios/special_note_individual_unit.cfg

@@ -0,0 +1,60 @@
+# wmllint: no translatables
+
+#####
+# API(s) being tested: [effect][special_note],[store_unit],unstore_unit]
+##
+# Actions:
+# Alice is a Spectre, a unit whose 'undeadspirit' movetype includes a special note.
+# An object adds an individual special note to Alice.
+# Store Alice
+# Unstore Alice
+# Store Alice
+##
+# Expected end state:
+# Both stored copies of Alice have their individual special note. The movetype special note
+# doesn't get mixed with the individual special note.
+#####
+#ifndef SCHEMA_VALIDATION
+{GENERIC_UNIT_TEST "special_note_individual_unit" (
+    [event]
+        name=start
+
+        [object]
+            [filter]
+                id=alice
+            [/filter]
+            [effect]
+                apply_to=profile
+                [special_note]
+                    # Non-translatable because it's later compared to the result of [store_unit].
+                    note="Haunts the Grey Woods." #wmllint: ignore
+                [/special_note]
+            [/effect]
+        [/object]
+
+        # The copies are called alice1 and alice2 so it's easy to work out which assert failed.
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=alice1
+        [/store_unit]
+        {ASSERT ({VARIABLE_CONDITIONAL alice1.special_note.length equals 1})}
+
+        [unstore_unit]
+            variable=alice1
+        [/unstore_unit]
+
+        [store_unit]
+            [filter]
+                id=alice
+            [/filter]
+            variable=alice2
+        [/store_unit]
+        {ASSERT ({VARIABLE_CONDITIONAL alice2.special_note.length equals 1})}
+        {ASSERT ({VARIABLE_CONDITIONAL alice2.special_note[0].note equals ("Haunts the Grey Woods.")})}
+
+        {SUCCEED}
+    [/event]
+) SIDE1_LEADER="Spectre"}
+#endif

src/movetype.cpp

@@ -868,10 +868,6 @@ bool movetype::has_terrain_defense_caps(const std::set<t_translation::terrain_co
 	return false;
 }
 
-/**
- * Merges the given config over the existing data.
- * If @a overwrite is false, the new values will be added to the old.
- */
 void movetype::merge(const config & new_cfg, bool overwrite)
 {
 	for (const auto & applies_to : movetype::effects) {
@@ -916,10 +912,7 @@ void movetype::merge(const config & new_cfg, const std::string & applies_to, boo
 const std::set<std::string> movetype::effects {"movement_costs",
 	"vision_costs", "jamming_costs", "defense", "resistance"};
 
-/**
- * Writes the movement type data to the provided config.
- */
-void movetype::write(config & cfg) const
+void movetype::write(config& cfg, bool include_notes) const
 {
 	movement_.write(cfg, "movement_costs", false);
 	vision_.write(cfg, "vision_costs", false);
@@ -927,10 +920,12 @@ void movetype::write(config & cfg) const
 	defense_.write(cfg, "defense");
 	resist_.write(cfg, "resistance");
 
-	if ( flying_ )
+	if(flying_)
 		cfg["flying"] = true;
 
-	for(const auto& note : special_notes_) {
-		cfg.add_child("special_note", config{"note", note});
+	if(include_notes) {
+		for(const auto& note : special_notes_) {
+			cfg.add_child("special_note", config{"note", note});
+		}
 	}
 }

src/movetype.hpp

@@ -334,9 +334,12 @@ public:
 	void merge(const config & new_cfg, bool overwrite=true);
 
 	/**
-	 * Merges the given config over the existing data.
-	 * @a applies_to which type of movement to change ("movement_costs", etc)
-	 * @a new_cfg data which could be one of the children of the config for the two-argument form of this function.
+	 * Merges the given config over the existing data; this 3-argument version affects only the
+	 * subelement identified by the @a applies_to argument.
+	 *
+	 * @param applies_to which type of movement to change ("movement_costs", etc)
+	 * @param new_cfg data which could be one of the children of the config for the two-argument form of this function.
+	 * @param overwrite if false, the new values will be added to the old.
 	 */
 	void merge(const config & new_cfg, const std::string & applies_to, bool overwrite=true);
 
@@ -346,8 +349,23 @@ public:
 	/** Contents of any [special_note] tags */
 	const std::vector<t_string>& special_notes() const { return special_notes_; }
 
-	/** Writes the movement type data to the provided config. */
-	void write(config & cfg) const;
+	/**
+	 * Writes the movement type data to the provided config.
+	 *
+	 * There is no default value for the include_notes argument. Given the implied contract that a
+	 * class with a constructor(const config&) and a write(config&) supports round-tripping the
+	 * data, the default would need to be true. However, this method has only two callers, and
+	 * neither of them want to include the notes:
+	 *
+	 * Movetype patching is unaffected by the notes, as they will be ignored by movetype::merge().
+	 *
+	 * [store_unit] is broken by the notes, because they end up in unit::special_notes_ instead of
+	 * movetype::special_notes_ after the subsequent [unstore_unit].
+	 *
+	 * @param cfg output
+	 * @param include_notes if false, omits any special notes
+	 */
+	void write(config& cfg, bool include_notes) const;
 
 private:
 	terrain_info movement_;

src/units/types.cpp

@@ -900,7 +900,7 @@ void patch_movetype(movetype& mt,
 {
 	LOG_CONFIG << "Patching " << new_key << " into movetype." << type_to_patch;
 	config mt_cfg;
-	mt.write(mt_cfg);
+	mt.write(mt_cfg, false);
 
 	if(!replace && mt_cfg.child_or_empty(type_to_patch).has_attribute(new_key)) {
 		// Don't replace if this type already exists in the config

src/units/unit.cpp

@@ -1433,7 +1433,7 @@ void unit::write(config& cfg, bool write_all) const
 	};
 
 	if(write_all || get_attr_changed(UA_MOVEMENT_TYPE)) {
-		movement_type_.write(cfg);
+		movement_type_.write(cfg, false);
 	}
 	if(write_all || get_attr_changed(UA_SMALL_PROFILE)) {
 		cfg["small_profile"] = small_profile_;

wml_test_schedule

@@ -199,6 +199,8 @@
 0 events-test_pre_attack_change_weapon
 0 test_store_unit_defense_on
 9 test_store_unit_defense_deprecated
+0 special_note_from_movetype
+0 special_note_individual_unit
 # Terrain mask tests
 0 test_terrain_mask_simple_nop
 0 test_terrain_mask_simple_set