From 4cfc30fc6feb12d209e33513317743351ac84574 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Tue, 17 Oct 2017 17:29:48 +0200 Subject: [PATCH 1/4] Sharing: remove the ShareManager::_jobContinuation It is growing indefinitively in case of error, causing a leak. Use a labda instead to pass the capture --- src/gui/sharemanager.cpp | 76 +++++++++++++--------------------------- src/gui/sharemanager.h | 3 -- 2 files changed, 24 insertions(+), 55 deletions(-) diff --git a/src/gui/sharemanager.cpp b/src/gui/sharemanager.cpp index 95a97645182..4ff75d3c524 100644 --- a/src/gui/sharemanager.cpp +++ b/src/gui/sharemanager.cpp @@ -21,17 +21,6 @@ #include #include -namespace { -struct CreateShare -{ - QString path; - OCC::Share::ShareType shareType; - QString shareWith; - OCC::Share::Permissions permissions; -}; -} // anonymous namespace -Q_DECLARE_METATYPE(CreateShare) - namespace OCC { Share::Share(AccountPtr account, @@ -267,51 +256,34 @@ void ShareManager::createShare(const QString &path, const Share::Permissions permissions) { auto job = new OcsShareJob(_account); - - // Store values that we need for creating this share later. - CreateShare continuation; - continuation.path = path; - continuation.shareType = shareType; - continuation.shareWith = shareWith; - continuation.permissions = permissions; - _jobContinuation[job] = QVariant::fromValue(continuation); - - connect(job, &OcsShareJob::shareJobFinished, this, &ShareManager::slotCreateShare); connect(job, &OcsJob::ocsError, this, &ShareManager::slotOcsError); + connect(job, &OcsShareJob::shareJobFinished, this, + [=](const QJsonDocument &reply) { + // Find existing share permissions (if this was shared with us) + Share::Permissions existingPermissions = SharePermissionDefault; + foreach (const QJsonValue &element, reply.object()["ocs"].toObject()["data"].toArray()) { + auto map = element.toObject(); + if (map["file_target"] == path) + existingPermissions = Share::Permissions(map["permissions"].toInt()); + } + + // Limit the permissions we request for a share to the ones the item + // was shared with initially. + auto perm = permissions; + if (permissions == SharePermissionDefault) { + perm = existingPermissions; + } else if (existingPermissions != SharePermissionDefault) { + perm &= existingPermissions; + } + + OcsShareJob *job = new OcsShareJob(_account); + connect(job, &OcsShareJob::shareJobFinished, this, &ShareManager::slotShareCreated); + connect(job, &OcsJob::ocsError, this, &ShareManager::slotOcsError); + job->createShare(path, shareType, shareWith, permissions); + }); job->getSharedWithMe(); } -void ShareManager::slotCreateShare(const QJsonDocument &reply) -{ - if (!_jobContinuation.contains(sender())) - return; - - CreateShare cont = _jobContinuation[sender()].value(); - if (cont.path.isEmpty()) - return; - _jobContinuation.remove(sender()); - - // Find existing share permissions (if this was shared with us) - Share::Permissions existingPermissions = SharePermissionDefault; - foreach (const QJsonValue &element, reply.object()["ocs"].toObject()["data"].toArray()) { - auto map = element.toObject(); - if (map["file_target"] == cont.path) - existingPermissions = Share::Permissions(map["permissions"].toInt()); - } - - // Limit the permissions we request for a share to the ones the item - // was shared with initially. - if (cont.permissions == SharePermissionDefault) { - cont.permissions = existingPermissions; - } else if (existingPermissions != SharePermissionDefault) { - cont.permissions &= existingPermissions; - } - - OcsShareJob *job = new OcsShareJob(_account); - connect(job, &OcsShareJob::shareJobFinished, this, &ShareManager::slotShareCreated); - connect(job, &OcsJob::ocsError, this, &ShareManager::slotOcsError); - job->createShare(cont.path, cont.shareType, cont.shareWith, cont.permissions); -} void ShareManager::slotShareCreated(const QJsonDocument &reply) { diff --git a/src/gui/sharemanager.h b/src/gui/sharemanager.h index 8c361b73263..971b8810bac 100644 --- a/src/gui/sharemanager.h +++ b/src/gui/sharemanager.h @@ -293,13 +293,10 @@ private slots: void slotLinkShareCreated(const QJsonDocument &reply); void slotShareCreated(const QJsonDocument &reply); void slotOcsError(int statusCode, const QString &message); - void slotCreateShare(const QJsonDocument &reply); - private: QSharedPointer parseLinkShare(const QJsonObject &data); QSharedPointer parseShare(const QJsonObject &data); - QMap _jobContinuation; AccountPtr _account; }; } From ab63984e33394b534f0bc82abb31f80722c8d577 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 23 Oct 2017 14:03:45 +0200 Subject: [PATCH 2/4] SyncEngine: remove SyncEngine::syncItemDiscovered It is unused. --- src/libsync/syncengine.cpp | 3 --- src/libsync/syncengine.h | 2 -- 2 files changed, 5 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index a54df4024e4..c66f5d444c7 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -571,7 +571,6 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other, dir = SyncFileItem::None; // For directories, metadata-only updates will be done after all their files are propagated. if (!isDirectory) { - emit syncItemDiscovered(*item); // Update the database now already: New remote fileid or Etag or RemotePerm // Or for files that were detected as "resolved conflict". @@ -683,8 +682,6 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other, } _syncItemMap.insert(key, item); - - emit syncItemDiscovered(*item); return re; } diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index 44c8ca7a276..9e3723c0d7e 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -105,8 +105,6 @@ class OWNCLOUDSYNC_EXPORT SyncEngine : public QObject // During update, before reconcile void rootEtag(QString); - // before actual syncing (after update+reconcile) for each item - void syncItemDiscovered(const SyncFileItem &); // after the above signals. with the items that actually need propagating void aboutToPropagate(SyncFileItemVector &); From 7ec2f9cadf4de5cdb3c2f85a0b3353510c1d7b44 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 23 Oct 2017 16:35:35 +0200 Subject: [PATCH 3/4] SyncFileStatusTracker: Detect changed in the shared flag ... even if the file is not changed. We get an UPDATE_METADATA in that case, so make sure we let the SyncFileStatusTracker know about it. That means we need to filter out UPDATE_METADATA in the other listeners of this signal. Issue #6098 --- src/gui/folder.cpp | 5 +++++ src/libsync/syncengine.cpp | 3 +++ test/testsyncengine.cpp | 2 +- test/testsyncfilestatustracker.cpp | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index c61c4ea68a2..8b7dcd14987 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -841,6 +841,11 @@ void Folder::slotTransmissionProgress(const ProgressInfo &pi) // a item is completed: count the errors and forward to the ProgressDispatcher void Folder::slotItemCompleted(const SyncFileItemPtr &item) { + if (item->_instruction == CSYNC_INSTRUCTION_NONE || item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA) { + // We only care about the updates that deserve to be shown in the UI + return; + } + // add new directories or remove gone away dirs to the watcher if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_NEW) { FolderMan::instance()->addMonitorPath(alias(), path() + item->_file); diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index c66f5d444c7..11258764160 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -607,6 +607,9 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other, } _journal->setFileRecordMetadata(item->toSyncJournalFileRecordWithInode(filePath)); + + // This might have changed the shared flag, so we must notify SyncFileStatusTracker for example + emit itemCompleted(item); } else { // The local tree is walked first and doesn't have all the info from the server. // Update only outdated data from the disk. diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 591ca653fa1..b44978542b4 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -16,7 +16,7 @@ bool itemDidComplete(const QSignalSpy &spy, const QString &path) for(const QList &args : spy) { auto item = args[0].value(); if (item->destination() == path) - return true; + return item->_instruction != CSYNC_INSTRUCTION_NONE && item->_instruction != CSYNC_INSTRUCTION_UPDATE_METADATA; } return false; } diff --git a/test/testsyncfilestatustracker.cpp b/test/testsyncfilestatustracker.cpp index ed828857d4c..bbc80f386c7 100644 --- a/test/testsyncfilestatustracker.cpp +++ b/test/testsyncfilestatustracker.cpp @@ -436,6 +436,8 @@ private slots: fakeFolder.remoteModifier().appendByte("S/s1"); fakeFolder.remoteModifier().insert("B/b3"); fakeFolder.remoteModifier().find("B/b3")->extraDavProperties = "0"; + fakeFolder.remoteModifier().find("A/a1")->isShared = true; // becomes shared + fakeFolder.remoteModifier().find("A", true); // change the etags of the parent StatusPushSpy statusSpy(fakeFolder.syncEngine()); @@ -458,6 +460,7 @@ private slots: QCOMPARE(statusSpy.statusOf("S/s1"), sharedUpToDateStatus); QCOMPARE(statusSpy.statusOf("B/b1").shared(), false); QCOMPARE(statusSpy.statusOf("B/b3"), sharedUpToDateStatus); + QCOMPARE(statusSpy.statusOf("A/a1"), sharedUpToDateStatus); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } From 758c36e9cfb666553a603fd79e3fe67bbfdd3671 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 23 Oct 2017 19:08:46 +0200 Subject: [PATCH 4/4] ShareDialog: trigger a sync for folder affected by a change of sharing This allow the sync engine to query the new metadata and update the overlay icons. Note: we also need to invalidate the etags because the server does not change the etag of parent directories that see their share-types changed. Issue #6098 --- src/gui/sharemanager.cpp | 38 ++++++++++++++++++++++++++++++++++++++ src/gui/sharemanager.h | 2 ++ 2 files changed, 40 insertions(+) diff --git a/src/gui/sharemanager.cpp b/src/gui/sharemanager.cpp index 4ff75d3c524..9901b47cf16 100644 --- a/src/gui/sharemanager.cpp +++ b/src/gui/sharemanager.cpp @@ -15,6 +15,8 @@ #include "sharemanager.h" #include "ocssharejob.h" #include "account.h" +#include "folderman.h" +#include "accountstate.h" #include #include @@ -23,6 +25,31 @@ namespace OCC { +/** + * When a share is modified, we need to tell the folders so they can adjust overlay icons + */ +static void updateFolder(const AccountPtr &account, const QString &path) +{ + foreach (Folder *f, FolderMan::instance()->map()) { + if (f->accountState()->account() != account) + continue; + auto folderPath = f->remotePath(); + if (path.startsWith(folderPath) && (path == folderPath || folderPath.endsWith('/') || path[folderPath.size()] == '/')) { + // Workaround the fact that the server does not invalidate the etags of parent directories + // when something is shared. + auto relative = path.midRef(folderPath.size()); + if (relative.startsWith('/')) + relative = relative.mid(1); + f->journalDb()->avoidReadFromDbOnNextSync(relative.toString()); + + // Schedule a sync so it can update the remote permission flag and let the socket API + // know about the shared icon. + f->scheduleThisFolderSoon(); + } + } +} + + Share::Share(AccountPtr account, const QString &id, const QString &path, @@ -43,6 +70,11 @@ AccountPtr Share::account() const return _account; } +QString Share::path() const +{ + return _path; +} + QString Share::getId() const { return _id; @@ -88,6 +120,8 @@ void Share::deleteShare() void Share::slotDeleted() { emit shareDeleted(); + + updateFolder(_account, _path); } void Share::slotOcsError(int statusCode, const QString &message) @@ -247,6 +281,8 @@ void ShareManager::slotLinkShareCreated(const QJsonDocument &reply) QSharedPointer share(parseLinkShare(data)); emit linkShareCreated(share); + + updateFolder(_account, share->path()); } @@ -292,6 +328,8 @@ void ShareManager::slotShareCreated(const QJsonDocument &reply) QSharedPointer share(parseShare(data)); emit shareCreated(share); + + updateFolder(_account, share->path()); } void ShareManager::fetchShares(const QString &path) diff --git a/src/gui/sharemanager.h b/src/gui/sharemanager.h index 971b8810bac..2a08babe3cd 100644 --- a/src/gui/sharemanager.h +++ b/src/gui/sharemanager.h @@ -64,6 +64,8 @@ class Share : public QObject */ AccountPtr account() const; + QString path() const; + /* * Get the id */