#5408: The MapResource class no longer calls the internal GlobalMap()…

… singleton to notify about status changes - it could be any map root node firing the change signal, not just the one loaded as primary map. The MapResource now offers a "modified status changed" signal which is subscribed by the main Map class owning it. As another refactoring step, the UndoFileChangeTracker is no longer using a single callback functor, since this can easily overridden if two MapResources are sharing the same root node (which is a valid use case during renaming).

include/imapresource.h

@@ -86,8 +86,12 @@ class IMapResource
 
     virtual void clear() = 0;
 
-    // Check if the file has been modified since it was last saved
-    virtual bool fileHasBeenModifiedSinceLastSave() = 0;
+    // Check if the target file on disk has been modified since it was last saved
+    virtual bool fileOnDiskHasBeenModifiedSinceLastSave() = 0;
+
+    // A signal that is emitted as soon as the map modified status changes due
+    // to a regular edit, undo or redo. The bool value corresponds to the new modification state.
+    virtual sigc::signal<void(bool)>& signal_modifiedStatusChanged() = 0;
 };
 typedef std::shared_ptr<IMapResource> IMapResourcePtr;
 

include/mapfile.h

@@ -1,7 +1,7 @@
 #pragma once
 
 #include "inode.h"
-#include <functional>
+#include <sigc++/signal.h>
 
 /**
  * The file change tracker monitors the changes made to a single map with the help
@@ -18,6 +18,6 @@ class IMapFileChangeTracker
 
     virtual void save() = 0;
     virtual bool saved() const = 0;
-    virtual void setChangedCallback(const std::function<void()>& changed) = 0;
+    virtual sigc::signal<void()>& signal_changed() = 0;
     virtual std::size_t changes() const = 0;
 };

libs/UndoFileChangeTracker.h

@@ -3,7 +3,7 @@
 #include "iundo.h"
 #include "mapfile.h"
 #include <limits>
-#include <functional>
+#include <sigc++/signal.h>
 
 class UndoFileChangeTracker :
     public IUndoSystem::Tracker,
@@ -14,7 +14,7 @@ class UndoFileChangeTracker :
 
 	std::size_t _size;
 	std::size_t _saved;
-	std::function<void()> _changed;
+    sigc::signal<void()> _changed;
 
 public:
 	UndoFileChangeTracker() :
@@ -25,19 +25,13 @@ class UndoFileChangeTracker :
 	void push() 
     {
         ++_size;
-        if (_changed)
-        {
-            _changed();
-        }
+        _changed.emit();
 	}
 
 	void pop()
     {
         --_size;
-        if (_changed)
-        {
-            _changed();
-        }
+        _changed.emit();
 	}
 
 	void pushOperation()
@@ -53,20 +47,13 @@ class UndoFileChangeTracker :
 	void clear() override
     {
 		_size = 0;
-
-        if (_changed)
-        {
-            _changed();
-        }
+        _changed.emit();
 	}
 
     void save() override 
     {
 		_saved = _size;
-        if (_changed)
-        {
-            _changed();
-        }
+        _changed.emit();
 	}
 
     // Returns true if the current undo history position corresponds to the most recently saved state
@@ -75,14 +62,9 @@ class UndoFileChangeTracker :
 		return _saved == _size;
 	}
 
-    void setChangedCallback(const std::function<void()>& changed) override
+    sigc::signal<void()>& signal_changed() override
     {
-		_changed = changed;
-
-        if (_changed)
-        {
-            _changed();
-        }
+        return _changed;
 	}
 
     std::size_t changes() const override

radiantcore/map/Map.cpp

@@ -92,6 +92,11 @@ void Map::connectToUndoSystem()
 {
     _postUndoListener.disconnect();
     _postRedoListener.disconnect();
+    _modifiedStatusListener.disconnect();
+
+    _modifiedStatusListener = _resource->signal_modifiedStatusChanged().connect(
+        [this](bool newStatus) { setModified(newStatus); }
+    );
 
     if (!_resource->getRootNode()) return;
 
@@ -433,6 +438,7 @@ void Map::freeMap()
     emitMapEvent(MapUnloaded);
 
     // Reset the resource pointer
+    _modifiedStatusListener.disconnect();
     _resource.reset();
 }
 
@@ -568,7 +574,8 @@ bool Map::save(const MapFormatPtr& mapFormat)
     }
 
     // Check if the map file has been modified in the meantime
-    if (_resource->fileHasBeenModifiedSinceLastSave() && !radiant::FileOverwriteConfirmation::SendAndReceiveAnswer(
+    if (_resource->fileOnDiskHasBeenModifiedSinceLastSave() && 
+        !radiant::FileOverwriteConfirmation::SendAndReceiveAnswer(
             fmt::format(_("The file {0} has been modified since it was last saved,\nperhaps by another application. "
                 "Do you really want to overwrite the file?"), _mapName), _("File modification detected")))
     {

radiantcore/map/Map.h

@@ -76,6 +76,7 @@ class Map :
 
     sigc::connection _postUndoListener;
     sigc::connection _postRedoListener;
+    sigc::connection _modifiedStatusListener;
 
     // Point trace for leak detection
     std::unique_ptr<PointFile> _pointTrace;

radiantcore/map/MapResource.cpp

@@ -12,7 +12,6 @@
 #include "iregistry.h"
 #include "imapinfofile.h"
 
-#include "map/Map.h"
 #include "map/RootNode.h"
 #include "mapfile.h"
 #include "gamelib.h"
@@ -237,16 +236,15 @@ const scene::IMapRootNodePtr& MapResource::getRootNode()
 void MapResource::setRootNode(const scene::IMapRootNodePtr& root)
 {
     // Unsubscribe from the old root node first
-    if (_mapRoot)
-    {
-        _mapRoot->getUndoChangeTracker().setChangedCallback(std::function<void()>());
-    }
+    _mapChangeCountListener.disconnect();
 
     _mapRoot = root;
 
     if (_mapRoot)
     {
-        _mapRoot->getUndoChangeTracker().setChangedCallback(std::bind(&MapResource::onMapChanged, this));
+        _mapChangeCountListener = _mapRoot->getUndoChangeTracker().signal_changed().connect(
+            sigc::mem_fun(this, &MapResource::onMapChanged)
+        );
     }
 }
 
@@ -255,16 +253,21 @@ void MapResource::clear()
     setRootNode(std::make_shared<RootNode>(""));
 }
 
-bool MapResource::fileHasBeenModifiedSinceLastSave()
+bool MapResource::fileOnDiskHasBeenModifiedSinceLastSave()
 {
     auto fullPath = getAbsoluteResourcePath();
 
     return os::fileOrDirExists(fullPath) && fs::last_write_time(fullPath) > _lastKnownModificationTime;
 }
 
+sigc::signal<void(bool)>& MapResource::signal_modifiedStatusChanged()
+{
+    return _signalModifiedStatusChanged;
+}
+
 void MapResource::onMapChanged()
 {
-	GlobalMap().setModified(!_mapRoot->getUndoChangeTracker().saved());
+    _signalModifiedStatusChanged.emit(!_mapRoot->getUndoChangeTracker().saved());
 }
 
 void MapResource::mapSave()

radiantcore/map/MapResource.h

@@ -8,6 +8,7 @@
 #include "RootNode.h"
 #include "os/fs.h"
 #include "stream/MapResourceStream.h"
+#include <sigc++/connection.h>
 
 namespace map
 {
@@ -37,6 +38,9 @@ class MapResource :
     // have been modified since the last load time
     fs::file_time_type _lastKnownModificationTime;
 
+    sigc::signal<void(bool)> _signalModifiedStatusChanged;
+    sigc::connection _mapChangeCountListener;
+
 public:
 	// Constructor
 	MapResource(const std::string& resourcePath);
@@ -53,7 +57,9 @@ class MapResource :
     virtual void clear() override;
 
     // Check if the file has been modified since it was last saved
-    virtual bool fileHasBeenModifiedSinceLastSave() override;
+    virtual bool fileOnDiskHasBeenModifiedSinceLastSave() override;
+
+    sigc::signal<void(bool)>& signal_modifiedStatusChanged() override;
 
 	// Save the map contents to the given filename using the given MapFormat export module
 	// Throws an OperationException if anything prevents successful completion

test/UndoRedo.cpp

@@ -610,7 +610,7 @@ TEST_F(UndoTest, MapChangeTracking)
 
     // Two undo steps will mark the map as modified
     GlobalUndoSystem().undo();
-    EXPECT_TRUE(GlobalMapModule().isModified()) << "Map should be modified after 2 undos";
+    EXPECT_TRUE(GlobalMapModule().isModified()) << "Map should be modified after 1 undo";
     GlobalUndoSystem().undo();
     EXPECT_TRUE(GlobalMapModule().isModified()) << "Map should be modified after 2 undos";