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

[dfaure (David Faure)]

dfaure added inline comments.

INLINE COMMENTS

> model.cpp:761
>      // Sometimes the folders need to be resurrected...
> -    d->mStorageModel->prepareForScan();
> +    d
> +    ->mStorageModel->prepareForScan();

is there a newline after d here, or is phabricator on crack?

> model.cpp:768
> +    // cache so there's no point saving the current threading cache.
> +    if (d->mThreadingCache.threading() != Aggregation::NoThreading && !isReload) {
> +        d->mThreadingCache.save();

More generally this could say if (d->mThreadingCache.isEnabled() && !isReload)
This way the logic of when to enable/disable cache is in a single place (a bit below), and we never risk calling save() with the cache disabled. Alternatively you could add an early return in save if !mEnabled (which would also address my other comment).

> threadingcache.cpp:74
> +    if (!mCacheId.isEmpty()) {
> +        save();
> +    }

If you go to a folder with threading, then to one without threading (-> saving old folder, and then mEnabled=false), and then you destroy the ThreadingCache, this will save again the cache for the old folder, right? I think this should test for mEnabled.

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/