-
-
Notifications
You must be signed in to change notification settings - Fork 620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: Better Text Rendering for cairo / lua #1501
base: main
Are you sure you want to change the base?
Conversation
Add empty files and various build options for the lua text bindings Adds requirements for Fontconfig, Freetype and Harfbuzz
✅ Deploy Preview for conkyweb canceled.
|
I thought I already removed this and squashed the commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall, just need to add documentation to https://github.com/brndnmtthws/conky/blob/main/doc/lua.yaml for the new functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably enable this in the macos build too, if you want to take a stab at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I figure out why the ubuntu builds are failing i'll have a shot at enabling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions in here to be documented in conky/doc/lua.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a shot at doing this in a way that hopefully makes sense, for some reason i couldn't generate docs locally but that doesn't seem related to any of my changes.
If your happy with that i'll probably do similar for the imlib2 bindings that also aren't currently documented.
Thanks for the feedback, i'll update the docs and finish of implementing the Top to Bottom features then finish up a couple of configs to use them before sending through some updates. But its good to know i'm heading in the right direction. |
After trying this patches, bug #75 still exist! |
Yes that is currently to be expected, the WIP patch just had the LTR support so I could get feedback on the API, RTL is almost there I just need to finish writing the tests. Also this change only works with the lua API, however it is the first step in my project to add a toolkit to make using the lua API much simpler. Although that is further from completion. |
@moceap Can you confirm that the following screenshot looks ok, it should be the days of the week. I'll probably do just a little more testing of my actual real world usecases before sending through an updated pull request |
No it's not OK. Letters in Arabic should be connected: You write: الإثنين Look at them here in the browser, they are connected. |
Thanks for the confirmation, that's what I thought i'll have to have a closer look at whats going wrong, the left to right seems to be ok. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never used HarfBuzz, but from what I picked up googling it, the way you're using it seems fine.
lua/libcairo_text_helper.h
Outdated
//alternatively you can use hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs()); | ||
hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs()); | ||
hb_buffer_set_direction(buf, text_direction); /* or LTR */ | ||
hb_buffer_set_script(buf, text_script); /* see hb-unicode.h */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which text_script
are you using to test out arabic? It should be "Arab" as defined in hb-common.h
.
} | ||
hb_script_t text_script = hb_script_from_string(script, -1); | ||
if (text_script == HB_SCRIPT_UNKNOWN) { | ||
text_script = HB_SCRIPT_COMMON; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try setting this to HB_SCRIPT_ARABIC
(string literal) instead of HB_SCRIPT_COMMON
and see if it works (though it will break everything else).
lua/libcairo_text_helper.h
Outdated
//alternatively you can use hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs()); | ||
hb_buffer_set_unicode_funcs(buf, hb_glib_get_unicode_funcs()); | ||
hb_buffer_set_direction(buf, text_direction); /* or LTR */ | ||
hb_buffer_set_script(buf, text_script); /* see hb-unicode.h */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be hb-common.h
as well. Just pointing it out (so it's easier for others to pick up), not that these comments will affect anything.
lua/libcairo_text_helper.h
Outdated
text_script = HB_SCRIPT_COMMON; | ||
} | ||
|
||
hb_language_t text_language = hb_language_from_string (language, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the language set to "ar"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the full setting was {font_family="Noto Sans Arabic", font_size=16, align=CAIRO_TEXT_ALIGN_LEFT, font_direction="RTL", font_script="HB_SCRIPT_ARABIC", font_language="ar"}
lua/libcairo_text_helper.h
Outdated
hb_buffer_destroy(buf); | ||
} | ||
|
||
void cairo_text_hp_simple_show(cairo_t *cr, int x, int y, const char *text, FontData *font) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's no need for these cairo_text_hp_simple_show_*
convenience functions. toluapp supports default values and harfbuzz can infer defaults too.
You can make all of the properties optional by:
- adding a call to
hb_buffer_guess_segment_properties (hb_buffer);
incairo_text_hp_intl_show
after specified properties are set. - setting argument order to be:
language
(because it's inferred from system (least smart)),script
(inferred from buffer contents),direction
(inferred from script)- as documented here
- that order would minimize the need for supplying well inferred values to set the one without a good default
- specifying argument defaults to be
nullptr
/NULL
in the header- e.g.
const char *direction = NULL
- and not calling
hb_*_from_string
if the value is null (i.e. empty string).
- e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same line of reasoning I'd position cairo alignment
argument after the font
argument and set it to default to CAIRO_TEXT_ALIGN_LEFT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the binding function should be able to load and cache a font. The cache can leak memory (no need to call free) as it would be weird to manage such things from a lua script and fonts will likely be needed for each draw call.
So the signature of cairo_text_hp_intl_show
in .pkg would finally look like: void cairo_text_hp_intl_show(cairo_t *cr, int x, int y, const char *text, const char *font = NULL, int font_size = 12, cairo_text_alignment_t alignment = CAIRO_TEXT_ALIGN_LEFT, const char *language = NULL, const char *script = NULL, const char *direction = NULL)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't see this until now, I'll certainly swap to the idea of default arguments that seems much better. Leaking fonts seems reasonable to me as well so i'm happy to just add a cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am implementing most of this, with the exception of language
which i'm going to default to english if not explicitly set. The reason being if I create a config / script for conky and want to distribute it, I don't want it to start breaking in random ways because other people are using a different language. Also much of conky's outputs such as process names CPU percentages etc work best in English. In this case if you are doing something that requires an explicit language you'll need to set it as well.
@simotek Take a look https://github.com/HOST-Oman/libraqm, It may fast the process. |
One thing I noticed at https://github.com/HOST-Oman/libraqm/blob/9d7ca9d59dba38e777bdcd096870776dab5e6e66/src/raqm.c#L1793C1-L1793C48 is that for the RTL case they seem to have hardcoded a -1 into the calculation for character position, So I might have to have a play with that later. It seemed like hardcoding a -1 into the width of each character would solve the issue but I didn't want to just do it without seeing it documented, but the fact this other codebase is suggests its probably the right solution |
Add empty files and various build options for the lua text bindings Adds requirements for Fontconfig, Freetype and Harfbuzz
This should go into its own PR but right now I need it for my system to work.
This replaces the old approach of using a series of function declarations.
For some reason RTL positioning is wrong, it seems somewhat based on font size 1/10th font size seems to get closer to correct so run with that for now, i'll continue investigating a proper fix.
✅ Deploy Preview for conkyweb ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
So far I've done all the interface changes, other then caching fonts, which is next on my list. As for the RTL codepaths, subtracting 1/10th of the font size seems to get much closer then -1 especially for larger font sizes. Its not perfect for all fonts at all sizes though, but is significantly better, i'll follow this up with upstream closer @moceap I presume this screenshot is getting much closer. At this point I have no idea if the text is actual words. |
Also if anyone has any idea what Ubuntu packages are needed to get cmake to find freetype that would be much appreciated so that I don't need to spin up a VM and test. |
Fonts are now cached in C, given the number of fonts and sizes I expect people will use, this implementation seems simpler then implementing hashmaps or attempting to do bindings in C++
This reverts commit 5036cc4. Fixed better upstream now
On my machine i'm using it with lua5.3 and I see packages for 5.4 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some inputs on tables in docs.
| script | A string containing a ISO 15924 script tag. | | ||
| | Default value: auto-detect from text | | ||
| direction | A string representing text direction eg `LTR`, `RTL` or `TTB` | | ||
| | Default value: auto-detect from text otherwise `LTR` | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra pipe (|
) at the end.
| script | A string containing a ISO 15924 script tag. | | ||
| | Default value: auto-detect from text | | ||
| direction | A string representing text direction eg `LTR`, `RTL` or `TTB` | | ||
| | Default value: auto-detect from text otherwise `LTR` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of putting default value into the next row, it might be better to put it in the same cell and use <br/>
so that it looks like this:
Argument | Description |
---|---|
language | A string containing a BCP 47 language tag. Default value: en |
Should be
|
Description
Introduction
I have attempted to resolve #900 by creating a new lua binding specifically for doing font rendering in Cairo using Freetype and Harfbuzz, fontconfig is also used to locate the correct fonts. I have chosen to follow the approach of the
imlib2_helper
binding and not expose functions from the headers of each of these libraries but rather provide simple functions that "internally" use each of these libraries.At the time of creating this PR I have no fixed thoughts on the API other then it works for my use case and tries to balance the simple act of rendering an English string while also allowing complex options such as RTL text as requested in #75 as well as being able to center and right align text #940 and #1017. So your thoughts on the API topic would be much appreciated, once the API is finalized I will add appropriate documentation. currently this is the last major step for be before being able to move to a lua only config.
Current API Description (Happy to make changes here)
Loading a font from a file is a slow enough operation that we probably don't want to do it multiple times so I separated loading fonts and freeing memory at close into separate functions.
The next set of functions cover the "Simple" English use cases, I could have added a parameter for alignment but I was trying to keep the common Left case as simple as possible.
The next function is the "catch all" with the main implementation and allows tweeking of the internalization options such as text direction in Harfbuzz for people looking to use non latin languages.
Finally when doing layouting sometimes its useful to know how wide the final string will be so the following find the text width, again there is a simple latin version and an internationalized version. In the future I'll also provide a Height function which will be important for supporting vertical text.
Screenshot
In the following screenshot the "Cairo" text is using Cairo's built in basic text rendering, the "Conky" text is using Conky's standard text rendering and the "Freetype" strings are using this new API with Freetype and Harfbuzz rendering the text in Cairo. All these cases are just using the default settings as found on openSUSE Tumbleweed.