[cairo] [PATCH] xcb: Initialize font options from Xft resources
lukas at oxygene.sk
Wed Sep 3 23:47:33 PDT 2014
Thanks for the review. I'll update the code and send a new patch later
today. Some comments bellow.
On Thu, Sep 4, 2014 at 8:23 AM, Uli Schlachter <psychon at znc.in> wrote:
> Where did you look into how these resources are implemented and how they should
> be parsed? Any source that should/can be quoted?
I had trouble getting to any official documentation and I couldn't
easily find the relevant parsing code in xlib, so I had to guess a
little bit. I used these two implementations as my reference:
I was actually considering building a small "xcb-util-xrm" lib out of
this, but that would complicate using it Cairo, which is what I
> I wonder if this doesn't better belong into something like a new
> cairo-xcb-resources.c to isolate it better from the rest. No strong opinion on this.
Good idea. There is a lot of code here, so I think it makes sense.
> Should be renamed to resource_parse_lines(). In the caller below I wondered "why
> does this only parse a single line?".
Will do. This is a left-over from my previous parsing code.
> I know that this break is necessary to make the parsing-in-parts work, but this
> also means that the very last line of the resources property won't be parsed if
> it doesn't end in a line break. Is this intended?
It was intended, but now that I think about it again, it probably
doesn't make too much sense. I'll add refactor the code a little bit
to attempt to parse the last line in resources_parser_done.
> Shouldn't this always subtract bytes_parsed, not just when it happens to be
> smaller than bytes_in_buffer? In other words: What happens if the buffer end was
> on a line break?
Correct, that's a bug, will fix it.
> How come you chose 1024?
Completely arbitrary decision. Qt actually uses 4KB and xcb-lib-cursor
16KB. I can make it bigger to match either of those.
(The number is in long units like offset, so the sizes are times 4.)
> I looked at cairo-xlib. If there is no xft.rgba entry, it uses RENDER to query
> the subpixel order. Could you prepare the code for this and leave behind a
> comment saying that this is missing? Perhaps even implement the necessary call
> to xcb_render_query_subpixel_order?
> Who actually acquires this mutex? In cairo-xlib,
> _cairo_xlib_screen_get_font_options() does this explicitly. In your patch,
> nothing does.
This part is based on a lot of assumptions, as I'm not familiar with
neither cairo not cairo-xcb code. I was simply going by what other
functions in this file do.
> Why this if? _cairo_xcb_screen_get() sets the xcb_screen member right after
> allocating the screen and, as far as I can tell, nothing ever unsets it.
Same as above, it was just a guess. Will remove it.
More information about the cairo