Port Sensor Face loading from plasmoid
ClosedPublic

Authored by mart on Apr 17 2020, 3:21 PM.

Details

Summary

The plasmoid loginc has been ported in the SensorFaceController public class
it's its responsibility to instantiate the right qml files to load a face and
writes all the face config in the KConfigGroup provided by the class constructor

the sensorfacecontroller will always be accessible as a property from the face
qml, ro read and write the config paramenters

depends on D28141

Test Plan

Diff Detail

Repository
R111 KSysguard Library
Branch
mart/sensor_face
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26576
Build 26594: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
mart edited the test plan for this revision. (Show Details)Apr 17 2020, 3:22 PM
mart added a reviewer: ahiemstra.
mart edited the summary of this revision. (Show Details)Apr 20 2020, 10:41 AM
ahiemstra added inline comments.Apr 23 2020, 10:45 AM
sensors/SensorFaceController.cpp
21 ↗(On Diff #80401)

This file is missing.

mart updated this revision to Diff 80982.Apr 23 2020, 10:48 AM
  • add missing file
mart updated this revision to Diff 81019.Apr 23 2020, 4:46 PM
  • move all face-related stuff in own library
  • use the new face library
mart updated this revision to Diff 81078.Apr 24 2020, 9:33 AM
  • move here faces and packagestructure
ahiemstra added inline comments.Apr 30 2020, 10:11 AM
sensors/CMakeLists.txt
52

This is no longer needed.

sensors/declarative/CMakeLists.txt
5

This is no longer needed.

ahiemstra added inline comments.Apr 30 2020, 10:15 AM
faces/SensorFaceController.h
43

Can you place this class (and related classes) in the KSysGuard namespace? Then it would match the rest of the libraries.

ahiemstra added inline comments.May 1 2020, 12:26 PM
faces/ConfigAppearance.qml
91

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.

128

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

faces/SensorFaceController.cpp
44

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

78

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

183

This seems to be unused?

248

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.

416

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

477

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

faces/SensorFaceController.h
48

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.

51

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.

77

This seems superfluous?

faces/facepackages/barchart/contents/ui/BarChart.qml
39

This should use the controller.

faces/facepackages/barchart/contents/ui/Config.qml
51

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.

faces/facepackages/barchart/contents/ui/FullRepresentation.qml
59

This import seems to be missing?

mart updated this revision to Diff 81882.May 4 2020, 1:24 PM
mart marked 7 inline comments as done.
  • start to port other faces
  • port faces to new api
  • add missing file
  • adapt to api changes
  • adress a part of comments
mart added inline comments.May 4 2020, 1:24 PM
faces/SensorFaceController.cpp
78

it kinda has to, as the original idea is to be able to use any preset as it was a standalone applet, so creating a preset means you can find it directly in the widget explorer. If presets are moved somewhere else, it needs somthing that synchronizes them for the extra plasmoids to still work, which it addsa new brittle level of complexity.

it is a bit annoying that is depending indirectly from libplasma for the package plugin, perhaps it could have an internal, non-plugin replacement for this package structure to have it working also when plasma is not installed

183

it's used in the component->beginCreate(context); next line

248

perhaps line chart?

ahiemstra added inline comments.May 4 2020, 1:59 PM
faces/SensorFaceController.cpp
248

I was more thinking something along the lines of a new face that's either completely empty or displays a "not configured" message.

ngraham edited the summary of this revision. (Show Details)May 4 2020, 2:43 PM
ngraham added a reviewer: VDG.May 4 2020, 3:37 PM
mart added inline comments.May 4 2020, 4:24 PM
faces/SensorFaceController.h
48

how would that work in current faces that have this binded to that single big number inside graphs?

ngraham added a subscriber: ngraham.May 4 2020, 4:45 PM
ngraham added inline comments.
faces/ConfigAppearance.qml
25

as QQC2

70

swipeListItem has its own label property; can we use that instead of overriding the contentItem?

faces/ConfigSensors.qml
25

as QQC2

faces/FaceDetailsConfig.qml
25

as QQC2

46

"cfg_" + key

faces/UsedSensorsView.qml
25

as QQC2

faces/facepackages/barchart/contents/ui/CompactRepresentation.qml
24

as QQC2

faces/facepackages/barchart/contents/ui/Config.qml
24

as QQC2

faces/facepackages/linechart/contents/ui/CompactRepresentation.qml
24

as QQC2

faces/facepackages/linechart/contents/ui/Config.qml
23

as QQC2

faces/facepackages/linechart/contents/ui/LineChart.qml
48

Well, should we? :)

faces/facepackages/piechart/contents/ui/CompactRepresentation.qml
24

as QQC2

faces/facepackages/piechart/contents/ui/Config.qml
23

as QQC2

faces/facepackages/table/contents/ui/Config.qml
22 ↗(On Diff #81882)

as QQC2

45 ↗(On Diff #81882)

fixit :)

53 ↗(On Diff #81882)

That would be nice, yes. :) Nobody can ever figure out what "ascending" and "descending" means in the context of sort ordering.

faces/facepackages/table/contents/ui/FullRepresentation.qml
24 ↗(On Diff #81882)

import QtQuick.Controls 2.2 as QQC2

102 ↗(On Diff #81882)

Ugh, I don't like any of this :(

141 ↗(On Diff #81882)

this was merged almost a year ago; probably not needed anymore

159 ↗(On Diff #81882)

resets the models

ahiemstra added inline comments.May 5 2020, 9:49 AM
faces/SensorFaceController.h
48

You'd use the first one from the list? If we want to limit it I think we need to limit it in the UI, not in the backend, since that leads to a complete inability to do anything different.

mart updated this revision to Diff 82089.May 6 2020, 1:08 PM
  • default to linechart
  • default to textonly
  • sensorIds highPrioritySensorIds
  • kill table visualization
  • TotalSensor->TotalSensors list
  • SupportsTotalSensor->SupportsTotalSensors
ngraham accepted this revision as: VDG.May 6 2020, 1:54 PM
ahiemstra accepted this revision.May 6 2020, 2:21 PM

Let's get this in and fix anything broken in follow ups.

This revision is now accepted and ready to land.May 6 2020, 2:21 PM
mart updated this revision to Diff 82116.May 6 2020, 3:55 PM
  • move face properties in own file
  • move face config in own file
mart updated this revision to Diff 82119.May 6 2020, 4:05 PM
mart marked 5 inline comments as done.
  • add space
mart marked 9 inline comments as done.May 6 2020, 4:08 PM
mart updated this revision to Diff 82185.May 7 2020, 10:04 AM
  • use regexp for multiple sensors
  • left-alignlegends when they are too wide
mart updated this revision to Diff 82188.May 7 2020, 10:16 AM
  • remove extra debug
mart updated this revision to Diff 82223.May 7 2020, 5:10 PM
  • use QJsonArrays for sensors

Hmm, this doesn't seem to apply cleanly on top of current master.

zzag added a subscriber: zzag.May 7 2020, 9:16 PM
zzag added inline comments.
faces/SensorFaceController.cpp
178

One could argue that "Error creating component" is a pretty common error message. Categorized logging can be useful for resolving such ambiguities.

mart updated this revision to Diff 82279.May 8 2020, 3:52 PM
  • sensorColors is now a map
  • fix preset uninstall
mart updated this revision to Diff 82289.May 8 2020, 4:40 PM
  • Merge branch 'sensors_lib' into mart/sensor_face
mart closed this revision.May 11 2020, 11:13 AM