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

Authored by ngraham on Thu, Aug 22, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Thu, Aug 22, 10:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptThu, Aug 22, 10:51 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Thu, Aug 22, 10:51 PM
ngraham edited the summary of this revision. (Show Details)Thu, Aug 22, 10:51 PM
ngraham updated this revision to Diff 64382.Thu, Aug 22, 11:02 PM

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

filipf requested changes to this revision.Fri, Aug 23, 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.Fri, Aug 23, 7:17 AM
broulik added inline comments.Fri, Aug 23, 7:17 AM
Modules/energy/package/contents/ui/main.qml
166–178

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

filipf resigned from this revision.Fri, Aug 23, 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.Fri, Aug 23, 8:05 AM
Modules/energy/package/contents/ui/main.qml
166–178

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.Fri, Aug 23, 12:46 PM
Modules/energy/package/contents/ui/main.qml
166–178

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.Fri, Aug 23, 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.Fri, Aug 23, 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.Fri, Aug 23, 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.Fri, Aug 30, 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.EditedFri, Aug 30, 5:33 PM

Which icon? battery-ups?

meven accepted this revision.Sat, Aug 31, 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.Sat, Aug 31, 6:07 AM
This revision was automatically updated to reflect the committed changes.