[poppler] Fontconfig patch

[Kristian Høgsberg]

Albert Astals Cid wrote:
> Ok, here it goes again the patch with the fixes from Jeff about styling and 
> the h - name thing, the fix for Brad if BOOK is not defined and the fix for 
> Stefan so he can define and extern fontconfig configuration object.
> 
> More comments?

Hi Albert,

Sorry for the late reply, I just got back from the Desktop Developers 
Conference.  This patch looks really good, I have a couple of comments 
below.

>>@@ -394,21 +344,21 @@
>>       } else if (!cmd->cmp("toUnicodeDir")) {
>> 	parseToUnicodeDir(tokens, fileName, line);
>>       } else if (!cmd->cmp("displayFontT1")) {
>>-	parseDisplayFont(tokens, displayFonts, displayFontT1, fileName, line);
>>+// 	parseDisplayFont(tokens, displayFonts, displayFontT1, fileName, line);
>>       } else if (!cmd->cmp("displayFontTT")) {
>>-	parseDisplayFont(tokens, displayFonts, displayFontTT, fileName, line);
>>+// 	parseDisplayFont(tokens, displayFonts, displayFontTT, fileName, line);
>>       } else if (!cmd->cmp("displayNamedCIDFontT1")) {
>>-	parseDisplayFont(tokens, displayNamedCIDFonts,
>>-			 displayFontT1, fileName, line);
>>+// 	parseDisplayFont(tokens, displayNamedCIDFonts,
>>+// 			 displayFontT1, fileName, line);
>>       } else if (!cmd->cmp("displayCIDFontT1")) {
>>-	parseDisplayFont(tokens, displayCIDFonts,
>>-			 displayFontT1, fileName, line);
>>+// 	parseDisplayFont(tokens, displayCIDFonts,
>>+// 			 displayFontT1, fileName, line);
>>       } else if (!cmd->cmp("displayNamedCIDFontTT")) {
>>-	parseDisplayFont(tokens, displayNamedCIDFonts,
>>-			 displayFontTT, fileName, line);
>>+// 	parseDisplayFont(tokens, displayNamedCIDFonts,
>>+// 			 displayFontTT, fileName, line);
>>       } else if (!cmd->cmp("displayCIDFontTT")) {
>>-	parseDisplayFont(tokens, displayCIDFonts,
>>-			 displayFontTT, fileName, line);
>>+// 	parseDisplayFont(tokens, displayCIDFonts,
>>+// 			 displayFontTT, fileName, line);
>>       } else if (!cmd->cmp("psFile")) {
>> 	parsePSFile(tokens, fileName, line);
>>       } else if (!cmd->cmp("psFont")) {

Let's just remove these function call entirely, but leave the if-cases 
so we don't get errors when reading the xpdfrc file.


>>+FcPattern *buildFcPattern(GfxFont *font)
>>+{
>>+  int weight = FC_WEIGHT_NORMAL,
>>+      slant = FC_SLANT_ROMAN,
>>+      width = FC_WIDTH_NORMAL,
>>+      spacing = FC_PROPORTIONAL;
>>+  bool deleteFamily = false;
>>+  char *family, *name, *lang, *aux = NULL;
>>+  const char *h = NULL;
>>+  FcPattern *p;
>> 
>>-  lockGlobalParams;
>>-  dfp = (DisplayFontParam *)displayFonts->lookup(fontName);
>>-  unlockGlobalParams;
>>-  return dfp;
>>+  // this is all heuristics will be overwritten if font had proper info
>>+  name = font->getName()->getCString();
>>+  
>>+  // remove the - from the names, for some reason, Fontconfig does not understand "MS-Mincho"
>>+  // but does with "MS Mincho"
>>+  int len = strlen(name);
>>+  for (int i = 0; i < len; i++)
>>+    name[i] = (name[i] == '-' ? ' ' : name[i]);
>>+  
>>+  // This is the "descriptors" font names use usually, we could add more

We need to tweak this heuristic a bit, I think.  As it is "Times-Roman" 
is turned into "Times Roman" which fontconfig doesn't find.  fontconfig 
knows about "Times" and "Times New Roman", but for "Times Roman" 
fontconfig returns the default sans face.  The problem is that 
"Times-Roman" is one of the 14 base PDF fonts and we should at least 
find a serif font for it.

I suggest that we treat the "-Roman" suffix as a modifier just like we 
handle "-Italic", "-Bold", and "-Oblique".  However we don't want to 
remove "Roman" from fonts such as "Times New Roman" or "Nimbus Roman 
No9", so we should only search for modifiers after either a '-' or a 
','.  I've attached a updated version of buildFcPattern() that does 
this; if this new version is OK with you, just go ahead and commit the 
whole thing.

Just a couple of stylistic issues:

This spacing is kinda funny:

   switch (font -> getStretch())

we usually just use font->getStretch(), and the cases should also match 
the style of the rest of the code, e.g.

     case GfxFont::UltraCondensed:
       width = FC_WIDTH_ULTRACONDENSED;
       break;

etc. and similar for font->getWeight().

All in all, great work.  This patch simplifies poppler font matching 
greatly.  We'll probably need to push some of our font aliases upstream 
to fontconfig or distros as Martin mentions, but that's where they 
belong.  Specifically, fontconfig should substitute "Standard Symbols L" 
for "Symbol" and "Dingbats" for "ZapfDingbats".

Kristian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: heuristic.cpp
Type: text/x-c++src
Size: 4754 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/poppler/attachments/20050721/1c2408e2/heuristic.cpp