#5613: Add an additional callback line between SpawnArgs and the owne…

…d KeyValue instance - once the instance gets a new value assigned (in whatever way) the SpawnArgs instance can trigger its notifyChange() method, which didn't happen before.

radiantcore/entity/KeyValue.cpp

@@ -6,14 +6,13 @@
 namespace entity 
 {
 
-KeyValue::KeyValue(SpawnArgs& owner, const std::string& value, const std::string& empty) :
-    _owner(owner),
+KeyValue::KeyValue(const std::string& value, const std::string& empty, 
+                   const std::function<void(const std::string&)>& valueChanged) :
     _value(value),
     _emptyValue(empty),
-    _undo(_value, std::bind(&KeyValue::importState, this, std::placeholders::_1), "KeyValue")
-{
-	notify();
-}
+    _undo(_value, std::bind(&KeyValue::importState, this, std::placeholders::_1), "KeyValue"),
+    _valueChanged(valueChanged)
+{}
 
 KeyValue::~KeyValue()
 {
@@ -30,7 +29,8 @@ void KeyValue::disconnectUndoSystem(IMapFileChangeTracker& changeTracker)
     _undo.disconnectUndoSystem(changeTracker);
 }
 
-void KeyValue::attach(KeyObserver& observer) {
+void KeyValue::attach(KeyObserver& observer)
+{
 	// Store the observer
 	_observers.push_back(&observer);
 
@@ -42,19 +42,24 @@ void KeyValue::detach(KeyObserver& observer)
 {
 	observer.onKeyValueChanged(_emptyValue);
 
-	KeyObservers::iterator found = std::find(_observers.begin(), _observers.end(), &observer);
-	if (found != _observers.end()) {
+	auto found = std::find(_observers.begin(), _observers.end(), &observer);
+
+	if (found != _observers.end())
+    {
 		_observers.erase(found);
 	}
 }
 
-const std::string& KeyValue::get() const {
+const std::string& KeyValue::get() const
+{
 	// Return the <empty> string if the actual value is ""
 	return (_value.empty()) ? _emptyValue : _value;
 }
 
-void KeyValue::assign(const std::string& other) {
-	if (_value != other) {
+void KeyValue::assign(const std::string& other)
+{
+	if (_value != other)
+    {
 		_undo.save();
 		_value = other;
 		notify();
@@ -66,22 +71,24 @@ void KeyValue::notify()
 	// Store the name locally, to avoid string-copy operations in the loop below
 	const std::string& value = get();
 
-	KeyObservers::reverse_iterator i = _observers.rbegin();
-	while(i != _observers.rend()) {
-		(*i++)->onKeyValueChanged(value);
+    // Notify the owning SpawnArgs instance
+    _valueChanged(value);
+
+	for (auto i = _observers.rbegin(); i != _observers.rend(); ++i)
+    {
+		(*i)->onKeyValueChanged(value);
 	}
 }
 
 void KeyValue::importState(const std::string& string) 
 {
-	// Add ourselves to the Undo event observers, to get notified after all this has been finished
+	// We notify our observers after the entire undo rollback is done
 	_undoHandler = GlobalUndoSystem().signal_postUndo().connect(
 		sigc::mem_fun(this, &KeyValue::onUndoRedoOperationFinished));
 	_redoHandler = GlobalUndoSystem().signal_postRedo().connect(
 		sigc::mem_fun(this, &KeyValue::onUndoRedoOperationFinished));
 
 	_value = string;
-	notify();
 }
 
 void KeyValue::onUndoRedoOperationFinished()

radiantcore/entity/KeyValue.h

@@ -21,8 +21,6 @@ class KeyValue final :
 	public sigc::trackable
 {
 private:
-    SpawnArgs& _owner;
-
 	typedef std::vector<KeyObserver*> KeyObservers;
 	KeyObservers _observers;
 
@@ -32,8 +30,12 @@ class KeyValue final :
 	sigc::connection _undoHandler;
 	sigc::connection _redoHandler;
 
+    // This is a specialised callback pointing to the owning SpawnArgs
+    std::function<void(const std::string&)> _valueChanged;
+
 public:
-	KeyValue(SpawnArgs& owner, const std::string& value, const std::string& empty);
+	KeyValue(const std::string& value, const std::string& empty, const std::function<void(const std::string&)>& valueChanged);
+
     KeyValue(const KeyValue& other) = delete;
     KeyValue& operator=(const KeyValue& other) = delete;
 

radiantcore/entity/SpawnArgs.cpp

@@ -71,14 +71,9 @@ void SpawnArgs::importState(const KeyValues& keyValues)
 		erase(_keyValues.begin());
 	}
 
-	/* greebo: This code somehow doesn't delete all the keys (only every second one)
-	for(KeyValues::iterator i = _keyValues.begin(); i != _keyValues.end();) {
-		erase(i++);
-	}*/
-
-	for (KeyValues::const_iterator i = keyValues.begin(); i != keyValues.end(); ++i)
+	for (const auto& pair : keyValues)
 	{
-		insert(i->first, i->second);
+		insert(pair.first, pair.second);
 	}
 }
 
@@ -303,20 +298,20 @@ void SpawnArgs::insert(const std::string& key, const std::string& value)
 
     if (i != _keyValues.end())
     {
-        // Key has been found
+        // Key has been found, assign the value
         i->second->assign(value);
-
-        // Notify observers of key change, using the found key as argument
-        // as the case of the incoming "key" might be different
-        notifyChange(i->first, value);
+        // Observer notification happens through the lambda callback we passed 
+        // to the KeyValue constructor
     }
 	else
 	{
 		// No key with that name found, create a new one
 		_undo.save();
 
 		// Allocate a new KeyValue object and insert it into the map
-		insert(key, std::make_shared<KeyValue>(*this, value, _eclass->getAttribute(key).getValue()));
+        // Capture the key by value in the lambda
+		insert(key, std::make_shared<KeyValue>(value, _eclass->getAttribute(key).getValue(),
+            [key, this](const std::string& value) { notifyChange(key, value); }));
 	}
 }
 

test/Entity.cpp

@@ -1234,6 +1234,7 @@ TEST_F(EntityTest, EntityObserverUndoRedo)
     constexpr const char* NewValue2 = "New_Unique_Value2";
     constexpr const char* NameKey = "name";
     constexpr const char* NewNameValue = "Ignazius";
+    auto originalName = guard->getKeyValue(NameKey);
 
     TestEntityObserver observer;
 
@@ -1293,8 +1294,10 @@ TEST_F(EntityTest, EntityObserverUndoRedo)
             "Erase stack doesn't have the expected kv " << pair.first << " = " << pair.second;
     }
 
-    // Everything else should be silent
-    EXPECT_TRUE(observer.changeStack.empty()) << "Change stack should be clean";
+    // The single key value change triggered one key value change notification
+    EXPECT_EQ(observer.changeStack.size(), 1) << "Change stack should just contain the single keyvalue change";
+    EXPECT_TRUE(stackHasKeyValuePair(observer.changeStack, NameKey, originalName))
+        << "Change stack should just contain the single keyvalue change";
 
     // REDO
     observer.reset();
@@ -1324,8 +1327,10 @@ TEST_F(EntityTest, EntityObserverUndoRedo)
             "Erase stack doesn't have the expected kv " << pair.first << " = " << pair.second;
     }
 
-    // Everything else should be silent
-    EXPECT_TRUE(observer.changeStack.empty()) << "Change stack should be clean";
+    // The single key value change triggered one key value change notification
+    EXPECT_EQ(observer.changeStack.size(), 1) << "Change stack should just contain the single keyvalue change";
+    EXPECT_TRUE(stackHasKeyValuePair(observer.changeStack, NameKey, NewNameValue))
+        << "Change stack should just contain the single keyvalue change";
 
     guard->detachObserver(&observer);
 }
@@ -1352,9 +1357,8 @@ TEST_F(EntityTest, EntityObserverUndoSingleKeyValue)
         guard->setKeyValue(NewKey, SomeOtherValue);
     }
 
-    observer.reset();
-
     // UNDO
+    observer.reset();
     GlobalUndoSystem().undo();
 
     EXPECT_EQ(guard->getKeyValue(NewKey), NewValue) << "Key value not reverted properly";