Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/save_refactor into lp:widelands

2017-04-23 Thread Klaus Halfmann
Review: Approve

All fine, lets get this in:

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-22 Thread Klaus Halfmann
Review: Approve compile, test, review

Sorry, the JavaIsms are all mine.

Checked the code again and played until the first autosave
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-21 Thread GunChleoc
Oops, I didn't notice those. Quick push before bunnybot snaps it up for merging.
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-21 Thread SirVer
Review: Needs Fixing



Diff comments:

> === modified file 'src/logic/save_handler.cc'
> --- src/logic/save_handler.cc 2017-01-25 18:55:59 +
> +++ src/logic/save_handler.cc 2017-04-21 16:12:49 +
> @@ -37,25 +37,119 @@
>  #include "wlapplication.h"
>  #include "wui/interactive_base.h"
>  
> +// The actual work of saving is done by the GameSaver
>  using Widelands::GameSaver;
>  
> -/**
> -* Check if autosave is not needed.
> +SaveHandler::SaveHandler()
> +   : next_save_realtime_(0),
> +initialized_(false),
> +allow_saving_(true),
> +save_requested_(false),
> +saving_next_tick_(false),
> +save_filename_(""),
> +autosave_filename_("wl_autosave"),
> +fs_type_(FileSystem::ZIP),
> +autosave_interval_in_ms_(DEFAULT_AUTOSAVE_INTERVAL * 60 * 1000),
> + number_of_rolls_(5)
> +{
> +}
> +
> +void SaveHandler::rollSaveFiles(const std::string& filename) {

Widelands style uses snake_case_for_functions. You have some Javaisms here :)

> +
> +int32_t rolls = number_of_rolls_;
> + log("Autosave: Rolling savefiles (count): %d\n", rolls);
> +rolls--;
> + std::string filename_previous = create_file_name(
> +get_base_dir(), (boost::format("%s_%02d") % filename % rolls).str());
> + if (rolls > 0 && g_fs->file_exists(filename_previous)) {
> + g_fs->fs_unlink(filename_previous); // Delete last of the 
> rolling files
> + log("Autosave: Deleted %s\n", filename_previous.c_str());
> + }
> + rolls--;
> + while (rolls >= 0) {
> + const std::string filename_next = create_file_name(
> +get_base_dir(), (boost::format("%s_%02d") % filename % 
> rolls).str());
> + if (g_fs->file_exists(filename_next)) {
> + g_fs->fs_rename(filename_next, filename_previous); // 
> e.g. wl_autosave_08 -> wl_autosave_09
> + log("Autosave: Rolled %s to %s\n", 
> filename_next.c_str(), filename_previous.c_str());
> + }
> + filename_previous = filename_next;
> + rolls--;
> + }
> +}
> +
> +/**
> + * Check if game should be saved at next tick / think.
> + *
> + * @return true if game should be saved ad next think().
> + */
> +bool SaveHandler::checkNextTick(Widelands::Game& game, uint32_t realtime) {
> +
> + // Perhaps save is due now?
> + if (autosave_interval_in_ms_ <= 0 || next_save_realtime_ > realtime) {
> + return false;  // no autosave or not due, yet
> + }
> +
> +next_save_realtime_ = realtime + autosave_interval_in_ms_;
> +
> + // check if game is paused (in any way)
> + if (game.game_controller()->is_paused_or_zero_speed()) {
> + return false;
> + }
> +
> + log("Autosave: %d ms interval elapsed, current gametime: %s, 
> saving...\n", autosave_interval_in_ms_,
> + gametimestring(game.get_gametime(), true).c_str());
> +
> + game.get_ibase()->log_message(_("Saving gameā€¦"));
> + return true;
> +}
> +
> +/**
> + * If saving fails restore the backup file.
> + *
> + * @return true when save was a success.
> + */
> +bool SaveHandler::saveAndHandleError(Widelands::Game& game,
> + const std::string& 
> complete_filename,
> + const std::string& 
> backup_filename) {
> + std::string error;
> +bool result = save_game(game, complete_filename, );
> + if (!result) {
> + log("Autosave: ERROR! - %s\n", error.c_str());
> + game.get_ibase()->log_message(_("Saving failed!"));
> +
> + // if backup file was created, move it back
> + if (backup_filename.length() > 0) {
> + if (g_fs->file_exists(complete_filename)) {
> + g_fs->fs_unlink(complete_filename);
> + }
> + g_fs->fs_rename(backup_filename, complete_filename);
> + }
> + // Wait 30 seconds until next save try
> + next_save_realtime_ += 3;
> + } else {
> + // if backup file was created, time to remove it
> + if (backup_filename.length() > 0 && 
> g_fs->file_exists(backup_filename))
> + g_fs->fs_unlink(backup_filename);
> + }
> +return result;
> +}
> +
> +/**
> + * Check if autosave is needed and allowed or save was requested by user.
>   */
>  void SaveHandler::think(Widelands::Game& game) {
> +
> + if (!allow_saving_ || game.is_replay()) {
> + return;
> + }
> +
>   uint32_t realtime = SDL_GetTicks();
>   initialize(realtime);
> - std::string filename = autosave_filename_;
> -
> - if (!allow_saving_) {
> - return;
> - }
> - if (game.is_replay()) {
> - return;
> - }
>  
>   // Are we saving now?
>   if (saving_next_tick_ || save_requested_) {
> +std::string filename = 

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/save_refactor into lp:widelands

2017-04-20 Thread Klaus Halfmann
Review: Approve compile, review

Thanks for the review,
I really should know about these member_variables_.

@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-20 Thread GunChleoc
Review: Approve

LGTM - please have a look at my changes and at the commit message and then 
merge if you're happy.
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-20 Thread GunChleoc
You still have a trailing whitespace - the easiest way to deal with those is to 
set your editor to automatically kill them whenever you save a file.
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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/save_refactor into lp:widelands

2017-04-18 Thread Klaus Halfmann
Review: Resubmit

Fiexed a typo and the trailing spaces travis complains about.
-- 
https://code.launchpad.net/~widelands-dev/widelands/save_refactor/+merge/322622
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/save_refactor.

___
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