Fix map editor not removing an actor properly.

If you edit an actor name, then delete the actor - it fails to be removed from the map in the editor. This is because the actor previews are keyed by ID. Editing their name edits their ID and breaks the stability of their hash code. This unstable hash code means the preview will now fail to be removed from collections, even though it's the "same" object.

Fix this by making the ID immutable to ensure hash stability - this means that a preview can be added and removed from collections successfully. Now when we edit the ID in the UI, we can't update the ID in place on the preview. Instead we must generate a new preview with the correct ID and swap it with the preview currently in use.

OpenRA.Mods.Common/Traits/World/EditorActorPreview.cs

@@ -32,7 +32,7 @@ public class EditorActorPreview : IEquatable<EditorActorPreview>
 
 		public string Type => reference.Type;
 
-		public string ID { get; set; }
+		public string ID { get; }
 		public PlayerReference Owner { get; set; }
 		public WPos CenterPosition { get; set; }
 		public IReadOnlyDictionary<CPos, SubCell> Footprint { get; private set; }
@@ -83,6 +83,11 @@ public EditorActorPreview(WorldRenderer worldRenderer, string id, ActorReference
 			onCellEntryChanged = _ => UpdateFromCellChange();
 		}
 
+		public EditorActorPreview WithId(string id)
+		{
+			return new EditorActorPreview(worldRenderer, id, reference.Clone(), Owner);
+		}
+
 		void UpdateFromCellChange()
 		{
 			CenterPosition = PreviewPosition(worldRenderer.World, reference);

OpenRA.Mods.Common/Widgets/Logic/Editor/ActorEditLogic.cs

@@ -59,6 +59,8 @@ enum ActorIDStatus { Normal = 0, Duplicate = 1, Empty = 3 }
 
 		EditorActorPreview SelectedActor => editor.DefaultBrush.Selection.Actor;
 
+		internal bool IsChangingSelection { get; set; }
+
 		[ObjectCreator.UseCtor]
 		public ActorEditLogic(Widget widget, World world, WorldRenderer worldRenderer, Dictionary<string, MiniYaml> logicArgs)
 		{
@@ -155,9 +157,12 @@ void HandleSelectionChanged()
 		{
 			if (SelectedActor != null)
 			{
-				Reset();
+				// Our edit control is updating the selection to account for an actor ID change.
+				// Don't try and reset, we're the ones who instigated the selection change!
+				if (!IsChangingSelection)
+					Reset();
 
-				editActorPreview = new EditActorPreview(SelectedActor);
+				editActorPreview = new EditActorPreview(this, editor, editorActorLayer, SelectedActor);
 
 				initialActorID = actorIDField.Text = SelectedActor.ID;
 
@@ -382,7 +387,7 @@ void Close()
 
 		void Save()
 		{
-			editorActionManager.Add(new EditActorEditorAction(editorActorLayer, SelectedActor, editActorPreview.GetDirtyHandles()));
+			editorActionManager.Add(new EditActorEditorAction(SelectedActor, editActorPreview.GetDirtyHandles()));
 			editActorPreview = null;
 			Close();
 		}
@@ -408,12 +413,12 @@ public void OnChange(T value)
 			this.value = value;
 		}
 
-		public void Do(EditorActorPreview actor)
+		public void Do(ref EditorActorPreview actor)
 		{
 			change(actor, value);
 		}
 
-		public void Undo(EditorActorPreview actor)
+		public void Undo(ref EditorActorPreview actor)
 		{
 			change(actor, initialValue);
 		}
@@ -424,8 +429,8 @@ public void Undo(EditorActorPreview actor)
 
 	public interface IEditActorHandle
 	{
-		void Do(EditorActorPreview actor);
-		void Undo(EditorActorPreview actor);
+		void Do(ref EditorActorPreview actor);
+		void Undo(ref EditorActorPreview actor);
 		bool IsDirty { get; }
 		bool ShouldDoOnSave { get; }
 	}
@@ -435,52 +440,56 @@ sealed class EditActorEditorAction : IEditorAction
 		[TranslationReference("name", "id")]
 		const string EditedActor = "notification-edited-actor";
 
-		public string Text { get; }
+		[TranslationReference("name", "old-id", "new-id")]
+		const string EditedActorId = "notification-edited-actor-id";
+
+		public string Text { get; private set; }
+		public EditorActorPreview Actor;
 
 		readonly IEnumerable<IEditActorHandle> handles;
-		readonly EditorActorLayer editorActorLayer;
-		EditorActorPreview actor;
-		readonly string actorId;
 
-		public EditActorEditorAction(EditorActorLayer editorActorLayer, EditorActorPreview actor, IEnumerable<IEditActorHandle> handles)
+		public EditActorEditorAction(EditorActorPreview actor, IEnumerable<IEditActorHandle> handles)
 		{
-			this.editorActorLayer = editorActorLayer;
-			actorId = actor.ID;
-			this.actor = actor;
+			Actor = actor;
 			this.handles = handles;
 			Text = TranslationProvider.GetString(EditedActor, Translation.Arguments("name", actor.Info.Name, "id", actor.ID));
 		}
 
 		public void Execute()
 		{
+			var before = Actor;
+
 			foreach (var editorActionHandle in handles.Where(h => h.ShouldDoOnSave))
-				editorActionHandle.Do(actor);
+				editorActionHandle.Do(ref Actor);
+
+			var after = Actor;
+			if (before != after)
+				Text = TranslationProvider.GetString(EditedActorId, Translation.Arguments("name", after.Info.Name, "old-id", before.ID, "new-id", after.ID));
 		}
 
 		public void Do()
 		{
-			actor = editorActorLayer[actorId];
 			foreach (var editorActionHandle in handles)
-				editorActionHandle.Do(actor);
+				editorActionHandle.Do(ref Actor);
 		}
 
 		public void Undo()
 		{
 			foreach (var editorActionHandle in handles)
-				editorActionHandle.Undo(actor);
+				editorActionHandle.Undo(ref Actor);
 		}
 	}
 
 	sealed class EditActorPreview
 	{
-		readonly EditorActorPreview actor;
 		readonly SetActorIdAction setActorIdAction;
 		readonly List<IEditActorHandle> handles = new();
+		EditorActorPreview actor;
 
-		public EditActorPreview(EditorActorPreview actor)
+		public EditActorPreview(ActorEditLogic logic, EditorViewportControllerWidget editor, EditorActorLayer editorActorLayer, EditorActorPreview actor)
 		{
 			this.actor = actor;
-			setActorIdAction = new SetActorIdAction(actor.ID);
+			setActorIdAction = new SetActorIdAction(logic, editor, editorActorLayer, actor.ID);
 			handles.Add(setActorIdAction);
 		}
 
@@ -507,12 +516,15 @@ public IEnumerable<IEditActorHandle> GetDirtyHandles()
 		public void Reset()
 		{
 			foreach (var handle in handles.Where(h => h.IsDirty))
-				handle.Undo(actor);
+				handle.Undo(ref actor);
 		}
 	}
 
 	public class SetActorIdAction : IEditActorHandle
 	{
+		readonly ActorEditLogic logic;
+		readonly EditorViewportControllerWidget editor;
+		readonly EditorActorLayer editorActorLayer;
 		readonly string initial;
 		string newID;
 
@@ -522,19 +534,35 @@ public void Set(string actorId)
 			newID = actorId;
 		}
 
-		public SetActorIdAction(string initial)
+		public SetActorIdAction(ActorEditLogic logic, EditorViewportControllerWidget editor, EditorActorLayer editorActorLayer, string initial)
 		{
+			this.logic = logic;
+			this.editor = editor;
+			this.editorActorLayer = editorActorLayer;
 			this.initial = initial;
 		}
 
-		public void Do(EditorActorPreview actor)
+		public void Do(ref EditorActorPreview actor)
 		{
-			actor.ID = newID;
+			// We can't update the ID of an EditorActorPreview in place - it's the hash and equality key of a preview.
+			// So instead we need to swap in an entirely new preview with the updated ID.
+			// This affects the actor layer, and the current selection.
+			editorActorLayer.Remove(actor);
+			actor = actor.WithId(newID);
+			editorActorLayer.Add(actor);
+			logic.IsChangingSelection = true;
+			editor.DefaultBrush.SetSelection(new EditorSelection { Actor = actor });
+			logic.IsChangingSelection = false;
 		}
 
-		public void Undo(EditorActorPreview actor)
+		public void Undo(ref EditorActorPreview actor)
 		{
-			actor.ID = initial;
+			editorActorLayer.Remove(actor);
+			actor = actor.WithId(initial);
+			editorActorLayer.Add(actor);
+			logic.IsChangingSelection = true;
+			editor.DefaultBrush.SetSelection(new EditorSelection { Actor = actor });
+			logic.IsChangingSelection = false;
 		}
 
 		public bool IsDirty { get; private set; }

mods/common/languages/en.ftl

@@ -848,6 +848,7 @@ mirror-mode =
 
 ## ActorEditLogic
 notification-edited-actor = Edited { $name } ({ $id })
+notification-edited-actor-id = Edited { $name } ({ $old-id }->{ $new-id })
 
 ## ConquestVictoryConditions, StrategicVictoryConditions
 notification-player-is-victorious = { $player } is victorious.