D23152: [energy kcm] Display Vendor and model

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R102:708879b54aa8: [energy kcm] Display Vendor and model 
(authored by meven).

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23152?vs=64424=64627

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

AFFECTED FILES
  Modules/energy/batterymodel.cpp
  Modules/energy/batterymodel.h
  Modules/energy/package/contents/ui/main.qml

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven marked 2 inline comments as done.
meven added a comment.


  @ngraham I think you mentioned this change in 
https://pointieststick.com/2019/08/24/kde-usability-productivity-week-85/ but 
it has 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 


REPOSITORY
  R102 KInfoCenter

BRANCH
  arcpatch-D23152

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven updated this revision to Diff 64424.
meven marked 2 inline comments as done.
meven added a comment.


  Make code more generic

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23152?vs=64390=64424

BRANCH
  arcpatch-D23152

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

AFFECTED FILES
  Modules/energy/batterymodel.cpp
  Modules/energy/batterymodel.h
  Modules/energy/package/contents/ui/main.qml

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> broulik wrote in 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)

I will do it as a followup.

> broulik wrote in main.qml:432
> Can you make this more generic, e.g.
> 
>   value = root["current" + uppercasefirst(modelData.source)]

Nice suggestion :)

REPOSITORY
  R102 KInfoCenter

BRANCH
  arcpatch-D23152

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> batterymodel.h:47
>  Q_INVOKABLE Solid::Battery *get(int index) const;
> +Q_INVOKABLE QString vendor(int index) const;
> +Q_INVOKABLE QString product(int index) const;

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)

> main.qml:432
> +if (modelData.source) {
> +if (modelData.source == "vendor") {
> +value = currentVendor;

Can you make this more generic, e.g.

  value = root["current" + uppercasefirst(modelData.source)]

REPOSITORY
  R102 KInfoCenter

BRANCH
  arcpatch-D23152

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven updated this revision to Diff 64390.
meven added a comment.


  Removes the Manufacturer section, add Current Charge

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23152?vs=63720=64390

BRANCH
  arcpatch-D23152

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

AFFECTED FILES
  Modules/energy/batterymodel.cpp
  Modules/energy/batterymodel.h
  Modules/energy/package/contents/ui/main.qml

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  That looks fantastic to me!

REPOSITORY
  R102 KInfoCenter

BRANCH
  dev2

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


  @ngraham
  
  How about ?
  F7269705: Screenshot_20190822_113559.png 

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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


  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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Nathaniel Graham]

ngraham added a comment.


  If the serial number is a property of the battery, then yes, it belongs in 
the Battery section IMO.

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


  In D23152#514689 , @ngraham wrote:
  
  > Mmm. 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 :
  F7263383: Screenshot_20190820_074405.png 


REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


  For reference in GNOME, the equivalent feature looks like :
  https://i.stack.imgur.com/V7Fpk.png
  
  I would be in favor of rename the "Manufacturer" section to "Manufacturing" 
or "Part" or "Component".

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven added a comment.


  In D23152#511775 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R102 KInfoCenter

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

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D23152: [energy kcm] Display Vendor and model

[Méven Car]

meven updated this revision to Diff 63720.
meven added a comment.


  Add a space

REPOSITORY
  R102 KInfoCenter

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23152?vs=63719=63720

BRANCH
  dev2

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

AFFECTED FILES
  Modules/energy/batterymodel.cpp
  Modules/energy/batterymodel.h
  Modules/energy/package/contents/ui/main.qml

To: meven, broulik, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart