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

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)
+        SyncFileStatus status = sharedFlag == UnknownShared
-            emit fileStatusChanged(getSystemDestination(relativePath), fileStatus(relativePath));
+            ? 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)
+        SyncFileStatus status = sharedFlag == UnknownShared
-            emit fileStatusChanged(getSystemDestination(relativePath), fileStatus(relativePath));
+            ? 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,
+        SharedFlag sharedFlag = item->_remotePerm.contains("S") ? Shared : NotShared;
-        // 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.
         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;

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 sharedState, PathKnownFlag isPathKnown = PathKnown);
-    SyncFileStatus resolveSyncAndErrorStatus(const QString &relativePath, SharedFlag isShared, PathKnownFlag isPathKnown = PathKnown);
 
     void invalidateParentPaths(const QString& path);
     QString getSystemDestination(const QString& relativePath);
-    void incSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange);
+    void incSyncCountAndEmitStatusChanged(const QString &relativePath, SharedFlag sharedState);
-    void decSyncCount(const QString &relativePath, EmitStatusChangeFlag emitStatusChange);
+    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;
 };
 

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);