[Kde-pim] Akonadi batch notifications review

[Daniel Vrátil]

On Saturday 18 of May 2013 13:12:26 Volker Krause wrote:
> Hi,
> 
> thanks a lot for implementing this! And sorry for taking so long with the
> review.

Hi,

thanks for reviewing the code. I've fixed the issues you mentioned and merged 
the branches in akonadi, kdepimlibs and kdepim-runtime to master.

Guys, if you see any regressions, please let me know asap :-)

> 
> On Monday 29 April 2013 21:28:41 Daniel Vrátil wrote:
<snip>
> > A bit of elaboration on the changes I've done.
> > 
> > Akonadi Server
> > Akonadi Server now uses and emits NotificationMessageV2. Unlike the "V1",
> > it can represent a single operation on multiple entities. Akonadi server
> > emits the NotificationMessageV2 via notifyV2() signal, and when there's
> > at least one subscriber that does not support the new version (support
> > for V2 is announced via client capabilities), it splits the V2
> > notification into multiple V1 notifications and emits them "the old way",
> > via notify(). A completely new operation called "ModifyFlags" has been
> > added. For this operation, the NotificationMessageV2 has two additional
> > attributes - addedFlags and removedFlags. When
> > Akonadi::AgentBase::ObserverV3 is used itemModified() will no longer
> > notify about flag changes (see below).
> Just some details, the overall approach looks good:
> DataStore::setItemFlags(): if the transaction is really needed there (and
> not already done by the caller), use the Transaction RAII class to ensure
> automatic rollback (there might be exceptions in there).

Dropped the transaction completely - it's called from a transaction in 
Storage::parseStream()

> 
> NotificationMessageV2::List should probably be a QVector, we couldn't change
> that in V1 anymore.

Done. Had to add NotificationMessageV2::appendAndCompress() for QList, because 
in Monitor we work with QQueue, which cannot be converted to non-const 
reference to QVector.

> 
> NotificationMessageV2::Item should probably be called "Entity" if it also
> can represent Collections.

Done

> 
> > Akonadi::Monitor
<snip>
> > Akonadi::AgentBase::ObserverV3
> > A new ObserverV3 has been added, with virtual method for every new batch
> > operation. Very important thing here is that if you use ObserverV3, you
> > will never be delivered the non-batch versions of notification from V1 or
> > V2. So even when ObserverV3::itemsRemoved() is not reimplemented in an
> > agent/resource, ObserverV2::itemRemoved() will simply not be called. This
> > is because of design limitation, that would require the Observer telling
> > back to Monitor to re-queue, split and re-emit the batch notification as
> > a set of per- item notifications.
> 
> Yep, keeping backward compatibility there is tricky, but I like your
> approach, the ObserverV3 constraints are fine IMHO.
> 
> We can look into how to simplify all this for KDE5 then, is there a use-case
> for keeping the single object signals?

I agree we could merge all Observers to one and drop single-item singals. If 
users don't want to support batches in their resources, they can just use 
Q_FOREACH :-) 

> 
> One issue in the implementation:
> ResourceBase::itemsX(): these skip all changes if any of the items is
> invalid (empty RID), that seems a bit extreme, maybe just drop them from
> the list instead?

Fixed.


Cheers,
Dan

> 
> > IMAP resource
> > IMAP resource has been updated to support itemsMoved and itemsRemoved and
> > itemsFlagsChanged.
> 
> Looks good.
> 
> 
> Feel free to merge it, and thanks again for all the work :) If anyone
> happens to look for things to work on, we can now optimize many resources
> by making use of the new notifications :)
> 
> regards,
> Volker

-- 
Daniel Vrátil
Associate Software Engineer, KDE Desktop Team
Red Hat, Inc

GPG Key: 0xC59D614F6F4AE348
Fingerprint: 4EC1 86E3 C54E 0B39 5FDD B5FB C59D 614F 6F4A E348
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20130518/5edc6641/attachment.sig>
-------------- next part --------------
_______________________________________________
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/