[energy kcm] Display Vendor and model
ClosedPublic

Authored by meven on Aug 14 2019, 12:17 PM.

Details

Summary

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.

Test Plan

After:

Diff Detail

Repository
R102 KInfoCenter
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Aug 14 2019, 12:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 14 2019, 12:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Aug 14 2019, 12:17 PM
meven edited the test plan for this revision. (Show Details)Aug 14 2019, 12:17 PM
meven retitled this revision from [energy kcm] Display Vendor and product to [energy kcm] Display Vendor and model.
meven updated this revision to Diff 63720.Aug 14 2019, 12:19 PM

Add a space

Is this the vendor and model for the battery itself? If so, maybe it should be under the "Battery" section towards the top.

meven added a comment.EditedAug 14 2019, 2:51 PM

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.

meven added a comment.EditedAug 16 2019, 4:58 PM

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".

"Manufacturer" is fine IMO. If it needs to be changed, maybe "Vendor" or "Hardware"

@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.

meven added a comment.Aug 20 2019, 5:45 AM

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.

meven added a comment.Aug 22 2019, 7:54 AM

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

meven added a comment.Aug 22 2019, 9:37 AM

@ngraham

How about ?

This is rebased on master and with D23130 applied and with the "Current charge" field added as I felt it was missing.

ngraham accepted this revision.Aug 22 2019, 3:35 PM

That looks fantastic to me!

This revision is now accepted and ready to land.Aug 22 2019, 3:35 PM
meven updated this revision to Diff 64390.Aug 23 2019, 6:42 AM

Removes the Manufacturer section, add Current Charge

meven edited the test plan for this revision. (Show Details)Aug 23 2019, 6:43 AM
broulik added inline comments.Aug 23 2019, 1:16 PM
Modules/energy/batterymodel.h
47

As a cleanup measure (before or after, but separate of this patch), can you remove those separate invokables

  1. Add Q_ENUM(Roles)
  2. qmlRegisterUncreatableType the BatteryModel
  3. from QML use data() which is invokable since Qt 5.5 (after this code was written iirc):
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)]
meven updated this revision to Diff 64424.Aug 23 2019, 2:58 PM
meven marked 2 inline comments as done.

Make code more generic

meven added inline comments.Aug 23 2019, 2:59 PM
Modules/energy/batterymodel.h
47

I will do it as a followup.

Modules/energy/package/contents/ui/main.qml
432

Nice suggestion :)

meven marked 2 inline comments as done.EditedSun, Aug 25, 6:53 AM

@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

This revision was automatically updated to reflect the committed changes.