widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

Arty has proposed merging lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1798816 in widelands: "Replay menu does not respect Show filenames after deleting a replay"
  https://bugs.launchpad.net/widelands/+bug/1798816

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands.

=== 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-14 12:37:16 +0000
@@ -156,14 +156,35 @@
 	/** 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();
+		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>();
+		} 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(_("Creating directory ‘%s’ failed."))
+				   % 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() {

=== 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-14 12:37:16 +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,10 +66,12 @@
 	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));
 	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>,
@@ -82,7 +87,24 @@
 	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-14 12:37:16 +0000
@@ -52,6 +52,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-14 12:37:16 +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-14 12:37:16 +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-14 12:37:16 +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-14 12:37:16 +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-14 12:37:16 +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()) {
@@ -269,12 +276,69 @@
 		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()) {
+			// Notify the player.
+			std::string caption = ngettext("Error Deleting File!",
+			   "Error Deleting Files!", failed_selections.size());
+			if (filetype_ == FileType::kReplay) {
+				if (selections.size() == 1) {
+					header = _("The replay could not be deleted.");
+				} else {
+					header =
+					   (boost::format(ngettext("%s replay could not be deleted.",
+					   "%s replays could not be deleted.", failed_selections.size()))
+					   % failed_selections.size()).str();
+				}
+			} else {
+				if (selections.size() == 1) {
+					header = _("The game could not be deleted.");
+				} else {
+					header =
+					   (boost::format(ngettext("%s game could not be deleted.",
+					   "%s games could not be deleted.", failed_selections.size()))
+					   % failed_selections.size()).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,9 @@
 	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-14 12:37:16 +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_;