Keep desktoptheme SVG files uncompressed in repo, install svgz
ClosedPublic

Authored by kossebau on Apr 1 2019, 12:30 PM.

Details

Summary

The SVG format being based on plain text, storing the SVG in the repository
not as .svgz, but .svg, helps both VCS tools (patching, showing diffs) as
well as allows some developers to edit the SVG directly in any text editor,
not only those which support automatic conversion from/to gzip format.

While most artists will continue (and which shall be okay) to use GUI
editors like inkscape, which might rewrite the complete structure on saving,
using uncompressed format in the repo still allows the occasional direct
edit of the text, .e.g. to change a colour, which then is also easily seen
in the commit diff.

To still keep the svgz format when deployed, a build step is introduced,
which uses gzip to create svgz files in the build dir. This conversion can
be controlled using the option GZIP_DESKTOPTHEME_SVG (default: ON).

Test Plan

Themes are still working (with & without cache removed).

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.
kossebau created this revision.Apr 1 2019, 12:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 1 2019, 12:30 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Apr 1 2019, 12:30 PM
kossebau added a comment.EditedApr 1 2019, 12:33 PM

A similar patch might also be interesting for the SVG-based icon themes in the future. Keeping focussed on Plasma theme here for now.

The SVGZ -> SVG conversion of all files not included in this uploaded patch, to keep it reviewable.

Locally do this:

Store a file svgztosvg.sh with this content in src/desktoptheme:

#!/bin/sh

SVGZ=$1

if [ ! -f "$SVGZ" ]; then
    echo "File not found: \"$SVGZ\""
    exit 1
fi

SVG=${SVGZ%.svgz}.svg

zcat $SVGZ > $SVG
rm $SVGZ

Then on the commandline do this:

cd plasma-framework/src/desktoptheme
find . -name "*.svgz" -exec sh svgztosvg.sh {} \;

Once done, do e.g. this to restore SVGZ files:

find . -name "*.svg" -exec rm {} \;
git checkout air breeze oxygen
fvogt added a subscriber: fvogt.Apr 1 2019, 12:39 PM

I'm very much in favor of this, but don't know anything about the cmake magic involved, so can't really say much about that.

src/desktoptheme/CMakeLists.txt
16

You could make this optional and set the STRIP_DESKTOPTHEME_SVG default depending on whether it's found.

kossebau added inline comments.Apr 1 2019, 12:41 PM
src/desktoptheme/CMakeLists.txt
16

Personally I favour the buildsystem not to randomly do what it can given the current system state, but only do what I explicitly tell it to do or what the default is. But whatever majority prefers.

kossebau updated this revision to Diff 55191.Apr 1 2019, 12:53 PM

Fix some naming inconsistencies one only sees when looking at a patch
through another viewer (like the phabricator review page :))

kossebau added a comment.EditedApr 1 2019, 2:52 PM

While the main purpose of this patch is to have plain-text SVG files in the repo, the build-processing to optimize install size/data-to-load is an interesting side-effect.

If you are curious about some numbers, here the install size estimated with du -chb for the respective subfolders desktoptheme/$theme, with strip meaning the flags to svgcleaner as in the current patch:

breezeairoxygen
old:1.236.2451.762.968864.292
strip+z:1.105.3891.605.054739.340
z:1.231.0781.762.734875.083
strip:2.250.8474.071.7773.164.541
svg 1:1:3.130.9825.624.5884.777.657

So with Oxygen there is a small increase when using gzip only, compared to before. Seems for some files inkscape or what stored those files before was more efficient compared to gzip -9.
IMHO the increase of 10791 is not that awful, after all in the long term we surely want to recommend the usage of the stripped variant. And for Breeze, after all the default, the plain gzip variant already gains 5167, so even without stripping the install size is in total not that much affected.

ngraham added a subscriber: ngraham.Apr 1 2019, 4:28 PM

Strong +1 on the idea. Shouldn't we put these Find<thing>.cmake files in ECM though? The Breeze icons repo could definitely benefit from FindSVGCleaner to automatically post-process the SVGs for example.

Strong +1 on the idea. Shouldn't we put these Find<thing>.cmake files in ECM though? The Breeze icons repo could definitely benefit from FindSVGCleaner to automatically post-process the SVGs for example.

