widelands-dev team mailing list archive

[TiborB <tiborb95@xxxxxxxxx>]

TiborB has proposed merging lp:~widelands-dev/widelands/bug-1566441_Saving_game into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1566441 in widelands: "String "Saving Game" appears too late "
  https://bugs.launchpad.net/widelands/+bug/1566441

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1566441_Saving_game/+merge/294720

Making test "Saving game..." appear before actual saving taking place.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1566441_Saving_game into lp:widelands.

=== modified file 'src/logic/game_controller.h'
--- src/logic/game_controller.h	2016-01-05 09:54:44 +0000
+++ src/logic/game_controller.h	2016-05-14 19:53:05 +0000
@@ -79,6 +79,13 @@
 	virtual bool is_paused() = 0;
 
 	/**
+	 * Whether the game is stopped.
+	 */
+	bool is_paused_or_zero_speed() {
+		return is_paused() || real_speed() == 0;
+	};
+
+	/**
 	 * Sets whether the game is paused.
 	 */
 	virtual void set_paused(const bool paused) = 0;

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2016-03-08 15:21:41 +0000
+++ src/logic/save_handler.cc	2016-05-14 19:53:05 +0000
@@ -28,6 +28,7 @@
 #include "base/log.h"
 #include "base/macros.h"
 #include "base/scoped_timer.h"
+#include "base/time_string.h"
 #include "base/wexception.h"
 #include "game_io/game_saver.h"
 #include "io/filesystem/filesystem.h"
@@ -53,15 +54,85 @@
 		return;
 	}
 
