[haiku-commits] Re: r37413 - haiku/trunk/src/servers/app

  • From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Wed, 07 Jul 2010 11:51:43 +0200

pulkomandy@xxxxxxxxx wrote:
> Minimal font overlay :

It's very minimal, indeed, and there seem to be some issues, too:

> +     if (glyphIndex==0)
> +             return NULL;

Coding style.

>               const GlyphCache* glyph = entry->Glyph(charCode);
>               if (glyph == NULL) {
> -                     fprintf(stderr, "failed to load glyph for 0x%04lx 
> (%c)\n", charCode,
> -                             isprint(charCode) ? (char)charCode : '-');
> +                     // Try to find a suitable glyph in another font
> +                     FontCache* cache = FontCache::Default();
> +                     bool needsWriteLock = false;
> +                     ServerFont f(*(gFontManager->GetStyleByIndex("VL 
> Gothic",
> 0)));
> +                             // We always try to get the glyph from VL 
> Gothic, so 
> we can display
> +                             // japanese character. Other scripts (indian, 
> ...) 
> should be handled
> +                             // too, perhaps with a charcode > font mapping.

Coding style: 'f' is not a valid name, 80 characters per line (also at 
other places), space after comma.
I miss a TODO comment pointing out that having a hard coded font is 
definitely not the way to go.
Note, that once there are more than one fall back font, the locking 
order must be enforced in order to avoid deadlocks.

> +                     fallbackEntry = cache->FontCacheEntryFor(f);
> +                     if (!fallbackEntry || !fallbackEntry->ReadLock()) {

I miss a check "fallbackEntry == entry"; no need to double lock, and do 
the double work. At least the outer entry must be write locked at this 
point, or this would easily cause dead locks.

BTW are you planning to leave the broken Deskbar settings as is, or are 
you working on this? At least a reply to my mail on this matter would 
be appreciated.

Bye,
   Axel.


Other related posts: