widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

Arty has proposed merging lp:~widelands-dev/widelands/robust-file-saving into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/robust-file-saving/+merge/358718
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/robust-file-saving into lp:widelands.

=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt	2018-07-19 16:43:14 +0000
+++ src/editor/CMakeLists.txt	2018-11-13 16:47:56 +0000
@@ -99,6 +99,7 @@
     logic
     logic_constants
     logic_filesystem_constants
+    logic_generic_save_handler
     logic_tribe_basic_info
     logic_widelands_geometry
     map_io

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2018-11-11 20:42:54 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2018-11-13 16:47:56 +0000
@@ -37,6 +37,7 @@
 #include "io/filesystem/layered_filesystem.h"
 #include "io/filesystem/zip_filesystem.h"
 #include "logic/filesystem_constants.h"
+#include "logic/generic_save_handler.h"
 #include "map_io/map_saver.h"
 #include "map_io/widelands_map_loader.h"
 #include "ui_basic/messagebox.h"
@@ -119,7 +120,9 @@
  * Called when the ok button was pressed or a file in list was double clicked.
  */
 void MainMenuSaveMap::clicked_ok() {
-	assert(ok_.enabled());
+	if (!ok_.enabled()) {
+		return;
+	}
 	std::string filename = editbox_->text();
 	std::string complete_filename;
 
@@ -243,15 +246,12 @@
 
 /**
  * Save the map in the current directory with
- * the current filename
+ * the given filename
  *
  * returns true if dialog should close, false if it
  * should stay open
  */
 bool MainMenuSaveMap::save_map(std::string filename, bool binary) {
-	//  Make sure that the current directory exists and is writeable.
-	g_fs->ensure_directory_exists(curdir_);
-
 	// Trim it for preceding/trailing whitespaces in user input
 	boost::trim(filename);
 
@@ -272,6 +272,7 @@
 		if (mbox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kBack)
 			return false;
 	}
+<<<<<<< TREE
 
 	//  Try deleting file (if it exists). If it fails, give a message and let the player choose a new
 	//  name.
@@ -288,6 +289,8 @@
 		mbox.run<UI::Panel::Returncodes>();
 		return false;
 	}
+=======
+>>>>>>> MERGE-SOURCE
 
 	Widelands::EditorGameBase& egbase = eia().egbase();
 	Widelands::Map* map = egbase.mutable_map();
@@ -305,6 +308,7 @@
 	} else {
 		map->delete_tag("artifacts");
 	}
+<<<<<<< TREE
 
 	//  Try saving.
 	try {
@@ -322,5 +326,38 @@
 	}
 
 	die();
