StatusTracker: Emit OK for the last child before parents (#5467)

The current logic tried to avoid a DB lookup just to fetch whether
the file is shared or not since that info is already in the
SyncFileItem. The implementation would however need to decrease the
sync count for itself (and parents) before emitting the new status,
thus emitting the OK status for parents before that last child that
ended the propagation for that folder.

Change the implementation to achieve what we want: give the
possibility to decSyncCount to use a pre-fetched sharing state while
still doing the emission for all involved files. This ensures that
the leaf file also gets its status emitted before its parents.

Issue #4797

diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp
index 4eab8a469f91e3814d0885995c50f2b00183a30e..9796974864ff63d1323577f866d6136e3524fbbc 100644 (file)
--- a/src/libsync/syncfilestatustracker.cpp
+++ b/src/libsync/syncfilestatustracker.cpp
@@ -129,42 +129,46 @@ void SyncFileStatusTracker::slotPathTouched(const QString& fileName)
     emit fileStatusChanged(fileName, SyncFileStatus::StatusSync);
 }
 
-void SyncFileStatusTracker::incSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange)
+void SyncFileStatusTracker::incSyncCountAndEmitStatusChanged(const QString &relativePath, SharedFlag sharedFlag)
 {
     // Will return 0 (and increase to 1) if the path wasn't in the map yet
     int count = _syncCount[relativePath]++;
     if (!count) {
-        if (emitStatusChange)
-            emit fileStatusChanged(getSystemDestination(relativePath), fileStatus(relativePath));
+        SyncFileStatus status = sharedFlag == UnknownShared
+            ? fileStatus(relativePath)
+            : resolveSyncAndErrorStatus(relativePath, sharedFlag);
+        emit fileStatusChanged(getSystemDestination(relativePath), status);
 
         // We passed from OK to SYNC, increment the parent to keep it marked as
         // SYNC while we propagate ourselves and our own children.
         Q_ASSERT(!relativePath.endsWith('/'));
         int lastSlashIndex = relativePath.lastIndexOf('/');
         if (lastSlashIndex != -1)
-            incSyncCount(relativePath.left(lastSlashIndex), EmitStatusChange);
+            incSyncCountAndEmitStatusChanged(relativePath.left(lastSlashIndex), UnknownShared);
         else if (!relativePath.isEmpty())
-            incSyncCount(QString(), EmitStatusChange);
+            incSyncCountAndEmitStatusChanged(QString(), UnknownShared);
     }
 }
 
-void SyncFileStatusTracker::decSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange)
+void SyncFileStatusTracker::decSyncCountAndEmitStatusChanged(const QString &relativePath, SharedFlag sharedFlag)
 {
     int count = --_syncCount[relativePath];
     if (!count) {
         // Remove from the map, same as 0
         _syncCount.remove(relativePath);
 
-        if (emitStatusChange)
-            emit fileStatusChanged(getSystemDestination(relativePath), fileStatus(relativePath));
+        SyncFileStatus status = sharedFlag == UnknownShared
+            ? fileStatus(relativePath)
+            : resolveSyncAndErrorStatus(relativePath, sharedFlag);
+        emit fileStatusChanged(getSystemDestination(relativePath), status);
 
         // We passed from SYNC to OK, decrement our parent.
         Q_ASSERT(!relativePath.endsWith('/'));
         int lastSlashIndex = relativePath.lastIndexOf('/');
         if (lastSlashIndex != -1)
-            decSyncCount(relativePath.left(lastSlashIndex), EmitStatusChange);
+            decSyncCountAndEmitStatusChanged(relativePath.left(lastSlashIndex), UnknownShared);
         else if (!relativePath.isEmpty())
-            decSyncCount(QString(), EmitStatusChange);
+            decSyncCountAndEmitStatusChanged(QString(), UnknownShared);
     }
 }
 
