widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

Okay, next round :). I think this converges..

> Time for the next round. FontSet now has its own class and parses itself. The singleton is gone - > FontHandler1 now does the job.

That is much better. But the singleton is still raining on our parade with the stand alone rich text renderer. See codereview comment.

> I pulled TextStyle out of the basic font class to avoid circular dependencies, and moved:
> src/wui/text_constants.h => src/graphic/text_constants.h
> src/wui/text_layout.cc => src/graphic/text_layout.cc
> src/wui/text_layout.h => src/graphic/text_layout.h

Makes sense. One issue that I see there is that ui_basic now depends on the FontSet and text styles we define. Not ideal, as ui_basic should be basic (in the sense that it can easily be reused for other projects), but not a show stopper. I also have no clear suggestion how to avoid this right now.

> If you think these should rather be in ui_basic, will do. I'm kind of in two minds about this.

Same here. Text formatting seems very essential to UI, so they kinda belong there. Special text formatting (like our yellow text for UI) though is very Widelands specific, so it kinda does not belong there. 


> - License and source info for downloaded fonts in i18n.

I looked over them, but I do not understand enough about licensing to understand if they are all okay. I think they are, but debian will probably teach us a lesson or two about licenses.

- Lua files in i18n
- FontHandler1 and classes that use it
- FontSet and classes that use it
- Options Menu
- New function get_string_with_default in lua_table.h.

all reviewed.
-- 
https://code.launchpad.net/~widelands-dev/widelands/fonts/+merge/238608
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/fonts.