#5108: Re-implement the Map::saveAs algorithm to create a new resourc…

…e instance, since the existing resource might be a read-only one. Unit tests extended.
codereader · Nov 29, 2020 · 390e11e · 390e11e
1 parent f152515
commit 390e11e
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 29 deletions.
diff --git a/include/imapresource.h b/include/imapresource.h
@@ -80,6 +80,10 @@ class IMapResource
 
     virtual const scene::IMapRootNodePtr& getRootNode() = 0;
 
+    // Sets the root node of this resource. The use case is to create a resource
+    // around an existing map root for e.g. saving a read-only resource to a new path
+    virtual void setRootNode(const scene::IMapRootNodePtr& root) = 0;
+
     virtual void clear() = 0;
 };
 typedef std::shared_ptr<IMapResource> IMapResourcePtr;

diff --git a/radiantcore/map/Map.cpp b/radiantcore/map/Map.cpp
@@ -548,37 +548,39 @@ bool Map::saveAs()
 {
     if (_saveInProgress) return false; // safeguard
 
-    MapFileSelection fileInfo =
-        MapFileManager::getMapFileSelection(false, _("Save Map"), filetype::TYPE_MAP, getMapName());
-
-	if (!fileInfo.fullPath.empty())
-	{
-        // Remember the old name, we might need to revert
-        std::string oldFilename = _mapName;
+    auto fileInfo = MapFileManager::getMapFileSelection(false, 
+        _("Save Map"), filetype::TYPE_MAP, getMapName());
 
-        // Rename the file and try to save
-        rename(fileInfo.fullPath);
+    if (fileInfo.fullPath.empty())
+    {
+        // Invalid filename entered, return false
+        return false;
+    }
 
-        // Try to save the file, this might fail
-		bool success = save(fileInfo.mapFormat);
+    // Remember the old resource, we might need to revert
+    auto oldResource = _resource;
 
-        if (success)
-		{
-            GlobalMRU().insert(fileInfo.fullPath);
-        }
-        else if (!success)
-		{
-            // Revert the name change if the file could not be saved
-            rename(oldFilename);
-        }
+    // Create a new resource pointing to the given path...
+    _resource = GlobalMapResourceManager().createFromPath(fileInfo.fullPath);
+
+    // ...and import the existing root node into that resource
+    _resource->setRootNode(oldResource->getRootNode());
 
-        return success;
-    }
-    else
-	{
-        // Invalid filename entered, return false
+    // Try to save the resource, this might fail
+    if (!save(fileInfo.mapFormat))
+    {
+        // Failure, revert the change
+        _resource = oldResource;
         return false;
     }
+
+    // Resource save was successful, notify about this name change
+    rename(fileInfo.fullPath);
+
+    // add an MRU entry on success
+    GlobalMRU().insert(fileInfo.fullPath);
+
+    return true;
 }
 
 void Map::saveCopyAs()
@@ -841,12 +843,15 @@ void Map::saveSelectedAsPrefab(const cmd::ArgumentList& args)
     }
 }
 
-void Map::rename(const std::string& filename) {
-    if (_mapName != filename) {
+void Map::rename(const std::string& filename)
+{
+    if (_mapName != filename)
+    {
         setMapName(filename);
         SceneChangeNotify();
     }
-    else {
+    else
+    {
         _resource->save();
         setModified(false);
     }

diff --git a/radiantcore/map/MapResource.cpp b/radiantcore/map/MapResource.cpp
@@ -226,6 +226,11 @@ const scene::IMapRootNodePtr& MapResource::getRootNode()
 	return _mapRoot;
 }
 
+void MapResource::setRootNode(const scene::IMapRootNodePtr& root)
+{
+    _mapRoot = root;
+}
+
 void MapResource::clear()
 {
     _mapRoot = std::make_shared<RootNode>("");

diff --git a/radiantcore/map/MapResource.h b/radiantcore/map/MapResource.h
@@ -43,6 +43,7 @@ class MapResource :
 	virtual void save(const MapFormatPtr& mapFormat = MapFormatPtr()) override;
 
 	virtual const scene::IMapRootNodePtr& getRootNode() override;
+    virtual void setRootNode(const scene::IMapRootNodePtr& root) override;
     virtual void clear() override;
 
 	// Save the map contents to the given filename using the given MapFormat export module

diff --git a/test/MapSavingLoading.cpp b/test/MapSavingLoading.cpp
@@ -883,6 +883,9 @@ TEST_F(MapSavingTest, saveArchivedMapWillAskForFilename)
     checkAltarScene();
 
     bool eventFired = false;
+    fs::path outputPath = _context.getTemporaryDataPath();
+    outputPath /= "altar_packed_copy.map";
+    auto format = GlobalMapFormatManager().getMapFormatForFilename(outputPath.string());
 
     // Subscribe to the event asking for the target path
     auto msgSubscription = GlobalRadiantCore().getMessageBus().addListener(
@@ -891,14 +894,26 @@ TEST_F(MapSavingTest, saveArchivedMapWillAskForFilename)
             [&](radiant::FileSelectionRequest& msg)
     {
         eventFired = true;
-        msg.setHandled(false);
+        msg.setHandled(true);
+        msg.setResult(radiant::FileSelectionRequest::Result
+        {
+            outputPath.string(),
+            format->getMapFormatName()
+        });
     }));
 
     // This should ask the user for a file location, i.e. fire the above event
     GlobalCommandSystem().executeCommand("SaveMap");
 
     EXPECT_TRUE(eventFired) << "Radiant didn't ask for a new filename when saving an archived map";
 
+    eventFired = false;
+
+    // Save again, this should no longer ask for a location
+    GlobalCommandSystem().executeCommand("SaveMap");
+
+    EXPECT_FALSE(eventFired);
+
     GlobalRadiantCore().getMessageBus().removeListener(msgSubscription);
 }