Merge lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands

[Klaus Halfmann]

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?

[Miroslav Remák (miroslavr256)]

See diff comments.

[Klaus Halfmann (klaus-halfmann)]

Thanks for the hints

[Miroslav Remák (miroslavr256)]

Diff reply.

[bunnybot (widelandsofficial)]

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.

[Klaus Halfmann (klaus-halfmann)]

hope I fixed the codecheck issues now.

[GunChleoc (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.

[Miroslav Remák (miroslavr256)]

Diff.

[Miroslav Remák (miroslavr256)]

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

[GunChleoc (gunchleoc)]

Tested and working :)

Please remove the TODO comment before merging.

review: Abstain

[GunChleoc (gunchleoc)]

review: Approve

[GunChleoc (gunchleoc)]

For merging, set a commit message, then add a comment with ATbunnybot merge in it.

[bunnybot (widelandsofficial)]

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.

[Klaus Halfmann (klaus-halfmann)]

Whats the Problem with Appveyor?
I did not find the Problem.
Did the build take to long?
Did I cancel it?

[Tino (tino79)]

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.

[GunChleoc (gunchleoc)]

I still don't agree with your TODO comment. The Fortress has multiple workareas on purpose - it shows you the area that it will be able to conquer when you upgrade to a Citadel. If you have ideas for improving the code, replacing the map with a uint is not the way to go.

[Klaus Halfmann (klaus-halfmann)]

Hello Gun, I reviewd that comment in workarea_info.h again.
Its just that I do not fully understand the usage of that map, yet.
So if you know what stings are used, please add them.

As of the implementation of std:map this will be correct, I guess.
But it usually is a abd idea to depend on some implemenation that
may change. OTOH the hashcode of integer may not change ever again.

Example: From Java JDK1.3 to 1.4 the Implementation of String.hashCode()
was changed. Some code had implicit dependencies on the order of Elements
in string based maps an therefore started to fail.

Please feel free to change the comment to your liking.

review: Needs Resubmitting

[GunChleoc (gunchleoc)]

Now I understand what you're getting at, you have a valid point about the sort order. Thanks for explaining :)

@bunnybot merge

=== modified file 'src/logic/map_objects/tribes/workarea_info.h'
--- src/logic/map_objects/tribes/workarea_info.h	2015-11-28 22:29:26 +0000
+++ src/logic/map_objects/tribes/workarea_info.h	2016-04-23 16:57:13 +0000
@@ -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,26 @@
 #include <set>
 #include <string>
 
-//  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 buildingr., e.g. Areas for Mines, Fields of a Farm.
+ * Worareas are shown on the Map when clicking on or placing a building.
+ *
+ * Each string contains a description of an activity (or similar)
+ * that can be performed within the radius. Examples are buldings
+ * that can be upgraded like a Fortress, and will have a bigger
+ * workarea then.
+ *
+ * See LuaBuildingDescription::get_workarea_radius, InteractiveBase::show_work_area
+ */
+
+// TODO(Hasi50): LuaBuildingDescription::get_workarea_radius will only give us the very first
+// size found, which seems to be correct but depends on the std:map implementation
+//
+// We could just use a unit8 as base for the map?
+// We should document (as const) the expected stings.
+
 using WorkareaInfo = std::map<uint32_t, std::set<std::string>>;
 
 #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-22 06:55:37 +0000
+++ src/scripting/lua_map.cc	2016-04-23 16:57:13 +0000
@@ -672,7 +672,7 @@
 	case (MapObjectType::WARE):
 	default:
 		throw LuaError((boost::format("upcasted_map_object_to_lua: Unknown %i") %
-		                static_cast<int>(mo->descr().type())).str());
+						static_cast<int>(mo->descr().type())).str());
 	}
 }
 #undef CAST_TO_LUA
@@ -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_;
+	if (!workareaInfo.empty()) {
+		lua_pushinteger(L, workareaInfo.begin()->first);
+	} else {
+		lua_pushnil(L);
+	}
 	return 1;
 }