Review Request 115326: fix theme cache cleanup and discarding

Wed Jan 29 14:48:58 UTC 2014

> On Jan. 29, 2014, 1:50 p.m., Aaron J. Seigo wrote:
> > this patch doesn't actually improve anything. in fact, the actual bug is left:
> > 
> >  pixmapCache = new KImageCache(pixmapCacheFileName, config.themeCacheKb() * 1024);
> > 
> > pixmapCacheFileName should have the version # in it. this was an oversight on my part when fixing br320855 in 59e88c777b92935cc95f28e52f84fe97563dec9d
> > 
> > this patch should not go in as is, but instead have the cache file name ammended to include the version information.

the point of this patch is not to fix cache versioning but cache cleaning such that http://i.imgur.com/v0Zyees.png does not happen. further more as mentioned in the description, fixing the cache versioning goes beyond the KImageCache construction but also requires the entire logic to be created for the svgelements cache. to that extent this patch is nothing but a hotfix and a step towards correct cache handling, not a complete fix.

> 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.

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
...

> On Jan. 29, 2014, 1:50 p.m., Aaron J. Seigo wrote:
> > plasma/theme.cpp, lines 291-302
> > <https://git.reviewboard.kde.org/r/115326/diff/3/?file=240803#file240803line291>
> >
> >     discardCache can take both PixmapCache and SvgElementsCache. in fact, calling it in this fashion will result in the cache first being invalidated and THEN deleted from disk.
> >     
> >     this should not even be necessary, however: if the cache is removed, then the svg elements also are removed.

pixmapCache is not delted from disk by discard, because there is no pixmapCache at this point as the if is only entered iff there is no pixmapCache to begin with.

as for the svg discard, the reason I chose to strip it out of the pixmap age check is because it's more explicit on it's own.

> On Jan. 29, 2014, 1:50 p.m., Aaron J. Seigo wrote:
> > plasma/theme.cpp, lines 306-307
> > <https://git.reviewboard.kde.org/r/115326/diff/3/?file=240803#file240803line306>
> >
> >     by creating the pixmap cache here, the above calls to discardCache won't really be doing anything as they act on the pixmapCache .. unless they delete the file from disk in which it doesn't matter.
> >     
> >     iow, you've got a very complex way of doing:
> >     
> >     QFile::remove(pixmapCacheFileName);
> >     pixmapCache = new KImageCache(pixmapCacheFileName, config.themeCacheKb() * 1024);

So, should I be moving it above the age check?

- Harald

-----------------------------------------------------------
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/20140129/fa761fb8/attachment-0001.html>