add preview images to fonts kcm
ClosedPublic

Authored by progwolff on Mar 5 2018, 5:24 PM.

Details

Summary

The fonts kcm offers different font rendering settings. Currently, one needs to apply the settings and
reopen the application to see the changes.
This patch adds a way to render fonts at different settings (mostly based on existing code in kfontinst) and
adds preview images to the sub-pixel and hinting comboboxes.

This is part of a planned redesign of the fonts kcm. See the discussion in https://phabricator.kde.org/T7927

Test Plan

Open the fonts kcm, click on the sub-pixel combobox. The preview images should look different.

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Plasma. · View Herald TranscriptMar 5 2018, 5:24 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
progwolff requested review of this revision.Mar 5 2018, 5:24 PM
progwolff updated this revision to Diff 28735.Mar 5 2018, 5:28 PM
  • cleanup
  • add license
progwolff edited the summary of this revision. (Show Details)Mar 5 2018, 5:31 PM
progwolff edited the summary of this revision. (Show Details)
progwolff edited the test plan for this revision. (Show Details)
broulik added a subscriber: broulik.Mar 5 2018, 5:43 PM

Thanks a lot for your patch! This makes the effect of those settings a lot easier to understand. I have a couple of nitpicks below.

kcms/fonts/package/contents/ui/main.qml
22

Is 2.2 also sufficient (Qt 5.9)?

143

The popup width is too small, the text cut off. Either make it larger or at least elide the text on the right

147

Use QtQuick Controls Label instead of plain Text for proper rendering and text color

153

This is the default, no need to explicitly specify

154

Also not needed

155

Coding style, add spaces between:

source: "image://preview/" + model.index + "_" + kcm.fontAASettings.hintCurrentIndex + ".png"
kcms/fonts/previewimageprovider.cpp
36

avoid splitting twice, I suggest storing

const auto sections = id.splitRef(QLatin1Char('_'));

Also do a bounds check

67

Your image does not take into account devicePixelRatio which makes it blurred on my high dpi screen.

I'm not sure how to exactly fix that, perhaps @2x works for the image provider, or you can manually implement this, to test run

QT_SCREEN_SCALE_FACTORS=2 kcmshell5 kcm_fonts
kcms/fonts/previewimageprovider.h
2

Include guard typically goes below copyright, you can also use #pragma once in plasma-desktop

abetts added a subscriber: abetts.Mar 5 2018, 5:47 PM

Thank you for this. This is what I thought was best. It is a method that many applications use. It is straightforward and quick. Is there a way that the list width can be dynamic so as to accommodate the name of the font family? Sometimes font families are very long. Also, I would suggest one of two ways to show the list:

Method 1:


Font Family Name
Sample Text
Sample numbers


Method 2:


FONT FAMILY NAME (Shows looks on the font family name label)


progwolff updated this revision to Diff 28737.Mar 5 2018, 6:13 PM
progwolff marked 7 inline comments as done.
progwolff edited the summary of this revision. (Show Details)
  • revert QtQuick.Controls version
  • coding style, unneccessary defaults, Label instead of Text
  • split only once, add range check
  • move include guards

Thanks for your feedback!

This patch was ment to provide the functionality we need for the UI redesign, but you are probably right that I should fix the mentioned problems before we land this.
I will see what I can do about the popup width, the preview text and high dpi screens tomorrow.

kcms/fonts/package/contents/ui/main.qml
22

Seems like even 2.0 is okay.
I misinterpreted the docs where it says import QtQuick.Controls 2.3.

abetts added a comment.Mar 5 2018, 6:22 PM

Thanks so much for your work! As I am not a developer, as you progress with the patch, can you also please post screenshots? Just so I get where your work is going.

progwolff updated this revision to Diff 28740.Mar 5 2018, 6:31 PM
  • fix face number
progwolff updated this revision to Diff 28767.Mar 5 2018, 9:17 PM
  • fix preview scale on high dpi screens
davidedmundson requested changes to this revision.Mar 5 2018, 10:07 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
kcms/fonts/package/contents/ui/main.qml
187

We dont' need these transforms.
There shouldn't be any high DPI specicfic code in QML.

kcms/fonts/previewrenderengine.cpp
137

change to

QImage image(draw(name, style,...));
image.setDevicePixelRatio(ratio);
return image;

This keeps the metadata of the image scaling with the image, and then the Image item knows automatically what the logical of this image is.

This revision now requires changes to proceed.Mar 5 2018, 10:07 PM
progwolff added inline comments.Mar 6 2018, 8:10 AM
kcms/fonts/previewrenderengine.cpp
137

Thanks, this would be much cleaner. Sadly it doesn't seem to work.
I tried this before. Also tried setDotsPerMeterX.

The image is drawn too big with QT_SCREEN_SCALE_FACTORS=2:

progwolff added inline comments.Mar 6 2018, 8:17 AM
kcms/fonts/previewrenderengine.cpp
137

It's a bug in Qt: https://bugreports.qt.io/browse/QTBUG-38127

Workaround: set the sourceSize to something.

progwolff updated this revision to Diff 28792.Mar 6 2018, 8:20 AM
  • revert qml transforms, workaround QTBUG-38127
progwolff marked 3 inline comments as done.Mar 6 2018, 8:21 AM

Is there a way that the list width can be dynamic so as to accommodate the name of the font family?

I would prefer not to include the font family name in the combobox at all.
Font family names might be quite short and contain only a subset of characters, so that the differences between the renderings might turn out too slight to notice.

Also, I would suggest one of two ways to show the list:

I would prefer not to have multiple lines of text in the combobox, as single lines are easier to compare against each other.

What do you think?

