From 0469236d80567cec217ea1dfb15c2c732a6712fa Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Mon, 2 May 2016 16:33:17 +0200 Subject: [PATCH] Overlay icon fixes (#4765) * Fix the sync status push for parent directories #4682 As before, we rely on metadata-update SyncFileItem entries for parent directories to notify us that a directory contains files to propagate, and to know when all children were propagated through its itemCompleted signal. Those metadata SyncFileItems however have a None direction and we need to add a explicit directory check to show them as Sync. This fix also handles new files as well as existing ones, so no need to keep a separate logic for new files. * Fix the root item sync status #4682 Make sure that we push the new status when the status of the SyncEngine changed. SyncEngine::started comes a bit late, only when the propagation starts, although it's better in this case since child folders will only switch to Sync in aboutToPropagate. Also fix an issue with SyncEngine::findSyncItem when using an empty fileName; this would match and return the wrong item, even though not currently happening with the code since fileStatus won't call it with an empty fileName anymore. * Simplify the root status logic Go through fileStatus like other cases to make sure that all use cases go through the same code path. This also makes sure to use lookupProblem which will use lower_bound which is more efficient for larger sets of sync problems. This also fixes the issue with lookupProblem that prevented it to properly match an empty pathToMatch, caused by the fact that the problem map contains relative paths not starting with a slash. * Avoid a SyncFileStatusTracker private overload with the same name Having an overload as a private function in the same class makes the code harder to follow. Rename the private fileStatus to syncFileItemStatus. * [osx] Fix missing overlay icons on client startup Since the statuses are cached and that we can't invalidate the cache, sending NOP would need to be overwritten by the default OK status once the client successfully connected. But instead of remembering which files we NOPed, rather wait until we are ready to sync before sending the REGISTER_PATH message to the socket API client. It will also prevent the client from sending unnecessary RETRIEVE_FILE_STATUS requests. Also remove AccountState::canSync, since it does the same as isConnected and syncing is not an account responsibility. --- src/gui/accountstate.cpp | 8 +-- src/gui/accountstate.h | 4 +- src/gui/folder.cpp | 7 +- src/gui/folder.h | 3 +- src/gui/folderman.cpp | 17 ++++- src/gui/folderman.h | 1 + src/gui/socketapi.cpp | 31 +++++---- src/gui/socketapi.h | 1 + src/libsync/syncengine.cpp | 2 +- src/libsync/syncfilestatustracker.cpp | 98 ++++++++++----------------- src/libsync/syncfilestatustracker.h | 5 +- 11 files changed, 85 insertions(+), 92 deletions(-) diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index 36da88e30..924317d76 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -95,6 +95,9 @@ void AccountState::setState(State state) } else if (oldState == SignedOut && _state == Disconnected) { checkConnectivity(); } + if (oldState == Connected || _state == Connected) { + emit isConnectedChanged(); + } } // might not have changed but the underlying _connectionErrors might have @@ -149,11 +152,6 @@ bool AccountState::isConnectedOrTemporarilyUnavailable() const return isConnected() || _state == ServiceUnavailable; } -bool AccountState::canSync() const -{ - return isConnected(); -} - void AccountState::tagLastSuccessfullETagRequest() { _timeSinceLastETagCheck.restart(); diff --git a/src/gui/accountstate.h b/src/gui/accountstate.h index 6ba0add89..61489c30c 100644 --- a/src/gui/accountstate.h +++ b/src/gui/accountstate.h @@ -102,9 +102,6 @@ public: bool isConnected() const; bool isConnectedOrTemporarilyUnavailable() const; - /// Returns whether sync actions are allowed to run. - bool canSync() const; - /// Triggers a ping to the server to update state and /// connection status and errors. void checkConnectivity(); @@ -130,6 +127,7 @@ private: signals: void stateChanged(int state); + void isConnectedChanged(); protected Q_SLOTS: void slotConnectionValidatorResult(ConnectionValidator::Status status, const QStringList& errors); diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 6f4407d3f..584c0fbc6 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -96,6 +96,7 @@ Folder::Folder(const FolderDefinition& definition, if (!setIgnoredFiles()) qWarning("Could not read system exclude file"); + connect(_accountState.data(), SIGNAL(isConnectedChanged()), this, SIGNAL(canSyncChanged())); connect(_engine.data(), SIGNAL(rootEtag(QString)), this, SLOT(etagRetreivedFromSyncEngine(QString))); connect(_engine.data(), SIGNAL(treeWalkResult(const SyncFileItemVector&)), this, SLOT(slotThreadTreeWalkResult(const SyncFileItemVector&)), Qt::QueuedConnection); @@ -229,7 +230,7 @@ bool Folder::syncPaused() const bool Folder::canSync() const { - return !syncPaused() && accountState()->canSync(); + return !syncPaused() && accountState()->isConnected(); } void Folder::setSyncPaused( bool paused ) @@ -248,6 +249,7 @@ void Folder::setSyncPaused( bool paused ) } emit syncPausedChanged(this, paused); emit syncStateChange(); + emit canSyncChanged(); } void Folder::setSyncState(SyncResult::Status state) @@ -685,7 +687,8 @@ void Folder::wipe() QFile::remove( stateDbFile + "-wal" ); QFile::remove( stateDbFile + "-journal" ); - FolderMan::instance()->socketApi()->slotRegisterPath(alias()); + if (canSync()) + FolderMan::instance()->socketApi()->slotRegisterPath(alias()); } bool Folder::setIgnoredFiles() diff --git a/src/gui/folder.h b/src/gui/folder.h index ad2211ffa..79a27ad62 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -131,8 +131,6 @@ public: /** * Returns true when the folder may sync. - * - * !syncPaused() && accountState->canSync(). */ bool canSync() const; @@ -203,6 +201,7 @@ signals: void progressInfo(const ProgressInfo& progress); void newBigFolderDiscovered(const QString &); // A new folder bigger than the threshold was discovered void syncPausedChanged(Folder*, bool paused); + void canSyncChanged(); /** * Fires for each change inside this folder that wasn't caused diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 3bda86756..16b5a21bb 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -152,7 +152,8 @@ void FolderMan::registerFolderMonitor( Folder *folder ) } // register the folder with the socket API - _socketApi->slotRegisterPath(folder->alias()); + if (folder->canSync()) + _socketApi->slotRegisterPath(folder->alias()); } void FolderMan::addMonitorPath( const QString& alias, const QString& path ) @@ -422,6 +423,17 @@ void FolderMan::slotFolderSyncPaused( Folder *f, bool paused ) } } +void FolderMan::slotFolderCanSyncChanged() +{ + Folder *f = qobject_cast(sender()); + Q_ASSERT(f); + if (f->canSync()) { + _socketApi->slotRegisterPath(f->alias()); + } else { + _socketApi->slotUnregisterPath(f->alias()); + } +} + // this really terminates the current sync process // ie. no questions, no prisoners // csync still remains in a stable state, regardless of that. @@ -539,7 +551,7 @@ void FolderMan::slotAccountStateChanged() } QString accountName = accountState->account()->displayName(); - if (accountState->canSync()) { + if (accountState->isConnected()) { qDebug() << "Account" << accountName << "connected, scheduling its folders"; foreach (Folder *f, _folderMap.values()) { @@ -803,6 +815,7 @@ Folder* FolderMan::addFolderInternal(FolderDefinition folderDefinition, AccountS connect(folder, SIGNAL(syncFinished(SyncResult)), SLOT(slotFolderSyncFinished(SyncResult))); connect(folder, SIGNAL(syncStateChange()), SLOT(slotForwardFolderSyncStateChange())); connect(folder, SIGNAL(syncPausedChanged(Folder*,bool)), SLOT(slotFolderSyncPaused(Folder*,bool))); + connect(folder, SIGNAL(canSyncChanged()), SLOT(slotFolderCanSyncChanged())); connect(&folder->syncEngine().syncFileStatusTracker(), SIGNAL(fileStatusChanged(const QString &, SyncFileStatus)), _socketApi.data(), SLOT(slotFileStatusChanged(const QString &, SyncFileStatus))); connect(folder, SIGNAL(watchedFileChangedExternally(QString)), diff --git a/src/gui/folderman.h b/src/gui/folderman.h index e597e52d4..aaa4e2d1e 100644 --- a/src/gui/folderman.h +++ b/src/gui/folderman.h @@ -143,6 +143,7 @@ signals: public slots: void slotRemoveFolder( Folder* ); void slotFolderSyncPaused(Folder *, bool paused); + void slotFolderCanSyncChanged(); void slotFolderSyncStarted(); void slotFolderSyncFinished( const SyncResult& ); diff --git a/src/gui/socketapi.cpp b/src/gui/socketapi.cpp index 91887f003..9c88bab0c 100644 --- a/src/gui/socketapi.cpp +++ b/src/gui/socketapi.cpp @@ -140,8 +140,10 @@ void SocketApi::slotNewConnection() _listeners.append(socket); foreach( Folder *f, FolderMan::instance()->map() ) { - QString message = buildRegisterPathMessage(f->path()); - sendMessage(socket, message); + if (f->canSync()) { + QString message = buildRegisterPathMessage(f->path()); + sendMessage(socket, message); + } } } @@ -180,6 +182,10 @@ void SocketApi::slotReadSocket() void SocketApi::slotRegisterPath( const QString& alias ) { + // Make sure not to register twice to each connected client + if (_registeredAliases.contains(alias)) + return; + Folder *f = FolderMan::instance()->folder(alias); if (f) { QString message = buildRegisterPathMessage(f->path()); @@ -187,13 +193,20 @@ void SocketApi::slotRegisterPath( const QString& alias ) sendMessage(socket, message); } } + + _registeredAliases.insert(alias); } void SocketApi::slotUnregisterPath( const QString& alias ) { + if (!_registeredAliases.contains(alias)) + return; + Folder *f = FolderMan::instance()->folder(alias); if (f) broadcastMessage(QLatin1String("UNREGISTER_PATH"), f->path(), QString::null, true ); + + _registeredAliases.remove(alias); } void SocketApi::slotUpdateFolderView(Folder *f) @@ -274,8 +287,6 @@ void SocketApi::command_RETRIEVE_FOLDER_STATUS(const QString& argument, QIODevic void SocketApi::command_RETRIEVE_FILE_STATUS(const QString& argument, QIODevice* socket) { - const QString nopString("NOP"); - if( !socket ) { qDebug() << "No valid socket object."; return; @@ -288,18 +299,12 @@ void SocketApi::command_RETRIEVE_FILE_STATUS(const QString& argument, QIODevice* Folder* syncFolder = FolderMan::instance()->folderForPath( argument ); if (!syncFolder) { // this can happen in offline mode e.g.: nothing to worry about - statusString = nopString; + statusString = QLatin1String("NOP"); } else { const QString file = QDir::cleanPath(argument).mid(syncFolder->cleanPath().length()+1); + SyncFileStatus fileStatus = syncFolder->syncEngine().syncFileStatusTracker().fileStatus(file); - // future: Send more specific states for paused, disconnected etc. - if( syncFolder->syncPaused() || !syncFolder->accountState()->isConnected() ) { - statusString = nopString; - } else { - SyncFileStatus fileStatus = syncFolder->syncEngine().syncFileStatusTracker().fileStatus(file); - - statusString = fileStatus.toSocketAPIString(); - } + statusString = fileStatus.toSocketAPIString(); } const QString message = QLatin1String("STATUS:") % statusString % QLatin1Char(':') % QDir::toNativeSeparators(argument); diff --git a/src/gui/socketapi.h b/src/gui/socketapi.h index 763ca2616..2b0aab74c 100644 --- a/src/gui/socketapi.h +++ b/src/gui/socketapi.h @@ -77,6 +77,7 @@ private: Q_INVOKABLE void command_SHARE_MENU_TITLE(const QString& argument, QIODevice* socket); QString buildRegisterPathMessage(const QString& path); + QSet _registeredAliases; QList _listeners; SocketApiServer _localServer; }; diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index a826d7840..1126a516e 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -1341,7 +1341,7 @@ SyncFileItem* SyncEngine::findSyncItem(const QString &fileName) const { Q_FOREACH(const SyncFileItemPtr &item, _syncedItems) { // Directories will appear in this list as well, and will get their status set once all children have been propagated - if ((item->_file == fileName || item->_renameTarget == fileName)) + if ((item->_file == fileName || (!item->_renameTarget.isEmpty() && item->_renameTarget == fileName))) return item.data(); } return 0; diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp index 7867b1408..c32e4d664 100644 --- a/src/libsync/syncfilestatustracker.cpp +++ b/src/libsync/syncfilestatustracker.cpp @@ -28,14 +28,15 @@ static SyncFileStatus::SyncFileStatusTag lookupProblem(const QString &pathToMatc // qDebug() << Q_FUNC_INFO << pathToMatch << severity << problemPath; if (problemPath == pathToMatch) { return severity; - } else if (severity == SyncFileStatus::StatusError && problemPath.startsWith(pathToMatch) && problemPath.at(pathToMatch.size()) == '/') { - Q_ASSERT(!pathToMatch.endsWith('/')); + } else if (severity == SyncFileStatus::StatusError + && problemPath.startsWith(pathToMatch) + && (pathToMatch.isEmpty() || problemPath.at(pathToMatch.size()) == '/')) { return SyncFileStatus::StatusWarning; } else if (!problemPath.startsWith(pathToMatch)) { // Starting at lower_bound we get the first path that is not smaller, - // since: "/a/" < "/a/aa" < "/a/aa/aaa" < "/a/ab/aba" - // If problemMap keys are ["/a/aa/aaa", "/a/ab/aba"] and pathToMatch == "/a/aa", - // lower_bound(pathToMatch) will point to "/a/aa/aaa", and the moment that + // since: "a/" < "a/aa" < "a/aa/aaa" < "a/ab/aba" + // If problemMap keys are ["a/aa/aaa", "a/ab/aba"] and pathToMatch == "a/aa", + // lower_bound(pathToMatch) will point to "a/aa/aaa", and the moment that // problemPath.startsWith(pathToMatch) == false, we know that we've looked // at everything that interest us. break; @@ -68,56 +69,27 @@ static inline bool showWarningInSocketApi(const SyncFileItem& item) || status == SyncFileItem::Restoration; } -static inline bool showSyncInSocketApi( const SyncFileItem& item) -{ - const auto inst = item._instruction; - return inst == CSYNC_INSTRUCTION_NEW; -} - SyncFileStatusTracker::SyncFileStatusTracker(SyncEngine *syncEngine) : _syncEngine(syncEngine) { connect(syncEngine, SIGNAL(aboutToPropagate(SyncFileItemVector&)), - this, SLOT(slotAboutToPropagate(SyncFileItemVector&))); + SLOT(slotAboutToPropagate(SyncFileItemVector&))); connect(syncEngine, SIGNAL(itemCompleted(const SyncFileItem&, const PropagatorJob&)), - this, SLOT(slotItemCompleted(const SyncFileItem&))); - connect(syncEngine, SIGNAL(started()), - SLOT(slotClearDirtyPaths())); + SLOT(slotItemCompleted(const SyncFileItem&))); + connect(syncEngine, SIGNAL(started()), SLOT(slotClearDirtyPaths())); + connect(syncEngine, SIGNAL(started()), SLOT(slotSyncEngineRunningChanged())); + connect(syncEngine, SIGNAL(finished(bool)), SLOT(slotSyncEngineRunningChanged())); } -SyncFileStatus SyncFileStatusTracker::rootStatus() +SyncFileItem SyncFileStatusTracker::rootSyncFileItem() { - /* Possible values for the status: - enum SyncFileStatusTag { - StatusNone, - StatusSync, - StatusWarning, - StatusUpToDate, - StatusError, - }; - */ - SyncFileStatus status = SyncFileStatus::StatusUpToDate; - - if( !_syncEngine ) return SyncFileStatus::StatusNone; - - if( _syncEngine->isSyncRunning() ) { - status = SyncFileStatus::StatusSync; - } else { - // sync is not running. Check dirty list and _syncProblems - int errs = 0; - for (auto it = _syncProblems.begin(); it != _syncProblems.end(); ++it) { - if( it->second == SyncFileStatus::StatusError ) { - errs ++; - break; // stop if an error found at all. - } - } - if( errs ) { - status = SyncFileStatus::StatusWarning; // some files underneath had errors - } - // Only warnings do not change the root emblem away from ok. - } - return status; - + SyncFileItem fakeRootItem; + // It's is not entirely correct to use the sync's status as we'll show the root folder as + // syncing even though no child might end up being propagated, but will give us something + // better than always UpToDate for now. + fakeRootItem._status = _syncEngine->isSyncRunning() ? SyncFileItem::NoStatus : SyncFileItem::Success; + fakeRootItem._isDirectory = true; + return fakeRootItem; } SyncFileStatus SyncFileStatusTracker::fileStatus(const QString& systemFileName) @@ -129,10 +101,10 @@ SyncFileStatus SyncFileStatusTracker::fileStatus(const QString& systemFileName) } if( fileName.isEmpty() ) { - // this is the root sync folder. - return rootStatus(); - + // This is the root sync folder, it doesn't have an entry in the database and won't be walked by csync, so create one manually. + return syncFileItemStatus(rootSyncFileItem()); } + // The SyncEngine won't notify us at all for CSYNC_FILE_SILENTLY_EXCLUDED // and CSYNC_FILE_EXCLUDE_AND_REMOVE excludes. Even though it's possible // that the status of CSYNC_FILE_EXCLUDE_LIST excludes will change if the user @@ -150,13 +122,13 @@ SyncFileStatus SyncFileStatusTracker::fileStatus(const QString& systemFileName) SyncFileItem* item = _syncEngine->findSyncItem(fileName); if (item) { - return fileStatus(*item); + return syncFileItemStatus(*item); } // If we're not currently syncing that file, look it up in the database to know if it's shared SyncJournalFileRecord rec = _syncEngine->journal()->getFileRecord(fileName); if (rec.isValid()) { - return fileStatus(rec.toSyncFileItem()); + return syncFileItemStatus(rec.toSyncFileItem()); } // Must be a new file, wait for the filesystem watcher to trigger a sync return SyncFileStatus(); @@ -185,10 +157,8 @@ void SyncFileStatusTracker::slotAboutToPropagate(SyncFileItemVector& items) _syncProblems[item->_file] = SyncFileStatus::StatusError; } else if (showWarningInSocketApi(*item)) { _syncProblems[item->_file] = SyncFileStatus::StatusWarning; - } else if( showSyncInSocketApi(*item)) { - _syncProblems[item->_file] = SyncFileStatus::StatusSync; } - emit fileStatusChanged(getSystemDestination(*item), fileStatus(*item)); + emit fileStatusChanged(getSystemDestination(*item), syncFileItemStatus(*item)); } // Make sure to push any status that might have been resolved indirectly since the last sync @@ -213,15 +183,17 @@ void SyncFileStatusTracker::slotItemCompleted(const SyncFileItem &item) invalidateParentPaths(item.destination()); } else if (showWarningInSocketApi(item)) { _syncProblems[item._file] = SyncFileStatus::StatusWarning; - } else if (showSyncInSocketApi(item)) { - // new items that were in state sync can now be erased - _syncProblems.erase(item._file); } else { // There is currently no situation where an error status set during discovery/update is fixed by propagation. Q_ASSERT(_syncProblems.find(item._file) == _syncProblems.end()); } - emit fileStatusChanged(getSystemDestination(item), fileStatus(item)); + emit fileStatusChanged(getSystemDestination(item), syncFileItemStatus(item)); +} + +void SyncFileStatusTracker::slotSyncEngineRunningChanged() +{ + emit fileStatusChanged(_syncEngine->localPath(), syncFileItemStatus(rootSyncFileItem())); } void SyncFileStatusTracker::slotClearDirtyPaths() @@ -231,11 +203,13 @@ void SyncFileStatusTracker::slotClearDirtyPaths() _dirtyPaths.clear(); } -SyncFileStatus SyncFileStatusTracker::fileStatus(const SyncFileItem& item) +SyncFileStatus SyncFileStatusTracker::syncFileItemStatus(const SyncFileItem& item) { // Hack to know if the item was taken from the sync engine (Sync), or from the database (UpToDate) - bool waitingForPropagation = item._direction != SyncFileItem::None && item._status == SyncFileItem::NoStatus; - + // Mark any directory in the SyncEngine's items as syncing, this is currently how we mark parent directories + // of currently syncing items since the PropagateDirectory job will mark the directorie's SyncFileItem::_status as Success + // once all child jobs have been completed. + bool waitingForPropagation = (item._isDirectory || item._direction != SyncFileItem::None) && item._status == SyncFileItem::NoStatus; SyncFileStatus status(SyncFileStatus::StatusUpToDate); if (waitingForPropagation) { status.set(SyncFileStatus::StatusSync); diff --git a/src/libsync/syncfilestatustracker.h b/src/libsync/syncfilestatustracker.h index 7aa461799..4158d696d 100644 --- a/src/libsync/syncfilestatustracker.h +++ b/src/libsync/syncfilestatustracker.h @@ -46,11 +46,12 @@ signals: private slots: void slotAboutToPropagate(SyncFileItemVector& items); void slotItemCompleted(const SyncFileItem& item); + void slotSyncEngineRunningChanged(); void slotClearDirtyPaths(); private: - SyncFileStatus fileStatus(const SyncFileItem& item); - SyncFileStatus rootStatus(); + SyncFileStatus syncFileItemStatus(const SyncFileItem& item); + SyncFileItem rootSyncFileItem(); void invalidateParentPaths(const QString& path); QString getSystemDestination(const SyncFileItem& syncEnginePath);