Review Request: This patch adds the Storage class for Plasma::DataEngine caching. This current implementation caches to disk. Soon, it will be akonadi.

Sat Jul 10 10:34:55 CEST 2010

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4548/#review6459
-----------------------------------------------------------

a good start. there are a number of rough edges that need to be ironed out, but the direction is good.

/trunk/KDE/kdelibs/plasma/datacontainer.h
<http://reviewboard.kde.org/r/4548/#comment6190>

    apidox are required.

/trunk/KDE/kdelibs/plasma/datacontainer.h
<http://reviewboard.kde.org/r/4548/#comment6191>

    apidox are required. method should be const.

/trunk/KDE/kdelibs/plasma/datacontainer.h
<http://reviewboard.kde.org/r/4548/#comment6192>

    apidox are required. method should be const.

/trunk/KDE/kdelibs/plasma/datacontainer.h
<http://reviewboard.kde.org/r/4548/#comment6188>

    both of these members must be in the private member. as it is, this breaks binary compatibility!

/trunk/KDE/kdelibs/plasma/datacontainer.cpp
<http://reviewboard.kde.org/r/4548/#comment6189>

    this should be initialized to true in the constructor as well.

/trunk/KDE/kdelibs/plasma/datacontainer.cpp
<http://reviewboard.kde.org/r/4548/#comment6187>

    needsCacheing()?

/trunk/KDE/kdelibs/plasma/dataengine.h
<http://reviewboard.kde.org/r/4548/#comment6193>

    in the wrong place in the header: it has separated removeAllData from its apidox. setEnableCache also requires apidox.

    i wonder, also, if we'll end up with use cases for a "global" setEnableCache which will dis- or en-able caching for all sources.

/trunk/KDE/kdelibs/plasma/dataengine.cpp
<http://reviewboard.kde.org/r/4548/#comment6186>

    why is the source being cached on disconnection? it really only ought to do so when it is the last disconnection (resulting in the source being deleted), set to be cached and the data that has not yet been cached.

/trunk/KDE/kdelibs/plasma/dataengine.cpp
<http://reviewboard.kde.org/r/4548/#comment6185>

    this can't go in like this; the dataengine should cache on demand: when a DataContainer is updated, if it is set for caching, then it should perform a time-delayed caching. but running through every source in every dataengine at three minute intervals whether it needs it or not isn't acceptable for performance.

/trunk/KDE/kdelibs/plasma/dataengine.cpp
<http://reviewboard.kde.org/r/4548/#comment6183>

    this pointer is being leaked. either delete it when done (e.g. connecting the finishing of the job to deleteLater() of the Storage object) or create it on the stack instead.

/trunk/KDE/kdelibs/plasma/dataengine.cpp
<http://reviewboard.kde.org/r/4548/#comment6184>

    should probably take a local copy of s->data() so that you are gauaranteed to be operating on the same collection over the course of the while loop and ensuring that it and s->data().constEnd() are from the same collection as well.

/trunk/KDE/kdelibs/plasma/dataengine.cpp
<http://reviewboard.kde.org/r/4548/#comment6182>

    this will work ok for low latency / synchronous storage fetching, which is what the current implementation is. but it really needs to be made properly async to avoid blocking.

    which means starting the job and populating the source when it returns ... but only if the source hasn't already been populated in the meantime.

/trunk/KDE/kdelibs/plasma/private/storage.h
<http://reviewboard.kde.org/r/4548/#comment6194>

    the file name should be storage_p.h since it is a private header.

/trunk/KDE/kdelibs/plasma/private/storage.h
<http://reviewboard.kde.org/r/4548/#comment6181>

    should be m_serviceName; it's a member. it should also be private.

/trunk/KDE/kdelibs/plasma/private/storage.cpp
<http://reviewboard.kde.org/r/4548/#comment6180>

    it is possible to reach this point without a setResult or setError call (in particular if the operationName isn't one of store or retrieve).

    i'd suggest ending this method with setError(1);

- Aaron

On 2010-07-08 16:47:24, Brian Pritchett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4548/
> -----------------------------------------------------------
> 
> (Updated 2010-07-08 16:47:24)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> DataEngines can mark or unmark their sources to be cached with void DataEngine::setEnableCache(const QString &source, bool cache). If the DataEngine has implemented their own source by inheriting DataContainer, then DataContainer::setEnableCache(bool cache) will work.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1147556 
>   /trunk/KDE/kdelibs/plasma/data/operations/storage.operations PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/datacontainer.h 1147556 
>   /trunk/KDE/kdelibs/plasma/datacontainer.cpp 1147556 
>   /trunk/KDE/kdelibs/plasma/dataengine.h 1147556 
>   /trunk/KDE/kdelibs/plasma/dataengine.cpp 1147556 
>   /trunk/KDE/kdelibs/plasma/private/dataengine_p.h 1147556 
>   /trunk/KDE/kdelibs/plasma/private/storage.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/private/storage.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4548/diff
> 
> 
> Testing
> -------
> 
> I have tested it with the microblogging dataengine/plasmoid.
> 
> 
> Thanks,
> 
> Brian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100710/0a1106b4/attachment-0001.htm 