@@ -177,6 +181,7 @@ void SyncFileStatusTracker::slotAboutToPropagate(SyncFileItemVector& items)
 
     foreach (const SyncFileItemPtr &item, items) {
         // qDebug() << Q_FUNC_INFO << "Investigating" << item->destination() << item->_status << item->_instruction;
+        _dirtyPaths.remove(item->destination());
 
         if (showErrorInSocketApi(*item)) {
             _syncProblems[item->_file] = SyncFileStatus::StatusError;
@@ -185,18 +190,16 @@ void SyncFileStatusTracker::slotAboutToPropagate(SyncFileItemVector& items)
             _syncProblems[item->_file] = SyncFileStatus::StatusWarning;
         }
 
-        // Mark this path as syncing for instructions that will result in propagation,
-        // but DontEmitStatusChange since we're going to emit for ourselves using the
-        // info in the SyncFileItem we received, parents will still be emit if needed.
+        SharedFlag sharedFlag = item->_remotePerm.contains("S") ? Shared : NotShared;
         if (item->_instruction != CSYNC_INSTRUCTION_NONE
             && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA
             && item->_instruction != CSYNC_INSTRUCTION_IGNORE
             && item->_instruction != CSYNC_INSTRUCTION_ERROR) {
-            incSyncCount(item->destination(), DontEmitStatusChange);
+            // Mark this path as syncing for instructions that will result in propagation.
+            incSyncCountAndEmitStatusChanged(item->destination(), sharedFlag);
+        } else {
+            emit fileStatusChanged(getSystemDestination(item->destination()), resolveSyncAndErrorStatus(item->destination(), sharedFlag));
         }
-
-        _dirtyPaths.remove(item->destination());
-        emit fileStatusChanged(getSystemDestination(item->destination()), resolveSyncAndErrorStatus(item->destination(), item->_remotePerm.contains("S") ? Shared : NotShared));
     }
 
     // Some metadata status won't trigger files to be synced, make sure that we
@@ -233,14 +236,16 @@ void SyncFileStatusTracker::slotItemCompleted(const SyncFileItem &item)
         _syncProblems.erase(item._file);
     }
 
-    // decSyncCount calls *must* be symetric with incSyncCount calls in slotAboutToPropagate
+    SharedFlag sharedFlag = item._remotePerm.contains("S") ? Shared : NotShared;
     if (item._instruction != CSYNC_INSTRUCTION_NONE
         && item._instruction != CSYNC_INSTRUCTION_UPDATE_METADATA
         && item._instruction != CSYNC_INSTRUCTION_IGNORE
         && item._instruction != CSYNC_INSTRUCTION_ERROR) {
-        decSyncCount(item.destination(), DontEmitStatusChange);
+        // decSyncCount calls *must* be symetric with incSyncCount calls in slotAboutToPropagate
+        decSyncCountAndEmitStatusChanged(item.destination(), sharedFlag);
+    } else {
+        emit fileStatusChanged(getSystemDestination(item.destination()), resolveSyncAndErrorStatus(item.destination(), sharedFlag));
     }
-    emit fileStatusChanged(getSystemDestination(item.destination()), resolveSyncAndErrorStatus(item.destination(), item._remotePerm.contains("S") ? Shared : NotShared));
 }
 
 void SyncFileStatusTracker::slotSyncFinished()
@@ -257,7 +262,7 @@ void SyncFileStatusTracker::slotSyncEngineRunningChanged()
     emit fileStatusChanged(getSystemDestination(QString()), resolveSyncAndErrorStatus(QString(), NotShared));
 }
 
-SyncFileStatus SyncFileStatusTracker::resolveSyncAndErrorStatus(const QString &relativePath, SharedFlag isShared, PathKnownFlag isPathKnown)
+SyncFileStatus SyncFileStatusTracker::resolveSyncAndErrorStatus(const QString &relativePath, SharedFlag sharedFlag, PathKnownFlag isPathKnown)
 {
     // If it's a new file and that we're not syncing it yet,
     // don't show any icon and wait for the filesystem watcher to trigger a sync.
@@ -272,7 +277,9 @@ SyncFileStatus SyncFileStatusTracker::resolveSyncAndErrorStatus(const QString &r
             status.set(problemStatus);
     }
 
-    if (isShared)
+    // The shared status needs to have been fetched from a SyncFileItem or the DB at this point.
+    Q_ASSERT(sharedFlag != UnknownShared);
+    if (sharedFlag == Shared)
         status.setSharedWithMe(true);
 
     return status;

diff --git a/src/libsync/syncfilestatustracker.h b/src/libsync/syncfilestatustracker.h
index 16848fd74b9dcfd99e68ea6a8c293c981e85ef45..4858944b45a34e29857ffe70760b64e06fe22e38 100644 (file)
--- a/src/libsync/syncfilestatustracker.h
+++ b/src/libsync/syncfilestatustracker.h
@@ -51,20 +51,22 @@ private slots:
     void slotSyncEngineRunningChanged();
 
 private:
-    enum SharedFlag { NotShared = 0, Shared };
+    enum SharedFlag { UnknownShared, NotShared, Shared };
     enum PathKnownFlag { PathUnknown = 0, PathKnown };
-    enum EmitStatusChangeFlag { DontEmitStatusChange = 0, EmitStatusChange };
-    SyncFileStatus resolveSyncAndErrorStatus(const QString &relativePath, SharedFlag isShared, PathKnownFlag isPathKnown = PathKnown);
+    SyncFileStatus resolveSyncAndErrorStatus(const QString &relativePath, SharedFlag sharedState, PathKnownFlag isPathKnown = PathKnown);
 
     void invalidateParentPaths(const QString& path);
     QString getSystemDestination(const QString& relativePath);
-    void incSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange);
-    void decSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange);
+    void incSyncCountAndEmitStatusChanged(const QString &relativePath, SharedFlag sharedState);
+    void decSyncCountAndEmitStatusChanged(const QString &relativePath, SharedFlag sharedState);
 
     SyncEngine* _syncEngine;
 
     std::map<QString, SyncFileStatus::SyncFileStatusTag> _syncProblems;
     QSet<QString> _dirtyPaths;
+    // Counts the number direct children currently being synced (has unfinished propagation jobs).
+    // We'll show a file/directory as SYNC as long as its sync count is > 0.
+    // A directory that starts/ends propagation will in turn increase/decrease its own parent by 1.
     QHash<QString, int> _syncCount;
 };
 

diff --git a/test/testsyncfilestatustracker.cpp b/test/testsyncfilestatustracker.cpp
index cb66ad3a8a25ad5761e67c32b58909268a32ced0..bcb6ae2058e0e0e11aef3f49fe9466bd0e1bdfbd 100644 (file)
--- a/test/testsyncfilestatustracker.cpp
+++ b/test/testsyncfilestatustracker.cpp
@@ -28,6 +28,24 @@ public:
         }
         return SyncFileStatus();
     }
+
+    bool statusEmittedBefore(const QString &firstPath, const QString &secondPath) const {
+        QFileInfo firstFile(_syncEngine.localPath(), firstPath);
+        QFileInfo secondFile(_syncEngine.localPath(), secondPath);
+        // Start from the end to get the latest status
+        int i = size() - 1;
+        for (; i >= 0; --i) {
+            if (QFileInfo(at(i)[0].toString()) == secondFile)
+                break;
+            else if (QFileInfo(at(i)[0].toString()) == firstFile)
+                return false;
+        }
+        for (; i >= 0; --i) {
+            if (QFileInfo(at(i)[0].toString()) == firstFile)
+                return true;
+        }
+        return false;
+    }
 };
 
 class TestSyncFileStatusTracker : public QObject
@@ -369,6 +387,28 @@ private slots:
         QCOMPARE(fakeFolder.syncEngine().syncFileStatusTracker().fileStatus("A/a"), SyncFileStatus(SyncFileStatus::StatusUpToDate));
     }
 
+    // Even for status pushes immediately following each other, macOS
+    // can sometimes have 1s delays between updates, so make sure that
+    // children are marked as OK before their parents do.
+    void childOKEmittedBeforeParent() {
+        FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+        fakeFolder.localModifier().appendByte("B/b1");
+        fakeFolder.remoteModifier().appendByte("C/c1");
+        StatusPushSpy statusSpy(fakeFolder.syncEngine());
+
+        fakeFolder.syncOnce();
+        verifyThatPushMatchesPull(fakeFolder, statusSpy);
+        QVERIFY(statusSpy.statusEmittedBefore("B/b1", "B"));
+        QVERIFY(statusSpy.statusEmittedBefore("C/c1", "C"));
+        QVERIFY(statusSpy.statusEmittedBefore("B", ""));
+        QVERIFY(statusSpy.statusEmittedBefore("C", ""));
+        QCOMPARE(statusSpy.statusOf(""), SyncFileStatus(SyncFileStatus::StatusUpToDate));
+        QCOMPARE(statusSpy.statusOf("B"), SyncFileStatus(SyncFileStatus::StatusUpToDate));
+        QCOMPARE(statusSpy.statusOf("B/b1"), SyncFileStatus(SyncFileStatus::StatusUpToDate));
+        QCOMPARE(statusSpy.statusOf("C"), SyncFileStatus(SyncFileStatus::StatusUpToDate));
+        QCOMPARE(statusSpy.statusOf("C/c1"), SyncFileStatus(SyncFileStatus::StatusUpToDate));
+    }
+
     void sharedStatus() {
         SyncFileStatus sharedUpToDateStatus(SyncFileStatus::StatusUpToDate);
         sharedUpToDateStatus.setSharedWithMe(true);