Differential D27735

[KConfigGui] Clear styleName font property for Regular font sytles
ClosedPublic
Actions

Authored by ahmadsamir on Feb 29 2020, 9:56 AM.

Details

Reviewers
dfaure
davidedmundson
cfeck
ervin
Group Reviewers
Frameworks
Commits
R237:a2774ff5b419: [KConfigGui] Clear styleName font property for Regular font sytles
Summary

If the styleName property is set for a QFont, using setBold(true) would
lead to Qt using an "emboldended"/synthetic font style instead of using
the bold style provided by the font itself (usually as a standalone font
file), the former looks ugly (IIUC, Freetype emboldens fonts as a last
resort for fonts that don't provide a bold style at all).

Accoring to upstream[1] the styleName property is useful for fonts with
fancy style names, and also it shouldn't be set if it's not needed; and
indeed using styleName with e.g. "Regular" doesn't make sense, as there
is no "Regular Bold" style AFAICS.

Checking for "Regular|Normal|Book|Roman" is based on examining the font
styles provided by the font packages available on OpenSuse Tumbleweed ATM,
(I didn't include some of the weird/non-common ones e.g. I've seen "Roma"
and "Rounded"). Some statistics about the "Regular"-like font styles from
my testing:
Regular: 2486
Normal: 66
Book: 20
Roman: 13

For more details see:
[1] https://bugreports.qt.io/browse/QTBUG-63792
https://bugs.kde.org/show_bug.cgi?id=378523

BUG: 378523

FIXED-IN: 5.68

Test Plan

All unit tests still pass.

Changing the fonts via e.g. the fonts KCM doesn't append the font sytleName,
to the relevant font config entry, if the "Regular" style or co. is used.

A simple test, look at the current dir name in the Dolphin url bar with
and without ",Regular" appended to the font= entry (assuming you're using
Noto Sans or DejaVu Sans as the styleName varies from font to font).

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Feb 29 2020, 9:56 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 29 2020, 9:56 AM
ahmadsamir requested review of this revision.Feb 29 2020, 9:56 AM
ahmadsamir added a comment.Feb 29 2020, 11:33 AM

Caveat: setting the general font to e.g. "Noto Sans" and then restarting the fonts KCM; opening the general font selection dialog, "Semi condensed" is selected instead of Regular. That looks like a bug in the static method nearestExistingFont() from plasma-desktop/kcms/fonts/fonts.cpp , I am working on porting the fonts KCM to QFontDialog, and will try to fix that there.

dfaure accepted this revision.Feb 29 2020, 12:07 PM

Wow that Qt bug report has really extensive investigation, very impressive.

The fix looks ok to me (without being an expert on the topic, it does seem to match what that bug report is saying).

This revision is now accepted and ready to land.Feb 29 2020, 12:07 PM
ahmadsamir updated this revision to Diff 76682.Feb 29 2020, 1:12 PM
ahmadsamir edited the summary of this revision. (Show Details)

Tweak commit message

ahmadsamir updated this revision to Diff 76721.Mar 1 2020, 9:02 AM
ahmadsamir edited the summary of this revision. (Show Details)

Add some relevant statistics

ahmadsamir added a comment.Mar 1 2020, 9:04 AM

To get those numbers I tested with this code:

QFontDatabase fdb;
const QStringList fontFamilies = fdb.families();
QStringList list;
int regularCnt = 0;
int normalCnt = 0;
int bookCnt = 0;
int romanCnt = 0;
for (const QString &family : fontFamilies) {
    const QStringList styles = fdb.styles(family);
    for (const QString &s : styles) {
        if (s == QLatin1String("Regular")) {
            ++regularCnt;
        } else if (s == QLatin1String("Normal")) {
            ++normalCnt;
        } else if (s == QLatin1String("Book")) {
            ++bookCnt;
        } else if (s == QLatin1String("Roman")) {
            ++romanCnt;
        }
    }
}

qDebug() << "Regular: " << regularCnt;
qDebug() << "Normal: " << normalCnt;
qDebug() << "Book: " << bookCnt;
qDebug() << "Roman: " << romanCnt;
Closed by commit R237:a2774ff5b419: [KConfigGui] Clear styleName font property for Regular font sytles (authored by ahmadsamir). · Explain WhyMar 2 2020, 10:51 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/gui/kconfiggroupgui.cpp (19 lines)

Diff 76759

View Options

src/gui/kconfiggroupgui.cpp

Loading...