Differential D23237

Do not display vendor, Product and capacity in network plasmoid
AbandonedPublic
Actions

Authored by meven on Aug 18 2019, 7:08 AM.

Details

Reviewers
ngraham
broulik
filipf
Group Reviewers
Plasma
VDG
Commits
R120:8e8441146aa8: Do not display vendor, Product and capacity in network plasmoid
Summary

Those information mostly clutter the plasmoid UI and are not very useful in the energy plasmoid
The battery plasmoid represents the state of the battery, for battery information we have the Energy information KCM in kinfocenter that will display this after D23152 .

Test Plan

Before:


After:

Diff Detail

Repository
R120 Plasma Workspace
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 18 2019, 7:08 AM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 18 2019, 7:08 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Aug 18 2019, 7:08 AM
meven edited the summary of this revision. (Show Details)Aug 18 2019, 7:53 AM
meven edited the test plan for this revision. (Show Details)
meven edited the summary of this revision. (Show Details)Aug 18 2019, 8:02 AM
ngraham accepted this revision.Aug 18 2019, 8:15 PM
ngraham added a reviewer: VDG.

+1, a simplified information display makes sense here IMO.

This revision is now accepted and ready to land.Aug 18 2019, 8:15 PM
filipf accepted this revision.Aug 18 2019, 8:25 PM
apol added a subscriber: apol.Aug 18 2019, 10:52 PM

LGTM but the title looks wrong, this isn't the network plasmoid.

meven edited the summary of this revision. (Show Details)Aug 19 2019, 7:03 AM
meven added a comment.Aug 19 2019, 7:18 AM
In D23237#514154, @apol wrote:

LGTM but the title looks wrong, this isn't the network plasmoid.

Fixed

I didn't except that to be that consensual ;)

Closed by commit R120:8e8441146aa8: Do not display vendor, Product and capacity in network plasmoid (authored by meven). · Explain WhyAug 19 2019, 7:18 AM
This revision was automatically updated to reflect the committed changes.
meven added a comment.Aug 19 2019, 7:20 AM
In D23237#514287, @meven wrote:
In D23237#514154, @apol wrote:

LGTM but the title looks wrong, this isn't the network plasmoid.

Fixed

Sorry it is not, I did a mistake.

broulik reopened this revision.EditedAug 19 2019, 8:48 AM

You cannot remove those from the dataengine, which is "Public API", there might be other users of that.
Instead, remove them from the battery monitor UI. And while at it maybe show the details always even if there are multiple batteries.
Capacity is also used in battery monitor to show a hint like "Your battery is broken"

Also, this is not the network plasmoid but battery.
Furthermore, I would appreciate if you could wait for maintainer approval before pushing changes that are clearly wrong, not everyone spends Sunday afternoon at a computer.

This revision is now accepted and ready to land.Aug 19 2019, 8:48 AM
broulik requested changes to this revision.Aug 19 2019, 8:58 AM
This revision now requires changes to proceed.Aug 19 2019, 8:58 AM
alexde added a subscriber: alexde.EditedAug 19 2019, 8:58 AM

In which GUI do I find "Vendor" and "Model" now? There's the "Energy Information" in KInfocenter, which
has a section "manufacturer" (in screenshot "Hersteller"), but only lists the serial number. Maby it could go there?

broulik added a comment.Aug 19 2019, 8:59 AM

It literally says in the commit message that this will be moved exactly there.

alexde added a comment.Aug 19 2019, 9:12 AM
In D23237#514325, @broulik wrote:

It literally says in the commit message that this will be moved exactly there.

I definitely need more coffee, sorry. O:-)

pkloc added a subscriber: pkloc.Aug 19 2019, 9:47 AM

Is this information really that cluttering to mandate removal? Sure, vendor and model may be too "advanced" but remaining capacity is an useful hint.
I always liked this sort of stuff when coming from Windows.

I would argue that capacity counts as "state".

meven added a comment.Aug 19 2019, 11:09 AM
In D23237#514328, @pkloc wrote:

Is this information really that cluttering to mandate removal? Sure, vendor and model may be too "advanced" but remaining capacity is an useful hint.

My intention is by no means to hide this data but move it somewhere more discreet proportionate to its usefulness.
We might want to keep it in the plasmoid but only when capacity < 90 for instance.

I always liked this sort of stuff when coming from Windows.

Differentiating from windows is not a reason to add or keep features.

In D23237#514317, @broulik wrote:

You cannot remove those from the dataengine, which is "Public API", there might be other users of that.
Instead, remove them from the battery monitor UI. And while at it maybe show the details always even if there are multiple batteries.
Capacity is also used in battery monitor to show a hint like "Your battery is broken"

Missed that.

Also, this is not the network plasmoid but battery.
Furthermore, I would appreciate if you could wait for maintainer approval before pushing changes that are clearly wrong, not everyone spends Sunday afternoon at a computer.

My bad, I overreacted, three approvals are kind of rare.

In the meantime I have reverted this patch.

meven abandoned this revision.Aug 19 2019, 11:45 AM

Abandoned in favor of D23260

Revision Contents
Changeset List

Diff 64019

View Options

dataengines/powermanagement/powermanagementengine.cpp

Loading...