Merge lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands

[Arty]

Improved error handling for file save dialogs

In Load/Save menu for games/replays:
- All deletion errors are caught, player gets a message.
- When deleting multiplayer replays, also the corresponding syncstream file is deleted.
- After deleting replays, the table now respects the Show Filenames setting.

In Save menu for maps:
- Errors when trying to create a directory are now caught, player gets a message.
- Directory creation now doesn't allow names with map extensions, because the game would just trying to interpret them as maps and not show as directories. A tooltip is shown.
- Directory creation can now also be triggered by pressing Enter in the name edit box.

Other file errors are caught and logged:
- syncstream deletion in SyncWrapper destructor
- in Map::get_correct_loader
- file deletion/renaming in GameClient::handle_packet

[Arty (artydent)]

Some of the "handling" is just catching and logging.

In GameClient::handle_packet, when renaming a file to a backup fails, we could possibly handle this by finding another filename to save to, but I think this would have to be agreed upon by the participating clients. I didn't attempt this, because I am not yet familiar with the network code.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4226. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/454985800.
Appveyor build 4022. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_handling_various_fs_errors-4022.

[GunChleoc (gunchleoc)]

I have done a code review via diff comments. Not tested yet.

How would you feel about trying your hand at code reviews too?

https://code.launchpad.net/widelands/+activereviews

Your C++ is already a lot better than mine was when I started doing them :)

review: Needs Fixing

[Arty (artydent)]

Fixed those things.

Sure, I can do code reviews. I have looked at other active reviews before but mostly ignored them because they were either trivial one-line fixes or part of the code I wasn't familiar with at all yet.
Anything in particular I should look out for in code reviews?

[GunChleoc (gunchleoc)]

Just look at it to see if anything catches your attention that doesn't look right and test the change.

You can also do the code review only and leave the testing to somebody else if you don't have time to test as well.

The purpose of reviews is to help avoid introducing new bugs and to prevent spaghetti code and to make the code easy to read by having a consistent code style.

[GunChleoc (gunchleoc)]

Code LGTM, still needs testing.

I have also added a reply to one of your comments.

review: Approve

[GunChleoc (gunchleoc)]

Testing done:

- When a directory with the name already exists, the "Create directory" dialog is closed. It would be good to reopen it with the name pre-filled.

review: Needs Fixing

[GunChleoc (gunchleoc)]

Tested and all good now :)

@bunnybot merge

review: Approve

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2018-11-12 06:52:50 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2018-11-19 20:04:35 +0000
@@ -155,15 +155,42 @@
 void MainMenuSaveMap::clicked_make_directory() {
 	/** TRANSLATORS: A folder that hasn't been given a name yet */
 	MainMenuSaveMapMakeDirectory md(this, _("unnamed"));
-	if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
-		g_fs->ensure_directory_exists(curdir_);
-		//  Create directory.
-		std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname();
-		// Trim it for preceding/trailing whitespaces in user input
-		boost::trim(fullname);
-		g_fs->make_directory(fullname);
-		fill_table();
+	bool open_dialogue = true;
+	while (open_dialogue) {
+		open_dialogue = false;
+		if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
+			std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname();
+			// Trim it for preceding/trailing whitespaces in user input
+			boost::trim(fullname);
+			if (g_fs->file_exists(fullname)) {
+				const std::string s =
+				   _("A file or directory with that name already exists.");
+				UI::WLMessageBox mbox(this, _("Error Creating Directory!"), s,
+				                      UI::WLMessageBox::MBoxType::kOk);
+				mbox.run<UI::Panel::Returncodes>();
+				open_dialogue = true;
+			} else {
+				try {
+					g_fs->ensure_directory_exists(curdir_);
+					//  Create directory.
+					g_fs->make_directory(fullname);
+				} catch (const FileError& e) {
+					log("directory creation failed in MainMenuSaveMap::"
+					    "clicked_make_directory: %s\n", e.what());
+					const std::string s =
+					   (boost::format(_("Error while creating directory ‘%s’."))
+					   % fullname).str();
+					UI::WLMessageBox mbox(this, _("Error Creating Directory!"), s,
+					                      UI::WLMessageBox::MBoxType::kOk);
+					mbox.run<UI::Panel::Returncodes>();
+				}
+				fill_table();
+			}
+		}
 	}
