Merge lp:~widelands-dev/widelands/robust-file-saving into lp:widelands

[Arty]

Introduces a new class to robustly handle saving of files (incl. dealing with backups, handling errors, etc.) and use it whenever maps/games are being saved.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4221. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/454613234.
Appveyor build 4017. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_robust_file_saving-4017.

[GunChleoc (gunchleoc)]

I have added 2 comments. I'll do another review round when you have looked at them.

[Arty (artydent)]

I admit the bitmask stuff is still my default way from before C++11, without having to re-define operators or typecast all the time. But you are right, I should do this properly with enum class now. I'll do that.

As for the code duplication between the game saving and map saving, I feel that this doesn't really belong into the GenericSaveHandler. I wanted to keep it on a separate abstraction level, in particular without relying on any UI stuff or other user interaction. (I am not even sure I should have put the generation of localized messages in there.)

The duplication part you pointed out isn't even that big (and half of it just comments), but there is overall a lot of code duplication between the map saving UI and the game saving UI. I think we should rather try to merge those into a single class. There are of course a few key differences between handling games and maps there, but those could possibly be dealt with via overloading or templates or even just via some parameters (including using lambda functions). Same goes for the loading UI and possibly a few others. I wouldn't mind looking into that, but that would be a somewhat bigger change (and require some more consideration how feasible this even is), so that would rather be something for the build21.

[Arty (artydent)]

Switched to a type safe error type.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4242. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/456239294.
Appveyor build 4037. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_robust_file_saving-4037.

[Arty (artydent)]

Oops, forgot to enable codecheck again. Will be fixed in the next round.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4287. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/459370561.
Appveyor build 4081. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_robust_file_saving-4081.

[GunChleoc (gunchleoc)]

Error from Travis log:

/home/travis/build/widelands/widelands/src/logic/generic_save_handler.cc:44:71: error: declaration of ‘error’ shadows a member of 'this' [-Werror=shadow]
 uint32_t GenericSaveHandler::get_index(GenericSaveHandler::Error error) {

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4296. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/459708047.
Appveyor build 4089. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_robust_file_saving-4089.

[Klaus Halfmann (klaus-halfmann)]

Hello artydent,

I checked out this code an attached a debugger,
I will try to gain some coverage an this way understand it.

Gun: shall this become part of R20 or is this is "only" an improvemnet?
Artydent: Im am using OSX, anything special that I should look for?

[Klaus Halfmann (klaus-halfmann)]

OK, I did some tests with normal saves and some ways to break it.
Experience tells me that Windows is more picky about such errors
(as it locks open files). I was unable to reproduce all the errors.
But anway, basic Saving is ok and nothing is broken.

The code looks ok for me so it can go in.

Gun: Do we have somone on Widows to do some more debugging?

I could spend some more time and try to break mores tuff :-)

review: Approve (compile, test, debug)

[Arty (artydent)]

Merry Christmas, and thanks for testing.

As for "ways to break it", there *should* at least be no new ways to do so. After all, the main purpose of this branch is to prevent any such breaking when we save maps/games. (And there aren't a lot of new things that could break on their own.) The actual saving routine itself could potentially break somewhere (I didn't change anything there), but as long as that involves throwing an exception, it will be caught.

Under Windows I tested this quite a lot myself. Usually, errors should "normally" not happen without someone trying to force them. Error scenarios that could be forced for testing would be:
1) after choosing a dir and filename, replace the dir by a non-dir file of that name, then try saving -> dir creation fails
2) lock a file (e.g. open it with a zip packer or so that locks the file), then try saving to that file -> renaming (for backup) fails
3) make a file read-only, then try saving to that file -> deleting backup (renamed file) fails (no ingame message though)
4) try saving on a read-only medium -> saving data fails
(There might be slight differences between platforms, especially with the file locking.)

Other errors would actually be hard to set off on purpose. That would likely require different processes to access the same file at the same time. Hard to even force, and extremely unlikely to happen by accident, but such errors are caught just in case. (In my testing for those scenarios I just temporarily added exceptions throws at some places.)

[Klaus Halfmann (klaus-halfmann)]

I think should get this in now,
I dont think it will break anything.

@bunnybot merge

review: Approve (reaprove)

[bunnybot (widelandsofficial)]

Error merging this proposal:

Output:
stdout:

stderr:
+N src/logic/generic_save_handler.cc
+N src/logic/generic_save_handler.h
 M src/editor/CMakeLists.txt
 M src/editor/ui_menus/main_menu_save_map.cc
 M src/logic/CMakeLists.txt
 M src/logic/filesystem_constants.h
 M src/logic/save_handler.cc
 M src/logic/save_handler.h
 M src/wlapplication.cc
 M src/wlapplication.h
 M src/wui/CMakeLists.txt
 M src/wui/game_main_menu_save_game.cc
 M src/wui/game_main_menu_save_game.h
Text conflict in src/wlapplication.cc
1 conflicts encountered.

[Klaus Halfmann (klaus-halfmann)]

Hello Arty I try to fix these conflicts, but maybe I will need you help?

[Klaus Halfmann (klaus-halfmann)]

Arty can, you take another look? Maybe my merge broke it ....

lets try to compile this at leasst

review: Needs Resubmitting (merge)

[Arty (artydent)]

Sorry, I am very busy lately and was not able to do anything Widelands-related or even look here at all. This will likely continue for quite some time, but I'll have a look at it this weekend.

[GunChleoc (gunchleoc)]

I have had a look at the diff for the trunk merge and the resolution of the merge conflict looks fine.

[Klaus Halfmann (klaus-halfmann)]

Uhm, why dont we get another build?

I foundd soum compile warnigns about the new JSON Objects not having virtual DTors.
but that is some other story.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

Bunnybot will only post here if there is a status change to prevent spamming the discussion. You'll need to visit Travis & AppVeyor manually when there is no change.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4498. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/495915762.
Appveyor build 4285. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_robust_file_saving-4285.

[GunChleoc (gunchleoc)]

Klaus & I have tested this on Linux, Mac and Windows now. All working as expected. Thanks for the great fix!

@bunnybot merge

review: Approve

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4498. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/495915762.

[GunChleoc (gunchleoc)]

@bunnybot merge force

=== modified file 'src/editor/CMakeLists.txt'
--- src/editor/CMakeLists.txt	2018-07-19 16:43:14 +0000
+++ src/editor/CMakeLists.txt	2019-02-20 09:56:48 +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-23 17:10:25 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2019-02-20 09:56:48 +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;
 
@@ -269,15 +272,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);
 
@@ -301,22 +301,6 @@
 		}
 	}
 
-	//  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) {
-		log("Unable to delete old map file %s while saving map: %s\n", complete_filename.c_str(),
-		    e.what());
-		const std::string s = (boost::format(_("File ‘%s’ could not be deleted.")) %
-		                       FileSystem::fs_filename(filename.c_str()))
-		                         .str() +
-		                      " " + _("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;
-	}
-
 	Widelands::EditorGameBase& egbase = eia().egbase();
 	Widelands::Map* map = egbase.mutable_map();
 
@@ -334,21 +318,37 @@
 		map->delete_tag("artifacts");
 	}
 
-	//  Try saving.
-	try {
-		std::unique_ptr<FileSystem> fs(g_fs->create_sub_file_system(
-		   complete_filename, binary ? FileSystem::ZIP : FileSystem::DIR));
-		std::unique_ptr<Widelands::MapSaver> wms(new Widelands::MapSaver(*fs, egbase));
-		wms->save();
-		fs.reset();
-	} catch (const std::exception& e) {
-		std::string s = _("Error Saving Map!\nSaved map file may be corrupt!\n\nReason "
-		                  "given:\n");
-		s += e.what();
-		UI::WLMessageBox mbox(&eia(), _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOk);
-		mbox.run<UI::Panel::Returncodes>();
-	}
-
-	die();
+	// Try saving the map.
+	GenericSaveHandler gsh(
+		[&egbase](FileSystem& fs) {
+			Widelands::MapSaver wms(fs, egbase);
+			wms.save();
+		},
+		complete_filename,
+		binary ? FileSystem::ZIP : FileSystem::DIR
+	);
+	GenericSaveHandler::Error error = gsh.save();
+
+	// If only the temporary backup couldn't be deleted, we still treat it as
+	// success. Automatic cleanup will deal with later. No need to bother the
+	// player with it.
+	if (error == GenericSaveHandler::Error::kSuccess ||
+	    error == GenericSaveHandler::Error::kDeletingBackupFailed) {
+		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::Error::kBackupFailed) {
+	  return false;
+	}
+
+	// In the other error cases close the dialog.
 	return true;
 }

