widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Approve

Only looked at the code, do you also want some testing?
Code looks good so far, two comments are in the diff.

Diff comments:

> 
> === modified file 'src/wui/shipwindow.cc'
> --- src/wui/shipwindow.cc	2018-04-07 16:59:00 +0000
> +++ src/wui/shipwindow.cc	2018-04-17 03:10:47 +0000
> @@ -163,9 +163,13 @@
>  		if (note.serial == ship_.serial()) {
>  			switch (note.action) {
>  			// Unable to cancel the expedition
> -			case Widelands::NoteShipWindow::Action::kNoPortLeft:
> -				if (upcast(InteractiveGameBase, igamebase, ship_.get_owner()->egbase().get_ibase())) {
> -					if (igamebase->can_act(ship_.get_owner()->player_number())) {
> +			case Widelands::NoteShipWindow::Action::kNoPortLeft: {
> +				Widelands::Ship* note_ship = ship_.get(igbase_.egbase());
> +				if (note_ship == nullptr) {
> +					return;

Can it happen that the pointer is invalid? Spontaneous I would say it shouldn't but I don't know enough about the timings between ships and their windows. If it can't/shouldn't, maybe use NEVER_HERE instead of the returns? If that is possible and done, maybe also create a small helper function for all the casts.

> +				}
> +				if (upcast(InteractiveGameBase, igamebase, note_ship->get_owner()->egbase().get_ibase())) {
> +					if (igamebase->can_act(note_ship->get_owner()->player_number())) {
>  						UI::WLMessageBox messagebox(
>  						   get_parent(),
>  						   /** TRANSLATORS: Window label when an expedition can't be canceled */
> @@ -208,17 +216,21 @@
>  
>  void ShipWindow::think() {
>  	UI::Window::think();
> -	InteractiveBase* ib = ship_.get_owner()->egbase().get_ibase();
> +	Widelands::Ship* ship = ship_.get(igbase_.egbase());
> +	if (ship == nullptr) {
> +		return;
> +	}
> +	InteractiveBase* ib = ship->get_owner()->egbase().get_ibase();

I don't know what all the *bases are doing, but this looks a bit strange. Can't we simply use igbase_.egbase().get_ibase() instead of using the ship?
(Okay, we need it further down anyway. But still...)

>  	bool can_act = false;
>  	if (upcast(InteractiveGameBase, igb, ib))
> -		can_act = igb->can_act(ship_.get_owner()->player_number());
> +		can_act = igb->can_act(ship->owner().player_number());
>  
> -	btn_destination_->set_enabled(ship_.get_destination(igbase_.egbase()));
> +	btn_destination_->set_enabled(ship->get_destination(igbase_.egbase()));
>  	btn_sink_->set_enabled(can_act);
>  
>  	display_->clear();
> -	for (uint32_t idx = 0; idx < ship_.get_nritems(); ++idx) {
> -		Widelands::ShippingItem item = ship_.get_item(idx);
> +	for (uint32_t idx = 0; idx < ship->get_nritems(); ++idx) {
> +		Widelands::ShippingItem item = ship->get_item(idx);
>  		Widelands::WareInstance* ware;
>  		Widelands::Worker* worker;
>  		item.get(igbase_.egbase(), &ware, &worker);


-- 
https://code.launchpad.net/~widelands-dev/widelands/ships_optr/+merge/343292
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/ships_optr.