+	table_.focus();
+	// TODO(Arty): In case of successful dir creation we should select the
+	// new dir in the table.
 }
 
 void MainMenuSaveMap::clicked_edit_options() {
@@ -256,8 +283,9 @@
 	boost::trim(filename);
 
 	//  OK, first check if the extension matches (ignoring case).
-	if (!boost::iends_with(filename, kWidelandsMapExtension))
+	if (!boost::iends_with(filename, kWidelandsMapExtension)) {
 		filename += kWidelandsMapExtension;
+	}
 
 	//  Append directory name.
 	const std::string complete_filename = curdir_ + g_fs->file_separator() + filename;
@@ -269,8 +297,9 @@
 		    FileSystem::fs_filename(filename.c_str()))
 		      .str();
 		UI::WLMessageBox mbox(this, _("Error Saving Map!"), s, UI::WLMessageBox::MBoxType::kOkCancel);
-		if (mbox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kBack)
+		if (mbox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kBack) {
 			return false;
+		}
 	}
 
 	//  Try deleting file (if it exists). If it fails, give a message and let the player choose a new

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc'
--- src/editor/ui_menus/main_menu_save_map_make_directory.cc	2018-07-08 13:53:45 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.cc	2018-11-19 20:04:35 +0000
@@ -19,9 +19,12 @@
 
 #include "editor/ui_menus/main_menu_save_map_make_directory.h"
 
+#include <boost/algorithm/string.hpp>
+
 #include "base/i18n.h"
 #include "graphic/font_handler.h"
 #include "io/filesystem/layered_filesystem.h"
+#include "logic/filesystem_constants.h"
 
 MainMenuSaveMapMakeDirectory::MainMenuSaveMapMakeDirectory(UI::Panel* const parent,
                                                            char const* dirname)
@@ -63,15 +66,23 @@
 	vbox_.set_size(get_inner_w() - 2 * padding_, get_inner_h() - 3 * padding_ - buth_);
 
 	edit_.set_text(dirname_);
-	edit_.changed.connect(boost::bind(&MainMenuSaveMapMakeDirectory::edit_changed, this));
+	edit_.changed.connect(
+	   boost::bind(&MainMenuSaveMapMakeDirectory::edit_changed, this));
+	edit_.ok.connect(
+	   boost::bind(&MainMenuSaveMapMakeDirectory::clicked_ok, this));
+	edit_.cancel.connect(
+	   boost::bind(&MainMenuSaveMapMakeDirectory::end_modal<UI::Panel::Returncodes>,
+	               boost::ref(*this), UI::Panel::Returncodes::kBack));
 	ok_button_.sigclicked.connect(
-	   boost::bind(&MainMenuSaveMapMakeDirectory::end_modal<UI::Panel::Returncodes>,
-	               boost::ref(*this), UI::Panel::Returncodes::kOk));
+	   boost::bind(&MainMenuSaveMapMakeDirectory::clicked_ok, this));
 	ok_button_.set_enabled(!dirname_.empty());
 	cancel_button_.sigclicked.connect(
 	   boost::bind(&MainMenuSaveMapMakeDirectory::end_modal<UI::Panel::Returncodes>,
 	               boost::ref(*this), UI::Panel::Returncodes::kBack));
 	center_to_parent();
+}
+
+void MainMenuSaveMapMakeDirectory::start() {
 	edit_.focus();
 }
 
@@ -82,7 +93,25 @@
 	const std::string& text = edit_.text();
 	// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
 	const bool is_legal_filename = FileSystem::is_legal_filename(text);
