Fix fonts KCM button state
ClosedPublic

Authored by bport on Jan 22 2020, 3:10 PM.

Details

Summary
  • Need to set "Regular" style name font to avoid not equal font

BUG: 416358

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 21499
Build 21517: arc lint + arc unit
bport created this revision.Jan 22 2020, 3:10 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 22 2020, 3:10 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Jan 22 2020, 3:10 PM
ngraham added inline comments.
kcms/fonts/fontssettings.kcfg
77
ervin added inline comments.Jan 22 2020, 3:50 PM
kcms/fonts/fonts.cpp
472

The curly brace is misplaced

kcms/fonts/fontssettings.kcfg
77

I guess this depends on how the font get serialized... and looking into KConfig we use toString() so stylename will appear AFAICT.

meven added inline comments.Jan 22 2020, 4:38 PM
kcms/fonts/fonts.cpp
473

You can make this static.

broulik added inline comments.
kcms/fonts/fonts.cpp
479

Kinda defies the purpose of using kconfigxt if we end up hardcoding the state in code again?

ervin added inline comments.Jan 23 2020, 2:30 PM
kcms/fonts/fonts.cpp
479

That part was never transitioned to KConfigXT though since it doesn't have a KConfig backend.

Clearly inheriting by hand from KCoreConfigSkeleton should be next on the options to be evaluated (not in that patch though).

bport added inline comments.Jan 29 2020, 1:44 PM
kcms/fonts/fontssettings.kcfg
77

This is only the default value, and default value will be not bold or italic so we will not have this kind of problem AFAIK.
I also tested to set bold to my general font and it work as expected

meven added inline comments.Jan 29 2020, 1:45 PM
kcms/fonts/fonts.cpp
473

Just a nitpick, I guess you plan to change it soon.

bport added inline comments.Jan 29 2020, 1:47 PM
kcms/fonts/fonts.cpp
473

It will be changed when we migrate this part to kconfigxt

bport updated this revision to Diff 74576.Jan 29 2020, 2:02 PM

fix coding style

We are getting duplicates of https://bugs.kde.org/show_bug.cgi?id=416358, so I guess it would be nice if this made it into 5.18.

We are getting duplicates of https://bugs.kde.org/show_bug.cgi?id=416358, so I guess it would be nice if this made it into 5.18.

Ok I will land it to 5.18 and then merge to master

ngraham accepted this revision.Feb 3 2020, 3:05 PM
This revision is now accepted and ready to land.Feb 3 2020, 3:05 PM
meven accepted this revision.Feb 3 2020, 3:34 PM
This revision was automatically updated to reflect the committed changes.
fvogt reopened this revision.Feb 4 2020, 9:33 AM
fvogt added a subscriber: fvogt.

Does this mean the default fonts have a style name of "Regular" now? That will break setting bold fonts and such, see https://phabricator.kde.org/D9070.

This revision is now accepted and ready to land.Feb 4 2020, 9:33 AM
bport added a comment.Feb 4 2020, 10:11 AM

Does this mean the default fonts have a style name of "Regular" now? That will break setting bold fonts and such, see https://phabricator.kde.org/D9070.

Default is not serialized in the configuration file (because it's the default value).

I do some check with current KCM version, and when we save it, we end up with font=Noto Sans,10,-1,5,87,1,0,0,0,0,Regular
Because nearest font algorithm return the regular one. So if I well understood everything this patch will not break anything.

fvogt closed this revision.Feb 4 2020, 10:13 AM

Does this mean the default fonts have a style name of "Regular" now? That will break setting bold fonts and such, see https://phabricator.kde.org/D9070.

Default is not serialized in the configuration file (because it's the default value).

I do some check with current KCM version, and when we save it, we end up with font=Noto Sans,10,-1,5,87,1,0,0,0,0,Regular
Because nearest font algorithm return the regular one. So if I well understood everything this patch will not break anything.

Ok, sounds good. Thanks for checking!