Fix [store_unit]'s stored coordinates for recalls

Units on the recall list might have x,y coordinates that are on the map, which
therefore need to be replaced with "recall,recall" within [store_unit]. The
existing code created a temporary variable, changed the coordinates, and then
returned the unchanged original instead of the temporary.

Add a new test that `[put_to_recall_list]` followed by `[modify_unit]`
doesn't move the unit back to the map.

data/lua/wml-tags.lua

@@ -403,7 +403,7 @@ function wml_actions.store_unit(cfg)
 			ucfg.x = 'recall'
 			ucfg.y = 'recall'
 		end
-		utils.vwriter.write(writer, u.__cfg)
+		utils.vwriter.write(writer, ucfg)
 		if kill_units then u:erase() end
 	end
 end

data/test/scenarios/put_to_recall_and_modify.cfg

@@ -0,0 +1,90 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [put_to_recall_list], [modify_unit], [role][auto_recall]
+##
+# Actions:
+# Create Charlie, belonging to side 1, on the map.
+# Repose Charlie on the recall list.
+# Assign Charlie to side 2.
+# Recall Charlie next to his side's leader.
+##
+# Expected end state:
+# Charlie is on the map next to side 2's leader.
+#####
+
+# This is almost the same as the modify_unit_which_recall_list test, except that
+# creating Charlie on the map can leave an on-map x,y pair in Charlie's data,
+# which needs to be handled within the WML API.
+{GENERIC_UNIT_TEST "put_to_recall_and_modify" (
+    [event]
+        name = start
+
+        # Create a unit on the map, belonging to side 1.
+        [unit]
+            x,y=2,2
+            type=Orcish Grunt
+            side=1
+            id=Charlie
+            name=_"Charlie"
+            role=TestSubject
+            canrecruit=no
+        [/unit]
+
+        [put_to_recall_list]
+            id=Charlie
+        [/put_to_recall_list]
+
+        # Check Charlie has moved to the recall list. Using count=0 is equivalent to putting a
+        # [not] around the [have_unit] - using two different but equivalent syntaxes makes it
+        # easier to work out which of the two asserts failed.
+        {ASSERT (
+            [have_unit]
+                count=0
+                id=Charlie
+                search_recall_list=no
+            [/have_unit]
+        )}
+
+        [modify_unit]
+            side=2
+            [filter]
+                id=Charlie
+            [/filter]
+        [/modify_unit]
+
+        # Check that Charlie is still on the recall list, that modifying him
+        # hasn't moved him back to the map.
+        {ASSERT (
+            [not]
+                [have_unit]
+                    id=Charlie
+                    search_recall_list=no
+                [/have_unit]
+            [/not]
+        )}
+
+        [role]
+            role=TestSubject
+            reassign=no
+            [auto_recall][/auto_recall]
+        [/role]
+
+        # Check that Charlie has been recalled next to side 2's leader
+        #
+        # Currently [role][auto-recall] uses the recall list (not the side attribute)
+        # to determine where on the map to recall the unit; if this changes then this
+        # test could pass with a false positive.
+        {ASSERT (
+            [have_unit]
+                id=Charlie
+                [filter_location]
+                    location_id=2
+                    radius=1
+                [/filter_location]
+            [/have_unit]
+        )}
+
+        {SUCCEED}
+    [/event]
+)}

data/test/scenarios/test_modify_unit.cfg

@@ -1,8 +1,21 @@
-# wmllint: no translatables
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [modify_unit] on the recall list, [role][auto_recall]
+##
+# Actions:
+# Create Charlie, belonging to side 1, on the recall list.
+# Assign Charlie to side 2.
+# Recall Charlie next to his side's leader.
+##
+# Expected end state:
+# Charlie is on the map next to side 2's leader.
+#####
 
 # In 1.14 and before, the implementation of [modify_unit] always stored and
-# unstored the unit. 1.16 introduced a "fast-path" which optimised setting some
-# attributes, but which changed the behavior of the API in some edge cases.
+# unstored the unit. 1.15.0 introduced a "fast-path" which optimised setting
+# some attributes, but which changed the behavior of the API in some edge
+# cases; 1.16.0 disabled that fast-path.
 #
 # The store and unstore also ensured that the animation code noticed the
 # change, and the fast path meant that changes for facing or low hitpoints
@@ -23,6 +36,7 @@
             type=Orcish Grunt
             side=1
             id=Charlie
+            name=_"Charlie"
             role=TestSubject
             canrecruit=no
         [/unit]
@@ -34,6 +48,17 @@
             [/filter]
         [/modify_unit]
 
+        # Check that Charlie is still on the recall list, that modifying him
+        # hasn't moved him back to the map.
+        {ASSERT (
+            [not]
+                [have_unit]
+                    id=Charlie
+                    search_recall_list=no
+                [/have_unit]
+            [/not]
+        )}
+
         [role]
             role=TestSubject
             reassign=no

wml_test_schedule

@@ -109,6 +109,7 @@
 0 replace_schedule_prestart
 0 modify_unit_facing
 0 modify_unit_which_recall_list
+0 put_to_recall_and_modify
 0 event_handlers_in_events_1
 0 event_handlers_in_events_3
 0 event_handlers_in_events_2