-	ok_button_.set_enabled(is_legal_filename);
-	edit_.set_tooltip(is_legal_filename ? "" : illegal_filename_tooltip_);
+	// Prevent the user from creating directory names that the game would
+	// try to interpret as maps
+	const bool has_map_extension =
+		 boost::iends_with(text, kWidelandsMapExtension) ||
+		 boost::iends_with(text, kS2MapExtension1) ||
+		 boost::iends_with(text, kS2MapExtension2);
+	ok_button_.set_enabled(is_legal_filename && !has_map_extension);
+	edit_.set_tooltip(is_legal_filename ?
+	   (has_map_extension ? _("This extension is reserved!") : "" ) :
+	   illegal_filename_tooltip_);
 	dirname_ = text;
 }
+
+/**
+ * Clicked OK button oder pressed Enter in edit box
+ */
+void MainMenuSaveMapMakeDirectory::clicked_ok() {
+	if (!ok_button_.enabled()) {
+		return;
+	}
+	end_modal<UI::Panel::Returncodes>(UI::Panel::Returncodes::kOk);
+}

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.h'
--- src/editor/ui_menus/main_menu_save_map_make_directory.h	2018-05-18 05:45:50 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.h	2018-11-19 20:04:35 +0000
@@ -37,6 +37,8 @@
 struct MainMenuSaveMapMakeDirectory : public UI::Window {
 	MainMenuSaveMapMakeDirectory(UI::Panel*, char const*);
 
+	void start() override;
+
 	char const* get_dirname() {
 		return dirname_.c_str();
 	}
@@ -52,6 +54,7 @@
 	UI::Button cancel_button_;
 	const std::string illegal_filename_tooltip_;
 	void edit_changed();
+	void clicked_ok();
 };
 
 #endif  // end of include guard: WL_EDITOR_UI_MENUS_MAIN_MENU_SAVE_MAP_MAKE_DIRECTORY_H

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2018-10-21 09:42:03 +0000
+++ src/logic/game.cc	2018-11-19 20:04:35 +0000
@@ -75,7 +75,13 @@
 Game::SyncWrapper::~SyncWrapper() {
 	if (dump_ != nullptr) {
 		if (!syncstreamsave_)
-			g_fs->fs_unlink(dumpfname_);
+			try {
+				g_fs->fs_unlink(dumpfname_);
+			} catch (const FileError& e) {
+				// not really a problem if deletion fails, but we'll log it
+				log("Deleting synchstream file %s failed: %s\n",
+				    dumpfname_.c_str(), e.what());
+			}
 	}
 }
 

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2018-05-22 11:29:41 +0000
+++ src/logic/map.cc	2018-11-19 20:04:35 +0000
@@ -32,6 +32,7 @@
 #include "build_info.h"
 #include "economy/flag.h"
 #include "economy/road.h"
+#include "io/filesystem/filesystem_exceptions.h"
 #include "io/filesystem/layered_filesystem.h"
 #include "logic/filesystem_constants.h"
 #include "logic/findimmovable.h"
@@ -1566,16 +1567,19 @@
 	std::string lower_filename = filename;
 	boost::algorithm::to_lower(lower_filename);
 
