widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-1664052-expedition-shipwindow-crash into lp:widelands.

Commit message:
Do not delete/recreate buttons in the ship window. This fixes a segfault with the button signals. This also makes NoteShipWindow::Action::kRefresh obsolete.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1664052 in widelands: "Memory corruption in the gui"
  https://bugs.launchpad.net/widelands/+bug/1664052

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1664052-expedition-shipwindow-crash/+merge/318986
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1664052-expedition-shipwindow-crash into lp:widelands.

=== modified file 'src/economy/portdock.cc'
--- src/economy/portdock.cc	2017-02-22 20:23:49 +0000
+++ src/economy/portdock.cc	2017-03-04 15:08:32 +0000
@@ -336,7 +336,6 @@
 			// The expedition goods are now on the ship, so from now on it is independent from the port
 			// and thus we switch the port to normal, so we could even start a new expedition,
 			cancel_expedition(game);
-			Notifications::publish(NoteShipWindow(ship.serial(), NoteShipWindow::Action::kRefresh));
 			return fleet_->update(game);
 		}
 	}

=== modified file 'src/logic/map_objects/tribes/ship.cc'
--- src/logic/map_objects/tribes/ship.cc	2017-02-22 20:23:49 +0000
+++ src/logic/map_objects/tribes/ship.cc	2017-03-04 15:08:32 +0000
@@ -680,8 +680,6 @@
 			}
 
 			expedition_.reset(nullptr);
-
-			Notifications::publish(NoteShipWindow(serial(), NoteShipWindow::Action::kRefresh));
 			return start_task_idle(game, descr().main_animation(), 1500);
 		}
 	}
@@ -924,9 +922,6 @@
 
 	// Delete the expedition and the economy it created.
 	expedition_.reset(nullptr);
-
-	// And finally update our ship window
-	Notifications::publish(NoteShipWindow(serial(), NoteShipWindow::Action::kRefresh));
 }
 
 /// Sinks the ship

=== modified file 'src/logic/map_objects/tribes/ship.h'
--- src/logic/map_objects/tribes/ship.h	2017-02-22 20:23:49 +0000
+++ src/logic/map_objects/tribes/ship.h	2017-03-04 15:08:32 +0000
@@ -59,7 +59,8 @@
 
 	Serial serial;
 
-	enum class Action { kRefresh, kClose };
+	// TODO(GunChleoc): We leave the 1 action for now, because notes will be merged in the shiplist branch.
+	enum class Action { kClose };
 	const Action action;
 
 	NoteShipWindow(Serial init_serial, const Action& init_action)

=== modified file 'src/wui/shipwindow.cc'
--- src/wui/shipwindow.cc	2017-02-25 13:27:40 +0000
+++ src/wui/shipwindow.cc	2017-03-04 15:08:32 +0000
@@ -51,132 +51,127 @@
 using namespace Widelands;
 
 ShipWindow::ShipWindow(InteractiveGameBase& igb, UniqueWindow::Registry& reg, Ship& ship)
-   : UniqueWindow(&igb, "shipwindow", &reg, 0, 0, ship.get_shipname()), igbase_(igb), ship_(ship) {
-	init(false);
-	shipnotes_subscriber_ = Notifications::subscribe<Widelands::NoteShipWindow>(
-	   [this](const Widelands::NoteShipWindow& note) {
-		   if (note.serial == ship_.serial()) {
-			   switch (note.action) {
-			   // The ship state has changed, e.g. expedition canceled
-			   case Widelands::NoteShipWindow::Action::kRefresh:
-				   init(true);
-				   break;
-			   // The ship is no more
-			   case Widelands::NoteShipWindow::Action::kClose:
-				   // Stop this from thinking to avoid segfaults
-				   set_thinks(false);
-				   die();
-				   break;
-			   default:
-				   break;
-			   }
-		   }
-		});
-}
-
-void ShipWindow::init(bool avoid_fastclick) {
+	: UniqueWindow(&igb, "shipwindow", &reg, 0, 0, ship.get_shipname()), igbase_(igb), ship_(ship),
+	  vbox_(this, 0, 0, UI::Box::Vertical),
+	  navigation_box_(&vbox_, 0, 0, UI::Box::Vertical),
+	  navigation_box_height_(0)
+{
+	vbox_.set_inner_spacing(5);
 	assert(ship_.get_owner());
 
-	vbox_.reset(new UI::Box(this, 0, 0, UI::Box::Vertical));
-
-	display_ = new ItemWaresDisplay(vbox_.get(), *ship_.get_owner());
+	display_ = new ItemWaresDisplay(&vbox_, *ship_.get_owner());
 	display_->set_capacity(ship_.descr().get_capacity());
-	vbox_->add(display_, UI::Box::Resizing::kAlign, UI::Align::kCenter);
+	vbox_.add(display_, UI::Box::Resizing::kAlign, UI::Align::kCenter);
 
 	// Expedition buttons
-	if (ship_.state_is_expedition()) {
-		UI::Box* exp_top = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
-		vbox_->add(exp_top, UI::Box::Resizing::kAlign, UI::Align::kCenter);
-		UI::Box* exp_mid = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
-		vbox_->add(exp_mid, UI::Box::Resizing::kAlign, UI::Align::kCenter);
-		UI::Box* exp_bot = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
-		vbox_->add(exp_bot, UI::Box::Resizing::kAlign, UI::Align::kCenter);
-
-		btn_scout_[WALK_NW - 1] =
-		   make_button(exp_top, "scnw", _("Scout towards the north west"), pic_scout_nw,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_NW));
-		exp_top->add(btn_scout_[WALK_NW - 1]);
-
-		btn_explore_island_cw_ = make_button(
-		   exp_top, "expcw", _("Explore the island’s coast clockwise"), pic_explore_cw,
-		   boost::bind(&ShipWindow::act_explore_island, this, IslandExploreDirection::kClockwise));
-		exp_top->add(btn_explore_island_cw_);
-
-		btn_scout_[WALK_NE - 1] =
-		   make_button(exp_top, "scne", _("Scout towards the north east"), pic_scout_ne,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_NE));
-		exp_top->add(btn_scout_[WALK_NE - 1]);
-
-		btn_scout_[WALK_W - 1] =
-		   make_button(exp_mid, "scw", _("Scout towards the west"), pic_scout_w,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_W));
-		exp_mid->add(btn_scout_[WALK_W - 1]);
-
-		btn_construct_port_ =
-		   make_button(exp_mid, "buildport", _("Construct a port at the current location"),
-		               pic_construct_port, boost::bind(&ShipWindow::act_construct_port, this));
-		exp_mid->add(btn_construct_port_);
-
-		btn_scout_[WALK_E - 1] =
-		   make_button(exp_mid, "sce", _("Scout towards the east"), pic_scout_e,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_E));
-		exp_mid->add(btn_scout_[WALK_E - 1]);
-
-		btn_scout_[WALK_SW - 1] =
-		   make_button(exp_bot, "scsw", _("Scout towards the south west"), pic_scout_sw,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_SW));
-		exp_bot->add(btn_scout_[WALK_SW - 1]);
-
-		btn_explore_island_ccw_ =
-		   make_button(exp_bot, "expccw", _("Explore the island’s coast counter clockwise"),
-		               pic_explore_ccw, boost::bind(&ShipWindow::act_explore_island, this,
-		                                            IslandExploreDirection::kCounterClockwise));
-		exp_bot->add(btn_explore_island_ccw_);
-
-		btn_scout_[WALK_SE - 1] =
-		   make_button(exp_bot, "scse", _("Scout towards the south east"), pic_scout_se,
-		               boost::bind(&ShipWindow::act_scout_towards, this, WALK_SE));
-		exp_bot->add(btn_scout_[WALK_SE - 1]);
-	}
+	UI::Box* exp_top = new UI::Box(&navigation_box_, 0, 0, UI::Box::Horizontal);
+	navigation_box_.add(exp_top, UI::Box::Resizing::kAlign, UI::Align::kCenter);
+	UI::Box* exp_mid = new UI::Box(&navigation_box_, 0, 0, UI::Box::Horizontal);
+	navigation_box_.add(exp_mid, UI::Box::Resizing::kAlign, UI::Align::kCenter);
+	UI::Box* exp_bot = new UI::Box(&navigation_box_, 0, 0, UI::Box::Horizontal);
+	navigation_box_.add(exp_bot, UI::Box::Resizing::kAlign, UI::Align::kCenter);
+
+	btn_scout_[WALK_NW - 1] =
+		make_button(exp_top, "scnw", _("Scout towards the north west"), pic_scout_nw,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_NW));
+	exp_top->add(btn_scout_[WALK_NW - 1]);
+
+	btn_explore_island_cw_ = make_button(
+		exp_top, "expcw", _("Explore the island’s coast clockwise"), pic_explore_cw,
+		boost::bind(&ShipWindow::act_explore_island, this, IslandExploreDirection::kClockwise));
+	exp_top->add(btn_explore_island_cw_);
+
+	btn_scout_[WALK_NE - 1] =
+		make_button(exp_top, "scne", _("Scout towards the north east"), pic_scout_ne,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_NE));
+	exp_top->add(btn_scout_[WALK_NE - 1]);
+
+	btn_scout_[WALK_W - 1] =
+		make_button(exp_mid, "scw", _("Scout towards the west"), pic_scout_w,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_W));
+	exp_mid->add(btn_scout_[WALK_W - 1]);
+
+	btn_construct_port_ =
+		make_button(exp_mid, "buildport", _("Construct a port at the current location"),
+						pic_construct_port, boost::bind(&ShipWindow::act_construct_port, this));
+	exp_mid->add(btn_construct_port_);
+
+	btn_scout_[WALK_E - 1] =
+		make_button(exp_mid, "sce", _("Scout towards the east"), pic_scout_e,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_E));
+	exp_mid->add(btn_scout_[WALK_E - 1]);
+
+	btn_scout_[WALK_SW - 1] =
+		make_button(exp_bot, "scsw", _("Scout towards the south west"), pic_scout_sw,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_SW));
+	exp_bot->add(btn_scout_[WALK_SW - 1]);
+
+	btn_explore_island_ccw_ =
+		make_button(exp_bot, "expccw", _("Explore the island’s coast counter clockwise"),
+						pic_explore_ccw, boost::bind(&ShipWindow::act_explore_island, this,
+															  IslandExploreDirection::kCounterClockwise));
+	exp_bot->add(btn_explore_island_ccw_);
+
+	btn_scout_[WALK_SE - 1] =
+		make_button(exp_bot, "scse", _("Scout towards the south east"), pic_scout_se,
+						boost::bind(&ShipWindow::act_scout_towards, this, WALK_SE));
+	exp_bot->add(btn_scout_[WALK_SE - 1]);
+
+	vbox_.add(&navigation_box_, UI::Box::Resizing::kAlign, UI::Align::kCenter);
+	navigation_box_height_ = navigation_box_.get_h();
+	navigation_box_.set_visible(false);
 
 	// Bottom buttons
