Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands
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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands
Continuous integration builds have changed state: Travis build 1020. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/123553136. Appveyor build 852. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1571009_work_area_radius-852. -- 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
Diff reply.
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-16 12:42:55 +
> @@ -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,
> + 0 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_;
Well, the entire file uses tabs and you've just introduced an inconsistency by
adding some space indents...
> +if (!workareaInfo.empty()) {
> +lua_pushinteger(L, workareaInfo.begin()->first);
> +} else {
> +lua_pushinteger(L, 0);
> +}
> 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
Thanks for the hints
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-16 12:42:55 +
> @@ -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,
> + 0 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_;
Mhh, Sirver prefers 3 spaces, and there is no consistent handling of this, yet.
> +if (!workareaInfo.empty()) {
> +lua_pushinteger(L, workareaInfo.begin()->first);
> +} else {
> +lua_pushinteger(L, 0);
Thhaks for the tip, im not firm with that lua-stuff and tried not pushing
anything (which failed).
I will try that now
> +}
> 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
See diff comments.
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-16 12:42:55 +
> @@ -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,
> + 0 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_;
Please use tabs for indentation.
> +if (!workareaInfo.empty()) {
> +lua_pushinteger(L, workareaInfo.begin()->first);
> +} else {
> +lua_pushinteger(L, 0);
I think it would be safer to use lua_pushnil here to indicate that there is NO
work area defined. Returning 0 could be understood as a work area with 0
radius, even if it may not make much sense. This behavior is also consistent
with many of our other functions. You don't have to modify any Lua code because
it already checks for a nil value.
> +}
> 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
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands
Klaus Halfmann has proposed merging
lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1571009 in widelands: "Work area radius: 45xxx in bzr7962[trunk]"
https://bugs.launchpad.net/widelands/+bug/1571009
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
This fixes 1571009 and adds some more Docs for the WorkareaInfo.
Im not usre about the Lua-Mapping, perhaps returning some kind of null might be
better?
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands.
=== 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-16 12:42:55 +
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2005, 2008 by the Widelands Development Team
+ * Copyright (C) 2005-2016 by the Widelands Development Team
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -25,9 +25,22 @@
#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?
+ */
using WorkareaInfo = std::map>;
#endif // end of include guard: WL_LOGIC_MAP_OBJECTS_TRIBES_WORKAREA_INFO_H
=== 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-16 12:42:55 +
@@ -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,
+ 0 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_;
+if (!workareaInfo.empty()) {
+lua_pushinteger(L, workareaInfo.begin()->first);
+} else {
+lua_pushinteger(L, 0);
+}
return 1;
}
___
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/revise-map-descr into lp:widelands
Continuous integration builds have changed state: Travis build 1018. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/123520813. Appveyor build 850. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_revise_map_descr-850. -- https://code.launchpad.net/~widelands-dev/widelands/revise-map-descr/+merge/292031 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/revise-map-descr 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/revise-map-descr into lp:widelands
Got a crash when trying to access a folder in 'editor load map' and 'New game': terminate called after throwing an instance of 'RT::SyntaxErrorImpl' what(): Syntax error at 1:65: expected an allowed tag, got 'parent'. String continues with: '' > 1. Fixed Yeah :-) > > 2. There are obsolete information: "Map size" and "2/3/4/... Player map". > > These information is always in the table, so why should we have it in the > > description again? > > We also use this in Editor-> Save Map. Do you think the information is useless > there as well? Yes, i think so. A mapmaker normally knows how much players he has placed on the map. The size of the map is mostly interesting when creating a new map because it has influence to max. number of players or shaping the terrain (how much unbuildable areas). And if a map is saved once, the size is shown in table. The size of the map is also shown in the Map options menu and as explained earlier i would prefer to make the Map Options be a part of the whole Map save process (i create a bug report for this idea). So i would say: > b. remove the info > 3. How is it now? Hm, maybe better but now there is a mix of coloring headers: Mapname, yellow; String "Author", white; Name of author yellow. If we think of Headers as static strings (Name of map, "Author", "Tags"...) and other texts as variable strings (Name of Author, List of tags, Text of description,...) i think all static texts should have the same color (f.e. yellow) and all variable texts should have the same color (f.e. white). The Name of the menu is also a static text and therefor should have the same color as other static texts. I believe making a distinction like this (static/variable texts) would bring some visual order to the menu. Having two colors is really good :-) > > Just a side note: > > Good idea, but not for Build 19 ;) Yes, of course ;) -- https://code.launchpad.net/~widelands-dev/widelands/revise-map-descr/+merge/292031 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/revise-map-descr 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/revise-map-descr into lp:widelands
1. Fixed > 2. There are obsolete information: "Map size" and "2/3/4/... Player map". > These information is always in the table, so why should we have it in the > description again? We also use this in Editor-> Save Map. Do you think the information is useless there as well? There are 3 ways we could go: a. leave the info in b. remove the info c. distinguish between loading and saving contexts 3. How is it now? > Just a side note: Good idea, but not for Build 19 ;) -- https://code.launchpad.net/~widelands-dev/widelands/revise-map-descr/+merge/292031 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/revise-map-descr 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
