From b68a28de8d6a49af00266e6930a03441ad73c53f Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Wed, 18 Jan 2017 15:40:52 +0100 Subject: [PATCH] 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 | 51 +++++++++++++++------------ src/libsync/syncfilestatustracker.h | 12 ++++--- test/testsyncfilestatustracker.cpp | 40 +++++++++++++++++++++ 3 files changed, 76 insertions(+), 27 deletions(-) diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp index 4eab8a469..979697486 100644 --- 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 16848fd74..4858944b4 100644 --- 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 _syncProblems; QSet _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 _syncCount; }; diff --git a/test/testsyncfilestatustracker.cpp b/test/testsyncfilestatustracker.cpp index cb66ad3a8..bcb6ae205 100644 --- 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);