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

[Tino]

Yes, inderectly: We  have rolling builds on Appveyor. This means the moment you 
commit a new revision, the current build of the branch is cancelled.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius.

___
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_1571009_work_area_radius into lp:widelands

[Klaus Halfmann]

Whats the Problem with Appveyor? 
I did not find the Problem.
Did the build take to long?
Did I cancel it?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius.

___
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_1571009_work_area_radius into lp:widelands

[bunnybot]

Continuous integration builds have changed state:

Travis build 1021. State: passed. Details: 
https://travis-ci.org/widelands/widelands/builds/123665737.
Appveyor build 853. State: failed. Details: 
https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1571009_work_area_radius-853.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius.

___
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_1571009_work_area_radius into lp:widelands

[GunChleoc]

For merging, set a commit message, then add a comment with ATbunnybot merge in 
it.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius.

___
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_1571009_work_area_radius into lp:widelands

[GunChleoc]

Review: Approve


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

___
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_1571009_work_area_radius into lp:widelands

[GunChleoc]

Review: Abstain

Tested and working :)

Please remove the TODO comment before merging.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is subscribed to branch 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius.

___
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_1571009_work_area_radius into lp:widelands

[Miroslav Remák]

> The Lua functions always return an int, that's how the interface works.
Sure, but why are you bringing this up? Has anyone said otherwise?
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius 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/fh1_multiline_textarea into lp:widelands

[GunChleoc]

The proposal to merge lp:~widelands-dev/widelands/fh1_multiline_textarea into 
lp:widelands has been updated.

Status: Needs review => Work in progress

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/fh1_multiline_textarea/+merge/292033
-- 
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/fh1_multiline_textarea 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

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

[Miroslav Remák]

Diff.

Diff comments:

> 
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc  2016-04-11 06:45:29 +
> +++ src/scripting/lua_map.cc  2016-04-17 06:41:21 +
> @@ -1871,10 +1871,16 @@
>  /* RST
>   .. attribute:: workarea_radius
>  
> - (RO) the workarea_radius of the building as an int.
> + (RO) the first workarea_radius of the building as an 
> int,
> +  nil in case bulding has no workareas
>  */
>  int LuaBuildingDescription::get_workarea_radius(lua_State * L) {
> - lua_pushinteger(L, get()->workarea_info_.begin()->first);
> +const WorkareaInfo& workareaInfo = get()->workarea_info_;

You missed this space indent here.

> + if (!workareaInfo.empty()) {
> + lua_pushinteger(L, workareaInfo.begin()->first);
> + } else {
> + lua_pushnil(L);
> + }
>   return 1;
>  }
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius 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

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

[GunChleoc]

The Lua functions always return an int, that's how the interface works.

I don't agree with your TODO comment - see diff comments.

Otherwise, code LGTM - I'm still waiting for the compiler for testing.

Diff comments:

> === modified file 'src/logic/map_objects/tribes/workarea_info.h'
> --- src/logic/map_objects/tribes/workarea_info.h  2015-11-28 22:29:26 
> +
> +++ src/logic/map_objects/tribes/workarea_info.h  2016-04-17 06:41:21 
> +
> @@ -25,9 +25,23 @@
>  #include 
>  #include 
>  
> -//  This type is used to store information about workareas. It stores radii 
> and
> -//  for each radius a set of strings. Each string contains a description of 
> an
> -//  activity (or similar) that can be performed within the radius.
> +/** The WorkareaInfo stores radii and for each radius a set of strings.
> + *
> + * A Workarea is a "circle" around a building that this building affects
> + * or is needed by this building, e.g. Areas for Mines, Fields of a Farm.
> + * Worareads are shown on the Map when clicking on a building.
> + *
> + * Each string contains a description of an  activity (or similar) i
> + * that can be performed within the radius.
> + */
> +
> +// TODO(Hasi50): In fact this complex idea of a workarea is not used.
> +// I do knot know of any building that has different sizes of workareas
> +// during its liftimer. LuaBuildingDescription::get_workarea_radiu does not 
> use it
> +// and the GUI does not show it.
> +//
> +// So we should just use a simple unit8 perhaps?

Have a look at:

FieldOverlayManager::OverlayId InteractiveBase::show_work_area

In this function, multiple overlays are generated from the map. You can see 
multiple overlays on the same building if you build a Fortress. So, a map is 
needed.

> +
>  using WorkareaInfo = std::map;
>  
>  #endif  // end of include guard: WL_LOGIC_MAP_OBJECTS_TRIBES_WORKAREA_INFO_H


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius 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

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

[Klaus Halfmann]

hope I fixed the codecheck issues now.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius 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