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.
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
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
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
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
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:
>
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,
7 matches
Mail list logo
