widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

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 :)

Diff comments:

> === 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.";

This string will always be in English - please mark it for translation.

> +			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."))

Suggestion: "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() {
> 
> === 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
> @@ -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())

Add {} please

> +		return;
> +	end_modal<UI::Panel::Returncodes>(UI::Panel::Returncodes::kOk);
> +}
> 
> === 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.)

Will these still be taken care of during the "old files" cleanup at startup? We don't need to fix it in this branch, but it's something worth investigating.

> +					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 used to do that too, but ngettext doesn't really work when there is no number in the string. In my language, the rule for 1 also matches 11.

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

We call failed_selections.size() a lot here - sticking it into a const unsigned int variable should make the code easier to read, and things easier to optimize for the compiler. Do not use size_t, because that's platform-dependent and the compilers will complain about the printf placeholder. uint8_t will be interpreted as char, so don't use that either. Generic unsigned int will do fine for us here :)

Also, %s is for char* - use %d for numbers. Same for games below.

> +					   "%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();
>  
> @@ -501,3 +566,9 @@
>  	table_.sort();
>  	table_.focus();
>  }
> +
> +void LoadOrSaveGame::set_show_filenames(bool show_filenames) {
> +	if (filetype_ != FileType::kReplay)

Add {}

> +		return;
> +	show_filenames_ = show_filenames;
> +}


-- 
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.