From 27109f787d24c9b037d72589b9188a811a909012 Mon Sep 17 00:00:00 2001
From: codereader <greebo@angua.at>
Date: Sun, 17 Oct 2021 09:25:38 +0200
Subject: [PATCH] #5613: Check KeyObserver persistence after undoing an
 operation. Make KeyValue sealed and noncopyable.

---
 radiantcore/entity/KeyValue.h |  4 +++-
 test/Entity.cpp               | 45 +++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/radiantcore/entity/KeyValue.h b/radiantcore/entity/KeyValue.h
index edd1cc4d2a..5b333170cf 100644
--- a/radiantcore/entity/KeyValue.h
+++ b/radiantcore/entity/KeyValue.h
@@ -16,7 +16,7 @@ class SpawnArgs;
 ///
 /// - Notifies observers when value changes - value changes to "" on destruction.
 /// - Provides undo support through the global undo system.
-class KeyValue :
+class KeyValue final :
 	public EntityKeyValue,
 	public sigc::trackable
 {
@@ -34,6 +34,8 @@ class KeyValue :
 
 public:
 	KeyValue(SpawnArgs& owner, const std::string& value, const std::string& empty);
+    KeyValue(const KeyValue& other) = delete;
+    KeyValue& operator=(const KeyValue& other) = delete;
 
 	~KeyValue();
 
diff --git a/test/Entity.cpp b/test/Entity.cpp
index 53da9382de..912a637d59 100644
--- a/test/Entity.cpp
+++ b/test/Entity.cpp
@@ -1396,6 +1396,51 @@ TEST_F(EntityTest, KeyObserverValueChange)
     EXPECT_EQ(observer.receivedValue, "") << "Observer didn't get the expected empty value";
 }
 
+// Check that an KeyObserver stays attached to the key value after Undo
+TEST_F(EntityTest, KeyObserverAttachedAfterUndo)
+{
+    auto guardNode = createByClassName("atdm:ai_builder_guard");
+    scene::addNodeToContainer(guardNode, GlobalMapModule().getRoot());
+    auto guard = Node_getEntity(guardNode);
+
+    constexpr const char* NewKeyName = "New_Unique_Key";
+    constexpr const char* NewKeyValue = "New_Unique_Value";
+    guard->setKeyValue(NewKeyName, NewKeyValue);
+
+    TestKeyObserver observer;
+    EntityKeyValue* keyValue = findKeyValue(guard, NewKeyName);
+    EXPECT_TRUE(keyValue != nullptr) << "Could not locate the key value";
+
+    // Monitor this new key
+    keyValue->attach(observer);
+    observer.reset();
+
+    // Open an undoable transaction and change that keyvalue
+    constexpr const char* SomeOtherValue = "SomeOtherValue";
+    {
+        UndoableCommand cmd("changeKeyValue");
+        guard->setKeyValue(NewKeyName, SomeOtherValue);
+    }
+
+    EXPECT_TRUE(observer.hasBeenInvoked) << "Observer didn't get notified on change";
+    EXPECT_EQ(observer.receivedValue, SomeOtherValue) << "Observer didn't get the correct value";
+
+    // Reset and hit Undo to revert the changed value
+    GlobalUndoSystem().undo();
+
+    // Key should be reverted now
+    EXPECT_EQ(guard->getKeyValue(NewKeyName), NewKeyValue);
+
+    // Reset the observer and check whether it still receives messages
+    observer.reset();
+    guard->setKeyValue(NewKeyName, SomeOtherValue);
+
+    EXPECT_TRUE(observer.hasBeenInvoked) << "Observer didn't get notified on assign";
+    EXPECT_EQ(observer.receivedValue, SomeOtherValue) << "Observer didn't get the correct value";
+
+    keyValue->detach(observer);
+}
+
 // KeyObserver doesn't get called when a key is removed entirely from the SpawnArgs
 TEST_F(EntityTest, KeyObserverKeyRemoval)
 {