Review Request 115326: fix theme cache cleanup and discarding

Thu Jan 30 10:58:07 UTC 2014

> 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 :/
> 
> Aaron J. Seigo wrote:
>     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
>

Okeydokey, new diff should cover everything then.

- Harald

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

On Jan. 30, 2014, 10:56 a.m., Harald Sitter wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115326/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2014, 10:56 a.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo and Martin Klapetek.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> - to decide whether or not to discard a cache type now the mtimes of metadata.desktop and the pixmap cache file (previously this was an invalid file compared to cache change time)
> - the discard check now compares the mtime of the actual pixmap cache file and the actual metadata file to ensure that we are comparing values with equal meaning (previously the kimagecache modification time was used  which appears to be the creation time of the object by default)
> - whether the cache needs to be discard is decided before kimagecache is created to avoid it altering the mtime, actual discarding still happens after initialization of the pixmap cache
> - introduced a new themeVersion member on the private class
> - svgelements cache is now using a versioned cache file whenever themeVersion is not empty
> - introduce svgelements cache maintenance in useCache()
> - pixmap cache is now using a versioned cache file whenever themeVersion is not empty (previously the wrong name var was used)
> - various variables inside useCache had their names adjusted to clearify their purpose.
> 
> 
> Diffs
> -----
> 
>   plasma/theme.cpp cb44878 
> 
> Diff: https://git.reviewboard.kde.org/r/115326/diff/
> 
> 
> Testing
> -------
> 
> cache file lineup:
>   cache/plasma-svgelements-default
>   cache/plasma-svgelements-default_v1.9
>   cache/plasma-svgelements-default_v2.0 (no pix cache)
>   cache/plasma_theme_default.kcache
>   cache/plasma_theme_default_v1.9.kcache
>   cache/plasma_theme_default_v2.1.kcache (no svg cache)
> 
> version of default theme for testing: 2.2
> 
> [T1] initial run without v2.2 caches:
> deleted all previous caches, correctly created plasma-svgelements-default_v2.2 and plasma_theme_default_v2.2.kcache.
> 
> [T2] subsequent run with v2.2 cache present:
> deleted all prevoius caches, existing v2.2 cache not deleted or discard by useCache().
> 
> [T3] subsequent run with v2.2 cache present and `touch metadata.desktop`:
> deleted all prevoius caches, existing v2.2 cache not deleted, but discarded by useCache().
> 
> [T4] subsequent run with v2.2. cache, but default theme has no version anymore:
> deleted *all* caches, discarded (newly created) empty caches plasma-svgelements-default and plasma_theme_default.kcache.
> 
> 
> Thanks,
> 
> Harald Sitter
> 
>

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