From 390e11e66fca42ef4ea5d9ffdfb494525571ca7f Mon Sep 17 00:00:00 2001 From: codereader Date: Sun, 29 Nov 2020 14:05:17 +0100 Subject: [PATCH] #5108: Re-implement the Map::saveAs algorithm to create a new resource instance, since the existing resource might be a read-only one. Unit tests extended. --- include/imapresource.h | 4 +++ radiantcore/map/Map.cpp | 61 ++++++++++++++++++--------------- radiantcore/map/MapResource.cpp | 5 +++ radiantcore/map/MapResource.h | 1 + test/MapSavingLoading.cpp | 17 ++++++++- 5 files changed, 59 insertions(+), 29 deletions(-) diff --git a/include/imapresource.h b/include/imapresource.h index 5b9829e456..7ff74b13c2 100644 --- 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 IMapResourcePtr; diff --git a/radiantcore/map/Map.cpp b/radiantcore/map/Map.cpp index d466aa6c80..60702f3aef 100644 --- 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 index 71fbf14797..4d317aa00b 100644 --- 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(""); diff --git a/radiantcore/map/MapResource.h b/radiantcore/map/MapResource.h index bb3afc246a..35f3bbd7d0 100644 --- 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 index 45bf70ec4d..fc510c9d1d 100644 --- 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,7 +894,12 @@ 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 @@ -899,6 +907,13 @@ TEST_F(MapSavingTest, saveArchivedMapWillAskForFilename) 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); }