Review Request 115326: fix theme cache cleanup and discarding

[Aaron J. Seigo]

> On Jan. 29, 2014, 1:50 p.m., Aaron J. Seigo wrote:
> > plasma/theme.cpp, lines 273-276
> > <https://git.reviewboard.kde.org/r/115326/diff/3/?file=240803#file240803line273>
> >
> >     it's nice to clean up the svg elements files; however without an active cache file it doesn't matter and they are a couple dozen K in size. didn't seem worth it for cache directory maintenance.
> >     
> >     leaving them on disk would result them in them being used again if the user installs a newer version of the theme, then later rolls back to an older version. however, given that the svg file should be tied to the version of the theme, all this accomplishes is having to re-calculate all the svg geometry data.
> >     
> >     i don't see the point in doing this at all.
> 
> Harald Sitter wrote:
>     well, from a downgrade POV perhaps the kimagecache cleanup also ought not delte all versions but those older than one or two versions down? that is, if downgrading is actually a viable use case of course. otherwise the logic would seem a bit incosistent. yeah, the kcache is sustantially larger, but I doubt any piece of software iterating the cache dir will care much for that reasoning when the dir suddenly contains
>     
>     plasma-svgelements-yolo_v1
>     plasma-svgelements-yolo_v2
>     plasma-svgelements-yolo_v3
>     plasma-svgelements-yolo_v4
>     plasma-svgelements-yolo_v5
>     plasma-svgelements-yolo_v6
>     ...
>     
>
> 
> Aaron J. Seigo wrote:
>     "from a downgrade POV perhaps the kimagecache cleanup also ought not delte all versions but those older than one or two versions down?"
>     
>     besides requiring a version parser / standard that gets followed, this is overly complex. downgrading themes is not common, which is why the caches are removed.
>     
>     "otherwise the logic would seem a bit incosistent."
>     
>     i don't see how: it keeps the cache for the current version, drops the rest.
>     
>     "yeah, the kcache is sustantially larger"
>     
>     which is a significant issue on devices with smaller disks or many users.
>     
>     "but I doubt any piece of software iterating the cache dir will care much for that reasoning when the dir suddenly contains"
>     
>     it's not about iterating the dir, but using substantial amounts of disk space for no reason.
> 
> Harald Sitter wrote:
>     So, what do you want me to do, or not to do? I am not following sorry :/

The code ought to:

* version both cache files to reflect the version # of the theme
* drop all other image caches (new / old, doesn't matter -> whatever isn't being used should be removed; elements cache files could also be removed but aren't a significant issue as they are a few dozen kb)

Can you try the patch posted here: http://pastebin.kde.org/p6czlim0q


- Aaron J.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115326/#review48535
-----------------------------------------------------------


On Jan. 28, 2014, 9:18 a.m., Harald Sitter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115326/
> -----------------------------------------------------------
> 
> (Updated Jan. 28, 2014, 9:18 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Martin Klapetek.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> - introduce svgelements cache handling in useCache()
> - to decide whether or not to discard a cache type now the mtimes of metadata.desktop and the respective cache file are compared (previously this was an invalid file compared to cache change time)
> - cache cleanup now happens entirely before creation of the KImageCache instance to prevent any sort of problem there
> 
> - add fixme comment about broken version-based cache handling. for one the version specific file name is not actually used for the KImageCache and for another the entire shebang is missing from svgelements all together. this has the very unfortunate side effect that the *active* cache files are deleted ever so often, it may be worth removing the code portions until someone finds time/interest to actually implement this in a working fashion.
> 
> 
> Diffs
> -----
> 
>   plasma/theme.cpp cb44878 
> 
> Diff: https://git.reviewboard.kde.org/r/115326/diff/
> 
> 
> Testing
> -------
> 
> upgraded from 4.8 straight to 4.12 -> theme caches correctly discarded
> 
> 
> Thanks,
> 
> Harald Sitter
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140130/777b69da/attachment-0001.html>