D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-05-05 Thread Ahmad Samir 
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:0325d6981810: [Fonts KCM] Remove redundant 
nearestExistingFont() (authored by ahmadsamir).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29155?vs=81110=81989

REVISION DETAIL
  https://phabricator.kde.org/D29155

AFFECTED FILES
  kcms/fonts/fonts.cpp

To: ahmadsamir, #plasma, bport, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-05-05 Thread Benjamin Port 
bport accepted this revision.
bport added a comment.


  ok then ship it

REPOSITORY
  R119 Plasma Desktop

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

To: ahmadsamir, #plasma, bport, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-05-05 Thread Ahmad Samir 
ahmadsamir added a comment.


  In D29155#663178 , @bport wrote:
  
  > 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".

REPOSITORY
  R119 Plasma Desktop

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

To: ahmadsamir, #plasma, bport, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-05-04 Thread Benjamin Port 
bport added a comment.


  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 ?

REPOSITORY
  R119 Plasma Desktop

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

To: ahmadsamir, #plasma, bport, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-04-24 Thread Nathaniel Graham 
ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

To: ahmadsamir, #plasma, bport, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-04-24 Thread Ahmad Samir 
ahmadsamir updated this revision to Diff 81110.
ahmadsamir edited the summary of this revision.
ahmadsamir removed a reviewer: ngraham.
ahmadsamir added a comment.


  Tweak commit message

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29155?vs=81109=81110

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

AFFECTED FILES
  kcms/fonts/fonts.cpp

To: ahmadsamir, #plasma, bport
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-04-24 Thread Ahmad Samir 
ahmadsamir added a reviewer: ngraham.
ahmadsamir added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D29155

To: ahmadsamir, #plasma, bport, ngraham
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29155: [Fonts KCM] Remove redundant nearestExistingFont()

2020-04-24 Thread Ahmad Samir 
ahmadsamir created this revision.
ahmadsamir added reviewers: Plasma, bport.
Herald added a project: Plasma.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  It seems that nearestExistingFont() gets the same result of
  `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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  l-setnearest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29155

AFFECTED FILES
  kcms/fonts/fonts.cpp

To: ahmadsamir, #plasma, bport
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart