[GTK Config] Construct font style by hand instead of relying on Qt function
ClosedPublic

Authored by gikari on Feb 13 2020, 7:19 PM.

Details

Summary

It has been reported, that on localized systems
the font style was written in local language.
Therefore config line was incorrectly read by
GTK and therefore applications displayed bad
font. To avoid that this commit is introduced.

BUG: 333146
FIXED-IN: 5.18.4

Test Plan
  1. Reload kded5
  2. Apply fonts with various styles and check if style is correctly propagated to GTK configuration files (e.g. ~/.config/gtk-3.0/settings.ini)
  3. Apply fonts with localized style name (e.g. Arial Обычный) and check, that font in GTK configs uses English name for style.
  4. Check that in above cases GTK applications correctly distinguish the styles (i.e. Medium font is bolder, that Regular, Condensed font is more condensed, that Regular, Italic font is more skewed)

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gikari created this revision.Feb 13 2020, 7:19 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 13 2020, 7:19 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gikari requested review of this revision.Feb 13 2020, 7:19 PM
gikari edited the summary of this revision. (Show Details)Feb 13 2020, 7:20 PM

I don't understand the problem in-depth, but isn't it possible to write non-localized style name instead?

I don't understand the problem in-depth, but isn't it possible to write non-localized style name instead?

Indeed.

Is the problem that the style name is already localized by the time we get it from the Fonts KCM?

cfeck added a subscriber: cfeck.Feb 13 2020, 9:41 PM

So... is this all related to bug https://bugs.kde.org/show_bug.cgi?id=378523 ?
:/

Is the problem that the style name is already localized by the time we get it from the Fonts KCM?

Yes. It's already displayed localized, so that I cannot do anything with it.

Is the problem that the style name is already localized by the time we get it from the Fonts KCM?

Yes. It's already displayed localized, so that I cannot do anything with it.

So maybe that's something we can fix in the Fonts KCM? Or Is it caused by the Qt bug ultimately causing https://bugs.kde.org/show_bug.cgi?id=378523?

gikari added a comment.EditedFeb 13 2020, 10:06 PM

So maybe that's something we can fix in the Fonts KCM? Or Is it caused by the Qt bug ultimately causing https://bugs.kde.org/show_bug.cgi?id=378523?

Probably a Qt bug, because Fonts KCM uses QtQuick.Dialogs.FontDialog, and there is already the localized style displayed at some fonts.

*groan*

I guess we need to fix it Qt then. I don't know much about fonts but maybe it could display the localized string in the UI but write an un-localized version to the config file? Would that work? And then maybe we can somehow finally fix https://bugs.kde.org/show_bug.cgi?id=378523.

Take this code:

static QString styleStringHelper(int weight, QFont::Style style)
{
    QString result;
    if (weight > QFont::Normal) {
        if (weight >= QFont::Black)
            result = QCoreApplication::translate("QFontDatabase", "Black");
        else if (weight >= QFont::ExtraBold)
            result = QCoreApplication::translate("QFontDatabase", "Extra Bold");
        else if (weight >= QFont::Bold)
            result = QCoreApplication::translate("QFontDatabase", "Bold");
        else if (weight >= QFont::DemiBold)
            result = QCoreApplication::translate("QFontDatabase", "Demi Bold");
        else if (weight >= QFont::Medium)
            result = QCoreApplication::translate("QFontDatabase", "Medium", "The Medium font weight");
    } else {
        if (weight <= QFont::Thin)
            result = QCoreApplication::translate("QFontDatabase", "Thin");
        else if (weight <= QFont::ExtraLight)
            result = QCoreApplication::translate("QFontDatabase", "Extra Light");
        else if (weight <= QFont::Light)
            result = QCoreApplication::translate("QFontDatabase", "Light");
    }

    if (style == QFont::StyleItalic)
        result += QLatin1Char(' ') + QCoreApplication::translate("QFontDatabase", "Italic");
    else if (style == QFont::StyleOblique)
        result += QLatin1Char(' ') + QCoreApplication::translate("QFontDatabase", "Oblique");

    if (result.isEmpty())
        result = QCoreApplication::translate("QFontDatabase", "Normal", "The Normal or Regular font weight");

    return result.simplified();
}

