Merge lp:~widelands-dev/widelands/bug-1366725 into lp:widelands

[GunChleoc]

Bugfix: building help now displays multiple tools if needed, e.g. for the Imperial Vineyard

[SirVer (sirver)]

review: Needs Fixing

[GunChleoc (gunchleoc)]

Diff comments are addressed

review: Needs Resubmitting

[SirVer (sirver)]

I think the code contains a bug. See below.

review: Needs Fixing

[GunChleoc (gunchleoc)]

Good catch! Bug fixed.

[TiborB (tiborb95)]

hey, what is it waiting for?
I compiled it and it looks fine to me.

[GunChleoc (gunchleoc)]

It is probably waiting on SirVer to find the time to review it ;)

[TiborB (tiborb95)]

SirVer is too busy - generally not just now. I communicated with him and he suggested that also other developers should review/approve merge requests. (I had my AI branch in mind specifically)

So I can approve this one as this looks a simple one, and SirVer has looked at it before, and I tested it. Would you mind?

[GunChleoc (gunchleoc)]

> SirVer is too busy - generally not just now. I communicated with him and he suggested that also other developers should review/approve merge requests. (I had my AI branch in mind specifically)
>
> So I can approve this one as this looks a simple one, and SirVer has looked at it before, and I tested it. Would you mind?

OK, I'll merge this then. Thanks for the review.

I'd rather not review the AI branch for a merge, because that's over my
head unfortunately. I am quite OK with general architecture and
consistency of stuff, but the ins and outs of heuristics and the finer
points of C++ are beyond me.

[TiborB (tiborb95)]

And dont you need "approve" review from other developer? (I am setting it to approve now)

AI branch is indeed too complex, and from my perspective resolving text conflicts is also quite complicated for me (when there are significant changes in trunk).

I just hope that if I help with other merge requests, SirVer will find a time for my branch sooner. :)

review: Approve

[SirVer (sirver)]

+1 for cross team reviews! We really have to find a way forward that does not make me the bottleneck all the time. I am stressed out a bit over Widelands responsibilities anyways :).

Also, code reviews are very educational for the reviewer and the reviewee. And they are great for code quality. I will keep looking over code reviews and try to keep up with doing them, I will also skim all commits (also those that I did not review), but not having to do the in-depth review always will save quite some time.

review: Approve

=== modified file 'tribes/scripting/format_help.lua'
--- tribes/scripting/format_help.lua	2014-07-27 12:51:29 +0000
+++ tribes/scripting/format_help.lua	2014-09-18 18:20:44 +0000
@@ -667,15 +667,15 @@
 		local worker_description = building_description.working_positions[1]
 		local becomes_description = nil
 		local number_of_workers = 0
-		local toolname = nil
+		local toolnames = {}
 
 		for i, worker_description in ipairs(building_description.working_positions) do
 
-			-- get the tool for the workers. This assumes that each building only uses 1 tool
+			-- Get the tools for the workers.
 			if(worker_description.buildable) then
 				for j, buildcost in ipairs(worker_description.buildcost) do
 					if( not (buildcost == "carrier" or buildcost == "none" or buildcost == nil)) then
-						toolname = buildcost
+						toolnames[#toolnames + 1] = buildcost
 					end
 				end
 			end
@@ -692,7 +692,9 @@
 			end
 		end
 
-		if(toolname) then result = result .. building_help_tool_string(tribename, toolname, number_of_workers) end
+		if(#toolnames > 0) then
+			result = result .. building_help_tool_string(tribename, toolnames, number_of_workers)
+		end
 
 		if(becomes_description) then
 
@@ -723,17 +725,21 @@
 -- RST
 -- .. function building_help_tool_string(tribename, toolname)
 --
---    Displays a tool with an intro text and image
+--    Displays tools with an intro text and images
 --
 --    :arg tribename: e.g. "barbarians".
---    :arg toolname: e.g. "felling_axe".
---    :arg no_of_workers: the number of workers using the tool; for plural formatting.
---    :returns: text_line for the tool
+--    :arg toolnames: e.g. {"shovel", "basket"}.
+--    :arg no_of_workers: the number of workers using the tools; for plural formatting.
+--    :returns: text_line for the tools
 --
-function building_help_tool_string(tribename, toolname, no_of_workers)
-	local ware_description = wl.Game():get_ware_description(tribename, toolname)
-	return text_line((ngettext("Worker uses:","Workers use:", no_of_workers)),
-		ware_description.descname, ware_description.icon_name)
+function building_help_tool_string(tribename, toolnames, no_of_workers)
+	local result = rt(h3(ngettext("Worker uses:","Workers use:", no_of_workers)))
+	local game  = wl.Game();
+	for i, toolname in ipairs(toolnames) do
+		local ware_description = game:get_ware_description(tribename, toolname)
+		result = result .. image_line(ware_description.icon_name, 1, p(ware_description.descname))
+	end
+	return result
 end
 
 -- RST