widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Code looks good to me. Three small comments are in the diff, feel free to ignore them.

The speed problem is not so great. Is this a general problem of the new renderer? Do you have an idea why it is so much slower?
On a side note: I don't like the "speed-hack" in revision 8540, but when it helps...

As far as I am concerned, this can be merged. But the speed problem should be looked into at some point.

Diff comments:

> 
> === modified file 'data/scripting/richtext.lua'
> --- data/scripting/richtext.lua	2017-07-03 10:16:59 +0000
> +++ data/scripting/richtext.lua	2017-12-19 18:07:21 +0000
> @@ -327,21 +327,24 @@
>  end
>  
>  -- RST
> --- .. function li_image(imagepath, text)
> +-- .. function li_image(imagepath, text[, vsp])
>  --
>  --    Places a paragraph of text to the right of an image
>  
>  --    :arg imagepath: the full path to the image file
>  --    :arg text_width_percent: the percentatge of space that the text will occupy

This parameter does not seem to exists any more.

>  --    :arg text: the text to be placed next to the image
> +--    :arg vsp: Vertical spacing. Default is 6.
>  --
> ---    :returns: the text wrapped in a paragraph and placed next to the image, The outer tag is a div.
> -function li_image(imagepath, text_width_percent, text)
> -   return p("<br>") .. div("width=100%", "") ..
> -         div(p(img(imagepath))) ..
> +--    :returns: the text wrapped in a paragraph and placed next to the image, the outer tag is a div.
> +function li_image(imagepath, text, vsp)
> +   if vsp == nil then vsp = 6 end
> +   return
> +      div("width=100%",
> +         div(p(vspace(vsp) .. img(imagepath) .. space(6))) ..
>           div(p(space(6))) ..
> -         div("width="..text_width_percent.."%", p(text)) ..
> -         div("width=100%", "")
> +         div("width=*", p(vspace(vsp) .. text .. vspace(2 * vsp)))
> +      )
>  end
>  
>  -- RST
> 
> === modified file 'data/txts/AUTHORS.lua'
> --- data/txts/AUTHORS.lua	2016-03-14 11:37:49 +0000
> +++ data/txts/AUTHORS.lua	2017-12-19 18:07:21 +0000
> @@ -26,7 +26,7 @@
>  return {
>     title = _"Developers",
>     text = rt(
> -      title(_"Widelands Development Team") ..
> +      p_font("align=center", "size=28 color=2F9131", _"Widelands Development Team") ..

Why no h1() or so for this? Also below.

>        list_authors()
>     )
>  }
> 
> === modified file 'src/ui_basic/fileview_panel.cc'
> --- src/ui_basic/fileview_panel.cc	2017-08-08 17:39:40 +0000
> +++ src/ui_basic/fileview_panel.cc	2017-12-19 18:07:21 +0000
> @@ -51,8 +51,18 @@
>  	   std::unique_ptr<UI::Box>(new UI::Box(this, 0, 0, UI::Box::Vertical, 0, 0, padding_)));
>  	size_t index = boxes_.size() - 1;
>  
> -	textviews_.push_back(std::unique_ptr<UI::MultilineTextarea>(
> -	   new UI::MultilineTextarea(boxes_.at(index).get(), 0, 0, Scrollbar::kSize, 0, content)));
> +	UI::MultilineTextarea* textarea =
> +		   new UI::MultilineTextarea(boxes_.at(index).get(), 0, 0, Scrollbar::kSize, 0);
> +	try {
> +		textarea->force_new_renderer();
> +		textarea->set_text(content);
> +	} catch (const std::exception& e) {
> +		log("Fileview: falling back to OLD font renderer: %s\n", e.what());
> +		textarea->force_new_renderer(false);
> +		textarea->set_text(content);
> +	}
> +
> +	textviews_.push_back(std::unique_ptr<UI::MultilineTextarea>(std::move(textarea)));

Is there a reason you are using std::move() here instead of just passing the pointer to the constructor?

>  	add((boost::format("about_%lu") % index).str(), title, boxes_.at(index).get(), "");
>  
>  	assert(boxes_.size() == textviews_.size());


-- 
https://code.launchpad.net/~widelands-dev/widelands/fh1-editorhelp/+merge/335283
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fh1-editorhelp.