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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180305/2a17e541/attachment-0001.html>