widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

lgtm. nice separation of concerns.

Diff comments:

> 
> === modified file 'src/wui/shipwindow.cc'
> --- src/wui/shipwindow.cc	2016-09-07 09:30:49 +0000
> +++ src/wui/shipwindow.cc	2016-09-30 18:45:06 +0000
> @@ -47,70 +46,51 @@
>  static const char pic_scout_sw[] = "images/wui/ship/ship_scout_sw.png";
>  static const char pic_scout_se[] = "images/wui/ship/ship_scout_se.png";
>  static const char pic_construct_port[] = "images/wui/editor/fsel_editor_set_port_space.png";
> -
> -namespace Widelands {
> -
> -/**
> - * Display information about a ship.
> - */
> -struct ShipWindow : UI::Window {
> -	ShipWindow(InteractiveGameBase& igb, Ship& ship, const std::string& title);
> -	virtual ~ShipWindow();
> -
> -	void think() override;
> -
> -	UI::Button* make_button(UI::Panel* parent,
> -	                        const std::string& name,
> -	                        const std::string& title,
> -	                        const std::string& picname,
> -	                        boost::function<void()> callback);
> -
> -	void act_goto();
> -	void act_destination();
> -	void act_sink();
> -	void act_debug();
> -	void act_cancel_expedition();
> -	void act_scout_towards(WalkingDir);
> -	void act_construct_port();
> -	void act_explore_island(IslandExploreDirection);
> -
> -private:
> -	InteractiveGameBase& igbase_;
> -	Ship& ship_;
> -
> -	UI::Button* btn_goto_;
> -	UI::Button* btn_destination_;
> -	UI::Button* btn_sink_;
> -	UI::Button* btn_debug_;
> -	UI::Button* btn_cancel_expedition_;
> -	UI::Button* btn_explore_island_cw_;
> -	UI::Button* btn_explore_island_ccw_;
> -	UI::Button*
> -	   btn_scout_[LAST_DIRECTION];  // format: DIRECTION - 1, as 0 is normally the current location.
> -	UI::Button* btn_construct_port_;
> -	ItemWaresDisplay* display_;
> -};
> -
> -ShipWindow::ShipWindow(InteractiveGameBase& igb, Ship& ship, const std::string& title)
> -   : Window(&igb, "shipwindow", 0, 0, 0, 0, title), igbase_(igb), ship_(ship) {
> -	assert(!ship_.window_);
> +}  // namespace
> +
> +using namespace Widelands;
> +
> +ShipWindow::ShipWindow(InteractiveGameBase& igb, Ship& ship)
> +   : Window(&igb, "shipwindow", 0, 0, 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

die is preferrable - the semantics are easier to understand and it has the same effect.

> +				   set_thinks(false);
> +				   die();
> +				   break;
> +			   default:
> +				   break;
> +			   }
> +		   }
> +		});
> +}
> +
> +void ShipWindow::init(bool avoid_fastclick) {
>  	assert(ship_.get_owner());
> -	ship_.window_ = this;
> -
> -	UI::Box* vbox = new UI::Box(this, 0, 0, UI::Box::Vertical);
> -
> -	display_ = new ItemWaresDisplay(vbox, *ship.get_owner());
> -	display_->set_capacity(ship.descr().get_capacity());
> -	vbox->add(display_, UI::Align::kHCenter, false);
> +
> +	vbox_.reset(new UI::Box(this, 0, 0, UI::Box::Vertical));
> +
> +	display_ = new ItemWaresDisplay(vbox_.get(), *ship_.get_owner());
> +	display_->set_capacity(ship_.descr().get_capacity());
> +	vbox_->add(display_, UI::Align::kHCenter, false);
>  
>  	// Expedition buttons
>  	if (ship_.state_is_expedition()) {
> -		UI::Box* exp_top = new UI::Box(vbox, 0, 0, UI::Box::Horizontal);
> -		vbox->add(exp_top, UI::Align::kHCenter, false);
> -		UI::Box* exp_mid = new UI::Box(vbox, 0, 0, UI::Box::Horizontal);
> -		vbox->add(exp_mid, UI::Align::kHCenter, false);
> -		UI::Box* exp_bot = new UI::Box(vbox, 0, 0, UI::Box::Horizontal);
> -		vbox->add(exp_bot, UI::Align::kHCenter, false);
> +		UI::Box* exp_top = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
> +		vbox_->add(exp_top, UI::Align::kHCenter, false);
> +		UI::Box* exp_mid = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
> +		vbox_->add(exp_mid, UI::Align::kHCenter, false);
> +		UI::Box* exp_bot = new UI::Box(vbox_.get(), 0, 0, UI::Box::Horizontal);
> +		vbox_->add(exp_bot, UI::Align::kHCenter, false);
>  
>  		btn_scout_[WALK_NW - 1] =
>  		   make_button(exp_top, "scnw", _("Scout towards the north west"), pic_scout_nw,


-- 
https://code.launchpad.net/~widelands-dev/widelands/notifications_shipwindow/+merge/307048
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/notifications_shipwindow.