Re: [PATCH] xcb: Initialize font options from Xft resources

Hi Uli,

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 <[email protected]> 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:

https://qt.gitorious.org/qt/qtbase/source/HEAD:src/plugins/platforms/xcb/qxcbscreen.cpp#L570
http://cgit.freedesktop.org/xcb/util-cursor/tree/cursor/cursor.c#n49

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
needed.

> 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?

Ok.

> 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.

Lukas
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo