[Fonts KCM] Remove redundant nearestExistingFont()
ClosedPublic

Authored by ahmadsamir on Apr 24 2020, 3:30 PM.

Details

Summary

It seems that nearestExistingFont() gets the same result as
fc-match font-name, which is basically the sans serif default font
on the system; this seems redundant as KFontChooser will fallback to
selecting the first font family in the list if the initial font it
was called with doesn't exist _and_ KFontChooser always puts "Sans Serif",
"Serif" and "Monospace" at the top of the list.

Removing nearestExistingFont() solves the issue in bug 420287 as
the font it returns will have the styleName property set, after we
went to so much trouble to clear that property so that setBold() can
work.

CCBUG: 420287

Test Plan

Before the patch:

  • In kdeglobals [General] set: fixed=Blah Mono,12,-1,5,50,0,0,0,0,0 toolBarFont=Bogus Serif,12,-1,5,50,0,0,0,0,0
  • Load the fonts KCM, and open the font dialog for Fixed and Toolbar, in both cases the default "sans serif" font on the system is selected, in my case it's "DejaVu Sans"

Apply the patch and repeat, the "Sans Serif" entry is selected, which is
an alias to "DejaVu Sans" on my system.

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.
ahmadsamir created this revision.Apr 24 2020, 3:30 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 24 2020, 3:30 PM
ahmadsamir requested review of this revision.Apr 24 2020, 3:30 PM
ahmadsamir added a reviewer: ngraham.EditedApr 24 2020, 3:32 PM

If this diff is a no-go, it'll have to be D27785.

ahmadsamir updated this revision to Diff 81110.Apr 24 2020, 3:34 PM
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir removed a reviewer: ngraham.

Tweak commit message

ngraham accepted this revision.Apr 24 2020, 4:04 PM
ngraham added a subscriber: ngraham.

Fixes the bug for me and seems like the right solution.

This revision is now accepted and ready to land.Apr 24 2020, 4:04 PM
bport added a comment.May 4 2020, 3:43 PM

I don't think we will have same behavior, we don't only check name but also size, type...
If we are ok to fallback in all case to the same font that can work.

From your test plan, something look strange you don't end with a monospaced font for fixed font as fallback ?

I don't think we will have same behavior, we don't only check name but also size, type...
If we are ok to fallback in all case to the same font that can work.

From your test plan, something look strange you don't end with a monospaced font for fixed font as fallback ?

What happens currently is it selects "Sans Serif".

bport accepted this revision.May 5 2020, 12:34 PM

ok then ship it

This revision was automatically updated to reflect the committed changes.