Merge lp://staging/~widelands-dev/widelands/fh1 into lp://staging/widelands

This is a really good start. I think the deprecated methods should vanish. I think this should be done before merging.

Of course the old <rt> renderer should be killed as well and the new one used in all places instead. We should not need to deal with rich text in code anymore, all formatting should be done in the new renderer. For this the new one needs to learn to render raw text and caret's as well.

Also, the Font:: methods that are now used should somehow be incorporated into the new rich text renderer - I am not too sure how this can be done. Maybe by rendering a simple string and returning its size.

Currently I use that method, rendering a test string to know the font height.

I'm coming to the multiline edit box, and I was thinking, should the wordwarpper be kept around? It is currently only used in the multiline textarea and the multiline edit box. For the multiline textarea, i think the wrapping done by the renderer will be sufficient. Now for editbox, in which caret must be rendered, that will be an issue. The multiline textarea is only used in one place in the map editor.

According to me, it would be nice to allow rendering of the caret further down in the new renderer and get rid of this wordwrap class. I was thinking of a caret tag, but it will interfere with surface caching. I'm just asking if you think it is a viable solution, and if you have some guess of where to implement that.

> Currently I use that method, rendering a test string to know the font height.

As mentioned, I think with the caching this approach is fine.

> I'm coming to the multiline edit box, and I was thinking, should the
> wordwarpper be kept around?
No, I think it should definitively be killed. The one in the richtext render should be strictly superior to what we have in wordwrap.

> It is currently only used in the multiline
> textarea and the multiline edit box. For the multiline textarea, i think the
> wrapping done by the renderer will be sufficient. Now for editbox, in which
> caret must be rendered, that will be an issue. The multiline textarea is only
> used in one place in the map editor.

This is the hardest thing to change imho. I am not really sure how to proceed either - I was thinking about a <caret> tag too. It might not be a problem - a human types at most 4-5 chars per second, we should be able to rerender the text this often. And the cache will just overflow with useless cached images, but eventually clear out. So I'd try this approach first.

> According to me, it would be nice to allow rendering of the caret further down
> in the new renderer and get rid of this wordwrap class. I was thinking of a
> caret tag, but it will interfere with surface caching. I'm just asking if you
> think it is a viable solution, and if you have some guess of where to
> implement that.

Okay, so I think I commented on this before reading this paragraph. As mentioned, I find this a difficult design decision and I am not sure if the tag is viable, but I have no clear path forward that is not using a tag, so I suggest trying that.

Again, thanks for working on this Charly!

I tried a caret tag renderer. It works fine except it seems there is a 1-pixel space inserted between two text nodes, even if the caret has zero-size. So I'm not quite sure it is the best option.
I still have other options to try, like removing the tag and continuing parsing the text while remembering somehow position of the caret, but it feels dirty.

Also, as I was trying to port the multiline editbox, I realized that if we want to support the direction keys, home end & co, we should definitely add some way to get information about the wrapping. I was thinking of a pointer to a wrapinginfo object that could be passed all the way down to the layouting node, which would fill it with data. We could put coords of every text node in there, including the caret placeholder, so that rendering can be done on top afterwards. Its still just an idea, if you have another one please share!

I do not care about the 1 pixel thingy. But you are right, cursor movements should not be handled by the text renderer. So the solution might really be to pass the layout boxes up again - then the <cursor> node might not be needed. I kinda like this solution, though I also feel that the separation of concerns is no longer true then - but so what?

After a few tries, I finally found a way to get the layout info in a not-so-dirty way.
So all basic widgets are ported now, with some fixes still needed: non-rendered whitespace makes the caret behaviour quite strange, and except of cropping when text doesnt fit width, a line break should be introduced.

I will then start to start cleaning everything. So now comes the question - should I use the font specified in the game option everywhere? I think it makes sense. Especially since, as for now, all widgets use the same font in all places - no special font in ui_fsmenu is used anymore.

> should I use the font specified in the game option everywhere? I think it makes sense. Especially since, as for now, all widgets use the same font in all places - no special font in ui_fsmenu is used anymore.

That would be a good thing, yes. I would However keep special font where explicitly defined (e.g. Widelands.ttf used for "Widelands")

Maybe it would be good to add a <rt text-style=sans / serif> in the long term and make sans and serif selectable? - than we could get rid of almost all explicite font definitions (but maybe Widelands.ttf) as the font renderer would set the appropriate font connected to sans resp. serif - but well I am getting off topic sorry - this should go to a bug report / wishlist item

