widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.