Review: Needs Fixing
The code for the fix looks good to me - there are quite a lot of other code
changes though, and I don't know what they are for (see diff comments).
I agree with merging the improved comments.
Diff comments:
> === modified file 'compile.sh'
> --- compile.sh 2016-10-26 06:54:00 +0000
> +++ compile.sh 2017-02-12 13:44:05 +0000
> @@ -158,8 +158,8 @@
> bzr pull
> cd build
> $buildtool
> -rm ../VERSION || true
> -rm ../widelands || true
> +rm -f ../VERSION || true
> +rm -f ../widelands || true
Changes to compile.sh don't belong in this branch.
> mv VERSION ../VERSION
> mv src/widelands ../widelands
> cd ..
>
> === modified file 'src/economy/expedition_bootstrap.h'
> --- src/economy/expedition_bootstrap.h 2017-02-09 19:23:44 +0000
> +++ src/economy/expedition_bootstrap.h 2017-02-12 13:44:05 +0000
> @@ -38,9 +38,16 @@
> class Warehouse;
> class Worker;
>
> -// Handles the mustering of workers and wares in a port for one Expedition.
> This
> -// object is created in the port dock as soon as the start expedition button
> is
> -// pressed. As soon as the ship is loaded, this object gets destroyed.
> +/**
> + * Handles the mustering of workers and wares in a port for an Expedition.
> + *
> + * This object is created in the port dock as soon as the start expedition
> button is
> + * pressed. As soon as the ship is loaded, this object gets destroyed.
> + * In case the Expedition is ::cancel()ed the wares will be returned to the
> port dock.
> + * Its the responsibility of the Owner to finally dispose a canceled
> ExpeditionBootstrap.
It's
> + *
> + */
> +
> class ExpeditionBootstrap {
> public:
> ExpeditionBootstrap(PortDock* const portdock);
> @@ -73,11 +83,18 @@
> // Delete all wares we currently handle.
> void cleanup(EditorGameBase& egbase);
>
> - // Save/Load this into a file. The actual data is stored in the
> buildingdata
> - // packet, and there in the warehouse data packet. The version
> parameter is
> - // the version number of the Warehouse packet.
Add the version parameter info back in.
> - void
> - load(Warehouse& warehouse, FileRead& fr, Game& game, MapObjectLoader&
> mol, uint16_t version);
> + /** Load this from a file.
> + *
> + * The actual data is stored in the buildingdata
> + * packet, and there in the warehouse data packet.
> + */
> + void load(Warehouse& warehouse, FileRead& fr, Game& game,
> MapObjectLoader& mol, uint16_t version);
> +
> + /** Save this into a file.
> + *
> + * The actual data is stored in the buildingdata
> + * packet, and there in the warehouse data packet.
> + */
> void save(FileWrite& fw, Game& game, MapObjectSaver& mos);
>
> private:
>
> === modified file 'src/graphic/align.h'
> --- src/graphic/align.h 2017-01-25 18:55:59 +0000
> +++ src/graphic/align.h 2017-02-12 13:44:05 +0000
> @@ -24,29 +24,41 @@
>
> namespace UI {
>
> +/**
> + * This Enum is a binary mix of one-dimensional and two-dimensional
> alignments.
> + *
> + * bits 0,1 values 0,1,2,3 are horizontal
Blank spaces after ,
> + * bits 2,3 values 0,4,8,12 are vertical
> + *
> + * mixed aligenments are results of a binary | operation.
> + *
> + */
> +
> + // TODO(klaus.halfmann): as this is not a real enum all compiler warnings
> about
> + // incomplete usage are useless.
> +
> enum class Align {
> - kLeft = 0,
> - kHCenter = 1,
> - kRight = 2,
> - kHorizontal = 3,
> -
> - kTop = 0,
> - kVCenter = 4,
> - kBottom = 8,
> - kVertical = 12,
> -
> - kTopLeft = 0,
> - kCenterLeft = Align::kVCenter,
> - kBottomLeft = Align::kBottom,
> -
> - kTopCenter = Align::kHCenter,
> - kCenter = Align::kHCenter | Align::kVCenter,
> - kBottomCenter = Align::kHCenter | Align::kBottom,
> -
> - kTopRight = Align::kRight,
> - kCenterRight = Align::kRight | Align::kVCenter,
> -
> - kBottomRight = Align::kRight | Align::kBottom,
> + kLeft = 0x00,
What is the reason for this change?
> + kHCenter = 0x01,
> + kRight = 0x02,
> + kHorizontal = 0x03,
> +
> + kTop = 0x00,
> + kVCenter = 0x04,
> + kBottom = 0x08,
> + kVertical = 0x0C,
> +
> + kTopLeft = kLeft | kTop,
> + kCenterLeft = kLeft | kVCenter,
> + kBottomLeft = kLeft | kBottom,
> +
> + kTopCenter = kHCenter | kTop,
> + kCenter = kHCenter | kVCenter,
> + kBottomCenter = kHCenter | kBottom,
> +
> + kTopRight = kRight | kTop,
> + kCenterRight = kRight | kVCenter,
> + kBottomRight = kRight | kBottom,
> };
>
> inline Align operator&(Align a, Align b) {
>
> === modified file 'src/notifications/notifications_impl.h'
> --- src/notifications/notifications_impl.h 2017-01-25 18:55:59 +0000
> +++ src/notifications/notifications_impl.h 2017-02-12 13:44:05 +0000
> @@ -70,7 +70,8 @@
>
> // Publishes 'message' to all subscribers.
> template <typename T> void publish(const T& message) {
> - for (void* p_subscriber :
> note_id_to_subscribers_[T::note_id()]) {
> + std::list<void*> subscribers = note_id_to_subscribers_[T::note_id()];
> + for (void* p_subscriber : subscribers) {
This change doesn't do anything - I find the older version easier to read.
> Subscriber<T>* subscriber =
> static_cast<Subscriber<T>*>(p_subscriber);
> subscriber->callback_(message);
> }
>
> === modified file 'src/ui_basic/box.cc'
> --- src/ui_basic/box.cc 2017-01-25 18:55:59 +0000
> +++ src/ui_basic/box.cc 2017-02-12 13:44:05 +0000
> @@ -376,16 +377,15 @@
> maxbreadth = get_inner_w();
> }
> switch (it.u.panel.align) {
> - case UI::Align::kHCenter:
> - breadth = (maxbreadth - breadth) / 2;
> - break;
> + case UI::Align::kHCenter:
> + breadth = (maxbreadth - breadth) >> 1;
Which problem does this solve?
> + break;
>
> - case UI::Align::kRight:
> - breadth = maxbreadth - breadth;
> - break;
> - case UI::Align::kLeft:
> - default:
> - breadth = 0;
> + case UI::Align::kRight:
> + breadth = maxbreadth - breadth;
> + break;
> + default: // UI::Align::left
> + breadth = 0;
> }
>
> if (orientation_ == Horizontal)
>
> === modified file 'src/wui/buildingwindow.cc'
> --- src/wui/buildingwindow.cc 2017-01-28 14:53:28 +0000
> +++ src/wui/buildingwindow.cc 2017-02-12 13:44:05 +0000
> @@ -170,6 +167,15 @@
> expeditionbtn_->sigclicked.connect(
>
> boost::bind(&BuildingWindow::act_start_or_cancel_expedition,
> boost::ref(*this)));
> capsbuttons->add(expeditionbtn_,
> UI::Align::kHCenter);
> +
> + expedition_canceled_subscriber_ =
> +
> Notifications::subscribe<Widelands::NoteExpeditionCanceled>([this, pd](
> + const Widelands::NoteExpeditionCanceled& canceld) {
canceld -> canceled
> + // Check this was not just any but our Expedition
> + if (canceld.bootstrap == pd->expedition_bootstrap())
> {
> + update_expedition_button(true);
> + }
> + });
> }
> } else if (upcast(const Widelands::ProductionSite,
> productionsite, &building_)) {
> if (!is_a(Widelands::MilitarySite, productionsite)) {
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1658489-epedition-tab/+merge/317047
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1658489-epedition-tab.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
