[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-22 Thread noreply 
The proposal to merge 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building into 
lp:widelands has been updated.

Status: Needs review => Merged

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
-- 
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-22 Thread GunChleoc 
@bunnybot merge
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-22 Thread SirVer 
Review: Approve

lgtm. minor nits inlined.

Diff comments:

> 
> === modified file 'src/logic/map_objects/immovable.cc'
> --- src/logic/map_objects/immovable.cc2016-11-03 07:37:58 +
> +++ src/logic/map_objects/immovable.cc2016-11-17 08:29:59 +
> @@ -340,8 +344,9 @@
>  ==
>  */
>  
> -Immovable::Immovable(const ImmovableDescr& imm_descr)
> +Immovable::Immovable(const ImmovableDescr& imm_descr, const 
> Widelands::Building* former_building)
> : BaseImmovable(imm_descr),
> + former_building_(former_building ? _building->descr() : nullptr),

rename: former_building_descr_

>   anim_(0),
>   animstart_(0),
>   program_(nullptr),
> @@ -349,6 +354,9 @@
>   anim_construction_total_(0),
>   anim_construction_done_(0),
>   program_step_(0) {
> + if (former_building) {

nit: != nullptr. Implicit casts to bool make me nervous and I find it enforces 
the type of the variable. your call.

> + set_owner(former_building->get_owner());
> + }
>  }
>  
>  Immovable::~Immovable() {


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-21 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 1619. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/176633949.
Appveyor build 1457. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_863185_census_on_destroyed_building-1457.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-21 Thread bunnybot 
Bunnybot encountered an error while working on this merge proposal:

('The read operation timed out',)
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-19 Thread bunnybot 
Continuous integration builds have changed state:

Travis build 1619. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/176633949.
Appveyor build 1457. State: success. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_863185_census_on_destroyed_building-1457.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-19 Thread bunnybot 
Bunnybot encountered an error while working on this merge proposal:

HTTP Error 500: Internal Server Error
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-17 Thread GunChleoc 
Review: Resubmit

Next try.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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/bug-863185-census-on-destroyed-building into lp:widelands

2016-11-13 Thread SirVer 
Review: Needs Fixing



Diff comments:

> === modified file 'src/logic/map_objects/immovable.cc'
> --- src/logic/map_objects/immovable.cc2016-11-03 07:20:57 +
> +++ src/logic/map_objects/immovable.cc2016-11-03 07:36:43 +
> @@ -339,6 +339,7 @@
>  
>  Immovable::Immovable(const ImmovableDescr& imm_descr)
> : BaseImmovable(imm_descr),
> + antecedent_(nullptr),

I am not familiar with this word. Can you find a simpler name?

>   anim_(0),
>   animstart_(0),
>   program_(nullptr),
> 
> === modified file 'src/logic/map_objects/immovable.h'
> --- src/logic/map_objects/immovable.h 2016-11-03 07:20:57 +
> +++ src/logic/map_objects/immovable.h 2016-11-03 07:36:43 +
> @@ -201,7 +201,11 @@
>   Immovable(const ImmovableDescr&);
>   ~Immovable();
>  
> - void set_owner(Player*);
> + /// If this immovable was created by a building, this can be set in 
> order to display information
> + /// about it. If this is a player immovable, you will need to set the 
> owner first.
> + void set_antecedent(const BuildingDescr& building);
> +
> + void set_owner(Player* player);

I think this function is overwritten in base classes. You'll have a bad time if 
you do not mark it as virtual here. Ideally you want to remove the overwritten 
functions too.

>  
>   Coords get_position() const {
>   return position_;
> 
> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc  2016-11-03 07:20:57 +
> +++ src/logic/map_objects/tribes/building.cc  2016-11-03 07:36:43 +
> @@ -441,8 +442,12 @@
>   const Coords pos = position_;
>   PlayerImmovable::destroy(egbase);
>   // We are deleted. Only use stack variables beyond this point
> - if (fire)
> - egbase.create_immovable(pos, "destroyed_building", 
> MapObjectDescr::OwnerType::kTribe);
> + if (fire) {
> + Immovable& destroyed_building =
> +egbase.create_immovable(pos, "destroyed_building", 
> MapObjectDescr::OwnerType::kTribe);

You'll probably end up with cleaner code if you get rid of 'set_owner' and 
'set_antcedent' and instead add a 'create_immovable_with_precedessor' in 
EditorGame and set up all the state in the constructor of Immovable and/or in 
the EgBase function.

> + destroyed_building.set_owner(get_owner());
> + destroyed_building.set_antecedent(descr());
> + }
>  }
>  
>  std::string Building::info_string(const InfoStringFormat& format) {


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-863185-census-on-destroyed-building/+merge/309818
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug-863185-census-on-destroyed-building.

___
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