> Maybe it would be good to add a <rt text-style=sans / serif> in the long term and make sans and serif selectable?
Yes, why not. However, as I understood from reading some bug reports, custom font support were implemented so that users using exotic locales could find a font that render text correctly. It might not be easy to switch serif/sans in those cases, except if we allow custom serif and custom sans font.

I see TTF handle bold/italic styles, so I wonder why it is handled in the code by using different fonts. This is not going to work if using the custom fonts. The bold faces processed by SDL_TTF do not look that bad imo.

If no objection, I am going to store the font filename in all case in the options, keeping the UI_FONT_NAME constant only to get the default value, and let TTF handle all the styles. The derived fonts in /fonts will be removed.

Seems that this is a matter of taste - I completely (like 100%) agree with you, however as far as I remember, SirVer does not (e.g. take a look at #1 of https://bugs.launchpad.net/widelands/+bug/1184185 )

For me it seems much better to have a simple option, and everything is working based upon that setting but working way than to to

concerning:
> However, as I understood from reading some bug reports, custom font support were implemented so that users using exotic locales could find a font that render text correctly. It might not be easy to switch serif/sans in those cases, except if we allow custom serif and custom sans font.

I see no problem here - if I write a text and decide to make it "more beautiful" by using a mixture of sans and serif, this decission is of course based upon Latin characters. So if e.g. someone reading the translated text in arabic and sets both styles - serif and sans - to the same TTF font, there is no problem with it (to be more specific: it isn't even a problem if someone is doing this in the untranslated version). Of course this sounds like "more clutter options", but we can still add it to the advanced options menu, as the font selection is at the moment + (and that's the major PLUS for me) We would be simply able to use serif and sans styles without having to worry about which ttf will actually be used. -> and after all that system will be much easier to maintain if for some reason the default font will be changed or removed at some point.

> For me it seems much better to have a simple option, and everything is working based upon that setting but working way than to to

seems I stopped there in the middle of a thought, but I guess everything below explains my further thoughts pretty much ;) - it's just that the whole system gets better maintainable when we use the italic/bold/underline functions of SDL_TTF and therefore don't have to take care about the fonts - e.g. one problem is that none of the ttfs is complete in matter of the available glyphs, but even worth the "completenes" differes between the different styles - "normal" is most complete, "italic" less, etc...

I am very pressed on time and have not read through all the comments yet. I have some strong opinions on some of those though, and I will comment as soon as I find 30 minutes to read through all of it. Just FYI :).

> I see TTF handle bold/italic styles, so I wonder why it is handled in the code by using different fonts. This is not going to work if using the custom fonts. The bold faces processed by SDL_TTF do not look that bad imo.

What SDL_TTF is when rendering bold is to render the same text twice with an offset of a pixel - this looks good often enough, but not always. With some fonts it gets completely unreadable. The same for italic: it renders each character at an angle. It is a fragile system that we do not control. When you use bold/italic in your text processor, it will only work if there is a bold/italic ttf installed on your system - and imho that is a good thing. Reverting back to SDL_TTF rendering is a step backwards imho.

Frankly, letting the user choose a font is not a good idea. First: we have no control what characters are supported in the fonts the user chooses which sets us up for incompatibilities which we cannot fix ourselves. Second: Widelands does not deal well with different font sizes in different places, this can be fixed, but it is much easier if we commit ourselves to a fixed number of fonts that we will support and ship with Widelands.

This leads me to the following suggestion: Why not link language codes to fonts and ship them with Widelands? We then have complete control over how Widelands looks and renders, which chars it supports and we can get rid of this option. I could see that we then also change the <font> tag to only allow style changes as you are suggesting and not change the font that is used for rendering.

Just to manage expectations: I am in vacations for the next two weeks. I might have access to email, but I will not check them regularly, so you will not hear from me much till beginning of September.

Btw, i also looked at richtext_test branch and I think it should be merged. You made it more general than I ever did, even if it does not completely work for you, it is better than what was there before. We can fix the tests when we want to add new features to the renderer.

I agree with the solution of linking fonts to locales. Things will be simplier. We could therefore remove the face attribute of the font tag, and indeed introduce new ones such as condensed, sherif and so on.

Apart for that point, the c++ code is almost completely using the new renderer correctly now. The biggest piece of work remaining is for all the extra texts, still using the old tag syntax - even when it's through the lua wrapper. It is a lot of work to rewrite all texts.
I think we could start by improving the lua interface to make it more abstract. Currently, several attributes are passed in, and they are not allowed by the new syntax.

