[Cogl] [PATCH 1/2] cogl-pango: Updates to no longer require a default context

[Robert Bragg]

On Wed, May 16, 2012 at 12:21 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Just two minor suggestions:
>
> Robert Bragg <robert at sixbynine.org> writes:
>
>> +PangoFontMap *
>> +cogl_pango_font_map_get_default (CoglContext *context)
>> +{
>> +  if (G_UNLIKELY (!default_font_map))
>> +    default_font_map = cogl_pango_font_map_new (context);
>>
>> -  return renderer;
>> +  return default_font_map;
>> +}
>
> I wonder if it might be better to attach the default font map as user
> data on the context instead of storing it in a global variable. I guess
> it's kind of academic as it's pretty unlikely that an application would
> ever use more than one CoglContext but it seems a bit odd that you can
> pass in a context yet there is only one single global font map and you
> would end up getting one that is attached to a different CoglContext if
> you tried to use multiple contexts.

Right, the reason I changed this is that I was getting in a tangle
with circular references with the previous approach of caching the
renderer as part of the font_map once I started removing use of the
default context. I had started by associating a context with the
font_map, but then I wanted the renderer to refer back to the font_map
to figure out what context it should use resulting in the circular
ref. I initially tried dealing with that problem using weak pointer
tricks for the renderer cache pointer but then hit some other problem
and figured I should look at how this works for the cairo backend for
pango.

For cairo they just have a global "default" font map and also a global
renderer cache (although I think they support multi-thread access and
if the cache is locked in-use then a transient renderer is created for
the thread I think) and since that would avoid the circular reference
issue I figured I'd copy it.

I think you're right though that it would be more appropriate for us
to associate the font_map with the context, and I think that probably
the renderer cache should also be associated with the context in the
same way. This will also solve the problem of being able to free the
font_map and renderer state when the context is freed which is another
problem this has currently.

>
>> +PangoRenderer *
>> +_cogl_pango_renderer_get_default (CoglContext *context)
>> +{
>> +  if (G_UNLIKELY (!default_renderer))
>> +    default_renderer = _cogl_pango_renderer_new (context);
>> +
>> +  return default_renderer;
>> +}
>
> Same here. This one is probably more important because it's used
> internally so it would mean you really can't use cogl-pango with more
> than one Cogl context.
>
> Otherwise looks good to me.

thanks,
- Robert

>
> Reviewed-by: Neil Roberts <neil at linux.intel.com>
>
> - Neil