-	if (boost::algorithm::ends_with(lower_filename, kWidelandsMapExtension)) {
-		try {
-			result.reset(new WidelandsMapLoader(g_fs->make_sub_file_system(filename), this));
-		} catch (...) {
-			//  If this fails, it is an illegal file.
-			//  TODO(unknown): catchall hides real errors! Replace with more specific code
+	try {
+		if (boost::algorithm::ends_with(lower_filename, kWidelandsMapExtension)) {
+			result.reset(
+			   new WidelandsMapLoader(g_fs->make_sub_file_system(filename), this));
+		} else if (boost::algorithm::ends_with(lower_filename, kS2MapExtension1) ||
+		           boost::algorithm::ends_with(lower_filename, kS2MapExtension2)) {
+			result.reset(new S2MapLoader(filename, *this));
 		}
-	} else if (boost::algorithm::ends_with(lower_filename, kS2MapExtension1) ||
-	           boost::algorithm::ends_with(lower_filename, kS2MapExtension2)) {
-		result.reset(new S2MapLoader(filename, *this));
+	} catch (const FileError& e) {
+		// file might not have existed
+		log("Map::get_correct_loader: File error: %s\n", e.what());
+	} catch (std::exception& e) {
+		log("Map::get_correct_loader: Unknown error: %s\n", e.what());
 	}
 	return result;
 }

=== modified file 'src/network/gameclient.cc'
--- src/network/gameclient.cc	2018-11-06 17:05:10 +0000
+++ src/network/gameclient.cc	2018-11-19 20:04:35 +0000
@@ -25,6 +25,7 @@
 #include <boost/format.hpp>
 
 #include "base/i18n.h"
+#include "base/log.h"
 #include "base/warning.h"
 #include "base/wexception.h"
 #include "build_info.h"
@@ -32,6 +33,7 @@
 #include "game_io/game_loader.h"
 #include "helper.h"
 #include "io/fileread.h"
+#include "io/filesystem/filesystem_exceptions.h"
 #include "io/filewrite.h"
 #include "logic/filesystem_constants.h"
 #include "logic/game.h"
@@ -618,7 +620,14 @@
 				}
 			}
 			// Don't overwrite the file, better rename the original one
-			g_fs->fs_rename(path, backup_file_name(path));
+			try {
+				g_fs->fs_rename(path, backup_file_name(path));
+			} catch (const FileError& e) {
+				log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+				    "%s\n", e.what());
+				// TODO(Arty): What now? It just means the next step will fail
+				// or possibly result in some corrupt file
+			}
 		}
 
 		// Yes we need the file!
@@ -707,7 +716,12 @@
 				s.unsigned_8(NETCMD_CHAT);
 				s.string(_("/me 's file failed md5 checksumming."));
 				d->net->send(s);
-				g_fs->fs_unlink(file_->filename);
+				try {
+					g_fs->fs_unlink(file_->filename);
+				} catch (const FileError& e) {
+					log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+					    "%s\n", e.what());
+				}
 			}
 			// Check file for validity
 			bool invalid = false;
