D11064: add preview images to fonts kcm

[David Edmundson]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R119:1793b51ed8b9: add preview images to fonts kcm (authored 
by progwolff, committed by davidedmundson).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=30325=32817

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson, ngraham
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 30325.
progwolff added a comment.


  - giant comments

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28856=30325

BRANCH
  arcpatch-D11064

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson, ngraham
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> progwolff wrote in previewrenderengine.cpp:136
> It's a bug in Qt: https://bugreports.qt.io/browse/QTBUG-38127
> 
> Workaround: set the sourceSize to something.

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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson, ngraham
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  @davidedmundson ping?

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson, ngraham
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson, ngraham
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  In D11064#220180 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  Sounds reasonable.
  
  F5743624: Screenshot_20180306_190613.png 


REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28856.
progwolff added a comment.


  - use Kirigami heading instead of QQuickControls2 Label

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28844=28856

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  In D11064#220134 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  In D11064#220131 , @ngraham wrote:
  
  > 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...

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  In D11064#220120 , @abetts wrote:
  
  > In D11064#220118 , @progwolff 
wrote:
  >
  > > 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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  In D11064#220118 , @progwolff 
wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff edited the test plan for this revision.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  Seems like VDG is happy then :D  
  Many thanks to both of you!
  
  @davidedmundson Any objections left?

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  In D11064#220102 , @progwolff 
wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  F5743545: Screenshot_20180306_170938.png 


REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28844.
progwolff added a comment.


  - Merge branch 'master' of git://anongit.kde.org/plasma-desktop
  - adjust label opacity and size

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28817=28844

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  You are right! I was confused. Looks great then! Would having a font number 
on the side help?
  
  Like this
  
  12  **Font Name**
  
A quick brown fox jumps over the rope

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  How about this?
  
  F5743529: Screenshot_20180306_164925.png 


REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  > F5743165: Screenshot_20180306_101833.png 

  
  I like this one! :D above

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


  Here we are counting like humans, not programmers. :) We like this one: 
https://phabricator.kde.org/file/data/s2ekczgv5q5xxqwofahp/PHID-FILE-7wh4qv3a3lx6xe7f5khm/Screenshot_20180306_101833.png

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  @abetts @ngraham 
  Do you count from 0 or 1?
  
  Number 2 is single line `The quick brown fox...`?

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28817.
progwolff added a comment.


  - fix parsing hinting index

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28811=28817

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: ngraham, davidedmundson, abetts, broulik, plasma-devel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28811.
progwolff added a comment.


  - style

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28804=28811

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28804.
progwolff added a comment.


  - 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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28802=28804

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff marked an inline comment as done.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28802.
progwolff added a comment.


  - adjust popup width to fit contents

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28792=28802

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  In D11064#219273 , @abetts wrote:
  
  > 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?
  
  F5743166: Screenshot_20180306_110024.png 

  
  F5743165: Screenshot_20180306_101833.png 

  
  F5743164: Screenshot_20180306_110122.png 


REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28792.
progwolff added a comment.


  - revert qml transforms, workaround QTBUG-38127

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28767=28792

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added inline comments.

INLINE COMMENTS

> davidedmundson wrote in previewrenderengine.cpp:136
> 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.

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`:
F5743060: Screenshot_20180306_090622.png 

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff marked 3 inline comments as done.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added inline comments.

INLINE COMMENTS

> progwolff wrote in previewrenderengine.cpp:136
> 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`:
> F5743060: Screenshot_20180306_090622.png 
> 

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

Workaround: set the sourceSize to something.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> main.qml:187
> +transform: Scale {
> +xScale: 1. / QtWindow.Screen.devicePixelRatio;
> +yScale: 1. / QtWindow.Screen.devicePixelRatio;

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

> previewrenderengine.cpp:136
> +
> +return draw(name, style, faceNo, txt, bgnd, fSize, text);
> +}

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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart, davidedmundson
Cc: davidedmundson, abetts, broulik, plasma-devel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28767.
progwolff added a comment.


  - fix preview scale on high dpi screens

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28740=28767

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28740.
progwolff added a comment.


  - fix face number

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28737=28740

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff added a comment.


  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.

INLINE COMMENTS

> broulik wrote in main.qml:22
> Is `2.2` also sufficient (Qt 5.9)?

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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28737.
progwolff marked 7 inline comments as done.
progwolff edited the summary of this revision.
progwolff added a comment.


  - revert QtQuick.Controls version
  - coding style, unneccessary defaults, Label instead of Text
  - split only once, add range check
  - move include guards

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28735=28737

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Kai Uwe Broulik]

broulik added a comment.


  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.

INLINE COMMENTS

> main.qml:22
>  import QtQuick.Layouts 1.1
> -import QtQuick.Controls 2.0 as QtControls
> +import QtQuick.Controls 2.3 as QtControls
>  import QtQuick.Dialogs 1.2 as QtDialogs

Is `2.2` also sufficient (Qt 5.9)?

> main.qml:143
> +id: subPixelDelegate
> +width: subPixelComboImage.implicitWidth
> +contentItem: ColumnLayout {

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

> main.qml:147
> +width: subPixelComboImage.implicitWidth
> +Text {
> +id: subPixelComboText

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

> main.qml:153
> +id: subPixelComboImage
> +fillMode: Image.Pad
> +sourceSize: undefined

This is the default, no need to explicitly specify

> main.qml:154
> +fillMode: Image.Pad
> +sourceSize: undefined
> +source: 
> "image://preview/"+model.index+"_"+kcm.fontAASettings.hintingCurrentIndex+".png"

Also not needed

> main.qml:155
> +sourceSize: undefined
> +source: 
> "image://preview/"+model.index+"_"+kcm.fontAASettings.hintingCurrentIndex+".png"
> +}

Coding style, add spaces between:

  source: "image://preview/" + model.index + "_" + 
kcm.fontAASettings.hintCurrentIndex + ".png"

> previewimageprovider.cpp:35
> +{
> +int subPixelIndex = id.split('_')[0].toInt();
> +int hintingIndex = id.split('_')[1].toInt();

avoid splitting twice, I suggest storing

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

Also do a bounds check

> previewimageprovider.cpp:66
> +PreviewRenderEngine eng(true);
> +QImage img = eng.drawAutoSize(m_font, text, bgnd, 
> eng.getDefaultPreviewString());
> +

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

> previewimageprovider.h:1
> +#ifndef __PREVIEW_IMAGE_PROVIDER_H__
> +#define __PREVIEW_IMAGE_PROVIDER_H__

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

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart
Cc: broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D11064: add preview images to fonts kcm

[Andres Betts]

abetts added a comment.


  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)
  
  _

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart
Cc: abetts, broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff updated this revision to Diff 28735.
progwolff added a comment.


  - cleanup
  - add license

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11064?vs=28734=28735

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff edited the summary of this revision.
progwolff edited the test plan for this revision.

REPOSITORY
  R119 Plasma Desktop

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

To: progwolff, #plasma, harmathy, mart
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11064: add preview images to fonts kcm

[Julian Wolff]

progwolff created this revision.
progwolff added reviewers: Plasma, harmathy, mart.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
progwolff requested review of this revision.

REVISION 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.

REPOSITORY
  R119 Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  kcms/CMakeLists.txt
  kcms/fonts/CMakeLists.txt
  kcms/fonts/fonts.cpp
  kcms/fonts/package/contents/ui/main.qml
  kcms/fonts/previewimageprovider.cpp
  kcms/fonts/previewimageprovider.h
  kcms/fonts/previewrenderengine.cpp
  kcms/fonts/previewrenderengine.h
  kcms/kfontinst/lib/FcEngine.cpp
  kcms/kfontinst/lib/FcEngine.h

To: progwolff, #plasma, harmathy, mart
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart