From d72f7e91094d5cfa96629128e0a966aa8b45ed48 Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Jul 2021 17:30:48 +0200 Subject: [PATCH 1/3] Check result of setPinState() Signed-off-by: Felix Weilbach --- src/csync/vio/csync_vio_local_unix.cpp | 3 ++- src/gui/accountsettings.cpp | 8 ++++++-- src/gui/folder.cpp | 8 ++++++-- src/gui/socketapi.cpp | 8 ++++++-- src/libsync/propagatedownload.cpp | 12 +++++++++--- src/libsync/propagateremotemove.cpp | 4 +++- src/libsync/propagateupload.cpp | 4 +++- src/libsync/propagatorjobs.cpp | 4 +++- test/testsyncvirtualfiles.cpp | 12 ++++++------ test/testsyncxattr.cpp | 10 +++++----- 10 files changed, 49 insertions(+), 24 deletions(-) diff --git a/src/csync/vio/csync_vio_local_unix.cpp b/src/csync/vio/csync_vio_local_unix.cpp index b0e5957bb2..ed045b16e1 100644 --- a/src/csync/vio/csync_vio_local_unix.cpp +++ b/src/csync/vio/csync_vio_local_unix.cpp @@ -124,7 +124,8 @@ std::unique_ptr csync_vio_local_readdir(csync_vio_handle_t *h if (vfs) { // Directly modifies file_stat->type. // We can ignore the return value since we're done here anyway. - vfs->statTypeVirtualFile(file_stat.get(), &handle->path); + const auto result = vfs->statTypeVirtualFile(file_stat.get(), &handle->path); + Q_UNUSED(result); } return file_stat; diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 1df8d32518..43dab82a57 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -826,7 +826,9 @@ void AccountSettings::slotEnableVfsCurrentFolder() folder->setRootPinState(PinState::Unspecified); for (const auto &entry : oldBlacklist) { folder->journalDb()->schedulePathForRemoteDiscovery(entry); - folder->vfs().setPinState(entry, PinState::OnlineOnly); + if (!folder->vfs().setPinState(entry, PinState::OnlineOnly)) { + qCWarning(lcAccountSettings) << "Could not set pin state of" << entry << "to online only"; + } } folder->slotNextSyncFullLocalDiscovery(); @@ -932,7 +934,9 @@ void AccountSettings::slotSetSubFolderAvailability(Folder *folder, const QString Q_ASSERT(!path.endsWith('/')); // Update the pin state on all items - folder->vfs().setPinState(path, state); + if (!folder->vfs().setPinState(path, state)) { + qCWarning(lcAccountSettings) << "Could not set pin state of" << path << "to" << state; + } // Trigger sync folder->schedulePathForLocalDiscovery(path); diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index b5bc8007c8..cda9b0cb27 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -636,7 +636,9 @@ void Folder::implicitlyHydrateFile(const QString &relativepath) // (suffix-virtual file's pin state is stored at the hydrated path) const auto pin = _vfs->pinState(relativepath); if (pin && *pin == PinState::OnlineOnly) { - _vfs->setPinState(relativepath, PinState::Unspecified); + if (!_vfs->setPinState(relativepath, PinState::Unspecified)) { + qCWarning(lcFolder) << "Could not set pin state of" << relativepath << "to unspecified"; + } } // Add to local discovery @@ -675,7 +677,9 @@ void Folder::setVirtualFilesEnabled(bool enabled) void Folder::setRootPinState(PinState state) { - _vfs->setPinState(QString(), state); + if (!_vfs->setPinState(QString(), state)) { + qCWarning(lcFolder) << "Could not set root pin state of" << _definition.alias; + } // We don't actually need discovery, but it's important to recurse // into all folders, so the changes can be applied. diff --git a/src/gui/socketapi.cpp b/src/gui/socketapi.cpp index 79e1e54821..16b6188682 100644 --- a/src/gui/socketapi.cpp +++ b/src/gui/socketapi.cpp @@ -796,7 +796,9 @@ void SocketApi::command_MAKE_AVAILABLE_LOCALLY(const QString &filesArg, SocketLi continue; // Update the pin state on all items - data.folder->vfs().setPinState(data.folderRelativePath, PinState::AlwaysLocal); + if (!data.folder->vfs().setPinState(data.folderRelativePath, PinState::AlwaysLocal)) { + qCWarning(lcSocketApi) << "Could not set pin state of" << data.folderRelativePath << "to always local"; + } // Trigger sync data.folder->schedulePathForLocalDiscovery(data.folderRelativePath); @@ -815,7 +817,9 @@ void SocketApi::command_MAKE_ONLINE_ONLY(const QString &filesArg, SocketListener continue; // Update the pin state on all items - data.folder->vfs().setPinState(data.folderRelativePath, PinState::OnlineOnly); + if (!data.folder->vfs().setPinState(data.folderRelativePath, PinState::OnlineOnly)) { + qCWarning(lcSocketApi) << "Could not set pin state of" << data.folderRelativePath << "to online only"; + } // Trigger sync data.folder->schedulePathForLocalDiscovery(data.folderRelativePath); diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 285446ec1f..64773953a3 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1079,15 +1079,21 @@ void PropagateDownloadFile::downloadFinished() // Move the pin state to the new location auto pin = propagator()->_journal->internalPinStates().rawForPath(virtualFile.toUtf8()); if (pin && *pin != PinState::Inherited) { - vfs->setPinState(_item->_file, *pin); - vfs->setPinState(virtualFile, PinState::Inherited); + if (!vfs->setPinState(_item->_file, *pin)) { + qCWarning(lcPropagateDownload) << "Could not set pin state of" << _item->_file; + } + if (!vfs->setPinState(virtualFile, PinState::Inherited)) { + qCWarning(lcPropagateDownload) << "Could not set pin state of" << virtualFile << " to inherited"; + } } } // Ensure the pin state isn't contradictory auto pin = vfs->pinState(_item->_file); if (pin && *pin == PinState::OnlineOnly) - vfs->setPinState(_item->_file, PinState::Unspecified); + if (!vfs->setPinState(_item->_file, PinState::Unspecified)) { + qCWarning(lcPropagateDownload) << "Could not set pin state of" << _item->_file << "to unspecified"; + } } updateMetadata(isConflict); diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index e65a5d4aa0..edd29306cb 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -239,7 +239,9 @@ void PropagateRemoteMove::finalize() // Delete old db data. propagator()->_journal->deleteFileRecord(_item->_originalFile); - vfs->setPinState(_item->_originalFile, PinState::Inherited); + if (!vfs->setPinState(_item->_originalFile, PinState::Inherited)) { + qCWarning(lcPropagateRemoteMove) << "Could not set pin state of" << _item->_originalFile << "to inherited"; + } SyncFileItem newItem(*_item); newItem._type = _item->_type; diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index dc7fc766cc..3136603cd4 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -781,7 +781,9 @@ void PropagateUploadFileCommon::finalize() auto &vfs = propagator()->syncOptions()._vfs; const auto pin = vfs->pinState(_item->_file); if (pin && *pin == PinState::OnlineOnly) { - vfs->setPinState(_item->_file, PinState::Unspecified); + if (!vfs->setPinState(_item->_file, PinState::Unspecified)) { + qCWarning(lcPropagateUpload) << "Could not set pin state of" << _item->_file << "to unspecified"; + } } } diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index b651aec81d..bf21e5e19b 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -248,7 +248,9 @@ void PropagateLocalRename::start() auto &vfs = propagator()->syncOptions()._vfs; auto pinState = vfs->pinState(_item->_originalFile); - vfs->setPinState(_item->_originalFile, PinState::Inherited); + if (!vfs->setPinState(_item->_originalFile, PinState::Inherited)) { + qCWarning(lcPropagateLocalRename) << "Could not set pin state of" << _item->_originalFile << "to inherited"; + } const auto oldFile = _item->_file; diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp index f468885185..5505c1f264 100644 --- a/test/testsyncvirtualfiles.cpp +++ b/test/testsyncvirtualfiles.cpp @@ -1211,8 +1211,8 @@ private slots: QCOMPARE(*vfs->availability("local"), VfsItemAvailability::Mixed); QCOMPARE(*vfs->availability("online"), VfsItemAvailability::Mixed); - vfs->setPinState("local", PinState::AlwaysLocal); - vfs->setPinState("online", PinState::OnlineOnly); + QVERIFY(vfs->setPinState("local", PinState::AlwaysLocal)); + QVERIFY(vfs->setPinState("online", PinState::OnlineOnly)); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(*vfs->availability("online"), VfsItemAvailability::OnlineOnly); @@ -1293,13 +1293,13 @@ private slots: QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename" DVSUFFIX), PinState::OnlineOnly); // When a file is hydrated or dehydrated due to pin state it retains its pin state - vfs->setPinState("onlinerenamed2/file1rename" DVSUFFIX, PinState::AlwaysLocal); + QVERIFY(vfs->setPinState("onlinerenamed2/file1rename" DVSUFFIX, PinState::AlwaysLocal)); QVERIFY(fakeFolder.syncOnce()); QVERIFY(fakeFolder.currentLocalState().find("onlinerenamed2/file1rename")); QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename"), PinState::AlwaysLocal); - vfs->setPinState("onlinerenamed2", PinState::Unspecified); - vfs->setPinState("onlinerenamed2/file1rename", PinState::OnlineOnly); + QVERIFY(vfs->setPinState("onlinerenamed2", PinState::Unspecified)); + QVERIFY(vfs->setPinState("onlinerenamed2/file1rename", PinState::OnlineOnly)); QVERIFY(fakeFolder.syncOnce()); QVERIFY(fakeFolder.currentLocalState().find("onlinerenamed2/file1rename" DVSUFFIX)); QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename" DVSUFFIX), PinState::OnlineOnly); @@ -1376,7 +1376,7 @@ private slots: cleanup(); // Dehydrate - vfs->setPinState(QString(), PinState::OnlineOnly); + QVERIFY(vfs->setPinState(QString(), PinState::OnlineOnly)); QVERIFY(!fakeFolder.syncOnce()); QVERIFY(itemInstruction(completeSpy, "A/igno" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); diff --git a/test/testsyncxattr.cpp b/test/testsyncxattr.cpp index bfcba680d5..896d1171eb 100644 --- a/test/testsyncxattr.cpp +++ b/test/testsyncxattr.cpp @@ -956,8 +956,8 @@ private slots: QCOMPARE(*vfs->availability("local"), VfsItemAvailability::Mixed); QCOMPARE(*vfs->availability("online"), VfsItemAvailability::Mixed); - vfs->setPinState("local", PinState::AlwaysLocal); - vfs->setPinState("online", PinState::OnlineOnly); + QVERIFY(vfs->setPinState("local", PinState::AlwaysLocal)); + QVERIFY(vfs->setPinState("online", PinState::OnlineOnly)); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(*vfs->availability("online"), VfsItemAvailability::OnlineOnly); @@ -1037,13 +1037,13 @@ private slots: QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename"), PinState::OnlineOnly); // When a file is hydrated or dehydrated due to pin state it retains its pin state - vfs->setPinState("onlinerenamed2/file1rename", PinState::AlwaysLocal); + QVERIFY(vfs->setPinState("onlinerenamed2/file1rename", PinState::AlwaysLocal)); QVERIFY(fakeFolder.syncOnce()); QVERIFY(fakeFolder.currentLocalState().find("onlinerenamed2/file1rename")); QCOMPARE(*vfs->pinState("onlinerenamed2/file1rename"), PinState::AlwaysLocal); - vfs->setPinState("onlinerenamed2", PinState::Unspecified); - vfs->setPinState("onlinerenamed2/file1rename", PinState::OnlineOnly); + QVERIFY(vfs->setPinState("onlinerenamed2", PinState::Unspecified)); + QVERIFY(vfs->setPinState("onlinerenamed2/file1rename", PinState::OnlineOnly)); QVERIFY(fakeFolder.syncOnce()); XAVERIFY_VIRTUAL(fakeFolder, "onlinerenamed2/file1rename"); From acf6cc0527644c24f6182193e094dc97b6237a55 Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Jul 2021 17:41:02 +0200 Subject: [PATCH 2/3] Remove unused functions and variables Signed-off-by: Felix Weilbach --- src/gui/application.cpp | 3 +-- src/gui/folderman.cpp | 2 +- src/gui/userstatus.cpp | 17 ----------------- src/libsync/clientsideencryption.cpp | 4 ++-- test/CMakeLists.txt | 1 + test/testhelper.cpp | 10 ++++++++++ test/testhelper.h | 15 ++++----------- 7 files changed, 19 insertions(+), 33 deletions(-) create mode 100644 test/testhelper.cpp diff --git a/src/gui/application.cpp b/src/gui/application.cpp index bce4e9afea..511da75586 100644 --- a/src/gui/application.cpp +++ b/src/gui/application.cpp @@ -478,7 +478,6 @@ void Application::slotCrash() void Application::slotownCloudWizardDone(int res) { - AccountManager *accountMan = AccountManager::instance(); FolderMan *folderMan = FolderMan::instance(); // During the wizard, scheduling of new syncs is disabled @@ -491,7 +490,7 @@ void Application::slotownCloudWizardDone(int res) // If one account is configured: enable autostart #ifndef QT_DEBUG - bool shouldSetAutoStart = (accountMan->accounts().size() == 1); + bool shouldSetAutoStart = AccountManager::instance()->accounts().size() == 1; #else bool shouldSetAutoStart = false; #endif diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 7f7f499c06..9b0f297e95 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -1075,7 +1075,7 @@ Folder *FolderMan::addFolder(AccountState *accountState, const FolderDefinition // Migration: The first account that's configured for a local folder shall // be saved in a backwards-compatible way. const auto folderList = FolderMan::instance()->map(); - const auto oneAccountOnly = std::none_of(folderList.cbegin(), folderList.cend(), [this, folder](const auto *other) { + const auto oneAccountOnly = std::none_of(folderList.cbegin(), folderList.cend(), [folder](const auto *other) { return other != folder && other->cleanPath() == folder->cleanPath(); }); diff --git a/src/gui/userstatus.cpp b/src/gui/userstatus.cpp index a8f06e3a6c..55d929eeeb 100644 --- a/src/gui/userstatus.cpp +++ b/src/gui/userstatus.cpp @@ -45,23 +45,6 @@ namespace { // it matches _preDefinedStatus, otherwise the default is online (0) return preDefinedStatus.value(status.toLower(), UserStatus::Status::Online); } - - QString enumToString(UserStatus::Status status) - { - switch (status) { - case UserStatus::Status::Away: - return QObject::tr("Away"); - case UserStatus::Status::DoNotDisturb: - return QObject::tr("Do not disturb"); - case UserStatus::Status::Invisible: - case UserStatus::Status::Offline: - return QObject::tr("Offline"); - case UserStatus::Status::Online: - return QObject::tr("Online"); - } - - Q_UNREACHABLE(); - } } UserStatus::UserStatus(QObject *parent) diff --git a/src/libsync/clientsideencryption.cpp b/src/libsync/clientsideencryption.cpp index 2611108d02..6c1cfd2cd9 100644 --- a/src/libsync/clientsideencryption.cpp +++ b/src/libsync/clientsideencryption.cpp @@ -1009,7 +1009,7 @@ void ClientSideEncryption::writePrivateKey(const AccountPtr &account) job->setInsecureFallback(false); job->setKey(kck); job->setBinaryData(_privateKey); - connect(job, &WritePasswordJob::finished, [this](Job *incoming) { + connect(job, &WritePasswordJob::finished, [](Job *incoming) { Q_UNUSED(incoming); qCInfo(lcCse()) << "Private key stored in keychain"; }); @@ -1028,7 +1028,7 @@ void ClientSideEncryption::writeCertificate(const AccountPtr &account) job->setInsecureFallback(false); job->setKey(kck); job->setBinaryData(_certificate.toPem()); - connect(job, &WritePasswordJob::finished, [this](Job *incoming) { + connect(job, &WritePasswordJob::finished, [](Job *incoming) { Q_UNUSED(incoming); qCInfo(lcCse()) << "Certificate stored in keychain"; }); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 511c33a3e3..b5272134ac 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -9,6 +9,7 @@ add_library(testutils syncenginetestutils.cpp pushnotificationstestutils.cpp themeutils.cpp + testhelper.cpp ) target_link_libraries(testutils PUBLIC ${APPLICATION_EXECUTABLE}sync Qt5::Test) diff --git a/test/testhelper.cpp b/test/testhelper.cpp new file mode 100644 index 0000000000..8ba0f151b2 --- /dev/null +++ b/test/testhelper.cpp @@ -0,0 +1,10 @@ +#include "testhelper.h" + +OCC::FolderDefinition folderDefinition(const QString &path) +{ + OCC::FolderDefinition d; + d.localPath = path; + d.targetPath = path; + d.alias = path; + return d; +} diff --git a/test/testhelper.h b/test/testhelper.h index 8bd5ca6f6b..5eaa2c5912 100644 --- a/test/testhelper.h +++ b/test/testhelper.h @@ -1,12 +1,11 @@ #ifndef TESTHELPER_H #define TESTHELPER_H -#include "folder.h" +#include "gui/folder.h" #include "creds/httpcredentials.h" -using namespace OCC; - -class HttpCredentialsTest : public HttpCredentials { +class HttpCredentialsTest : public OCC::HttpCredentials +{ public: HttpCredentialsTest(const QString& user, const QString& password) : HttpCredentials(user, password) @@ -17,12 +16,6 @@ public: } }; -static FolderDefinition folderDefinition(const QString &path) { - FolderDefinition d; - d.localPath = path; - d.targetPath = path; - d.alias = path; - return d; -} +OCC::FolderDefinition folderDefinition(const QString &path); #endif // TESTHELPER_H From ccd27870a75015f55cbd6ac170ff109dd2228dec Mon Sep 17 00:00:00 2001 From: Felix Weilbach Date: Fri, 23 Jul 2021 17:47:05 +0200 Subject: [PATCH 3/3] Don't compare integers with different signs Signed-off-by: Felix Weilbach --- src/csync/vio/csync_vio_local_unix.cpp | 2 +- src/gui/tray/UserModel.cpp | 2 +- test/pushnotificationstestutils.cpp | 9 +++++---- test/pushnotificationstestutils.h | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/csync/vio/csync_vio_local_unix.cpp b/src/csync/vio/csync_vio_local_unix.cpp index ed045b16e1..c5e22abb3c 100644 --- a/src/csync/vio/csync_vio_local_unix.cpp +++ b/src/csync/vio/csync_vio_local_unix.cpp @@ -125,7 +125,7 @@ std::unique_ptr csync_vio_local_readdir(csync_vio_handle_t *h // Directly modifies file_stat->type. // We can ignore the return value since we're done here anyway. const auto result = vfs->statTypeVirtualFile(file_stat.get(), &handle->path); - Q_UNUSED(result); + Q_UNUSED(result) } return file_stat; diff --git a/src/gui/tray/UserModel.cpp b/src/gui/tray/UserModel.cpp index aa36c0d9ba..9073925719 100644 --- a/src/gui/tray/UserModel.cpp +++ b/src/gui/tray/UserModel.cpp @@ -79,7 +79,7 @@ void User::showDesktopNotification(const QString &title, const QString &message) } // after one hour, clear the gui log notification store - constexpr quint64 clearGuiLogInterval = 60 * 60 * 1000; + constexpr qint64 clearGuiLogInterval = 60 * 60 * 1000; if (_guiLogTimer.elapsed() > clearGuiLogInterval) { _notificationCache.clear(); } diff --git a/test/pushnotificationstestutils.cpp b/test/pushnotificationstestutils.cpp index 60727592f9..389d41dd7f 100644 --- a/test/pushnotificationstestutils.cpp +++ b/test/pushnotificationstestutils.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include "pushnotificationstestutils.h" @@ -116,15 +117,15 @@ uint32_t FakeWebSocketServer::textMessagesCount() const return _processTextMessageSpy->count(); } -QString FakeWebSocketServer::textMessage(uint32_t messageNumber) const +QString FakeWebSocketServer::textMessage(int messageNumber) const { - Q_ASSERT(messageNumber < _processTextMessageSpy->count()); + Q_ASSERT(0 <= messageNumber && messageNumber < _processTextMessageSpy->count()); return _processTextMessageSpy->at(messageNumber).at(1).toString(); } -QWebSocket *FakeWebSocketServer::socketForTextMessage(uint32_t messageNumber) const +QWebSocket *FakeWebSocketServer::socketForTextMessage(int messageNumber) const { - Q_ASSERT(messageNumber < _processTextMessageSpy->count()); + Q_ASSERT(0 <= messageNumber && messageNumber < _processTextMessageSpy->count()); return _processTextMessageSpy->at(messageNumber).at(0).value(); } diff --git a/test/pushnotificationstestutils.h b/test/pushnotificationstestutils.h index 12c14d1e57..f1eca050f9 100644 --- a/test/pushnotificationstestutils.h +++ b/test/pushnotificationstestutils.h @@ -40,9 +40,9 @@ public: uint32_t textMessagesCount() const; - QString textMessage(uint32_t messageNumber) const; + QString textMessage(int messageNumber) const; - QWebSocket *socketForTextMessage(uint32_t messageNumber) const; + QWebSocket *socketForTextMessage(int messageNumber) const; void clearTextMessages();