+=======
+
+	// Try saving the map.
+	GenericSaveHandler gsh(
+		[&egbase](FileSystem& fs) {
+			Widelands::MapSaver wms(fs, egbase);
+			wms.save();
+		},
+		complete_filename,
+		binary ? FileSystem::ZIP : FileSystem::DIR
+	);
+	uint32_t error = gsh.save();
+	if (error == GenericSaveHandler::kSuccess ||
+	    error == GenericSaveHandler::kDeletingBackupFailed) {
+			// No need to bother the player if only the temporary backup couldn't
+			// be deleted. Automatic cleanup will try to deal with it later.
+		egbase.get_ibase()->log_message(_("Map saved"));
+		return true;
+	}
+
+	std::string msg = gsh.localized_formatted_result_message();
+	UI::WLMessageBox mbox(
+	   this, _("Error Saving Map!"), msg, UI::WLMessageBox::MBoxType::kOk);
+	mbox.run<UI::Panel::Returncodes>();
+
+	// If only the backup failed (likely just because of a file lock),
+	// then leave the dialog open for the player to try with a new filename.
+	if (error == GenericSaveHandler::kBackupFailed) {
+	  return false;
+	}
+
+	// In the other error cases close the dialog.
+>>>>>>> MERGE-SOURCE
 	return true;
 }

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/disk_filesystem.cc	2018-11-13 16:47:56 +0000
@@ -167,15 +167,13 @@
 }
 
 /**
- * Create a sub filesystem out of this filesystem
+ * Make a sub filesystem out of this filesystem
  */
 FileSystem* RealFSImpl::make_sub_file_system(const std::string& path) {
 	FileSystemPath fspath(canonicalize_name(path));
 
 	if (!fspath.exists_) {
-		throw wexception("RealFSImpl: unable to create sub filesystem, path does not exist for '%s'"
-		                 " in directory '%s'",
-		                 fspath.c_str(), directory_.c_str());
+		throw FileNotFoundError("RealFSImpl::make_sub_file_system", fspath);
 	}
 
 	if (fspath.is_directory_)
@@ -190,10 +188,7 @@
 FileSystem* RealFSImpl::create_sub_file_system(const std::string& path, Type const fs) {
 	FileSystemPath fspath(canonicalize_name(path));
 	if (fspath.exists_)
-		throw wexception(
-		   "path '%s'' already exists in directory '%s', can not create a filesystem from it",
-		   path.c_str(), directory_.c_str());
-
+		throw FileError("RealFSImpl::create_sub_file_system", fspath, "path already exists, cannot create new filesystem from it");
 	if (fs == FileSystem::DIR) {
 		ensure_directory_exists(path);
 		return new RealFSImpl(fspath);
@@ -221,20 +216,22 @@
 void RealFSImpl::unlink_file(const std::string& file) {
 	FileSystemPath fspath(canonicalize_name(file));
 	if (!fspath.exists_) {
-		throw wexception(
-		   "RealFSImpl: unable to unlink file, path does not exist for '%s' in directory '%s'",
-		   fspath.c_str(), directory_.c_str());
+		throw FileNotFoundError("RealFSImpl::unlink_file", fspath);
 	}
 	if (fspath.is_directory_) {
-		throw wexception(
-		   "RealFSImpl: unable to unlink file, path '%s' in directory '%s' is a directory",
-		   fspath.c_str(), directory_.c_str());
+		throw FileTypeError("RealFSImpl::unlink_file", fspath, "path is a directory");
 	}
-
 #ifndef _WIN32
-	unlink(fspath.c_str());
+	if (unlink(fspath.c_str()) == -1)
+		throw FileError("RealFSImpl::unlink_file", fspath, strerror(errno));
 #else
-	DeleteFile(fspath.c_str());
+	// Note: We could possibly replace this with _unlink()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
+	if (!DeleteFile(fspath.c_str()))
+		throw FileError("RealFSImpl::unlink_file", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
 }
 
@@ -244,14 +241,10 @@
 void RealFSImpl::unlink_directory(const std::string& file) {
 	FileSystemPath fspath(canonicalize_name(file));
 	if (!fspath.exists_) {
-		throw wexception("RealFSImpl: unable to unlink directory, path does not exist for '%s'"
-		                 " in directory '%s'",
-		                 fspath.c_str(), directory_.c_str());
+		throw FileNotFoundError("RealFSImpl::unlink_directory", fspath);
 	}
 	if (!fspath.is_directory_) {
-		throw wexception("RealFSImpl: unable to unlink directory, path '%s' in directory '%s'"
-		                 " is not a directory",
-		                 fspath.c_str(), directory_.c_str());
+		throw FileTypeError("RealFSImpl::unlink_directory", fspath, "path is not a directory");
 	}
 
 	FilenameSet files = list_directory(file);
@@ -268,14 +261,18 @@
 			unlink_file(*pname);
 	}
 
-// NOTE: this might fail if this directory contains CVS dir,
-// so no error checking here
+// NOTE: this might fail if this directory contains CVS dir
 #ifndef _WIN32
-	rmdir(fspath.c_str());
+	if (rmdir(fspath.c_str()) == -1)
+		throw FileError("RealFSImpl::unlink_directory", fspath, strerror(errno));
 #else
+	// Note: We could possibly replace this with _rmdir()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
 	if (!RemoveDirectory(fspath.c_str()))
-		throw wexception(
-		   "'%s' could not be deleted in directory '%s'.", fspath.c_str(), directory_.c_str());
+		throw FileError("RealFSImpl::unlink_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
 }
 
@@ -292,25 +289,20 @@
 	boost::replace_all(clean_dirname, "\\", "/");
 #endif
 
-	try {
-		std::string::size_type it = 0;
-		while (it < clean_dirname.size()) {
-			it = clean_dirname.find('/', it);
-
-			FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
-			if (fspath.exists_ && !fspath.is_directory_)
-				throw wexception("'%s' in directory '%s' exists and is not a directory",
-				                 clean_dirname.substr(0, it).c_str(), directory_.c_str());
-			if (!fspath.exists_)
-				make_directory(clean_dirname.substr(0, it));
-
-			if (it == std::string::npos)
-				break;
-			++it;
-		}
-	} catch (const std::exception& e) {
-		throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s",
-		                 clean_dirname.c_str(), directory_.c_str(), e.what());
+	std::string::size_type it = 0;
+	while (it < clean_dirname.size()) {
+		it = clean_dirname.find('/', it);
+
+		FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
+		if (fspath.exists_ && !fspath.is_directory_)
+			throw FileTypeError("RealFSImpl::ensure_directory_exists",
+			   fspath, "path is not a directory");
+		if (!fspath.exists_)
+			make_directory(clean_dirname.substr(0, it));
+
+		if (it == std::string::npos)
+			break;
+		++it;
 	}
 }
 
@@ -318,33 +310,37 @@
  * Create this directory, throw an error if it already exists or
  * if a file is in the way or if the creation fails.
  *
- * Pleas note, this function does not honor parents,
+ * Please note, this function does not honor parents,
  * make_directory("onedir/otherdir/onemoredir") will fail
  * if either onedir or otherdir is missing
  */
 void RealFSImpl::make_directory(const std::string& dirname) {
 	FileSystemPath fspath(canonicalize_name(dirname));
 	if (fspath.exists_)
-		throw wexception("a file/directory with the name '%s' already exists in directory '%s'",
-		                 dirname.c_str(), directory_.c_str());
+		throw FileError("RealFSImpl::make_directory", fspath, "a file or directory with that name already exists");
 
-	if
-#ifdef _WIN32
-	   (!CreateDirectory(fspath.c_str(), NULL))
+#ifndef _WIN32
+	if (!mkdir(fspath.c_str(), 0x1FF) == -1)
+		throw FileError("RealFSImpl::make_directory", fspath, strerror(errno));
 #else
-	   (mkdir(fspath.c_str(), 0x1FF) == -1)
+	// Note: We could possibly replace this with _mkdir()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
+	if (!CreateDirectory(fspath.c_str(), NULL))
+		throw FileError("RealFSImpl::make_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
-		throw DirectoryCannotCreateError("RealFSImpl::make_directory", dirname, strerror(errno));
 }
 
 /**
- * Read the given file into alloced memory; called by FileRead::open.
+ * Read the given file into allocated memory; called by FileRead::open.
  * Throws an exception if the file couldn't be opened.
  */
 void* RealFSImpl::load(const std::string& fname, size_t& length) {
 	const std::string fullname = canonicalize_name(fname);
 	if (is_directory(fullname)) {
-		throw FileError("RealFSImpl::load", fullname);
+		throw FileTypeError("RealFSImpl::load", fullname, "path is a directory");
 	}
 
 	FILE* file = nullptr;
@@ -353,7 +349,7 @@
 	try {
 		file = fopen(fullname.c_str(), "rb");
 		if (!file)
-			throw FileError("RealFSImpl::load", fullname);
+			throw FileError("RealFSImpl::load", fullname, "could not open file for reading");
 
 		// determine the size of the file (rather quirky, but it doesn't require
 		// potentially unportable functions)
@@ -371,6 +367,9 @@
 
 		// allocate a buffer and read the entire file into it
 		data = malloc(size + 1);  //  TODO(unknown): memory leak!
+		if (!data)
+			throw wexception("RealFSImpl::load: memory allocation failed for reading file %s (%s) with size %" PRIuS "",
+			                 fname.c_str(), fullname.c_str(), size);
 		int result = fread(data, size, 1, file);
 		if (size && (result != 1)) {
 			throw wexception("RealFSImpl::load: read failed for %s (%s) with size %" PRIuS "",
@@ -383,10 +382,10 @@
 
 		length = size;
 	} catch (...) {
-		if (file) {
+		if (file)
 			fclose(file);
-		}
-		free(data);
+		if (data)
+			free(data);
 		throw;
 	}
 
@@ -410,7 +409,7 @@
 
 	FILE* const f = fopen(fullname.c_str(), append ? "a" : "wb");
 	if (!f)
-		throw wexception("could not open %s (%s) for writing", fname.c_str(), fullname.c_str());
+		throw FileError("RealFSImpl::write", fullname, "could not open file for writing");
 
 	size_t const c = fwrite(data, length, 1, f);
 	fclose(f);
@@ -424,8 +423,7 @@
 	const std::string fullname1 = canonicalize_name(old_name);
 	const std::string fullname2 = canonicalize_name(new_name);
 	if (rename(fullname1.c_str(), fullname2.c_str()) != 0)
-		throw wexception("DiskFileSystem: unable to rename %s to %s: %s", fullname1.c_str(),
-		                 fullname2.c_str(), strerror(errno));
+		throw FileError("RealFSImpl::fs_rename", fullname1, std::string("unable to rename file to ") + fullname2 + ", " + strerror(errno));
 }
 
 /*****************************************************************************
@@ -439,7 +437,7 @@
 struct RealFSStreamRead : public StreamRead {
 	explicit RealFSStreamRead(const std::string& fname) : file_(fopen(fname.c_str(), "rb")) {
 		if (!file_)
-			throw wexception("could not open %s for reading", fname.c_str());
+			throw FileError("RealFSStreamRead::RealFSStreamRead", fname, "could not open file for reading");
 	}
 
 	~RealFSStreamRead() override {
@@ -477,7 +475,7 @@
 	explicit RealFSStreamWrite(const std::string& fname) : filename_(fname) {
 		file_ = fopen(fname.c_str(), "wb");
 		if (!file_)
-			throw wexception("could not open %s for writing", fname.c_str());
+			throw FileError("RealFSStreamWrite::RealFSStreamWrite", fname, "could not open file for writing");
 	}
 
 	~RealFSStreamWrite() override {

=== modified file 'src/io/filesystem/filesystem.cc'
--- src/io/filesystem/filesystem.cc	2018-07-12 04:41:20 +0000
+++ src/io/filesystem/filesystem.cc	2018-11-13 16:47:56 +0000
@@ -306,7 +306,7 @@
 	std::list<std::string>::iterator i;
 
 #ifdef _WIN32
-	// remove all slashes with backslashes so following can work.
+	// replace all slashes with backslashes so following can work.
 	for (uint32_t j = 0; j < path.size(); ++j) {
 		if (path[j] == '/')
 			path[j] = '\\';
@@ -463,7 +463,7 @@
 		fs.ensure_directory_exists(".widelands");
 		fs.fs_unlink(".widelands");
 		return true;
-	} catch (...) {
+	} catch (const FileError& e) {
 		log("Directory %s is not writeable - next try\n", path);
 	}
 

=== modified file 'src/io/filesystem/zip_filesystem.cc'
--- src/io/filesystem/zip_filesystem.cc	2018-10-21 22:21:34 +0000
+++ src/io/filesystem/zip_filesystem.cc	2018-11-13 16:47:56 +0000
@@ -268,12 +268,14 @@
 	if (!file_exists(path)) {
 		throw wexception(
 		   "ZipFilesystem::make_sub_file_system: The path '%s' does not exist in zip file '%s'.",
-		   path.c_str(), zip_file_->path().c_str());
+		   (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
+		   zip_file_->path().c_str());
 	}
 	if (!is_directory(path)) {
-		throw wexception("ZipFilesystem::make_sub_file_system: "
-		                 "The path '%s' needs to be a directory in zip file '%s'.",
-		                 path.c_str(), zip_file_->path().c_str());
+		throw wexception(
+		   "ZipFilesystem::make_sub_file_system: The path '%s' needs to be a directory in zip file '%s'.",
+		   (basedir_in_zip_file_.empty()?path:basedir_in_zip_file_+"/"+path).c_str(),
+		   zip_file_->path().c_str());
 	}
 
 	std::string localpath = path;
@@ -291,7 +293,10 @@
 // see Filesystem::create
 FileSystem* ZipFilesystem::create_sub_file_system(const std::string& path, Type const type) {
 	if (file_exists(path)) {
-		throw wexception("ZipFilesystem::create_sub_file_system: Sub file system already exists.");
+		throw wexception(
+		   "ZipFilesystem::create_sub_file_system: Path '%s' already exists in zip file %s.",
+		   (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
+		   zip_file_->path().c_str());
 	}
 
 	if (type != FileSystem::DIR)
@@ -438,8 +443,9 @@
 		   "ZipFilesystem::write", complete_filename,
 		   (boost::format("in path '%s'', Error") % zip_file_->path() % strerror(errno)).str());
 	default:
-		throw FileError("ZipFilesystem::write", complete_filename,
-		                (boost::format("in path '%s'") % zip_file_->path()).str());
+		throw FileError(
+		   "ZipFilesystem::write", complete_filename,
+		   (boost::format("in path '%s'") % zip_file_->path()).str());
 	}
 
 	zipCloseFileInZip(zip_file_->write_handle());

=== modified file 'src/logic/CMakeLists.txt'
--- src/logic/CMakeLists.txt	2018-08-24 07:12:20 +0000
+++ src/logic/CMakeLists.txt	2018-11-13 16:47:56 +0000
@@ -7,7 +7,6 @@
     base_i18n
 )
 
-
 wl_library(logic_widelands_geometry
   SRCS
     widelands_geometry.cc
@@ -81,6 +80,7 @@
     filesystem_constants.h
     filesystem_constants.cc
 )
+
 wl_library(logic_tribe_basic_info
   SRCS
     map_objects/tribes/tribe_basic_info.cc
@@ -93,6 +93,18 @@
 
 )
 
+wl_library(logic_generic_save_handler
+  SRCS
+    generic_save_handler.h
+    generic_save_handler.cc
+  DEPENDS
+    base_i18n
+    base_log
+    base_time_string
+    io_filesystem
+    logic_filesystem_constants
+)
+
 wl_library(logic
   SRCS
     ai_dna_handler.cc
@@ -281,6 +293,7 @@
     logic_filesystem_constants
     logic_game_controller
     logic_game_settings
+    logic_generic_save_handler
     logic_tribe_basic_info
     logic_widelands_geometry
     map_io

=== modified file 'src/logic/editor_game_base.cc'
--- src/logic/editor_game_base.cc	2018-11-11 20:42:54 +0000
+++ src/logic/editor_game_base.cc	2018-11-13 16:47:56 +0000
@@ -120,8 +120,14 @@
 	// if a file with that name already exists, then try a few name modifications
 	if (g_fs->file_exists(complete_filename)) {
 		int suffix;
+<<<<<<< TREE
 		for (suffix = 0; suffix <= 9; suffix++) {
 			complete_filename = filename + "-" + std::to_string(suffix) + kTempFileExtension;
+=======
+		for (suffix = 0; suffix <= 9; suffix++)
+		{
+			complete_filename = filename + "-" + std::to_string(suffix) + kTempFileExtension;
+>>>>>>> MERGE-SOURCE
 			if (!g_fs->file_exists(complete_filename))
 				break;
 		}
@@ -137,7 +143,12 @@
 	// save necessary map data (we actually save the whole map)
 	std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*tmp_fs_, *this));
 	wms->save();
-
+<<<<<<< TREE
+
+=======
+	delete wms;
+
+>>>>>>> MERGE-SOURCE
 	// swap map fs
 	std::unique_ptr<FileSystem> mapfs(tmp_fs_->make_sub_file_system("."));
 	map_.swap_filesystem(mapfs);

=== modified file 'src/logic/filesystem_constants.h'
--- src/logic/filesystem_constants.h	2018-11-09 08:00:35 +0000
+++ src/logic/filesystem_constants.h	2018-11-13 16:47:56 +0000
@@ -45,6 +45,11 @@
 // We delete (accidentally remaining) temp files older than a week
 constexpr double kTempFilesKeepAroundTime = 7 * 24 * 60 * 60;
 
+/// Filesystem names for for temporary backup when overwriting files during saving
+const std::string kTempBackupExtension = ".tmp";
+// We delete (accidentally remaining) temp backup files older than a day
+constexpr double kTempBackupsKeepAroundTime = 24 * 60 * 60;
+
 /// Filesystem names and timeouts for replays
 const std::string kReplayDir = "replays";
 const std::string kReplayExtension = ".wrpl";

=== added file 'src/logic/generic_save_handler.cc'
--- src/logic/generic_save_handler.cc	1970-01-01 00:00:00 +0000
+++ src/logic/generic_save_handler.cc	2018-11-13 16:47:56 +0000
@@ -0,0 +1,278 @@
+/*
+ * Copyright (C) 2002-2018 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include "logic/generic_save_handler.h"
+
+#include <memory>
+#include <string>
+
+#include <boost/format.hpp>
+
+#include "base/i18n.h"
+#include "base/log.h"
+#include "base/time_string.h"
+#include "io/filesystem/filesystem.h"
+#include "io/filesystem/filesystem_exceptions.h"
+#include "io/filesystem/layered_filesystem.h"
+#include "logic/filesystem_constants.h"
+
+void GenericSaveHandler::clear_errors() {
+	error_ = kSuccess;
+	for (uint32_t errbit = 0; errbit < maxErrors; errbit++) {
+		error_msg_[errbit].clear();
+	}
+	backup_filename_.clear();
+	return;
+}
+
+void GenericSaveHandler::make_backup() {
+	std::string backup_filename_base =
+	   dir_ + g_fs->file_separator() + timestring() + "_" + filename_;
+	backup_filename_ = backup_filename_base + kTempBackupExtension;
+
+	// If a file with that name already exists, then try some name modifications.
+	if (g_fs->file_exists(backup_filename_))
+	{
+		int suffix;
+		for (suffix = 0; suffix <= 9; suffix++)
+		{
+			backup_filename_ = backup_filename_base + "-" + std::to_string(suffix)
+			   + kTempBackupExtension;
+			if (!g_fs->file_exists(backup_filename_)) {
+			  break;
+			}
+		}
+		if (suffix > 9) {
+			error_ |= kBackupFailed;
+			error_msg_[bitBackupFailed] =
+			   (boost::format("GenericSaveHandler::make_backup: %s: for all "
+			   "considered filenames a file already existed (last filename tried "
+			   "was %s)\n") % complete_filename_.c_str() % backup_filename_).str();
+			log("%s", error_msg_[bitBackupFailed].c_str());
+			return;
+		}
+	}
+
+	// Try to rename file.
+	try {
+		g_fs->fs_rename(complete_filename_, backup_filename_);
+	} catch (const FileError& e) {
+			error_ |= kBackupFailed;
+			error_msg_[bitBackupFailed] = (boost::format("GenericSaveHandler::make_backup: file %s "
+			   "could not be renamed: %s\n") % complete_filename_.c_str() % backup_filename_).str();
+			log("%s", error_msg_[bitBackupFailed].c_str());
+		return;
+	}
+
+	return;
+}
+
+void GenericSaveHandler::save_file() {
+	// Write data to file/dir.
+	try {
+		std::unique_ptr<FileSystem>
+		   fs(g_fs->create_sub_file_system(complete_filename_, type_));
+		do_save_(*fs);
+	} catch (const std::exception& e) {
+		error_ |= kSavingDataFailed;
+		error_msg_[bitSavingDataFailed] =
+		   (boost::format("GenericSaveHandler::save_file: data could not be "
+		   "written to file %s: %s\n") % complete_filename_.c_str() % e.what()
+		   ).str();
+		log("%s", error_msg_[bitSavingDataFailed].c_str());
+	}
+
+	if (error_ & kSavingDataFailed) {
+		// Delete remnants of the failed save attempt.
+		if (g_fs->file_exists(complete_filename_)) {
+			try {
+				g_fs->fs_unlink(complete_filename_);
+			} catch (const FileError& e) {
+				error_ |= kCorruptFileLeft;
+				error_msg_[bitCorruptFileLeft] =
+				   (boost::format("GenericSaveHandler::save_file: possibly corrupt "
+				   "file %s could not be deleted: %s\n") % complete_filename_.c_str()
+					 % e.what()).str();
+				log("%s", error_msg_[bitCorruptFileLeft].c_str());
+			}
+		}
+	}
+	return;
+}
+
+uint32_t GenericSaveHandler::save() {
+	try {  // everything additionally in one big try block
+	       // to catch any unexpected errors
+		clear_errors();
+
+		//  Make sure that the current directory exists and is writeable.
+		try {
+			g_fs->ensure_directory_exists(dir_);
+		} catch (const FileError& e) {
+			error_ |= kCreatingDirFailed;
+			error_msg_[bitCreatingDirFailed] =
+			   (boost::format("GenericSaveHandler::save: directory %s could not be "
+			   "created: %s\n") % dir_.c_str() % e.what()).str();
+			log("%s", error_msg_[bitCreatingDirFailed].c_str());
+			return error_;
+		}
+
+		// Make a backup if file already exists.
+		if (g_fs->file_exists(complete_filename_)) {
+			make_backup();
+		}
+		if (error_) {
+		  return error_;
+		}
+
+		// Write data to file/dir.
+		save_file();
+
+		// Restore or delete backup if one was made.
+		if (!backup_filename_.empty())
+		{
+			if (!error_) {
+				// Delete backup.
+				try {
+					g_fs->fs_unlink(backup_filename_);
+				} catch (const FileError& e) {
+					error_ |= kDeletingBackupFailed;
+					error_msg_[bitDeletingBackupFailed] =
+					   (boost::format("GenericSaveHandler::save: backup file %s could "
+					   "not be deleted: %s\n") % backup_filename_.c_str() % e.what()
+					   ).str();
+					log("%s", error_msg_[bitDeletingBackupFailed].c_str());
+				}
+
+			} else {
+				if (error_ & kCorruptFileLeft) {
+					error_ |= kRestoringBackupFailed;
+					error_msg_[bitRestoringBackupFailed] =
+					   (boost::format("GenericSaveHandler::save: file %s could not be "
+					   "restored from backup %s: file still exists\n")
+					   % complete_filename_.c_str() % backup_filename_.c_str()).str();
+					log("%s", error_msg_[bitRestoringBackupFailed].c_str());
+				}
+				else {
+					// Restore backup.
+					try {
+						g_fs->fs_rename(backup_filename_, complete_filename_);
+					} catch (const FileError& e) {
+						error_ |= kRestoringBackupFailed;
+						error_msg_[bitRestoringBackupFailed] =
+						   (boost::format("GenericSaveHandler::save: file %s could not "
+						   "be restored from backup %s: %s\n") % backup_filename_.c_str()
+						   % backup_filename_.c_str() % e.what()).str();
+						log("%s", error_msg_[bitRestoringBackupFailed].c_str());
+					}
+				}
+			}
+		}
+
+	} catch (const std::exception& e) {
+		error_ |= kUnexpectedError;
+		error_msg_[bitUnexpectedError] =
+		   (boost::format("GenericSaveHandler::save: unknown error: %s\n")
+		   % e.what()).str();
+		log("%s", error_msg_[bitUnexpectedError].c_str());
+	}
+
+	return error_;
+}
+
+std::string GenericSaveHandler::error_message(uint32_t error_mask) {
+	error_mask &= error_;
+	std::string err_msg;
+	for (uint32_t errind = 0; errind < maxErrors; errind++) {
+		if ((error_mask >> errind) & 1) {
+			err_msg += error_msg_[errind];
+		}
+	}
+	return err_msg;
+}
+
+std::string GenericSaveHandler::localized_formatted_result_message() {
+	std::string msg;
+
+	if (error_ == kSuccess || error_ == kDeletingBackupFailed) {
+		// no need to mention a failed backup deletion
+		return _("File successfully saved!");
+	}
+
+	if (error_ == kCreatingDirFailed) {
+		return
+		   (boost::format(_("Directory ‘%s’ could not be created!"))
+		   % dir_).str();
+	}
+
+	if (error_ == kBackupFailed) {
+		return
+		   (boost::format(_("File ‘%s’ could not be removed!"))
+		   % complete_filename_.c_str()).str() + "\n"
+			 + _("Try saving under a different name!");
+	}
+
+	// from here on multiple errors might have occurred
+	if (error_ & kSavingDataFailed) {
+		msg =
+		   (boost::format(_("Error writing data to file ‘%s’!"))
+		   % complete_filename_.c_str()).str();
+	}
+
+	if (error_ & kCorruptFileLeft) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg += (boost::format(_("Saved file may be corrupt!"))).str();
+	}
+
+	if (error_ & kRestoringBackupFailed) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg +=
+		   (boost::format(_("File ‘%s’ could not be restored!"))
+		   % complete_filename_.c_str()).str() + "\n" +
+		   (boost::format(_("Backup file ‘%s’ will be available for some time."))
+		   % backup_filename_.c_str()).str();
+	}
+
+	if (!backup_filename_.empty() &&
+	    (error_ & kSavingDataFailed) &&
+	    !(error_ & kCorruptFileLeft) &&
+	    !(error_ & kRestoringBackupFailed)) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg +=
+		   (boost::format(_("File ‘%s’ was restored from backup."))
+		   % complete_filename_.c_str()).str();
+	}
+
+	if (error_ & kUnexpectedError) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg +=
+		   (boost::format(_("An unexpected error occurred:" ))).str()
+		   + "\n" + error_msg_[bitUnexpectedError];
+	}
+
+	return msg;
+}

=== added file 'src/logic/generic_save_handler.h'
--- src/logic/generic_save_handler.h	1970-01-01 00:00:00 +0000
+++ src/logic/generic_save_handler.h	2018-11-13 16:47:56 +0000
@@ -0,0 +1,138 @@
+/*
+ * Copyright (C) 2002-2018 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef WL_LOGIC_GENERIC_SAVE_HANDLER_H
+#define WL_LOGIC_GENERIC_SAVE_HANDLER_H
+
+#include <functional>
+#include <string>
+
+#include <stdint.h>
+
+#include "io/filesystem/filesystem.h"
+
+// just some constants for convenience
+namespace {
+	enum ErrorBitIndex : uint32_t {
+		bitCreatingDirFailed = 0,
+		bitBackupFailed,
+		bitSavingDataFailed,
+		bitCorruptFileLeft,
+		bitDeletingBackupFailed,
+		bitRestoringBackupFailed,
+		bitUnexpectedError,
+		maxErrors
+	};
+}
+
+/**
+ * Class that provides handling all errors for generic file saving.
+ * It stores error codes and error messages.
+ * It can also generate an overview message aimed at a human user.
+ *
+ * The saving routine (what actually writes data to a file system)
+ * must be provided.
+ */
+// TODO(Arty): Possibly make it more generic, allowing to provide options
+// whether to overwrite files, whether to make backups, maybe allow
+// to provide a naming scheme, etc.
+class GenericSaveHandler {
+public:
+	// error constants; usable as bit masks
+	static constexpr uint32_t
+	   kSuccess = 0;
+	static constexpr uint32_t
+	   kCreatingDirFailed = uint32_t(1) << bitCreatingDirFailed;
+	static constexpr uint32_t
+	   kBackupFailed = uint32_t(1) << bitBackupFailed;
+	static constexpr uint32_t
+	   kSavingDataFailed = uint32_t(1) << bitSavingDataFailed;
+	static constexpr uint32_t
+	   kCorruptFileLeft = uint32_t(1) << bitCorruptFileLeft;
+	static constexpr uint32_t
+	   kDeletingBackupFailed = uint32_t(1) << bitDeletingBackupFailed;
+	static constexpr uint32_t
+	   kRestoringBackupFailed = uint32_t(1) << bitRestoringBackupFailed;
+	static constexpr uint32_t
+	   kUnexpectedError = uint32_t(1) << bitUnexpectedError;
+	static constexpr uint32_t
+	   kAllErrors = (1 << maxErrors) - 1;
+
+	explicit GenericSaveHandler(
+	   std::function<void(FileSystem&)> do_save,  // function that actually saves data to the filesystem
+	   std::string complete_filename,
+	   FileSystem::Type type)
+   : do_save_(do_save),
+     complete_filename_(complete_filename),
+     dir_(FileSystem::fs_dirname(complete_filename.c_str())),
+     filename_(FileSystem::fs_filename(complete_filename.c_str())),
+     type_(type),
+     error_(kSuccess) {};
+
+	/**
+	 * Tries to save a file.
+	 *
+	 * If the file already exists, it will be overwritten but a temporary backup
+	 * is made, which will be restored if saving fails and deleted otherwise.
+	 *
+	 * Catches ALL errors.
+	 *
+	 * Error messages for all errors are written to the log but also stored.
+	 * Stores and returns an error code (bit mask of all occurred errors).
+	 */
+	uint32_t save();
+
+	// returns the stored error code (of the last saving operation)
+	uint32_t error() { return error_; };
+
+	// Returns the combination of error_messages (of occurred errors)
+	// specified by a bit mask.
+	std::string error_message(uint32_t error_mask = kAllErrors);
+
+	// Generates a localized formatted message describing the result of
+	// the last saving attempt.
+	// Aimed to be sufficiently informative for a human user.
+	std::string localized_formatted_result_message();
+
+private:
+	std::function<void(FileSystem&)> do_save_;
+	std::string complete_filename_;
+	std::string dir_;
+	std::string filename_;
+	FileSystem::Type type_;
+
+	// Backup filename is automatically generated when saving but is also
+	// stored for generating messages containing backup-related things.
+	std::string backup_filename_;
+	uint32_t error_;
+	std::string error_msg_[maxErrors];
+
+	void clear_errors();
+
+	// Finds a suitable backup filename and tries to rename a file.
+	// Stores an errorcode and error message (if applicable).
+	void make_backup();
+
+	// Saves a file. Assumes file doesn't exist.
+	// Stores an errorcode and error message (if applicable).
+	void save_file();
+};
+
+
+#endif  // end of include guard: WL_LOGIC_GENERIC_SAVE_HANDLER_H

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2018-08-26 18:22:27 +0000
+++ src/logic/save_handler.cc	2018-11-13 16:47:56 +0000
@@ -32,14 +32,13 @@
 #include "base/wexception.h"
 #include "game_io/game_saver.h"
 #include "io/filesystem/filesystem.h"
+#include "io/filesystem/filesystem_exceptions.h"
 #include "logic/filesystem_constants.h"
 #include "logic/game.h"
 #include "logic/game_controller.h"
+#include "logic/generic_save_handler.h"
 #include "wui/interactive_base.h"
 
-// The actual work of saving is done by the GameSaver
-using Widelands::GameSaver;
-
 SaveHandler::SaveHandler()
    : next_save_realtime_(0),
      initialized_(false),
@@ -52,40 +51,71 @@
      number_of_rolls_(5) {
 }
 
-void SaveHandler::roll_save_files(const std::string& filename) {
-
-	int32_t rolls = number_of_rolls_;
-	log("Autosave: Rolling savefiles (count): %d\n", rolls);
-	rolls--;
-	std::string filename_previous =
-	   create_file_name(kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
-	if (rolls > 0 && g_fs->file_exists(filename_previous)) {
-		g_fs->fs_unlink(filename_previous);  // Delete last of the rolling files
-		log("Autosave: Deleted %s\n", filename_previous.c_str());
-	}
+bool SaveHandler::roll_save_files(const std::string& filename,
+                                  std::string* const error) {
+	int32_t rolls = 0;
+	std::string filename_previous;
+
+	// Only roll the smallest necessary number of files.
+	while (rolls < number_of_rolls_) {
+		filename_previous = create_file_name(
+		   kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
+		if (!g_fs->file_exists(filename_previous)) {
+		  break;
+		}
+		rolls++;
+	}
+
+	// If there is a file missing in the sequence; no need to delete any file.
+	if (rolls < number_of_rolls_) {
+		log("Autosave: Rolling savefiles (count): %d of %d\n",
+		    rolls, number_of_rolls_);
+	}
+	else {
+		log("Autosave: Rolling savefiles (count): %d\n", rolls);
+		rolls--;
+		filename_previous = create_file_name(
+		   kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
+		if (rolls > 0) {
+			try {
+				g_fs->fs_unlink(filename_previous);  // Delete last of the rolling files
+				log("Autosave: Deleted %s\n", filename_previous.c_str());
+			} catch (const FileError& e) {
+				log("Autosave: Unable to delete file %s: %s\n",
+				    filename_previous.c_str(), e.what());
+				if (error) {
+					*error =
+					   (boost::format("Autosave: Unable to delete file %s: %s\n")
+					   % filename_previous.c_str() % e.what()).str();
+				}
+				return false;
+			}
+		}
+	}
+
 	rolls--;
 	while (rolls >= 0) {
-		const std::string filename_next =
-		   create_file_name(kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
-		if (g_fs->file_exists(filename_next)) {
-			try {
-				g_fs->fs_rename(
-				   filename_next, filename_previous);  // e.g. wl_autosave_08 -> wl_autosave_09
-				log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
-			} catch (const std::exception& e) {
-				log("Autosave: Unable to rename file %s to %s: %s\n", filename_previous.c_str(),
-				    filename_next.c_str(), e.what());
-			}
+		const std::string filename_next = create_file_name(
+		   kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
+		try {
+			g_fs->fs_rename(filename_next, filename_previous);  // e.g. wl_autosave_08 -> wl_autosave_09
+			log("Autosave: Rolled %s to %s\n",
+			    filename_next.c_str(), filename_previous.c_str());
+		} catch (const FileError& e) {
+			log("Autosave: Unable to roll file %s to %s: %s\n",
+			    filename_previous.c_str(), filename_next.c_str(), e.what());
+			return false;
 		}
 		filename_previous = filename_next;
 		rolls--;
 	}
+	return true;
 }
 
 /**
  * Check if game should be saved at next tick / think.
  *
- * @return true if game should be saved ad next think().
+ * @return true if game should be saved at next think().
  */
 bool SaveHandler::check_next_tick(Widelands::Game& game, uint32_t realtime) {
 	// Perhaps save is due now?
@@ -99,50 +129,14 @@
 	}
 
 	log("Autosave: %d ms interval elapsed, current gametime: %s, saving...\n",
-	    autosave_interval_in_ms_, gametimestring(game.get_gametime(), true).c_str());
+	    autosave_interval_in_ms_,
+	    gametimestring(game.get_gametime(), true).c_str());
 
 	game.get_ibase()->log_message(_("Saving game…"));
 	return true;
 }
 
 /**
- * If saving fails restore the backup file.
- *
- * @return true when save was a success.
- */
-bool SaveHandler::save_and_handle_error(Widelands::Game& game,
-                                        const std::string& complete_filename,
-                                        const std::string& backup_filename) {
-	std::string error;
-	bool result = save_game(game, complete_filename, &error);
-	if (!result) {
-		log("Autosave: ERROR! - %s\n", error.c_str());
-		game.get_ibase()->log_message(_("Saving failed!"));
-
-		// if backup file was created, move it back
-		if (backup_filename.length() > 0) {
-			if (g_fs->file_exists(complete_filename)) {
-				g_fs->fs_unlink(complete_filename);
-			}
-			g_fs->fs_rename(backup_filename, complete_filename);
-		}
-		// Wait 30 seconds until next save try
-		next_save_realtime_ = SDL_GetTicks() + 30000;
-	} else {
-		// if backup file was created, time to remove it
-		if (backup_filename.length() > 0 && g_fs->file_exists(backup_filename)) {
-			g_fs->fs_unlink(backup_filename);
-		}
-
-		// Count save interval from end of save.
-		// This prevents us from going into endless autosave cycles if the save should take longer
-		// than the autosave interval.
-		next_save_realtime_ = SDL_GetTicks() + autosave_interval_in_ms_;
-	}
-	return result;
-}
-
-/**
  * Check if autosave is needed and allowed or save was requested by user.
  */
 void SaveHandler::think(Widelands::Game& game) {
@@ -155,6 +149,9 @@
 
 	// Are we saving now?
 	if (saving_next_tick_ || save_requested_) {
+		saving_next_tick_ = false;
+		bool save_success = true;
+		std::string error;
 		std::string filename = autosave_filename_;
 		if (save_requested_) {
 			// Requested by user
@@ -166,59 +163,34 @@
 			save_filename_ = "";
 		} else {
 			// Autosave ...
-			roll_save_files(filename);
-			filename = (boost::format("%s_00") % autosave_filename_).str();
-			log("Autosave: saving as %s\n", filename.c_str());
-		}
-
-		// Saving now
-		std::string complete_filename = create_file_name(kSaveDir, filename);
-		std::string backup_filename;
-
-		// always overwrite a file
-		if (g_fs->file_exists(complete_filename)) {
-			filename += "2";
-			backup_filename = create_file_name(kSaveDir, filename);
-			if (g_fs->file_exists(backup_filename)) {
-				g_fs->fs_unlink(backup_filename);
-			}
-			if (save_requested_) {
-				// Exceptions (e.g. no file permissions) will be handled in UI
-				g_fs->fs_rename(complete_filename, backup_filename);
-			} else {
-				// We're autosaving, try to handle file permissions issues
-				try {
-					g_fs->fs_rename(complete_filename, backup_filename);
-				} catch (const std::exception& e) {
-					log("Autosave: Unable to rename file %s to %s: %s\n", complete_filename.c_str(),
-					    backup_filename.c_str(), e.what());
-					// See if we can find a file that doesn't exist and save to that
-					int current_autosave = 0;
-					do {
-						filename = create_file_name(
-						   kSaveDir,
-						   (boost::format("%s_0%d") % autosave_filename_ % (++current_autosave)).str());
-					} while (current_autosave < 9 && g_fs->file_exists(filename));
-					complete_filename = filename;
-					log("Autosave: saving as %s instead\n", complete_filename.c_str());
-					try {
-						g_fs->fs_rename(complete_filename, backup_filename);
-					} catch (const std::exception&) {
-						log("Autosave failed, skipping this interval\n");
-						saving_next_tick_ = check_next_tick(game, realtime);
-						return;
-					}
-				}
-			}
-		}
-
-		if (!save_and_handle_error(game, complete_filename, backup_filename)) {
+			save_success = roll_save_files(filename, &error);
+			if (save_success) {
+				filename = (boost::format("%s_00") % autosave_filename_).str();
+				log("Autosave: saving as %s\n", filename.c_str());
+			}
+		}
+
+		if (save_success) {
+			// Saving now (always overwrite file)
+			std::string complete_filename = create_file_name(kSaveDir, filename);
+			save_success = save_game(game, complete_filename, &error);
+		}
+		if (!save_success) {
+			log("Autosave: ERROR! - %s\n", error.c_str());
+			game.get_ibase()->log_message(_("Saving failed!"));
+
+			// Wait 30 seconds until next save try
+			next_save_realtime_ = SDL_GetTicks() + 30000;
 			return;
 		}
 
+		// Count save interval from end of save.
+		// This prevents us from going into endless autosave cycles if the save
+		// should take longer than the autosave interval.
+		next_save_realtime_ = SDL_GetTicks() + autosave_interval_in_ms_;
+
 		log("Autosave: save took %d ms\n", SDL_GetTicks() - realtime);
 		game.get_ibase()->log_message(_("Game saved"));
-		saving_next_tick_ = false;
 	} else {
 		saving_next_tick_ = check_next_tick(game, realtime);
 	}
@@ -262,34 +234,41 @@
 	return complete_filename;
 }
 
+
 /*
  * Save the game using the GameSaver.
  *
- * Will copy text of exceptions to error string.
- *
- * returns true if saved, false in case some error occured.
+ * Overwrites file if it exists.
+ *
+ * Will copy text of errors to error string.
+ *
+ * Returns true if saved, false in case some error occured.
  */
 bool SaveHandler::save_game(Widelands::Game& game,
                             const std::string& complete_filename,
-                            std::string* const error) {
+                            std::string* const error_str) {
 	ScopedTimer save_timer("SaveHandler::save_game() took %ums");
 
-	// Make sure that the base directory exists
-	g_fs->ensure_directory_exists(kSaveDir);
-
-	// Make a filesystem out of this
-	std::unique_ptr<FileSystem> fs;
-	fs.reset(g_fs->create_sub_file_system(complete_filename, fs_type_));
-
-	bool result = true;
-	GameSaver gs(*fs, game);
-	try {
-		gs.save();
-	} catch (const std::exception& e) {
-		if (error)
-			*error = e.what();
-		result = false;
-	}
-
-	return result;
+	// save game via the GenericSaveHandler
+	GenericSaveHandler gsh(
+		[&game](FileSystem& fs) {
+			Widelands::GameSaver gs(fs, game);
+			gs.save();
+		},
+		complete_filename,
+		fs_type_
+	);
+	gsh.save();
+
+	// Ignore it if only the temporary backup wasn't deleted
+	// but save was successfull otherwise
+	if (gsh.error() == GenericSaveHandler::kSuccess ||
+	    gsh.error() == GenericSaveHandler::kDeletingBackupFailed) {
+		return true;
+	}
+
+	if (error_str) {
+		*error_str = gsh.error_message();
+	}
+	return false;
 }

=== modified file 'src/logic/save_handler.h'
--- src/logic/save_handler.h	2018-04-07 16:59:00 +0000
+++ src/logic/save_handler.h	2018-11-13 16:47:56 +0000
@@ -41,8 +41,13 @@
 	SaveHandler();
 
 	void think(Widelands::Game&);
-	std::string create_file_name(const std::string& dir, const std::string& filename) const;
-	bool save_game(Widelands::Game&, const std::string& filename, std::string* error = nullptr);
+	std::string create_file_name(const std::string& dir,
+	                             const std::string& filename) const;
+
+	// Saves the game, overwrites file, handles errors
+	bool save_game(Widelands::Game&,
+	               const std::string& filename,
+	               std::string* error_str = nullptr);
 
 	const std::string get_cur_filename() {
 		return current_filename_;
@@ -82,11 +87,8 @@
 	int32_t number_of_rolls_;  // For rolling file update
 
 	void initialize(uint32_t gametime);
-	void roll_save_files(const std::string& filename);
+	bool roll_save_files(const std::string& filename, std::string* error);
 	bool check_next_tick(Widelands::Game& game, uint32_t realtime);
-	bool save_and_handle_error(Widelands::Game& game,
-	                           const std::string& complete_filename,
-	                           const std::string& backup_filename);
 };
 
 #endif  // end of include guard: WL_LOGIC_SAVE_HANDLER_H

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2018-11-11 20:42:54 +0000
+++ src/wlapplication.cc	2018-11-13 16:47:56 +0000
@@ -58,6 +58,7 @@
 #include "graphic/text_constants.h"
 #include "helper.h"
 #include "io/filesystem/disk_filesystem.h"
+#include "io/filesystem/filesystem_exceptions.h"
 #include "io/filesystem/layered_filesystem.h"
 #include "logic/ai_dna_handler.h"
 #include "logic/filesystem_constants.h"
@@ -212,10 +213,10 @@
 	return tfile;
 }
 
-// Returns true if 'filename' was autogenerated, i.e. if 'extract_creation_day' can return a date
-// and it is old enough to be deleted.
+// Returns true if 'filename' was autogenerated, i.e. if 'extract_creation_day'
+// can return a date and it is old enough to be deleted.
 bool is_autogenerated_and_expired(const std::string& filename,
-                                  const double keep_time = kReplayKeepAroundTime) {
+                                  const double keep_time) {
 	tm tfile;
 	if (!extract_creation_day(filename, &tfile)) {
 		return false;
@@ -340,6 +341,7 @@
 	cleanup_replays();
 	cleanup_ai_files();
 	cleanup_temp_files();
+	cleanup_temp_backups();
 
 	Section& config = g_options.pull_section("global");
 
@@ -1458,28 +1460,24 @@
 }
 
 /**
- * Delete the syncstream (.wss) files in the replay directory on startup
+ * Delete old syncstream (.wss) files in the replay directory on startup
  * Delete old replay files on startup
  */
 void WLApplication::cleanup_replays() {
 	for (const std::string& filename :
 	     filter(g_fs->list_directory(kReplayDir), [](const std::string& fn) {
-		     return boost::ends_with(
-		        fn, (boost::format("%s%s") % kReplayExtension % kSyncstreamExtension).str());
-		  })) {
-		if (is_autogenerated_and_expired(filename)) {
-			log("Delete syncstream %s\n", filename.c_str());
-			g_fs->fs_unlink(filename);
-		}
-	}
-
-	for (const std::string& filename :
-	     filter(g_fs->list_directory(kReplayDir),
-	            [](const std::string& fn) { return boost::ends_with(fn, kReplayExtension); })) {
-		if (is_autogenerated_and_expired(filename)) {
+	       return boost::ends_with(fn, kReplayExtension) ||
+	         boost::ends_with(fn, kReplayExtension + kSavegameExtension) ||
+	         boost::ends_with(fn, kReplayExtension + kSyncstreamExtension);
+	     })) {
+		if (is_autogenerated_and_expired(filename, kReplayKeepAroundTime)) {
 			log("Deleting replay %s\n", filename.c_str());
-			g_fs->fs_unlink(filename);
-			g_fs->fs_unlink(filename + kSavegameExtension);
+			try {
+				g_fs->fs_unlink(filename);
+			} catch (const FileError& e) {
+				log("WLApplication::cleanup_replays: File %s couldn't be deleted.\n",
+				    filename.c_str());
+			}
 		}
 	}
 }
@@ -1490,11 +1488,17 @@
 void WLApplication::cleanup_ai_files() {
 	for (const std::string& filename :
 	     filter(g_fs->list_directory(kAiDir), [](const std::string& fn) {
-		     return boost::ends_with(fn, kAiExtension) || boost::contains(fn, "ai_player");
-		  })) {
+		     return boost::ends_with(fn, kAiExtension) ||
+				        boost::contains(fn, "ai_player");
+		   })) {
 		if (is_autogenerated_and_expired(filename, kAIFilesKeepAroundTime)) {
 			log("Deleting generated ai file: %s\n", filename.c_str());
-			g_fs->fs_unlink(filename);
+			try {
+				g_fs->fs_unlink(filename);
+			} catch (const FileError& e) {
+				log("WLApplication::cleanup_ai_files: File %s couldn't be deleted.\n",
+				    filename.c_str());
+			}
 		}
 	}
 }
@@ -1504,13 +1508,64 @@
  */
 void WLApplication::cleanup_temp_files() {
 	for (const std::string& filename :
+<<<<<<< TREE
 	     filter(g_fs->list_directory(kTempFileDir),
 	            [](const std::string& fn) { return boost::ends_with(fn, kTempFileExtension); })) {
+=======
+	     filter(g_fs->list_directory(kTempFileDir), [](const std::string& fn) {
+		     return boost::ends_with(fn, kTempFileExtension);
+		   })) {
+>>>>>>> MERGE-SOURCE
 		if (is_autogenerated_and_expired(filename, kTempFilesKeepAroundTime)) {
 			log("Deleting old temp file: %s\n", filename.c_str());
-			g_fs->fs_unlink(filename);
-		}
-	}
+			try {
+				g_fs->fs_unlink(filename);
+			} catch (const FileError& e) {
+				log("WLApplication::cleanup_temp_files: File %s couldn't be deleted.\n",
+				    filename.c_str());
+			}
+		}
+	}
+}
+
+/**
+ * Recursively delete temporary backup files in a given directory
+ */
+void WLApplication::cleanup_temp_backups(std::string dir) {
+	for (const std::string& filename :
+	     filter(g_fs->list_directory(dir), [](const std::string& fn) {
+		     return boost::ends_with(fn, kTempBackupExtension);
+		   })) {
+		if (is_autogenerated_and_expired(filename, kTempBackupsKeepAroundTime)) {
+			log("Deleting old temp backup file: %s\n", filename.c_str());
+			try {
+				g_fs->fs_unlink(filename);
+			} catch (const FileError& e) {
+				log("WLApplication::cleanup_temp_backups: File %s couldn't be deleted.\n",
+				    filename.c_str());
+			}
+		}
+	}
+	// recursively delete in subdirs
+	for (const std::string& dirname :
+	     filter(g_fs->list_directory(dir), [](const std::string& fn) {
+		     return g_fs->is_directory(fn) &&
+	         // avoid searching within savegames/maps/backups that were created
+					 // as directories instead of zipfiles
+	         !boost::ends_with(fn, kSavegameExtension) &&
+	         !boost::ends_with(fn, kWidelandsMapExtension) &&
+	         !boost::ends_with(fn, kTempBackupExtension);
+		   })) {
+		cleanup_temp_backups(dirname);
+	}
+}
+
+/**
+ * Delete old temporary backup files that might still lurk around (game crashes etc.)
+ */
+void WLApplication::cleanup_temp_backups() {
+	cleanup_temp_backups(kSaveDir);
+	cleanup_temp_backups(kMapsDir);
 }
 
 bool WLApplication::redirect_output(std::string path) {

=== modified file 'src/wlapplication.h'
--- src/wlapplication.h	2018-10-21 22:21:34 +0000
+++ src/wlapplication.h	2018-11-13 16:47:56 +0000
@@ -213,10 +213,10 @@
 	void setup_homedir();
 
 	void cleanup_replays();
-
 	void cleanup_ai_files();
-
 	void cleanup_temp_files();
+	void cleanup_temp_backups(std::string dir);
+	void cleanup_temp_backups();
 
 	bool redirect_output(std::string path = "");
 

=== modified file 'src/wui/CMakeLists.txt'
--- src/wui/CMakeLists.txt	2018-09-12 21:24:39 +0000
+++ src/wui/CMakeLists.txt	2018-11-13 16:47:56 +0000
@@ -284,6 +284,7 @@
     logic_filesystem_constants
     logic_game_controller
     logic_game_settings
+    logic_generic_save_handler
     logic_tribe_basic_info
     logic_widelands_geometry
     network

=== modified file 'src/wui/game_main_menu_save_game.cc'
--- src/wui/game_main_menu_save_game.cc	2018-11-03 18:21:36 +0000
+++ src/wui/game_main_menu_save_game.cc	2018-11-13 16:47:56 +0000
@@ -21,6 +21,7 @@
 
 #include <memory>
 
+#include <boost/algorithm/string.hpp>
 #include <boost/format.hpp>
 
 #include "base/i18n.h"
@@ -32,6 +33,7 @@
 #include "logic/filesystem_constants.h"
 #include "logic/game.h"
 #include "logic/game_controller.h"
+#include "logic/generic_save_handler.h"
 #include "logic/playersmanager.h"
 #include "ui_basic/messagebox.h"
 #include "wui/interactive_gamebase.h"
@@ -154,63 +156,17 @@
 	}
 }
 
-static void dosave(InteractiveGameBase& igbase, const std::string& complete_filename) {
-	Widelands::Game& game = igbase.game();
-
-	std::string error;
-	if (!game.save_handler().save_game(game, complete_filename, &error)) {
-		std::string s = _("Game Saving Error!\nSaved game file may be corrupt!\n\n"
-		                  "Reason given:\n");
-		s += error;
-		UI::WLMessageBox mbox(&igbase, _("Save Game Error!"), s, UI::WLMessageBox::MBoxType::kOk);
-		mbox.run<UI::Panel::Returncodes>();
-	}
-	game.save_handler().set_current_filename(complete_filename);
-}
-
-struct SaveWarnMessageBox : public UI::WLMessageBox {
-	SaveWarnMessageBox(GameMainMenuSaveGame& parent, const std::string& filename)
-	   : UI::WLMessageBox(&parent,
-	                      _("Save Game Error!"),
-	                      (boost::format(_("A file with the name ‘%s’ already exists. Overwrite?")) %
-	                       FileSystem::fs_filename(filename.c_str()))
-	                         .str(),
-	                      MBoxType::kOkCancel),
-	     filename_(filename) {
-	}
-
-	GameMainMenuSaveGame& menu_save_game() {
-		return dynamic_cast<GameMainMenuSaveGame&>(*get_parent());
-	}
-
-	void clicked_ok() override {
-		g_fs->fs_unlink(filename_);
-		dosave(menu_save_game().igbase(), filename_);
-		menu_save_game().die();
-	}
-
-	void clicked_back() override {
-		die();
-	}
-
-private:
-	std::string const filename_;
-};
-
 void GameMainMenuSaveGame::ok() {
-	if (filename_editbox_.text().empty()) {
+	if (!ok_.enabled()) {
 		return;
 	}
 
-	std::string const complete_filename =
-	   igbase().game().save_handler().create_file_name(curdir_, filename_editbox_.text());
-
-	//  Check if file exists. If it does, show a warning.
-	if (g_fs->file_exists(complete_filename)) {
-		new SaveWarnMessageBox(*this, complete_filename);
+	std::string filename = filename_editbox_.text();
+	if (save_game(filename,
+	              !g_options.pull_section("global").get_bool("nozip", false))) {
+		die();
 	} else {
-		dosave(igbase(), complete_filename);
-		die();
+		load_or_save_.table_.focus();
 	}
 }
 
@@ -245,3 +201,72 @@
 	}
 	igbase().game().game_controller()->set_paused(paused);
 }
+
+/**
+ * Save the game in the Savegame directory with
+ * the given filename
+ *
+ * returns true if dialog should close, false if it
+ * should stay open
+ */
+bool GameMainMenuSaveGame::save_game(std::string filename, bool binary) {
+	// Trim it for preceding/trailing whitespaces in user input
+	boost::trim(filename);
+
+	//  OK, first check if the extension matches (ignoring case).
+	if (!boost::iends_with(filename, kSavegameExtension)) {
+		filename += kSavegameExtension;
+	}
+
+	//  Append directory name.
+	const std::string complete_filename =
+	   curdir_ + g_fs->file_separator() + filename;
+
+	//  Check if file exists. If so, show a warning.
+	if (g_fs->file_exists(complete_filename)) {
+		const std::string s =
+		   (boost::format(_("A file with the name ‘%s’ already exists. Overwrite?"))
+		   % FileSystem::fs_filename(filename.c_str())).str();
+		UI::WLMessageBox mbox(this, _("Error Saving Game!"), s,
+		                      UI::WLMessageBox::MBoxType::kOkCancel);
+		if (mbox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kBack) {
+			return false;
+		}
+	}
+
+	// Try saving the game.
+	Widelands::Game& game = igbase().game();
+	GenericSaveHandler gsh(
+		[&game](FileSystem& fs) {
+			Widelands::GameSaver gs(fs, game);
+			gs.save();
+		},
+		complete_filename,
+		binary ? FileSystem::ZIP : FileSystem::DIR
+	);
+	uint32_t error = gsh.save();
+	if (error == GenericSaveHandler::kSuccess ||
+	    error == GenericSaveHandler::kDeletingBackupFailed) {
+			// No need to bother the player if only the temporary backup couldn't be
+			// deleted. Automatic cleanup will try to deal with it later.
+		game.save_handler().set_current_filename(complete_filename);
+		igbase().log_message(_("Game saved"));
+		return true;
+	}
+
+	// Show player an error message.
+	std::string msg = gsh.localized_formatted_result_message();
+	UI::WLMessageBox mbox(this, _("Error Saving Game!"), msg,
+	                      UI::WLMessageBox::MBoxType::kOk);
+	mbox.run<UI::Panel::Returncodes>();
+
+	// If only the backup failed (likely just because of a file lock),
+	// then leave the dialog open for the player to try with a new filename.
+	if (error == GenericSaveHandler::kBackupFailed) {
+	  return false;
+	}
+
+	// In the other error cases close the dialog.
+	igbase().log_message(_("Saving failed!"));
+	return true;
+}

=== modified file 'src/wui/game_main_menu_save_game.h'
--- src/wui/game_main_menu_save_game.h	2018-05-23 04:40:43 +0000
+++ src/wui/game_main_menu_save_game.h	2018-11-13 16:47:56 +0000
@@ -60,6 +60,8 @@
 	/// Called when the OK button is clicked or the Return key pressed in the edit box.
 	void ok();
 
+	bool save_game(std::string filename, bool binary);
+
 	/// Pause/unpause the game
 	void pause_game(bool paused);