[Energy KCM] Port away from WorkspaceComponents.BatteryIcon and improve presentation
ClosedPublic

Authored by ngraham on Aug 22 2019, 10:51 PM.

Details

Summary

Using this Plasma component in an app causes the icons to disappear when using a dark
Plasma theme and a light color scheme, or vice versa. This patch fixes that by porting
away from the Plasma component and instead using a Kirigami icon to show the icons.

The patch also spruces up the visual presentation a bit.

BUG: 411051
FIXED-IN: 5.17.0

Depends on D23365

Test Plan

Diff Detail

Repository
R102 KInfoCenter
Branch
port-away-from-WorkspaceComponents.BatteryIcon (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15516
Build 15534: arc lint + arc unit
ngraham created this revision.Aug 22 2019, 10:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 22 2019, 10:51 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Aug 22 2019, 10:51 PM
ngraham edited the summary of this revision. (Show Details)Aug 22 2019, 10:51 PM
ngraham updated this revision to Diff 64382.Aug 22 2019, 11:02 PM

Don't use width: and height: on items in a Layout

filipf requested changes to this revision.Aug 23 2019, 7:17 AM
filipf added a subscriber: filipf.

Tested on 5.16 so disregard the other changes:

Before (dark Plasma theme, light color scheme):

After (dark Plasma theme, light color scheme):

Before (light Plasma theme, light color scheme):

After (light Plasma theme, light color scheme):

Before (color-aware Plasma theme, dark color scheme):

After (color-aware Plasma theme, dark color scheme):


+1 for the idea, but two problems are evident:

  • at least one new Kirigami icon is not monochrome -> input-mouse
  • at least one new Kirigami icon is not color scheme-aware -> battery-full

Other icons should also be checked, wouldn't hurt to see what support in other icons themes is like as well.

This revision now requires changes to proceed.Aug 23 2019, 7:17 AM
broulik added inline comments.Aug 23 2019, 7:17 AM
Modules/energy/package/contents/ui/main.qml
157–164

Now you lost the battery level and charging state in the icon.

filipf resigned from this revision.Aug 23 2019, 7:23 AM

I forgot I was using the La Capitaine icon theme so the above doesn't apply in the sense of now working well with defaults. Seems OK with Breeze.

broulik added inline comments.Aug 23 2019, 8:05 AM
Modules/energy/package/contents/ui/main.qml
157–164

Can use the following

var iconNameParts = ["battery"];
// Round percentage to the nearest 10% and pad it with leading zeroes to match the icon names
iconNameParts.push(String(Math.round(model.battery.chargePercent / 10) * 10).padStart(3, "0"));
if (model.battery.chargeState === 1) { // charging
    iconNameParts.push("charging");
}
return iconNameParts.join("-");
ngraham added inline comments.Aug 23 2019, 12:46 PM
Modules/energy/package/contents/ui/main.qml
157–164

My thought was that the battery level indicator inside the icon wasn't necessary because this UI has a current charge level indicator below the icon. Might be nice to show the plugged-in status though.

ngraham updated this revision to Diff 64414.Aug 23 2019, 1:17 PM
  • Add charging indicator to icon when possible
  • Clean up layout
  • Add more text to make things clearer
ngraham updated this revision to Diff 64415.Aug 23 2019, 1:18 PM

Remove debug thingy

ngraham retitled this revision from [Energy KCM] Port away from WorkspaceComponents.BatteryIcon to [Energy KCM] Port away from WorkspaceComponents.BatteryIcon and improve presentation.Aug 23 2019, 1:18 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham marked 3 inline comments as done.
meven added a comment.Aug 30 2019, 5:16 PM

I couldn't not get the new icon to show up.
Any idea ?

Other than the patch seems fine and works nicely

ngraham added a comment.EditedAug 30 2019, 5:33 PM

Which icon? battery-ups?

meven accepted this revision.Aug 31 2019, 6:07 AM

Which icon? battery-ups?

Yes those added in D23365 or at least they don't correspond to the screenshots.

But this works fine.

I noticed an issue (not related here) where if you unplug your device shortly after plugging it in, the displayed state in both battery plasmoid and kinfocenter stay charging.

This revision is now accepted and ready to land.Aug 31 2019, 6:07 AM
This revision was automatically updated to reflect the committed changes.