Review Request 115326: fix theme cache cleanup and discarding

[Harald Sitter]

> On Jan. 27, 2014, 4:30 p.m., Martin Klapetek wrote:
> > plasma/theme.cpp, lines 298-300
> > <https://git.reviewboard.kde.org/r/115326/diff/2/?file=240703#file240703line298>
> >
> >     Seems like these two could be squashed into one but...meh, up to you

ETOOLONGSTATEMENT IMO.


> On Jan. 27, 2014, 4:30 p.m., Martin Klapetek wrote:
> > plasma/theme.cpp, line 311
> > <https://git.reviewboard.kde.org/r/115326/diff/2/?file=240703#file240703line311>
> >
> >     One thing I don't understand (not related to your changes though) - why does this return cacheTheme which never ever changes inside this method? And it also never returns anywhere else, so why is it even returning a boolean value when its value is already known before this method? :O

That's the million dollar question isn't it ;)

useCache is a getter, except it's really not. It is however being (ab)used as single entry point for pre-cache-access conditions, and therefore a convenient lazy-init function.
i.e. when one calls useCache() one actually does this from an effect POV:

if (d->useCache) {
 d->initPixmapCache();
 KImageCache cache = d->pixmapCache;
 [...]
}

To that extent it would really be better (for the readability) of the respective functions to move the lazy-init into pixmapCache() and svgElementsCache() functions the resulting access code remains almost the same as currently:

if (d->useCache) {
  KImageCache cache = d->pixmapCache();
  [...]
}

This way however there's no file IO in the if. Ever. Additionally right now there is no active lazy init for the svg elements cache it appears. The only way to get that cache initalized is by somehow getting a discardCache(...) call which will tear it down and then (re)init :/.


- Harald


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


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/20140128/c2528419/attachment.html>