[Kde-pim] [Differential] [Updated] D1636: Implement client-side threading cache

[dfaure (David Faure)]

dfaure added a comment.


  Well done for having the courage to touch the messagelist code :-)
  
  I can't really review the core logic, but here's some feedback mostly on the cache file handling.

INLINE COMMENTS

> model.cpp:762
>  
> +    d->mThreadingCache.save();
> +    d->mThreadingCache.load(d->mStorageModel->id(), d->mAggregation);

This reads strange... What is the purpose of save() followed by load()? It sounds like reloading what we just saved, i.e. the load being a no-op. I'm guessing this isn't the case, but then it's worth a comment or renaming....

> threadingcache.cpp:40
> +    int cacheSize;
> +} cacheHeader;
> +}

Why is this instance a global variable? It looks like it's only used in one method.

> threadingcache.cpp:62
> +    mAggregation = aggregation;
> +    QFile cacheFile(QStandardPaths::locate(QStandardPaths::CacheLocation,
> +                                           QStringLiteral("/messagelist/threading/%1").arg(id),

If locate() can't find the file, it will return an empty string, which will lead to

- a runtime warning from QFile about the filename being empty
- unusable debug output only showing empty strings

I recommend splitting the locate() and the QFile ctor -- and then you can remove the if !exists check, locate already checked that for you.
Instead, check for isEmpty on the string, and possibly add the relative path (/messagelist/threading/%1) to the debug output.

> threadingcache.cpp:73
> +    if (!cacheFile.open(QIODevice::ReadOnly)) {
> +        qCWarning(MESSAGELIST_LOG) << "Failed to open cache file:" << cacheFile.errorString();
> +        return;

This warning should also display the path to the file, so people can find out which dir they messed up permissions on (e.g.)

> threadingcache.cpp:77
> +
> +    const qint64 readSize = cacheFile.read((char *) &cacheHeader, sizeof(CacheHeader));
> +    if (readSize != sizeof(CacheHeader)) {

This isn't byte-order-independent or architecture-independent (32/64 bits), unlike QDataStream which you use for the rest. Not sure it's real problem though; in the unlikely event that someone copies his home dir from a 32 bit computer to a 64 bit computer, he should hit your "unknown version" code path, hopefully ;-). But maybe using QDataStream would be safer and more consistent?

> threadingcache.cpp:98
> +        // The cache is valid, but for a different grouping/threading configuration.
> +        qCDebug(MESSAGELIST_LOG) << "\tFailed: Threading mismatch";
> +        cacheFile.close();

s/Failed/Unusable cache/.  Failed in debug output sounds like a bug that has to be investigated, while this is a normal condition here.

> threadingcache.cpp:163
> +
> +    cacheFile.close();
> +}

The QFile destructor closes the file, so this line is unnecessary.

REPOSITORY
  rMESSAGELIB PIM: Message Library

REVISION DETAIL
  https://phabricator.kde.org/D1636

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dvratil, mlaurent, vkrause, dfaure
Cc: kde-pim, dvasin, winterz, smartins, vkrause, mlaurent, knauss, dvratil
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/