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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