-	UI::Box* buttons = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
-	vbox_->add(buttons);
+	UI::Box* buttons = new UI::Box(&vbox_, 0, 0, UI::Box::Horizontal);
+	vbox_.add(buttons);
 
 	btn_goto_ = make_button(
-	   buttons, "goto", _("Go to ship"), pic_goto, boost::bind(&ShipWindow::act_goto, this));
+		buttons, "goto", _("Go to ship"), pic_goto, boost::bind(&ShipWindow::act_goto, this));
 	buttons->add(btn_goto_);
 	btn_destination_ = make_button(buttons, "destination", _("Go to destination"), pic_destination,
-	                               boost::bind(&ShipWindow::act_destination, this));
+											 boost::bind(&ShipWindow::act_destination, this));
 	btn_destination_->set_enabled(false);
 	buttons->add(btn_destination_);
 
 	btn_sink_ = make_button(
-	   buttons, "sink", _("Sink the ship"), pic_sink, boost::bind(&ShipWindow::act_sink, this));
+		buttons, "sink", _("Sink the ship"), pic_sink, boost::bind(&ShipWindow::act_sink, this));
 	buttons->add(btn_sink_);
 
-	if (ship_.state_is_expedition()) {
-		btn_cancel_expedition_ =
-		   make_button(buttons, "cancel_expedition", _("Cancel the Expedition"),
-		               pic_cancel_expedition, boost::bind(&ShipWindow::act_cancel_expedition, this));
-		buttons->add(btn_cancel_expedition_);
-	}
+	btn_cancel_expedition_ =
+		make_button(buttons, "cancel_expedition", _("Cancel the Expedition"),
+						pic_cancel_expedition, boost::bind(&ShipWindow::act_cancel_expedition, this));
+	buttons->add(btn_cancel_expedition_);
 
 	if (igbase_.get_display_flag(InteractiveBase::dfDebug)) {
 		btn_debug_ = make_button(buttons, "debug", _("Show Debug Window"), pic_debug,
-		                         boost::bind(&ShipWindow::act_debug, this));
+										 boost::bind(&ShipWindow::act_debug, this));
 		btn_debug_->set_enabled(true);
 		buttons->add(btn_debug_);
 	}
