Allow to change toolbar font separately again
ClosedPublic

Authored by rkflx on Jul 14 2017, 7:22 AM.

Details

Summary

The fonts KCM supports changing the toolbar font, but with the port to
QPA/Plasma 5 in aa16577f this stopped working and toolbars just used the
general font. This is fixed by setting QPlatformTheme's ToolButtonFont
to the toolbar font chosen. However, there are some caveats:

Default toolbar font size is 9, while the general font size is 10.
This would result in a sudden shrink in toolbar widths everywhere (or
worse for weird leftover settings users might have tried unsuccessfully).

In some places QToolButtons are used outside of toolbars, sometimes
right next to QPushButtons. As due to the bug both had the same font,
this might now lead to unexpected differences.

Therefore, to keep visual consistency with past Plasma 5 releases, let's
change the default toolbar font size to 10 and provide a kconf_update
script to copy the general font to the toolbar font once.

BUG: 358254
FIXED-IN: 5.11.0

Test Plan

make install, run kconf_update: toolBarFont in kdeglobals gets updated
immediately

Change "Toolbar" font in "kcmshell5 fonts", start your favorite KDE
and Qt-only applications: toolbar font and/or font size changed
accordingly

Diff Detail

Repository
R135 Integration for Qt applications in Plasma
Branch
Plasma/5.10
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx created this revision.Jul 14 2017, 7:22 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 14 2017, 7:22 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rkflx edited the summary of this revision. (Show Details)Jul 14 2017, 7:26 AM
rkflx edited the test plan for this revision. (Show Details)
rkflx added a comment.Jul 14 2017, 7:30 AM

Some screenshots of applications with QToolButtons and other elements looking out of place with a tiny toolbar font size:

Toolviews in KDevelop (bad) vs. Kate (good)
Marble ("Clear Track" button)
Spectacle (buttons at the bottom)
Okular ("Zoom" combobox)
Gwenview (usage of general vs. small vs. toolbar font (Courier used only for illustration purposes) )

Screenshots for Plasma 5 with toolbar font sizes 8pt, 9pt (former broken default), 10pt (new default, same as menu bar):

For reference, here is how it looked in KDE4 with toolbar font sizes 7pt, 8pt (default), 9pt (same as menu bar) (please ignore font sizes being larger for Plasma 5 in general as introduced in 67d27e23):

Personally, I like the smaller toolbar font size, as this allows for more buttons horizontally.

@jensreuterberg: What would be the best place to start a discussion about a new default size? (Forum, Phabricator task, Phabricator Differential?)

rkflx planned changes to this revision.Aug 19 2017, 1:29 PM

No review in over a month. On the positive side, I got more ideas in the meantime.

Let's see if I can find another way to change the toolbar font without affecting single QToolButtons. Things to try (maybe even both, depending on release schedules):

  • add ToolbarFont in Qt (similar additions done before, since no BC promised for QPA)
  • set ToolbarFont via QApplication::setFont(toolbarFont, "QToolBar"); (inspired by 26cb58c1) (works perfectly via main.cpp, not sure how to do it via platformtheme yet)

Meanwhile, if there are any other comments (especially regarding what new default to pick), please share ;)

davidedmundson accepted this revision.Aug 19 2017, 2:55 PM
davidedmundson added a subscriber: davidedmundson.

Hey, sorry about the lack of review.

I think the default being the current default makes the most sense.
Patch absolutely makes sense. I like how you even added an update script.

Lets merge this as-is, and then do your other changes afterwards

Do you have commit access?

rkflx requested review of this revision.Aug 19 2017, 3:25 PM
rkflx added a subscriber: fvogt.

No worries and thanks for the review :)

I'll setup my recently granted commit access and commit to master (even though originally intended for 5.10, but I'll need to rebase on Fabians changes in 1b21b597 anyway).

If you (or @fvogt) have the time, could you also look at D6698? Should be trivial…

Things to work on in the future:

  • restrict toolbar font size to only affect QToolbars
  • think about going to different default size
This revision is now accepted and ready to land.Aug 19 2017, 3:25 PM
fvogt added a comment.Aug 19 2017, 3:56 PM

Looks good to me as well (+ the other patch), I just added two comments.

src/platformtheme/fonts_global_toolbar.pl
12

Not sure, but maybe the script should only read/change the [General] section?

14

Is it guaranteed that font= comes before toolBarFont?

rkflx marked 2 inline comments as done.Aug 20 2017, 10:39 AM
rkflx added inline comments.
src/platformtheme/fonts_global_toolbar.pl
12

Not done in the other update scripts either and not strictly necessary, but you are right. I'll fix it in the upd file by adding the section there, no additional perl needed.

14

Good catch, but can only happen for a manually edited config file. Instead of adding an ugly second loop, it turns out upd syntax is powerful enough so I can omit the perl script entirely (only needed if we'd want something like toolBarFont[size]=generalFont[size]-1), it even sorts the config file alphabetically.

rkflx updated this revision to Diff 18431.Aug 20 2017, 10:40 AM
rkflx marked 2 inline comments as done.
  • add FIXED-IN
  • rebase on Fabian's changes
  • take care of Fabian's comments (extend upd, remove pl)
rkflx edited the summary of this revision. (Show Details)Aug 20 2017, 10:50 AM
rkflx edited the test plan for this revision. (Show Details)
fvogt accepted this revision.Aug 20 2017, 10:55 AM

Latest changes look good to me as well.

This revision was automatically updated to reflect the committed changes.