[Kde-pim] ChangeRecorder's pipeline

[David Faure]

On Sunday 14 October 2012 03:11:10 David Faure wrote:
> Hi Volker,
> 
> 88d868d9a52f1d0 introduced "d->pipeline.enqueue(msg)" in
> ChangeRecorder::replayNext() (which is still there in another form).
> 
> But the msg isn't removed from d->pendingNotifications (until the final call
> to changeProcessed), so if saveNotifications is called this message will be
> saved duplicated, won't it?
> 
> The handling of these two lists in the Monitor base class seems sane to me:
> it's always pipeline = [A B C], pending = [D E] and stuff moves towards the
> left: A gets emitted when data is available, D moves in after C in
> dispatchNotification, and saving at that point would lead to [B C D E], all
> fine there.
> 
> But in ChangeRecorder the items get duplicated into the pipeline, and
> removed from the pipeline when data is available, and later on removed from
> pendingNotifications when changeProcessed() is called. It took me 2 hours
> to find this :-}    (more details below)
> 
> It's almost fine to "abuse" the two lists that way, but I think
> saveNotifications() should then skip the pipeline when called from a
> ChangeRecorder, to avoid saving duplicates.
> However, in addition, this makes things harder for any shared code that uses
> pipeline and pendingNotifications --- like what I plan to do, to speed up
> saving.

Wait -- at that point my thinking took a wrong direction.
Saving only happens in ChangeRecorder, there's no saving in a pure Monitor.
[nor in a ChangeRecorder with change recording disabled, right?].

So the fix is a lot simpler...

1) don't save the pipeline, its only item is always at the "head" of savingNotifications.

--- a/akonadi/changerecorder_p.cpp
+++ b/akonadi/changerecorder_p.cpp
@@ -296,12 +296,7 @@ void ChangeRecorderPrivate::saveTo(QIODevice *device)
   QDataStream stream( device );
   stream.setVersion( QDataStream::Qt_4_6 );
 
-  stream << (qulonglong)(pipeline.count() + pendingNotifications.count());
-
-  for ( int i = 0; i < pipeline.count(); ++i ) {
-    const NotificationMessage msg = pipeline.at( i );
-    addToStream( stream, msg );
-  }
+  stream << (qulonglong)(pendingNotifications.count());
 
   for ( int i = 0; i < pendingNotifications.count(); ++i ) {
     const NotificationMessage msg = pendingNotifications.at( i );


2) ensure that a ChangeRecorder with change recording disabled really
behaves like a pure Monitor, like the docu says, i.e. never saves notifications
to the file.

--- a/akonadi/changerecorder.cpp
+++ b/akonadi/changerecorder.cpp
@@ -38,7 +38,8 @@ ChangeRecorder::ChangeRecorder( ChangeRecorderPrivate *privateclass, QObject * p
 ChangeRecorder::~ChangeRecorder()
 {
   Q_D( ChangeRecorder );
-  d->saveNotifications();
+  if ( d->enableChangeRecording )
+    d->saveNotifications();
 }
 
 void ChangeRecorder::setConfig(QSettings * settings)
@@ -49,7 +50,8 @@ void ChangeRecorder::setConfig(QSettings * settings)
     Q_ASSERT( d->pendingNotifications.isEmpty() );
     d->loadNotifications();
   } else if ( d->settings ) {
-    d->saveNotifications();
+    if ( d->enableChangeRecording )
+      d->saveNotifications();
     d->settings = settings;
   }
 }



-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE, in particular KDE Frameworks 5

_______________________________________________
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/