I think the rewrite of the README should wait for this to happen.

sounds good. do you think we should rush this before b18 or wait till after b18? It seems like rushing is dangerous but could be beneficial. Your call anyways - I think I will not be able to help a lot.

About rewriting the texts: All scenario texts should not need a rewrite, they do not use the rich text syntax explicitly. Only the ingame texts need rewrites, that is not too much to do imho.

No rush - I won"t have much time neither before release. And while not using the rich text syntax explicitly, i have the feeling the scenario texts will need to be reworked as well because the lua formatting interface does not abstract completely the old renderer.

Just a comment, an important though:

This branch is linked to be the fix for two high rated bugs, which should definitely be fixed for build18. Therefore I guess a soon merge (if nothing speaks against it) would be a good idea. If you decide to not merge before build18 feature freeze, we should still try to adapt the fixes for said bugs to current trunk, so the bugs can be fixed there as well.

This is what i get after merging trunk.
I made a test ui you can launch with ./widelands --testui

There are still some layout issues I need to fix.

Concerning the widgets, there are 2 minor issues left : Consecutive line breaks and RTL auto-wrapping.
Other than that, all use new font renderer and (multiline) editboxes work great.
Sirver, I will try to check the changes in your branch.

That's a lot of code. I need to make a chunk of time to review this.

My branch is about removing the legacy richtext renderer and using the new one everywhere (i.e. especially in scenario texts).

I will try to tidy the patch.

As we should first converge the API in graphics/ and text_layout.h, maybe you should have a look there in the first place. From what I see, your text_layout looks more appealing. I think we should merge yours into this one and update text_layout and text_area.

This patch is big, but changes to all ui elements was required. I would also like to unify all signals/getters/setters of basic elements

There are still some important changes that needs to be done here, with respect to the previous comments. That is linking font to locale, (re)introducing bold/italic and serif/condensed styles attributes and removing face attribute.

My branch is about removing the legacy richtext renderer and using the new one everywhere (especially in editboxes), so we should find some common grounds :). In particular, I introduced LayoutInfo to expose text nodes position and updated the rt_renderer to allow keeping extra spaces.

The aim should be to get this (and my branch) merged into trunk ASAP and then branch again to tackle the remaining issues. Smaller branches are much easier to work with anyways.

I try to look into this in more detail on the weekend.

I merged your branch and updated textarea, editbox and multilineeditbox, with various fixes.

I started looking into this now - the testui crashes on game close. and there are a lot of fixmes in the code that I do not understand and that could use a comment. I converted them into NOCOM. I also merged trunk.

I will be on vacation starting tuesday in a week for ~3 weeks. Just fyi :)

Sirver, can you take a look at text_layout.h? I defined it as a read-only interface, along with an implementation that allows adding nodes info. I am not sure how/where to define that implementation. It is only used in the fonthandler(which downcast and reset it) and rt_render.

Also, I removed custom font attributes, and linked font and horizontal align to the locale used. I also set an option to scale fonts to screen resolution.

- Is that font scale option needed? All text areas will have to specify their height in order to get correctly aligned if fonts are not scaled at higher resolution
- Concerning MultilineTextarea, in your branch you expected richtext, while here simple text is allowed and formatted as_ui_font, is that ok?

Happy vacation :)

RTL support is coming :)

This means a new dependency though, which is the ICU library. It comes with all kinds of i18n bells and whistles that I will play some more with for Build 20. For now, it would be good if our packaging wizards could double-check that the new library will work for everybody.

I will be gone from my development machine again, so if anybody else would like to play with this branch, be my guest. I especially need help with the text input boxes (editbox, multilineeditbox), which don't work at all at the moment.

Also, text flowing around images is broken (c.f. Campaign dialogs), and for some elements either the text height is reported wrongly or the valign is off - I don't know which yet (You can see this in the full screen menus, e.g. Load Game).

I'm setting this to "resubmit" only to greedily grab everyone's attention. This isn't ready for review by a long shot ;)

It does compile and work on win32, small fix only needed atm for a release build:
format.cc, function get_halign() does only contain an assert, a return statement is missing.

Thanks, I will look into this when I get back home :)

Seems to me that this branch is some kind incompatible with trunk... i get 186 conflicts when i try to merge trunk.

So what is the state here? Should we unlink the bugs?

I have removed the links to "fix commited" bugs.

To keep merging this branch is very high maintenance, and at this point only I know the code well enough to do it without jumping off a bridge ;)

Let's retarget the bugs.