widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.