widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc'
> --- src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc	2014-09-10 14:08:25 +0000
> +++ src/editor/ui_menus/editor_tool_set_terrain_options_menu.cc	2014-09-24 08:43:32 +0000
> @@ -21,6 +21,7 @@
>  
>  #include <memory>
>  
> +#include <boost/format.hpp>
>  #include <SDL_keysym.h>
>  
>  #include "base/i18n.h"
> @@ -66,6 +67,9 @@
>  
>  	constexpr int kSmallPicHeight = 20;
>  	constexpr int kSmallPicWidth = 20;
> +
> +	std::string tooltip;
> +
>  	for (size_t checkfor = 0; check[checkfor] >= 0; ++checkfor) {
>  		const TerrainDescription::Type ter_is = terrain_descr.get_is();
>  		if (ter_is != check[checkfor])
> @@ -80,34 +84,51 @@
>  		if (ter_is == TerrainDescription::GREEN) {
>  			surf->blit(pt, green->surface(), Rect(0, 0, green->width(), green->height()));
>  			pt.x += kSmallPicWidth + 1;
> +			/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +			tooltip = _("Green");
>  		} else {
>  			if (ter_is & TerrainDescription::WATER) {
>  				surf->blit(pt, water->surface(), Rect(0, 0, water->width(), water->height()));
>  				pt.x += kSmallPicWidth + 1;
> +				/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +				tooltip = _("Water");
>  			}
> -			if (ter_is & TerrainDescription::MOUNTAIN) {
> +			else if (ter_is & TerrainDescription::MOUNTAIN) {

why else? there are terrains that are green and mountains, right? not sure though, haven't checked this.

>  				surf->blit(pt, mountain->surface(), Rect(0, 0, mountain->width(), mountain->height()));
>  				pt.x += kSmallPicWidth + 1;
> +				/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +				tooltip = _("Mountains");
>  			}
>  			if (ter_is & TerrainDescription::ACID) {
>  				surf->blit(pt, dead->surface(), Rect(0, 0, dead->width(), dead->height()));
>  				pt.x += kSmallPicWidth + 1;
> +				/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +				tooltip = ((boost::format("%s %s")) % tooltip % _("Dead")).str();
>  			}
>  			if (ter_is & TerrainDescription::UNPASSABLE) {
>  				surf->blit(
>  				   pt, unpassable->surface(), Rect(0, 0, unpassable->width(), unpassable->height()));
>  				pt.x += kSmallPicWidth + 1;
> +				/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +				tooltip = ((boost::format("%s %s")) % tooltip % _("Unpassable")).str();
>  			}
> -			if (ter_is & TerrainDescription::DRY)
> +			if (ter_is & TerrainDescription::DRY) {
>  				surf->blit(pt, dry->surface(), Rect(0, 0, dry->width(), dry->height()));
> +				/** TRANSLATORS: This is a terrain type tooltip in the editor */
> +				 tooltip = ((boost::format("%s %s")) % tooltip % _("Treeless")).str();

instead, use a vector<string>, collect all the tooltips and use boost to concatenate the collected strings. Your code here is highly inefficient (though it does not really matter as it is run only once when the window opens) and it reads more confusing than needed.

> +			}
>  		}
> +
> +		/** TRANSLATORS: %1% = terrain name, %2% = list of terrain types  */
> +		tooltip = ((boost::format("%1%: %2%")) % terrain_descr.descname() % tooltip).str();
> +
>  		// Make sure we delete this later on.
>  		offscreen_images->emplace_back(new_in_memory_image("dummy_hash", surf));
>  		break;
>  	}
>  
>  	std::unique_ptr<const Image>& image = offscreen_images->back();
> -	UI::Checkbox* cb = new UI::Checkbox(parent, Point(0, 0), image.get());
> +	UI::Checkbox* cb = new UI::Checkbox(parent, Point(0, 0), image.get(), tooltip);
>  	cb->set_desired_size(image->width() + 1, image->height() + 1);
>  	return cb;
>  }
> 


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