Merge lp:~widelands-dev/widelands/save_refactor into lp:widelands
- save_refactor
- Merge into trunk
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 |
Related bugs: |
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/
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
bunnybot (widelandsofficial) wrote : | # |
Klaus Halfmann (klaus-halfmann) wrote : | # |
Fiexed a typo and the trailing spaces travis complains about.
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.
GunChleoc (gunchleoc) wrote : | # |
LGTM - please have a look at my changes and at the commit message and then merge if you're happy.
Klaus Halfmann (klaus-halfmann) wrote : | # |
Thanks for the review,
I really should know about these member_variables_.
@bunnybot merge
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:/
GunChleoc (gunchleoc) wrote : | # |
The Travis site was having problems yesterday, so let's try this again.
@bunnybot merge
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:/
SirVer (sirver) : | # |
GunChleoc (gunchleoc) wrote : | # |
Oops, I didn't notice those. Quick push before bunnybot snaps it up for merging.
Klaus Halfmann (klaus-halfmann) wrote : | # |
Sorry, the JavaIsms are all mine.
Checked the code again and played until the first autosave
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 2110. State: passed. Details: https:/
Appveyor build 1945. State: success. Details: https:/
Klaus Halfmann (klaus-halfmann) wrote : | # |
All fine, lets get this in:
@bunnybot merge
Preview Diff
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 |
Continuous integration builds have changed state:
Travis build 2089. State: failed. Details: https:/ /travis- ci.org/ widelands/ widelands/ builds/ 222841079. /ci.appveyor. com/project/ widelands- dev/widelands/ build/_ widelands_ dev_widelands_ save_refactor- 1924.
Appveyor build 1924. State: success. Details: https:/