Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
Review: Approve Tested and all good now :) @bunnybot merge -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
Review: Approve
Code LGTM, still needs testing.
I have also added a reply to one of your comments.
Diff comments:
>
> === modified file 'src/wui/load_or_save_game.cc'
> --- src/wui/load_or_save_game.cc 2018-10-26 07:09:29 +
> +++ src/wui/load_or_save_game.cc 2018-11-14 12:37:16 +
> @@ -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 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!",
Those are sins of the past ;)
> +"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();
> }
> 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.
___
Mailing list:
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
Review: Needs Fixing 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. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
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. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
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 +
> +++ src/wui/load_or_save_game.cc 2018-11-14 12:37:16 +
> @@ -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 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.",
>
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
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 +
> +++ src/editor/ui_menus/main_menu_save_map.cc 2018-11-14 12:37:16 +
> @@ -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::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();
> + } 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();
> + }
> + 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 +
> +++ src/editor/ui_menus/main_menu_save_map_make_directory.cc 2018-11-14
> 12:37:16 +
> @@ -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::kOk);
> +}
>
> === modified file 'src/wui/load_or_save_game.cc'
> --- src/wui/load_or_save_game.cc 2018-10-26 07:09:29 +
> +++ src/wui/load_or_save_game.cc 2018-11-14 12:37:16 +
> @@ -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 failed_selections;
> + bool failed;
> for (const uint32_t index : selections) {
> + failed = false;
> const std::string& deleteme = get_filename(index);
> - g_fs->fs_unlink(deleteme);
> -
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
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. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