Drop the QCoreApplication::translate("QFontDatabase", " stuff, and it should work fine.

Take this code

How about other styles, such as Condensed, Book etc?

Can we give a shot to @davidedmundson proposal? Maybe slightly extended to cover some extra enum cases if necessary?

bport added a comment.EditedFeb 17 2020, 12:09 PM

I think a good fix can be to use @davidedmundson solution combined with a check to know if font styleName is not set (if it set it will be not translated.

I think we can do something like that
QFont yourFont = QFont(...);
auto f = QFontInfo(youFont)
f.styleName(); <<= is in theory empty if it's not enforced. In other case stuff based on enum will be ok. Goal of style name is to express "irregular" style name according to Qt documentation

I just read bug report again and the from seems to came from , not from localization.

By the way I neved end with localized style, how do you achieve that ? (I mean even in UI)

gikari updated this revision to Diff 75858.Feb 17 2020, 7:43 PM
gikari edited the summary of this revision. (Show Details)

Use big helper function for constructing font style. Please test, many fonts may break and not work.

gikari retitled this revision from [GTK Config] Write Font without style name to [GTK Config] Construct fontStyle by hand instead of relaying on Qt function.Feb 17 2020, 8:32 PM
gikari edited the summary of this revision. (Show Details)
gikari retitled this revision from [GTK Config] Construct fontStyle by hand instead of relaying on Qt function to [GTK Config] Construct font style by hand instead of relying on Qt function.
gikari edited the summary of this revision. (Show Details)Feb 19 2020, 8:04 AM
chauvin added inline comments.
kded/configvalueprovider.cpp
50

I think

QStringLiteral(", ")

should be replaced by a space (see my comment)

however I don't know what will happen in ambigious cases such like:
"DejaVu Sans" + " " + "Condensed Bold" + " " +"11"
and
"DejaVu Sans Condensed" + " " + "Bold" + " " +"11"

gikari added inline comments.Feb 20 2020, 8:33 PM
kded/configvalueprovider.cpp
50

Comma is important, if not the comma, the GTK would not be able to parse font with spaces in name. Comma fixed that bug: https://bugs.kde.org/show_bug.cgi?id=380980

chauvin added inline comments.Feb 20 2020, 8:55 PM
kded/configvalueprovider.cpp
50

I observed a different behavior with ~/.gtkrc-2.0 (libgtk version 2.62.1)
There is perhaps a difference betwen gtk2 and gtk3

chauvin added inline comments.Feb 20 2020, 10:42 PM
kded/configvalueprovider.cpp
50

Sorry I missread the version of libgtk2.0 it is 2.24.32 (not 2.62.1)

It works with the following line in ~/.gtkrc-2.0:

gtk-font-name="Ubuntu, Light 11"
gtk-font-name="Ubuntu Light Regular 11"

but it doesn't work with:

gtk-font-name="Ubuntu Light, Regular 11"

So I assume it is a bug in gtk2 parser and the comma is not the problem

I am not sure but I think the parsing is done in this function:
https://github.com/GNOME/pango/blob/mainline/pango/fonts.c#L1265

A minor style nitpick, otherwise LGTM. As mentioned by the author, this likely require extensive testing before going in.

kded/configvalueprovider.cpp
53

Space should be before & not after

gikari updated this revision to Diff 76312.Feb 24 2020, 5:08 PM
gikari marked an inline comment as done.

Code style changes

ervin added a comment.Feb 25 2020, 2:35 PM

LGTM, now requires swift testing to not miss the next bugfix release

gikari edited the test plan for this revision. (Show Details)Feb 25 2020, 3:20 PM

Do you need to reload any daemon to get this to work?

Do you need to reload any daemon to get this to work?

Yes

gikari edited the test plan for this revision. (Show Details)Feb 25 2020, 4:33 PM

Thanks.

So if I set the general font to 9pt Ubuntu Italic, then in the config file I see gtk-font-name=Ubuntu, Italic 9 and Thunderbird respects the font, and the size, but not the style; it's not italic. :(

Or is that a separate issue?

Thanks.

So if I set the general font to 9pt Ubuntu Italic, then in the config file I see gtk-font-name=Ubuntu, Italic 9 and Thunderbird respects the font, and the size, but not the style; it's not italic. :(

Or is that a separate issue?

Does the Italicness only not work with Ubuntu font?

All fonts tested. :(

All fonts tested. :(

Hm, I can't get it work too. On GNOME via tweaks Italic fonts also do not work, so I guess it is a GTK bug.

gikari edited the summary of this revision. (Show Details)Feb 25 2020, 8:30 PM

BAH!

So what kind of font styles do work?

All fonts tested. :(

Thunderbird is a gtk3 application ?

If I manually change .gtkrc-2.0 with this line:

gtk-font-name="Ubuntu, Italic 9"

Gimp and Inskape display an italic font on my machine.

You're right, it does work for GTK2 apps on my system, but not GTK3 apps. :/

BAH I say!

gikari marked 4 inline comments as done.Feb 26 2020, 10:44 AM

I have just tested a couple of fonts.

  1. I've only tested Monospaced font with Russian locale, so I do not know it other ones work. Qt just ignores style string and any style of Monospaced is just Monospaced, 10 in the config.
  2. Seems like GTK2 works fine with fonts and most of them works (Ubuntu Light for example).
  3. GTK3 however have troubles with font styles and ignores what is after a comma. So that may be a GTK3 bug.

All in all it seems like the patch does not introduce any regressions (for me at least). Behaves better, that what was before: localized font styles are just ignored in the process, so the patch fixes a bug, where fonts in GTK apps are totally wrong and set to default.

Agreed. Otherwise it does work and fixes the bug, and the code looks sensible to me.

However I know what @davidedmundson is going to say: "Working around Qt bugs requires a code comment mentioning the Qt bug report." :) So let's add that.

Also I could not find a bug report in https://gitlab.gnome.org/GNOME/gtk/issues about this issue of GTK3 ignoring the font style. Can you file one?

I am missing context on this bug, but I wonder why there were no issues with font configuration prior to 5.18

Was there a different workaround then?

Prior to 5.18, you had to set the GTK font separately; now it's auto-synced.

Were you able to use an italic/bold/etc GTK font in 5.17 or earlier? I must admit I never tried, so if not being able to now is a regression, that's why I never noticed. :(

Prior to 5.18, you had to set the GTK font separately; now it's auto-synced.

Right, so that's the difference in font style names between Qt and GTK that caused the problem?

Were you able to use an italic/bold/etc GTK font in 5.17 or earlier? I must admit I never tried, so if not being able to now is a regression, that's why I never noticed. :(

Never tried, but Regular worked :)

Prior to 5.18, you had to set the GTK font separately; now it's auto-synced.

Right, so that's the difference in font style names between Qt and GTK that caused the problem?

The problem being solved here is that starting with 5.18, style names were being localized, which GTK doesn't understand, causing it to fall back to the default font. This patch fixes that.

Were you able to use an italic/bold/etc GTK font in 5.17 or earlier? I must admit I never tried, so if not being able to now is a regression, that's why I never noticed. :(

Never tried, but Regular worked :)

And with this patch, it still will. :)

The problem being solved here is that starting with 5.18, style names were being localized, which GTK doesn't understand, causing it to fall back to the default font. This patch fixes that.

I see :)
I just thought maybe the pre-existing solution from the deleted KCM could be reused since it seemed to work well.