Agreed. Though being cautious by nature, I wanted to first see how this works out for the Plasma themes, before applying the same for the Breeze icons & any other SVG resources. Compare also the comment added in src/desktoptheme/CMakeLists.txt:

# Helper function, private for now
# Once it has matured and proven, add to public macros

So not being an urgent fix, I would have waited at least 2-3 KF releases, before pushing this further onto the stack and silblings. But that's just me, if majority already wants this now, would do the needed bits.

ndavis added a subscriber: ndavis.Apr 1 2019, 5:30 PM

Be careful that you are not stripping stylesheets when you use SVG Cleaner. scour is less effective than SVG Cleaner, but it doesn't strip stylesheets and it's available in more distros.

GB_2 added a subscriber: GB_2.Apr 1 2019, 5:38 PM

Be careful that you are not stripping stylesheets when you use SVG Cleaner. scour is less effective than SVG Cleaner, but it doesn't strip stylesheets and it's available in more distros.

Yes, see https://community.kde.org/Guidelines_and_HOWTOs/Icon_Workflow_Tips#SVG_optimization

In D20166#441733, @GB_2 wrote:

Be careful that you are not stripping stylesheets when you use SVG Cleaner. scour is less effective than SVG Cleaner, but it doesn't strip stylesheets and it's available in more distros.

Yes, see https://community.kde.org/Guidelines_and_HOWTOs/Icon_Workflow_Tips#SVG_optimization

Aha, thanks for the pointer, had not yet seen that wiki page. Indeed I now see that svgcleaner loses the styles, that's a blocker then. I knew I should set the default of stripping to OFF, as I was not yet sure how to test the equality of the results, besides superficial visual comparisons. scour broke more in my eyes on first tests, so I had not continued that, also was I not sure about the dependencies this pulls in (e..g for packagers). Guess I should add another option which stripping tool to use? :P
Or rather remove the stripping from this patch again for now, until we have one tool which reliably works for our needs.

Strictly speaking, there's no need to optimize the SVGs as a part of this, so maybe, yeah, let's split that out into a separate feature (and one that the Breeze Icons repo would ultimately be interested in as well).

ndavis added a comment.Apr 1 2019, 8:29 PM

Strictly speaking, there's no need to optimize the SVGs as a part of this, so maybe, yeah, let's split that out into a separate feature (and one that the Breeze Icons repo would ultimately be interested in as well).

Yes. Ideally, designers should optimize their work before it is landed or at least not submit an SVG with tons of metadata.

kossebau updated this revision to Diff 55231.Apr 1 2019, 8:31 PM

Remove stripping feature

Strictly speaking, there's no need to optimize the SVGs as a part of this, so maybe, yeah, let's split that out into a separate feature (and one that the Breeze Icons repo would ultimately be interested in as well).

Yes. Ideally, designers should optimize their work before it is landed or at least not submit an SVG with tons of metadata.

IMHO the SVG files in the repo may contain comments and metadatas, for further work on them. Like source code also has all the oomments and metadata.
Only the deployed graphics resource should then be limited and optimized for what is needed at runtime.

At least I had dealt with quite some "final rendered" SVG files I would have liked to modify for some needs, but could hardly as the base resources where not available (and if it was only guides & Co.).

kossebau retitled this revision from Keep desktoptheme SVG files uncompressed in repo, install (stripped) svgz to Keep desktoptheme SVG files uncompressed in repo, install svgz.Apr 1 2019, 8:40 PM
kossebau edited the summary of this revision. (Show Details)
bruns added a subscriber: bruns.Apr 1 2019, 8:45 PM

Strictly speaking, there's no need to optimize the SVGs as a part of this, so maybe, yeah, let's split that out into a separate feature (and one that the Breeze Icons repo would ultimately be interested in as well).

Yes. Ideally, designers should optimize their work before it is landed or at least not submit an SVG with tons of metadata.

On the other hand, this makes it harder for everyone else to do any changes later - think of e.g guides used for alignement. Also, stripping any whitespace makes diffs of minor changes quite large, as diffs align on lines, not tokens.

ndavis added a comment.EditedApr 1 2019, 10:36 PM

IMHO the SVG files in the repo may contain comments and metadatas, for further work on them. Like source code also has all the oomments and metadata.
Only the deployed graphics resource should then be limited and optimized for what is needed at runtime.