-	set_center_panel(vbox_.get());
+	set_center_panel(&vbox_);
 	set_thinks(true);
 	set_fastclick_panel(btn_goto_);
-	if (!avoid_fastclick) {
-		move_out_of_the_way();
-		warp_mouse_to_fastclick_panel();
-	}
+	move_out_of_the_way();
+	warp_mouse_to_fastclick_panel();
+
+	shipnotes_subscriber_ = Notifications::subscribe<Widelands::NoteShipWindow>(
+	   [this](const Widelands::NoteShipWindow& note) {
+		   if (note.serial == ship_.serial()) {
+			   switch (note.action) {
+				// TODO(GunChleoc): We leave the 1 action for now, because notes will be merged in the shiplist branch.
+			   case Widelands::NoteShipWindow::Action::kClose:
+				   // Stop thinking to avoid segfaults
+				   set_thinks(false);
+				   die();
+				   break;
+			   default:
+				   break;
+			   }
+		   }
+		});
 }
 
+
 void ShipWindow::think() {
 	UI::Window::think();
 	InteractiveBase* ib = ship_.get_owner()->egbase().get_ibase();
@@ -203,7 +198,14 @@
 	}
 
 	// Expedition specific buttons
-	Ship::ShipStates state = ship_.get_ship_state();
+	if (navigation_box_.is_visible() != ship_.state_is_expedition()) {
+		navigation_box_.set_visible(ship_.state_is_expedition());
+		navigation_box_.set_desired_size(navigation_box_.get_w(), ship_.state_is_expedition() ? navigation_box_height_ : 0);
+		btn_cancel_expedition_->set_visible(ship_.state_is_expedition());
+		btn_cancel_expedition_->set_enabled(ship_.state_is_expedition());
+		layout();
+	}
+
 	if (ship_.state_is_expedition()) {
 		/* The following rules apply:
 		 * - The "construct port" button is only active, if the ship is waiting for commands and found
@@ -216,6 +218,7 @@
 		 * matter if
 		 *   in waiting or already expedition/scouting mode)
 		 */
+		Ship::ShipStates state = ship_.get_ship_state();
 		btn_construct_port_->set_enabled(can_act &&
 		                                 (state == Ship::ShipStates::kExpeditionPortspaceFound));
 		bool coast_nearby = false;

=== modified file 'src/wui/shipwindow.h'
--- src/wui/shipwindow.h	2017-02-21 20:38:33 +0000
+++ src/wui/shipwindow.h	2017-03-04 15:08:32 +0000
@@ -39,9 +39,6 @@
 	ShipWindow(InteractiveGameBase& igb, UI::UniqueWindow::Registry& reg, Widelands::Ship& ship);
 
 private:
-	// Resets the vbox_ and fills it with the currently needed buttons, then positions the window.
-	void init(bool avoid_fastclick);
-
 	void think() override;
 
 	UI::Button* make_button(UI::Panel* parent,
@@ -62,7 +59,8 @@
 	InteractiveGameBase& igbase_;
 	Widelands::Ship& ship_;
 
-	std::unique_ptr<UI::Box> vbox_;
+	UI::Box vbox_;
+	UI::Box navigation_box_;
 	UI::Button* btn_goto_;
 	UI::Button* btn_destination_;
 	UI::Button* btn_sink_;
@@ -74,6 +72,7 @@
 	UI::Button* btn_scout_[Widelands::LAST_DIRECTION];
 	UI::Button* btn_construct_port_;
 	ItemWaresDisplay* display_;
+	int navigation_box_height_;
 	std::unique_ptr<Notifications::Subscriber<Widelands::NoteShipWindow>> shipnotes_subscriber_;
 	DISALLOW_COPY_AND_ASSIGN(ShipWindow);
 };