Combine display OSD icon files and move to plasma icon theme
ClosedPublic

Authored by pstefan on Aug 21 2018, 10:39 AM.

Details

Summary

Move the icons from D10937 to plasma-frameworks and combine all the sizes

BUG: 395714

Test Plan

"osd-shutd-screen.svg" does not display correctly for me. I can't find any difference between it and the rest of the icons. Help appreciated.

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pstefan created this revision.Aug 21 2018, 10:39 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 21 2018, 10:39 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pstefan requested review of this revision.Aug 21 2018, 10:39 AM
pstefan edited the summary of this revision. (Show Details)Aug 21 2018, 10:39 AM
pstefan added reviewers: ngraham, VDG.
pstefan retitled this revision from Combine display OSD icon files and mobe to plasma icon theme to Combine display OSD icon files and move to plasma icon theme.
pstefan updated this revision to Diff 40143.Aug 21 2018, 12:29 PM
  • Moved all OSD icons into video.svgz and renamed some of them.
ngraham requested changes to this revision.Aug 21 2018, 12:37 PM

If we rename the icons, we will need to correspondingly change the OSD code itself to use the new names: https://cgit.kde.org/kscreen.git/tree/kded/qml/OsdSelector.qml#n51

Unfortunately, that code lives in KScreen, which uses the Plasma release schedule rather than the Frameworks schedule. Therefore we cannot guarantee that people will be using the right combination of the two pieces of code; it's perfectly possible for a distro to ship KDE Frameworks 5.50 (with the renamed files) but KDE Plasma 5.13.5 (which would use the old names). If these lived on-disk, I'd suggest just making a symlink. Can we do that here internally? Is there any way to give one image two names in the svgz file?

Either way, that should be done in a separate commit. Please revert the name change and we'll do that later. It's important to keep patches as small as possible to keep the git history clean, and also make it easy to revert any individual change that caused problems.

This revision now requires changes to proceed.Aug 21 2018, 12:37 PM
pstefan updated this revision to Diff 40154.Aug 21 2018, 1:52 PM
  • Reverse name change
broulik requested changes to this revision.Aug 21 2018, 2:15 PM
broulik added a subscriber: broulik.

File looking good, not really a fan of the new thin lined icon style, but the file must be named osd.svgz (Plasma always uses the part before the first hyphen for lookup)

This revision now requires changes to proceed.Aug 21 2018, 2:15 PM
pstefan updated this revision to Diff 40156.Aug 21 2018, 2:23 PM
  • Modes OSD files out of video.svgz into osd.svgz. Removed the 64-64- prefix.
pstefan updated this revision to Diff 40157.Aug 21 2018, 2:32 PM
  • changed osd-sbs-right to osd-sbs-sright
broulik accepted this revision.Aug 21 2018, 2:39 PM

When I apply this patch and compile and deploy plasma-framework to /usr/ and then delete the Breeze icons versions, I still don't see the new icons in the switcher. What am I doing wrong?

Did you nuke the plasma icon cache? ~/.cache/plasma* (We need to bump the theme version number before frameworks tagging, then this will work automatically)

Ah, that did it.

So should we bump the version in this patch, then?

ngraham edited the summary of this revision. (Show Details)Aug 22 2018, 10:13 PM
ngraham accepted this revision.Sep 12 2018, 1:55 PM

Nah, I'll land this now, and bump X-KDE-PluginInfo-Version in src/desktoptheme/breeze/metadata.desktop before Frameworks 5.51 tagging.

Thanks so much for your work here, Phil! I know it's been a long road, but I'm going to land this now.

For your next patch, for all the icons that currently have a desktop-looking screen, could you make additional versions that have a laptop instead? Then we can patch KScreen for Plasma 5.15 to dynamically show one set or other other depending on whether you're on a laptop or a desktop. That'll be totally awesome.

This revision is now accepted and ready to land.Sep 12 2018, 1:55 PM
This revision was automatically updated to reflect the committed changes.