@@ -727,10 +741,15 @@
 					invalid = true;
 			}
 			if (invalid) {
-				g_fs->fs_unlink(file_->filename);
-				// Restore original file, if there was one before
-				if (g_fs->file_exists(backup_file_name(file_->filename)))
-					g_fs->fs_rename(backup_file_name(file_->filename), file_->filename);
+				try {
+					g_fs->fs_unlink(file_->filename);
+					// Restore original file, if there was one before
+					if (g_fs->file_exists(backup_file_name(file_->filename)))
+						g_fs->fs_rename(backup_file_name(file_->filename), file_->filename);
+				} catch (const FileError& e) {
+					log("file error in GameClient::handle_packet: case NETCMD_FILE_PART: "
+					    "%s\n", e.what());
+				}
 				s.reset();
 				s.unsigned_8(NETCMD_CHAT);
 				s.string(_("/me checked the received file. Although md5 check summing succeeded, "

=== modified file 'src/ui_fsmenu/loadgame.cc'
--- src/ui_fsmenu/loadgame.cc	2018-06-08 16:36:29 +0000
+++ src/ui_fsmenu/loadgame.cc	2018-11-19 20:04:35 +0000
@@ -158,7 +158,8 @@
 }
 
 void FullscreenMenuLoadGame::fill_table() {
-	load_or_save_.fill_table(showing_filenames_);
+	load_or_save_.set_show_filenames(showing_filenames_);
+	load_or_save_.fill_table();
 }
 
 const std::string& FullscreenMenuLoadGame::filename() const {

=== modified file 'src/wui/load_or_save_game.cc'
--- src/wui/load_or_save_game.cc	2018-10-26 07:09:29 +0000
+++ src/wui/load_or_save_game.cc	2018-11-19 20:04:35 +0000
@@ -32,6 +32,7 @@
 #include "game_io/game_preload_packet.h"
 #include "graphic/font_handler.h"
 #include "helper.h"
+#include "io/filesystem/filesystem_exceptions.h"
 #include "io/filesystem/layered_filesystem.h"
 #include "logic/filesystem_constants.h"
 #include "logic/game_controller.h"
@@ -74,6 +75,7 @@
      table_box_(new UI::Box(parent, 0, 0, UI::Box::Vertical)),
      table_(table_box_, 0, 0, 0, 0, style, UI::TableRows::kMultiDescending),
      filetype_(filetype),
+     show_filenames_(false),
      localize_autosave_(localize_autosave),
      // Savegame description
      game_details_(
@@ -136,8 +138,13 @@
 }
 
 const std::string LoadOrSaveGame::filename_list_string() const {
+	return filename_list_string(table_.selections());
+}
+
+const std::string
+LoadOrSaveGame::filename_list_string(const std::set<uint32_t>& selections) const {
 	boost::format message;
-	for (const uint32_t index : table_.selections()) {
+	for (const uint32_t index : selections) {
 		const SavegameData& gamedata = games_data_[table_.get(table_.get_record(index))];
 
 		if (gamedata.errormessage.empty()) {
@@ -263,18 +270,75 @@
 
 		UI::WLMessageBox confirmationBox(
 		   parent_->get_parent()->get_parent(),
-		   ngettext("Confirm Deleting File", "Confirm Deleting Files", no_selections), message,
+		   no_selections == 1 ? _("Confirm Deleting File") : _("Confirm Deleting Files"),
+		   message,
 		   UI::WLMessageBox::MBoxType::kOkCancel);
 		do_delete = confirmationBox.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk;
 		table_.focus();
 	}
 	if (do_delete) {
+		// Failed deletions aren't a serious problem, we just catch the errors
+		// and keep track to notify the player.
+		std::set<uint32_t> failed_selections;
+		bool failed;
 		for (const uint32_t index : selections) {
+			failed = false;
 			const std::string& deleteme = get_filename(index);
-			g_fs->fs_unlink(deleteme);
-			if (filetype_ == FileType::kReplay) {
-				g_fs->fs_unlink(deleteme + kSavegameExtension);
-			}
+			try {
+				g_fs->fs_unlink(deleteme);
+			} catch (const FileError& e) {
+				log("player-requested file deletion failed: %s", e.what());
+				failed = true;
+			}
+			if (filetype_ == FileType::kReplay) {
+				try {
+					g_fs->fs_unlink(deleteme + kSavegameExtension);
+					// If at least one of the two relevant files of a replay are
+					// successfully deleted then count it as success.
+					// (From the player perspective the replay is gone.)
+					failed = false;
+					// If it was a multiplayer replay, also delete the synchstream. Do
+					// it here, so it's only attempted if replay deletion was successful.
+					if (g_fs->file_exists(deleteme + kSyncstreamExtension)) {
+						g_fs->fs_unlink(deleteme + kSyncstreamExtension);
+					}
+				} catch (const FileError& e) {
+					log("player-requested file deletion failed: %s", e.what());
+				}
+			}
+			if (failed) {
+				failed_selections.insert(index);
+			}
+		}
+		if (!failed_selections.empty()) {
+			const uint32_t no_failed = failed_selections.size();
+			// Notify the player.
+			const std::string caption = (no_failed == 1) ?
+			   _("Error Deleting File!") : _("Error Deleting Files!");
+			if (filetype_ == FileType::kReplay) {
+				if (selections.size() == 1) {
+					header = _("The replay could not be deleted.");
+				} else {
+					header =
+					   (boost::format(ngettext("%d replay could not be deleted.",
+					   "%d replays could not be deleted.", no_failed)) % no_failed).str();
+				}
+			} else {
+				if (selections.size() == 1) {
+					header = _("The game could not be deleted.");
+				} else {
+					header =
+					   (boost::format(ngettext("%d game could not be deleted.",
+					   "%d games could not be deleted.", no_failed)) % no_failed).str();
+				}
+			}
+			std::string message = (boost::format("%s\n%s") % header
+			   % filename_list_string(failed_selections)).str();
+			UI::WLMessageBox msgBox(
+			   parent_->get_parent()->get_parent(),
+				 caption, message,
+			   UI::WLMessageBox::MBoxType::kOk);
+			msgBox.run<UI::Panel::Returncodes>();
 		}
 		fill_table();
 
@@ -297,7 +361,7 @@
 	return delete_;
 }
 
-void LoadOrSaveGame::fill_table(bool show_filenames) {
+void LoadOrSaveGame::fill_table() {
 
 	clear_selections();
 	table_.clear();
@@ -309,8 +373,9 @@
 			return boost::ends_with(fn, kReplayExtension);
 		});
 		// Update description column title for replays
-		table_.set_column_tooltip(2, show_filenames ? _("Filename: Map name (start of replay)") :
-		                                              _("Map name (start of replay)"));
+		table_.set_column_tooltip(2, show_filenames_ ?
+		   _("Filename: Map name (start of replay)") :
+		   _("Map name (start of replay)"));
 	} else {
 		gamefiles = g_fs->list_directory(kSaveDir);
 	}
@@ -459,7 +524,7 @@
 				te.set_string(1, gametypestring);
 				if (filetype_ == FileType::kReplay) {
 					const std::string map_basename =
-					   show_filenames ?
+					   show_filenames_ ?
 					      map_filename(gamedata.filename, gamedata.mapname, localize_autosave_) :
 					      gamedata.mapname;
 					te.set_string(2, (boost::format(pgettext("mapname_gametime", "%1% (%2%)")) %
@@ -472,7 +537,7 @@
 			} else {
 				te.set_string(1, map_filename(gamedata.filename, gamedata.mapname, localize_autosave_));
 			}
-		} catch (const WException& e) {
+		} catch (const std::exception& e) {
 			std::string errormessage = e.what();
 			boost::replace_all(errormessage, "\n", "<br>");
 			gamedata.errormessage =
@@ -501,3 +566,10 @@
 	table_.sort();
 	table_.focus();
 }
+
+void LoadOrSaveGame::set_show_filenames(bool show_filenames) {
+	if (filetype_ != FileType::kReplay) {
+		return;
+	}
+	show_filenames_ = show_filenames;
+}

=== modified file 'src/wui/load_or_save_game.h'
--- src/wui/load_or_save_game.h	2018-10-25 04:00:51 +0000
+++ src/wui/load_or_save_game.h	2018-11-19 20:04:35 +0000
@@ -57,7 +57,10 @@
 	void select_by_name(const std::string& name);
 
 	/// Read savegame/replay files and fill the table and games data.
-	void fill_table(bool show_filenames = false);
+	void fill_table();
+
+	/// Set whether to show filenames. Has only an effect for Replays.
+	void set_show_filenames(bool);
 
 	/// The table panel
 	UI::Table<uintptr_t const>& table();
@@ -80,6 +83,8 @@
 
 	/// Formats the current table selection as a list of filenames with savedate information.
 	const std::string filename_list_string() const;
+	/// Formats a given table selection as a list of filenames with savedate information.
+	const std::string filename_list_string(const std::set<uint32_t>& selections) const;
 
 	/// Reverse default sort order for save date column
 	bool compare_date_descending(uint32_t, uint32_t) const;
@@ -88,6 +93,7 @@
 	UI::Box* table_box_;
 	UI::Table<uintptr_t const> table_;
 	FileType filetype_;
+	bool show_filenames_;
 	bool localize_autosave_;
 	std::vector<SavegameData> games_data_;
 	GameDetails game_details_;