widelands-dev team mailing list archive

[cghislai <charlyghislain@xxxxxxxxx>]

cghislai has proposed merging lp:~widelands-dev/widelands/replay_fix into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1206211 in widelands: ""Follow" function in watch window crashes in replays or when playing as a spectator"
  https://bugs.launchpad.net/widelands/+bug/1206211
  Bug #1206441 in widelands: "Autosave leads to crash on replays"
  https://bugs.launchpad.net/widelands/+bug/1206441
  Bug #1206917 in widelands: "Pausing during save dialog behaves different in replays and games"
  https://bugs.launchpad.net/widelands/+bug/1206917

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/replay_fix/+merge/178109

This fixes a few issues related to replay/spectator mode.
- The Panel::die() method is now virtual. It is used to destroy Window and overwritten in the save dialog as well to restore the game speed
- Pausing is not handled in iteractive_spect anymore, only in savegame window
- Watch window does not rely on interactive player anymore
- Autosave does not rely on interactive player anymore
-- 
https://code.launchpad.net/~widelands-dev/widelands/replay_fix/+merge/178109
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/replay_fix into lp:widelands.

=== modified file 'src/save_handler.cc'
--- src/save_handler.cc	2013-07-26 05:57:03 +0000
+++ src/save_handler.cc	2013-08-01 16:01:48 +0000
@@ -25,9 +25,11 @@
 #include "log.h"
 #include "logic/game.h"
 #include "profile/profile.h"
+#include "upcast.h"
 #include "wexception.h"
 #include "wlapplication.h"
-#include "wui/interactive_player.h"
+#include "wui/interactive_base.h"
+#include "wui/interactive_gamebase.h"
 
 using Widelands::Game_Saver;
 
@@ -68,8 +70,10 @@
 
 	// TODO: 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_ipl()->get_chat_provider()->send_local
-		(_("Saving game..."));
+	upcast(Interactive_GameBase, igbase, game.get_ibase());
+	if (igbase) {
+		igbase->get_chat_provider()->send_local(_("Saving game..."));
+	}
 
 	// save the game
 	const std::string complete_filename = create_file_name(get_base_dir(), filename);
@@ -88,8 +92,7 @@
 	std::string error;
 	if (!save_game(game, complete_filename, &error)) {
 		log("Autosave: ERROR! - %s\n", error.c_str());
-		game.get_ipl()->get_chat_provider()->send_local
-			(_("Saving failed!"));
+		igbase->get_chat_provider()->send_local(_("Saving failed!"));
 
 		// if backup file was created, move it back
 		if (backup_filename.length() > 0) {
@@ -108,8 +111,9 @@
 	}
 
 	log("Autosave: save took %d ms\n", m_last_saved_time - realtime);
-	game.get_ipl()->get_chat_provider()->send_local
-		(_("Game saved"));
+	if (igbase) {
+		igbase->get_chat_provider()->send_local(_("Game saved"));
+	}
 }
 
 /**

=== modified file 'src/ui_basic/panel.h'
--- src/ui_basic/panel.h	2013-07-26 19:16:51 +0000
+++ src/ui_basic/panel.h	2013-08-01 16:01:48 +0000
@@ -235,7 +235,7 @@
 	std::string ui_fn();
 
 protected:
-	void die();
+	virtual void die();
 	bool keyboard_free() {return !(_focus);}
 
 	virtual void update_desired_size();

=== modified file 'src/ui_basic/unique_window.cc'
--- src/ui_basic/unique_window.cc	2013-07-26 20:19:36 +0000
+++ src/ui_basic/unique_window.cc	2013-08-01 16:01:48 +0000
@@ -42,7 +42,7 @@
 */
 void UniqueWindow::Registry::destroy() {
 	if (window) {
-		delete window;
+		window->die();
 	}
 }
 
@@ -51,7 +51,7 @@
 */
 void UniqueWindow::Registry::toggle() {
 	if (window) {
-		delete window;
+		window->die();
 	} else {
 		constr();
 	}

=== modified file 'src/ui_basic/window.cc'
--- src/ui_basic/window.cc	2013-08-01 03:25:26 +0000
+++ src/ui_basic/window.cc	2013-08-01 16:01:48 +0000
@@ -453,12 +453,7 @@
 		grab_mouse(true);
 	} else if (btn == SDL_BUTTON_RIGHT) {
 		play_click();
-		if (is_modal())
-			end_modal(0);
-		else
-			delete this; //  is this 100% safe?
-		//  FIXME No, at least provide a flag for making a window unclosable and
-		//  FIXME provide a callback.
+		die();
 	}
 
 	return true;
@@ -490,6 +485,20 @@
 	return true;
 }
 
