Merge lp:~widelands-dev/widelands/save_refactor into lp:widelands

Proposed by Klaus Halfmann
Status: Merged
Merged at revision: 8341
Proposed branch: lp:~widelands-dev/widelands/save_refactor
Merge into: lp:widelands
Diff against target: 386 lines (+151/-95)
4 files modified
src/logic/save_handler.cc (+125/-83)
src/logic/save_handler.h (+22/-10)
src/wlapplication.cc (+3/-1)
src/wui/buildingwindow.cc (+1/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/save_refactor
Reviewer Review Type Date Requested Status
Klaus Halfmann Approve
SirVer Needs Fixing
GunChleoc Approve
Review via email: mp+322622@code.launchpad.net

Commit message

SaveHandler refactoring + small code fixes in wlapplication and buildingwindow

- Pulled out functions in savehandler
- Added more comments
- Extracted functions from main saving-method
- Avoid reading config more then once per game
- Changed scheduling logic to use simpler operations
- Later declaration of Game variable in wlapplication.cc
- Replaced , at end of line with ; in src/wui/buildingwindow.cc

Description of the change

I refactored the SaveHandler

* added more comments
* extracted functions from main saving-method
* Avoid reading config more then once per game
* changed scheduling logic to use simpler operations

- Do we need a new transtlaition as I changed some user visible log message?
- regression_test.py was ok

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 2089. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/222841079.
Appveyor build 1924. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_save_refactor-1924.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Fiexed a typo and the trailing spaces travis complains about.

review: Needs Resubmitting
Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

LGTM - please have a look at my changes and at the commit message and then merge if you're happy.

review: Approve
Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

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

@bunnybot merge

review: Approve (compile, review)
Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 2093. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/223927463.

Revision history for this message
GunChleoc (gunchleoc) wrote :

The Travis site was having problems yesterday, so let's try this again.

@bunnybot merge

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 2096. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/224242140.

Revision history for this message
SirVer (sirver) :
review: Needs Fixing
Revision history for this message
GunChleoc (gunchleoc) wrote :

Oops, I didn't notice those. Quick push before bunnybot snaps it up for merging.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Sorry, the JavaIsms are all mine.

Checked the code again and played until the first autosave

review: Approve (compile, test, review)
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 2110. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/224614499.
Appveyor build 1945. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_save_refactor-1945.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

All fine, lets get this in:

@bunnybot merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/save_handler.cc'
2--- src/logic/save_handler.cc 2017-01-25 18:55:59 +0000
3+++ src/logic/save_handler.cc 2017-04-22 06:53:11 +0000
4@@ -37,25 +37,119 @@
5 #include "wlapplication.h"
6 #include "wui/interactive_base.h"
7
8+// The actual work of saving is done by the GameSaver
9 using Widelands::GameSaver;
10
11-/**
12-* Check if autosave is not needed.
13+SaveHandler::SaveHandler()
14+ : next_save_realtime_(0),
15+ initialized_(false),
16+ allow_saving_(true),
17+ save_requested_(false),
18+ saving_next_tick_(false),
19+ save_filename_(""),
20+ autosave_filename_("wl_autosave"),
21+ fs_type_(FileSystem::ZIP),
22+ autosave_interval_in_ms_(DEFAULT_AUTOSAVE_INTERVAL * 60 * 1000),
23+ number_of_rolls_(5)
24+{
25+}
26+
27+void SaveHandler::roll_save_files(const std::string& filename) {
28+
29+ int32_t rolls = number_of_rolls_;
30+ log("Autosave: Rolling savefiles (count): %d\n", rolls);
31+ rolls--;
32+ std::string filename_previous = create_file_name(
33+ get_base_dir(), (boost::format("%s_%02d") % filename % rolls).str());
34+ if (rolls > 0 && g_fs->file_exists(filename_previous)) {
35+ g_fs->fs_unlink(filename_previous); // Delete last of the rolling files
36+ log("Autosave: Deleted %s\n", filename_previous.c_str());
37+ }
38+ rolls--;
39+ while (rolls >= 0) {
40+ const std::string filename_next = create_file_name(
41+ get_base_dir(), (boost::format("%s_%02d") % filename % rolls).str());
42+ if (g_fs->file_exists(filename_next)) {
43+ g_fs->fs_rename(filename_next, filename_previous); // e.g. wl_autosave_08 -> wl_autosave_09
44+ log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
45+ }
46+ filename_previous = filename_next;
47+ rolls--;
48+ }
49+}
50+
51+/**
52+ * Check if game should be saved at next tick / think.
53+ *
54+ * @return true if game should be saved ad next think().
55+ */
56+bool SaveHandler::check_next_tick(Widelands::Game& game, uint32_t realtime) {
57+
58+ // Perhaps save is due now?
59+ if (autosave_interval_in_ms_ <= 0 || next_save_realtime_ > realtime) {
60+ return false; // no autosave or not due, yet
61+ }
62+
63+ next_save_realtime_ = realtime + autosave_interval_in_ms_;
64+
65+ // check if game is paused (in any way)
66+ if (game.game_controller()->is_paused_or_zero_speed()) {
67+ return false;
68+ }
69+
70+ log("Autosave: %d ms interval elapsed, current gametime: %s, saving...\n", autosave_interval_in_ms_,
71+ gametimestring(game.get_gametime(), true).c_str());
72+
73+ game.get_ibase()->log_message(_("Saving game…"));
74+ return true;
75+}
76+
77+/**
78+ * If saving fails restore the backup file.
79+ *
80+ * @return true when save was a success.
81+ */
82+bool SaveHandler::save_and_handle_error(Widelands::Game& game,
83+ const std::string& complete_filename,
84+ const std::string& backup_filename) {
85+ std::string error;
86+ bool result = save_game(game, complete_filename, &error);
87+ if (!result) {
88+ log("Autosave: ERROR! - %s\n", error.c_str());
89+ game.get_ibase()->log_message(_("Saving failed!"));
90+
91+ // if backup file was created, move it back
92+ if (backup_filename.length() > 0) {
93+ if (g_fs->file_exists(complete_filename)) {
94+ g_fs->fs_unlink(complete_filename);
95+ }
96+ g_fs->fs_rename(backup_filename, complete_filename);
97+ }
98+ // Wait 30 seconds until next save try
99+ next_save_realtime_ += 30000;
100+ } else {
101+ // if backup file was created, time to remove it
102+ if (backup_filename.length() > 0 && g_fs->file_exists(backup_filename))
103+ g_fs->fs_unlink(backup_filename);
104+ }
105+ return result;
106+}
107+
108+/**
109+ * Check if autosave is needed and allowed or save was requested by user.
110 */
111 void SaveHandler::think(Widelands::Game& game) {
112+
113+ if (!allow_saving_ || game.is_replay()) {
114+ return;
115+ }
116+
117 uint32_t realtime = SDL_GetTicks();
118 initialize(realtime);
119- std::string filename = autosave_filename_;
120-
121- if (!allow_saving_) {
122- return;
123- }
124- if (game.is_replay()) {
125- return;
126- }
127
128 // Are we saving now?
129 if (saving_next_tick_ || save_requested_) {
130+ std::string filename = autosave_filename_;
131 if (save_requested_) {
132 // Requested by user
133 if (!save_filename_.empty()) {
134@@ -66,27 +160,7 @@
135 save_filename_ = "";
136 } else {
137 // Autosave ...
138- // Roll savefiles
139- int32_t number_of_rolls =
140- g_options.pull_section("global").get_int("rolling_autosave", 5) - 1;
141- log("Autosave: Rolling savefiles (count): %d\n", number_of_rolls + 1);
142- std::string filename_previous = create_file_name(
143- get_base_dir(), (boost::format("%s_%02d") % filename % number_of_rolls).str());
144- if (number_of_rolls > 0 && g_fs->file_exists(filename_previous)) {
145- g_fs->fs_unlink(filename_previous);
146- log("Autosave: Deleted %s\n", filename_previous.c_str());
147- }
148- number_of_rolls--;
149- while (number_of_rolls >= 0) {
150- const std::string filename_next = create_file_name(
151- get_base_dir(), (boost::format("%s_%02d") % filename % number_of_rolls).str());
152- if (g_fs->file_exists(filename_next)) {
153- g_fs->fs_rename(filename_next, filename_previous);
154- log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
155- }
156- filename_previous = filename_next;
157- number_of_rolls--;
158- }
159+ roll_save_files(filename);
160 filename = (boost::format("%s_00") % autosave_filename_).str();
161 log("Autosave: saving as %s\n", filename.c_str());
162 }
163@@ -106,66 +180,37 @@
164 }
165
166 std::string error;
167- if (!save_game(game, complete_filename, &error)) {
168- log("Autosave: ERROR! - %s\n", error.c_str());
169- game.get_ibase()->log_message(_("Saving failed!"));
170-
171- // if backup file was created, move it back
172- if (backup_filename.length() > 0) {
173- if (g_fs->file_exists(complete_filename)) {
174- g_fs->fs_unlink(complete_filename);
175- }
176- g_fs->fs_rename(backup_filename, complete_filename);
177- }
178- // Wait 30 seconds until next save try
179- last_saved_realtime_ = last_saved_realtime_ + 30000;
180+ if (!save_and_handle_error(game, complete_filename, backup_filename)) {
181 return;
182- } else {
183- // if backup file was created, time to remove it
184- if (backup_filename.length() > 0 && g_fs->file_exists(backup_filename))
185- g_fs->fs_unlink(backup_filename);
186 }
187
188 log("Autosave: save took %d ms\n", SDL_GetTicks() - realtime);
189 game.get_ibase()->log_message(_("Game saved"));
190- last_saved_realtime_ = realtime;
191 saving_next_tick_ = false;
192
193 } else {
194- // Perhaps save is due now?
195- const int32_t autosave_interval_in_seconds =
196- g_options.pull_section("global").get_int("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60);
197- if (autosave_interval_in_seconds <= 0) {
198- return; // no autosave requested
199- }
200-
201- const int32_t elapsed = (realtime - last_saved_realtime_) / 1000;
202- if (elapsed < autosave_interval_in_seconds) {
203- return;
204- }
205-
206- // check if game is paused (in any way)
207- if (game.game_controller()->is_paused_or_zero_speed()) {
208- // Wait 30 seconds until next save try
209- last_saved_realtime_ = last_saved_realtime_ + 30000;
210- return;
211- }
212- log("Autosave: %d s interval elapsed, current gametime: %s, saving...\n", elapsed,
213- gametimestring(game.get_gametime(), true).c_str());
214-
215- saving_next_tick_ = true;
216- game.get_ibase()->log_message(_("Saving game…"));
217+ saving_next_tick_ = check_next_tick(game, realtime);
218 }
219 }
220
221 /**
222-* Initialize autosave timer
223+ * Lazy intialisation on first call.
224 */
225 void SaveHandler::initialize(uint32_t realtime) {
226 if (initialized_)
227 return;
228
229- last_saved_realtime_ = realtime;
230+ Section& global = g_options.pull_section("global");
231+
232+ fs_type_ = global.get_bool("nozip", false) ? FileSystem::DIR : FileSystem::ZIP;
233+
234+ autosave_interval_in_ms_ =
235+ global.get_int("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60) * 1000;
236+
237+ next_save_realtime_ = realtime + autosave_interval_in_ms_;
238+
239+ number_of_rolls_ = global.get_int("rolling_autosave", 5);
240+
241 initialized_ = true;
242 }
243
244@@ -188,26 +233,23 @@
245 }
246
247 /*
248- * Save the game
249- *
250- * returns true if saved
251+ * Save the game using the GameSaver.
252+ *
253+ * Will copy text of exceptions to error string.
254+ *
255+ * returns true if saved, false in case some error occured.
256 */
257 bool SaveHandler::save_game(Widelands::Game& game,
258 const std::string& complete_filename,
259 std::string* const error) {
260 ScopedTimer save_timer("SaveHandler::save_game() took %ums");
261
262- bool const binary = !g_options.pull_section("global").get_bool("nozip", false);
263 // Make sure that the base directory exists
264 g_fs->ensure_directory_exists(get_base_dir());
265
266 // Make a filesystem out of this
267 std::unique_ptr<FileSystem> fs;
268- if (!binary) {
269- fs.reset(g_fs->create_sub_file_system(complete_filename, FileSystem::DIR));
270- } else {
271- fs.reset(g_fs->create_sub_file_system(complete_filename, FileSystem::ZIP));
272- }
273+ fs.reset(g_fs->create_sub_file_system(complete_filename, fs_type_));
274
275 bool result = true;
276 GameSaver gs(*fs, game);
277
278=== modified file 'src/logic/save_handler.h'
279--- src/logic/save_handler.h 2017-01-25 18:55:59 +0000
280+++ src/logic/save_handler.h 2017-04-22 06:53:11 +0000
281@@ -25,6 +25,8 @@
282
283 #include <stdint.h>
284
285+#include "io/filesystem/filesystem.h"
286+
287 namespace Widelands {
288 class Game;
289 }
290@@ -32,17 +34,15 @@
291 // default autosave interval in minutes
292 #define DEFAULT_AUTOSAVE_INTERVAL 15
293
294+/**
295+ * Takes care of manual or autosave via think().
296+ *
297+ * Note that this handler is used for replays via the ReplayWriter, too.
298+ */
299 class SaveHandler {
300 public:
301- SaveHandler()
302- : last_saved_realtime_(0),
303- initialized_(false),
304- allow_saving_(true),
305- save_requested_(false),
306- saving_next_tick_(false),
307- save_filename_(""),
308- autosave_filename_("wl_autosave") {
309- }
310+ SaveHandler();
311+
312 void think(Widelands::Game&);
313 std::string create_file_name(const std::string& dir, const std::string& filename) const;
314 bool save_game(Widelands::Game&, const std::string& filename, std::string* error = nullptr);
315@@ -59,19 +59,22 @@
316 void set_autosave_filename(const std::string& filename) {
317 autosave_filename_ = filename;
318 }
319+ // Used by lua only
320 void set_allow_saving(bool t) {
321 allow_saving_ = t;
322 }
323+ // Used by lua only
324 bool get_allow_saving() {
325 return allow_saving_;
326 }
327+ // Used by lua only
328 void request_save(const std::string& filename = "") {
329 save_requested_ = true;
330 save_filename_ = filename;
331 }
332
333 private:
334- uint32_t last_saved_realtime_;
335+ uint32_t next_save_realtime_;
336 bool initialized_;
337 bool allow_saving_;
338 bool save_requested_;
339@@ -80,7 +83,16 @@
340 std::string current_filename_;
341 std::string autosave_filename_;
342
343+ FileSystem::Type fs_type_;
344+ int32_t autosave_interval_in_ms_;
345+ int32_t number_of_rolls_; // For rolling file update
346+
347 void initialize(uint32_t gametime);
348+ void roll_save_files(const std::string& filename);
349+ bool check_next_tick(Widelands::Game& game, uint32_t realtime);
350+ bool save_and_handle_error(Widelands::Game& game,
351+ const std::string& complete_filename, const std::string& backup_filename);
352+
353 };
354
355 #endif // end of include guard: WL_LOGIC_SAVE_HANDLER_H
356
357=== modified file 'src/wlapplication.cc'
358--- src/wlapplication.cc 2017-03-25 15:32:49 +0000
359+++ src/wlapplication.cc 2017-04-22 06:53:11 +0000
360@@ -1206,11 +1206,13 @@
361 SinglePlayerGameSettingsProvider sp;
362 FullscreenMenuLaunchSPG lgm(&sp);
363 const FullscreenMenuBase::MenuTarget code = lgm.run<FullscreenMenuBase::MenuTarget>();
364- Widelands::Game game;
365
366 if (code == FullscreenMenuBase::MenuTarget::kBack) {
367 return false;
368 }
369+
370+ Widelands::Game game;
371+
372 if (code == FullscreenMenuBase::MenuTarget::kScenarioGame) { // scenario
373 try {
374 game.run_splayer_scenario_direct(sp.get_map().c_str(), "");
375
376=== modified file 'src/wui/buildingwindow.cc'
377--- src/wui/buildingwindow.cc 2017-03-02 08:43:30 +0000
378+++ src/wui/buildingwindow.cc 2017-04-22 06:53:11 +0000
379@@ -98,7 +98,7 @@
380 capscache_ = 0;
381 caps_setup_ = false;
382 toggle_workarea_ = nullptr;
383- avoid_fastclick_ = avoid_fastclick,
384+ avoid_fastclick_ = avoid_fastclick;
385
386 vbox_.reset(new UI::Box(this, 0, 0, UI::Box::Vertical));
387

Subscribers

People subscribed via source and target branches

to status/vote changes: