New font renderer doesn't always consume newline nodes

[GunChleoc]

When the width parameter is used, the new font renderer doesn't always consume newline nodes. This can happen especially if there are 2 divs in a row, and the first one is wider than 50%.

[Notabilis (notabilis27)]

Can you please provide a sample hint/line/file/branch where this happens? Optimally with a description what does and what should happen. I am not sure whether its possible to (easily) reproduce the problem only with your description.

[GunChleoc (gunchleoc)]

I need to finish the fh1-editorhelp branch first in order to provide more information. Basically, I needed to create the bug to reference it in the code, and I need the code in trunk before I can build a good bug...

[GunChleoc (gunchleoc)]

I have attached a branch to this bug that will show the error.

To reproduce, open the Tribal Encyclopedia and look at the Wares help.

The result will look like on the left, but it should look like on the right.

The error is triggered by commit https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1738760-fh1-newlines/revision/8589 when the width of the first div is increased to over 50%.

[Notabilis (notabilis27)]

Thanks for the branch. I am assuming that you mixed up left and right when describing the screenshot? That is, the correct case is with one line for both text and icon?

I only started looking into it, but I noticed that in the correct case the relevant C++ code seems to be called twice for each entry. Is this correct? In the broken case it is only called once.

[GunChleoc (gunchleoc)]

The correct version is on the left indeed.

I have no idea whether your assumption is the right one. I hope it is! I didn't write that bit of code and I have never wrapped my head around it.

[Notabilis (notabilis27)]

Oh, okay. I thought you did. Well, doesn't matters.

Some digging turned out that the multiple calls are fine; I used a "bad" ware for testing. Multiple calls happen when it turns out (while arranging the text) that no scrollbar is required. In that case more horizontal space is available and the text is arranged again. In the broken case a new line is used for the image so a scrollbar is needed; in the correct case it fits without scrollbar.

My current error-candidate is in graphic/text/rt_render.cc line 1440. When the remaining space in the line is less than the width of the div, the remaining space is reset to the full line width. With a width of >50 percent this clause is triggered. This results in the full space for the following div-fill element. Since the sum of the widths of the div's is bigger than the line, the line is broken and rendered in two lines.
I am actually not sure what this clause should do at all. It is doing useful stuff (removing it makes it worse) but for me the logic behind it seems to be wrong.

By doing some changes (currently very ugly code) it seems to work now. Every text+image is in one line, except when the percentage of the first div is so big that there isn't enough space for the second div with the fill (i.e., the contained images would be drawn on the window border). In that case, a new line is filled by the second div.

While this should solve the problem for div->50% + div-fill, I am not sure if this fix is enough. In the initial description you said "especially if there are 2 divs": Are there other cases where the effect also happens? They wouldn't be fixed by my current changes.

Disclaimer: I am not really understanding all that's going on when arranging the text.

[GunChleoc (gunchleoc)]

The way the thing always behaved is that when no width is specified, there is a minimum width that is used for text, so the text won't autoexpand like it used to do with the old renderer. I did a change with width=*" that will work for the last div to simply fill the remaining space - that was the idea, anyway. I also introduced the width=x%, because when we write Lua code, we won't always know how wide the UI element will be. We still need to have the fallback to switch to a new line when the div can't be fitted though. This fallback shouldn't be triggered in this case, because the images could be rearranged into a column to fit the div - their div should be autoshrunk.

Took me lots of hacking and fiddling, because I don't fully understand the code either. So, I don't expect your hack to be any worse than my hack!

The bug seems to go away when I do this, so width=* seems to be the culprit:

=== modified file 'data/tribes/scripting/help/format_help.lua'
--- data/tribes/scripting/help/format_help.lua 2018-02-14 08:30:04 +0000
+++ data/tribes/scripting/help/format_help.lua 2018-02-19 08:51:48 +0000
@@ -203,7 +203,7 @@
       consumed_items_string =
          div("width=100%",
             div("width=70%", p(vspace(6) .. text .. space(6))) ..
- div("width=*", p("align=right", vspace(6) .. images .. vspace(12)))
+ div("width=30%", p("align=right", vspace(6) .. images .. vspace(12)))
          )
          .. consumed_items_string
    end

