[Fonts KCM] Change how nearestExistingFonts() finds a matching font
AbandonedPublic

Authored by ahmadsamir on Mar 2 2020, 3:38 PM.

Details

Reviewers
davidedmundson
broulik
ervin
meven
bport
Group Reviewers
Plasma
Summary

A "Regular" font will have an empty styleName e.g. "DejaVu Sans,12,-1,5,50,0,0,0,0,0"
so that setBold(true) can work; we deliberately clear the styleName for
"Regular"-like font styles when saving via KConfig, see: writeEntryGui()
from kconfiggroupgui.cpp and https://phabricator.kde.org/D27735 for more
gory details.

Change nearestExistingFont() to return the same font it's been called on
if the search result gives us the same font (same weight, same style), and
interpret the case when the styleName is empty to mean it's using a
"Regular"-like style if the font weight is QFont::Normal and the styleName
is "Regular|Normal|Book|Roman".

Due to the change to clear font styleName property when saving via
KConfig (D27735), if in kdegloblas you removed the ",Regular|Normal|Book|Roman"
from the end of the *font*= entries, openin the font selection dialog
in the KCM, the very first style is selected, not "Regular" and co. as
it should be. This will be addressed in an upcoming diff.

Test Plan
  • Remove ",Regular|Normal|Book|Roman" from *font* entries in kdeglobals
  • Open the fonts kcm, notice that the Apply button is enabled even when you haven't changed anything
  • Apply the diff then try again, the Apply button should be disabled until you actually change something

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D27785 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23582
Build 23600: arc lint + arc unit
ahmadsamir created this revision.Mar 2 2020, 3:38 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 2 2020, 3:38 PM
ahmadsamir requested review of this revision.Mar 2 2020, 3:38 PM
ahmadsamir updated this revision to Diff 76779.Mar 2 2020, 3:49 PM
ahmadsamir edited the summary of this revision. (Show Details)

Remove setNeedsSave(false) from load(), doesn't seem to be needed any more

TODO: fix the font dialog to select the correct "Regular"-like style (I have a working patch but needs more work).

FTR, removing setNeedsSave(false) from load(), it seems to work sometimes, and then some other times it doesn't work (i.e. changing anything, Apply doesn't get enabled)...

bport requested changes to this revision.Mar 9 2020, 9:43 AM
bport added inline comments.
kcms/fonts/fonts.cpp
117

You can use && to have only one if there

169

Those change will compare font twice, here and on setters so I will keep old code there

192

I will keep that until we have a proper tested fix for https://phabricator.kde.org/D27452
can prevent bug and ensure apply button is on the good state on all case

This revision now requires changes to proceed.Mar 9 2020, 9:43 AM
ahmadsamir updated this revision to Diff 77258.Mar 9 2020, 10:04 AM
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir removed a reviewer: bport.

More detailed commit message

ahmadsamir edited the summary of this revision. (Show Details)Mar 9 2020, 10:11 AM
ahmadsamir marked 3 inline comments as done.Mar 9 2020, 10:43 AM
ahmadsamir added inline comments.
kcms/fonts/fonts.cpp
117

OK.

169

Indeed; looking at the code generated by kconfig compiler in <builddir>/kcms/fonts/fontsettings.h:

/**
  Set General font
*/
void setFont( const QFont & v )
{
  if (v != mFont && !isFontImmutable()) {
    mFont = v;
    Q_EMIT fontChanged();
  }
}

I will revert. And another point is that the code in fonts.cpp doesn't need to check for immutability since it's checked by the setters.

192

OK, fair point, I will revert that bit.

ahmadsamir updated this revision to Diff 77264.Mar 9 2020, 12:00 PM
ahmadsamir marked 3 inline comments as done.
ahmadsamir retitled this revision from [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary to [Fonts KCM] Change how nearestExistingFonts() finds a matching font.
ahmadsamir edited the summary of this revision. (Show Details)

Address comments

ahmadsamir updated this revision to Diff 77265.Mar 9 2020, 12:02 PM
ahmadsamir edited the summary of this revision. (Show Details)

Tweak commit message

ahmadsamir updated this revision to Diff 77268.Mar 9 2020, 12:07 PM
ahmadsamir retitled this revision from [Fonts KCM] Change how nearestExistingFonts() finds a matching font to [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary.
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir added a subscriber: bport.

Tweak commit message

ahmadsamir updated this revision to Diff 77442.Mar 11 2020, 5:28 PM
ahmadsamir retitled this revision from [Fonts KCM] Change setNearestExistingFonts() to set the fonts only when necessary to [Fonts KCM] Change how nearestExistingFonts() finds a matching font.
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir removed a subscriber: bport.

Rebase
Actually address comments

bport accepted this revision.Mar 16 2020, 9:30 AM

Ok for me but need to wait approval on font dialog review (to ensure correct font style is selected when editing)

This revision is now accepted and ready to land.Mar 16 2020, 9:30 AM

Ok for me but need to wait approval on font dialog review (to ensure correct font style is selected when editing)

Thanks.

The font dialog review, D27808, took a different turn..., see https://phabricator.kde.org/D27808#625255 for details.

ahmadsamir abandoned this revision.Apr 1 2020, 2:01 PM

Not needed with D27808, which will use KFontChooserDialog everywhere.