Code LGTM - just some minor nits.
Diff comments:
>
> === modified file 'src/editor/ui_menus/main_menu_save_map.cc'
> --- src/editor/ui_menus/main_menu_save_map.cc 2018-06-01 08:50:29 +0000
> +++ src/editor/ui_menus/main_menu_save_map.cc 2018-10-22 18:50:51 +0000
> @@ -272,16 +272,14 @@
> if (mbox.run<UI::Panel::Returncodes>() ==
> UI::Panel::Returncodes::kBack)
> return false;
> }
> -
> - // save to a tmp file/dir first, rename later
> - // (important to keep script files in the script directory)
> - const std::string tmp_name = complete_filename + ".tmp";
> - if (g_fs->file_exists(tmp_name)) {
> +
> + // Try deleting file (if it exists). If it fails, give a message and
> let the player choose a new name.
> + try {
> + g_fs->fs_unlink(complete_filename);
> + } catch (const std::exception& e) {
> const std::string s =
> - (boost::format(
> - _("A file with the name ‘%s.tmp’ already exists. You
> have to remove it manually.")) %
> - FileSystem::fs_filename(filename.c_str()))
> - .str();
> + (boost::format(_("File ‘%s.tmp’ could not be deleted.")) %
> FileSystem::fs_filename(filename.c_str())).str()
The .tmp ending is no longer used here
> + + " " + _("Try saving under a different name!");
> UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s,
> UI::WLMessageBox::MBoxType::kOk);
> mbox.run<UI::Panel::Returncodes>();
> return false;
>
> === modified file 'src/logic/editor_game_base.cc'
> --- src/logic/editor_game_base.cc 2018-09-28 05:41:33 +0000
> +++ src/logic/editor_game_base.cc 2018-10-22 18:50:51 +0000
> @@ -67,12 +70,86 @@
> : gametime_(0),
> lua_(lua_interface),
> player_manager_(new PlayersManager(*this)),
> - ibase_(nullptr) {
> + ibase_(nullptr),
> + tmp_fs_(nullptr) {
> if (!lua_) // TODO(SirVer): this is sooo ugly, I can't say
> lua_.reset(new LuaEditorInterface(this));
> }
>
> EditorGameBase::~EditorGameBase() {
> + delete_tempfile();
> +}
> +
> +/**
> + * deletes the temporary file/dir
> + * also resets the map filesystem if it points to the temporary file
> + */
> +void EditorGameBase::delete_tempfile() {
> + if (!tmp_fs_)
Please add {} here. We are trying to avoid dropping optional {}, because that
can lead to bugs.
> + return;
> + std::string fs_filename = tmp_fs_->get_basename();
> + std::string mapfs_filename = map_.filesystem()->get_basename();
> + if (mapfs_filename == fs_filename)
> + map_.reset_filesystem();
> + tmp_fs_.reset();
> + try {
> + g_fs->fs_unlink(fs_filename);
> + } catch (const std::exception& e) {
> + // if file deletion fails then we have an abaondoned file lying
> around, but otherwise that's unproblematic
abaondoned -> abandoned
> + log("EditorGameBase::delete_tempfile: deleting temporary
> file/dir failed: %s\n", e.what());
> + }
> +}
> +
> +/**
> + * creates a new file/dir, saves the map data, and reassigns the map
> filesystem
> + * does not delete the former temp file if one exists
> + * throws an exception if something goes wrong
> + */
> +void EditorGameBase::create_tempfile_and_save_mapdata(FileSystem::Type const
> type) {
> + // should only be called when a map was already loaded
> + assert(map_.filesystem());
> +
> + g_fs->ensure_directory_exists(kTempFileDir);
> +
> + std::string filename = kTempFileDir + g_fs->file_separator() +
> timestring() + "_mapdata";
> + std::string complete_filename = filename + kTempFileExtension;
> +
> + // if a file with that name already exists, then try a few name
> modifications
> + if (g_fs->file_exists(complete_filename))
> + {
> + int suffix;
> + for (suffix = 0; suffix <= 9; suffix++)
> + {
> + complete_filename = filename + "-" +
> std::to_string(suffix) + ".tmp";
> + if (!g_fs->file_exists(complete_filename))
> + break;
> + }
> + if (suffix > 9) {
> + throw
> wexception("EditorGameBase::create_tempfile_and_save_mapdata(): for all
> considered filenames a file already existed");
> + }
> + }
> +
> + // create tmp_fs_
> + tmp_fs_.reset(g_fs->create_sub_file_system(complete_filename, type));
> +
> + // save necessary map data (we actually save the whole map)
> + Widelands::MapSaver* wms = new Widelands::MapSaver(*tmp_fs_, *this);
We're trying to get rid of raw pointers. Try if Widelands::MapSaver*
wms(Widelands::MapSaver(*tmp_fs_, *this)); does the job, and it not, use a
unique_ptr.
> + wms->save();
> + delete wms;
> +
> + // swap map fs
> + std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
> + map_.swap_filesystem(mapfs);
> + mapfs.reset();
> +
> + // This is just a convenience hack:
> + // If tmp_fs_ is a zip filesystem then - because of the way zip
> filesystems are currently implemented -
> + // the file is still in zip mode right now, which means that the file
> isn't finalized yet, i.e.,
> + // not even a valid zip file until zip mode ends. To force ending the
> zip mode (thus finalizing the file)
> + // we simply perform a (otherwise useless) filesystem request.
> + // It's not strictly necessary, but this way we get a valid zip file
> immediately istead of
> + // at some unkown later point (when an unzip operation happens or a
> filesystem object destructs).
> + tmp_fs_->file_exists("binary");
> }
>
> void EditorGameBase::think() {
>
> === modified file 'src/logic/filesystem_constants.h'
> --- src/logic/filesystem_constants.h 2018-04-22 16:01:32 +0000
> +++ src/logic/filesystem_constants.h 2018-10-22 18:50:51 +0000
> @@ -38,6 +38,13 @@
> const std::string kS2MapExtension1 = ".swd";
> const std::string kS2MapExtension2 = ".wld";
>
> +/// Filesystem names for temp files holding static data that needs to be
> accessible via filesystem
> +/// Kept in a separate dir to avoid filesystem conflicts
> +const std::string kTempFileDir = "in_progress";
Why not just call it "temp"?
> +const std::string kTempFileExtension = ".tmp";
> +// We delete (accidentally remaining) temp files older than a week
> +constexpr double kTempFilesKeepAroundTime = 7 * 24 * 60 * 60;
> +
> /// Filesystem names and timeouts for replays
> const std::string kReplayDir = "replays";
> const std::string kReplayExtension = ".wrpl";
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1753230-working-with-tempfiles/+merge/357656
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1753230-working-with-tempfiles into
lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