[Notabilis (notabilis27)]

I think I don't really understand what should be happening, sorry. I can try to code something, but I am not yet sure what is required.

1) The div can have some width given in an attribute. Okay. Is this a minimum width? I guess it has to be, since, e.g., when defining a width of only 2 pixels but an text/image is contained in the div, the div should probably be wide enough to contain the image. And with multiple images... With your comment I guess that the widest image/element should define the width of the div and the other images should be rendered below it in multiple lines? This would explain the "widest subnode" code in DivTagHandler::enter().

2) What should happen if there are only, e.g., 2 pixels left in a line and a fill-div is encountered containing an image? This happens when "width=99%" is used in the first inner div. Leaving the div in the same line would render the image on the window border. Should the div use minimum width but be moved to the next line (and then be left aligned)? Or should it fill the complete next line?

3) What if I define a div with width=25% but the parent div already has width=50% ? Is the inner width relative to the width of the parent element(s)? Also affects the width=* variant: How wide should it fill?

4) I tried to re-enable the disabled NEVER_HERE() regarding newline nodes. It still fails with my code, probably since vspace* also creates newline nodes (there are also other newline nodes but I don't know where they are from). No newline is optically rendered, though. Do you know whether the NEVER_HERE() call is valid?
* What is this even doing in the problematic line? Removing vspace and space does not change the result. Probably because the included image is higher than 12 pixel? Before the text it looks to me like a try to enforce a linebreak, but p is already doing so.

5) Kind of related to the previous question: Is the title/description of this bug correct? Is the problem that there are newline nodes or is the problem that width=* does not work as indented? If the problem indeed are the newline nodes, does that mean that something should remove them before rendering?

[GunChleoc (gunchleoc)]

1) I guess this depends on how tolerant we want to be. So I guess, if the div contains images that don't fit, make the div go multiline and expand its width only if there is an image in it that doesn't fit after going multiline. If the image doesn't fit the overall canvas, we should either have an error or some clipping.

2) Move to the next line and fill it. At 99%, I think we can safely assume that the intention is 100%

3) I think we can go either way with that one, whatever is easier to implement

4) I think the vspace() and space() can go in this case, I don't know why I added them in the first place. They do make a difference in the Editor Tree/Terrain help. The intention is to add a visual gap that's higher than just a line break, but less than 2 line breaks.

5) I now think the problem is that width=* does not behave as intended. The Newline not being consumed is a symptom of that.

[Notabilis (notabilis27)]

Thanks for the answers. I ... did things and the result might be what we want. The behavior with very big images hasn't been checked yet, but the ware help looks okay to me.

I pushed it to the attached branch for testing. Don't bother with the code, it is far from ready for review. I added the ware listings multiple times in the ware help to test different div widths.
The shorter lines are using an outer div of only 90 percent. The multiline entries near the bottom use percentages greater than 90 percent for the first inner div so there is not enough space for the fill-div, resulting in a second line with the fill-div.

If you think it looks okay and you can't find any new bugs, I will prepare the branch for code review.

[GunChleoc (gunchleoc)]

Looking great! :D

This also fixes https://bugs.launchpad.net/widelands/+bug/1738759 BTW, so I'm very happy.

[Notabilis (notabilis27)]

Glad to hear it.

The other bug isn't fixed I think. When looking at the editor tips, the last line is sill wrongly indented. Might have become better at some places, though, since some lines are broken at other positions now. I will look into it now.

[Notabilis (notabilis27)]

Leading (and trailing) spaces should be gone with the newest commit now.
If you don't have any objections I will start preparing the branch for review.

[GunChleoc (gunchleoc)]

Yes, please do :)

[GunChleoc (gunchleoc)]

Fixed in build20-rc1