Add battery icons
ClosedPublic

Authored by ndavis on Feb 12 2019, 6:22 AM.

Details

Reviewers
hein
ngraham
Group Reviewers
VDG
Commits
R266:b04959059668: Add battery icons
Summary

Adds battery icons at 16, 22 and 32 px. Includes symlinks for symbolic icons and standard icon names for compatibility with 3rd parties. Adds status/32 to index.theme. Changes status/22 and status/22@2x from Scalable to Fixed. Requested by @hein for drone control app.

Test Plan


Diff Detail

Repository
R266 Breeze Icons
Branch
battery-icons (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8309
Build 8327: arc lint + arc unit
ndavis created this revision.Feb 12 2019, 6:22 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 12 2019, 6:22 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ndavis requested review of this revision.Feb 12 2019, 6:22 AM
ndavis edited the test plan for this revision. (Show Details)Feb 12 2019, 6:27 AM
ndavis updated this revision to Diff 51461.Feb 12 2019, 6:31 AM

Add status/32 to index.theme

Not certain if I've got the index.themes right

ndavis updated this revision to Diff 51462.EditedFeb 12 2019, 6:36 AM

Fix missing PositiveText stylesheet for battery-020-charging at 32px

ndavis edited the summary of this revision. (Show Details)Feb 12 2019, 6:46 AM

I like it!
Would you consider moving the green charging to either the filled-in or the white part where the symbol is currently above the line where the filled-in part ends?

filipf added a subscriber: filipf.Feb 12 2019, 2:53 PM

Nice. Shouldn't the plug here be green, same as everywhere else?

Nice. Shouldn't the plug here be green, same as everywhere else?

This was fixed in the last change

I like it!
Would you consider moving the green charging to either the filled-in or the white part where the symbol is currently above the line where the filled-in part ends?

I think having a different location based on battery level would get annoying

I think having a different location based on battery level would get annoying

You're probably right.

So I guess this means some duplication between the Breeze icon theme and the Breeze Plasma theme for a while, right?

If we get this in before Frameworks 5.58, then Plasma 5.16 can depend on having them in the Breeze icon theme and maybe we can port the code to use these as icons from the icon theme rather than SVGs from the Plasma theme and then delete them from the Plasma theme. Baby steps...

So I guess this means some duplication between the Breeze icon theme and the Breeze Plasma theme for a while, right?

Not 100%. I need to make a device icon for an uninterruptible power supply. I suppose I could add that to this diff, but it's not immediately necessary either. I'll get it done before 5.57 is tagged.

If we get this in before Frameworks 5.58, then Plasma 5.16 can depend on having them in the Breeze icon theme and maybe we can port the code to use these as icons from the icon theme rather than SVGs from the Plasma theme and then delete them from the Plasma theme. Baby steps...

That's the idea, but moving Plasma theme icons into Breeze icons only has the potential to make the issues with color vs monochrome worse. I don't have a problem with continuing for now since it's highly unlikely that any of these icons will be used where we would want a color icon to go even though some of these are 32px.

maybe we can port the code to use these as icons from the icon theme rather than SVGs from the Plasma theme and then delete them from the Plasma theme

And in the process break all third party Plasma themes?

maybe we can port the code to use these as icons from the icon theme rather than SVGs from the Plasma theme and then delete them from the Plasma theme

And in the process break all third party Plasma themes?

Won't 3rd party themes contain their own icons?

Won't 3rd party themes contain their own icons?

Sure but in the style of a Plasma battery.svgz, so if we change the BatteryIcon to use icon theme instead of Plasma theme icons (which none of the other tray icons do, btw) we'll effectively break third party themes.

Won't 3rd party themes contain their own icons?

Sure but in the style of a Plasma battery.svgz, so if we change the BatteryIcon to use icon theme instead of Plasma theme icons (which none of the other tray icons do, btw) we'll effectively break third party themes.

Does a plasmoid need to be told to get the icons from the Plasma theme? Or will it fetch the icon from the icon theme if it does not exist in the plasma theme?

The battery icon is composited of multiple SVG items layered ontop of each other, it doesn't just load a single icon.

I'm in favor of adding proper battery icons to the Breeze iconset, definitely need "battery", "battery-caution", "battery-low" icons (from the freedesktop icon spec) but we cannot remove them from the Plasma theme.

The battery icon is composited of multiple SVG items layered ontop of each other, it doesn't just load a single icon.

I'm in favor of adding proper battery icons to the Breeze iconset, definitely need "battery", "battery-caution", "battery-low" icons (from the freedesktop icon spec) but we cannot remove them from the Plasma theme.

Fair enough. Fun fact: there is no desktop environment that uses battery-caution and battery-low in the manner defined by the fd.o icon naming spec.

ngraham accepted this revision.Feb 13 2019, 3:35 PM

Ideally I would like to move all icons out of Plasma themes, because these themes are not 100% self-contained and often use icons from the icon theme anyway, and users get confused as to why changing their icon theme only changes some (but not all) Plasma icons. And it makes life a bit harder for us to maintain icons in two different places.

However that is indeed another discussion and this looks good to me the way it is.

This revision is now accepted and ready to land.Feb 13 2019, 3:35 PM

The do a proper proposal (I bet you already have a task about that) instead of just slipping things in one by one in unrelated reviews

Not trying to slip anything in (this patch doesn't and can't touch plasma-framework, after all), just mentioning it. :)

The proposal is in T10046: Improve the colors, color consistency and colorscheme compatibility of Breeze.

ndavis added a comment.EditedFeb 14 2019, 5:42 AM

This is the configuration for status/32:

[status/32]
Size=32
Context=Status
Type=Fixed

This currently prevents the 32px icons in this patch from being used:

[status/22]
Size=22
Context=Status
Type=Scalable
MinSize=22
MaxSize=32

Would changing status/22 to Fixed cause a problem? It doesn't appear to cause any real issue when I look at the change through Cuttlefish.

Would changing status/22 to Fixed cause a problem? It doesn't appear to cause any real issue when I look at the change through Cuttlefish.

Run dolphin with QT_SCALE_FACTOR=2 in the environment and see if the Places panel icons are still monochrome.

Would changing status/22 to Fixed cause a problem? It doesn't appear to cause any real issue when I look at the change through Cuttlefish.

Run dolphin with QT_SCALE_FACTOR=2 in the environment and see if the Places panel icons are still monochrome.

Places panel icons aren't status icons. I tried the same environment variable on Cuttlefish and it still looks ok.

ndavis updated this revision to Diff 51712.Feb 14 2019, 8:21 PM

Change status/22 and status/22@2x from Scalable to Fixed

ndavis edited the summary of this revision. (Show Details)Feb 14 2019, 8:23 PM
This revision was automatically updated to reflect the committed changes.