widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

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?

Diff comments:

> 
> === 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
> @@ -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.)

Technically, in this branch a remaining savegame (when the deletion of the replay file succeeded) would not be cleaned up (but this issue was there before).
This is already fixed in the "robust file saving" branch though. When I added the new cleanup routine there, I also fixed the other cleanup routines with it (mainly catching file errors but also fixing this particular issue).

> +					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!",

I actually thought about this, but saw that in a Russian po-file all the plurals are defined for strings without placeholder anyway. Should also have checked whether it's actually displayed properly.
There was another older wrong use of this in this cc-file, I fixed this one, too. (Or shouldn't I have? I figured there'd be new translations to do anyway.)

> +			   "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();
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/handling-various-fs-errors.