+/**
+ * Close the window. Overwrite this virtual method if you want
+ * to take some action before the window is destroyed, or to
+ * prevent it
+ */
+void Window::die()
+{
+	if (is_modal()) {
+		end_modal(0);
+	} else {
+		Panel::die();
+	}
+}
+
 
 void Window::restore() {
 	assert(_is_minimal);

=== modified file 'src/ui_basic/window.h'
--- src/ui_basic/window.h	2013-07-26 20:19:36 +0000
+++ src/ui_basic/window.h	2013-08-01 16:01:48 +0000
@@ -90,6 +90,7 @@
 	bool handle_tooltip();
 
 protected:
+	virtual void die();
 	virtual void layout();
 	virtual void update_desired_size();
 

=== modified file 'src/wui/game_main_menu_save_game.cc'
--- src/wui/game_main_menu_save_game.cc	2013-07-26 20:19:36 +0000
+++ src/wui/game_main_menu_save_game.cc	2013-08-01 16:01:48 +0000
@@ -143,11 +143,7 @@
 	}
 
 	m_editbox->focus();
-	if (parent.game().get_ipl() && !parent.game().get_ipl()->is_multiplayer()) {
-		// Pause the game only if we are part of the game
-		// and not in multiplayer
-		parent.game().gameController()->setPaused(true);
-	}
+	pause_game(true);
 }
 
 
@@ -319,10 +315,8 @@
 
 void Game_Main_Menu_Save_Game::die()
 {
+	pause_game(false);
 	UI::UniqueWindow::die();
-	if (igbase().game().get_ipl() && !igbase().game().get_ipl()->is_multiplayer()) {
-		igbase().game().gameController()->setPaused(false);
-	}
 }
 
 
@@ -371,3 +365,14 @@
 	if (g_fs->FileExists(complete_filename))
 		new DeletionMessageBox(*this, complete_filename);
 }
+
+void Game_Main_Menu_Save_Game::pause_game(bool paused)
+{
+	Interactive_Player* ipl = igbase().get_game()->get_ipl();
+	if (ipl && ipl->is_multiplayer()) {
+		// Do not pause in multiplayer
+		return;
+	}
+	igbase().game().gameController()->setPaused(paused);
+}
+

=== modified file 'src/wui/game_main_menu_save_game.h'
--- src/wui/game_main_menu_save_game.h	2013-07-26 19:16:51 +0000
+++ src/wui/game_main_menu_save_game.h	2013-08-01 16:01:48 +0000
@@ -39,9 +39,10 @@
 
 	void fill_list();
 	void select_by_name(std::string name);
+protected:
+	virtual void die();
 private:
 	Interactive_GameBase & igbase();
-	void die();
 	void selected      (uint32_t);
 	void double_clicked(uint32_t);
 	void edit_box_changed();
@@ -49,6 +50,7 @@
 	void delete_clicked();
 
 	bool save_game(std::string);
+	void pause_game(bool paused);
 
 	UI::Listselect<std::string> m_ls;
 	UI::EditBox * m_editbox;

=== modified file 'src/wui/interactive_spectator.cc'
--- src/wui/interactive_spectator.cc	2013-07-26 20:19:36 +0000
+++ src/wui/interactive_spectator.cc	2013-08-01 16:01:48 +0000
@@ -185,7 +185,6 @@
 	if (m_mainm_windows.savegame.window)
 		delete m_mainm_windows.savegame.window;
 	else {
-		game().gameController()->setDesiredSpeed(0);
 		new Game_Main_Menu_Save_Game(*this, m_mainm_windows.savegame);
 	}
 }
@@ -259,7 +258,6 @@
 
 		case SDLK_s:
 			if (code.mod & (KMOD_LCTRL | KMOD_RCTRL)) {
-				game().gameController()->setDesiredSpeed(0);
 				new Game_Main_Menu_Save_Game(*this, m_mainm_windows.savegame);
 			} else
 				set_display_flag

=== modified file 'src/wui/watchwindow.cc'
--- src/wui/watchwindow.cc	2013-07-26 20:19:36 +0000
+++ src/wui/watchwindow.cc	2013-08-01 16:01:48 +0000
@@ -256,12 +256,14 @@
 		pos = bob->calc_drawpos(game(), pos);
 
 		Widelands::Map & map = game().map();
-		if (1 < game().get_ipl()->player().vision(map.get_index(bob->get_position(), map.get_width()))) {
+		// Drop the tracking of it leaves our vision range
+		Interactive_Player* ipl = game().get_ipl();
+		if (ipl && 1 >= ipl->player().vision(map.get_index(bob->get_position(), map.get_width()))) {
+			// Not in sight
+			views[cur_index].tracking = 0;
+		} else {
 			mapview.set_viewpoint
 				(pos - Point(mapview.get_w() / 2, mapview.get_h() / 2), false);
-		} else {
-			// stop tracking
-			views[cur_index].tracking = 0;
 		}
 	}
 
@@ -327,9 +329,10 @@
 			p = bob->calc_drawpos(g, p);
 			int32_t const dist =
 				MapviewPixelFunctions::calc_pix_distance(map, p, pos);
+			Interactive_Player* ipl = game().get_ipl();
 			if
 				((!closest || closest_dist > dist)
-				 && (1 < game().get_ipl()->player().vision(map.get_index(bob->get_position(), map.get_width()))))
+				 && (!ipl || 1 < ipl->player().vision(map.get_index(bob->get_position(), map.get_width()))))
 			{
 				closest = bob;
 				closest_dist = dist;