=== modified file 'src/logic/CMakeLists.txt'
--- src/logic/CMakeLists.txt	2018-08-24 07:12:20 +0000
+++ src/logic/CMakeLists.txt	2019-02-20 09:56:48 +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/filesystem_constants.h'
--- src/logic/filesystem_constants.h	2019-01-16 22:22:41 +0000
+++ src/logic/filesystem_constants.h	2019-02-20 09:56:48 +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	2019-02-20 09:56:48 +0000
@@ -0,0 +1,308 @@
+/*
+ * 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() {
+	error_ = Error::kNone;
+	for (uint32_t index = 0; index < maxErrors_; index++) {
+		error_msg_[index].clear();
+	}
+	backup_filename_.clear();
+	return;
+}
+
+uint32_t GenericSaveHandler::get_index(GenericSaveHandler::Error err) {
+	if (err == Error::kNone) {
+		return maxErrors_;
+	}
+	uint32_t error_uint = static_cast<uint32_t>(err);
+	for (uint32_t index = 0; index < maxErrors_; index++) {
+		if (error_uint & 1) {
+			return index;
+		}
+		error_uint >>= 1;
+	}
+	return maxErrors_;
+}
+
+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_ |= Error::kBackupFailed;
+			uint32_t index = get_index(Error::kBackupFailed);
+			error_msg_[index] =
+			   (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_[index].c_str());
+			return;
+		}
+	}
+
+	// Try to rename file.
+	try {
+		g_fs->fs_rename(complete_filename_, backup_filename_);
+	} catch (const FileError& e) {
+			error_ |= Error::kBackupFailed;
+			uint32_t index = get_index(Error::kBackupFailed);
+			error_msg_[index] =
+			   (boost::format("GenericSaveHandler::make_backup: file %s "
+			   "could not be renamed: %s\n") % complete_filename_.c_str()
+			   % backup_filename_).str();
+			log("%s", error_msg_[index].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_ |= Error::kSavingDataFailed;
+			uint32_t index = get_index(Error::kSavingDataFailed);
+			error_msg_[index] =
+		   (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_[index].c_str());
+	}
+
+	if ((error_ & Error::kSavingDataFailed) != Error::kNone) {
+		// 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_ |= Error::kCorruptFileLeft;
+				uint32_t index = get_index(Error::kCorruptFileLeft);
+				error_msg_[index] =
+				   (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_[index].c_str());
+			}
+		}
+	}
+	return;
+}
+
+GenericSaveHandler::Error GenericSaveHandler::save() {
+	try {  // everything additionally in one big try block
+	       // to catch any unexpected errors
+		clear();
+
+		//  Make sure that the current directory exists and is writeable.
+		try {
+			g_fs->ensure_directory_exists(dir_);
+		} catch (const FileError& e) {
+			error_ |= Error::kCreatingDirFailed;
+			uint32_t index = get_index(Error::kCreatingDirFailed);
+			error_msg_[index] =
+			   (boost::format("GenericSaveHandler::save: directory %s could not be "
+			   "created: %s\n") % dir_.c_str() % e.what()).str();
+			log("%s", error_msg_[index].c_str());
+			return error_;
+		}
+
+		// Make a backup if file already exists.
+		if (g_fs->file_exists(complete_filename_)) {
+			make_backup();
+		}
+		if (error_ != Error::kNone) {
+		  return error_;
+		}
+
+		// Write data to file/dir.
+		save_file();
+
+		// Restore or delete backup if one was made.
+		if (!backup_filename_.empty())
+		{
+			if (error_ == Error::kNone) {
+				// Delete backup.
+				try {
+					g_fs->fs_unlink(backup_filename_);
+				} catch (const FileError& e) {
+					error_ |= Error::kDeletingBackupFailed;
+					uint32_t index = get_index(Error::kDeletingBackupFailed);
+					error_msg_[index] =
+					   (boost::format("GenericSaveHandler::save: backup file %s could "
+					   "not be deleted: %s\n") % backup_filename_.c_str() % e.what()
+					   ).str();
+					log("%s", error_msg_[index].c_str());
+				}
+
+			} else {
+				if ((error_ & Error::kCorruptFileLeft) != Error::kNone) {
+					error_ |= Error::kRestoringBackupFailed;
+					uint32_t index = get_index(Error::kRestoringBackupFailed);
+					error_msg_[index] =
+					   (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_[index].c_str());
+				}
+				else {
+					// Restore backup.
+					try {
+						g_fs->fs_rename(backup_filename_, complete_filename_);
+					} catch (const FileError& e) {
+						error_ |= Error::kRestoringBackupFailed;
+						uint32_t index = get_index(Error::kRestoringBackupFailed);
+						error_msg_[index] =
+						   (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_[index].c_str());
+					}
+				}
+			}
+		}
+
+	} catch (const std::exception& e) {
+		error_ |= Error::kUnexpectedError;
+		uint32_t index = get_index(Error::kUnexpectedError);
+		error_msg_[index] =
+		   (boost::format("GenericSaveHandler::save: unknown error: %s\n")
+		   % e.what()).str();
+		log("%s", error_msg_[index].c_str());
+	}
+
+	return error_;
+}
+
+std::string GenericSaveHandler::error_message(GenericSaveHandler::Error error_mask) {
+	uint32_t error_uint =
+	   static_cast<uint32_t>(error_mask) & static_cast<uint32_t>(error_);
+	std::string err_msg;
+	for (uint32_t index = 0; index < maxErrors_; index++) {
+		if ((error_uint >> index) & 1) {
+			err_msg += error_msg_[index];
+		}
+	}
+	return err_msg;
+}
+
+std::string GenericSaveHandler::localized_formatted_result_message() {
+	std::string msg;
+
+	if (error_ == Error::kSuccess) {
+		return _("File successfully saved!");
+	}
+
+	if (error_ == Error::kDeletingBackupFailed) {
+		return std::string(_("File successfully saved!")) + "\n" +
+		   (boost::format(_("Backup file ‘%s’ could not be deleted."))).str();
+	}
+
+	if (error_ == Error::kCreatingDirFailed) {
+		return
+		   (boost::format(_("Directory ‘%s’ could not be created!"))
+		   % dir_).str();
+	}
+
+	if (error_ == 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_ & Error::kSavingDataFailed) != Error::kNone) {
+		msg =
+		   (boost::format(_("Error writing data to file ‘%s’!"))
+		   % complete_filename_.c_str()).str();
+	}
+
+	if ((error_ & Error::kCorruptFileLeft) != Error::kNone) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg += (boost::format(_("Saved file may be corrupt!"))).str();
+	}
+
+	if ((error_ & Error::kRestoringBackupFailed) != Error::kNone) {
+		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_ & Error::kSavingDataFailed) != Error::kNone) &&
+	    !((error_ & Error::kCorruptFileLeft) != Error::kNone) &&
+	    !((error_ & Error::kRestoringBackupFailed) != Error::kNone)) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg +=
+		   (boost::format(_("File ‘%s’ was restored from backup."))
+		   % complete_filename_.c_str()).str();
+	}
+
+	if ((error_ & Error::kUnexpectedError) != Error::kNone) {
+		if (!msg.empty()) {
+			msg += "\n";
+		}
+		msg +=
+		   (boost::format(_("An unexpected error occurred:" ))).str()
+		   + "\n" + error_message(Error::kUnexpectedError);
+	}
+
+	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	2019-02-20 09:56:48 +0000
@@ -0,0 +1,151 @@
+/*
+ * 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"
+
+/**
+ * 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; also usable as bit masks
+	enum class Error : uint32_t {
+		kNone                  =   0,
+		kSuccess               =   0,
+		kCreatingDirFailed     =   1,
+		kBackupFailed          =   2,
+		kDeletingBackupFailed  =   4,
+		kSavingDataFailed      =   8,
+		kCorruptFileLeft       =  16,
+		kRestoringBackupFailed =  32,
+		kUnexpectedError       =  64,
+		kAllErrors             = 127
+	};
+
+	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_(Error::kSuccess) {};
+     error_(static_cast<Error>(1132)) {};
+
+	/**
+	 * 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).
+	 */
+	Error save();
+
+	// returns the stored error code (of the last saving operation)
+	Error error() { return error_; };
+
+	// Returns the combination of error_messages (of occurred errors)
+	// specified by a bit mask.
+	std::string error_message(Error error_mask = Error::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_;
+
+	Error error_;
+
+	static constexpr uint32_t maxErrors_ = 7;
+	static_assert(
+	   (1ul << maxErrors_) == static_cast<uint32_t>(Error::kAllErrors) + 1,
+	   "value of maxErrors_ doesn't match!");
+	std::string error_msg_[maxErrors_];
+
+	// Returns the lowest array index of the an error.
+	// Intended for use with single errors to get their array index.
+	uint32_t get_index(Error);
+
+	void clear();
+
+	// 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 yet.
+	// Stores an errorcode and error message (if applicable).
+	void save_file();
+};
+
+
+inline constexpr GenericSaveHandler::Error
+operator|(GenericSaveHandler::Error e1, GenericSaveHandler::Error e2) {
+	return static_cast<GenericSaveHandler::Error>
+	   (static_cast<uint32_t>(e1) | static_cast<uint32_t>(e2));
+}
+
+inline constexpr GenericSaveHandler::Error
+operator&(GenericSaveHandler::Error e1, GenericSaveHandler::Error e2) {
+	return static_cast<GenericSaveHandler::Error>
+	   (static_cast<uint32_t>(e1) & static_cast<uint32_t>(e2));
+}
+
+inline GenericSaveHandler::Error&
+operator|=(GenericSaveHandler::Error& e1, GenericSaveHandler::Error e2) {
+	return e1 = e1 | e2;
+}
+
+inline GenericSaveHandler::Error&
+operator&=(GenericSaveHandler::Error& e1, GenericSaveHandler::Error e2) {
+	return e1 = e1 & e2;
+}
+
+#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	2019-02-20 09:56:48 +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::Error::kSuccess ||
+	    gsh.error() == GenericSaveHandler::Error::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	2019-02-20 09:56:48 +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	2019-02-12 08:02:22 +0000
+++ src/wlapplication.cc	2019-02-20 09:56:48 +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,8 +213,8 @@
 	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) {
 	tm tfile;
@@ -340,6 +341,7 @@
 	cleanup_replays();
 	cleanup_ai_files();
 	cleanup_temp_files();
+	cleanup_temp_backups();
 
 	Section& config = g_options.pull_section("global");
 
@@ -1458,7 +1460,7 @@
 }
 
 /**
- * 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() {
@@ -1467,29 +1469,16 @@
 		     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);
-			assert(filename.length() > kSyncstreamExtension.length());
-			std::string filename_wse =
-			   filename.substr(0, filename.length() - kSyncstreamExtension.length());
-			filename_wse += kSyncstreamExcerptExtension;
-			if (g_fs->file_exists(filename_wse)) {
-				log("Delete syncstream excerpt %s\n", filename_wse.c_str());
-				g_fs->fs_unlink(filename_wse);
+		if (is_autogenerated_and_expired(filename, kReplayKeepAroundTime)) {
+			log("Delete syncstream or replay %s\n", filename.c_str());
+			try {
+				g_fs->fs_unlink(filename);
+			} catch (const FileError& e) {
+				log("WLApplication::cleanup_replays: File %s couldn't be deleted.\n",
+				    filename.c_str());
 			}
 		}
 	}
-
-	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)) {
-			log("Deleting replay %s\n", filename.c_str());
-			g_fs->fs_unlink(filename);
-			g_fs->fs_unlink(filename + kSavegameExtension);
-		}
-	}
 }
 
 /**
@@ -1502,7 +1491,12 @@
 	     })) {
 		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());
+			}
 		}
 	}
 }
@@ -1512,13 +1506,59 @@
  */
 void WLApplication::cleanup_temp_files() {
 	for (const std::string& filename :
-	     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);
+		   })) {
 		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	2019-02-20 09:56:48 +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	2019-02-20 09:56:48 +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	2019-02-20 09:56:48 +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,74 @@
 	}
 	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
+	);
+	GenericSaveHandler::Error error = gsh.save();
+
+	// If only the temporary backup couldn't be deleted, we still treat it as
+	// success. Automatic cleanup will deal with later. No need to bother the
+	// player with it.
+	if (error == GenericSaveHandler::Error::kSuccess ||
+	    error == GenericSaveHandler::Error::kDeletingBackupFailed) {
+		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::Error::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	2019-02-20 09:56:48 +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);