From 82190eaa81e6736cb924beb01671be768a921b05 Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Thu, 24 Mar 2016 17:42:04 +0100 Subject: [PATCH] Refactor the overlay icon logic to show errors as a warning for parent folders #3634 This also remove all smartness from the SocketApi about the status of a file and solely use info from the current and last sync. This simplifies the logic a lot and prevents any discrepancy between the status shown in the activity log and the one displayed on the overlay icon of a file. The main benefit of the additional simplicity is that we are able to push all new status of a file reliably (including warnings for parent folders) to properly update the icon on overlay implementations that don't allow us invalidating the status cache, like on OS X. Both errors and warning from the last sync are now kept in a set, which is used to also affect parent folders of an error. To make sure that errors don't become warning icons on a second sync, SyncFileItem::_hasBlacklistEntry is also interpreted as an error. This also renames StatusIgnore to StatusWarning to match this semantic. SyncEngine::aboutToPropagate is used in favor of SyncEngine::syncItemDiscovered since the latter is emitted before file permission warnings are set on the SyncFileItem. SyncEngine::finished is not used since we have all the needed information in SyncEngine::itemCompleted. --- src/libsync/syncengine.cpp | 29 +-- src/libsync/syncengine.h | 2 +- src/libsync/syncfilestatus.cpp | 3 +- src/libsync/syncfilestatus.h | 2 +- src/libsync/syncfilestatustracker.cpp | 275 ++++++++++---------------- src/libsync/syncfilestatustracker.h | 34 ++-- src/libsync/utility.cpp | 12 -- src/libsync/utility.h | 5 - 8 files changed, 128 insertions(+), 234 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 3e119face..a7f75b422 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -1306,33 +1306,14 @@ void SyncEngine::restoreOldFiles() } } -bool SyncEngine::estimateState(QString fn, csync_ftw_type_e t, SyncFileStatus* s) +SyncFileItem* SyncEngine::findSyncItem(const QString &fileName) const { - Q_UNUSED(t); - QString pat(fn); - if( t == CSYNC_FTW_TYPE_DIR && ! fn.endsWith(QLatin1Char('/'))) { - pat.append(QLatin1Char('/')); - } - Q_FOREACH(const SyncFileItemPtr &item, _syncedItems) { - //qDebug() << Q_FUNC_INFO << fn << item->_status << item->_file << fn.startsWith(item->_file) << item->_file.startsWith(fn); - - if (item->_file.startsWith(pat) || - item->_file == fn || item->_renameTarget == fn /* the same directory or file */) { - if (item->_status == SyncFileItem::NormalError - || item->_status == SyncFileItem::FatalError) - s->set(SyncFileStatus::StatusError); - else if (item->_status == SyncFileItem::FileIgnored) - s->set(SyncFileStatus::StatusIgnore); - else if (item->_status == SyncFileItem::Success) - s->set(SyncFileStatus::StatusUpToDate); - else - s->set(SyncFileStatus::StatusSync); - qDebug() << Q_FUNC_INFO << "Setting" << fn << "to" << s->toSocketAPIString(); - return true; - } + // 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)) + return item.data(); } - return false; + return 0; } qint64 SyncEngine::timeSinceFileTouched(const QString& fn) const diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index f78dad06b..934e0728c 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -85,7 +85,7 @@ public: /* Return true if we detected that another sync is needed to complete the sync */ bool isAnotherSyncNeeded() { return _anotherSyncNeeded; } - bool estimateState(QString fn, csync_ftw_type_e t, SyncFileStatus* s); + SyncFileItem* findSyncItem(const QString &fileName) const; /** Get the ms since a file was touched, or -1 if it wasn't. * diff --git a/src/libsync/syncfilestatus.cpp b/src/libsync/syncfilestatus.cpp index 6d234a3bd..2afd5c099 100644 --- a/src/libsync/syncfilestatus.cpp +++ b/src/libsync/syncfilestatus.cpp @@ -59,7 +59,8 @@ QString SyncFileStatus::toSocketAPIString() const case StatusSync: statusString = QLatin1String("SYNC"); break; - case StatusIgnore: + case StatusWarning: + // The protocol says IGNORE, but all implementations show a yellow warning sign. statusString = QLatin1String("IGNORE"); break; case StatusUpToDate: diff --git a/src/libsync/syncfilestatus.h b/src/libsync/syncfilestatus.h index 1af254f7a..fddcf1af0 100644 --- a/src/libsync/syncfilestatus.h +++ b/src/libsync/syncfilestatus.h @@ -30,7 +30,7 @@ public: enum SyncFileStatusTag { StatusNone, StatusSync, - StatusIgnore, + StatusWarning, StatusUpToDate, StatusError, }; diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp index 86da7b342..5ae8b9692 100644 --- a/src/libsync/syncfilestatustracker.cpp +++ b/src/libsync/syncfilestatustracker.cpp @@ -1,5 +1,6 @@ /* * Copyright (C) by Klaas Freitag + * Copyright (C) by Jocelyn Turcotte * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -12,17 +13,25 @@ */ #include "syncfilestatustracker.h" -#include "filesystem.h" #include "syncengine.h" #include "syncjournaldb.h" #include "syncjournalfilerecord.h" -#include "utility.h" - -#include -#include namespace OCC { +static SyncFileStatus::SyncFileStatusTag lookupProblem(const QString &pathToMatch, const std::set &set) +{ + for (auto it = set.cbegin(); it != set.cend(); ++it) { + qDebug() << Q_FUNC_INFO << pathToMatch << it->severity << it->path; + auto problemPath = it->path; + if (problemPath == pathToMatch) + return it->severity; + else if (it->severity == SyncFileStatus::StatusError && problemPath.startsWith(pathToMatch)) + return SyncFileStatus::StatusWarning; + } + return SyncFileStatus::StatusNone; +} + /** * Whether this item should get an ERROR icon through the Socket API. * @@ -31,229 +40,143 @@ namespace OCC { * icon as the problem is most likely going to resolve itself quickly and * automatically. */ -static bool showErrorInSocketApi(const SyncFileItem& item) +static inline bool showErrorInSocketApi(const SyncFileItem& item) { const auto status = item._status; return status == SyncFileItem::NormalError - || status == SyncFileItem::FatalError; + || status == SyncFileItem::FatalError + || item._hasBlacklistEntry; } -static void addErroredSyncItemPathsToList(const SyncFileItemVector& items, QSet* set) { - foreach (const SyncFileItemPtr &item, items) { - if (showErrorInSocketApi(*item)) { - set->insert(item->_file); - } - } +static inline bool showWarningInSocketApi(const SyncFileItem& item) +{ + const auto status = item._status; + return status == SyncFileItem::FileIgnored + || status == SyncFileItem::Conflict + || status == SyncFileItem::Restoration; } SyncFileStatusTracker::SyncFileStatusTracker(SyncEngine *syncEngine) : _syncEngine(syncEngine) { - connect(syncEngine, SIGNAL(treeWalkResult(const SyncFileItemVector&)), - this, SLOT(slotThreadTreeWalkResult(const SyncFileItemVector&))); connect(syncEngine, SIGNAL(aboutToPropagate(SyncFileItemVector&)), this, SLOT(slotAboutToPropagate(SyncFileItemVector&))); - connect(syncEngine, SIGNAL(finished(bool)), SLOT(slotSyncFinished())); connect(syncEngine, SIGNAL(itemCompleted(const SyncFileItem&, const PropagatorJob&)), this, SLOT(slotItemCompleted(const SyncFileItem&))); - connect(syncEngine, SIGNAL(syncItemDiscovered(const SyncFileItem&)), - this, SLOT(slotItemDiscovered(const SyncFileItem&))); -} - -bool SyncFileStatusTracker::estimateState(QString fn, csync_ftw_type_e t, SyncFileStatus* s) -{ - if (t == CSYNC_FTW_TYPE_DIR) { - if (Utility::doesSetContainPrefix(_stateLastSyncItemsWithError, fn)) { - qDebug() << Q_FUNC_INFO << "Folder has error" << fn; - s->set(SyncFileStatus::StatusError); - return true; - } - // If sync is running, check _syncedItems, possibly give it StatusSync - if (_syncEngine->isSyncRunning()) { - if (_syncEngine->estimateState(fn, t, s)) { - return true; - } - } - return false; - } else if ( t== CSYNC_FTW_TYPE_FILE) { - // check if errorList has the directory/file - if (Utility::doesSetContainPrefix(_stateLastSyncItemsWithError, fn)) { - s->set(SyncFileStatus::StatusError); - return true; - } - // If sync running: _syncedItems -> SyncingState - if (_syncEngine->isSyncRunning()) { - if (_syncEngine->estimateState(fn, t, s)) { - return true; - } - } - } - return false; } -/** - * Get status about a single file. - */ SyncFileStatus SyncFileStatusTracker::fileStatus(const QString& systemFileName) { - QString file = _syncEngine->localPath(); QString fileName = systemFileName.normalized(QString::NormalizationForm_C); - QString fileNameSlash = fileName; - - if(fileName != QLatin1String("/") && !fileName.isEmpty()) { - file += fileName; - } - if( fileName.endsWith(QLatin1Char('/')) ) { fileName.truncate(fileName.length()-1); qDebug() << "Removed trailing slash: " << fileName; - } else { - fileNameSlash += QLatin1Char('/'); } - const QFileInfo fi(file); - if( !FileSystem::fileExists(file, fi) ) { - qDebug() << "OO File " << file << " is not existing"; - return SyncFileStatus(SyncFileStatus::StatusError); - } + SyncFileItem* item = _syncEngine->findSyncItem(fileName); + if (item) + return fileStatus(*item); - // file is ignored? - // Qt considers .lnk files symlinks on Windows so we need to work - // around that here. - if( fi.isSymLink() -#ifdef Q_OS_WIN - && fi.suffix() != "lnk" -#endif - ) { - return SyncFileStatus(SyncFileStatus::StatusIgnore); - } + // 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()); - csync_ftw_type_e type = CSYNC_FTW_TYPE_FILE; - if( fi.isDir() ) { - type = CSYNC_FTW_TYPE_DIR; - } - - // Is it excluded? - if( _syncEngine->excludedFiles().isExcluded(file, _syncEngine->localPath(), _syncEngine->ignoreHiddenFiles()) ) { - return SyncFileStatus(SyncFileStatus::StatusIgnore); - } - - // Error if it is in the selective sync blacklist - foreach(const auto &s, _syncEngine->journal()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList)) { - if (fileNameSlash.startsWith(s)) { - return SyncFileStatus(SyncFileStatus::StatusError); - } - } - - SyncFileStatus status(SyncFileStatus::StatusNone); - SyncJournalFileRecord rec = _syncEngine->journal()->getFileRecord(fileName ); - - if (estimateState(fileName, type, &status)) { - qDebug() << "Folder estimated status for" << fileName << "to" << status.toSocketAPIString(); - } else if (fileName == "") { - // sync folder itself - // FIXME: The new parent folder logic should take over this, treating the root the same as any folder. - } else if (type == CSYNC_FTW_TYPE_DIR) { - if (rec.isValid()) { - status.set(SyncFileStatus::StatusUpToDate); - } else { - qDebug() << "Could not determine state for folder" << fileName << "will set StatusSync"; - status.set(SyncFileStatus::StatusSync); - } - } else if (type == CSYNC_FTW_TYPE_FILE) { - if (rec.isValid()) { - if( FileSystem::getModTime(fi.absoluteFilePath()) == Utility::qDateTimeToTime_t(rec._modtime) ) { - status.set(SyncFileStatus::StatusUpToDate); - } else { - if (rec._remotePerm.isNull() || rec._remotePerm.contains("W") ) { - status.set(SyncFileStatus::StatusSync); - } else { - status.set(SyncFileStatus::StatusError); - } - } - } else { - qDebug() << "Could not determine state for file" << fileName << "will set StatusSync"; - status.set(SyncFileStatus::StatusSync); - } - } - - if (rec.isValid() && rec._remotePerm.contains("S")) - status.setSharedWithMe(true); - - // FIXME: Wrong, but will go away - if (status.tag() == SyncFileStatus::StatusSync) { - // check the parent folder if it is shared and if it is allowed to create a file/dir within - QDir d( fi.path() ); - auto parentPath = d.path(); - auto dirRec = _syncEngine->journal()->getFileRecord(parentPath); - bool isDir = type == CSYNC_FTW_TYPE_DIR; - while( !d.isRoot() && !(d.exists() && dirRec.isValid()) ) { - d.cdUp(); // returns true if the dir exists. - - parentPath = d.path(); - // cut the folder path - dirRec = _syncEngine->journal()->getFileRecord(parentPath); - - isDir = true; - } - if( dirRec.isValid() && !dirRec._remotePerm.isNull()) { - if( (isDir && !dirRec._remotePerm.contains("K")) - || (!isDir && !dirRec._remotePerm.contains("C")) ) { - status.set(SyncFileStatus::StatusError); - } - } - } - return status; -} - -void SyncFileStatusTracker::slotThreadTreeWalkResult(const SyncFileItemVector& items) -{ - addErroredSyncItemPathsToList(items, &_stateLastSyncItemsWithErrorNew); + // Must be a new file, wait for the filesystem watcher to trigger a sync + return SyncFileStatus(); } void SyncFileStatusTracker::slotAboutToPropagate(SyncFileItemVector& items) { - addErroredSyncItemPathsToList(items, &_stateLastSyncItemsWithErrorNew); -} + std::set oldProblems; + std::swap(_syncProblems, oldProblems); -void SyncFileStatusTracker::slotSyncFinished() -{ - _stateLastSyncItemsWithError = _stateLastSyncItemsWithErrorNew; - _stateLastSyncItemsWithErrorNew.clear(); + foreach (const SyncFileItemPtr &item, items) { + qDebug() << Q_FUNC_INFO << "Investigating" << item->destination() << item->_status; + + if (showErrorInSocketApi(*item)) + _syncProblems.insert({item->_file, SyncFileStatus::StatusError}); + else if (showWarningInSocketApi(*item)) + _syncProblems.insert({item->_file, SyncFileStatus::StatusWarning}); + + QString systemFileName = _syncEngine->localPath() + item->destination(); + // the trailing slash for directories must be appended as the filenames coming in + // from the plugins have that too. Otherwise the matching entry item is not found + // in the plugin. + if( item->_type == SyncFileItem::Type::Directory ) + systemFileName += QLatin1Char('/'); + emit fileStatusChanged(systemFileName, fileStatus(*item)); + } + + // Make sure to push any status that might have been resolved indirectly since the last sync + // (like an error file being deleted from disk) + for (auto it = _syncProblems.begin(); it != _syncProblems.end(); ++it) + oldProblems.erase(*it); + for (auto it = oldProblems.begin(); it != oldProblems.end(); ++it) { + if (it->severity == SyncFileStatus::StatusError) + invalidateParentPaths(it->path); + emit fileStatusChanged(_syncEngine->localPath() + it->path, fileStatus(it->path)); + } } void SyncFileStatusTracker::slotItemCompleted(const SyncFileItem &item) { + qDebug() << Q_FUNC_INFO << item.destination() << item._status; + if (showErrorInSocketApi(item)) { - _stateLastSyncItemsWithErrorNew.insert(item._file); + _syncProblems.insert({item._file, SyncFileStatus::StatusError}); + invalidateParentPaths(item.destination()); + } else if (showWarningInSocketApi(item)) { + _syncProblems.insert({item._file, SyncFileStatus::StatusWarning}); + } 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()); } QString systemFileName = _syncEngine->localPath() + item.destination(); - // the trailing slash for directories must be appended as the filenames coming in // from the plugins have that too. Otherwise the matching entry item is not found // in the plugin. if( item._type == SyncFileItem::Type::Directory ) { systemFileName += QLatin1Char('/'); } - - auto status = fileStatus(item.destination()); - emit fileStatusChanged(systemFileName, status); + emit fileStatusChanged(systemFileName, fileStatus(item)); } -void SyncFileStatusTracker::slotItemDiscovered(const SyncFileItem &item) +SyncFileStatus SyncFileStatusTracker::fileStatus(const SyncFileItem& item) { - QString systemFileName = _syncEngine->localPath() + item.destination(); + // 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; - // the trailing slash for directories must be appended as the filenames coming in - // from the plugins have that too. Otherwise the matching entry item is not found - // in the plugin. - if( item._type == SyncFileItem::Type::Directory ) { - systemFileName += QLatin1Char('/'); + SyncFileStatus status(SyncFileStatus::StatusUpToDate); + if (waitingForPropagation) { + status.set(SyncFileStatus::StatusSync); + } else if (showErrorInSocketApi(item)) { + status.set(SyncFileStatus::StatusError); + } else if (showWarningInSocketApi(item)) { + status.set(SyncFileStatus::StatusWarning); + } else { + // After a sync finished, we need to show the users issues from that last sync like the activity list does. + // Also used for parent directories showing a warning for an error child. + SyncFileStatus::SyncFileStatusTag problemStatus = lookupProblem(item.destination(), _syncProblems); + if (problemStatus != SyncFileStatus::StatusNone) + status.set(problemStatus); } - emit fileStatusChanged(systemFileName, SyncFileStatus(SyncFileStatus::StatusSync)); + if (item._remotePerm.contains("S")) + status.setSharedWithMe(true); + + return status; +} + +void SyncFileStatusTracker::invalidateParentPaths(const QString& path) +{ + QStringList splitPath = path.split('/', QString::SkipEmptyParts); + for (int i = 0; i < splitPath.size(); ++i) { + QString parentPath = splitPath.mid(0, i).join('/'); + emit fileStatusChanged(_syncEngine->localPath() + parentPath, fileStatus(parentPath)); + } } } diff --git a/src/libsync/syncfilestatustracker.h b/src/libsync/syncfilestatustracker.h index 4ed067ff1..d7ed3deae 100644 --- a/src/libsync/syncfilestatustracker.h +++ b/src/libsync/syncfilestatustracker.h @@ -1,5 +1,6 @@ /* * Copyright (C) by Klaas Freitag + * Copyright (C) by Jocelyn Turcotte * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -17,38 +18,43 @@ #include "ownsql.h" #include "syncfileitem.h" #include "syncfilestatus.h" -#include -#include +#include namespace OCC { class SyncEngine; +struct Problem { + QString path; + SyncFileStatus::SyncFileStatusTag severity = SyncFileStatus::StatusError; + + Problem(const QString &path) : path(path) { } + Problem(const QString &path, SyncFileStatus::SyncFileStatusTag severity) : path(path), severity(severity) { } + + // Assume that each path will only have one severity, don't care about it in sorting + bool operator<(const Problem &other) const { return path < other.path; } + bool operator==(const Problem &other) const { return path == other.path; } +}; + class OWNCLOUDSYNC_EXPORT SyncFileStatusTracker : public QObject { Q_OBJECT public: - SyncFileStatusTracker(SyncEngine *syncEngine); + SyncFileStatusTracker(SyncEngine* syncEngine); SyncFileStatus fileStatus(const QString& systemFileName); signals: void fileStatusChanged(const QString& systemFileName, SyncFileStatus fileStatus); private slots: - void slotThreadTreeWalkResult(const SyncFileItemVector& items); void slotAboutToPropagate(SyncFileItemVector& items); - void slotSyncFinished(); - void slotItemCompleted(const SyncFileItem &item); - void slotItemDiscovered(const SyncFileItem &item); + void slotItemCompleted(const SyncFileItem& item); private: - bool estimateState(QString fn, csync_ftw_type_e t, SyncFileStatus* s); - - SyncEngine *_syncEngine; - // SocketAPI: Cache files and folders that had errors so that they can - // get a red ERROR icon. - QSet _stateLastSyncItemsWithErrorNew; // gets moved to _stateLastSyncItemsWithError at end of sync - QSet _stateLastSyncItemsWithError; + SyncFileStatus fileStatus(const SyncFileItem& item); + void invalidateParentPaths(const QString& path); + SyncEngine* _syncEngine; + std::set _syncProblems; }; } diff --git a/src/libsync/utility.cpp b/src/libsync/utility.cpp index 6fca88232..5fed2f92f 100644 --- a/src/libsync/utility.cpp +++ b/src/libsync/utility.cpp @@ -236,18 +236,6 @@ QString Utility::toCSyncScheme(const QString &urlStr) return url.toString(); } -bool Utility::doesSetContainPrefix(const QSet &l, const QString &p) { - - Q_FOREACH (const QString &setPath, l) { - //qDebug() << Q_FUNC_INFO << p << setPath << setPath.startsWith(p); - if (setPath.startsWith(p)) { - return true; - } - } - //qDebug() << "-> NOOOOO!!!" << p << l.count(); - return false; -} - QString Utility::escape(const QString &in) { #if QT_VERSION < QT_VERSION_CHECK(5, 0, 0) diff --git a/src/libsync/utility.h b/src/libsync/utility.h index 7aa31e357..b8e276b10 100644 --- a/src/libsync/utility.h +++ b/src/libsync/utility.h @@ -40,11 +40,6 @@ namespace Utility OWNCLOUDSYNC_EXPORT void setLaunchOnStartup(const QString &appName, const QString& guiName, bool launch); OWNCLOUDSYNC_EXPORT qint64 freeDiskSpace(const QString &path); OWNCLOUDSYNC_EXPORT QString toCSyncScheme(const QString &urlStr); - /** Like QLocale::toString(double, 'f', prec), but drops trailing zeros after the decimal point */ - - OWNCLOUDSYNC_EXPORT bool doesSetContainPrefix(const QSet &l, const QString &p); - - /** * @brief compactFormatDouble - formats a double value human readable.