progwolff updated this revision to Diff 28802.Mar 6 2018, 10:27 AM
  • adjust popup width to fit contents
progwolff marked an inline comment as done.Mar 6 2018, 10:29 AM
progwolff updated this revision to Diff 28804.Mar 6 2018, 10:54 AM
  • never show scrollbars for the combobox popup (force pixel aligment)
  • prepare for multi-line preview images, resize images to pixel-align on high dpi displays

Out of those three, my vote goes to #2. The versions with numbers and symbols look really messy.

progwolff updated this revision to Diff 28817.Mar 6 2018, 12:08 PM
  • fix parsing hinting index
abetts added a comment.Mar 6 2018, 3:05 PM

Number 2 seems like the best option. I would also make sure that there is a color difference between title label and sample text. That way it is easier to distinguish between list items. When all labels are the same color, you give equal importance to all elements.

@abetts @ngraham
Do you count from 0 or 1?

Number 2 is single line The quick brown fox...?

abetts added a comment.Mar 6 2018, 3:23 PM

I like this one! :D above

Also FYI @progwolff your screenshots are messed up because if a bug in Spectacle that was recently fixed. Until you get that fix, you can use Active Window mode instead of Window Under Cursor mode.

How about this?

abetts added a comment.Mar 6 2018, 3:56 PM

That seems pretty good to me. I would even make the title label bigger, so that it looks like a header.

Also, can the list be longer? At least, double the size? People tend to have a lot of fonts installed. We install a lot of them by default. This can help the user click and scroll less when looking for the right font. I have seen other OSs where they literally fill the screen with the list just to help you find what you need faster.

The list will always have the same 6 entries. This is not about choosing a font, but about choosing details about how a font is rendered.

abetts added a comment.Mar 6 2018, 4:03 PM
This comment was removed by abetts.

We don't have a font name here. The choices are always "Vendor default", "None", "RGB", "BGR", "vertical RGB", "vertical BGR".
I am not sure what a number would mean here?

progwolff updated this revision to Diff 28844.Mar 6 2018, 4:08 PM
  • Merge branch 'master' of git://anongit.kde.org/plasma-desktop
  • adjust label opacity and size
abetts added a comment.Mar 6 2018, 4:10 PM

We don't have a font name here. The choices are always "Vendor default", "None", "RGB", "BGR", "vertical RGB", "vertical BGR".
I am not sure what a number would mean here?

Forget what I said. I keep getting confused.

Seems like VDG is happy then :D
Many thanks to both of you!

@davidedmundson Any objections left?

progwolff edited the test plan for this revision. (Show Details)Mar 6 2018, 4:15 PM
abetts added a comment.Mar 6 2018, 4:16 PM

Seems like VDG is happy then :D
Many thanks to both of you!

@davidedmundson Any objections left?

I don't have any objections. I would say, however, keep an open mind to feedback. If this lands, people may react differently to the work than what we think here. Interacting with your work everyday is different. Be ready in case changes are needed.

Seems like VDG is happy then :D
Many thanks to both of you!

@davidedmundson Any objections left?

I don't have any objections. I would say, however, keep an open mind to feedback. If this lands, people may react differently to the work than what we think here. Interacting with your work everyday is different. Be ready in case changes are needed.

Of course!
It's probably the mixture of different ideas and tastes that come together what makes open source software so great.
And as stated in the summary, we will continue the discussion in https://phabricator.kde.org/T7927.

I'm not thrilled with the really light 50% opacity labels, to be honest.

I'm not thrilled with the really light 50% opacity labels, to be honest.

What do you propose? I am not much of a designer...

abetts added a comment.Mar 6 2018, 4:38 PM

I would use this as a guide

https://community.kde.org/KDE_Visual_Design_Group/HIG/Color

And I would select Icon Grey for the header and Shade Black for the text below.

I thought it was okay before. It's very unusual and unnerving to have a header be de-emphasized compared to its content. My vote goes to using the standard colors from the active theme without messing with them.

My vote goes to using the standard colors from the active theme without messing with them.

At least we should not hardcode colors. So it's either an opacity of about 0.8 to roughly match Icon Grey in the default palette, or just standard colors.

The difference between 0.8 opacity and 1.0 is so subtle that there really isn't a reason to do it IMHO. At that point you're just slightly impairing readability with no benefit. See also D10899 and D10902.

...All of which makes me think that this should use a Heading rather than a plain old QQC2 Label. That way it's automatically consistent with other Headings, and will inherit any future style changes automatically.

progwolff updated this revision to Diff 28856.Mar 6 2018, 6:04 PM
  • use Kirigami heading instead of QQuickControls2 Label

Sounds reasonable.

Since this is a patch for plasma-desktop, perhaps it would be more appropriate to use the Heading from PlasmaComponents rather than the one from Kirigami?

Since this is a patch for plasma-desktop, perhaps it would be more appropriate to use the Heading from PlasmaComponents rather than the one from Kirigami?

We already have Kirigami.FormLayout and Kirigami.Separator. PlasmaComponents would need an additional import.

ngraham accepted this revision.Mar 6 2018, 6:49 PM

OK, I won't block on that, but do let a Plasma developer weigh in on the subject.

harmathy accepted this revision.Mar 8 2018, 4:53 PM
progwolff marked 3 inline comments as done.Mar 10 2018, 12:16 PM
mart added inline comments.Mar 23 2018, 2:28 PM
kcms/fonts/previewrenderengine.cpp
137

can you put the workaround with a giant comment that points to the qt bug?

progwolff updated this revision to Diff 30325.Mar 23 2018, 3:09 PM
  • giant comments
mart accepted this revision.Apr 3 2018, 11:22 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2018, 3:36 PM
This revision was automatically updated to reflect the committed changes.