195868 - GTK2: GetSystemFontInfo() needs to return valid fonts for X fonts

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Masaki Katakai]

GetSystemFontInfo() of GTK2 just returns

  pango_font_description_get_family(desc)

as family name. Usually the family name does not fit to X
Window fonts. Actually "sans" is returned on my desktop
running Russian locale. I want Russian fonts for rendering
on UI, but LoadFont() returns korean fonts because korean
font was found before Russian and it convers the code points
of Russian. Also the same problem of bug 149692 would happen.

We can take the same approach of bug 149692 here.
We need to give more information founday, family,
charset encoding and registry when we use X fonts.

Comment 1

[Masaki Katakai]

Some discussion in bug 181053.

Comment 2

[Masaki Katakai]

This problem is serious on Solaris Russian locale.
With localized GUI message, Mozilla now uses
2byte-font of Korean.

Comment 3

[Masaki Katakai]

Attachment: patch

same approach of bug 149692

- get XLFD names by pango_x_font_subfont_xlfd()
- return FFRE

I don't think XFT needs the changes. So I use ifndef MOZ_ENABLE_XFT.

Comment 4

[Christopher Blizzard (:blizzard)]

Are you guys actually using Gtk2 without Xft?

Comment 5

[Masaki Katakai]

Yes, we are not using XFT.

Comment 6

[Christopher Blizzard (:blizzard)]

Comment on attachment 116181 [details] [diff] [review]
patch

>-#ifdef MOZ_WIDGET_GTK
>-
> static void
> AppendFontFFREName(nsString& aString, const char* aXLFDName)
> {

gtk2 w/ xft doesn't need this function - in fact, xft ignores FFRE fonts that
are passed to it in the FFRE format.  You might just want to use
MOZ_ENABLE_COREFONTS here instead of using the widget + xft settings to detect
if core fonts are compiled in.

> #ifdef MOZ_WIDGET_GTK2
>+#ifndef MOZ_ENABLE_XFT

Once again, use MOZ_ENABLE_COREFONTS instead.

>   aFont->name.Truncate();
>+#ifdef MOZ_ENABLE_XFT
>   aFont->name.AppendWithConversion(pango_font_description_get_family(desc));
>+#else
>+  xlfd_from_pango_font_description(aWidget, desc, aFont->name);
>+#endif

Technically you should figure out if you're using Xft or not to determine which
function to use and use the COREFONTS define as above to determine when to
compile it in.	This is included as part of the gfx module (see NS_IsXftEnabled
in nsFontMetricsUtils.cpp - it should be safe to use.)

>   aFont->weight = pango_font_description_get_weight(desc);
> 
>   gint size;
>@@ -1052,3 +1063,128 @@
>   return 0;
> }
> #endif /* MOZ_ENABLE_XFT */
>+
>+#ifdef MOZ_WIDGET_GTK2
>+#ifndef MOZ_ENABLE_XFT

COREFONTS, as above.

Comment 7

[Masaki Katakai]

Attachment: 2nd patch

Thank you for review, I have attached the 2nd patch, please review.

I have changed to the following.

When Xft is enabled, even if COREXFONT is compiled in,
xlfd_from_pango_font_description() will be called.

+#ifdef MOZ_ENABLE_XFT
+  if (NS_IsXftEnabled()) {
+    aFont->name.AppendWithConversion(pango_font_description_get_family(desc));

+  }
+#endif /* MOZ_ENABLE_XFT */
+
+#ifdef MOZ_ENABLE_COREXFONTS
+  // if name already set by Xft, do nothing
+  if (!aFont->name.Length()) {
+    xlfd_from_pango_font_description(aWidget, desc, aFont->name);
+  }
+#endif /* MOZ_ENABLE_COREXFONTS */

Comment 8

[Christopher Blizzard (:blizzard)]

Comment on attachment 116401 [details] [diff] [review]
2nd patch

r=blizzard

Comment 9

[rbs]

Comment on attachment 116401 [details] [diff] [review]
2nd patch

sr=rbs, my own style would have been |if (!...)| here:
+  if (aFontDesc == NULL) 
+  if (fontmap == NULL) {
+  if (font == NULL) {
...

Comment 10

[Masaki Katakai]

Thank you very much for quick review and super-review.
Sure. I'll correct == NULL and will check-in.

Comment 11

[Masaki Katakai]

Attachment: 3rd patch

I'm very sorry I had a mistake. We don't need the last line

aFont->name.AppendWithConversion(pango_font_description_get_family(desc));

We already call it in if (NS_IsXftEnabled()) {}. I have removed
the line and corrected if ( == NULL).

  aFont->name.Truncate();
#ifdef MOZ_ENABLE_XFT
  if (NS_IsXftEnabled()) {
    aFont->name.AppendWithConversion(pango_font_description_get_family(desc));
  }
#endif /* MOZ_ENABLE_XFT */

#ifdef MOZ_ENABLE_COREXFONTS
  // if name already set by Xft, do nothing
  if (!aFont->name.Length()) {
    xlfd_from_pango_font_description(aWidget, desc, aFont->name);
  }
#endif /* MOZ_ENABLE_COREXFONTS */

  aFont->name.AppendWithConversion(pango_font_description_get_family(desc));

Comment 12

[Masaki Katakai]

Comment on attachment 116525 [details] [diff] [review]
3rd patch

Sorry for dup work. I have removed the line below because it's duplicate.

-  aFont->name.AppendWithConversion(pango_font_description_get_family(desc));

Could you r= and sr= again?

Comment 13

[Christopher Blizzard (:blizzard)]

Comment on attachment 116525 [details] [diff] [review]
3rd patch

r=blizzard

Comment 14

[Masaki Katakai]

patch checked in to Trunk.

Thank you very much for review and super-review.