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