-	if (save_requested_) {
-		if (!save_filename_.empty()) {
-			filename = save_filename_;
-		}
-
-		log("Autosave: save requested : %s\n", filename.c_str());
-		save_requested_ = false;
-		save_filename_ = "";
+	// Are we saving now?
+	if (saving_next_tick_ || save_requested_) {
+		if (save_requested_) {
+			// Requested by user
+			if (!save_filename_.empty()) {
+				filename = save_filename_;
+			}
+			log("Gamesave: save requested: %s\n", filename.c_str());
+			save_requested_ = false;
+			save_filename_ = "";
+		} else {
+			// Autosave ...
+			// Roll savefiles
+			int32_t number_of_rolls = g_options.pull_section("global").get_int("rolling_autosave", 5) - 1;
+			log("Autosave: Rolling savefiles (count): %d\n", number_of_rolls + 1);
+			std::string filename_previous =
+				create_file_name(get_base_dir(), (boost::format("%s_%02d") % filename % number_of_rolls).str());
+			if (number_of_rolls > 0 && g_fs->file_exists(filename_previous)) {
+				g_fs->fs_unlink(filename_previous);
+				log("Autosave: Deleted %s\n", filename_previous.c_str());
+			}
+			number_of_rolls--;
+			while (number_of_rolls >= 0) {
+				const std::string filename_next =
+					create_file_name(get_base_dir(),
+						(boost::format("%s_%02d") % filename % number_of_rolls).str());
+				if (g_fs->file_exists(filename_next)) {
+					g_fs->fs_rename(filename_next, filename_previous);
+					log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
+				}
+				filename_previous = filename_next;
+				number_of_rolls--;
+			}
+			filename = (boost::format("%s_00") % autosave_filename_).str();
+			log("Autosave: saving as %s\n", filename.c_str());
+		}
+
+		// Saving now
+		const std::string complete_filename = create_file_name(get_base_dir(), filename);
+		std::string backup_filename;
+
+		// always overwrite a file
+		if (g_fs->file_exists(complete_filename)) {
+			filename += "2";
+			backup_filename = create_file_name (get_base_dir(), filename);
+			if (g_fs->file_exists(backup_filename)) {
+				g_fs->fs_unlink(backup_filename);
+			}
+			g_fs->fs_rename(complete_filename, backup_filename);
+		}
+
+		std::string error;
+		if (!save_game(game, complete_filename, &error)) {
+			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
+			last_saved_realtime_ = last_saved_realtime_ + 30000;
+			return;
+		} 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);
+		}
+
+		log("Autosave: save took %d ms\n", SDL_GetTicks() - realtime);
+		game.get_ibase()->log_message(_("Game saved"));
+		last_saved_realtime_ = realtime;
+		saving_next_tick_ =  false;
+
 	} else {
+		// Perhaps save is due now?
 		const int32_t autosave_interval_in_seconds =
 			g_options.pull_section("global").get_int("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60);
 		if (autosave_interval_in_seconds <= 0) {
@@ -73,76 +144,19 @@
 			return;
 		}
 
-		if (game.game_controller()->is_paused()) { // check if game is paused
+		// check if game is paused (in any way)
+		if (game.game_controller()->is_paused_or_zero_speed()) {
 			// Wait 30 seconds until next save try
 			last_saved_realtime_ = last_saved_realtime_ + 30000;
 			return;
 		}
-		// Roll autosaves
-		int32_t number_of_rolls = g_options.pull_section("global").get_int("rolling_autosave", 5) - 1;
-		std::string filename_previous =
-			create_file_name(get_base_dir(), (boost::format("%s_%02d") % filename % number_of_rolls).str());
-		if (number_of_rolls > 0 && g_fs->file_exists(filename_previous)) {
-			g_fs->fs_unlink(filename_previous);
-			log("Autosave: Deleted %s\n", filename_previous.c_str());
-		}
-		number_of_rolls--;
-		while (number_of_rolls >= 0) {
-			const std::string filename_next =
-				create_file_name(get_base_dir(), (boost::format("%s_%02d") % filename % number_of_rolls).str());
-			if (g_fs->file_exists(filename_next)) {
-				g_fs->fs_rename(filename_next, filename_previous);
-				log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
-			}
-			filename_previous = filename_next;
-			number_of_rolls--;
-		}
-		filename = (boost::format("%s_00") % autosave_filename_).str();
-		log("Autosave: interval elapsed (%d s), saving %s\n", elapsed, filename.c_str());
-	}
-
-	// TODO(unknown): defer saving to next tick so that this message is shown
-	// before the actual save, or put the saving logic in another thread
-	game.get_ibase()->log_message(_("Saving game…"));
-
-	// save the game
-	const std::string complete_filename = create_file_name(get_base_dir(), filename);
-	std::string backup_filename;
-
-	// always overwrite a file
-	if (g_fs->file_exists(complete_filename)) {
-		filename += "2";
-		backup_filename = create_file_name (get_base_dir(), filename);
-		if (g_fs->file_exists(backup_filename)) {
-			g_fs->fs_unlink(backup_filename);
-		}
-		g_fs->fs_rename(complete_filename, backup_filename);
-	}
-
-	std::string error;
-	if (!save_game(game, complete_filename, &error)) {
-		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
-		last_saved_realtime_ = last_saved_realtime_ + 30000;
-		return;
-	} 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);
-	}
-
-	log("Autosave: save took %d ms\n", SDL_GetTicks() - realtime);
-	game.get_ibase()->log_message(_("Game saved"));
-	last_saved_realtime_ = realtime;
+		log("Autosave: %d s interval elapsed, current gametime: %s, saving...\n",
+			elapsed,
+			gametimestring(game.get_gametime(), true).c_str());
+
+		saving_next_tick_ = true;
+		game.get_ibase()->log_message(_("Saving game…"));
+	}
 }
 
 /**

=== modified file 'src/logic/save_handler.h'
--- src/logic/save_handler.h	2016-02-21 20:02:09 +0000
+++ src/logic/save_handler.h	2016-05-14 19:53:05 +0000
@@ -33,7 +33,8 @@
 class SaveHandler {
 public:
 	SaveHandler() : last_saved_realtime_(0), initialized_(false), allow_saving_(true),
-		save_requested_(false), save_filename_(""), autosave_filename_("wl_autosave") {}
+		save_requested_(false), saving_next_tick_(false), save_filename_(""),
+		autosave_filename_("wl_autosave") {}
 	void think(Widelands::Game &);
 	std::string create_file_name(const std::string& dir, const std::string& filename) const;
 	bool save_game
@@ -58,6 +59,7 @@
 	bool initialized_;
 	bool allow_saving_;
 	bool save_requested_;
+	bool saving_next_tick_;
 	std::string save_filename_;
 	std::string current_filename_;
 	std::string autosave_filename_;