widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

Couple of code comments, but mostly nits. Otherwise lgtm. Not tested.

The launchpad diff has a lot of weird looking indents - I think that is because we mess up indentation and alignment while using tabs. I wish we would use spaces - all diffs then look always the same and there is no differentiation between indentation and alignment. 

Diff comments:

> 
> === modified file 'src/editor/ui_menus/editor_main_menu_save_map.cc'
> --- src/editor/ui_menus/editor_main_menu_save_map.cc	2015-12-17 09:36:59 +0000
> +++ src/editor/ui_menus/editor_main_menu_save_map.cc	2016-01-29 17:34:45 +0000
> @@ -86,9 +86,7 @@
>  	                           editbox_label_.get_x() + editbox_label_.get_w() + padding_,
>  	                           editbox_label_.get_y(),
>  	                           tablew_ - editbox_label_.get_w() - padding_ + 1,
> -	                           buth_,
> -	                           g_gr->images().get("pics/but1.png"),
> -	                           UI::Align::kLeft);
> +										g_gr->images().get("pics/but1.png"));

indent looks weird? would not have happened with spaces only indent :)

>  
>  	editbox_->set_text(parent.egbase().map().get_name());
>  	editbox_->changed.connect(boost::bind(&MainMenuSaveMap::edit_box_changed, this));
> 
> === modified file 'src/graphic/text_layout.cc'
> --- src/graphic/text_layout.cc	2016-01-28 21:27:04 +0000
> +++ src/graphic/text_layout.cc	2016-01-29 17:34:45 +0000
> @@ -27,10 +27,20 @@
>  
>  #include "base/utf8.h"
>  #include "graphic/font_handler1.h"
> +#include "graphic/image.h"
>  #include "graphic/text/bidi.h"
>  #include "graphic/text/font_set.h"
>  #include "graphic/text_constants.h"
>  
> +uint32_t text_width(const std::string& text, int ptsize) {
> +	return UI::g_fh1->render(as_editorfont(text, ptsize - UI::g_fh1->fontset().size_offset()))->width();
> +}
> +
> +uint32_t text_height(const std::string& text, int ptsize) {
> +	return UI::g_fh1->render(as_editorfont(text.empty() ? "." : text,
> +														ptsize - UI::g_fh1->fontset().size_offset()))->height();

indent again?

> +}
> +
>  std::string richtext_escape(const std::string& given_text) {
>  	std::string text = given_text;
>  	boost::replace_all(text, ">", ">");
> @@ -59,6 +69,16 @@
>  	return as_aligned(txt, UI::Align::kLeft, size, clr);
>  }
>  
> +std::string as_editorfont(const std::string& text, int ptsize, const RGBColor& clr) {
> +	// UI Text is always bold due to historic reasons
> +	static boost::format
> +			f("<rt keep_spaces=1><p><font face=serif size=%i bold=1 shadow=1 color=%s>%s</font></p></rt>");

indent? Also is static here really correct? It seems like the % operator mutates 'f', so will it cycle through the placeholders after doing it once?

> +	f % ptsize;
> +	f % clr.hex_value();
> +	f % richtext_escape(text);
> +	return f.str();
> +}
> +
>  std::string as_aligned(const std::string & txt, UI::Align align, int ptsize, const RGBColor& clr) {
>  	std::string alignment = "left";
>  	if ((align & UI::Align::kHorizontal) == UI::Align::kRight) {
> 
> === modified file 'src/graphic/text_layout.h'
> --- src/graphic/text_layout.h	2015-12-13 21:04:01 +0000
> +++ src/graphic/text_layout.h	2016-01-29 17:34:45 +0000
> @@ -46,6 +59,10 @@
>   */
>  std::string as_uifont
>  	(const std::string&, int ptsize = UI_FONT_SIZE_SMALL, const RGBColor& clr = UI_FONT_CLR_FG);
> +
> +std::string as_editorfont(const std::string& text, int ptsize = UI_FONT_SIZE_SMALL,
> +								  const RGBColor& clr = UI_FONT_CLR_FG);

indent?

> +
>  std::string as_aligned(const std::string & txt, UI::Align align, int ptsize = UI_FONT_SIZE_SMALL,
>  							  const RGBColor& clr = UI_FONT_CLR_FG);
>  
> 
> === modified file 'src/graphic/wordwrap.cc'
> --- src/graphic/wordwrap.cc	2016-01-28 21:27:04 +0000
> +++ src/graphic/wordwrap.cc	2016-01-29 17:34:45 +0000
> @@ -33,32 +33,6 @@
>  #include "graphic/rendertarget.h"
>  #include "graphic/text/bidi.h"
>  
> -namespace {
> -std::string as_editorfont(const std::string& text,
> -								  int ptsize = UI_FONT_SIZE_SMALL,
> -								  const RGBColor& clr = UI_FONT_CLR_FG) {
> -	// UI Text is always bold due to historic reasons

I see, we used that before. But if you understand it, I would love an explanation :)

> -	static boost::format
> -			f("<rt keep_spaces=1><p><font face=serif size=%i bold=1 shadow=1 color=%s>%s</font></p></rt>");
> -	f % ptsize;
> -	f % clr.hex_value();
> -	f % richtext_escape(text);
> -	return f.str();
> -}
> -
> -// This is inefficient; only call when we need the exact width.
> -uint32_t text_width(const std::string& text, int ptsize) {
> -	return UI::g_fh1->render(as_editorfont(text, ptsize - UI::g_fh1->fontset().size_offset()))->width();
> -}
> -
> -// This is inefficient; only call when we need the exact height.
> -uint32_t text_height(const std::string& text, int ptsize) {
> -	return UI::g_fh1->render(as_editorfont(text.empty() ? "." : text,
> -														ptsize - UI::g_fh1->fontset().size_offset()))->height();
> -}
> -
> -} // namespace
> -
>  namespace UI {
>  
>  /**
> 
> === modified file 'src/ui_basic/multilinetextarea.cc'
> --- src/ui_basic/multilinetextarea.cc	2016-01-28 21:27:04 +0000
> +++ src/ui_basic/multilinetextarea.cc	2016-01-29 17:34:45 +0000
> @@ -87,8 +87,9 @@
>  	for (int i = 0; i < 2; ++i) {
>  		if (m_text.compare(0, 3, "<rt")) {
>  			isrichtext = false;
> -			boost::replace_all(m_text, "\n", "<br>");
> -			const Image* text_im = UI::g_fh1->render(as_uifont(m_text, m_style.font->size(), m_style.fg),
> +			std::string text_to_render = richtext_escape(m_text);

you do this a couple of times. Pull out a method?

> +			boost::replace_all(text_to_render, "\n", "<br>");
> +			const Image* text_im = UI::g_fh1->render(as_uifont(text_to_render, m_style.font->size(), m_style.fg),
>  																  get_eff_w() - 2 * RICHTEXT_MARGIN);
>  			height = text_im->height();
>  		} else {


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