D28922: Port Sensor Face loading from plasmoid

[Arjen Hiemstra]

ahiemstra added inline comments.

INLINE COMMENTS

> ConfigAppearance.qml:90
> +    }
> +    RowLayout {
> +        Kirigami.FormData.label: i18n("Presets:")

It would probably be nice if we can make this a GridLayout with flow bound to the FormLayout's wide setting. Right now, if the layout becomes less wide, these buttons will be pushed out of bounds.

> ConfigAppearance.qml:127
> +
> +    RowLayout {
> +        Kirigami.FormData.label: i18n("Display Style:")

As above, these also have a tendency to get pushed out of bounds.

> SensorFaceController.cpp:43
> +
> +    auto list = KPackage::PackageLoader::self()->listPackages(QStringLiteral("Plasma/SensorApplet"));
> +    // NOTE: This will diable completely the internal in-memory cache 

"Plasma/SensorApplet" should probably be changed to something non-plasma?

> SensorFaceController.cpp:77
> +    clear();
> +    QList<KPluginMetaData> plugins = KPackage::PackageLoader::self()->findPackages(QStringLiteral("Plasma/Applet"), QString(), [](const KPluginMetaData &plugin) {
> +        return plugin.value(QStringLiteral("X-Plasma-RootPath")) == QStringLiteral("org.kde.plasma.systemmonitor");

Similar to above, does it still make sense to use "Plasma/Applet" here?

> SensorFaceController.cpp:182
> +    //TODO: add i18n context object
> +    QQmlContext *context = new QQmlContext(engine);
> +    QObject *guiObject = component->beginCreate(context);

This seems to be unused?

> SensorFaceController.cpp:247
> +
> +    setFaceId(d->appearanceGroup.readEntry("chartFace", QStringLiteral("org.kde.ksysguard.piechart")));
> +}

I think we should have a "default face" for when no specfic face has been set yet, that isn't a pie chart. At least for ksysguard the default pie chart feels weird.

> SensorFaceController.cpp:415
> +
> +    d->faceMetadata = new KDesktopFile(d->facePackage.path() + QStringLiteral("metadata.desktop"));
> +

Can we define this in the package and then use filePath("metadata") or so?

> SensorFaceController.cpp:476
> +    d->faceConfigUi = d->createConfigUi(QStringLiteral(":/FaceDetailsConfig.qml"),
> +    {{QStringLiteral("controller"), QVariant::fromValue(this)},
> +         {QStringLiteral("source"), d->facePackage.filePath("ui", QStringLiteral("Config.qml"))}});

Code style is a bit messed up here. Maybe run clang-format over the entire thing?

> SensorFaceController.h:47
> +    Q_PROPERTY(QString faceId READ faceId WRITE setFaceId NOTIFY faceIdChanged)
> +    Q_PROPERTY(QString totalSensor READ totalSensor WRITE setTotalSensor NOTIFY totalSensorChanged)
> +    Q_PROPERTY(QStringList sensorIds READ sensorIds WRITE setSensorIds NOTIFY sensorIdsChanged)

I think it would be a good idea to make this a list of ids as well. My current use case would be the "used/total" version of the pie chart that is used in KSysGuardQML, which would need two "total" sensors.

> SensorFaceController.h:50
> +    Q_PROPERTY(QStringList sensorColors READ sensorColors WRITE setSensorColors NOTIFY sensorColorsChanged)
> +    Q_PROPERTY(QStringList textOnlySensorIds READ textOnlySensorIds WRITE setTextOnlySensorIds NOTIFY textOnlySensorIdsChanged)
> +

Hmm, maybe these names are a little too use-case specific? I wonder if it would make sense to name them "highPrioritySensorsIds", "sensorIds", "lowPrioritySensorIds" or so.

> SensorFaceController.h:76
> +
> +    //TODO: just QQuickItem
> +    QQuickItem *fullRepresentation();

This seems superfluous?

> BarChart.qml:38
> +
> +    stacked: plasmoid.nativeInterface.faceConfiguration.barChartStacked
> +

This should use the controller.

> Config.qml:50
> +    }
> +    Controls.SpinBox {
> +        id: rangeFromSpin

All these are missing `editable: true` making it rather annoying to edit.

Also, we probably should set `from: -99999999` and `to: 99999999` or so on all of them, the minimum and maximum range is rather undefined.

> FullRepresentation.qml:58
> +
> +    Faces.ExtendedLegend {
> +        Layout.fillWidth: true

This import seems to be missing?

REPOSITORY
  R111 KSysguard Library

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

To: mart, #plasma, ahiemstra
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200501/a9478e92/attachment-0001.html>