Fix fonts KCM button state
ClosedPublic

Authored by bport on Wed, Jan 22, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Wed, Jan 22, 3:10 PM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Jan 22, 3:10 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Wed, Jan 22, 3:10 PM
ngraham added inline comments.
kcms/fonts/fontssettings.kcfg
77
ervin added inline comments.Wed, Jan 22, 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.Wed, Jan 22, 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.Thu, Jan 23, 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.Wed, Jan 29, 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.Wed, Jan 29, 1:45 PM
kcms/fonts/fonts.cpp
473

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

bport added inline comments.Wed, Jan 29, 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.Wed, Jan 29, 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.Mon, Feb 3, 3:05 PM
This revision is now accepted and ready to land.Mon, Feb 3, 3:05 PM
meven accepted this revision.Mon, Feb 3, 3:34 PM
This revision was automatically updated to reflect the committed changes.
fvogt reopened this revision.Tue, Feb 4, 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.Tue, Feb 4, 9:33 AM
bport added a comment.Tue, Feb 4, 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.Tue, Feb 4, 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!