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