The problem being solved here is that starting with 5.18, style names were being localized, which GTK doesn't understand, causing it to fall back to the default font. This patch fixes that.

I see :)
I just thought maybe the pre-existing solution from the deleted KCM could be reused since it seemed to work well.

I literally copied the font construction algorithm from GTK KCM. May be the font, that was passed from the Qt Widgets picker was different.

However I know what @davidedmundson is going to say: "Working around Qt bugs requires a code comment mentioning the Qt bug report." :) So let's add that.

Also I could not find a bug report in https://gitlab.gnome.org/GNOME/gtk/issues about this issue of GTK3 ignoring the font style. Can you file one?

So, I need to mention two bug reports: Qt one and GTK one. And should I create both of them? In case of GTK everything is somewhat clear, but in case of Qt I do not even know what to report (or how to do that correctly, or where it is broken): font picker just gives you a font with style name in local language, which is correctly applied for Qt applications, but not for GTK ones.

So, I need to mention two bug reports: Qt one and GTK one. And should I create both of them? In case of GTK everything is somewhat clear, but in case of Qt I do not even know what to report (or how to do that correctly, or where it is broken): font picker just gives you a font with style name in local language, which is correctly applied for Qt applications, but not for GTK ones.

Hm, if it's less a bug and more a fundamental incompatibility between what Qt provides and what GTK expects, I guess we don't need a bug report.

Ping! Does it work for everybody? For me everything is OK.

ngraham accepted this revision.Sun, Mar 8, 11:18 PM

Works for me and seems sane.

This revision is now accepted and ready to land.Sun, Mar 8, 11:18 PM
gikari edited the summary of this revision. (Show Details)Tue, Mar 10, 4:25 PM
ervin accepted this revision.Mon, Mar 16, 9:08 AM
This revision was automatically updated to reflect the committed changes.