The code is not great, I would be open to have a suggestion to improve it.
I tried to pass Solid::Device directly to the qml side and using QHash to store vendor and product but they are not QObject so it can't work.
Details
- Reviewers
broulik ngraham - Commits
- R102:708879b54aa8: [energy kcm] Display Vendor and model
After:
Diff Detail
- Repository
- R102 KInfoCenter
- Branch
- arcpatch-D23152
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 15518 Build 15536: arc lint + arc unit
Is this the vendor and model for the battery itself? If so, maybe it should be under the "Battery" section towards the top.
Yes it is.
Since all the information in this section are about battery, I feel more appropriate to keep this in its own category.
Although the section is called "Manufacturer", a "detail", or "part" section title, something to convey it is about the actual battery hardware.
And it makes me think that the first section has a not great title currently "Battery", I would suggest instead "Battery status".
Also the "Rechargeable" information is not very informative.
By the way my plan is to add vendor and model here to remove it in the plasma applet after, since there it does not make much sense.
For reference in GNOME, the equivalent feature looks like :
https://i.stack.imgur.com/V7Fpk.png
I would be in favor of renaming the "Manufacturer" section to "Manufacturing" or "Part" or "Component".
@broulik is it the right path to add those information coming from Solide::Device to the BatteryModel that mostly wraps Solid::Battery ?
Am I missing a simple way to expand the BatteryModel to add the two fields Vendor and Product ?
Mmmmmmm..... I still think these properties would fit in the existing Battery section better. They're all properties of the battery, so I don't see why it wouldn't work to put them there.
Does it mean you would put the serial number in the battery section ?
The Energy section are also properties of the batteries, yet its content concerns the energy properties of the battery, and the Battery section concerns the battery state.
Yet Vendor and product as well as serial concerns another class of properties of the battery, how it was manufactured.
To add to my point, I also think it would look less appealing :
If the serial number is a property of the battery, then yes, it belongs in the Battery section IMO.
In the end it would mean we would have only two categories "Battery" and "Energy" (since I am about to remove the system one).
Just to make sure, that's what you suggest @ngraham
Modules/energy/batterymodel.h | ||
---|---|---|
47 | As a cleanup measure (before or after, but separate of this patch), can you remove those separate invokables
kcm.batteries.data(kcm.batteries.index(0, 0), BatteryModel.VendorRole) | |
Modules/energy/package/contents/ui/main.qml | ||
432 | Can you make this more generic, e.g. value = root["current" + uppercasefirst(modelData.source)] |
@ngraham I think you mentioned this change in https://pointieststick.com/2019/08/24/kde-usability-productivity-week-85/ but it has not landed yet.
@broulik I think it is ready.
I have the clean up follow up ready : D23391
I am adding the technology field to the KCM in D23392