widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

> I want to remove the dependency from logic to wui,

that is a noble and excellent goal!

> and I think the Notification system is a good way of doing that.

I designed it with this idea in mind - to decouple user interface from logic.

I think the big problem in this change is the tracking of the open ship windows - I'd try getting rid of that by subscribing again for each ship window. 


Diff comments:

> 
> === modified file 'src/logic/map_objects/tribes/ship.cc'
> --- src/logic/map_objects/tribes/ship.cc	2016-09-07 09:30:49 +0000
> +++ src/logic/map_objects/tribes/ship.cc	2016-09-28 15:34:04 +0000
> @@ -926,8 +924,7 @@
>  	expedition_.reset(nullptr);
>  
>  	// And finally update our ship window
> -	if (upcast(InteractiveGameBase, igb, game.get_ibase()))

getting rid of game.get_ibase() would be a big win too!

> -		refresh_window(*igb);
> +	Notifications::publish(NoteShipWindow(*this, NoteShipWindow::Action::kRefresh));
>  }
>  
>  /// Sinks the ship
> 
> === modified file 'src/logic/map_objects/tribes/ship.h'
> --- src/logic/map_objects/tribes/ship.h	2016-09-07 09:30:49 +0000
> +++ src/logic/map_objects/tribes/ship.h	2016-09-28 15:34:04 +0000
> @@ -59,6 +54,19 @@
>  	}
>  };
>  
> +struct NoteShipWindow {
> +	CAN_BE_SENT_AS_NOTE(NoteId::ShipWindow)
> +
> +	Ship& ship;

I am unsure if this is save. It assumes that 'ship' is valid when this notification is processed - e.g. if the ship is deleting itself. I think the notification system does not queue up notifications, but runs them immediately - so it should be save. But could you doublecheck?

Also, can this be const ref? The observers should only query state, not change it.

> +
> +	enum class Action { kRefresh, kRemove, kClosed };
> +	const Action action;
> +
> +	NoteShipWindow(Ship& init_ship, const Action& init_action)
> +		: ship(init_ship), action(init_action) {
> +	}
> +};
> +
>  class ShipDescr : public BobDescr {
>  public:
>  	ShipDescr(const std::string& init_descname, const LuaTable& t);
> 
> === modified file 'src/wui/interactive_gamebase.cc'
> --- src/wui/interactive_gamebase.cc	2016-08-04 15:49:05 +0000
> +++ src/wui/interactive_gamebase.cc	2016-09-28 15:34:04 +0000
> @@ -63,6 +64,52 @@
>       toggle_buildhelp_(INIT_BTN(
>          "wui/menus/menu_toggle_buildhelp", "buildhelp", _("Show Building Spaces (on/off)"))) {
>  	toggle_buildhelp_.sigclicked.connect(boost::bind(&InteractiveGameBase::toggle_buildhelp, this));
> +
> +	shipnotes_subscriber_ = Notifications::subscribe<Widelands::NoteShipWindow>(

Pull out into a OnNoteShipWindow member function? it reduces right drift and makes the constructor easier to read.

> +	   [this](const Widelands::NoteShipWindow& note) {
> +		   const Widelands::Serial serial = note.ship.serial();
> +		   switch (note.action) {
> +		   // The ship state has changed or the user requested a new window
> +		   case Widelands::NoteShipWindow::Action::kRefresh: {
> +			   bool is_refreshing = false;
> +			   Point pos(0, 0);
> +			   ShipWindow* shipwindow;
> +			   if (shipwindows_.count(serial) == 1) {

I think you do not require shipwindows_. Instead give each ShipWindow the serial of the ship it is watching and create a new NoteShipWindow observer there - it can then refresh itself if needed. It makes sense to pull the request for opening a ship window and for updating it into sparate messages for notification. 

Alternatively, you can add boost::signal2 signals to the ship and signal that on changes - that is what we do in the UI for callbacks. I like the notifications approach better for now - until it proves to inefficient.

> +				   shipwindow = shipwindows_.at(serial);
> +				   if (shipwindow) {
> +					   is_refreshing = true;
> +					   pos = shipwindow->get_pos();
> +					   delete shipwindow;
> +					   shipwindow = nullptr;
> +				   }
> +			   }
> +			   shipwindow = new ShipWindow(*this, note.ship, is_refreshing);
> +			   shipwindows_.insert(std::pair<Widelands::Serial, ShipWindow*>(serial, shipwindow));
> +			   if (is_refreshing) {
> +				   shipwindow->set_pos(pos);
> +			   }
> +
> +		   } break;
> +		   // The ship has been removed
> +		   case Widelands::NoteShipWindow::Action::kRemove: {
> +			   if (shipwindows_.count(serial) == 1) {
> +				   ShipWindow* shipwindow = shipwindows_.at(serial);
> +				   if (shipwindow) {
> +					   delete shipwindow;
> +					   shipwindow = nullptr;
> +				   }
> +				   shipwindows_.erase(serial);
> +			   }
> +
> +		   } break;
> +		   // The user closed the window
> +		   case Widelands::NoteShipWindow::Action::kClosed: {
> +			   if (shipwindows_.count(serial) == 1) {
> +				   shipwindows_.erase(serial);
> +			   }
> +		   } break;
> +		   }
> +		});
>  }
>  
>  /// \return a pointer to the running \ref Game instance.


-- 
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.