At least I had dealt with quite some "final rendered" SVG files I would have liked to modify for some needs, but could hardly as the base resources where not available (and if it was only guides & Co.).

I can see where you're coming from, but guides left by other people aren't that useful in my experience. I still have to guess how they made their shapes in the first place and there's no comment field that's visible from within Inkscape. Nobody ever puts comments in the code either. As long as you know how to use a Grid, Group/Ungroup and Combine/Break Apart, it's not hard to edit most existing icons. The few that are hard would not be improved by in-code comments or guides. Most of what I say comes from the perspective of someone working on icons though. Maybe guides would be more useful for people working on widgets, so we can make an exception for those.

[...] Also, stripping any whitespace makes diffs of minor changes quite large, as diffs align on lines, not tokens.

Then we can use optimization rules that auto format and indent everything. It will expand the size of color icons a lot more than monochrome icons, but maybe we can live with that.

kossebau updated this revision to Diff 55383.Apr 3 2019, 10:29 PM

Remove last bit about stripping that was still in the patch

Recent example where plain SVGs in the repo would have made the change immdiate visible, as only the strings of some id attributes where changed (to match the new icon names), but instead all we see with our tool is an exchange of the complete binary blob: D20261

So, do I correctly see that people are rather positive about this change, at least no-one objecting it? If so, I would propose to merge it after KDE Frameworks 5.57 has been branched this WE, i.e. merge Friday next week for another 7 days of consideration, but with some weeks before the next release so people using master can test-drive things some more before release, not only me :)

Who feels like giving explicit +1 or the needed Accepts? :)

If so, I would propose to merge it after KDE Frameworks 5.57 has been branched this WE, i.e. merge Friday next week for another 7 days of consideration, but with some weeks before the next release so people using master can test-drive things some more before release, not only me :)

Makes sense to me. That said, I just gave this a whirl and got a CMake error immediately:

CMake Error at src/desktoptheme/CMakeLists.txt:30 (message):
  No files passed when calling plasma_install_desktoptheme_svgs.
Call Stack (most recent call first):
  src/desktoptheme/oxygen/CMakeLists.txt:5 (plasma_install_desktoptheme_svgs)

If so, I would propose to merge it after KDE Frameworks 5.57 has been branched this WE, i.e. merge Friday next week for another 7 days of consideration, but with some weeks before the next release so people using master can test-drive things some more before release, not only me :)

Makes sense to me. That said, I just gave this a whirl and got a CMake error immediately:

CMake Error at src/desktoptheme/CMakeLists.txt:30 (message):
  No files passed when calling plasma_install_desktoptheme_svgs.
Call Stack (most recent call first):
  src/desktoptheme/oxygen/CMakeLists.txt:5 (plasma_install_desktoptheme_svgs)

Yes, because an important hint was already moved out of sight, so let my recite my first comment:

The SVGZ -> SVG conversion of all files not included in this uploaded patch, to keep it reviewable.

Locally do this:

Store a file svgztosvg.sh with this content in src/desktoptheme:

#!/bin/sh

SVGZ=$1

if [ ! -f "$SVGZ" ]; then
    echo "File not found: \"$SVGZ\""
    exit 1
fi

SVG=${SVGZ%.svgz}.svg

zcat $SVGZ > $SVG
rm $SVGZ

Then on the commandline do this:

cd plasma-framework/src/desktoptheme
find . -name "*.svgz" -exec sh svgztosvg.sh {} \;

Once done, do e.g. this to restore SVGZ files:

find . -name "*.svg" -exec rm {} \;
git checkout air breeze oxygen
ngraham accepted this revision.Apr 5 2019, 3:47 PM

Sorry, I should learn to read...

When following the instructions, this works flawlessly. Indeed, let's do it for 5.58 so we have lots of time for testing. Thanks for this very nice patch, @kossebau!

This revision is now accepted and ready to land.Apr 5 2019, 3:47 PM
This revision was automatically updated to reflect the committed changes.

The CI is a bit unhappy with the gzip depedency on Windows.
Might one just write a minimal KArchive based gzip'er for this? gzip isn't there on any normal Windows machine, even if you have libz.

The CI is a bit unhappy with the gzip depedency on Windows.
Might one just write a minimal KArchive based gzip'er for this? gzip isn't there on any normal Windows machine, even if you have libz.

D20499 is currently in the works, needing people != me who use windows to test, please :)