[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-27 Thread noreply 
The proposal to merge lp:~widelands-dev/widelands/ships_optr into lp:widelands 
has been updated.

Status: Needs review => Merged

For more details, see:
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.

___
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

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-26 Thread GunChleoc 
Regarding the first comment, I simply imitated the fix that SirVer programmed 
for the Building Window. After months of trying to fix that thing, using OPtr 
is what it finally fixed it. I guess we have some concurrency issues here, i.d. 
a delay with handling the notification that the ship is not there any more, or 
the window taking too long to die. So, we expect this to become nullptr 
sometimes.

For comment 2, I simply used ibase_. This was old code left over from when the 
ShipWindow was owned by Ship. Thanks for flagging it up :)

I have tested this in the ship statistics branch, and it resolved a crash 
there, so I think we're fine here on testing.

@bunnybot merge
-- 
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.

___
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

Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-24 Thread Notabilis 
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 +
> +++ src/wui/shipwindow.cc 2018-04-17 03:10:47 +
> @@ -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(), , );


-- 
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.

___
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

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-17 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3387. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/367477817.
Appveyor build 3193. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ships_optr-3193.
-- 
https://code.launchpad.net/~widelands-dev/widelands/ships_optr/+merge/343292
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/ships_optr into lp:widelands.

___
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

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-16 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 3385. State: failed. Details: 
https://travis-ci.org/widelands/widelands/builds/367063651.
Appveyor build 3191. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ships_optr-3191.
-- 
https://code.launchpad.net/~widelands-dev/widelands/ships_optr/+merge/343292
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/ships_optr into lp:widelands.

___
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

[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ships_optr into lp:widelands

2018-04-16 Thread GunChleoc 
GunChleoc has proposed merging lp:~widelands-dev/widelands/ships_optr into 
lp:widelands.

Commit message:
The ShipWindow now tracks the existence of ships via OPtr, analogous to the 
BuildingWindow code.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/ships_optr/+merge/343292

This is a prerequisite for the ship statistics window to fix some memory issues 
- I decide to split off this bit to make the diffs smaller.
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/ships_optr into lp:widelands.
=== modified file 'src/wui/interactive_gamebase.cc'
--- src/wui/interactive_gamebase.cc	2018-04-07 16:59:00 +
+++ src/wui/interactive_gamebase.cc	2018-04-16 08:10:15 +
@@ -236,7 +236,7 @@
 UI::UniqueWindow::Registry& registry =
    unique_windows().get_registry((boost::format("ship_%d") % ship->serial()).str());
 registry.open_window = [this, , ship] {
-	new ShipWindow(*this, registry, *ship);
+	new ShipWindow(*this, registry, ship);
 };
 registry.create();
 return true;

=== modified file 'src/wui/shipwindow.cc'
--- src/wui/shipwindow.cc	2018-04-07 16:59:00 +
+++ src/wui/shipwindow.cc	2018-04-16 08:10:15 +
@@ -53,18 +53,18 @@
 
 using namespace Widelands;
 
-ShipWindow::ShipWindow(InteractiveGameBase& igb, UniqueWindow::Registry& reg, Ship& ship)
-   : UniqueWindow(, "shipwindow", , 0, 0, ship.get_shipname()),
+ShipWindow::ShipWindow(InteractiveGameBase& igb, UniqueWindow::Registry& reg, Ship* ship)
+   : UniqueWindow(, "shipwindow", , 0, 0, ship->get_shipname()),
  igbase_(igb),
  ship_(ship),
  vbox_(this, 0, 0, UI::Box::Vertical),
  navigation_box_(_, 0, 0, UI::Box::Vertical),
  navigation_box_height_(0) {
 	vbox_.set_inner_spacing(kPadding);
-	assert(ship_.get_owner());
+	assert(ship->get_owner());
 
-	display_ = new ItemWaresDisplay(_, *ship_.get_owner());
-	display_->set_capacity(ship_.descr().get_capacity());
+	display_ = new ItemWaresDisplay(_, ship->owner());
+	display_->set_capacity(ship->descr().get_capacity());
 	vbox_.add(display_, UI::Box::Resizing::kAlign, UI::Align::kCenter);
 
 	// Expedition buttons
@@ -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;
+}
+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 */
@@ -175,7 +179,7 @@
 		messagebox.run();
 	}
 }
-break;
+			} break;
 			// The ship is no more
 			case Widelands::NoteShipWindow::Action::kClose:
 // Stop this from thinking to avoid segfaults
@@ -195,10 +199,14 @@
 }
 
 void ShipWindow::set_button_visibility() {
-	if (navigation_box_.is_visible() != ship_.state_is_expedition()) {
-		navigation_box_.set_visible(ship_.state_is_expedition());
+	Widelands::Ship* ship = ship_.get(igbase_.egbase());
+	if (ship == nullptr) {
+		return;
+	}
+	if (navigation_box_.is_visible() != ship->state_is_expedition()) {
+		navigation_box_.set_visible(ship->state_is_expedition());
 		navigation_box_.set_desired_size(
-		   navigation_box_.get_w(), ship_.state_is_expedition() ? navigation_box_height_ : 0);
+		   navigation_box_.get_w(), ship->state_is_expedition() ? navigation_box_height_ : 0);
 		layout();
 	}
 	if (btn_cancel_expedition_->is_visible() != btn_cancel_expedition_->enabled()) {
@@ -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();
 	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 ite