D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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,

D23152: [energy kcm] Display Vendor and model

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.

D23152: [energy kcm] Display Vendor and model

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:

D23152: [energy kcm] Display Vendor and model

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,

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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,

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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

D23152: [energy kcm] Display Vendor and model

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,

D23152: [energy kcm] Display Vendor and model

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