Merge lp:~widelands-dev/widelands/bug-1658489-epedition-tab into lp:widelands

[Klaus Halfmann]

Fixed the bug in buildingwindow.cc + some minor changes.

Moral: Notifications are global. So when receiving one check if it is for you.

[kaputtnik (franku)]

As far i can see the bug is fixed :-)

But the indentation of code is broken. Looks like widelands c++ code uses tabs for indentation, whereas you use spaces?

review: Approve (testing)

[GunChleoc (gunchleoc)]

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.

review: Needs Fixing

[Klaus Halfmann (klaus-halfmann)]

Will clean up the code along your comments, thx.

[GunChleoc (gunchleoc)]

Sounds good to me :)

If you have an idea on how to get rid of the compiler warnings elegantly, please do go ahead. They are annoying, but behave differently with the different compilers.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1943. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/201326099.
Appveyor build 1778. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1658489_epedition_tab-1778.

[Klaus Halfmann (klaus-halfmann)]

Reworked that code going to compile/tets/push this now

[Klaus Halfmann (klaus-halfmann)]

Lets try again

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

You still have code changes in src/graphic/align.h and src/wui/building_ui.cc that are unrelated to this branch ;)

[Klaus Halfmann (klaus-halfmann)]

Mhh, you did annotate this as "needs fixing".
If you insist I will include these changes in some other branch ...

[GunChleoc (gunchleoc)]

Yes please. It makes for a confusing commit history (see my comments). I also like to "smuggle" in some small improvements here and there in a branch, but it must at least be documented and explained in the commit message. If the commit should break something, it will make it harder to find later on if it isn't commented.

[Klaus Halfmann (klaus-halfmann)]

Can this go in now?

I am vaction now and would like to start some other things...

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

Yes. Thanks for the fix and the cleanup :)

@bunnybot merge

review: Approve

=== modified file 'src/economy/expedition_bootstrap.cc'
--- src/economy/expedition_bootstrap.cc	2017-02-12 09:10:57 +0000
+++ src/economy/expedition_bootstrap.cc	2017-02-17 18:17:30 +0000
@@ -91,8 +91,12 @@
 		warehouse->refresh_options(*igb);
 }
 
+/**
+ * Cancel the Expediton by putting back all wares and workers.
+‚ */
 void ExpeditionBootstrap::cancel(Game& game) {
-	// Put all wares from the WaresQueues back into the warehouse
+
+    // Put all wares from the WaresQueues back into the warehouse
 	Warehouse* const warehouse = portdock_->get_warehouse();
 	for (std::unique_ptr<InputQueue>& iq : queues_) {
 		if (iq->get_type() == wwWARE) {
@@ -112,6 +116,7 @@
 	if (upcast(InteractiveGameBase, igb, warehouse->owner().egbase().get_ibase())) {
 		warehouse->refresh_options(*igb);
 	}
+
 	Notifications::publish(NoteExpeditionCanceled(this));
 }
 

=== modified file 'src/economy/expedition_bootstrap.h'
--- src/economy/expedition_bootstrap.h	2017-02-12 09:10:57 +0000
+++ src/economy/expedition_bootstrap.h	2017-02-17 18:17:30 +0000
@@ -38,9 +38,15 @@
 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.
+ * It is the responsibility of the Owner to finally dispose a canceled ExpeditionBootstrap.
+ */
+
 class ExpeditionBootstrap {
 public:
 	ExpeditionBootstrap(PortDock* const portdock);
@@ -53,9 +59,12 @@
 	// the corresponding warehouse.
 	void cancel(Game& game);
 
-	// Returns a list of workers and wares that are ready to go to an
-	// expedition. Ownership is transferred and the object is in an undefined
-	// state after this and must be deleted.
+    /**
+     * Returns a list of workers and wares that are ready to go to an expediton.
+     *
+     * Ownership is transferred and the object is in an undefined
+     * state after this and must be deleted.
+     */
 	void get_waiting_workers_and_wares(Game&,
 	                                   const TribeDescr&,
 	                                   std::vector<Worker*>* return_workers,
@@ -73,11 +82,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.
-	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:
@@ -87,6 +103,7 @@
 	// Tests if all wares for the expedition have arrived. If so, informs the portdock.
 	void is_ready(Game& game);
 
+    /** The Expedition is bootstapped here. */
 	PortDock* const portdock_;  // not owned
 	Economy* economy_;
 

=== modified file 'src/graphic/align.h'
--- src/graphic/align.h	2017-01-25 18:55:59 +0000
+++ src/graphic/align.h	2017-02-17 18:17:30 +0000
@@ -24,6 +24,18 @@
 
 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
+ * 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,

=== modified file 'src/wui/buildingwindow.cc'
--- src/wui/buildingwindow.cc	2017-01-28 14:53:28 +0000
+++ src/wui/buildingwindow.cc	2017-02-17 18:17:30 +0000
@@ -159,9 +159,6 @@
 		// Check if this is a port building and if yes show expedition button
 		if (upcast(Widelands::Warehouse const, warehouse, &building_)) {
 			if (Widelands::PortDock* pd = warehouse->get_portdock()) {
-				expedition_canceled_subscriber_ =
-				   Notifications::subscribe<Widelands::NoteExpeditionCanceled>([this](
-				      const Widelands::NoteExpeditionCanceled&) { update_expedition_button(true); });
 				expeditionbtn_ =
 				   new UI::Button(capsbuttons, "start_or_cancel_expedition", 0, 0, 34, 34,
 				                  g_gr->images().get("images/ui_basic/but4.png"),
@@ -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& canceled) {
+                        // Check this was not just any but our Expedition
+                        if (canceled.bootstrap == pd->expedition_bootstrap()) {
+                            update_expedition_button(true);
+                        }
+                    });
 			}
 		} else if (upcast(const Widelands::ProductionSite, productionsite, &building_)) {
 			if (!is_a(Widelands::MilitarySite, productionsite)) {