From 4e967846677909f81ae40211fb740510df5ba2c8 Mon Sep 17 00:00:00 2001 From: modSpike Date: Tue, 16 Sep 2025 11:14:06 +0200 Subject: [PATCH 01/23] new point --- src/gui/connectionvalidator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/connectionvalidator.cpp b/src/gui/connectionvalidator.cpp index a42beaaf2f8..d11d5496913 100644 --- a/src/gui/connectionvalidator.cpp +++ b/src/gui/connectionvalidator.cpp @@ -226,7 +226,7 @@ void ConnectionValidator::slotAuthFailed() if (job->reply()->error() == QNetworkReply::SslHandshakeFailedError) { _errors << job->errorStringParsingBody(); stat = NetworkInformation::instance()->isBehindCaptivePortal() ? CaptivePortal : SslError; - + // dc-150 credentials::stillValid always returns true. the logic here needs fix } else if (job->reply()->error() == QNetworkReply::AuthenticationRequiredError || !_account->credentials()->stillValid(job->reply())) { qCWarning(lcConnectionValidator) << "******** Password is wrong!" << job->reply()->error() << job; _errors << tr("The provided credentials are not correct"); From 5fa90c64cb9c479ddf9bfab389b52d277d41bf27 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 14:37:19 +0200 Subject: [PATCH 02/23] switch shared pointer to qpointer in connection validater and added nullptr checks --- src/gui/accountstate.cpp | 2 +- src/gui/connectionvalidator.cpp | 46 ++++++++++++++++++++++++++++----- src/gui/connectionvalidator.h | 7 ++--- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index bf1a40218b4..726a6cc7f10 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -311,7 +311,7 @@ void AccountState::checkConnectivity(bool blockJobs) // ======= here we setup a new ConnectionValidator - _connectionValidator = new ConnectionValidator(_account, this); + _connectionValidator = new ConnectionValidator(_account.get(), this); connect(_connectionValidator, &ConnectionValidator::connectionResult, this, &AccountState::slotConnectionValidatorResult); connect(_connectionValidator, &ConnectionValidator::sslErrors, this, [blockJobs, this](const QList &errors) { handleSslConnectionErrors(errors, blockJobs); }); diff --git a/src/gui/connectionvalidator.cpp b/src/gui/connectionvalidator.cpp index d11d5496913..bc50e002100 100644 --- a/src/gui/connectionvalidator.cpp +++ b/src/gui/connectionvalidator.cpp @@ -15,7 +15,6 @@ #include "gui/clientproxy.h" #include "gui/fetchserversettings.h" #include "gui/networkinformation.h" -#include "gui/tlserrordialog.h" #include "libsync/account.h" #include "libsync/creds/abstractcredentials.h" #include "libsync/networkjobs.h" @@ -42,7 +41,7 @@ namespace { }; } -ConnectionValidator::ConnectionValidator(AccountPtr account, QObject *parent) +ConnectionValidator::ConnectionValidator(Account *account, QObject *parent) : QObject(parent) , _account(account) , _checkServerJob(nullptr) @@ -112,9 +111,14 @@ void ConnectionValidator::systemProxyLookupDone(const QNetworkProxy &proxy) // The actual check void ConnectionValidator::slotCheckServerAndAuth() { - Q_ASSERT(_checkServerJob == nullptr); - auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account, _clearCookies, this); + if (!_account) { + qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted"; + reportResult(NotConfigured); + return; + } + Q_ASSERT(_checkServerJob == nullptr); + auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account->sharedFromThis(), _clearCookies, this); _checkServerJob = checkServerFactory.startJob(_account->url(), this); connect(_checkServerJob->reply()->manager(), &AccessManager::sslErrors, this, [this](QNetworkReply *reply, const QList &errors) { @@ -127,6 +131,12 @@ void ConnectionValidator::slotCheckServerAndAuth() void ConnectionValidator::checkServerJobFinished() { + if (!_account) { + qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted"; + reportResult(NotConfigured); + return; + } + if (_checkServerJob->success()) { const auto result = _checkServerJob->result().value(); @@ -169,6 +179,12 @@ void ConnectionValidator::checkServerJobFinished() void ConnectionValidator::statusFound(const QUrl &url, const QJsonObject &info) { + if (!_account) { + qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted"; + reportResult(NotConfigured); + return; + } + // status.php was found. qCInfo(lcConnectionValidator) << "** Application: ownCloud found: " << url << " with version " @@ -207,8 +223,14 @@ void ConnectionValidator::checkAuthentication() // simply GET the WebDAV root, will fail if credentials are wrong. // continue in slotAuthCheck here :-) + if (!_account) { + _errors << tr("No ownCloud account configured"); + reportResult(NotConfigured); + return; + } + // we explicitly use a legacy dav path here - auto *job = new PropfindJob(_account, _account->url(), Theme::instance()->webDavPath(), PropfindJob::Depth::Zero, this); + auto *job = new PropfindJob(_account->sharedFromThis(), _account->url(), Theme::instance()->webDavPath(), PropfindJob::Depth::Zero, this); job->setAuthenticationJob(true); // don't retry job->setTimeout(timeoutToUse()); job->setProperties({ QByteArrayLiteral("getlastmodified") }); @@ -220,6 +242,12 @@ void ConnectionValidator::checkAuthentication() // Lisa todo: I hit this once and nothing happened in the gui?! void ConnectionValidator::slotAuthFailed() { + if (!_account) { + qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted"; + reportResult(NotConfigured); + return; + } + auto job = qobject_cast(sender()); Status stat = Timeout; @@ -248,9 +276,15 @@ void ConnectionValidator::slotAuthFailed() void ConnectionValidator::slotAuthSuccess() { + if (!_account) { + qCWarning(lcConnectionValidator) << "Bailing out, Account had been deleted"; + reportResult(NotConfigured); + return; + } + _errors.clear(); if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) { - auto *fetchSetting = new FetchServerSettingsJob(_account, this); + auto *fetchSetting = new FetchServerSettingsJob(_account->sharedFromThis(), this); const auto unsupportedServerError = [this] { _errors.append({tr("The configured server for this client is too old."), tr("Please update to the latest server and restart the client.")}); }; diff --git a/src/gui/connectionvalidator.h b/src/gui/connectionvalidator.h index 5936fa57ebe..75c07c3e4e6 100644 --- a/src/gui/connectionvalidator.h +++ b/src/gui/connectionvalidator.h @@ -17,11 +17,11 @@ #include "gui/owncloudguilib.h" #include "common/chronoelapsedtimer.h" -#include "gui/guiutility.h" #include "libsync/accountfwd.h" #include #include +#include #include #include @@ -84,7 +84,7 @@ class OWNCLOUDGUI_EXPORT ConnectionValidator : public QObject { Q_OBJECT public: - explicit ConnectionValidator(AccountPtr account, QObject *parent); + explicit ConnectionValidator(Account *account, QObject *parent); ~ConnectionValidator() override; enum class ValidationMode { @@ -152,8 +152,9 @@ protected Q_SLOTS: void statusFound(const QUrl &url, const QJsonObject &info); void checkServerJobFinished(); + QPointer _account; + QStringList _errors; - AccountPtr _account; bool _clearCookies = false; Utility::ChronoElapsedTimer _duration; From 670b3aff4a7a34e8836d53f3803bbbe7352faac8 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 14:47:16 +0200 Subject: [PATCH 03/23] removed accessor for nam on abstract core job refactored CheckServerJobFactory to take a raw account pointer. Also removed the parent arg to the factory as it's fairly useless adjusted the parenting of the one-off nam created in the factory to be sure it is parented by the job, so it is cleaned up along with the job. minimal update to oauth to deal with the new factory interface --- src/gui/connectionvalidator.cpp | 2 +- src/gui/creds/oauth.cpp | 7 ++++++- src/libsync/abstractcorejob.cpp | 4 ++-- src/libsync/abstractcorejob.h | 6 ++---- .../networkjobs/checkserverjobfactory.cpp | 16 ++++++++++++---- src/libsync/networkjobs/checkserverjobfactory.h | 4 +++- 6 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/gui/connectionvalidator.cpp b/src/gui/connectionvalidator.cpp index bc50e002100..b252bb9c4ec 100644 --- a/src/gui/connectionvalidator.cpp +++ b/src/gui/connectionvalidator.cpp @@ -118,7 +118,7 @@ void ConnectionValidator::slotCheckServerAndAuth() } Q_ASSERT(_checkServerJob == nullptr); - auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account->sharedFromThis(), _clearCookies, this); + auto checkServerFactory = CheckServerJobFactory::createFromAccount(_account, _clearCookies); _checkServerJob = checkServerFactory.startJob(_account->url(), this); connect(_checkServerJob->reply()->manager(), &AccessManager::sslErrors, this, [this](QNetworkReply *reply, const QList &errors) { diff --git a/src/gui/creds/oauth.cpp b/src/gui/creds/oauth.cpp index b8ea8a190d8..3730777c2c3 100644 --- a/src/gui/creds/oauth.cpp +++ b/src/gui/creds/oauth.cpp @@ -489,9 +489,14 @@ void AccountBasedOAuth::startAuthentication() void AccountBasedOAuth::fetchWellKnown() { + if (!_account) { + qCWarning(lcOauth) << "Unable to fetch well known, account has been deleted"; + return; + } + qCDebug(lcOauth) << "starting CheckServerJob before fetching" << wellKnownPathC; - auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account, true, this).startJob(_serverUrl, this); + auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account.get(), true).startJob(_serverUrl, this); connect(checkServerJob, &CoreJob::finished, this, [checkServerJob, this]() { if (checkServerJob->success()) { diff --git a/src/libsync/abstractcorejob.cpp b/src/libsync/abstractcorejob.cpp index 9e6c87b57a4..f627a4571f8 100644 --- a/src/libsync/abstractcorejob.cpp +++ b/src/libsync/abstractcorejob.cpp @@ -26,10 +26,10 @@ AbstractCoreJobFactory::~AbstractCoreJobFactory() { } -QNetworkAccessManager *AbstractCoreJobFactory::nam() const +/*QNetworkAccessManager *AbstractCoreJobFactory::nam() const { return _nam; -} +}*/ void AbstractCoreJobFactory::setJobResult(CoreJob *job, const QVariant &result) { diff --git a/src/libsync/abstractcorejob.h b/src/libsync/abstractcorejob.h index aa0d4a98716..f2755902bff 100644 --- a/src/libsync/abstractcorejob.h +++ b/src/libsync/abstractcorejob.h @@ -127,7 +127,8 @@ class OWNCLOUDSYNC_EXPORT AbstractCoreJobFactory } protected: - [[nodiscard]] QNetworkAccessManager *nam() const; + + QNetworkAccessManager *_nam; /** * Set job result. Needed because the jobs' methods are protected, and this class is a friend of Job. @@ -141,8 +142,5 @@ class OWNCLOUDSYNC_EXPORT AbstractCoreJobFactory * @param errorMessage network error or other suitable error message */ static void setJobError(CoreJob *job, const QString &errorMessage); - -private: - QNetworkAccessManager *_nam; }; } diff --git a/src/libsync/networkjobs/checkserverjobfactory.cpp b/src/libsync/networkjobs/checkserverjobfactory.cpp index cf5961743e1..6b187adcaf9 100644 --- a/src/libsync/networkjobs/checkserverjobfactory.cpp +++ b/src/libsync/networkjobs/checkserverjobfactory.cpp @@ -68,13 +68,16 @@ QUrl CheckServerJobResult::serverUrl() const // could signal out "finished" with a copy of the result? // once "finished" it could delete the job, woopie do! // caller could keep an adapter around as member for re-use if needed, or hit and quit. -CheckServerJobFactory CheckServerJobFactory::createFromAccount(const AccountPtr &account, bool clearCookies, QObject *parent) +CheckServerJobFactory CheckServerJobFactory::createFromAccount(const Account *account, bool clearCookies) { + // this is horrid but we can't return an "empty" factory so just crash. + // TODO: the jobs need a lot of work. + Q_ASSERT(account); + // in order to receive all ssl erorrs we need a fresh QNam + // see startJob where we parent the nam to the job itself to ensure it is cleaned up asap auto nam = account->credentials()->createAccessManager(); nam->setCustomTrustedCaCertificates(account->approvedCerts()); - // todo: DC-150 this means the nam does not get deleted with the job but hangs around until the parent is gone? investigate! - nam->setParent(parent); // do we start with the old cookies or new if (!(clearCookies && Theme::instance()->connectionValidatorClearCookies())) { const auto newJar = account->accessManager()->ownCloudCookieJar()->clone(); @@ -94,7 +97,12 @@ CoreJob *CheckServerJobFactory::startJob(const QUrl &url, QObject *parent) req.setRawHeader(QByteArrayLiteral("OC-Connection-Validator"), QByteArrayLiteral("desktop")); req.setMaximumRedirectsAllowed(0); - auto job = new CheckServerCoreJob(nam()->get(req), parent); + auto job = new CheckServerCoreJob(_nam->get(req), parent); + // this looks shady, but for the check server jobs we create a new nam on every run. The old approach was to parent it to the value + // passed to the factory but in fact that was often the connection validator which lives for a really long time, and hence is not the + // best parent for that temp nam. + // setting the job as parent is a more reasonable choice but this is not a great place for it - + _nam->setParent(job); // make this handle maintenance mode - only if necessary /* QObject::connect(job->reply(), &QNetworkReply::redirected, job, [job] { diff --git a/src/libsync/networkjobs/checkserverjobfactory.h b/src/libsync/networkjobs/checkserverjobfactory.h index 3063f759172..c1b46538e46 100644 --- a/src/libsync/networkjobs/checkserverjobfactory.h +++ b/src/libsync/networkjobs/checkserverjobfactory.h @@ -45,8 +45,10 @@ class OWNCLOUDSYNC_EXPORT CheckServerJobFactory : public AbstractCoreJobFactory * clearCookies: Whether to clear the cookies before we start the CheckServerJob job * This option also depends on Theme::instance()->connectionValidatorClearCookies() */ - static CheckServerJobFactory createFromAccount(const AccountPtr &account, bool clearCookies, QObject *parent); + static CheckServerJobFactory createFromAccount(const Account *account, bool clearCookies); + // the parent arg should be a fallback - we should be deleting jobs explicitly as soon as they are finished + // also note we create a "one off" nam for each checkServerJob so that is parented by the job for ensured cleanup CoreJob *startJob(const QUrl &url, QObject *parent) override; }; From ebc2d1f55444f17fac7c2751f57f91130bafedc5 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 14:58:26 +0200 Subject: [PATCH 04/23] lots of small cleanups/updates/removing accessors that have no business existing. --- src/gui/folderstatusmodel.cpp | 2 +- src/libsync/abstractnetworkjob.cpp | 10 ++++------ src/libsync/abstractnetworkjob.h | 7 +------ src/libsync/account.cpp | 2 +- src/libsync/account.h | 2 +- src/libsync/discovery.cpp | 2 -- src/libsync/networkjobs/fetchuserinfojobfactory.cpp | 2 +- src/libsync/owncloudpropagator.cpp | 6 +++--- src/libsync/propagateremotemkdir.cpp | 3 ++- .../testconnectionvalidator.cpp | 2 +- test/testfolderman.cpp | 3 +-- 11 files changed, 16 insertions(+), 25 deletions(-) diff --git a/src/gui/folderstatusmodel.cpp b/src/gui/folderstatusmodel.cpp index 062fe961490..633b36d3cbf 100644 --- a/src/gui/folderstatusmodel.cpp +++ b/src/gui/folderstatusmodel.cpp @@ -25,8 +25,8 @@ #include #include #include +#include -#include using namespace std::chrono_literals; diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index b3fdeea451d..0d4e038eaeb 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -19,7 +19,6 @@ #include "common/asserts.h" #include "networkjobs.h" #include "account.h" -#include "owncloudpropagator.h" #include "httplogger.h" #include "creds/abstractcredentials.h" @@ -58,14 +57,14 @@ AbstractNetworkJob::AbstractNetworkJob(AccountPtr account, const QUrl &baseUrl, Q_ASSERT(baseUrl.isValid()); } -QUrl AbstractNetworkJob::baseUrl() const +/*QUrl AbstractNetworkJob::baseUrl() const { return _baseUrl; -} +}*/ QUrl AbstractNetworkJob::url() const { - return Utility::concatUrlPath(baseUrl(), path(), query()); + return Utility::concatUrlPath(_baseUrl, path(), query()); } @@ -423,8 +422,7 @@ QDebug operator<<(QDebug debug, const OCC::AbstractNetworkJob *job) { QDebugStateSaver saver(debug); debug.setAutoInsertSpaces(false); - debug << job->metaObject()->className() << "(" << job->account().data() << ", " << job->url().toDisplayString() - << ", " << job->_verb; + debug << job->metaObject()->className() << "(" << job->url().toDisplayString() << ", " << job->_verb; if (auto reply = job->_reply) { debug << ", Original-Request-ID: " << reply->request().rawHeader("Original-Request-ID") << ", X-Request-ID: " << reply->request().rawHeader("X-Request-ID"); diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index 9757cd80171..fab477aa475 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -56,14 +56,9 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject virtual void start(); - AccountPtr account() const { return _account; } + // todo: this is used by FolderWizardRemotePath - get rid of it then get rid of this accessor QString path() const { return _path; } - /* - * A base Url, for most of the jobs this will be the WebDAV entry point. - */ - QUrl baseUrl() const; - /* * The absolute url: baseUrl() + path() + query() */ diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index d7f53d50d54..9bfb3ba08eb 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -264,7 +264,7 @@ void Account::clearCookieJar() _am->setCookieJar(new CookieJar); } -AccessManager *Account::accessManager() +AccessManager *Account::accessManager() const { return _am; } diff --git a/src/libsync/account.h b/src/libsync/account.h index c44adac573b..295632b6f3e 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -210,7 +210,7 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject void clearCookieJar(); - AccessManager *accessManager(); + AccessManager *accessManager() const; JobQueue *jobQueue(); diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index e1f3cbd357e..d5f4d135060 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -14,7 +14,6 @@ #include "discovery.h" #include "csync.h" -#include "owncloudpropagator.h" #include "syncfileitem.h" #include "csync/csync_exclude.h" @@ -25,7 +24,6 @@ #include "libsync/theme.h" -#include #include #include diff --git a/src/libsync/networkjobs/fetchuserinfojobfactory.cpp b/src/libsync/networkjobs/fetchuserinfojobfactory.cpp index b1b98359868..14404dd7c21 100644 --- a/src/libsync/networkjobs/fetchuserinfojobfactory.cpp +++ b/src/libsync/networkjobs/fetchuserinfojobfactory.cpp @@ -53,7 +53,7 @@ CoreJob *FetchUserInfoJobFactory::startJob(const QUrl &url, QObject *parent) req.setAttribute(OCC::DontAddCredentialsAttribute, true); req.setAttribute(QNetworkRequest::AuthenticationReuseAttribute, QNetworkRequest::Manual); - auto *job = new CoreJob(nam()->get(req), parent); + auto *job = new CoreJob(_nam->get(req), parent); QObject::connect(job->reply(), &QNetworkReply::finished, job, [job] { const auto data = job->reply()->readAll(); diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index ebb0516e8b7..f8de0cad251 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -520,9 +520,9 @@ void OwncloudPropagator::start(SyncFileItemSet &&items) // since it would be done before the actual remove (issue #1845) // NOTE: Currently this means that we don't update those etag at all in this sync, // but it should not be a problem, they will be updated in the next sync. - for (auto &dir : directories) { - if (dir.second->item()->instruction() == CSYNC_INSTRUCTION_UPDATE_METADATA) { - dir.second->item()->setInstruction(CSYNC_INSTRUCTION_NONE); + for (auto &aDir : directories) { + if (aDir.second->item()->instruction() == CSYNC_INSTRUCTION_UPDATE_METADATA) { + aDir.second->item()->setInstruction(CSYNC_INSTRUCTION_NONE); _anotherSyncNeeded = true; } } diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index d497c6ec5e6..8aa8c1c982c 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -114,7 +114,8 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() _item->_fileId = _job->reply()->rawHeader("OC-FileId"); propagator()->_activeJobList.append(this); - auto propfindJob = new PropfindJob(_job->account(), _job->baseUrl(), _job->path(), PropfindJob::Depth::Zero, this); + auto propfindJob = + new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); propfindJob->setProperties({"http://owncloud.org/ns:permissions"}); connect(propfindJob, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &result) { propagator()->_activeJobList.removeOne(this); diff --git a/test/testconnectionvalidator/testconnectionvalidator.cpp b/test/testconnectionvalidator/testconnectionvalidator.cpp index 284229f5dee..2d82e8916c1 100644 --- a/test/testconnectionvalidator/testconnectionvalidator.cpp +++ b/test/testconnectionvalidator/testconnectionvalidator.cpp @@ -147,7 +147,7 @@ private Q_SLOTS: return nullptr; }); - ConnectionValidator val(fakeFolder.account(), nullptr); + ConnectionValidator val(fakeFolder.account().get(), nullptr); val.checkServer(ConnectionValidator::ValidationMode::ValidateAuthAndUpdate); QSignalSpy spy(&val, &ConnectionValidator::connectionResult); diff --git a/test/testfolderman.cpp b/test/testfolderman.cpp index 6edfafce1c5..e05420694d4 100644 --- a/test/testfolderman.cpp +++ b/test/testfolderman.cpp @@ -11,10 +11,9 @@ #include "common/utility.h" #include "folderman.h" -#include "account.h" #include "accountstate.h" -#include "configfile.h" +#include "guiutility.h" #include "testutils/testutils.h" #include From a82315a9e17923116344c945afc3faac3fdfbd8a Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 15:31:43 +0200 Subject: [PATCH 05/23] removed the oc10 related folderwizardremotepath and updated deps also cleaned up some additional accessors in the abstract network job --- src/gui/folderwizard/CMakeLists.txt | 1 - src/gui/folderwizard/folderwizard.cpp | 6 - src/gui/folderwizard/folderwizard_p.h | 6 +- .../folderwizard/folderwizardremotepath.cpp | 367 ------------------ src/gui/folderwizard/folderwizardremotepath.h | 80 ---- src/libsync/abstractnetworkjob.cpp | 7 +- src/libsync/abstractnetworkjob.h | 19 +- src/libsync/networkjobs/jsonjob.cpp | 2 +- 8 files changed, 13 insertions(+), 475 deletions(-) delete mode 100644 src/gui/folderwizard/folderwizardremotepath.cpp delete mode 100644 src/gui/folderwizard/folderwizardremotepath.h diff --git a/src/gui/folderwizard/CMakeLists.txt b/src/gui/folderwizard/CMakeLists.txt index 9bbfbb6d692..7f5be4d0e1a 100644 --- a/src/gui/folderwizard/CMakeLists.txt +++ b/src/gui/folderwizard/CMakeLists.txt @@ -3,7 +3,6 @@ add_library(folderwizard STATIC folderwizardlocalpath.cpp folderwizardsourcepage.ui - folderwizardremotepath.cpp folderwizardtargetpage.ui folderwizardselectivesync.cpp diff --git a/src/gui/folderwizard/folderwizard.cpp b/src/gui/folderwizard/folderwizard.cpp index 1cb08b26aa7..d1d516a0399 100644 --- a/src/gui/folderwizard/folderwizard.cpp +++ b/src/gui/folderwizard/folderwizard.cpp @@ -16,7 +16,6 @@ #include "folderwizard_p.h" #include "folderwizardlocalpath.h" -#include "folderwizardremotepath.h" #include "folderwizardselectivesync.h" #include "spacespage.h" @@ -101,11 +100,6 @@ QString FolderWizardPrivate::initialLocalPath() const defaultSyncRoot(), _spacesPage->currentSpace()->displayName(), FolderMan::NewFolderType::SpacesSyncRoot, _account->account()->uuid()); } -QString FolderWizardPrivate::remotePath() const -{ - return _folderWizardTargetPage ? _folderWizardTargetPage->targetPath() : QString(); -} - uint32_t FolderWizardPrivate::priority() const { return _spacesPage->currentSpace()->priority(); diff --git a/src/gui/folderwizard/folderwizard_p.h b/src/gui/folderwizard/folderwizard_p.h index 9c1730fa1fc..40341d41893 100644 --- a/src/gui/folderwizard/folderwizard_p.h +++ b/src/gui/folderwizard/folderwizard_p.h @@ -37,7 +37,10 @@ class FolderWizardPrivate QString initialLocalPath() const; - QString remotePath() const; + // todo: #44 this used to be calculated by the folderWizardRemotePath impl, which was oc10 specific and is not longer used + // However! the folder defition still wants this value so I'm leaving this dummy here for now (which is the value returned for non oc10 + // accounts), to be investigated later + QString remotePath() const { return QString(); } uint32_t priority() const; @@ -57,7 +60,6 @@ class FolderWizardPrivate QPointer _account; class SpacesPage *_spacesPage; class FolderWizardLocalPath *_folderWizardSourcePage = nullptr; - class FolderWizardRemotePath *_folderWizardTargetPage = nullptr; class FolderWizardSelectiveSync *_folderWizardSelectiveSyncPage = nullptr; }; diff --git a/src/gui/folderwizard/folderwizardremotepath.cpp b/src/gui/folderwizard/folderwizardremotepath.cpp deleted file mode 100644 index 0d80742b40e..00000000000 --- a/src/gui/folderwizard/folderwizardremotepath.cpp +++ /dev/null @@ -1,367 +0,0 @@ -/* - * Copyright (C) by Hannah von Reth - * Copyright (C) by Duncan Mac-Vicar P. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - */ -#include "folderwizardremotepath.h" -#include "ui_folderwizardtargetpage.h" - -#include "folderwizard_p.h" - -#include "gui/folderman.h" - -#include "libsync/theme.h" - -#include "resources/resources.h" - -#include -#include - -using namespace OCC; - -FolderWizardRemotePath::FolderWizardRemotePath(FolderWizardPrivate *parent) - : FolderWizardPage(parent) - , _ui(new Ui_FolderWizardTargetPage) - , _warnWasVisible(false) - -{ - _ui->setupUi(this); - _ui->warningIcon->setPixmap(Resources::getCoreIcon(QStringLiteral("warning")).pixmap(_ui->warningIcon->size())); - _ui->warningFrame->hide(); // This is `show()`n in `FolderWizardRemotePath::showWarn` - - _ui->folderTreeWidget->setSortingEnabled(true); - _ui->folderTreeWidget->sortByColumn(0, Qt::AscendingOrder); - - connect(_ui->addFolderButton, &QAbstractButton::clicked, this, &FolderWizardRemotePath::slotAddRemoteFolder); - connect(_ui->refreshButton, &QAbstractButton::clicked, this, &FolderWizardRemotePath::slotRefreshFolders); - connect(_ui->folderTreeWidget, &QTreeWidget::itemExpanded, this, &FolderWizardRemotePath::slotItemExpanded); - connect(_ui->folderTreeWidget, &QTreeWidget::currentItemChanged, this, &FolderWizardRemotePath::slotCurrentItemChanged); - connect(_ui->folderEntry, &QLineEdit::textEdited, this, &FolderWizardRemotePath::slotFolderEntryEdited); - - _lscolTimer.setInterval(500); - _lscolTimer.setSingleShot(true); - connect(&_lscolTimer, &QTimer::timeout, this, &FolderWizardRemotePath::slotLsColFolderEntry); - - _ui->folderTreeWidget->header()->setSectionResizeMode(0, QHeaderView::ResizeToContents); - // Make sure that there will be a scrollbar when the contents is too wide - _ui->folderTreeWidget->header()->setStretchLastSection(false); -} - -void FolderWizardRemotePath::slotAddRemoteFolder() -{ - QTreeWidgetItem *current = _ui->folderTreeWidget->currentItem(); - - QString parent(QLatin1Char('/')); - if (current) { - parent = current->data(0, Qt::UserRole).toString(); - } - - QInputDialog *dlg = new QInputDialog(this); - - dlg->setWindowTitle(tr("Create Remote Folder")); - dlg->setLabelText(tr("Enter the name of the new folder to be created below '%1':") - .arg(parent)); - dlg->open(this, SLOT(slotCreateRemoteFolder(QString))); - dlg->setAttribute(Qt::WA_DeleteOnClose); -} - -void FolderWizardRemotePath::slotCreateRemoteFolder(const QString &folder) -{ - if (folder.isEmpty()) - return; - - QTreeWidgetItem *current = _ui->folderTreeWidget->currentItem(); - QString fullPath; - if (current) { - fullPath = current->data(0, Qt::UserRole).toString(); - } - // clean user input - fullPath = QDir::cleanPath(QStringLiteral("%1/%2").arg(fullPath, folder)).replace(QRegularExpression(QStringLiteral("/+")), QStringLiteral("/")); - - - MkColJob *job = new MkColJob(folderWizardPrivate()->accountState()->account(), folderWizardPrivate()->davUrl(), fullPath, {}, this); - /* check the owncloud configuration file and query the ownCloud */ - connect(job, &MkColJob::finishedWithoutError, - this, &FolderWizardRemotePath::slotCreateRemoteFolderFinished); - connect(job, &AbstractNetworkJob::networkError, this, &FolderWizardRemotePath::slotHandleMkdirNetworkError); - job->start(); -} - -void FolderWizardRemotePath::slotCreateRemoteFolderFinished() -{ - qCDebug(lcFolderWizard) << "webdav mkdir request finished"; - showWarn(tr("Folder was successfully created on %1.").arg(Theme::instance()->appNameGUI())); - slotRefreshFolders(); - _ui->folderEntry->setText(static_cast(sender())->path()); - slotLsColFolderEntry(); -} - -void FolderWizardRemotePath::slotHandleMkdirNetworkError(QNetworkReply *reply) -{ - qCWarning(lcFolderWizard) << "webdav mkdir request failed:" << reply->error(); - if (!folderWizardPrivate()->accountState()->account()->credentials()->stillValid(reply)) { - showWarn(tr("Authentication failed accessing %1").arg(Theme::instance()->appNameGUI())); - } else { - showWarn(tr("Failed to create the folder on %1. Please check manually.") - .arg(Theme::instance()->appNameGUI())); - } -} - -void FolderWizardRemotePath::slotHandleLsColNetworkError() -{ - auto *job = qobject_cast(sender()); - // Ignore 404s, otherwise users will get annoyed by error popups - // when not typing fast enough. It's still clear that a given path - // was not found, because the 'Next' button is disabled and no entry - // is selected in the tree view. - if (job->httpStatusCode() == 404) { - showWarn(QString()); // hides the warning pane - return; - } - showWarn(tr("Failed to list a folder. Error: %1") - .arg(job->errorStringParsingBody())); -} - -static QTreeWidgetItem *findFirstChild(QTreeWidgetItem *parent, const QString &text) -{ - for (int i = 0; i < parent->childCount(); ++i) { - QTreeWidgetItem *child = parent->child(i); - if (child->text(0) == text) { - return child; - } - } - return nullptr; -} - -void FolderWizardRemotePath::recursiveInsert(QTreeWidgetItem *parent, QStringList pathTrail, const QString &path) -{ - if (pathTrail.isEmpty()) - return; - - const QString parentPath = parent->data(0, Qt::UserRole).toString(); - const QString folderName = pathTrail.first(); - QString folderPath; - if (parentPath == QLatin1Char('/')) { - folderPath = folderName; - } else { - folderPath = parentPath + QLatin1Char('/') + folderName; - } - QTreeWidgetItem *item = findFirstChild(parent, folderName); - if (!item) { - item = new QTreeWidgetItem(parent); - item->setIcon(0, Resources::getCoreIcon(QStringLiteral("folder-sync"))); - item->setText(0, folderName); - item->setData(0, Qt::UserRole, folderPath); - item->setToolTip(0, folderPath); - item->setChildIndicatorPolicy(QTreeWidgetItem::ShowIndicator); - } - - pathTrail.removeFirst(); - recursiveInsert(item, pathTrail, path); -} - -bool FolderWizardRemotePath::selectByPath(QString path) -{ - if (path.startsWith(QLatin1Char('/'))) { - path = path.mid(1); - } - if (path.endsWith(QLatin1Char('/'))) { - path.chop(1); - } - - QTreeWidgetItem *it = _ui->folderTreeWidget->topLevelItem(0); - if (!path.isEmpty()) { - const QStringList pathTrail = path.split(QLatin1Char('/')); - for (const auto &trail : pathTrail) { - if (!it) { - return false; - } - it = findFirstChild(it, trail); - } - } - if (!it) { - return false; - } - - _ui->folderTreeWidget->setCurrentItem(it); - _ui->folderTreeWidget->scrollToItem(it); - return true; -} - -const QString &FolderWizardRemotePath::targetPath() const -{ - return _targetPath; -} - -void FolderWizardRemotePath::slotUpdateDirectories(const QStringList &list) -{ - QString webdavFolder = folderWizardPrivate()->davUrl().path(); - - QTreeWidgetItem *root = _ui->folderTreeWidget->topLevelItem(0); - if (!root) { - root = new QTreeWidgetItem(_ui->folderTreeWidget); - root->setText(0, Theme::instance()->appNameGUI()); - root->setIcon(0, Theme::instance()->applicationIcon()); - root->setToolTip(0, tr("Choose this to sync the entire account")); - root->setData(0, Qt::UserRole, QStringLiteral("/")); - } - QStringList sortedList = list; - Utility::sortFilenames(sortedList); - for (auto path : std::as_const(sortedList)) { - path.remove(webdavFolder); - QStringList paths = path.split(QLatin1Char('/')); - if (paths.last().isEmpty()) - paths.removeLast(); - recursiveInsert(root, paths, path); - } - root->setExpanded(true); -} - -void FolderWizardRemotePath::slotRefreshFolders() -{ - runPropFindJob(QStringLiteral("/")); - _ui->folderTreeWidget->clear(); - _ui->folderEntry->clear(); -} - -void FolderWizardRemotePath::slotItemExpanded(QTreeWidgetItem *item) -{ - QString dir = item->data(0, Qt::UserRole).toString(); - if (!dir.startsWith(QLatin1Char('/'))) { - dir.prepend(QLatin1Char('/')); - } - runPropFindJob(dir); -} - -void FolderWizardRemotePath::slotCurrentItemChanged(QTreeWidgetItem *item) -{ - if (item) { - QString dir = item->data(0, Qt::UserRole).toString(); - if (!dir.startsWith(QLatin1Char('/'))) { - dir.prepend(QLatin1Char('/')); - } - _ui->folderEntry->setText(dir); - } - - Q_EMIT completeChanged(); -} - -void FolderWizardRemotePath::slotFolderEntryEdited(const QString &text) -{ - if (selectByPath(text)) { - _lscolTimer.stop(); - return; - } - - _ui->folderTreeWidget->setCurrentItem(nullptr); - _lscolTimer.start(); // avoid sending a request on each keystroke -} - -void FolderWizardRemotePath::slotLsColFolderEntry() -{ - QString path = _ui->folderEntry->text(); - - PropfindJob *job = runPropFindJob(path); - // No error handling, no updating, we do this manually - // because of extra logic in the typed-path case. - disconnect(job, nullptr, this, nullptr); - connect(job, &PropfindJob::finishedWithError, - this, &FolderWizardRemotePath::slotHandleLsColNetworkError); - connect(job, &PropfindJob::directoryListingSubfolders, - this, &FolderWizardRemotePath::slotTypedPathFound); -} - -void FolderWizardRemotePath::slotTypedPathFound(const QStringList &subpaths) -{ - slotUpdateDirectories(subpaths); - selectByPath(_ui->folderEntry->text()); -} - -PropfindJob *FolderWizardRemotePath::runPropFindJob(const QString &path) -{ - PropfindJob *job = new PropfindJob(folderWizardPrivate()->accountState()->account(), folderWizardPrivate()->davUrl(), path, PropfindJob::Depth::One, this); - job->setProperties({ QByteArrayLiteral("resourcetype") }); - connect(job, &PropfindJob::directoryListingSubfolders, - this, &FolderWizardRemotePath::slotUpdateDirectories); - connect(job, &PropfindJob::finishedWithError, - this, &FolderWizardRemotePath::slotHandleLsColNetworkError); - job->start(); - - return job; -} - -FolderWizardRemotePath::~FolderWizardRemotePath() -{ - delete _ui; -} - -bool FolderWizardRemotePath::isComplete() const -{ - if (!_ui->folderTreeWidget->currentItem()) - return false; - - QStringList warnStrings; - QString dir = _ui->folderTreeWidget->currentItem()->data(0, Qt::UserRole).toString(); - if (!dir.startsWith(QLatin1Char('/'))) { - dir.prepend(QLatin1Char('/')); - } - const_cast(this)->_targetPath = dir; - - bool ok = true; - - for (auto *f : std::as_const(FolderMan::instance()->folders())) { - if (f->accountState()->account() != folderWizardPrivate()->accountState()->account()) { - continue; - } - QString curDir = f->remotePathTrailingSlash(); - if (QDir::cleanPath(dir) == QDir::cleanPath(curDir)) { - if (Theme::instance()->allowDuplicatedFolderSyncPair()) { - warnStrings.append(tr("This folder is already being synced.")); - } else { - ok = false; - warnStrings.append(tr("This folder can't be synced. Please choose another one.")); - } - } else if (dir.startsWith(curDir)) { - warnStrings.append(tr("You are already syncing %1, which is a parent folder of %2.").arg(Utility::escape(curDir), Utility::escape(dir))); - } else if (curDir.startsWith(dir)) { - warnStrings.append(tr("You are already syncing %1, which is a subfolder of %2.").arg(Utility::escape(curDir), Utility::escape(dir))); - } - } - - showWarn(FolderWizardPrivate::formatWarnings(warnStrings, !ok)); - return ok; -} - -void FolderWizardRemotePath::cleanupPage() -{ - showWarn(); -} - -void FolderWizardRemotePath::initializePage() -{ - showWarn(); - slotRefreshFolders(); -} - -void FolderWizardRemotePath::showWarn(const QString &msg) const -{ - if (msg.isEmpty()) { - _ui->warningFrame->hide(); - } else { - _ui->warningFrame->show(); - _ui->warningLabel->setText(QStringLiteral("%1").arg(msg)); - } -} diff --git a/src/gui/folderwizard/folderwizardremotepath.h b/src/gui/folderwizard/folderwizardremotepath.h deleted file mode 100644 index ee49a046e05..00000000000 --- a/src/gui/folderwizard/folderwizardremotepath.h +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright (C) by Hannah von Reth - * Copyright (C) by Duncan Mac-Vicar P. - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - */ -#pragma once - - -#include "gui/folder.h" -#include "gui/folderwizard/folderwizard_p.h" - -#include - - -class QTreeWidgetItem; - -class Ui_FolderWizardTargetPage; - -namespace OCC { - -/** - * @brief page to ask for the target folder - * @ingroup gui - */ - -class FolderWizardRemotePath : public FolderWizardPage -{ - Q_OBJECT -public: - explicit FolderWizardRemotePath(FolderWizardPrivate *parent); - ~FolderWizardRemotePath() override; - - bool isComplete() const override; - - void initializePage() override; - void cleanupPage() override; - - const QString &targetPath() const; - -protected Q_SLOTS: - - void showWarn(const QString & = QString()) const; - void slotAddRemoteFolder(); - void slotCreateRemoteFolder(const QString &); - void slotCreateRemoteFolderFinished(); - void slotHandleMkdirNetworkError(QNetworkReply *); - void slotHandleLsColNetworkError(); - void slotUpdateDirectories(const QStringList &); - void slotRefreshFolders(); - void slotItemExpanded(QTreeWidgetItem *); - void slotCurrentItemChanged(QTreeWidgetItem *); - void slotFolderEntryEdited(const QString &text); - void slotLsColFolderEntry(); - void slotTypedPathFound(const QStringList &subpaths); - -private: - PropfindJob *runPropFindJob(const QString &path); - void recursiveInsert(QTreeWidgetItem *parent, QStringList pathTrail, const QString &path); - bool selectByPath(QString path); - Ui_FolderWizardTargetPage *_ui; - bool _warnWasVisible; - QTimer _lscolTimer; - - QString _targetPath; -}; - -} diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index 0d4e038eaeb..2ddcbd70559 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -64,7 +64,7 @@ AbstractNetworkJob::AbstractNetworkJob(AccountPtr account, const QUrl &baseUrl, QUrl AbstractNetworkJob::url() const { - return Utility::concatUrlPath(_baseUrl, path(), query()); + return Utility::concatUrlPath(_baseUrl, _path, _query); } @@ -73,11 +73,6 @@ void AbstractNetworkJob::setQuery(const QUrlQuery &query) _query = query; } -QUrlQuery AbstractNetworkJob::query() const -{ - return _query; -} - void AbstractNetworkJob::setTimeout(const std::chrono::seconds sec) { _timeout = sec; diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index fab477aa475..88666fa0802 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -56,13 +56,6 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject virtual void start(); - // todo: this is used by FolderWizardRemotePath - get rid of it then get rid of this accessor - QString path() const { return _path; } - - /* - * The absolute url: baseUrl() + path() + query() - */ - QUrl url() const; QNetworkReply *reply() const; @@ -169,6 +162,11 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject void finishedSignal(QPrivateSignal); protected: + /* + * The absolute url: baseUrl() + path() + query() + */ + QUrl url() const; + /** Initiate a network request, returning a QNetworkReply. * * Calls setReply() and setupConnections() on it. @@ -191,8 +189,6 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject */ virtual void finished() = 0; - QByteArray _responseTimestamp; - QString replyStatusString(); /* @@ -201,9 +197,10 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject * The query must be fully encoded. */ void setQuery(const QUrlQuery &query); - QUrlQuery query() const; AccountPtr _account; + QUrlQuery _query; + QByteArray _responseTimestamp; private: /** Makes this job drive a pre-made QNetworkReply @@ -217,8 +214,6 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject const QUrl _baseUrl; const QString _path; - QUrlQuery _query; - std::chrono::seconds _timeout = httpTimeout; bool _timedout = false; // set to true when the timeout slot is received bool _aborted = false; diff --git a/src/libsync/networkjobs/jsonjob.cpp b/src/libsync/networkjobs/jsonjob.cpp index 4e0e7cdc198..bf826ee6c71 100644 --- a/src/libsync/networkjobs/jsonjob.cpp +++ b/src/libsync/networkjobs/jsonjob.cpp @@ -65,7 +65,7 @@ JsonApiJob::JsonApiJob(AccountPtr account, const QString &path, const QByteArray { _request.setRawHeader(QByteArrayLiteral("OCS-APIREQUEST"), QByteArrayLiteral("true")); - auto q = query(); + auto q = _query; // HACK: q will be empty for POST, PUT, PATCH request as we utilise the body (see SimpleNetworkJob) // however the oc api requires the format header to be part of the url q.addQueryItem(QStringLiteral("format"), QStringLiteral("json")); From 58c0325195c23f64b6692221126a756c78292b94 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 18:15:31 +0200 Subject: [PATCH 06/23] updated AccountBasedOAuth and request auth impls to use *account --- src/gui/creds/credentials.cpp | 11 +++++---- src/gui/creds/credentials.h | 5 ++-- src/gui/creds/oauth.cpp | 4 ++-- src/gui/creds/oauth.h | 6 ++--- .../creds/requestauthenticationcontroller.cpp | 11 ++++----- .../creds/requestauthenticationcontroller.h | 7 +++--- src/libsync/account.cpp | 23 +++++++++++-------- src/libsync/creds/abstractcredentials.h | 5 ++-- test/testoauth.cpp | 2 +- 9 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/gui/creds/credentials.cpp b/src/gui/creds/credentials.cpp index 68037768db5..df043991f75 100644 --- a/src/gui/creds/credentials.cpp +++ b/src/gui/creds/credentials.cpp @@ -75,7 +75,8 @@ class CredentialsAccessManager : public AccessManager private: // The credentials object dies along with the account, while the QNAM might // outlive both. - // Lisa todo: that statement is very concerning. investigate. + // Lisa todo: that statement is very concerning. investigate -> In fact the nam is parented by the creds by default so no, it + // should not outlive the creds unless there are shenanigans wrt reparenting it. QPointer _cred; }; @@ -283,9 +284,11 @@ bool Credentials::networkAvailable() void Credentials::refreshAccessTokenInternal() { + if (!_account) + return; // parent with nam to ensure we reset when the nam is reset // todo: #22 - the parenting here is highly questionable, as is the use of the shared account ptr - _oAuthJob = new AccountBasedOAuth(_account->sharedFromThis(), _account->accessManager()); + _oAuthJob = new AccountBasedOAuth(_account, this); connect(_oAuthJob, &AccountBasedOAuth::refreshError, this, &Credentials::handleRefreshError); connect(_oAuthJob, &AccountBasedOAuth::refreshFinished, this, &Credentials::handleRefreshSuccess); @@ -297,7 +300,7 @@ void Credentials::askFromUser() { // I think this can happen when the re-auth process has already quasi started and is waiting for user input, but // we get a prompt to log in again. I am not quite sure as it's very hard to reproduce. - if (_requestAuth) { + if (_requestAuth || !_account) { // let the existing instance ride return; } @@ -311,7 +314,7 @@ void Credentials::askFromUser() // if the auth fails the gui stays until the user gets it right or clicks the stay logged out button. connect(_requestAuth, &RequestAuthenticationController::authenticationSucceeded, this, &Credentials::askFromUserSucceeded); connect(_requestAuth, &RequestAuthenticationController::requestLogout, this, &Credentials::askFromUserLogout); - _requestAuth->startAuthentication(_account->sharedFromThis()); + _requestAuth->startAuthentication(_account); } void Credentials::askFromUserSucceeded(const QString &token, const QString &refreshToken) diff --git a/src/gui/creds/credentials.h b/src/gui/creds/credentials.h index 4ceb39773bd..0aa28955e1e 100644 --- a/src/gui/creds/credentials.h +++ b/src/gui/creds/credentials.h @@ -46,9 +46,8 @@ class OWNCLOUDGUI_EXPORT Credentials : public AbstractCredentials // this access manager is monitored for authenticationRequired signal // it also becomes the access manager member in the account. Try to invert this so the account // creates it and connects the creds slot to the _am. - // it is ALSO used by the checkServerJobFactory where it sets the parent to the parent of that job. This is so - // shady and needs to be fixed, but we may have to leave that to a future refactoring. - // the main point is I am not sure the creds should be creating the nam - I think this should be an account responsibility in the end. + // note this is used by the checkServerJobFactory where a new nam is "needed" for every job due to options to clear cookies + // and other factors that mean we don't want to use the account's nam directly. AccessManager *createAccessManager() const override; // todo: DC-112 I think this needs a naming update. diff --git a/src/gui/creds/oauth.cpp b/src/gui/creds/oauth.cpp index 3730777c2c3..d85112d9593 100644 --- a/src/gui/creds/oauth.cpp +++ b/src/gui/creds/oauth.cpp @@ -476,7 +476,7 @@ void OAuth::openBrowser() } } -AccountBasedOAuth::AccountBasedOAuth(AccountPtr account, QObject *parent) +AccountBasedOAuth::AccountBasedOAuth(Account *account, QObject *parent) : OAuth(account->url(), account->davUser(), account->accessManager(), parent) , _account(account) { @@ -496,7 +496,7 @@ void AccountBasedOAuth::fetchWellKnown() qCDebug(lcOauth) << "starting CheckServerJob before fetching" << wellKnownPathC; - auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account.get(), true).startJob(_serverUrl, this); + auto *checkServerJob = CheckServerJobFactory::createFromAccount(_account, true).startJob(_serverUrl, this); connect(checkServerJob, &CoreJob::finished, this, [checkServerJob, this]() { if (checkServerJob->success()) { diff --git a/src/gui/creds/oauth.h b/src/gui/creds/oauth.h index 6e6be39e80e..a27017d8aaa 100644 --- a/src/gui/creds/oauth.h +++ b/src/gui/creds/oauth.h @@ -63,7 +63,7 @@ class OAuth : public QObject Q_ENUM(PromptValuesSupported) Q_DECLARE_FLAGS(PromptValuesSupportedFlags, PromptValuesSupported) - OAuth(const QUrl &serverUrl, const QString &davUser, QNetworkAccessManager *networkAccessManager,QObject *parent); + OAuth(const QUrl &serverUrl, const QString &davUser, QNetworkAccessManager *networkAccessManager, QObject *parent); ~OAuth() override; virtual void startAuthentication(); @@ -146,7 +146,7 @@ class AccountBasedOAuth : public OAuth Q_OBJECT public: - explicit AccountBasedOAuth(AccountPtr account, QObject *parent = nullptr); + explicit AccountBasedOAuth(Account *account, QObject *parent); void startAuthentication() override; @@ -162,7 +162,7 @@ class AccountBasedOAuth : public OAuth void fetchWellKnown() override; private: - AccountPtr _account; + QPointer _account; }; QString toString(OAuth::PromptValuesSupportedFlags s); diff --git a/src/gui/creds/requestauthenticationcontroller.cpp b/src/gui/creds/requestauthenticationcontroller.cpp index daced708360..222bfd221a2 100644 --- a/src/gui/creds/requestauthenticationcontroller.cpp +++ b/src/gui/creds/requestauthenticationcontroller.cpp @@ -48,10 +48,9 @@ void RequestAuthenticationController::handleLogOut() } // this ultimately needs to be chained to creds::requestLogout signal Q_EMIT requestLogout(); - // cleanup(); } -void RequestAuthenticationController::startAuthentication(const AccountPtr account) +void RequestAuthenticationController::startAuthentication(Account *account) { Q_ASSERT(account); if (_oauth) { @@ -65,7 +64,7 @@ void RequestAuthenticationController::startAuthentication(const AccountPtr accou if (_widget && _modalWidget == nullptr) { // first show of the gui connect(_widget, &RequestAuthenticationWidget::connectClicked, this, &RequestAuthenticationController::handleSignIn); connect(_widget, &RequestAuthenticationWidget::stayLoggedOutClicked, this, &RequestAuthenticationController::handleLogOut); - AccountSettings *settings = ocApp()->gui()->settingsDialog()->accountSettings(account.get()); + AccountSettings *settings = ocApp()->gui()->settingsDialog()->accountSettings(_account); _modalWidget = new AccountModalWidget(QString(), _widget, settings); settings->addModalAccountWidget(_modalWidget); } @@ -107,7 +106,7 @@ void RequestAuthenticationController::handleOAuthResult(OAuth::Result result, co } - if (!errString.isEmpty()) { + if (!errString.isEmpty() && _account) { if (_widget) { _widget->setErrorMessage(errString); startAuthentication(_account); @@ -119,8 +118,6 @@ void RequestAuthenticationController::handleOAuthResult(OAuth::Result result, co Q_EMIT authenticationSucceeded(accessToken, refreshToken); } - ownCloudGui::raise(); - // need to check case where auth fails and user tries again. Does it work to just re-open - // the web ui? I have doubts. + ownCloudGui::raise();. } } diff --git a/src/gui/creds/requestauthenticationcontroller.h b/src/gui/creds/requestauthenticationcontroller.h index a89231e0a39..fd1bac21fc9 100644 --- a/src/gui/creds/requestauthenticationcontroller.h +++ b/src/gui/creds/requestauthenticationcontroller.h @@ -30,7 +30,7 @@ class RequestAuthenticationController : public QObject public: explicit RequestAuthenticationController(RequestAuthenticationWidget *widget, QObject *parent); - void startAuthentication(const AccountPtr account); + void startAuthentication(Account *account); Q_SIGNALS: void requestLogout(); @@ -44,11 +44,12 @@ class RequestAuthenticationController : public QObject void authUrlReady(); void handleOAuthResult(OAuth::Result, const QString &accessToken, const QString &refreshToken); + AccountBasedOAuth *_oauth = nullptr; + QPointer _account = nullptr; + // these will be cleaned up by the SettingsDialog automatically when the AccountModalWidget is "finished" // that is handled internally when accept() or reject() is called QPointer _widget; QPointer _modalWidget; - AccountBasedOAuth *_oauth = nullptr; - AccountPtr _account = nullptr; }; } diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 9bfb3ba08eb..6936dab49c4 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -212,25 +212,31 @@ AbstractCredentials *Account::credentials() const } // the credentials should be instantiated with the account as parent. we have to pass the creds in as the tests use their own -// sublcass of AbstractCredentials = FakeCredentials. +// subclass of AbstractCredentials = FakeCredentials. void Account::setCredentials(AbstractCredentials *cred) { - Q_ASSERT(cred); - - if (_credentials == cred) + if (!cred || _credentials == cred) return; - // set active credential manager + // keep any cookies for new nam QNetworkCookieJar *jar = nullptr; if (_am) { jar = _am->cookieJar(); jar->setParent(nullptr); - _am->deleteLater(); + } + // get rid of the old credentials if they exist - imo this should never happen but who knows + // note the access manager is parented by the creds so let that take care of deleting the am, but verify it's gone + if (_credentials) { + delete _credentials; + _credentials = nullptr; + Q_ASSERT(_am == nullptr); } _credentials = cred; - _am = _credentials->createAccessManager(); + if (jar) { + _am->setCookieJar(jar); + } // the network access manager takes ownership when setCache is called, so we have to reinitialize it every time we reset the manager _networkCache = new QNetworkDiskCache(this); @@ -238,9 +244,6 @@ void Account::setCredentials(AbstractCredentials *cred) _networkCache->setCacheDirectory(networkCacheLocation); _am->setCache(_networkCache); - if (jar) { - _am->setCookieJar(jar); - } connect(_credentials, &AbstractCredentials::fetched, this, [this] { Q_EMIT credentialsFetched(); _queueGuard.unblock(); diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h index 0572b2bd99a..edde7df1577 100644 --- a/src/libsync/creds/abstractcredentials.h +++ b/src/libsync/creds/abstractcredentials.h @@ -15,6 +15,7 @@ #pragma once #include +#include #include "accessmanager.h" #include "accountfwd.h" @@ -103,8 +104,8 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject protected: // the account should be the parent of the creds, so it should not go out of scope while we are using this. - // however, those may be famous last words depending on how the tear - Account *_account; + // however, those may be famous last words depending on how the teardown happens so keep it in a weak ref + QPointer _account; // todo: DC-112 I don't understand why this is needed but will try to figure it out bool _wasEverFetched; diff --git a/test/testoauth.cpp b/test/testoauth.cpp index d4ff3084cb5..b5a33458fd4 100644 --- a/test/testoauth.cpp +++ b/test/testoauth.cpp @@ -155,7 +155,7 @@ class OAuthTestCase : public QObject QObject::connect(&desktopServiceHook, &DesktopServiceHook::hooked, this, &OAuthTestCase::openBrowserHook); - auto out = std::make_unique(account); + auto out = std::make_unique(account.get(), nullptr); QObject::connect(out.get(), &OAuth::result, this, &OAuthTestCase::oauthResult); return out; } From 23a4a147613d60c9f2e422562b80d18dbac396fb Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 18 Sep 2025 18:33:19 +0200 Subject: [PATCH 07/23] comments and fixed typo --- src/gui/creds/oauth.cpp | 6 +++++- src/gui/creds/requestauthenticationcontroller.cpp | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/gui/creds/oauth.cpp b/src/gui/creds/oauth.cpp index d85112d9593..4f4baaa7248 100644 --- a/src/gui/creds/oauth.cpp +++ b/src/gui/creds/oauth.cpp @@ -476,6 +476,9 @@ void OAuth::openBrowser() } } +// todo: I was contemplating how we can make sure the passed account isn't null before we use it +// to seed the OAuth ctr, and really, I'm not sure this should be a subclass of oauth in the first place. Instead it could simply use an +// oauth instance to complete the tasks it can't do itself -> this could possibly be a "has a" not an "is a" impl AccountBasedOAuth::AccountBasedOAuth(Account *account, QObject *parent) : OAuth(account->url(), account->davUser(), account->accessManager(), parent) , _account(account) @@ -520,7 +523,8 @@ void AccountBasedOAuth::refreshAuthentication(const QString &refreshToken) return; } - // I don't see where this ever gets set to false + // I don't see where this ever gets set to false - seems to rely on a one shot run before creating a new instance where the value + // is initialized to false. hm. _isRefreshingToken = true; diff --git a/src/gui/creds/requestauthenticationcontroller.cpp b/src/gui/creds/requestauthenticationcontroller.cpp index 222bfd221a2..de6951c534b 100644 --- a/src/gui/creds/requestauthenticationcontroller.cpp +++ b/src/gui/creds/requestauthenticationcontroller.cpp @@ -118,6 +118,6 @@ void RequestAuthenticationController::handleOAuthResult(OAuth::Result result, co Q_EMIT authenticationSucceeded(accessToken, refreshToken); } - ownCloudGui::raise();. + ownCloudGui::raise(); } } From 0dcd95b47b6ece4ff4e7aea9183cf698f75f67a9 Mon Sep 17 00:00:00 2001 From: modSpike Date: Fri, 19 Sep 2025 13:01:00 +0200 Subject: [PATCH 08/23] removed dep on account from activity data and updated uses --- src/gui/activitydata.cpp | 8 ++++---- src/gui/activitydata.h | 4 ++-- src/gui/models/activitylistmodel.cpp | 4 ++-- src/gui/servernotificationhandler.cpp | 17 +++++++---------- test/modeltests/testactivitymodel.cpp | 26 ++++++++++++-------------- 5 files changed, 27 insertions(+), 32 deletions(-) diff --git a/src/gui/activitydata.cpp b/src/gui/activitydata.cpp index d57ba96d667..e585719fa0a 100644 --- a/src/gui/activitydata.cpp +++ b/src/gui/activitydata.cpp @@ -19,12 +19,12 @@ namespace OCC { -Activity::Activity(Activity::Type type, const QString &id, AccountPtr acc, const QString &subject, const QString &message, const QString &file, - const QUrl &link, const QDateTime &dateTime, const QVector &&links) +Activity::Activity(Activity::Type type, const QString &id, const QString &accName, const QUuid &uid, const QString &subject, const QString &message, + const QString &file, const QUrl &link, const QDateTime &dateTime, const QVector &&links) : _type(type) , _id(id) - , _accName(acc->displayNameWithHost()) - , _uuid(acc->uuid()) + , _accName(accName) + , _uuid(uid) , _subject(subject) , _message(message) , _file(file) diff --git a/src/gui/activitydata.h b/src/gui/activitydata.h index 0051a8df9f2..7a1f1a67b49 100644 --- a/src/gui/activitydata.h +++ b/src/gui/activitydata.h @@ -52,8 +52,8 @@ class OWNCLOUDGUI_EXPORT Activity NotificationType }; Activity() = default; - explicit Activity(Type type, const QString &id, AccountPtr acc, const QString &subject, const QString &message, const QString &file, const QUrl &link, - const QDateTime &dateTime, const QVector &&links = {}); + explicit Activity(Type type, const QString &id, const QString &accName, const QUuid &uid, const QString &subject, const QString &message, + const QString &file, const QUrl &link, const QDateTime &dateTime, const QVector &&links = {}); Type type() const; diff --git a/src/gui/models/activitylistmodel.cpp b/src/gui/models/activitylistmodel.cpp index 2ad3174d4c4..aeec6d8560d 100644 --- a/src/gui/models/activitylistmodel.cpp +++ b/src/gui/models/activitylistmodel.cpp @@ -208,8 +208,8 @@ void ActivityListModel::startFetchJob(AccountState *ast) list.reserve(activities.size()); for (const auto &activ : activities) { const auto json = activ.toObject(); - list.append(Activity{Activity::ActivityType, json.value(QStringLiteral("id")).toString(), ast->account(), - json.value(QStringLiteral("subject")).toString(), json.value(QStringLiteral("message")).toString(), + list.append(Activity{Activity::ActivityType, json.value(QStringLiteral("id")).toString(), ast->account()->displayNameWithHost(), + ast->account()->uuid(), json.value(QStringLiteral("subject")).toString(), json.value(QStringLiteral("message")).toString(), json.value(QStringLiteral("file")).toString(), QUrl(json.value(QStringLiteral("link")).toString()), QDateTime::fromString(json.value(QStringLiteral("date")).toString(), Qt::ISODate)}); } diff --git a/src/gui/servernotificationhandler.cpp b/src/gui/servernotificationhandler.cpp index e92ccc8c1b5..4e81db3e431 100644 --- a/src/gui/servernotificationhandler.cpp +++ b/src/gui/servernotificationhandler.cpp @@ -63,6 +63,9 @@ void ServerNotificationHandler::slotFetchNotifications(AccountState *ptr) void ServerNotificationHandler::slotNotificationsReceived(JsonApiJob *job, AccountState *accountState) { + if (!accountState || !accountState->account()) + return; + if (job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() != 200) { qCWarning(lcServerNotification) << "Notifications failed with status code " << job->ocsStatus(); return; @@ -98,16 +101,10 @@ void ServerNotificationHandler::slotNotificationsReceived(JsonApiJob *job, Accou al._isPrimary = false; linkList.append(al); - list.append(Activity { - Activity::NotificationType, - id, - accountState->account(), - json.value(QStringLiteral("subject")).toString(), - json.value(QStringLiteral("message")).toString(), - QString(), - QUrl(json.value(QStringLiteral("link")).toString()), - QDateTime::fromString(json.value(QStringLiteral("datetime")).toString(), Qt::ISODate), - std::move(linkList) }); + list.append(Activity{Activity::NotificationType, id, accountState->account()->displayNameWithHost(), accountState->account()->uuid(), + json.value(QStringLiteral("subject")).toString(), json.value(QStringLiteral("message")).toString(), QString(), + QUrl(json.value(QStringLiteral("link")).toString()), QDateTime::fromString(json.value(QStringLiteral("datetime")).toString(), Qt::ISODate), + std::move(linkList)}); } Q_EMIT newNotificationList(list); } diff --git a/test/modeltests/testactivitymodel.cpp b/test/modeltests/testactivitymodel.cpp index e63a63db18a..a75d34b1401 100644 --- a/test/modeltests/testactivitymodel.cpp +++ b/test/modeltests/testactivitymodel.cpp @@ -32,26 +32,24 @@ private Q_SLOTS: auto acc1 = createDummyAccount(); auto acc2 = createDummyAccount(); - /* auto creds0 = new FakeCredentials(acc1->account().get(), fakeAm); - acc1->account()->setCredentials(creds0); - auto creds1 = new FakeCredentials(acc2->account().get(), fakeAm); - acc2->account()->setCredentials(creds1);*/ model->setActivityList({ - Activity{Activity::ActivityType, QStringLiteral("1"), acc1->account(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), - QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, - Activity{Activity::ActivityType, QStringLiteral("2"), acc1->account(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), - QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, - Activity{Activity::ActivityType, QStringLiteral("021ad48a-80ae-4af6-b878-aeb836bd367d"), acc2->account(), QStringLiteral("test"), + Activity{Activity::ActivityType, QStringLiteral("1"), acc1->account()->displayNameWithHost(), acc1->account()->uuid(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, + Activity{Activity::ActivityType, QStringLiteral("2"), acc1->account()->displayNameWithHost(), acc1->account()->uuid(), QStringLiteral("test"), + QStringLiteral("test"), QStringLiteral("foo.cpp"), QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, + Activity{Activity::ActivityType, QStringLiteral("021ad48a-80ae-4af6-b878-aeb836bd367d"), acc2->account()->displayNameWithHost(), + acc2->account()->uuid(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), + QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, }); model->setActivityList({ - Activity{Activity::ActivityType, QStringLiteral("1"), acc2->account(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), - QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, - Activity{Activity::ActivityType, QStringLiteral("2"), acc1->account(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), - QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, - Activity{Activity::ActivityType, QStringLiteral("021ad48a-80ae-4af6-b878-aeb836bd367d"), acc2->account(), QStringLiteral("test"), + Activity{Activity::ActivityType, QStringLiteral("1"), acc2->account()->displayNameWithHost(), acc2->account()->uuid(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, + Activity{Activity::ActivityType, QStringLiteral("2"), acc1->account()->displayNameWithHost(), acc1->account()->uuid(), QStringLiteral("test"), + QStringLiteral("test"), QStringLiteral("foo.cpp"), QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, + Activity{Activity::ActivityType, QStringLiteral("021ad48a-80ae-4af6-b878-aeb836bd367d"), acc2->account()->displayNameWithHost(), + acc2->account()->uuid(), QStringLiteral("test"), QStringLiteral("test"), QStringLiteral("foo.cpp"), + QUrl(QStringLiteral("https://owncloud.com")), QDateTime::currentDateTime()}, }); model->slotRemoveAccount(AccountManager::instance()->accounts().first()); } From 8eaa6f3ff0104eba1bdaaec205a216cdfb7c5be4 Mon Sep 17 00:00:00 2001 From: modSpike Date: Mon, 22 Sep 2025 18:43:48 +0200 Subject: [PATCH 09/23] small cleanups, removed some accessors, including those to get the account, and got rid of account member for DiscoverySingleDirectoryJob as it was never used --- src/gui/creds/credentials.cpp | 2 +- src/libsync/abstractnetworkjob.cpp | 31 +++++++++++++++--------------- src/libsync/abstractnetworkjob.h | 1 - src/libsync/discovery.cpp | 2 +- src/libsync/discovery.h | 5 +++-- src/libsync/discoveryphase.cpp | 3 +-- src/libsync/discoveryphase.h | 4 +--- src/libsync/syncengine.cpp | 7 +++---- src/libsync/syncengine.h | 1 - test/testsyncconflict.cpp | 4 ++-- test/testsyncengine.cpp | 2 +- 11 files changed, 28 insertions(+), 34 deletions(-) diff --git a/src/gui/creds/credentials.cpp b/src/gui/creds/credentials.cpp index df043991f75..89997b34b06 100644 --- a/src/gui/creds/credentials.cpp +++ b/src/gui/creds/credentials.cpp @@ -169,7 +169,7 @@ bool Credentials::stillValid(QNetworkReply *reply) if (reply->error() == QNetworkReply::AuthenticationRequiredError) { slotAuthentication(reply, nullptr); } - return true; + return true; } void Credentials::slotAuthentication(QNetworkReply *reply, QAuthenticator *authenticator) diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index 2ddcbd70559..5fd420de2da 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -57,11 +57,6 @@ AbstractNetworkJob::AbstractNetworkJob(AccountPtr account, const QUrl &baseUrl, Q_ASSERT(baseUrl.isValid()); } -/*QUrl AbstractNetworkJob::baseUrl() const -{ - return _baseUrl; -}*/ - QUrl AbstractNetworkJob::url() const { return Utility::concatUrlPath(_baseUrl, _path, _query); @@ -94,11 +89,6 @@ QNetworkReply *AbstractNetworkJob::reply() const return _reply; } -bool AbstractNetworkJob::isAuthenticationJob() const -{ - return _isAuthenticationJob; -} - void AbstractNetworkJob::setAuthenticationJob(bool b) { _isAuthenticationJob = b; @@ -106,7 +96,7 @@ void AbstractNetworkJob::setAuthenticationJob(bool b) bool AbstractNetworkJob::needsRetry() const { - if (isAuthenticationJob()) { + if (_isAuthenticationJob) { qCDebug(lcNetworkJob) << "Not Retry auth job" << this << url(); return false; } @@ -160,7 +150,7 @@ void AbstractNetworkJob::sendRequest(const QByteArray &verb, _request.setPriority(_priority); _request.setTransferTimeout(duration_cast(_timeout).count()); - if (!isAuthenticationJob() && _account->jobQueue()->enqueue(this)) { + if (!_isAuthenticationJob && _account->jobQueue()->enqueue(this)) { return; } @@ -190,9 +180,16 @@ void AbstractNetworkJob::adoptRequest(QPointer reply) } void AbstractNetworkJob::slotFinished() -{ +{ _finished = true; + if (!_reply) { + qCWarning(lcNetworkJob) << "Network job finished but reply is nullptr - aborting" << this; + Q_EMIT networkError(nullptr); + deleteLater(); + return; + } + if (!_account->credentials()->stillValid(_reply) && !ignoreCredentialFailure()) { _account->invalidCredentialsEncountered(); } @@ -212,9 +209,11 @@ void AbstractNetworkJob::slotFinished() // get the Date timestamp from reply _responseTimestamp = _reply->rawHeader("Date"); - if (!reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).isNull() && !(isAuthenticationJob() || reply()->request().hasRawHeader(QByteArrayLiteral("OC-Connection-Validator")))) { + if (!reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).isNull() + && !(_isAuthenticationJob || reply()->request().hasRawHeader(QByteArrayLiteral("OC-Connection-Validator")))) { Q_EMIT _account->unknownConnectionState(); - qCWarning(lcNetworkJob) << this << "Unsupported redirect on" << _reply->url().toString() << "to" << reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).toString(); + qCWarning(lcNetworkJob) << this << "Unsupported redirect on" << _reply->url().toString() << "to" + << reply()->attribute(QNetworkRequest::RedirectionTargetAttribute).toString(); Q_EMIT networkError(_reply); if (_account->jobQueue()->retry(this)) { qCWarning(lcNetworkJob) << "Retry Nr:" << _retryCount << _reply->url(); @@ -431,7 +430,7 @@ QDebug operator<<(QDebug debug, const OCC::AbstractNetworkJob *job) } } if (job->_timedout) { - debug << ", timedout"; + debug << ", timed out"; } debug << ")"; return debug.maybeSpace(); diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index 88666fa0802..bce7e2a5faf 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -120,7 +120,6 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject static constexpr std::chrono::seconds DefaultHttpTimeout { 5 * 60 }; /** whether or noth this job should be restarted after authentication */ - bool isAuthenticationJob() const; void setAuthenticationJob(bool b); /** How many times was that job retried */ diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index d5f4d135060..42b6cfb96fb 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1323,7 +1323,7 @@ DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery() void ProcessDirectoryJob::startAsyncLocalQuery() { QString localPath = _discoveryData->_localDir + _currentFolder._local; - auto localJob = new DiscoverySingleLocalDirectoryJob(_discoveryData->_account, localPath, _discoveryData->_syncOptions._vfs.data()); + auto localJob = new DiscoverySingleLocalDirectoryJob(localPath, _discoveryData->_syncOptions._vfs.data()); _discoveryData->_currentlyActiveJobs++; _pendingAsyncJobs++; diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index adcc8269ed3..a45b59e8537 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -14,10 +14,11 @@ #pragma once -#include +#include "common/syncjournaldb.h" #include "discoveryphase.h" #include "syncfileitem.h" -#include "common/syncjournaldb.h" +#include +#include class ExcludedFiles; diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 53b0667ceb7..6d6bfb4ba14 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -188,11 +188,10 @@ bool DiscoveryPhase::isSpace() const return !(Utility::urlEqual(_account->davUrl(), _baseUrl) || _account->davUrl().isParentOf(_baseUrl)); } -DiscoverySingleLocalDirectoryJob::DiscoverySingleLocalDirectoryJob(const AccountPtr &account, const QString &localPath, OCC::Vfs *vfs, QObject *parent) +DiscoverySingleLocalDirectoryJob::DiscoverySingleLocalDirectoryJob(const QString &localPath, OCC::Vfs *vfs, QObject *parent) : QObject(parent) , QRunnable() , _localPath(localPath) - , _account(account) , _vfs(vfs) { qRegisterMetaType >("QVector"); diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 7f4e3ee313c..1334cac2017 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -24,7 +24,6 @@ #include #include #include -#include #include "syncoptions.h" #include "syncfileitem.h" @@ -86,7 +85,7 @@ class DiscoverySingleLocalDirectoryJob : public QObject, public QRunnable { Q_OBJECT public: - explicit DiscoverySingleLocalDirectoryJob(const AccountPtr &account, const QString &localPath, OCC::Vfs *vfs, QObject *parent = nullptr); + explicit DiscoverySingleLocalDirectoryJob(const QString &localPath, OCC::Vfs *vfs, QObject *parent = nullptr); void run() override; Q_SIGNALS: @@ -99,7 +98,6 @@ class DiscoverySingleLocalDirectoryJob : public QObject, public QRunnable private Q_SLOTS: private: QString _localPath; - AccountPtr _account; OCC::Vfs* _vfs; public: }; diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 3d90460753e..ee93c0f2083 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -381,8 +381,7 @@ void SyncEngine::startSync() } qCInfo(lcEngine) << "#### Discovery start ####################################################" << _duration.duration(); - qCInfo(lcEngine) << "Server" << account()->capabilities().status().versionString() - << (account()->isHttp2Supported() ? "Using HTTP/2" : ""); + qCInfo(lcEngine) << "Server" << _account->capabilities().status().versionString() << (_account->isHttp2Supported() ? "Using HTTP/2" : ""); _progressInfo->_status = ProgressInfo::Discovery; Q_EMIT transmissionProgress(*_progressInfo); @@ -710,10 +709,10 @@ void SyncEngine::restoreOldFiles(SyncFileItemSet &syncItems) } } -AccountPtr SyncEngine::account() const +/*AccountPtr SyncEngine::account() const { return _account; -} +}*/ void SyncEngine::setLocalDiscoveryOptions(LocalDiscoveryStyle style, std::set paths) { diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index dcbc35796de..4e3ed011c75 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -88,7 +88,6 @@ class OWNCLOUDSYNC_EXPORT SyncEngine : public QObject /* Returns whether another sync is needed to complete the sync */ bool isAnotherSyncNeeded() { return _anotherSyncNeeded; } - AccountPtr account() const; SyncJournalDb *journal() const { return _journal; } QString localPath() const { return _localPath; } diff --git a/test/testsyncconflict.cpp b/test/testsyncconflict.cpp index bff19d051b5..0b49a5e7cd6 100644 --- a/test/testsyncconflict.cpp +++ b/test/testsyncconflict.cpp @@ -119,7 +119,7 @@ private Q_SLOTS: } else { // Verify that the conflict names don't have the user name for (const auto &name : conflicts) { - QVERIFY(!name.contains(fakeFolder.syncEngine().account()->davDisplayName())); + QVERIFY(!name.contains(fakeFolder.account()->davDisplayName())); } QVERIFY(expectAndWipeConflict(fakeFolder, QStringLiteral("A/a1"))); @@ -178,7 +178,7 @@ private Q_SLOTS: QCOMPARE(Utility::conflictFileBaseNameFromPattern(conflictMap[a1FileId].toUtf8()), QByteArray("A/a1")); // Check that the conflict file contains the username - QVERIFY(conflictMap[a1FileId].contains(QString::fromLatin1("(conflicted copy %1 ").arg(fakeFolder.syncEngine().account()->davDisplayName()))); + QVERIFY(conflictMap[a1FileId].contains(QString::fromLatin1("(conflicted copy %1 ").arg(fakeFolder.account()->davDisplayName()))); QCOMPARE(remote.find(conflictMap[a1FileId])->contentChar, 'L'); QCOMPARE(remote.find(QStringLiteral("A/a1"))->contentChar, 'R'); diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 87691f8f647..17b2e47c8e8 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -713,7 +713,7 @@ private Q_SLOTS: cap[QStringLiteral("dav")] = dav; return cap; }; - fakeFolder.syncEngine().account()->setCapabilities({fakeFolder.account()->url(), invalidFilenameRegexCapabilities(QStringLiteral("my[fgh]ile"))}); + fakeFolder.account()->setCapabilities({fakeFolder.account()->url(), invalidFilenameRegexCapabilities(QStringLiteral("my[fgh]ile"))}); fakeFolder.localModifier().insert(QStringLiteral("C/myfile.txt")); QVERIFY(fakeFolder.applyLocalModificationsAndSync()); QVERIFY(!fakeFolder.currentRemoteState().find(QStringLiteral("C/myfile.txt"))); From a9593c79972f3a06fa4858ba79392cbecc8d2398 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 25 Sep 2025 18:48:31 +0200 Subject: [PATCH 10/23] changed folder wizard and related class dependencies from the account state to the spacesmanager as that was all that was needed eliminated more accessors to get the account, replaced more accountptr with just account*. identified cases where the account dep can be removed completely with correct refactoring to make jobs only run in controllers/managers (where it's reasonable). removed no longer used deprecated function from SpacesManager --- src/gui/accountsettings.cpp | 23 +++++------ src/gui/accountsettings.h | 2 +- src/gui/folderman.cpp | 4 +- src/gui/folderwizard/folderwizard.cpp | 41 ++++++++++--------- src/gui/folderwizard/folderwizard.h | 3 +- src/gui/folderwizard/folderwizard_p.h | 9 ++-- .../folderwizard/folderwizardlocalpath.cpp | 2 +- .../folderwizardselectivesync.cpp | 4 +- .../folderwizard/folderwizardselectivesync.h | 3 +- src/gui/folderwizard/spacespage.cpp | 6 ++- src/gui/folderwizard/spacespage.h | 7 ++-- src/gui/selectivesyncwidget.cpp | 13 ++++-- src/gui/selectivesyncwidget.h | 11 ++--- src/gui/spaces/spaceimageprovider.cpp | 9 ++-- src/gui/spaces/spaceimageprovider.h | 7 ++-- src/gui/spaces/spacesbrowser.cpp | 9 ++-- src/gui/spaces/spacesbrowser.h | 3 +- src/libsync/account.cpp | 10 ++--- src/libsync/discovery.cpp | 23 +++++------ src/libsync/graphapi/space.cpp | 3 ++ src/libsync/graphapi/spacesmanager.cpp | 18 ++------ src/libsync/graphapi/spacesmanager.h | 16 ++++---- 22 files changed, 116 insertions(+), 110 deletions(-) diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 78b725d36f5..1c9dcaa95b7 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -68,7 +68,7 @@ AccountSettings::AccountSettings(AccountState *accountState, QWidget *parent) // as usual we do too many things in the ctr and we need to eval all the code paths to make sure they handle // the QPointer properly, but as a stopgap to catch null states asap before they trickle down into other areas: - Q_ASSERT(_accountState); + Q_ASSERT(_accountState && _accountState->account()); _model = new FolderStatusModel(this); @@ -83,19 +83,19 @@ AccountSettings::AccountSettings(AccountState *accountState, QWidget *parent) _sortModel = weightedModel; - ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(_accountState->account())); + ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(_accountState->account()->spacesManager())); ui->quickWidget->setOCContext(QUrl(QStringLiteral("qrc:/qt/qml/org/ownCloud/gui/qml/FolderDelegate.qml")), this); connect(FolderMan::instance(), &FolderMan::folderListChanged, _model, &FolderStatusModel::resetFolders); ui->connectionStatusLabel->clear(); - connect(_accountState.data(), &AccountState::stateChanged, this, &AccountSettings::slotAccountStateChanged); - slotAccountStateChanged(); + connect(_accountState, &AccountState::stateChanged, this, &AccountSettings::slotAccountStateChanged); + slotAccountStateChanged(_accountState->state()); buildManageAccountMenu(); - connect(_accountState.get(), &AccountState::isSettingUpChanged, this, &AccountSettings::accountSettingUpChanged); + connect(_accountState, &AccountState::isSettingUpChanged, this, &AccountSettings::accountSettingUpChanged); connect(ui->stackedWidget, &QStackedWidget::currentChanged, this, [this] { ui->manageAccountButton->setEnabled(ui->stackedWidget->currentWidget() == ui->quickWidget); }); @@ -257,11 +257,11 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder) void AccountSettings::showSelectiveSyncDialog(Folder *folder) { - if (!_accountState) { + if (!_accountState || !_accountState->account()) { return; } - auto *selectiveSync = new SelectiveSyncWidget(_accountState->account(), this); + auto *selectiveSync = new SelectiveSyncWidget(_accountState->account().get(), this); selectiveSync->setDavUrl(folder->webDavUrl()); bool ok; selectiveSync->setFolderInfo( @@ -279,11 +279,11 @@ void AccountSettings::showSelectiveSyncDialog(Folder *folder) void AccountSettings::slotAddFolder() { - if (!_accountState) { + if (!_accountState || !_accountState->account()) { return; } - FolderWizard *folderWizard = new FolderWizard(_accountState, this); + FolderWizard *folderWizard = new FolderWizard(_accountState->account().get(), this); folderWizard->setAttribute(Qt::WA_DeleteOnClose); connect(folderWizard, &QDialog::accepted, this, &AccountSettings::slotFolderWizardAccepted); @@ -550,13 +550,12 @@ void AccountSettings::buildManageAccountMenu() } // Refactoring todo: the signal sends the new account state, refactor this to use that param -void AccountSettings::slotAccountStateChanged() +void AccountSettings::slotAccountStateChanged(AccountState::State state) { - if (!_accountState) { + if (!_accountState || !_accountState->account()) { return; } - const AccountState::State state = _accountState->state(); const AccountPtr account = _accountState->account(); qCDebug(lcAccountSettings) << "Account state changed to" << state << "for account" << account; diff --git a/src/gui/accountsettings.h b/src/gui/accountsettings.h index 144b92017a4..c4e41bae130 100644 --- a/src/gui/accountsettings.h +++ b/src/gui/accountsettings.h @@ -78,7 +78,7 @@ class OWNCLOUDGUI_EXPORT AccountSettings : public QWidget void syncedSpacesChanged(); public Q_SLOTS: - void slotAccountStateChanged(); + void slotAccountStateChanged(AccountState::State state); void slotSpacesUpdated(); void slotAddFolder(); diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index fc9fbc715aa..4d838b7cd53 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -257,7 +257,7 @@ void FolderMan::loadSpacesWhenReady(AccountState *accountState, bool useVfs) settings.beginGroup("Accounts"); // prepare the root - reality check this as I think the user can change this from default? - const QString localDir(spacesMgr->account()->defaultSyncRoot()); + const QString localDir(accountState->account()->defaultSyncRoot()); if (!prepareFolder(localDir)) { return; } @@ -270,7 +270,7 @@ void FolderMan::loadSpacesWhenReady(AccountState *accountState, bool useVfs) folderDef.setPriority(space->priority()); - QString localPath = findGoodPathForNewSyncFolder(localDir, folderDef.displayName(), NewFolderType::SpacesFolder, spacesMgr->account()->uuid()); + QString localPath = findGoodPathForNewSyncFolder(localDir, folderDef.displayName(), NewFolderType::SpacesFolder, accountState->account()->uuid()); folderDef.setLocalPath(localPath); folderDef.setTargetPath({}); diff --git a/src/gui/folderwizard/folderwizard.cpp b/src/gui/folderwizard/folderwizard.cpp index d1d516a0399..6ec2cd242ea 100644 --- a/src/gui/folderwizard/folderwizard.cpp +++ b/src/gui/folderwizard/folderwizard.cpp @@ -65,31 +65,38 @@ QString FolderWizardPrivate::defaultSyncRoot() const { // this should never happen when we have set up the account using spaces - there is ALWAYS a default root when spaces are in play // and they are always in play so this check is bogus. todo: #43 - if (!_account->account()->hasDefaultSyncRoot()) { + if (!_account->hasDefaultSyncRoot()) { const auto folderType = FolderMan::NewFolderType::SpacesSyncRoot; // todo: #43 : FolderMan::NewFolderType::OC10SyncRoot; - return FolderMan::suggestSyncFolder(folderType, _account->account()->uuid()); + return FolderMan::suggestSyncFolder(folderType, _account->uuid()); } else { - return _account->account()->defaultSyncRoot(); + return _account->defaultSyncRoot(); } } -FolderWizardPrivate::FolderWizardPrivate(FolderWizard *q, AccountState *account) +QUuid FolderWizardPrivate::uuid() const +{ + if (_account) + return _account->uuid(); + return {}; +} + +FolderWizardPrivate::FolderWizardPrivate(FolderWizard *q, Account *account) : q_ptr(q) , _account(account) , _folderWizardSourcePage(new FolderWizardLocalPath(this)) , _folderWizardSelectiveSyncPage(nullptr) { - _spacesPage = new SpacesPage(account->account(), q); + if (_account) { + _spacesPage = new SpacesPage(account->spacesManager(), q); q->setPage(FolderWizard::Page_Space, _spacesPage); - + } q->setPage(FolderWizard::Page_Source, _folderWizardSourcePage); - // When VFS is available (currently only with Windows' CFApi), and it is forced on, Spaces are meant to be synced as a whole. const bool showPage = VfsPluginManager::instance().bestAvailableVfsMode() != Vfs::WindowsCfApi || !Theme::instance()->forceVirtualFilesOption(); if (showPage) { - _folderWizardSelectiveSyncPage = new FolderWizardSelectiveSync(this); + _folderWizardSelectiveSyncPage = new FolderWizardSelectiveSync(_account, this); q->setPage(FolderWizard::Page_SelectiveSync, _folderWizardSelectiveSyncPage); } } @@ -97,7 +104,7 @@ FolderWizardPrivate::FolderWizardPrivate(FolderWizard *q, AccountState *account) QString FolderWizardPrivate::initialLocalPath() const { return FolderMan::findGoodPathForNewSyncFolder( - defaultSyncRoot(), _spacesPage->currentSpace()->displayName(), FolderMan::NewFolderType::SpacesSyncRoot, _account->account()->uuid()); + defaultSyncRoot(), _spacesPage->currentSpace()->displayName(), FolderMan::NewFolderType::SpacesSyncRoot, _account->uuid()); } uint32_t FolderWizardPrivate::priority() const @@ -124,10 +131,10 @@ QString FolderWizardPrivate::displayName() const return _spacesPage->currentSpace()->displayName(); } -AccountState *FolderWizardPrivate::accountState() +/*AccountState *FolderWizardPrivate::accountState() { - return _account; -} + return _accountState; +}*/ bool FolderWizardPrivate::useVirtualFiles() const { @@ -145,7 +152,7 @@ bool FolderWizardPrivate::useVirtualFiles() const return useVirtualFiles; } -FolderWizard::FolderWizard(AccountState *account, QWidget *parent) +FolderWizard::FolderWizard(Account *account, QWidget *parent) : QWizard(parent) , d_ptr(new FolderWizardPrivate(this, account)) { @@ -155,18 +162,14 @@ FolderWizard::FolderWizard(AccountState *account, QWidget *parent) setWizardStyle(QWizard::ModernStyle); } -FolderWizard::~FolderWizard() -{ -} - FolderMan::SyncConnectionDescription FolderWizard::result() { Q_D(FolderWizard); const QString localPath = d->_folderWizardSourcePage->localPath(); - if (!d->_account->account()->hasDefaultSyncRoot()) { + if (!d->_account->hasDefaultSyncRoot()) { if (FileSystem::isChildPathOf(localPath, d->defaultSyncRoot())) { - d->_account->account()->setDefaultSyncRoot(d->defaultSyncRoot()); + d->_account->setDefaultSyncRoot(d->defaultSyncRoot()); if (!QFileInfo::exists(d->defaultSyncRoot())) { OC_ASSERT(QDir().mkpath(d->defaultSyncRoot())); } diff --git a/src/gui/folderwizard/folderwizard.h b/src/gui/folderwizard/folderwizard.h index 7c9cbc13eee..fed091781ce 100644 --- a/src/gui/folderwizard/folderwizard.h +++ b/src/gui/folderwizard/folderwizard.h @@ -45,8 +45,7 @@ class FolderWizard : public QWizard }; Q_ENUM(PageType) - explicit FolderWizard(AccountState *account, QWidget *parent = nullptr); - ~FolderWizard() override; + explicit FolderWizard(Account *account, QWidget *parent); FolderMan::SyncConnectionDescription result(); diff --git a/src/gui/folderwizard/folderwizard_p.h b/src/gui/folderwizard/folderwizard_p.h index 40341d41893..cd68dca1b7e 100644 --- a/src/gui/folderwizard/folderwizard_p.h +++ b/src/gui/folderwizard/folderwizard_p.h @@ -32,7 +32,7 @@ Q_DECLARE_LOGGING_CATEGORY(lcFolderWizard); class FolderWizardPrivate { public: - FolderWizardPrivate(FolderWizard *q, AccountState *account); + FolderWizardPrivate(FolderWizard *q, Account *account); static QString formatWarnings(const QStringList &warnings, bool isError = false); QString initialLocalPath() const; @@ -46,19 +46,20 @@ class FolderWizardPrivate QString defaultSyncRoot() const; + QUuid uuid() const; QUrl davUrl() const; QString spaceId() const; bool useVirtualFiles() const; QString displayName() const; - AccountState *accountState(); + // AccountState *accountState(); private: Q_DECLARE_PUBLIC(FolderWizard) FolderWizard *q_ptr; - QPointer _account; - class SpacesPage *_spacesPage; + QPointer _account; + class SpacesPage *_spacesPage = nullptr; class FolderWizardLocalPath *_folderWizardSourcePage = nullptr; class FolderWizardSelectiveSync *_folderWizardSelectiveSyncPage = nullptr; }; diff --git a/src/gui/folderwizard/folderwizardlocalpath.cpp b/src/gui/folderwizard/folderwizardlocalpath.cpp index c3048399023..ef27ab9d6f5 100644 --- a/src/gui/folderwizard/folderwizardlocalpath.cpp +++ b/src/gui/folderwizard/folderwizardlocalpath.cpp @@ -64,7 +64,7 @@ QString FolderWizardLocalPath::localPath() const bool FolderWizardLocalPath::isComplete() const { auto folderType = FolderMan::NewFolderType::SpacesFolder; - auto accountUuid = folderWizardPrivate()->accountState()->account()->uuid(); + auto accountUuid = folderWizardPrivate()->uuid(); QString errorStr = FolderMan::instance()->checkPathValidityForNewFolder(localPath(), folderType, accountUuid); bool isOk = errorStr.isEmpty(); diff --git a/src/gui/folderwizard/folderwizardselectivesync.cpp b/src/gui/folderwizard/folderwizardselectivesync.cpp index 095d1b47b54..f58df255984 100644 --- a/src/gui/folderwizard/folderwizardselectivesync.cpp +++ b/src/gui/folderwizard/folderwizardselectivesync.cpp @@ -34,11 +34,11 @@ using namespace OCC; -FolderWizardSelectiveSync::FolderWizardSelectiveSync(FolderWizardPrivate *parent) +FolderWizardSelectiveSync::FolderWizardSelectiveSync(Account *account, FolderWizardPrivate *parent) : FolderWizardPage(parent) { QVBoxLayout *layout = new QVBoxLayout(this); - _selectiveSync = new SelectiveSyncWidget(folderWizardPrivate()->accountState()->account(), this); + _selectiveSync = new SelectiveSyncWidget(account, this); layout->addWidget(_selectiveSync); if (Theme::instance()->showVirtualFilesOption() && VfsPluginManager::instance().bestAvailableVfsMode() == Vfs::WindowsCfApi) { diff --git a/src/gui/folderwizard/folderwizardselectivesync.h b/src/gui/folderwizard/folderwizardselectivesync.h index 59248846d50..ea85bd394d2 100644 --- a/src/gui/folderwizard/folderwizardselectivesync.h +++ b/src/gui/folderwizard/folderwizardselectivesync.h @@ -23,6 +23,7 @@ namespace OCC { +class Account; class SelectiveSyncWidget; /** @@ -33,7 +34,7 @@ class FolderWizardSelectiveSync : public FolderWizardPage { Q_OBJECT public: - explicit FolderWizardSelectiveSync(FolderWizardPrivate *parent); + explicit FolderWizardSelectiveSync(Account *account, FolderWizardPrivate *parent); ~FolderWizardSelectiveSync() override; bool validatePage() override; diff --git a/src/gui/folderwizard/spacespage.cpp b/src/gui/folderwizard/spacespage.cpp index 1036939998b..ba4be7a5aef 100644 --- a/src/gui/folderwizard/spacespage.cpp +++ b/src/gui/folderwizard/spacespage.cpp @@ -14,17 +14,19 @@ #include "spacespage.h" #include "ui_spacespage.h" +#include "spacesmanager.h" + #include "theme.h" using namespace OCC; -SpacesPage::SpacesPage(AccountPtr acc, QWidget *parent) +SpacesPage::SpacesPage(GraphApi::SpacesManager *spacesMgr, QWidget *parent) : QWizardPage(parent) , ui(new Ui::SpacesPage) { ui->setupUi(this); - ui->widget->setAccount(acc); + ui->widget->setSpacesManager(spacesMgr); ui->label->setText( Theme::instance()->spacesAreCalledFolders() ? tr("Select a folder to sync it to your computer.") : tr("Select a Space to sync it to your computer.")); diff --git a/src/gui/folderwizard/spacespage.h b/src/gui/folderwizard/spacespage.h index 3f4d5ec69ba..0386fca8bfd 100644 --- a/src/gui/folderwizard/spacespage.h +++ b/src/gui/folderwizard/spacespage.h @@ -15,8 +15,6 @@ #include "gui/spaces/spacesmodel.h" -#include "accountfwd.h" - #include @@ -25,12 +23,15 @@ class SpacesPage; } namespace OCC { + +class GraphApi::SpacesManager; + class SpacesPage : public QWizardPage { Q_OBJECT public: - explicit SpacesPage(AccountPtr acc, QWidget *parent = nullptr); + explicit SpacesPage(GraphApi::SpacesManager *spacesMgr, QWidget *parent); ~SpacesPage(); bool isComplete() const override; diff --git a/src/gui/selectivesyncwidget.cpp b/src/gui/selectivesyncwidget.cpp index a4bb6603398..da3e0ea1cb8 100644 --- a/src/gui/selectivesyncwidget.cpp +++ b/src/gui/selectivesyncwidget.cpp @@ -60,7 +60,7 @@ class SelectiveSyncTreeViewItem : public QTreeWidgetItem } }; -SelectiveSyncWidget::SelectiveSyncWidget(AccountPtr account, QWidget *parent) +SelectiveSyncWidget::SelectiveSyncWidget(Account *account, QWidget *parent) : QWidget(parent) , _account(account) , _inserting(false) @@ -98,9 +98,13 @@ QSize SelectiveSyncWidget::sizeHint() const return QWidget::sizeHint().expandedTo(QSize(600, 600)); } +// todo DC-150 or beyond: jobs should not be run in the gui! create a controller to run the job, and eliminate the account dep here in this widget void SelectiveSyncWidget::refreshFolders() { - auto *job = new PropfindJob(_account, davUrl(), _folderPath, PropfindJob::Depth::One, this); + if (!_account) + return; + + auto *job = new PropfindJob(_account->sharedFromThis(), davUrl(), _folderPath, PropfindJob::Depth::One, this); job->setProperties({QByteArrayLiteral("resourcetype"), QByteArrayLiteral("http://owncloud.org/ns:size")}); connect(job, &PropfindJob::directoryListingSubfolders, this, &SelectiveSyncWidget::slotUpdateDirectories); connect(job, &PropfindJob::finishedWithError, this, [job, this] { @@ -290,11 +294,14 @@ void SelectiveSyncWidget::slotUpdateDirectories(QStringList list) void SelectiveSyncWidget::slotItemExpanded(QTreeWidgetItem *item) { + if (!_account) + return; + QString dir = item->data(0, Qt::UserRole).toString(); if (dir.isEmpty()) { return; } - PropfindJob *job = new PropfindJob(_account, davUrl(), Utility::concatUrlPathItems({_folderPath, dir}), PropfindJob::Depth::One, this); + PropfindJob *job = new PropfindJob(_account->sharedFromThis(), davUrl(), Utility::concatUrlPathItems({_folderPath, dir}), PropfindJob::Depth::One, this); job->setProperties({QByteArrayLiteral("resourcetype"), QByteArrayLiteral("http://owncloud.org/ns:size")}); connect(job, &PropfindJob::directoryListingSubfolders, this, &SelectiveSyncWidget::slotUpdateDirectories); job->start(); diff --git a/src/gui/selectivesyncwidget.h b/src/gui/selectivesyncwidget.h index 1692f97af6e..5c2e382f5fa 100644 --- a/src/gui/selectivesyncwidget.h +++ b/src/gui/selectivesyncwidget.h @@ -13,10 +13,10 @@ */ #pragma once -#include "accountfwd.h" -#include -#include + +#include #include +#include #include "csync_exclude.h" @@ -27,6 +27,7 @@ class QLabel; namespace OCC { class Folder; +class Account; /** * @brief The SelectiveSyncWidget contains a folder tree with labels @@ -36,7 +37,7 @@ class SelectiveSyncWidget : public QWidget { Q_OBJECT public: - explicit SelectiveSyncWidget(AccountPtr account, QWidget *parent = nullptr); + explicit SelectiveSyncWidget(Account *account, QWidget *parent); /// Returns a list of blacklisted paths, each including the trailing / QSet createBlackList(QTreeWidgetItem *root = nullptr) const; @@ -62,7 +63,7 @@ private Q_SLOTS: QUrl davUrl() const; private: - AccountPtr _account; + QPointer _account; QString _folderPath; QString _rootName; diff --git a/src/gui/spaces/spaceimageprovider.cpp b/src/gui/spaces/spaceimageprovider.cpp index 2b3e41bc32b..1012ea3137e 100644 --- a/src/gui/spaces/spaceimageprovider.cpp +++ b/src/gui/spaces/spaceimageprovider.cpp @@ -19,20 +19,23 @@ using namespace OCC; using namespace Spaces; -SpaceImageProvider::SpaceImageProvider(const AccountPtr &account) +SpaceImageProvider::SpaceImageProvider(GraphApi::SpacesManager *spacesMgr) : QQuickImageProvider(QQuickImageProvider::Pixmap, QQuickImageProvider::ForceAsynchronousImageLoading) - , _account(account.data()) + , _spacesManager(spacesMgr) { } QPixmap SpaceImageProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize) { + if (!_spacesManager) + return {}; + QIcon icon; if (id == QLatin1String("placeholder")) { icon = Resources::getCoreIcon(QStringLiteral("space")); } else { const auto ids = id.split(QLatin1Char('/')); - const auto *space = _account->spacesManager()->space(ids.last()); + const auto *space = _spacesManager->space(ids.last()); if (space) { icon = space->image()->image(); } diff --git a/src/gui/spaces/spaceimageprovider.h b/src/gui/spaces/spaceimageprovider.h index 1a2845e73e0..9e1905e3767 100644 --- a/src/gui/spaces/spaceimageprovider.h +++ b/src/gui/spaces/spaceimageprovider.h @@ -14,7 +14,8 @@ #pragma once -#include "libsync/account.h" +#include "spacesmanager.h" +#include #include @@ -23,12 +24,12 @@ class SpaceImageProvider : public QQuickImageProvider { Q_OBJECT public: - SpaceImageProvider(const AccountPtr &account); + SpaceImageProvider(GraphApi::SpacesManager *spacesMgr); QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize) override; private: - Account *_account; + QPointer _spacesManager; }; } diff --git a/src/gui/spaces/spacesbrowser.cpp b/src/gui/spaces/spacesbrowser.cpp index 0a276f4ba49..f1d7d6b7b94 100644 --- a/src/gui/spaces/spacesbrowser.cpp +++ b/src/gui/spaces/spacesbrowser.cpp @@ -61,12 +61,11 @@ SpacesBrowser::~SpacesBrowser() delete ui; } -void SpacesBrowser::setAccount(OCC::AccountPtr acc) +void SpacesBrowser::setSpacesManager(OCC::GraphApi::SpacesManager *spacesMgr) { - _acc = acc; - if (acc) { - _model->setSpacesManager(acc->spacesManager()); - ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(acc)); + if (spacesMgr) { + _model->setSpacesManager(spacesMgr); + ui->quickWidget->engine()->addImageProvider(QStringLiteral("space"), new Spaces::SpaceImageProvider(spacesMgr)); } } diff --git a/src/gui/spaces/spacesbrowser.h b/src/gui/spaces/spacesbrowser.h index fcc330a029c..73cf41a958d 100644 --- a/src/gui/spaces/spacesbrowser.h +++ b/src/gui/spaces/spacesbrowser.h @@ -39,7 +39,7 @@ class SpacesBrowser : public QWidget explicit SpacesBrowser(QWidget *parent = nullptr); ~SpacesBrowser(); - void setAccount(OCC::AccountPtr acc); + void setSpacesManager(OCC::GraphApi::SpacesManager *spacesMgr); GraphApi::Space *currentSpace(); @@ -51,7 +51,6 @@ class SpacesBrowser : public QWidget private: ::Ui::SpacesBrowser *ui; - OCC::AccountPtr _acc; SpacesModel *_model; QSortFilterProxyModel *_sortModel; GraphApi::Space *_currentSpace = nullptr; diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 6936dab49c4..034d72dc300 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -79,6 +79,8 @@ Account::Account(const QUuid &uuid, const QString &user, const QUrl &url, QObjec } QDir().mkpath(resourcesCacheDir); _resourcesCache = new ResourcesCache(resourcesCacheDir, this); + + _spacesManager = new GraphApi::SpacesManager(this); } AccountPtr Account::create(const QUuid &uuid, const QString &user, const QUrl &url) @@ -211,8 +213,9 @@ AbstractCredentials *Account::credentials() const return _credentials; } -// the credentials should be instantiated with the account as parent. we have to pass the creds in as the tests use their own -// subclass of AbstractCredentials = FakeCredentials. +// todo: in a sane world the credentials should be instantiated by the account (as parent) as using this setter there isthe chance that it could be set or unset +// randomly or even multiple times like this is really not good. For now we have to pass the creds in as the tests use their own subclass of AbstractCredentials +// = FakeCredentials, so that has to be worked out before we can move this creds handling into the account proper, where it should be. void Account::setCredentials(AbstractCredentials *cred) { if (!cred || _credentials == cred) @@ -350,9 +353,6 @@ void Account::setCapabilities(const Capabilities &caps) if (versionChanged) { Q_EMIT serverVersionChanged(); } - if (!_spacesManager) { - _spacesManager = new GraphApi::SpacesManager(this); - } } Account::ServerSupportLevel Account::serverSupportLevel() const diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 42b6cfb96fb..b12b3c46fa1 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1267,27 +1267,27 @@ void ProcessDirectoryJob::dbError() DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery() { - auto serverJob = new DiscoverySingleDirectoryJob(_discoveryData->_account, _discoveryData->_baseUrl, - _discoveryData->_remoteFolder + _currentFolder._server, this); + auto discoveryJob = + new DiscoverySingleDirectoryJob(_discoveryData->_account, _discoveryData->_baseUrl, _discoveryData->_remoteFolder + _currentFolder._server, this); if (!_dirItem) - serverJob->setIsRootPath(); // query the fingerprint on the root - connect(serverJob, &DiscoverySingleDirectoryJob::etag, this, &ProcessDirectoryJob::etag); + discoveryJob->setIsRootPath(); // query the fingerprint on the root + connect(discoveryJob, &DiscoverySingleDirectoryJob::etag, this, &ProcessDirectoryJob::etag); _discoveryData->_currentlyActiveJobs++; _pendingAsyncJobs++; - connect(serverJob, &DiscoverySingleDirectoryJob::finished, this, [this, serverJob](const auto &results) { + connect(discoveryJob, &DiscoverySingleDirectoryJob::finished, this, [this, discoveryJob](const auto &results) { _discoveryData->_currentlyActiveJobs--; _pendingAsyncJobs--; if (results) { _serverNormalQueryEntries = *results; _serverQueryDone = true; - if (!serverJob->_dataFingerprint.isEmpty() && _discoveryData->_dataFingerprint.isEmpty()) - _discoveryData->_dataFingerprint = serverJob->_dataFingerprint; + if (!discoveryJob->_dataFingerprint.isEmpty() && _discoveryData->_dataFingerprint.isEmpty()) + _discoveryData->_dataFingerprint = discoveryJob->_dataFingerprint; if (_localQueryDone) this->process(); } else { auto code = results.error().code; qCWarning(lcDisco) << "Server error in directory" << _currentFolder._server << code; - if (serverJob->isRootPath()) { + if (discoveryJob->isRootPath()) { if (code == 404 && _discoveryData->isSpace()) { Q_EMIT _discoveryData->fatalError(tr("This Space is currently unavailable")); return; @@ -1314,10 +1314,9 @@ DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery() .arg(_currentFolder._server.isEmpty() ? QStringLiteral("/") : _currentFolder._server, results.error().message)); } }); - connect(serverJob, &DiscoverySingleDirectoryJob::firstDirectoryPermissions, this, - [this](const RemotePermissions &perms) { _rootPermissions = perms; }); - serverJob->start(); - return serverJob; + connect(discoveryJob, &DiscoverySingleDirectoryJob::firstDirectoryPermissions, this, [this](const RemotePermissions &perms) { _rootPermissions = perms; }); + discoveryJob->start(); + return discoveryJob; } void ProcessDirectoryJob::startAsyncLocalQuery() diff --git a/src/libsync/graphapi/space.cpp b/src/libsync/graphapi/space.cpp index 60a58c51c7f..264de69b820 100644 --- a/src/libsync/graphapi/space.cpp +++ b/src/libsync/graphapi/space.cpp @@ -156,6 +156,9 @@ void SpaceImage::update() _etag = newEtag; _url = QUrl(img->getWebDavUrl()); + + // todo DC-150: this job should run in the spaces manager, then just set the image on this spaceImage object. We don't need all these deps floating + // around at every level and we *definitely* don't want the spaces manager to hand out the account to whoever wants it auto job = _space->_spaceManager->account()->resourcesCache()->makeGetJob(_url, this); // TODO: next problem = this routine is correctly run when the icon has changed on the server, but the icon in the gui does not get refreshed! diff --git a/src/libsync/graphapi/spacesmanager.cpp b/src/libsync/graphapi/spacesmanager.cpp index f05be7c824b..546a7833120 100644 --- a/src/libsync/graphapi/spacesmanager.cpp +++ b/src/libsync/graphapi/spacesmanager.cpp @@ -47,7 +47,7 @@ SpacesManager::SpacesManager(Account *parent) void SpacesManager::refresh() { - if (!OC_ENSURE(_account->accessManager())) { + if (!_account || !_account->accessManager()) { return; } if (!_account->credentials()->ready()) { @@ -59,7 +59,7 @@ void SpacesManager::refresh() drivesJob->setTimeout(refreshTimeoutC); connect(drivesJob, &Drives::finishedSignal, this, [drivesJob, this] { // a system which provides multiple personal spaces the name of the drive is always used as display name - auto hasManyPersonalSpaces = this->account()->capabilities().spacesSupport().hasMultiplePersonalSpaces; + auto hasManyPersonalSpaces = _account->capabilities().spacesSupport().hasMultiplePersonalSpaces; drivesJob->deleteLater(); if (drivesJob->httpStatusCode() == 200) { @@ -75,7 +75,7 @@ void SpacesManager::refresh() } Q_EMIT spaceChanged(space); } - for (const QString &id : oldKeys) { + for (const QString &id : std::as_const(oldKeys)) { auto *oldSpace = _spacesMap.take(id); oldSpace->deleteLater(); } @@ -95,17 +95,7 @@ Space *SpacesManager::space(const QString &id) const { if (id.isEmpty()) return nullptr; - return _spacesMap.value(id); -} - -Space *SpacesManager::spaceByUrl(const QUrl &url) const -{ - auto it = std::find_if(_spacesMap.cbegin(), _spacesMap.cend(), - [url](const auto *space) { return OCC::Utility::urlEqual(QUrl(space->drive().getRoot().getWebDavUrl()), url); }); - if (it != _spacesMap.cend()) { - return *it; - } - return {}; + return _spacesMap.value(id, nullptr); } Account *SpacesManager::account() const diff --git a/src/libsync/graphapi/spacesmanager.h b/src/libsync/graphapi/spacesmanager.h index 80a4628a837..1eb8f5d14ad 100644 --- a/src/libsync/graphapi/spacesmanager.h +++ b/src/libsync/graphapi/spacesmanager.h @@ -16,18 +16,16 @@ #include "owncloudlib.h" -#include "libsync/accountfwd.h" #include "libsync/graphapi/space.h" #include -#include - -#include - class QTimer; namespace OCC { + +class Account; + namespace GraphApi { class OWNCLOUDSYNC_EXPORT SpacesManager : public QObject @@ -41,9 +39,9 @@ namespace GraphApi { QVector spaces() const; - // deprecated: we need to migrate to id based spaces - [[deprecated("Use space(const QString &id)")]] Space *spaceByUrl(const QUrl &url) const; - + // todo DC-150: remove this accessor and take responsibility for running job to retrieve/update space image as needed + // once that is complete we can get rid of the account memeber entirely (and even revert the parent arg to a simple QObject) + // by passing the value for hasManyPersonalSpaces to this via ctr Account *account() const; /** @@ -59,7 +57,7 @@ namespace GraphApi { private: void refresh(); - Account *_account; + QPointer _account; QTimer *_refreshTimer; QMap _spacesMap; bool _ready = false; From 9c8619af43c168fbf20e34367d7ec442625f2ea9 Mon Sep 17 00:00:00 2001 From: modSpike Date: Fri, 26 Sep 2025 09:12:19 +0200 Subject: [PATCH 11/23] fixed forward declaration --- src/gui/folderwizard/spacespage.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/gui/folderwizard/spacespage.h b/src/gui/folderwizard/spacespage.h index 0386fca8bfd..03e076c15fc 100644 --- a/src/gui/folderwizard/spacespage.h +++ b/src/gui/folderwizard/spacespage.h @@ -24,7 +24,9 @@ class SpacesPage; namespace OCC { -class GraphApi::SpacesManager; +namespace GraphApi { + class SpacesManager; +} class SpacesPage : public QWizardPage { From 1895805d26ab02e10f4b51e725b8443813d5f4a8 Mon Sep 17 00:00:00 2001 From: modSpike Date: Fri, 26 Sep 2025 10:50:53 +0200 Subject: [PATCH 12/23] fixed the long standing issue in folderstatusmodel with the convoluted setAccountStatus function. Now the account status handling is much safer and clearer. --- src/gui/accountsettings.cpp | 11 ++--- src/gui/folderstatusmodel.cpp | 90 ++++++++++++++--------------------- src/gui/folderstatusmodel.h | 6 +-- 3 files changed, 44 insertions(+), 63 deletions(-) diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 1c9dcaa95b7..741fd5786d3 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -68,13 +68,10 @@ AccountSettings::AccountSettings(AccountState *accountState, QWidget *parent) // as usual we do too many things in the ctr and we need to eval all the code paths to make sure they handle // the QPointer properly, but as a stopgap to catch null states asap before they trickle down into other areas: - Q_ASSERT(_accountState && _accountState->account()); - - - _model = new FolderStatusModel(this); + if (!_accountState || !_accountState->account()) + return; - // see comments in this impl as it needs work - _model->setAccountState(_accountState); + _model = new FolderStatusModel(_accountState, this); auto weightedModel = new QSortFilterProxyModel(this); weightedModel->setSourceModel(_model); @@ -146,7 +143,7 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder) // am not able to easily determine what should happen in this handler if the state is null. For now we just assert // to make the "source" of the nullptr obvious before it trickles down into sub-areas and causes a crash that's harder // to id - Q_ASSERT(_accountState); + Q_ASSERT(_accountState && _accountState->account()); // qpointer for async calls const auto isDeployed = folder->isDeployed(); diff --git a/src/gui/folderstatusmodel.cpp b/src/gui/folderstatusmodel.cpp index 0358fe7a997..9e6fef5f183 100644 --- a/src/gui/folderstatusmodel.cpp +++ b/src/gui/folderstatusmodel.cpp @@ -174,63 +174,27 @@ namespace { } } - -FolderStatusModel::FolderStatusModel(QObject *parent) +FolderStatusModel::FolderStatusModel(AccountState *accountState, QObject *parent) : QAbstractListModel(parent) - , _accountState(nullptr) -{ -} - -FolderStatusModel::~FolderStatusModel() { } - -void FolderStatusModel::setAccountState(AccountState *accountState) + , _accountState(accountState) { - // Refactoring todo: what is the logic here? I especially don't understand why we are expecting current _accountState to - // be nullptr (via assert) when this is called. - // if this ptr should only be set once: - // public setter must be removed - // the ptr should be passed to the FolderStatusModel ctr as a one shot setting. - // if useful, split the setup routine(s) into a "configure" method that can be called after the ctr. - // I am also in favor of independent "connect" and "disconnect" functions to keep all that logic in one place - // instead of spread out all over the place. call it after construction. - beginResetModel(); - _folders.clear(); - // at least test to see if the "new" account state is legit before we go through all the setup - if (accountState && _accountState != accountState) { - Q_ASSERT(!_accountState); - _accountState = accountState; - - connect(FolderMan::instance(), &FolderMan::folderSyncStateChange, this, &FolderStatusModel::slotFolderSyncStateChange); - - if (_accountState && _accountState->account() && _accountState->account()->spacesManager()) { - connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::updated, this, - [this] { Q_EMIT dataChanged(index(0, 0), index(rowCount() - 1, 0)); }); - connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::spaceChanged, this, [this](auto *space) { - for (int i = 0; i < rowCount(); ++i) { - if (_folders[i]->_folder->space() == space) { - Q_EMIT dataChanged(index(i, 0), index(i, 0)); - break; - } + connect(FolderMan::instance(), &FolderMan::folderSyncStateChange, this, &FolderStatusModel::slotFolderSyncStateChange); + + if (_accountState && _accountState->account() && _accountState->account()->spacesManager()) { + // todo: we should not update the whole folder model any time the spaces are updated, implement spaceAdded and removed to deal with that incrementally + connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::updated, this, + [this] { Q_EMIT dataChanged(index(0, 0), index(rowCount() - 1, 0)); }); + connect(_accountState->account()->spacesManager(), &GraphApi::SpacesManager::spaceChanged, this, [this](auto *space) { + for (int i = 0; i < rowCount(); ++i) { + if (_folders[i]->_folder->space() == space) { + Q_EMIT dataChanged(index(i, 0), index(i, 0)); + break; } - }); - } - } - for (const auto &f : FolderMan::instance()->folders()) { - if (!_accountState) - break; - if (f->accountState() != _accountState) - continue; - - _folders.push_back(std::make_unique(f)); - - connect(ProgressDispatcher::instance(), &ProgressDispatcher::progressInfo, this, [f, this](Folder *folder, const ProgressInfo &progress) { - if (folder == f) { - slotSetProgress(progress, f); } }); } - endResetModel(); + resetFolders(); } QVariant FolderStatusModel::data(const QModelIndex &index, int role) const @@ -444,10 +408,30 @@ void FolderStatusModel::slotFolderSyncStateChange(Folder *f) void FolderStatusModel::resetFolders() { - if (_accountState == nullptr) + beginResetModel(); + _folders.clear(); + + if (!_accountState) { + endResetModel(); return; - // what does this even do? see the impl - setAccountState(_accountState); + } + + // todo: there is already a plan to organize folders in the folderman by account using a lookup on the uuid. this kind of filtering in the dependent is not + // ok. also the folder should not have an accessor for the account state or any other "powerful" object. + for (const auto &f : FolderMan::instance()->folders()) { + if (f->accountState() != _accountState) + continue; + + _folders.push_back(std::make_unique(f)); + + connect(ProgressDispatcher::instance(), &ProgressDispatcher::progressInfo, this, [f, this](Folder *folder, const ProgressInfo &progress) { + if (folder == f) { + slotSetProgress(progress, f); + } + }); + } + + endResetModel(); } } // namespace OCC diff --git a/src/gui/folderstatusmodel.h b/src/gui/folderstatusmodel.h index cc3ae30e5e0..a13de407703 100644 --- a/src/gui/folderstatusmodel.h +++ b/src/gui/folderstatusmodel.h @@ -60,9 +60,9 @@ class FolderStatusModel : public QAbstractListModel }; Q_ENUMS(Roles) - FolderStatusModel(QObject *parent = nullptr); - ~FolderStatusModel() override; - void setAccountState(AccountState *accountState); + FolderStatusModel(AccountState *accountState, QObject *parent = nullptr); + + // void setAccountState(AccountState *accountState); Folder *folder(const QModelIndex &index) const; From d9fb533195fdaf5b8db417629187e0934323c985 Mon Sep 17 00:00:00 2001 From: modSpike Date: Fri, 26 Sep 2025 13:34:20 +0200 Subject: [PATCH 13/23] make gcc happy and added todo's to really fix this mess --- src/gui/folderstatusmodel.cpp | 2 ++ src/gui/folderstatusmodel.h | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/gui/folderstatusmodel.cpp b/src/gui/folderstatusmodel.cpp index 9e6fef5f183..80dde72a443 100644 --- a/src/gui/folderstatusmodel.cpp +++ b/src/gui/folderstatusmodel.cpp @@ -197,6 +197,8 @@ FolderStatusModel::FolderStatusModel(AccountState *accountState, QObject *parent resetFolders(); } +FolderStatusModel::~FolderStatusModel() { } + QVariant FolderStatusModel::data(const QModelIndex &index, int role) const { if (_accountState == nullptr) diff --git a/src/gui/folderstatusmodel.h b/src/gui/folderstatusmodel.h index a13de407703..72a3b7b5ae7 100644 --- a/src/gui/folderstatusmodel.h +++ b/src/gui/folderstatusmodel.h @@ -62,7 +62,9 @@ class FolderStatusModel : public QAbstractListModel FolderStatusModel(AccountState *accountState, QObject *parent = nullptr); - // void setAccountState(AccountState *accountState); + // todo: #45 this only "needs" to exist as if it is removed there are gcc errors on the unique_ptr for the SubFolderInfo (which is reported as "incomplete" + // type?) in the anonymous namspace + ~FolderStatusModel() override; Folder *folder(const QModelIndex &index) const; @@ -82,6 +84,8 @@ private Q_SLOTS: int indexOf(Folder *f) const; QPointer _accountState; + + // todo: #45 I don't see why these need to be unique pointers instead of simple instances. std::vector> _folders; }; From 46f64f090b58a220939c054e18ebbebb2ba2030d Mon Sep 17 00:00:00 2001 From: modSpike Date: Tue, 30 Sep 2025 17:06:25 +0200 Subject: [PATCH 14/23] did an interim refactor of the account storage in socketapi. Instead of a qset of accountptr instances I changed it to a hash of uuid -> QPointer I think we can actually replace this with a qset of defaultSyncRoot since the roots are unique to the account, and that is all the class needs to function, but don't want to get too far off track with this pr so do that another time --- src/gui/accountstate.cpp | 3 ++- src/gui/socketapi/socketapi.cpp | 29 +++++++++++++---------------- src/gui/socketapi/socketapi.h | 11 ++++++++--- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index 726a6cc7f10..601acaf4440 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -77,8 +77,9 @@ AccountState::AccountState(AccountPtr account) connect(_account->credentials(), &AbstractCredentials::requestLogout, this, [this] { setState(State::SignedOut); }); + // todo: no we should not directly register the account, but let accountAdded signal trigger it if (FolderMan::instance()) { - FolderMan::instance()->socketApi()->registerAccount(account); + FolderMan::instance()->socketApi()->registerAccount(_account.get()); } } diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index dfd9ee87704..104e35fda22 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -22,7 +22,6 @@ #include "accountmanager.h" #include "common/asserts.h" -#include "common/depreaction.h" #include "common/syncjournalfilerecord.h" #include "common/version.h" #include "filesystem.h" @@ -154,11 +153,7 @@ SocketApi::SocketApi(QObject *parent) // Wire up the server instance to us, so we can accept new connections: connect(&_localServer, &SocketApiServer::newConnection, this, &SocketApi::slotNewConnection); - connect(AccountManager::instance(), &AccountManager::accountRemoved, this, [this](const auto &accountState) { - if (_registeredAccounts.contains(accountState->account())) { - unregisterAccount(accountState->account()); - } - }); + connect(AccountManager::instance(), &AccountManager::accountRemoved, this, &SocketApi::unregisterAccount); } SocketApi::~SocketApi() @@ -208,7 +203,7 @@ void SocketApi::slotNewConnection() auto listener = QSharedPointer::create(socket); _listeners.insert(socket, listener); for (const auto &a : std::as_const(_registeredAccounts)) { - if (a->hasDefaultSyncRoot()) { + if (a && !a->defaultSyncRoot().isEmpty()) { broadcastMessage(buildRegisterPathMessage(Utility::stripTrailingSlash(a->defaultSyncRoot()))); } } @@ -318,29 +313,31 @@ void SocketApi::slotReadSocket() } -void SocketApi::registerAccount(const AccountPtr &a) +void SocketApi::registerAccount(Account *a) { // Make sure not to register twice to each connected client - if (_registeredAccounts.contains(a)) { + if (!a || _registeredAccounts.contains(a->uuid())) { return; } - if (a->hasDefaultSyncRoot()) { + _registeredAccounts.insert(a->uuid(), a); + + if (!a->defaultSyncRoot().isEmpty()) { broadcastMessage(buildRegisterPathMessage(Utility::stripTrailingSlash(a->defaultSyncRoot()))); } - _registeredAccounts.insert(a); } -void SocketApi::unregisterAccount(const AccountPtr &a) +void SocketApi::unregisterAccount(AccountState *state) { - if (!_registeredAccounts.contains(a)) { + if (!state || !state->account()) return; - } - if (a->hasDefaultSyncRoot()) { + Account *a = state->account().get(); + + if (!a->defaultSyncRoot().isEmpty()) { broadcastMessage(buildMessage(unregisterPathMessageC(), Utility::stripTrailingSlash(a->defaultSyncRoot()))); } - _registeredAccounts.remove(a); + _registeredAccounts.remove(a->uuid()); } void SocketApi::slotRegisterPath(Folder *folder) diff --git a/src/gui/socketapi/socketapi.h b/src/gui/socketapi/socketapi.h index 4b8421e0b74..2c851e60ad0 100644 --- a/src/gui/socketapi/socketapi.h +++ b/src/gui/socketapi/socketapi.h @@ -57,8 +57,11 @@ class OWNCLOUDGUI_EXPORT SocketApi : public QObject void startShellIntegration(); public Q_SLOTS: - void registerAccount(const AccountPtr &a); - void unregisterAccount(const AccountPtr &a); + // todo: registerAccount is called directly in accountState ctr but should be connected to AccountManager::accountAdded instead asap + void registerAccount(Account *a); + // unregisterAccount *is* connected to accountRemoved but hopefully we can get rid of the AccountState part and send the account itself + // asap. + void unregisterAccount(AccountState *state); void slotUpdateFolderView(Folder *f); void slotUnregisterPath(Folder *f); void slotRegisterPath(Folder *f); @@ -150,7 +153,9 @@ private Q_SLOTS: QString _socketPath; QSet _registeredFolders; - QSet _registeredAccounts; + // todo: we really should not keep any pointer to the account, as we only ever need the defaultSyncRoot, but this needs to go into a future + // refactoring to ensure full testing of that change. IMO we could just store the defaultSyncRoot alone, no uuid or account required at all + QHash> _registeredAccounts; QMap> _listeners; SocketApiServer _localServer; }; From 4ebde8eddd0ee05c2e2935a6a046050237ca431d Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 10:13:45 +0200 Subject: [PATCH 15/23] more random conversions from shared to raw or q pointer --- src/gui/accountsettings.cpp | 2 +- src/gui/folder.cpp | 2 +- src/gui/folderman.cpp | 4 ++-- src/gui/folderman.h | 2 +- src/gui/models/activitylistmodel.cpp | 6 +++--- src/gui/newaccountwizard/newaccountbuilder.cpp | 2 +- src/gui/newaccountwizard/newaccountbuilder.h | 4 ++-- src/gui/settingsdialog.cpp | 6 ++++-- src/gui/settingsdialog.h | 2 +- src/gui/socketapi/socketapi.cpp | 2 +- src/libsync/appprovider.cpp | 7 +++++-- src/libsync/appprovider.h | 2 +- 12 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 741fd5786d3..22288a7f2e4 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -553,7 +553,7 @@ void AccountSettings::slotAccountStateChanged(AccountState::State state) return; } - const AccountPtr account = _accountState->account(); + Account *account = _accountState->account().get(); qCDebug(lcAccountSettings) << "Account state changed to" << state << "for account" << account; FolderMan *folderMan = FolderMan::instance(); diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 861e41f3abb..c8dd74039ec 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -115,7 +115,7 @@ Folder::Folder(const FolderDefinition &definition, AccountState *accountState, s _journal.wipeErrorBlacklistCategory(SyncJournalErrorBlacklistRecord::Category::LocalSoftError); // todo: the engine needs to be created externally, presumably by the folderman, and passed in by injection // current impl can result in an invalid engine which is just a mess given the folder is useless without it - _engine.reset(new SyncEngine(_accountState->account(), webDavUrl(), path(), remotePath(), &_journal)); + _engine.reset(new SyncEngine(_accountState->account().get(), webDavUrl(), path(), remotePath(), &_journal)); // pass the setting if hidden files are to be ignored, will be read in csync_update _engine->setIgnoreHiddenFiles(ignoreHiddenFiles); diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 1eb58bc6739..20441887b6f 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -634,7 +634,7 @@ Folder *FolderMan::folderForPath(const QString &path, QString *relativePath) return nullptr; } -QStringList FolderMan::findFileInLocalFolders(const QString &relPath, const AccountPtr acc) +QStringList FolderMan::findFileInLocalFolders(const QString &relPath, const Account *acc) { QStringList re; @@ -644,7 +644,7 @@ QStringList FolderMan::findFileInLocalFolders(const QString &relPath, const Acco serverPath.prepend(QLatin1Char('/')); for (auto *folder : std::as_const(_folders)) { - if (acc != nullptr && folder->accountState()->account() != acc) { + if (acc && folder->accountState() && folder->accountState()->account().get() != acc) { continue; } if (!serverPath.startsWith(folder->remotePath())) diff --git a/src/gui/folderman.h b/src/gui/folderman.h index 3441ba1cfc4..841a3613af3 100644 --- a/src/gui/folderman.h +++ b/src/gui/folderman.h @@ -217,7 +217,7 @@ class OWNCLOUDGUI_EXPORT FolderMan : public QObject * incoming relative server path. The method checks with all existing sync * folders. */ - QStringList findFileInLocalFolders(const QString &relPath, const AccountPtr acc); + QStringList findFileInLocalFolders(const QString &relPath, const Account *acc); /** Returns the folder by id or NULL if no folder with the id exists. */ [[deprecated("directly reference the folder")]] Folder *folder(const QByteArray &id); diff --git a/src/gui/models/activitylistmodel.cpp b/src/gui/models/activitylistmodel.cpp index aeec6d8560d..1a07a5b9da3 100644 --- a/src/gui/models/activitylistmodel.cpp +++ b/src/gui/models/activitylistmodel.cpp @@ -51,7 +51,7 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const const auto &a = _finalList.at(index.row()); AccountState *accountState = AccountManager::instance()->accountState(a.accountUuid()); - if (!accountState) { + if (!accountState || !accountState->account()) { return {}; } const auto column = static_cast(index.column()); @@ -71,12 +71,12 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const return Utility::timeAgoInWords(a.dateTime()); } case ActivityRole::Path: { - QStringList list = FolderMan::instance()->findFileInLocalFolders(a.file(), accountState->account()); + QStringList list = FolderMan::instance()->findFileInLocalFolders(a.file(), accountState->account().get()); if (!list.isEmpty()) { return list.at(0); } // File does not exist anymore? Let's try to open its path - list = FolderMan::instance()->findFileInLocalFolders(QFileInfo(a.file()).path(), accountState->account()); + list = FolderMan::instance()->findFileInLocalFolders(QFileInfo(a.file()).path(), accountState->account().get()); if (!list.isEmpty()) { return list.at(0); } diff --git a/src/gui/newaccountwizard/newaccountbuilder.cpp b/src/gui/newaccountwizard/newaccountbuilder.cpp index bb82ee91783..e0914b0a1b5 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.cpp +++ b/src/gui/newaccountwizard/newaccountbuilder.cpp @@ -84,7 +84,7 @@ void NewAccountBuilder::completeAccountSetup() // if selective sync is selected, we need to run the folder wizard asap. if (_syncType == NewAccount::SyncType::SELECTIVE_SYNC) - Q_EMIT requestFolderWizard(_account); + Q_EMIT requestFolderWizard(_account.get()); deleteLater(); } diff --git a/src/gui/newaccountwizard/newaccountbuilder.h b/src/gui/newaccountwizard/newaccountbuilder.h index 8be34157a05..cd5dd3a486f 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.h +++ b/src/gui/newaccountwizard/newaccountbuilder.h @@ -32,8 +32,8 @@ class NewAccountBuilder : public QObject void buildAccount(); Q_SIGNALS: - void requestSetUpSyncFoldersForAccount(AccountState *, bool useVfs); - void requestFolderWizard(OCC::AccountPtr account); + void requestSetUpSyncFoldersForAccount(OCC::AccountState *, bool useVfs); + void requestFolderWizard(OCC::Account *account); void unableToCompleteAccountCreation(const QString &error); private: diff --git a/src/gui/settingsdialog.cpp b/src/gui/settingsdialog.cpp index 46fc8979f44..a0c6ac4098c 100644 --- a/src/gui/settingsdialog.cpp +++ b/src/gui/settingsdialog.cpp @@ -296,9 +296,11 @@ void SettingsDialog::createNewAccount() ocApp()->gui()->runAccountWizard(); } -void SettingsDialog::runFolderWizard(AccountPtr account) +void SettingsDialog::runFolderWizard(Account *account) { - setCurrentAccount(account.get()); + if (!account) + return; + setCurrentAccount(account); accountSettings(_currentAccount)->slotAddFolder(); } diff --git a/src/gui/settingsdialog.h b/src/gui/settingsdialog.h index cc3647af2b6..f3192a3c041 100644 --- a/src/gui/settingsdialog.h +++ b/src/gui/settingsdialog.h @@ -71,7 +71,7 @@ class SettingsDialog : public QMainWindow public Q_SLOTS: // this is a direct call from QML void createNewAccount(); - void runFolderWizard(AccountPtr account); + void runFolderWizard(Account *account); Q_SIGNALS: diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index 104e35fda22..24a6ca3ea50 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -667,7 +667,7 @@ Q_INVOKABLE void OCC::SocketApi::command_OPEN_APP_LINK(const QString &localFile, const auto &provider = data.folder->accountState()->account()->appProvider(); const auto record = data.journalRecord(); if (record.isValid()) { - provider.open(data.folder->accountState()->account(), localFile, record._fileId); + provider.open(data.folder->accountState()->account().get(), localFile, record._fileId); } } } diff --git a/src/libsync/appprovider.cpp b/src/libsync/appprovider.cpp index 9afd1cd07a6..4409a40de13 100644 --- a/src/libsync/appprovider.cpp +++ b/src/libsync/appprovider.cpp @@ -73,12 +73,15 @@ const AppProvider::Provider &AppProvider::app(const QString &localPath) const return app(mimeType); } -bool AppProvider::open(const AccountPtr &account, const QString &localPath, const QByteArray &fileId) const +bool AppProvider::open(Account *account, const QString &localPath, const QByteArray &fileId) const { + if (!account) + return false; + const auto &a = app(localPath); if (a.isValid()) { SimpleNetworkJob::UrlQuery query { { QStringLiteral("file_id"), QString::fromUtf8(fileId) } }; - auto *job = new JsonJob(account, account->capabilities().appProviders().openWebUrl, {}, "POST", query); + auto *job = new JsonJob(account->sharedFromThis(), account->capabilities().appProviders().openWebUrl, {}, "POST", query); QObject::connect(job, &JsonJob::finishedSignal, [account, job, localPath] { if (job->httpStatusCode() == 200) { const auto url = QUrl(job->data().value(QStringLiteral("uri")).toString()); diff --git a/src/libsync/appprovider.h b/src/libsync/appprovider.h index 8a1a473222f..e883f2c4a24 100644 --- a/src/libsync/appprovider.h +++ b/src/libsync/appprovider.h @@ -53,7 +53,7 @@ class OWNCLOUDSYNC_EXPORT AppProvider const Provider &app(const QMimeType &mimeType) const; const Provider &app(const QString &localPath) const; - bool open(const AccountPtr &account, const QString &localPath, const QByteArray &fileId) const; + bool open(Account *account, const QString &localPath, const QByteArray &fileId) const; private: From 6465b9b06883899cf39770f2c5550b9e4ae6d1f2 Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 10:25:45 +0200 Subject: [PATCH 16/23] updated the syncEngine account pointer --- src/libsync/syncengine.cpp | 12 +++++++++--- src/libsync/syncengine.h | 6 ++---- test/testutils/syncenginetestutils.cpp | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 2c6f005a8d6..6a476e4b5ab 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -44,7 +44,7 @@ Q_LOGGING_CATEGORY(lcEngine, "sync.engine", QtInfoMsg) // doc in header std::chrono::seconds SyncEngine::minimumFileAgeForUpload(2s); -SyncEngine::SyncEngine(AccountPtr account, const QUrl &baseUrl, const QString &localPath, const QString &remotePath, OCC::SyncJournalDb *journal) +SyncEngine::SyncEngine(Account *account, const QUrl &baseUrl, const QString &localPath, const QString &remotePath, OCC::SyncJournalDb *journal) : _account(account) , _baseUrl(baseUrl) , _needsUpdate(false) @@ -256,6 +256,9 @@ void SyncEngine::conflictRecordMaintenance() void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) { + if (!_account) + return; + if (Utility::isConflictFile(item->_file)) _seenConflictFiles.insert(item->_file); if (item->instruction() == CSYNC_INSTRUCTION_NONE) { @@ -296,6 +299,9 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) void SyncEngine::startSync() { + if (!_account) + return; + if (_syncRunning) { OC_ASSERT(false); return; @@ -385,7 +391,7 @@ void SyncEngine::startSync() // TODO: add a constructor to DiscoveryPhase // pass a syncEngine object rather than copying everything to another object - _discoveryPhase.reset(new DiscoveryPhase(_account, syncOptions(), _baseUrl)); + _discoveryPhase.reset(new DiscoveryPhase(_account->sharedFromThis(), syncOptions(), _baseUrl)); _discoveryPhase->_excludes = _excludedFiles.get(); _discoveryPhase->_statedb = _journal; _discoveryPhase->_localDir = _localPath; @@ -552,7 +558,7 @@ void SyncEngine::slotDiscoveryFinished() // do a database commit _journal->commit(QStringLiteral("post treewalk")); - _propagator = QSharedPointer::create(_account, syncOptions(), _baseUrl, _localPath, _remotePath, _journal); + _propagator = QSharedPointer::create(_account->sharedFromThis(), syncOptions(), _baseUrl, _localPath, _remotePath, _journal); connect(_propagator.data(), &OwncloudPropagator::itemCompleted, this, &SyncEngine::slotItemCompleted); connect(_propagator.data(), &OwncloudPropagator::progress, diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index ad33d9f637a..f9450b856ac 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -18,7 +18,6 @@ #include "accountfwd.h" #include "common/checksums.h" #include "common/chronoelapsedtimer.h" -#include "common/utility.h" #include "discoveryphase.h" #include "progressdispatcher.h" #include "syncfileitem.h" @@ -52,8 +51,7 @@ class OWNCLOUDSYNC_EXPORT SyncEngine : public QObject { Q_OBJECT public: - SyncEngine(AccountPtr account, const QUrl &baseUrl, const QString &localPath, - const QString &remotePath, SyncJournalDb *journal); + SyncEngine(Account *account, const QUrl &baseUrl, const QString &localPath, const QString &remotePath, SyncJournalDb *journal); ~SyncEngine() override; Q_INVOKABLE void startSync(); @@ -204,7 +202,7 @@ private Q_SLOTS: // Must only be acessed during update and reconcile SyncFileItemSet _syncItems; - AccountPtr _account; + QPointer _account; const QUrl _baseUrl; bool _needsUpdate; bool _syncRunning; diff --git a/test/testutils/syncenginetestutils.cpp b/test/testutils/syncenginetestutils.cpp index 5c20407bf34..e9a7830cc16 100644 --- a/test/testutils/syncenginetestutils.cpp +++ b/test/testutils/syncenginetestutils.cpp @@ -887,7 +887,7 @@ FakeFolder::FakeFolder(const FileInfo &fileTemplate, OCC::Vfs::Mode vfsMode, boo _journalDb.reset(new OCC::SyncJournalDb(localPath() + QStringLiteral(".sync_test.db"))); // TODO: davUrl - _syncEngine.reset(new OCC::SyncEngine(account(), account()->davUrl(), localPath(), QString(), _journalDb.get())); + _syncEngine.reset(new OCC::SyncEngine(account().get(), account()->davUrl(), localPath(), QString(), _journalDb.get())); _syncEngine->setSyncOptions(OCC::SyncOptions { QSharedPointer(OCC::VfsPluginManager::instance().createVfsFromPlugin(vfsMode).release()) }); // Ignore temporary files from the download. (This is in the default exclude list, but we don't load it) @@ -926,7 +926,7 @@ void FakeFolder::switchToVfs(QSharedPointer vfs) opts._vfs = vfs; _syncEngine->setSyncOptions(opts); - OCC::VfsSetupParams vfsParams(account(), account()->davUrl(), false, &syncEngine()); + OCC::VfsSetupParams vfsParams(account()->sharedFromThis(), account()->davUrl(), false, &syncEngine()); vfsParams.filesystemPath = localPath(); vfsParams.remotePath = QLatin1Char('/'); vfsParams.journal = _journalDb.get(); From 47d626ba7fd7c9d067471558bec2021d1cc04b8b Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 12:57:06 +0200 Subject: [PATCH 17/23] AbstractNetworkJob now uses QPointer for the account (as do all subclasses now) --- src/gui/accountsettings.cpp | 2 +- src/gui/activitywidget.cpp | 4 +- src/gui/connectionvalidator.cpp | 2 +- src/gui/fetchserversettings.cpp | 8 +-- src/gui/models/activitylistmodel.cpp | 64 +++++++++---------- src/gui/notificationconfirmjob.cpp | 2 +- src/gui/notificationconfirmjob.h | 2 +- src/gui/owncloudgui.cpp | 2 +- src/gui/protocolwidget.cpp | 2 +- src/gui/selectivesyncwidget.cpp | 4 +- src/gui/servernotificationhandler.cpp | 22 +++---- src/gui/socketapi/socketapi.cpp | 16 ++--- src/libsync/abstractnetworkjob.cpp | 11 +++- src/libsync/abstractnetworkjob.h | 4 +- src/libsync/appprovider.cpp | 2 +- src/libsync/discovery.cpp | 4 +- src/libsync/discoveryphase.cpp | 2 +- src/libsync/graphapi/jobs/drives.cpp | 2 +- src/libsync/graphapi/jobs/drives.h | 2 +- src/libsync/graphapi/spacesmanager.cpp | 2 +- src/libsync/networkjobs.cpp | 32 ++++++---- src/libsync/networkjobs.h | 29 +++++---- src/libsync/networkjobs/jsonjob.cpp | 4 +- src/libsync/networkjobs/jsonjob.h | 4 +- src/libsync/networkjobs/resources.cpp | 2 +- src/libsync/propagatedownload.cpp | 15 ++--- src/libsync/propagatedownload.h | 2 +- src/libsync/propagateremotedelete.cpp | 6 +- src/libsync/propagateremotedelete.h | 2 +- src/libsync/propagateremotemkdir.cpp | 10 +-- src/libsync/propagateremotemove.cpp | 5 +- src/libsync/propagateremotemove.h | 4 +- src/libsync/propagateuploadcommon.cpp | 8 +-- src/libsync/propagateuploadfile.cpp | 6 +- src/libsync/propagateuploadtus.cpp | 11 ++-- src/libsync/putfilejob.cpp | 3 +- src/libsync/putfilejob.h | 4 +- .../testconnectionvalidator.cpp | 2 +- test/testjobqueue.cpp | 2 +- test/testutils/syncenginetestutils.cpp | 2 +- test/testutils/syncenginetestutils.h | 2 +- 41 files changed, 153 insertions(+), 161 deletions(-) diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 22288a7f2e4..65c83557bb4 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -187,7 +187,7 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder) if (folder->accountState()->account()->capabilities().privateLinkPropertyAvailable()) { QString path = folder->remotePathTrailingSlash(); menu->addAction(CommonStrings::showInWebBrowser(), [path, davUrl = folder->webDavUrl(), this] { - fetchPrivateLinkUrl(_accountState->account(), davUrl, path, this, [](const QUrl &url) { Utility::openBrowser(url, nullptr); }); + fetchPrivateLinkUrl(_accountState->account().get(), davUrl, path, this, [](const QUrl &url) { Utility::openBrowser(url, nullptr); }); }); } diff --git a/src/gui/activitywidget.cpp b/src/gui/activitywidget.cpp index 563f344835a..9c284a403ba 100644 --- a/src/gui/activitywidget.cpp +++ b/src/gui/activitywidget.cpp @@ -36,8 +36,6 @@ #include "ui_activitywidget.h" -#include - using namespace std::chrono; using namespace std::chrono_literals; @@ -319,7 +317,7 @@ void ActivityWidget::slotSendNotificationRequest(const QString &accountName, con if (validVerbs.contains(verb)) { if (auto acc = AccountManager::instance()->account(accountName)) { // TODO: host validation? - auto *job = new NotificationConfirmJob(acc->account(), QUrl(link), verb, this); + auto *job = new NotificationConfirmJob(acc->account().get(), QUrl(link), verb, this); job->setWidget(theSender); connect(job, &NotificationConfirmJob::finishedSignal, this, [job, this] { diff --git a/src/gui/connectionvalidator.cpp b/src/gui/connectionvalidator.cpp index 1055137bfa5..9083ff82c68 100644 --- a/src/gui/connectionvalidator.cpp +++ b/src/gui/connectionvalidator.cpp @@ -230,7 +230,7 @@ void ConnectionValidator::checkAuthentication() // we explicitly use a legacy dav path here - remote.php/webdav is available across all former, current and future server implementations (oc10, oCIS and // Kiteworks PDN) - auto *job = new PropfindJob(_account->sharedFromThis(), _account->url(), QString("remote.php/webdav/"), PropfindJob::Depth::Zero, this); + auto *job = new PropfindJob(_account, _account->url(), QString("remote.php/webdav/"), PropfindJob::Depth::Zero, this); job->setAuthenticationJob(true); // don't retry job->setTimeout(timeoutToUse()); diff --git a/src/gui/fetchserversettings.cpp b/src/gui/fetchserversettings.cpp index c23bdcc704f..8ac2793ce28 100644 --- a/src/gui/fetchserversettings.cpp +++ b/src/gui/fetchserversettings.cpp @@ -44,7 +44,7 @@ FetchServerSettingsJob::FetchServerSettingsJob(const OCC::AccountPtr &account, Q void FetchServerSettingsJob::start() { // The main flow now needs the capabilities - auto *job = new JsonApiJob(_account, QStringLiteral("ocs/v2.php/cloud/capabilities"), {}, {}, this); + auto *job = new JsonApiJob(_account.get(), QStringLiteral("ocs/v2.php/cloud/capabilities"), {}, {}, this); job->setAuthenticationJob(isAuthJob()); job->setTimeout(fetchSettingsTimeout()); @@ -71,7 +71,7 @@ void FetchServerSettingsJob::start() Q_EMIT finishedSignal(Result::UnsupportedServer); return; } - auto *userJob = new JsonApiJob(_account, QStringLiteral("ocs/v2.php/cloud/user"), SimpleNetworkJob::UrlQuery{}, QNetworkRequest{}, this); + auto *userJob = new JsonApiJob(_account.get(), QStringLiteral("ocs/v2.php/cloud/user"), SimpleNetworkJob::UrlQuery{}, QNetworkRequest{}, this); userJob->setAuthenticationJob(isAuthJob()); userJob->setTimeout(fetchSettingsTimeout()); connect(userJob, &JsonApiJob::finishedSignal, this, [userJob, this] { @@ -124,13 +124,13 @@ void FetchServerSettingsJob::runAsyncUpdates() // so we just set them free if (_account->capabilities().avatarsAvailable()) { // the avatar job uses the legacy WebDAV URL and ocis will require a new approach - auto *avatarJob = new AvatarJob(_account, _account->davUser(), 128, nullptr); + auto *avatarJob = new AvatarJob(_account.get(), _account->davUser(), 128, nullptr); connect(avatarJob, &AvatarJob::avatarPixmap, this, [this](const QPixmap &img) { _account->setAvatar(AvatarJob::makeCircularAvatar(img)); }); avatarJob->start(); }; if (_account->capabilities().appProviders().enabled) { - auto *jsonJob = new JsonJob(_account, _account->capabilities().appProviders().appsUrl, {}, "GET"); + auto *jsonJob = new JsonJob(_account.get(), _account->capabilities().appProviders().appsUrl, {}, "GET"); connect(jsonJob, &JsonJob::finishedSignal, this, [jsonJob, this] { _account->setAppProvider(AppProvider{jsonJob->data()}); }); jsonJob->start(); } diff --git a/src/gui/models/activitylistmodel.cpp b/src/gui/models/activitylistmodel.cpp index 1a07a5b9da3..127ce9887e0 100644 --- a/src/gui/models/activitylistmodel.cpp +++ b/src/gui/models/activitylistmodel.cpp @@ -181,48 +181,48 @@ bool ActivityListModel::canFetchMore(const QModelIndex &) const return false; } -void ActivityListModel::startFetchJob(AccountState *ast) +void ActivityListModel::startFetchJob(AccountState *accountState) { - if (!ast || !ast->isConnected()) { + if (!accountState || !accountState->isConnected() || !accountState->account()) { return; } - auto *job = new JsonApiJob(ast->account(), QStringLiteral("ocs/v2.php/cloud/activity"), { { QStringLiteral("page"), QStringLiteral("0") }, { QStringLiteral("pagesize"), QStringLiteral("100") } }, {}, this); + auto *job = new JsonApiJob(accountState->account().get(), QStringLiteral("ocs/v2.php/cloud/activity"), + {{QStringLiteral("page"), QStringLiteral("0")}, {QStringLiteral("pagesize"), QStringLiteral("100")}}, {}, this); - QObject::connect( - job, &JsonApiJob::finishedSignal, this, [job, ast, this] { - _currentlyFetching.remove(ast); - const auto activities = job->data().value(QStringLiteral("ocs")).toObject().value(QStringLiteral("data")).toArray(); + QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, accountState, this] { + _currentlyFetching.remove(accountState); + const auto activities = job->data().value(QStringLiteral("ocs")).toObject().value(QStringLiteral("data")).toArray(); - /* - * in case the activity app is disabled or not installed, the server returns an empty 500 response instead of a response - * with the expected status code 999 - * we are not entirely sure when this has changed, but it is likely that there is a regression in the activity addon - * to support this new behavior, we have to fake the expected status code - */ - if (job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() != 200) { - Q_EMIT activityJobStatusCode(ast, 999); - return; - } + /* + * in case the activity app is disabled or not installed, the server returns an empty 500 response instead of a response + * with the expected status code 999 + * we are not entirely sure when this has changed, but it is likely that there is a regression in the activity addon + * to support this new behavior, we have to fake the expected status code + */ + if (job->reply()->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() != 200) { + Q_EMIT activityJobStatusCode(accountState, 999); + return; + } - ActivityList list; - list.reserve(activities.size()); - for (const auto &activ : activities) { - const auto json = activ.toObject(); - list.append(Activity{Activity::ActivityType, json.value(QStringLiteral("id")).toString(), ast->account()->displayNameWithHost(), - ast->account()->uuid(), json.value(QStringLiteral("subject")).toString(), json.value(QStringLiteral("message")).toString(), - json.value(QStringLiteral("file")).toString(), QUrl(json.value(QStringLiteral("link")).toString()), - QDateTime::fromString(json.value(QStringLiteral("date")).toString(), Qt::ISODate)}); - } + ActivityList list; + list.reserve(activities.size()); + for (const auto &activ : activities) { + const auto json = activ.toObject(); + list.append(Activity{Activity::ActivityType, json.value(QStringLiteral("id")).toString(), accountState->account()->displayNameWithHost(), + accountState->account()->uuid(), json.value(QStringLiteral("subject")).toString(), json.value(QStringLiteral("message")).toString(), + json.value(QStringLiteral("file")).toString(), QUrl(json.value(QStringLiteral("link")).toString()), + QDateTime::fromString(json.value(QStringLiteral("date")).toString(), Qt::ISODate)}); + } - _activityLists[ast] = std::move(list); + _activityLists[accountState] = std::move(list); - Q_EMIT activityJobStatusCode(ast, job->ocsStatus()); + Q_EMIT activityJobStatusCode(accountState, job->ocsStatus()); - combineActivityLists(); - }); + combineActivityLists(); + }); - _currentlyFetching.insert(ast, job); - qCInfo(lcActivity) << "Start fetching activities for " << ast->account()->displayNameWithHost(); + _currentlyFetching.insert(accountState, job); + qCInfo(lcActivity) << "Start fetching activities for " << accountState->account()->displayNameWithHost(); job->start(); } diff --git a/src/gui/notificationconfirmjob.cpp b/src/gui/notificationconfirmjob.cpp index b237df11bc6..7778f37c610 100644 --- a/src/gui/notificationconfirmjob.cpp +++ b/src/gui/notificationconfirmjob.cpp @@ -21,7 +21,7 @@ namespace OCC { Q_DECLARE_LOGGING_CATEGORY(lcNotifications) -NotificationConfirmJob::NotificationConfirmJob(AccountPtr account, const QUrl &rootUrl, const QByteArray &verb, QObject *parent) +NotificationConfirmJob::NotificationConfirmJob(Account *account, const QUrl &rootUrl, const QByteArray &verb, QObject *parent) : JsonApiJob(account, rootUrl, {}, verb, {}, {}, parent) { } diff --git a/src/gui/notificationconfirmjob.h b/src/gui/notificationconfirmjob.h index 6a4356c839c..df2a8b91327 100644 --- a/src/gui/notificationconfirmjob.h +++ b/src/gui/notificationconfirmjob.h @@ -41,7 +41,7 @@ class NotificationConfirmJob : public JsonApiJob Q_OBJECT public: - explicit NotificationConfirmJob(AccountPtr account, const QUrl &root, const QByteArray &verb, QObject *parent = nullptr); + explicit NotificationConfirmJob(Account *account, const QUrl &root, const QByteArray &verb, QObject *parent = nullptr); /** * @brief Start the OCS request diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp index 56ce2d2cd81..04e4741ed49 100644 --- a/src/gui/owncloudgui.cpp +++ b/src/gui/owncloudgui.cpp @@ -319,7 +319,7 @@ void ownCloudGui::slotShowShareInBrowser(const QString &sharePath, const QString } if (folder->accountState()->account()->capabilities().filesSharing().sharing_roles) { - fetchPrivateLinkUrl(folder->accountState()->account(), folder->webDavUrl(), sharePath, this, [](const QUrl &url) { + fetchPrivateLinkUrl(folder->accountState()->account().get(), folder->webDavUrl(), sharePath, this, [](const QUrl &url) { const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("sharing")}}); Utility::openBrowser(queryUrl, nullptr); }); diff --git a/src/gui/protocolwidget.cpp b/src/gui/protocolwidget.cpp index 39fc3209cad..a092b2161c9 100644 --- a/src/gui/protocolwidget.cpp +++ b/src/gui/protocolwidget.cpp @@ -134,7 +134,7 @@ void ProtocolWidget::showContextMenu(QWidget *parent, QTableView *table, Models: // "Open in Browser" action auto showInWebBrowserAction = menu->addAction(CommonStrings::showInWebBrowser()); showInWebBrowserAction->setEnabled(false); - fetchPrivateLinkUrl(data.folder()->accountState()->account(), data.folder()->webDavUrl(), data.folder()->remotePathTrailingSlash() + data.path(), + fetchPrivateLinkUrl(data.folder()->accountState()->account().get(), data.folder()->webDavUrl(), data.folder()->remotePathTrailingSlash() + data.path(), parent, [showInWebBrowserAction, parent, pos = menu->actions().size(), menu = QPointer(menu)](const QUrl &url) { // as fetchPrivateLinkUrl is async we need to check the menu still exists if (menu) { diff --git a/src/gui/selectivesyncwidget.cpp b/src/gui/selectivesyncwidget.cpp index da3e0ea1cb8..f1f73202ce6 100644 --- a/src/gui/selectivesyncwidget.cpp +++ b/src/gui/selectivesyncwidget.cpp @@ -104,7 +104,7 @@ void SelectiveSyncWidget::refreshFolders() if (!_account) return; - auto *job = new PropfindJob(_account->sharedFromThis(), davUrl(), _folderPath, PropfindJob::Depth::One, this); + auto *job = new PropfindJob(_account, davUrl(), _folderPath, PropfindJob::Depth::One, this); job->setProperties({QByteArrayLiteral("resourcetype"), QByteArrayLiteral("http://owncloud.org/ns:size")}); connect(job, &PropfindJob::directoryListingSubfolders, this, &SelectiveSyncWidget::slotUpdateDirectories); connect(job, &PropfindJob::finishedWithError, this, [job, this] { @@ -301,7 +301,7 @@ void SelectiveSyncWidget::slotItemExpanded(QTreeWidgetItem *item) if (dir.isEmpty()) { return; } - PropfindJob *job = new PropfindJob(_account->sharedFromThis(), davUrl(), Utility::concatUrlPathItems({_folderPath, dir}), PropfindJob::Depth::One, this); + PropfindJob *job = new PropfindJob(_account, davUrl(), Utility::concatUrlPathItems({_folderPath, dir}), PropfindJob::Depth::One, this); job->setProperties({QByteArrayLiteral("resourcetype"), QByteArrayLiteral("http://owncloud.org/ns:size")}); connect(job, &PropfindJob::directoryListingSubfolders, this, &SelectiveSyncWidget::slotUpdateDirectories); job->start(); diff --git a/src/gui/servernotificationhandler.cpp b/src/gui/servernotificationhandler.cpp index 4e81db3e431..ea6605ea2b8 100644 --- a/src/gui/servernotificationhandler.cpp +++ b/src/gui/servernotificationhandler.cpp @@ -30,18 +30,19 @@ ServerNotificationHandler::ServerNotificationHandler(QObject *parent) { } -void ServerNotificationHandler::slotFetchNotifications(AccountState *ptr) +void ServerNotificationHandler::slotFetchNotifications(AccountState *accountState) { // check connectivity and credentials - if (!(ptr && ptr->isConnected() && ptr->account() && ptr->account()->credentials() && ptr->account()->credentials()->ready())) { + if (!(accountState && accountState->isConnected() && accountState->account() && accountState->account()->credentials() + && accountState->account()->credentials()->ready())) { deleteLater(); return; } // check if the account has notifications enabled. If the capabilities are // not yet valid, its assumed that notifications are available. - if (ptr->account()->hasCapabilities()) { - if (!ptr->account()->capabilities().notificationsAvailable()) { - qCInfo(lcServerNotification) << "Account" << ptr->account()->displayNameWithHost() << "does not have notifications enabled."; + if (accountState->account()->hasCapabilities()) { + if (!accountState->account()->capabilities().notificationsAvailable()) { + qCInfo(lcServerNotification) << "Account" << accountState->account()->displayNameWithHost() << "does not have notifications enabled."; deleteLater(); return; } @@ -51,12 +52,11 @@ void ServerNotificationHandler::slotFetchNotifications(AccountState *ptr) } // if the previous notification job has finished, start next. - auto *job = new JsonApiJob(ptr->account(), notificationsPath, {}, {}, this); - QObject::connect(job, &JsonApiJob::finishedSignal, - this, [job, ptr, this] { - slotNotificationsReceived(job, ptr); - deleteLater(); - }); + auto *job = new JsonApiJob(accountState->account().get(), notificationsPath, {}, {}, this); + QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, accountState, this] { + slotNotificationsReceived(job, accountState); + deleteLater(); + }); job->start(); } diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index 24a6ca3ea50..8297d1de480 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -508,12 +508,7 @@ void SocketApi::fetchPrivateLinkUrlHelper(const QString &localFile, const std::f return; } - fetchPrivateLinkUrl( - fileData.folder->accountState()->account(), - fileData.folder->webDavUrl(), - fileData.serverRelativePath, - this, - targetFun); + fetchPrivateLinkUrl(fileData.folder->accountState()->account().get(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, targetFun); } void SocketApi::command_COPY_PRIVATE_LINK(const QString &localFile, SocketListener *) @@ -535,10 +530,11 @@ void SocketApi::command_OPEN_PRIVATE_LINK_VERSIONS(const QString &localFile, Soc { const auto fileData = FileData::get(localFile); if (fileData.isValid() && fileData.folder->accountState()->account()->capabilities().filesSharing().sharing_roles) { - fetchPrivateLinkUrl(fileData.folder->accountState()->account(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, [](const QUrl &url) { - const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("versions")}}); - Utility::openBrowser(queryUrl, nullptr); - }); + fetchPrivateLinkUrl( + fileData.folder->accountState()->account().get(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, [](const QUrl &url) { + const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("versions")}}); + Utility::openBrowser(queryUrl, nullptr); + }); } else { fetchPrivateLinkUrlHelper(localFile, [](const QUrl &link) { Utility::openBrowser(Utility::concatUrlPath(link, {}, {{QStringLiteral("details"), QStringLiteral("versionsTabView")}}), nullptr); diff --git a/src/libsync/abstractnetworkjob.cpp b/src/libsync/abstractnetworkjob.cpp index 5fd420de2da..151ae2ab594 100644 --- a/src/libsync/abstractnetworkjob.cpp +++ b/src/libsync/abstractnetworkjob.cpp @@ -46,7 +46,7 @@ seconds AbstractNetworkJob::httpTimeout = [] { return seconds(def); }(); -AbstractNetworkJob::AbstractNetworkJob(AccountPtr account, const QUrl &baseUrl, const QString &path, QObject *parent) +AbstractNetworkJob::AbstractNetworkJob(Account *account, const QUrl &baseUrl, const QString &path, QObject *parent) : QObject(parent) , _account(account) , _baseUrl(baseUrl) @@ -128,6 +128,11 @@ bool AbstractNetworkJob::needsRetry() const void AbstractNetworkJob::sendRequest(const QByteArray &verb, const QNetworkRequest &req, QIODevice *requestBody) { + if (!_account) { + slotFinished(); + return; + } + _verb = verb; _request = req; @@ -183,8 +188,8 @@ void AbstractNetworkJob::slotFinished() { _finished = true; - if (!_reply) { - qCWarning(lcNetworkJob) << "Network job finished but reply is nullptr - aborting" << this; + if (!_reply || !_account) { + qCWarning(lcNetworkJob) << "Network job finished but reply and/or account is nullptr - aborting" << this; Q_EMIT networkError(nullptr); deleteLater(); return; diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index bce7e2a5faf..aa6773b69ef 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -51,7 +51,7 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject { Q_OBJECT public: - explicit AbstractNetworkJob(AccountPtr account, const QUrl &baseUrl, const QString &path, QObject *parent = nullptr); + explicit AbstractNetworkJob(Account *account, const QUrl &baseUrl, const QString &path, QObject *parent = nullptr); ~AbstractNetworkJob() override; virtual void start(); @@ -197,7 +197,7 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject */ void setQuery(const QUrlQuery &query); - AccountPtr _account; + QPointer _account; QUrlQuery _query; QByteArray _responseTimestamp; diff --git a/src/libsync/appprovider.cpp b/src/libsync/appprovider.cpp index 4409a40de13..5a198dcb25a 100644 --- a/src/libsync/appprovider.cpp +++ b/src/libsync/appprovider.cpp @@ -81,7 +81,7 @@ bool AppProvider::open(Account *account, const QString &localPath, const QByteAr const auto &a = app(localPath); if (a.isValid()) { SimpleNetworkJob::UrlQuery query { { QStringLiteral("file_id"), QString::fromUtf8(fileId) } }; - auto *job = new JsonJob(account->sharedFromThis(), account->capabilities().appProviders().openWebUrl, {}, "POST", query); + auto *job = new JsonJob(account, account->capabilities().appProviders().openWebUrl, {}, "POST", query); QObject::connect(job, &JsonJob::finishedSignal, [account, job, localPath] { if (job->httpStatusCode() == 200) { const auto url = QUrl(job->data().value(QStringLiteral("uri")).toString()); diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index b12b3c46fa1..2eb5c9f9b32 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -549,7 +549,7 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( } else { // we need to make a request to the server to know that the original file is deleted on the server _pendingAsyncJobs++; - auto job = new RequestEtagJob(_discoveryData->_account, _discoveryData->_baseUrl, _discoveryData->_remoteFolder + originalPath, this); + auto job = new RequestEtagJob(_discoveryData->_account.get(), _discoveryData->_baseUrl, _discoveryData->_remoteFolder + originalPath, this); connect(job, &RequestEtagJob::finishedSignal, this, [=]() mutable { _pendingAsyncJobs--; QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs); @@ -913,7 +913,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( // We must query the server to know if the etag has not changed _pendingAsyncJobs++; QString serverOriginalPath = _discoveryData->_remoteFolder + _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down); - auto job = new RequestEtagJob(_discoveryData->_account, _discoveryData->_baseUrl, serverOriginalPath, this); + auto job = new RequestEtagJob(_discoveryData->_account.get(), _discoveryData->_baseUrl, serverOriginalPath, this); connect(job, &RequestEtagJob::finishedSignal, this, [job, recurseQueryServer, path = path, postProcessLocalNew, processRename, base, item, originalPath, this] { if (job->httpStatusCode() == 404 || (job->etag().toUtf8() != base._etag && !item->isDirectory()) || _discoveryData->isRenamed(originalPath)) { diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index 6d6bfb4ba14..e3f2e67f66f 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -274,7 +274,7 @@ DiscoverySingleDirectoryJob::DiscoverySingleDirectoryJob(const AccountPtr &accou void DiscoverySingleDirectoryJob::start() { // Start the actual HTTP job - _proFindJob = new PropfindJob(_account, _baseUrl, _subPath, PropfindJob::Depth::One, this); + _proFindJob = new PropfindJob(_account.get(), _baseUrl, _subPath, PropfindJob::Depth::One, this); QList props { "resourcetype", diff --git a/src/libsync/graphapi/jobs/drives.cpp b/src/libsync/graphapi/jobs/drives.cpp index 296a8d2ff2a..416861d5a5e 100644 --- a/src/libsync/graphapi/jobs/drives.cpp +++ b/src/libsync/graphapi/jobs/drives.cpp @@ -28,7 +28,7 @@ namespace { const auto mountpointC = QLatin1String("mountpoint"); } -Drives::Drives(const AccountPtr &account, QObject *parent) +Drives::Drives(Account *account, QObject *parent) : JsonJob(account, account->url(), QStringLiteral("/graph/v1.0/me/drives"), "GET", {}, {}, parent) { } diff --git a/src/libsync/graphapi/jobs/drives.h b/src/libsync/graphapi/jobs/drives.h index 6bfb030917c..bda75cca589 100644 --- a/src/libsync/graphapi/jobs/drives.h +++ b/src/libsync/graphapi/jobs/drives.h @@ -26,7 +26,7 @@ namespace GraphApi { { Q_OBJECT public: - Drives(const AccountPtr &account, QObject *parent = nullptr); + Drives(Account *account, QObject *parent = nullptr); ~Drives(); const QList &drives() const; diff --git a/src/libsync/graphapi/spacesmanager.cpp b/src/libsync/graphapi/spacesmanager.cpp index 546a7833120..c5dd8c92bcb 100644 --- a/src/libsync/graphapi/spacesmanager.cpp +++ b/src/libsync/graphapi/spacesmanager.cpp @@ -55,7 +55,7 @@ void SpacesManager::refresh() } // TODO: leak the job until we fixed the ownership https://github.com/owncloud/client/issues/11203 - auto drivesJob = new Drives(_account->sharedFromThis(), nullptr); + auto drivesJob = new Drives(_account, nullptr); drivesJob->setTimeout(refreshTimeoutC); connect(drivesJob, &Drives::finishedSignal, this, [drivesJob, this] { // a system which provides multiple personal spaces the name of the drive is always used as display name diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp index 74acd41ba28..690eec2d98f 100644 --- a/src/libsync/networkjobs.cpp +++ b/src/libsync/networkjobs.cpp @@ -32,7 +32,7 @@ Q_LOGGING_CATEGORY(lcPropfindJob, "sync.networkjob.propfind", QtInfoMsg) Q_LOGGING_CATEGORY(lcAvatarJob, "sync.networkjob.avatar", QtInfoMsg) Q_LOGGING_CATEGORY(lcMkColJob, "sync.networkjob.mkcol", QtInfoMsg) -RequestEtagJob::RequestEtagJob(AccountPtr account, const QUrl &rootUrl, const QString &path, QObject *parent) +RequestEtagJob::RequestEtagJob(Account *account, const QUrl &rootUrl, const QString &path, QObject *parent) : PropfindJob(account, rootUrl, path, PropfindJob::Depth::Zero, parent) { setProperties({ QByteArrayLiteral("getetag") }); @@ -51,8 +51,7 @@ const QString &RequestEtagJob::etag() const } /*********************************************************************************************/ -MkColJob::MkColJob(AccountPtr account, const QUrl &url, const QString &path, - const QMap &extraHeaders, QObject *parent) +MkColJob::MkColJob(Account *account, const QUrl &url, const QString &path, const QMap &extraHeaders, QObject *parent) : AbstractNetworkJob(account, url, path, parent) , _extraHeaders(extraHeaders) { @@ -216,7 +215,7 @@ bool LsColXMLParser::parse(const QByteArray &xml, QHash *sizes, /*********************************************************************************************/ -PropfindJob::PropfindJob(AccountPtr account, const QUrl &url, const QString &path, Depth depth, QObject *parent) +PropfindJob::PropfindJob(Account *account, const QUrl &url, const QString &path, Depth depth, QObject *parent) : AbstractNetworkJob(account, url, path, parent) , _depth(depth) { @@ -323,7 +322,7 @@ const QHash &PropfindJob::sizes() const /*********************************************************************************************/ -AvatarJob::AvatarJob(AccountPtr account, const QString &userId, int size, QObject *parent) +AvatarJob::AvatarJob(Account *account, const QString &userId, int size, QObject *parent) : AbstractNetworkJob(account, account->url(), QStringLiteral("remote.php/dav/avatars/%1/%2.png").arg(userId, QString::number(size)), parent) { setStoreInCache(true); @@ -374,7 +373,7 @@ void AvatarJob::finished() /*********************************************************************************************/ -EntityExistsJob::EntityExistsJob(AccountPtr account, const QUrl &rootUrl, const QString &path, QObject *parent) +EntityExistsJob::EntityExistsJob(Account *account, const QUrl &rootUrl, const QString &path, QObject *parent) : AbstractNetworkJob(account, rootUrl, path, parent) { } @@ -392,14 +391,16 @@ void EntityExistsJob::finished() /*********************************************************************************************/ -SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QNetworkRequest &req, QObject *parent) +SimpleNetworkJob::SimpleNetworkJob( + Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QNetworkRequest &req, QObject *parent) : AbstractNetworkJob(account, rootUrl, path, parent) , _request(req) , _verb(verb) { } -SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) +SimpleNetworkJob::SimpleNetworkJob( + Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) : SimpleNetworkJob(account, rootUrl, path, verb, req, parent) { Q_ASSERT((QList { "GET", "PUT", "POST", "DELETE", "HEAD", "PATCH" }.contains(verb))); @@ -422,19 +423,22 @@ SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, cons } } -SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QJsonObject &arguments, const QNetworkRequest &req, QObject *parent) +SimpleNetworkJob::SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QJsonObject &arguments, + const QNetworkRequest &req, QObject *parent) : SimpleNetworkJob(account, rootUrl, path, verb, QJsonDocument(arguments).toJson(), req, parent) { _request.setHeader(QNetworkRequest::ContentTypeHeader, QStringLiteral("application/json")); } -SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QIODevice *requestBody, const QNetworkRequest &req, QObject *parent) +SimpleNetworkJob::SimpleNetworkJob( + Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QIODevice *requestBody, const QNetworkRequest &req, QObject *parent) : SimpleNetworkJob(account, rootUrl, path, verb, req, parent) { _device = requestBody; } -SimpleNetworkJob::SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QByteArray &&requestBody, const QNetworkRequest &req, QObject *parent) +SimpleNetworkJob::SimpleNetworkJob( + Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QByteArray &&requestBody, const QNetworkRequest &req, QObject *parent) : SimpleNetworkJob(account, rootUrl, path, verb, new QBuffer(&_body), req, parent) { _body = std::move(requestBody); @@ -470,10 +474,10 @@ void SimpleNetworkJob::newReplyHook(QNetworkReply *reply) } } -void fetchPrivateLinkUrl(AccountPtr account, const QUrl &baseUrl, const QString &remotePath, QObject *target, - const std::function &targetFun) +void fetchPrivateLinkUrl( + Account *account, const QUrl &baseUrl, const QString &remotePath, QObject *target, const std::function &targetFun) { - if (account->capabilities().privateLinkPropertyAvailable()) { + if (account && account->capabilities().privateLinkPropertyAvailable()) { // Retrieve the new link by PROPFIND auto *job = new PropfindJob(account, baseUrl, remotePath, PropfindJob::Depth::Zero, target); job->setProperties({ QByteArrayLiteral("http://owncloud.org/ns:privatelink") }); diff --git a/src/libsync/networkjobs.h b/src/libsync/networkjobs.h index 57c80553b90..8b8f2792631 100644 --- a/src/libsync/networkjobs.h +++ b/src/libsync/networkjobs.h @@ -43,7 +43,7 @@ class OWNCLOUDSYNC_EXPORT EntityExistsJob : public AbstractNetworkJob { Q_OBJECT public: - explicit EntityExistsJob(AccountPtr account, const QUrl &rootUrl, const QString &path, QObject *parent = nullptr); + explicit EntityExistsJob(Account *account, const QUrl &rootUrl, const QString &path, QObject *parent = nullptr); void start() override; Q_SIGNALS: @@ -79,7 +79,7 @@ class OWNCLOUDSYNC_EXPORT PropfindJob : public AbstractNetworkJob Zero, One } Q_ENUMS(Depth); - explicit PropfindJob(AccountPtr account, const QUrl &url, const QString &path, Depth depth, QObject *parent = nullptr); + explicit PropfindJob(Account *account, const QUrl &url, const QString &path, Depth depth, QObject *parent = nullptr); void start() override; /** @@ -127,7 +127,7 @@ class OWNCLOUDSYNC_EXPORT AvatarJob : public AbstractNetworkJob * @param userId The user for which to obtain the avatar * @param size The size of the avatar (square so size*size) */ - explicit AvatarJob(AccountPtr account, const QString &userId, int size, QObject *parent = nullptr); + explicit AvatarJob(Account *account, const QString &userId, int size, QObject *parent = nullptr); void start() override; @@ -155,8 +155,7 @@ class OWNCLOUDSYNC_EXPORT MkColJob : public AbstractNetworkJob HeaderMap _extraHeaders; public: - explicit MkColJob(AccountPtr account, const QUrl &url, const QString &path, - const HeaderMap &extraHeaders, QObject *parent = nullptr); + explicit MkColJob(Account *account, const QUrl &url, const QString &path, const HeaderMap &extraHeaders, QObject *parent = nullptr); void start() override; Q_SIGNALS: @@ -174,7 +173,7 @@ class OWNCLOUDSYNC_EXPORT RequestEtagJob : public PropfindJob { Q_OBJECT public: - explicit RequestEtagJob(AccountPtr account, const QUrl &rootUrl, const QString &path, QObject *parent = nullptr); + explicit RequestEtagJob(Account *account, const QUrl &rootUrl, const QString &path, QObject *parent = nullptr); const QString &etag() const; @@ -196,10 +195,14 @@ class OWNCLOUDSYNC_EXPORT SimpleNetworkJob : public AbstractNetworkJob using UrlQuery = QList>; // fully qualified urls can be passed in the QNetworkRequest - explicit SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QIODevice *requestBody = nullptr, const QNetworkRequest &req = {}, QObject *parent = nullptr); - explicit SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req = {}, QObject *parent = nullptr); - explicit SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QJsonObject &arguments, const QNetworkRequest &req = {}, QObject *parent = nullptr); - explicit SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QByteArray &&requestBody, const QNetworkRequest &req = {}, QObject *parent = nullptr); + explicit SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QIODevice *requestBody = nullptr, + const QNetworkRequest &req = {}, QObject *parent = nullptr); + explicit SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const UrlQuery &arguments, + const QNetworkRequest &req = {}, QObject *parent = nullptr); + explicit SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QJsonObject &arguments, + const QNetworkRequest &req = {}, QObject *parent = nullptr); + explicit SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, QByteArray &&requestBody, + const QNetworkRequest &req = {}, QObject *parent = nullptr); virtual ~SimpleNetworkJob(); @@ -214,7 +217,7 @@ class OWNCLOUDSYNC_EXPORT SimpleNetworkJob : public AbstractNetworkJob QNetworkRequest _request; private: - explicit SimpleNetworkJob(AccountPtr account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QNetworkRequest &req, QObject *parent); + explicit SimpleNetworkJob(Account *account, const QUrl &rootUrl, const QString &path, const QByteArray &verb, const QNetworkRequest &req, QObject *parent); QByteArray _verb; QByteArray _body; @@ -234,8 +237,8 @@ class OWNCLOUDSYNC_EXPORT SimpleNetworkJob : public AbstractNetworkJob * Note: targetFun is guaranteed to be called only through the event * loop and never directly. */ -void OWNCLOUDSYNC_EXPORT fetchPrivateLinkUrl(AccountPtr account, const QUrl &baseUrl, const QString &remotePath, QObject *target, - const std::function &targetFun); +void OWNCLOUDSYNC_EXPORT fetchPrivateLinkUrl( + Account *account, const QUrl &baseUrl, const QString &remotePath, QObject *target, const std::function &targetFun); } // namespace OCC diff --git a/src/libsync/networkjobs/jsonjob.cpp b/src/libsync/networkjobs/jsonjob.cpp index bf826ee6c71..d89fd075d21 100644 --- a/src/libsync/networkjobs/jsonjob.cpp +++ b/src/libsync/networkjobs/jsonjob.cpp @@ -60,7 +60,7 @@ const QJsonObject &JsonJob::data() const } -JsonApiJob::JsonApiJob(AccountPtr account, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) +JsonApiJob::JsonApiJob(Account *account, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) : JsonJob(account, account->url(), path, verb, arguments, req, parent) { _request.setRawHeader(QByteArrayLiteral("OCS-APIREQUEST"), QByteArrayLiteral("true")); @@ -72,7 +72,7 @@ JsonApiJob::JsonApiJob(AccountPtr account, const QString &path, const QByteArray setQuery(q); } -JsonApiJob::JsonApiJob(AccountPtr account, const QString &path, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) +JsonApiJob::JsonApiJob(Account *account, const QString &path, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent) : JsonApiJob(account, path, QByteArrayLiteral("GET"), arguments, req, parent) { } diff --git a/src/libsync/networkjobs/jsonjob.h b/src/libsync/networkjobs/jsonjob.h index f34cc825b4f..a97af454574 100644 --- a/src/libsync/networkjobs/jsonjob.h +++ b/src/libsync/networkjobs/jsonjob.h @@ -63,8 +63,8 @@ class OWNCLOUDSYNC_EXPORT JsonApiJob : public JsonJob { Q_OBJECT public: - explicit JsonApiJob(AccountPtr account, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent); - explicit JsonApiJob(AccountPtr account, const QString &path, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent); + explicit JsonApiJob(Account *account, const QString &path, const QByteArray &verb, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent); + explicit JsonApiJob(Account *account, const QString &path, const UrlQuery &arguments, const QNetworkRequest &req, QObject *parent); // the OCS status code: 100 (!) for success int ocsStatus() const; diff --git a/src/libsync/networkjobs/resources.cpp b/src/libsync/networkjobs/resources.cpp index e8598514d44..ea7e3868ecc 100644 --- a/src/libsync/networkjobs/resources.cpp +++ b/src/libsync/networkjobs/resources.cpp @@ -82,7 +82,7 @@ QIcon ResourceJob::asIcon() const } ResourceJob::ResourceJob(ResourcesCache *cache, const QUrl &rootUrl, const QString &path, QObject *parent) - : SimpleNetworkJob(cache->account()->sharedFromThis(), rootUrl, path, "GET", {}, {}, parent) + : SimpleNetworkJob(cache->account(), rootUrl, path, "GET", {}, {}, parent) , _cache(cache) { setStoreInCache(true); diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index e6e59acd67c..f76649481f8 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -152,9 +152,8 @@ QString OWNCLOUDSYNC_EXPORT createDownloadTmpFileName(const QString &previous) } // DOES NOT take ownership of the device. -GETFileJob::GETFileJob(AccountPtr account, const QUrl &url, const QString &path, QIODevice *device, - const QMap &headers, const QString &expectedEtagForResume, - qint64 resumeStart, QObject *parent) +GETFileJob::GETFileJob(Account *account, const QUrl &url, const QString &path, QIODevice *device, const QMap &headers, + const QString &expectedEtagForResume, qint64 resumeStart, QObject *parent) : AbstractNetworkJob(account, url, path, parent) , _device(device) , _headers(headers) @@ -571,9 +570,8 @@ void PropagateDownloadFile::startFullDownload() if (_item->_directDownloadUrl.isEmpty()) { // Normal job, download from oC instance - _job = new GETFileJob(propagator()->account(), propagator()->webDavUrl(), - propagator()->fullRemotePath(_item->_file), - &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); + _job = new GETFileJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), &_tmpFile, headers, + _expectedEtagForResume, _resumeStart, this); } else { // We were provided a direct URL, use that one qCInfo(lcPropagateDownload) << "directDownloadUrl given for " << _item->_file << _item->_directDownloadUrl; @@ -583,10 +581,7 @@ void PropagateDownloadFile::startFullDownload() } QUrl url = QUrl::fromUserInput(_item->_directDownloadUrl); - _job = new GETFileJob(propagator()->account(), - url, - {}, - &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); + _job = new GETFileJob(propagator()->account().get(), url, {}, &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); } _job->setExpectedContentLength(_item->_size - _resumeStart); diff --git a/src/libsync/propagatedownload.h b/src/libsync/propagatedownload.h index 219dae88701..6e31a6c435a 100644 --- a/src/libsync/propagatedownload.h +++ b/src/libsync/propagatedownload.h @@ -38,7 +38,7 @@ class OWNCLOUDSYNC_EXPORT GETFileJob : public AbstractNetworkJob public: // DOES NOT take ownership of the device. // For directDownloadUrl: - explicit GETFileJob(AccountPtr account, const QUrl &url, const QString &path, QIODevice *device, const QMap &headers, + explicit GETFileJob(Account *account, const QUrl &url, const QString &path, QIODevice *device, const QMap &headers, const QString &expectedEtagForResume, qint64 resumeStart, QObject *parent = nullptr); qint64 currentDownloadPosition(); diff --git a/src/libsync/propagateremotedelete.cpp b/src/libsync/propagateremotedelete.cpp index 11eff0c3b7a..bbba691e97b 100644 --- a/src/libsync/propagateremotedelete.cpp +++ b/src/libsync/propagateremotedelete.cpp @@ -24,7 +24,7 @@ namespace OCC { Q_LOGGING_CATEGORY(lcDeleteJob, "sync.networkjob.delete", QtInfoMsg) Q_LOGGING_CATEGORY(lcPropagateRemoteDelete, "sync.propagator.remotedelete", QtInfoMsg) -DeleteJob::DeleteJob(AccountPtr account, const QUrl &url, const QString &path, QObject *parent) +DeleteJob::DeleteJob(Account *account, const QUrl &url, const QString &path, QObject *parent) : AbstractNetworkJob(account, url, path, parent) { } @@ -48,9 +48,7 @@ void PropagateRemoteDelete::start() qCDebug(lcPropagateRemoteDelete) << _item->_file; - _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), - propagator()->fullRemotePath(_item->_file), - this); + _job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); connect(_job.data(), &DeleteJob::finishedSignal, this, &PropagateRemoteDelete::slotDeleteJobFinished); propagator()->_activeJobList.append(this); _job->start(); diff --git a/src/libsync/propagateremotedelete.h b/src/libsync/propagateremotedelete.h index 76a812f0d84..f33656a0e1c 100644 --- a/src/libsync/propagateremotedelete.h +++ b/src/libsync/propagateremotedelete.h @@ -26,7 +26,7 @@ class DeleteJob : public AbstractNetworkJob { Q_OBJECT public: - explicit DeleteJob(AccountPtr account, const QUrl &url, const QString &path, QObject *parent = nullptr); + explicit DeleteJob(Account *account, const QUrl &url, const QString &path, QObject *parent = nullptr); void start() override; void finished() override; diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index 8aa8c1c982c..0efa9706911 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -45,9 +45,7 @@ void PropagateRemoteMkdir::start() return slotStartMkcolJob(); } - _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), - propagator()->fullRemotePath(_item->_file), - this); + _job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); connect(qobject_cast(_job), &DeleteJob::finishedSignal, this, &PropagateRemoteMkdir::slotStartMkcolJob); _job->start(); } @@ -59,9 +57,7 @@ void PropagateRemoteMkdir::slotStartMkcolJob() qCDebug(lcPropagateRemoteMkdir) << _item->_file; - _job = new MkColJob(propagator()->account(), propagator()->webDavUrl(), - propagator()->fullRemotePath(_item->_file), {}, - this); + _job = new MkColJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), {}, this); connect(qobject_cast(_job), &MkColJob::finishedWithError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); connect(qobject_cast(_job), &MkColJob::finishedWithoutError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); _job->start(); @@ -115,7 +111,7 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() propagator()->_activeJobList.append(this); auto propfindJob = - new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + new PropfindJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); propfindJob->setProperties({"http://owncloud.org/ns:permissions"}); connect(propfindJob, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &result) { propagator()->_activeJobList.removeOne(this); diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index f3c8bd3d902..84bec1235b2 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -28,8 +28,7 @@ namespace OCC { Q_LOGGING_CATEGORY(lcMoveJob, "sync.networkjob.move", QtInfoMsg) Q_LOGGING_CATEGORY(lcPropagateRemoteMove, "sync.propagator.remotemove", QtInfoMsg) -MoveJob::MoveJob(AccountPtr account, const QUrl &url, const QString &path, const QString &destination, - const HeaderMap &extraHeaders, QObject *parent) +MoveJob::MoveJob(Account *account, const QUrl &url, const QString &path, const QString &destination, const HeaderMap &extraHeaders, QObject *parent) : AbstractNetworkJob(account, url, path, parent) , _destination(destination) , _extraHeaders(extraHeaders) @@ -72,7 +71,7 @@ void PropagateRemoteMove::start() auto itemType = _item->_type; OC_ASSERT(itemType != ItemTypeVirtualFileDownload && itemType != ItemTypeVirtualFileDehydration); - _job = new MoveJob(propagator()->account(), propagator()->webDavUrl(), remoteSource, remoteDestination, {}, this); + _job = new MoveJob(propagator()->account().get(), propagator()->webDavUrl(), remoteSource, remoteDestination, {}, this); connect(_job.data(), &MoveJob::finishedSignal, this, &PropagateRemoteMove::slotMoveJobFinished); propagator()->_activeJobList.append(this); _job->start(); diff --git a/src/libsync/propagateremotemove.h b/src/libsync/propagateremotemove.h index 05db27bbbc3..081493cf20c 100644 --- a/src/libsync/propagateremotemove.h +++ b/src/libsync/propagateremotemove.h @@ -29,8 +29,8 @@ class MoveJob : public AbstractNetworkJob HeaderMap _extraHeaders; public: - explicit MoveJob(AccountPtr account, const QUrl &url, const QString &path, const QString &destination, - const HeaderMap &_extraHeaders, QObject *parent = nullptr); + explicit MoveJob( + Account *account, const QUrl &url, const QString &path, const QString &destination, const HeaderMap &_extraHeaders, QObject *parent = nullptr); void start() override; void finished() override; diff --git a/src/libsync/propagateuploadcommon.cpp b/src/libsync/propagateuploadcommon.cpp index 9669532ba2e..e98b852ea3d 100644 --- a/src/libsync/propagateuploadcommon.cpp +++ b/src/libsync/propagateuploadcommon.cpp @@ -99,9 +99,7 @@ void PropagateUploadCommon::start() return slotComputeContentChecksum(); } - auto job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), - propagator()->fullRemotePath(_item->_file), - this); + auto job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); addChildJob(job); connect(job, &DeleteJob::finishedSignal, this, &PropagateUploadCommon::slotComputeContentChecksum); job->start(); @@ -374,8 +372,8 @@ void PropagateUploadCommon::finalize() if (_item->_remotePerm.isNull()) { qCWarning(lcPropagateUpload) << "PropagateUploadCommon::finalize: Missing permissions for" << propagator()->fullRemotePath(_item->_file); - auto *permCheck = - new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + auto *permCheck = new PropfindJob( + propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); addChildJob(permCheck); permCheck->setProperties({ "http://owncloud.org/ns:permissions" }); connect(permCheck, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &map) { diff --git a/src/libsync/propagateuploadfile.cpp b/src/libsync/propagateuploadfile.cpp index 81c8e84286b..9bb29bf56d0 100644 --- a/src/libsync/propagateuploadfile.cpp +++ b/src/libsync/propagateuploadfile.cpp @@ -17,7 +17,6 @@ #include "common/checksums.h" #include "common/syncjournaldb.h" #include "filesystem.h" -#include "networkjobs.h" #include "owncloudpropagator_p.h" #include "propagatorjobs.h" #include "putfilejob.h" @@ -28,8 +27,6 @@ #include #include -#include -#include #include namespace OCC { @@ -88,7 +85,8 @@ void PropagateUploadFile::startUpload() // TODO: review usage of scoped pointer vs qt memory management // job takes ownership of device via a QScopedPointer. Job deletes itself when finishing - PUTFileJob *job = new PUTFileJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(path), std::move(device), headers, this); + PUTFileJob *job = + new PUTFileJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(path), std::move(device), headers, this); addChildJob(job); connect(job, &PUTFileJob::finishedSignal, this, &PropagateUploadFile::slotPutFinished); connect(job, &PUTFileJob::uploadProgress, this, &PropagateUploadFile::slotUploadProgress); diff --git a/src/libsync/propagateuploadtus.cpp b/src/libsync/propagateuploadtus.cpp index 52d047c0ec9..303131c0213 100644 --- a/src/libsync/propagateuploadtus.cpp +++ b/src/libsync/propagateuploadtus.cpp @@ -84,7 +84,7 @@ SimpleNetworkJob *PropagateUploadFileTUS::makeCreationWithUploadJob(QNetworkRequ request->setRawHeader(QByteArrayLiteral("Upload-Metadata"), encodedMetaData.join(',')); request->setRawHeader(QByteArrayLiteral("Upload-Length"), QByteArray::number(_item->_size)); - return new SimpleNetworkJob(propagator()->account(), propagator()->webDavUrl(), {}, "POST", device, *request, this); + return new SimpleNetworkJob(propagator()->account().get(), propagator()->webDavUrl(), {}, "POST", device, *request, this); } QNetworkRequest PropagateUploadFileTUS::prepareRequest(const quint64 &chunkSize) @@ -120,7 +120,7 @@ void PropagateUploadFileTUS::doStartUpload() if (info.validate(_item->_size, _item->_modtime, _item->_checksumHeader)) { _location = info._url; Q_ASSERT(_location.isValid()); - auto job = new SimpleNetworkJob(propagator()->account(), _location, {}, "HEAD", nullptr, {}, this); + auto job = new SimpleNetworkJob(propagator()->account().get(), _location, {}, "HEAD", nullptr, {}, this); connect(job, &SimpleNetworkJob::finishedSignal, this, &PropagateUploadFileTUS::slotChunkFinished); job->start(); } else { @@ -150,7 +150,7 @@ void PropagateUploadFileTUS::startNextChunk() SimpleNetworkJob *job; if (_currentOffset != 0) { qCDebug(lcPropagateUploadTUS) << "Starting to patch upload:" << propagator()->fullRemotePath(_item->_file); - job = new SimpleNetworkJob(propagator()->account(), _location, {}, "PATCH", device, req, this); + job = new SimpleNetworkJob(propagator()->account().get(), _location, {}, "PATCH", device, req, this); } else { Q_ASSERT(_location.isEmpty()); qCDebug(lcPropagateUploadTUS) << "Starting creation with upload:" << propagator()->fullRemotePath(_item->_file); @@ -189,7 +189,7 @@ void PropagateUploadFileTUS::slotChunkFinished() qCWarning(lcPropagateUploadTUS) << propagator()->fullRemotePath(_item->_file) << "Encountered a timeout -> get progress for" << _location; QNetworkRequest req; setTusVersionHeader(req); - auto updateJob = new SimpleNetworkJob(propagator()->account(), propagator()->webDavUrl(), _location.path(), "HEAD", {}, req, this); + auto updateJob = new SimpleNetworkJob(propagator()->account().get(), propagator()->webDavUrl(), _location.path(), "HEAD", {}, req, this); addChildJob(updateJob); connect(updateJob, &SimpleNetworkJob::finishedSignal, this, &PropagateUploadFileTUS::slotChunkFinished); updateJob->start(); @@ -255,7 +255,8 @@ void PropagateUploadFileTUS::slotChunkFinished() if (!_finished) { // Either the ETag or the remote Permissions were not in the headers of the reply. // Start a PROPFIND to fetch these data from the server. - auto check = new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + auto check = new PropfindJob( + propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); addChildJob(check); check->setProperties({ "http://owncloud.org/ns:fileid", "http://owncloud.org/ns:permissions", "getetag" }); connect(check, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &map) { diff --git a/src/libsync/putfilejob.cpp b/src/libsync/putfilejob.cpp index a6b275fa244..b23e1260172 100644 --- a/src/libsync/putfilejob.cpp +++ b/src/libsync/putfilejob.cpp @@ -34,7 +34,8 @@ namespace OCC { Q_LOGGING_CATEGORY(lcPutJob, "sync.networkjob.put", QtInfoMsg) -PUTFileJob::PUTFileJob(AccountPtr account, const QUrl &url, const QString &path, std::unique_ptr &&device, const QMap &headers, QObject *parent) +PUTFileJob::PUTFileJob( + Account *account, const QUrl &url, const QString &path, std::unique_ptr &&device, const QMap &headers, QObject *parent) : AbstractNetworkJob(account, url, path, parent) , _device(device.release()) , _headers(headers) diff --git a/src/libsync/putfilejob.h b/src/libsync/putfilejob.h index 281bfdaeb40..3c06c03a28e 100644 --- a/src/libsync/putfilejob.h +++ b/src/libsync/putfilejob.h @@ -36,8 +36,8 @@ class PUTFileJob : public AbstractNetworkJob QElapsedTimer _requestTimer; public: - explicit PUTFileJob(AccountPtr account, const QUrl &url, const QString &path, std::unique_ptr &&device, - const HeaderMap &headers, QObject *parent = nullptr); + explicit PUTFileJob( + Account *account, const QUrl &url, const QString &path, std::unique_ptr &&device, const HeaderMap &headers, QObject *parent = nullptr); ~PUTFileJob() override; void start() override; diff --git a/test/testconnectionvalidator/testconnectionvalidator.cpp b/test/testconnectionvalidator/testconnectionvalidator.cpp index 2d82e8916c1..284229f5dee 100644 --- a/test/testconnectionvalidator/testconnectionvalidator.cpp +++ b/test/testconnectionvalidator/testconnectionvalidator.cpp @@ -147,7 +147,7 @@ private Q_SLOTS: return nullptr; }); - ConnectionValidator val(fakeFolder.account().get(), nullptr); + ConnectionValidator val(fakeFolder.account(), nullptr); val.checkServer(ConnectionValidator::ValidationMode::ValidateAuthAndUpdate); QSignalSpy spy(&val, &ConnectionValidator::connectionResult); diff --git a/test/testjobqueue.cpp b/test/testjobqueue.cpp index 4ca5262a584..a73e7782214 100644 --- a/test/testjobqueue.cpp +++ b/test/testjobqueue.cpp @@ -20,7 +20,7 @@ class TestJob : public AbstractNetworkJob // AbstractNetworkJob interface public: // TODO: davurl - TestJob(AccountPtr account) + TestJob(Account *account) : AbstractNetworkJob(account, account->davUrl(), QStringLiteral("/A/a1")) { } diff --git a/test/testutils/syncenginetestutils.cpp b/test/testutils/syncenginetestutils.cpp index e9a7830cc16..e3ff394e42d 100644 --- a/test/testutils/syncenginetestutils.cpp +++ b/test/testutils/syncenginetestutils.cpp @@ -887,7 +887,7 @@ FakeFolder::FakeFolder(const FileInfo &fileTemplate, OCC::Vfs::Mode vfsMode, boo _journalDb.reset(new OCC::SyncJournalDb(localPath() + QStringLiteral(".sync_test.db"))); // TODO: davUrl - _syncEngine.reset(new OCC::SyncEngine(account().get(), account()->davUrl(), localPath(), QString(), _journalDb.get())); + _syncEngine.reset(new OCC::SyncEngine(account(), account()->davUrl(), localPath(), QString(), _journalDb.get())); _syncEngine->setSyncOptions(OCC::SyncOptions { QSharedPointer(OCC::VfsPluginManager::instance().createVfsFromPlugin(vfsMode).release()) }); // Ignore temporary files from the download. (This is in the default exclude list, but we don't load it) diff --git a/test/testutils/syncenginetestutils.h b/test/testutils/syncenginetestutils.h index 8a254a66cf1..a45fe6614ca 100644 --- a/test/testutils/syncenginetestutils.h +++ b/test/testutils/syncenginetestutils.h @@ -575,7 +575,7 @@ class FakeFolder : public QObject void switchToVfs(QSharedPointer vfs); - OCC::AccountPtr account() const { return _accountState->account(); } + OCC::Account *account() const { return _accountState->account().get(); } OCC::SyncEngine &syncEngine() const { return *_syncEngine; } OCC::SyncJournalDb &syncJournal() const { return *_journalDb; } From b46da56eb9dcb114722fc42cab96713c8a43d490 Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 15:43:35 +0200 Subject: [PATCH 18/23] made account member in AbstractNetworkJob private to ensure it's not used directly by subclasses updated fetchServerSettings and discovery classes --- src/gui/accountstate.cpp | 2 +- src/gui/connectionvalidator.cpp | 2 +- src/gui/fetchserversettings.cpp | 16 ++++++++++++---- src/gui/fetchserversettings.h | 8 ++++---- src/libsync/abstractnetworkjob.h | 3 ++- src/libsync/discovery.cpp | 20 ++++++++++++++++++-- src/libsync/discovery.h | 1 + src/libsync/discoveryphase.cpp | 15 ++++++++++----- src/libsync/discoveryphase.h | 29 ++++++++++++++--------------- src/libsync/syncengine.cpp | 12 ++++-------- 10 files changed, 67 insertions(+), 41 deletions(-) diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index 601acaf4440..770cf0bd0ed 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -200,7 +200,7 @@ void AccountState::setState(State state) // ensure the connection validator is done _queueGuard.unblock(); // update capabilities and fetch relevant settings - _fetchCapabilitiesJob = new FetchServerSettingsJob(account(), this); + _fetchCapabilitiesJob = new FetchServerSettingsJob(account().get(), this); connect(_fetchCapabilitiesJob.get(), &FetchServerSettingsJob::finishedSignal, this, [oldState, this] { // Lisa todo: I do not understand this logic at all - review it if (oldState == Connected || _state == Connected) { diff --git a/src/gui/connectionvalidator.cpp b/src/gui/connectionvalidator.cpp index 9083ff82c68..2b1bb3842c4 100644 --- a/src/gui/connectionvalidator.cpp +++ b/src/gui/connectionvalidator.cpp @@ -285,7 +285,7 @@ void ConnectionValidator::slotAuthSuccess() _errors.clear(); if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) { - auto *fetchSetting = new FetchServerSettingsJob(_account->sharedFromThis(), this); + auto *fetchSetting = new FetchServerSettingsJob(_account, this); const auto unsupportedServerError = [this] { _errors.append({tr("The configured server for this client is too old."), tr("Please update to the latest server and restart the client.")}); }; diff --git a/src/gui/fetchserversettings.cpp b/src/gui/fetchserversettings.cpp index 8ac2793ce28..6cb149903ee 100644 --- a/src/gui/fetchserversettings.cpp +++ b/src/gui/fetchserversettings.cpp @@ -34,7 +34,7 @@ auto fetchSettingsTimeout() } // TODO: move to libsync? -FetchServerSettingsJob::FetchServerSettingsJob(const OCC::AccountPtr &account, QObject *parent) +FetchServerSettingsJob::FetchServerSettingsJob(OCC::Account *account, QObject *parent) : QObject(parent) , _account(account) { @@ -43,8 +43,13 @@ FetchServerSettingsJob::FetchServerSettingsJob(const OCC::AccountPtr &account, Q void FetchServerSettingsJob::start() { + if (!_account) { + Q_EMIT finishedSignal(Result::Undefined); + return; + } + // The main flow now needs the capabilities - auto *job = new JsonApiJob(_account.get(), QStringLiteral("ocs/v2.php/cloud/capabilities"), {}, {}, this); + auto *job = new JsonApiJob(_account, QStringLiteral("ocs/v2.php/cloud/capabilities"), {}, {}, this); job->setAuthenticationJob(isAuthJob()); job->setTimeout(fetchSettingsTimeout()); @@ -114,6 +119,9 @@ void FetchServerSettingsJob::start() void FetchServerSettingsJob::runAsyncUpdates() { + if (!_account) + return; + // those jobs are: // - never auth jobs // - might get queued @@ -124,13 +132,13 @@ void FetchServerSettingsJob::runAsyncUpdates() // so we just set them free if (_account->capabilities().avatarsAvailable()) { // the avatar job uses the legacy WebDAV URL and ocis will require a new approach - auto *avatarJob = new AvatarJob(_account.get(), _account->davUser(), 128, nullptr); + auto *avatarJob = new AvatarJob(_account, _account->davUser(), 128, nullptr); connect(avatarJob, &AvatarJob::avatarPixmap, this, [this](const QPixmap &img) { _account->setAvatar(AvatarJob::makeCircularAvatar(img)); }); avatarJob->start(); }; if (_account->capabilities().appProviders().enabled) { - auto *jsonJob = new JsonJob(_account.get(), _account->capabilities().appProviders().appsUrl, {}, "GET"); + auto *jsonJob = new JsonJob(_account, _account->capabilities().appProviders().appsUrl, {}, "GET"); connect(jsonJob, &JsonJob::finishedSignal, this, [jsonJob, this] { _account->setAppProvider(AppProvider{jsonJob->data()}); }); jsonJob->start(); } diff --git a/src/gui/fetchserversettings.h b/src/gui/fetchserversettings.h index 672dbc30f5f..9ea77521d26 100644 --- a/src/gui/fetchserversettings.h +++ b/src/gui/fetchserversettings.h @@ -14,9 +14,9 @@ #pragma once -#include "libsync/accountfwd.h" - +#include "account.h" #include +#include namespace OCC { class Capabilities; @@ -27,7 +27,7 @@ class FetchServerSettingsJob : public QObject public: enum class Result { Success, TimeOut, InvalidCredentials, UnsupportedServer, Undefined }; Q_ENUM(Result); - FetchServerSettingsJob(const AccountPtr &account, QObject *parent); + FetchServerSettingsJob(Account *account, QObject *parent); void start(); @@ -40,7 +40,7 @@ class FetchServerSettingsJob : public QObject // returns whether the started jobs should be excluded from the retry queue bool isAuthJob() const; - const AccountPtr _account; + QPointer _account; }; } diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index aa6773b69ef..c54854bc811 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -197,11 +197,12 @@ class OWNCLOUDSYNC_EXPORT AbstractNetworkJob : public QObject */ void setQuery(const QUrlQuery &query); - QPointer _account; QUrlQuery _query; QByteArray _responseTimestamp; private: + QPointer _account; + /** Makes this job drive a pre-made QNetworkReply * * This reply cannot have a QIODevice request body because we can't get diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 2eb5c9f9b32..40c2aea3a19 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -39,6 +39,8 @@ void ProcessDirectoryJob::start() if (_queryServer == NormalQuery) { _serverJob = startAsyncServerQuery(); + if (!_serverJob) + return; } else { _serverQueryDone = true; } @@ -548,8 +550,12 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( done = true; } else { // we need to make a request to the server to know that the original file is deleted on the server + if (_discoveryData->_account == nullptr) { + Q_EMIT _discoveryData->fatalError(tr("account was deleted. Unable to continue")); + return; + } _pendingAsyncJobs++; - auto job = new RequestEtagJob(_discoveryData->_account.get(), _discoveryData->_baseUrl, _discoveryData->_remoteFolder + originalPath, this); + auto job = new RequestEtagJob(_discoveryData->_account, _discoveryData->_baseUrl, _discoveryData->_remoteFolder + originalPath, this); connect(job, &RequestEtagJob::finishedSignal, this, [=]() mutable { _pendingAsyncJobs--; QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs); @@ -910,6 +916,11 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( if (wasDeletedOnClient.first) { finalize(processRename(path), wasDeletedOnClient.second.toUtf8() == base._etag ? ParentNotChanged : NormalQuery); } else { + if (_discoveryData->_account == nullptr) { + Q_EMIT _discoveryData->fatalError(tr("account was deleted. Unable to continue")); + return; + } + // We must query the server to know if the etag has not changed _pendingAsyncJobs++; QString serverOriginalPath = _discoveryData->_remoteFolder + _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down); @@ -1267,8 +1278,13 @@ void ProcessDirectoryJob::dbError() DiscoverySingleDirectoryJob *ProcessDirectoryJob::startAsyncServerQuery() { + if (_discoveryData->_account == nullptr) { + Q_EMIT _discoveryData->fatalError(tr("account was deleted. Unable to continue")); + return nullptr; + } + auto discoveryJob = - new DiscoverySingleDirectoryJob(_discoveryData->_account, _discoveryData->_baseUrl, _discoveryData->_remoteFolder + _currentFolder._server, this); + new DiscoverySingleDirectoryJob(_discoveryData->_account.get(), _discoveryData->_baseUrl, _discoveryData->_remoteFolder + _currentFolder._server, this); if (!_dirItem) discoveryJob->setIsRootPath(); // query the fingerprint on the root connect(discoveryJob, &DiscoverySingleDirectoryJob::etag, this, &ProcessDirectoryJob::etag); diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index a45b59e8537..1d173811b79 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -50,6 +50,7 @@ class ProcessDirectoryJob : public QObject Q_OBJECT struct PathTuple; + public: enum QueryMode { NormalQuery, diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index e3f2e67f66f..eca92a9db0d 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -15,7 +15,6 @@ #include "discoveryphase.h" #include "discovery.h" -#include "account.h" #include "common/asserts.h" #include "common/checksums.h" @@ -25,8 +24,6 @@ #include #include #include -#include - namespace OCC { @@ -185,6 +182,8 @@ void DiscoveryPhase::scheduleMoreJobs() bool DiscoveryPhase::isSpace() const { + if (!_account) + return false; return !(Utility::urlEqual(_account->davUrl(), _baseUrl) || _account->davUrl().isParentOf(_baseUrl)); } @@ -260,7 +259,7 @@ void DiscoverySingleLocalDirectoryJob::run() { Q_EMIT finished(results); } -DiscoverySingleDirectoryJob::DiscoverySingleDirectoryJob(const AccountPtr &account, const QUrl &baseUrl, const QString &path, QObject *parent) +DiscoverySingleDirectoryJob::DiscoverySingleDirectoryJob(Account *account, const QUrl &baseUrl, const QString &path, QObject *parent) : QObject(parent) , _subPath(path) , _account(account) @@ -273,8 +272,14 @@ DiscoverySingleDirectoryJob::DiscoverySingleDirectoryJob(const AccountPtr &accou void DiscoverySingleDirectoryJob::start() { + if (!_account) { + Q_EMIT finished(HttpError{0, tr("The account was deleted before we could start the propfind job")}); + deleteLater(); + return; + } + // Start the actual HTTP job - _proFindJob = new PropfindJob(_account.get(), _baseUrl, _subPath, PropfindJob::Depth::One, this); + _proFindJob = new PropfindJob(_account, _baseUrl, _subPath, PropfindJob::Depth::One, this); QList props { "resourcetype", diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 1334cac2017..fad08cee980 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -15,17 +15,14 @@ #pragma once #include -#include -#include -#include -#include -#include + +#include "account.h" #include "networkjobs.h" -#include -#include -#include -#include "syncoptions.h" #include "syncfileitem.h" +#include "syncoptions.h" + +#include +#include class ExcludedFiles; @@ -37,7 +34,6 @@ enum class LocalDiscoveryStyle { }; -class Account; class SyncJournalDb; class ProcessDirectoryJob; @@ -112,7 +108,7 @@ class DiscoverySingleDirectoryJob : public QObject { Q_OBJECT public: - explicit DiscoverySingleDirectoryJob(const AccountPtr &account, const QUrl &baseUrl, const QString &path, QObject *parent = nullptr); + explicit DiscoverySingleDirectoryJob(Account *account, const QUrl &baseUrl, const QString &path, QObject *parent = nullptr); // Specify that this is the root and we need to check the data-fingerprint void setIsRootPath() { _isRootPath = true; } bool isRootPath() const { return _isRootPath; } @@ -133,7 +129,7 @@ private Q_SLOTS: QVector _results; QString _subPath; QString _firstEtag; - AccountPtr _account; + QPointer _account; const QUrl _baseUrl; // The first result is for the directory itself and need to be ignored. // This flag is true if it was already ignored. @@ -232,14 +228,14 @@ class DiscoveryPhase : public QObject public: // input - DiscoveryPhase(const AccountPtr &account, const SyncOptions &options, const QUrl &baseUrl, QObject *parent = nullptr) + DiscoveryPhase(Account *account, const SyncOptions &options, const QUrl &baseUrl, QObject *parent = nullptr) : QObject(parent) - , _account(account) , _syncOptions(options) , _baseUrl(baseUrl) + , _account(account) { } - AccountPtr _account; + const SyncOptions _syncOptions; const QUrl _baseUrl; QString _localDir; // absolute path to the local directory. ends with '/' @@ -276,6 +272,9 @@ class DiscoveryPhase : public QObject */ void silentlyExcluded(const QString &folderPath); void excluded(const QString &folderPath); + +private: + QPointer _account; }; /// Implementation of DiscoveryPhase::adjustRenamedPath diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 6a476e4b5ab..34735fefd8d 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -24,7 +24,6 @@ #include "discovery.h" #include "discoveryphase.h" #include "owncloudpropagator.h" -#include "propagateremotedelete.h" #include @@ -299,8 +298,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) void SyncEngine::startSync() { - if (!_account) + if (!_account) { + finalize(false); return; + } if (_syncRunning) { OC_ASSERT(false); @@ -391,7 +392,7 @@ void SyncEngine::startSync() // TODO: add a constructor to DiscoveryPhase // pass a syncEngine object rather than copying everything to another object - _discoveryPhase.reset(new DiscoveryPhase(_account->sharedFromThis(), syncOptions(), _baseUrl)); + _discoveryPhase.reset(new DiscoveryPhase(_account, syncOptions(), _baseUrl)); _discoveryPhase->_excludes = _excludedFiles.get(); _discoveryPhase->_statedb = _journal; _discoveryPhase->_localDir = _localPath; @@ -698,11 +699,6 @@ void SyncEngine::restoreOldFiles(SyncFileItemSet &syncItems) } } -/*AccountPtr SyncEngine::account() const -{ - return _account; -}*/ - void SyncEngine::setLocalDiscoveryOptions(LocalDiscoveryStyle style, std::set paths) { _localDiscoveryStyle = style; From fc1e8279514a64b2f494b740f3c9c41a6e30f8f1 Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 17:04:39 +0200 Subject: [PATCH 19/23] updated propagator classes --- src/libsync/owncloudpropagator.cpp | 7 +------ src/libsync/owncloudpropagator.h | 15 ++++++++------- src/libsync/propagatedownload.cpp | 4 ++-- src/libsync/propagateremotedelete.cpp | 2 +- src/libsync/propagateremotemkdir.cpp | 6 +++--- src/libsync/propagateremotemove.cpp | 2 +- src/libsync/propagateuploadcommon.cpp | 6 +++--- src/libsync/propagateuploadfile.cpp | 3 +-- src/libsync/propagateuploadtus.cpp | 12 ++++++------ src/libsync/syncengine.cpp | 2 +- 10 files changed, 27 insertions(+), 32 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index f8de0cad251..5a9eb60b900 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -75,11 +75,6 @@ qint64 freeSpaceLimit() return value; } -OwncloudPropagator::~OwncloudPropagator() -{ -} - - int OwncloudPropagator::maximumActiveTransferJob() { if (!_syncOptions._parallelNetworkJobs) { @@ -715,7 +710,7 @@ void OwncloudPropagator::reportProgress(const SyncFileItem &item, qint64 bytes) Q_EMIT progress(item, bytes); } -AccountPtr OwncloudPropagator::account() const +Account *OwncloudPropagator::account() const { return _account; } diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index c2e58339c07..73c20ebd14d 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -15,12 +15,14 @@ #ifndef OWNCLOUDPROPAGATOR_H #define OWNCLOUDPROPAGATOR_H +#include +#include + #include #include -#include #include -#include "accountfwd.h" +#include "account.h" #include "common/syncjournaldb.h" #include "csync.h" #include "syncfileitem.h" @@ -136,6 +138,7 @@ public Q_SLOTS: * Emitted when the abort is fully finished */ void abortFinished(SyncFileItem::Status status = SyncFileItem::NormalError); + protected: OwncloudPropagator *propagator() const; @@ -382,7 +385,7 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject public: OwncloudPropagator( - AccountPtr account, const SyncOptions &options, const QUrl &baseUrl, const QString &localDir, const QString &remoteFolder, SyncJournalDb *progressDb) + Account *account, const SyncOptions &options, const QUrl &baseUrl, const QString &localDir, const QString &remoteFolder, SyncJournalDb *progressDb) : _journal(progressDb) , _finishedEmitted(false) , _anotherSyncNeeded(false) @@ -395,8 +398,6 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject qRegisterMetaType("PropagatorJob::AbortType"); } - ~OwncloudPropagator() override; - void start(SyncFileItemSet &&_syncedItems); const SyncOptions &syncOptions() const; @@ -472,7 +473,7 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject void abort(); - AccountPtr account() const; + Account *account() const; QUrl webDavUrl() const { @@ -556,7 +557,7 @@ private Q_SLOTS: void insufficientRemoteStorage(); private: - AccountPtr _account; + QPointer _account; QScopedPointer _rootJob; SyncOptions _syncOptions; bool _jobScheduled = false; diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index f76649481f8..ca1a7684c3a 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -570,7 +570,7 @@ void PropagateDownloadFile::startFullDownload() if (_item->_directDownloadUrl.isEmpty()) { // Normal job, download from oC instance - _job = new GETFileJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), &_tmpFile, headers, + _job = new GETFileJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); } else { // We were provided a direct URL, use that one @@ -581,7 +581,7 @@ void PropagateDownloadFile::startFullDownload() } QUrl url = QUrl::fromUserInput(_item->_directDownloadUrl); - _job = new GETFileJob(propagator()->account().get(), url, {}, &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); + _job = new GETFileJob(propagator()->account(), url, {}, &_tmpFile, headers, _expectedEtagForResume, _resumeStart, this); } _job->setExpectedContentLength(_item->_size - _resumeStart); diff --git a/src/libsync/propagateremotedelete.cpp b/src/libsync/propagateremotedelete.cpp index bbba691e97b..6e68f20d686 100644 --- a/src/libsync/propagateremotedelete.cpp +++ b/src/libsync/propagateremotedelete.cpp @@ -48,7 +48,7 @@ void PropagateRemoteDelete::start() qCDebug(lcPropagateRemoteDelete) << _item->_file; - _job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); + _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); connect(_job.data(), &DeleteJob::finishedSignal, this, &PropagateRemoteDelete::slotDeleteJobFinished); propagator()->_activeJobList.append(this); _job->start(); diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index 0efa9706911..1cc2fcba843 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -45,7 +45,7 @@ void PropagateRemoteMkdir::start() return slotStartMkcolJob(); } - _job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); + _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); connect(qobject_cast(_job), &DeleteJob::finishedSignal, this, &PropagateRemoteMkdir::slotStartMkcolJob); _job->start(); } @@ -57,7 +57,7 @@ void PropagateRemoteMkdir::slotStartMkcolJob() qCDebug(lcPropagateRemoteMkdir) << _item->_file; - _job = new MkColJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), {}, this); + _job = new MkColJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), {}, this); connect(qobject_cast(_job), &MkColJob::finishedWithError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); connect(qobject_cast(_job), &MkColJob::finishedWithoutError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); _job->start(); @@ -111,7 +111,7 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() propagator()->_activeJobList.append(this); auto propfindJob = - new PropfindJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); propfindJob->setProperties({"http://owncloud.org/ns:permissions"}); connect(propfindJob, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &result) { propagator()->_activeJobList.removeOne(this); diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index 84bec1235b2..3695fbc10ec 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -71,7 +71,7 @@ void PropagateRemoteMove::start() auto itemType = _item->_type; OC_ASSERT(itemType != ItemTypeVirtualFileDownload && itemType != ItemTypeVirtualFileDehydration); - _job = new MoveJob(propagator()->account().get(), propagator()->webDavUrl(), remoteSource, remoteDestination, {}, this); + _job = new MoveJob(propagator()->account(), propagator()->webDavUrl(), remoteSource, remoteDestination, {}, this); connect(_job.data(), &MoveJob::finishedSignal, this, &PropagateRemoteMove::slotMoveJobFinished); propagator()->_activeJobList.append(this); _job->start(); diff --git a/src/libsync/propagateuploadcommon.cpp b/src/libsync/propagateuploadcommon.cpp index e98b852ea3d..2c8c19d3a3c 100644 --- a/src/libsync/propagateuploadcommon.cpp +++ b/src/libsync/propagateuploadcommon.cpp @@ -99,7 +99,7 @@ void PropagateUploadCommon::start() return slotComputeContentChecksum(); } - auto job = new DeleteJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); + auto job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), this); addChildJob(job); connect(job, &DeleteJob::finishedSignal, this, &PropagateUploadCommon::slotComputeContentChecksum); job->start(); @@ -372,8 +372,8 @@ void PropagateUploadCommon::finalize() if (_item->_remotePerm.isNull()) { qCWarning(lcPropagateUpload) << "PropagateUploadCommon::finalize: Missing permissions for" << propagator()->fullRemotePath(_item->_file); - auto *permCheck = new PropfindJob( - propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + auto *permCheck = + new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); addChildJob(permCheck); permCheck->setProperties({ "http://owncloud.org/ns:permissions" }); connect(permCheck, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &map) { diff --git a/src/libsync/propagateuploadfile.cpp b/src/libsync/propagateuploadfile.cpp index 9bb29bf56d0..a2f7bb43bf7 100644 --- a/src/libsync/propagateuploadfile.cpp +++ b/src/libsync/propagateuploadfile.cpp @@ -85,8 +85,7 @@ void PropagateUploadFile::startUpload() // TODO: review usage of scoped pointer vs qt memory management // job takes ownership of device via a QScopedPointer. Job deletes itself when finishing - PUTFileJob *job = - new PUTFileJob(propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(path), std::move(device), headers, this); + PUTFileJob *job = new PUTFileJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(path), std::move(device), headers, this); addChildJob(job); connect(job, &PUTFileJob::finishedSignal, this, &PropagateUploadFile::slotPutFinished); connect(job, &PUTFileJob::uploadProgress, this, &PropagateUploadFile::slotUploadProgress); diff --git a/src/libsync/propagateuploadtus.cpp b/src/libsync/propagateuploadtus.cpp index 303131c0213..2adcb4ab7a1 100644 --- a/src/libsync/propagateuploadtus.cpp +++ b/src/libsync/propagateuploadtus.cpp @@ -84,7 +84,7 @@ SimpleNetworkJob *PropagateUploadFileTUS::makeCreationWithUploadJob(QNetworkRequ request->setRawHeader(QByteArrayLiteral("Upload-Metadata"), encodedMetaData.join(',')); request->setRawHeader(QByteArrayLiteral("Upload-Length"), QByteArray::number(_item->_size)); - return new SimpleNetworkJob(propagator()->account().get(), propagator()->webDavUrl(), {}, "POST", device, *request, this); + return new SimpleNetworkJob(propagator()->account(), propagator()->webDavUrl(), {}, "POST", device, *request, this); } QNetworkRequest PropagateUploadFileTUS::prepareRequest(const quint64 &chunkSize) @@ -120,7 +120,7 @@ void PropagateUploadFileTUS::doStartUpload() if (info.validate(_item->_size, _item->_modtime, _item->_checksumHeader)) { _location = info._url; Q_ASSERT(_location.isValid()); - auto job = new SimpleNetworkJob(propagator()->account().get(), _location, {}, "HEAD", nullptr, {}, this); + auto job = new SimpleNetworkJob(propagator()->account(), _location, {}, "HEAD", nullptr, {}, this); connect(job, &SimpleNetworkJob::finishedSignal, this, &PropagateUploadFileTUS::slotChunkFinished); job->start(); } else { @@ -150,7 +150,7 @@ void PropagateUploadFileTUS::startNextChunk() SimpleNetworkJob *job; if (_currentOffset != 0) { qCDebug(lcPropagateUploadTUS) << "Starting to patch upload:" << propagator()->fullRemotePath(_item->_file); - job = new SimpleNetworkJob(propagator()->account().get(), _location, {}, "PATCH", device, req, this); + job = new SimpleNetworkJob(propagator()->account(), _location, {}, "PATCH", device, req, this); } else { Q_ASSERT(_location.isEmpty()); qCDebug(lcPropagateUploadTUS) << "Starting creation with upload:" << propagator()->fullRemotePath(_item->_file); @@ -189,7 +189,7 @@ void PropagateUploadFileTUS::slotChunkFinished() qCWarning(lcPropagateUploadTUS) << propagator()->fullRemotePath(_item->_file) << "Encountered a timeout -> get progress for" << _location; QNetworkRequest req; setTusVersionHeader(req); - auto updateJob = new SimpleNetworkJob(propagator()->account().get(), propagator()->webDavUrl(), _location.path(), "HEAD", {}, req, this); + auto updateJob = new SimpleNetworkJob(propagator()->account(), propagator()->webDavUrl(), _location.path(), "HEAD", {}, req, this); addChildJob(updateJob); connect(updateJob, &SimpleNetworkJob::finishedSignal, this, &PropagateUploadFileTUS::slotChunkFinished); updateJob->start(); @@ -255,8 +255,8 @@ void PropagateUploadFileTUS::slotChunkFinished() if (!_finished) { // Either the ETag or the remote Permissions were not in the headers of the reply. // Start a PROPFIND to fetch these data from the server. - auto check = new PropfindJob( - propagator()->account().get(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); + auto check = + new PropfindJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->_file), PropfindJob::Depth::Zero, this); addChildJob(check); check->setProperties({ "http://owncloud.org/ns:fileid", "http://owncloud.org/ns:permissions", "getetag" }); connect(check, &PropfindJob::directoryListingIterated, this, [this](const QString &, const QMap &map) { diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 34735fefd8d..b3287223d83 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -559,7 +559,7 @@ void SyncEngine::slotDiscoveryFinished() // do a database commit _journal->commit(QStringLiteral("post treewalk")); - _propagator = QSharedPointer::create(_account->sharedFromThis(), syncOptions(), _baseUrl, _localPath, _remotePath, _journal); + _propagator = QSharedPointer::create(_account, syncOptions(), _baseUrl, _localPath, _remotePath, _journal); connect(_propagator.data(), &OwncloudPropagator::itemCompleted, this, &SyncEngine::slotItemCompleted); connect(_propagator.data(), &OwncloudPropagator::progress, From 651fad98b1af1e2a340181b60b6b7bf41e521e84 Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 19:52:44 +0200 Subject: [PATCH 20/23] finished the bulk of the updates. vfs is still to be updated so currently vfs tests don't pass - I am pushing this so I can pull the vfs changes --- src/gui/accountmanager.cpp | 73 ++++++++++--------- src/gui/accountmanager.h | 9 ++- src/gui/accountsettings.cpp | 20 ++--- src/gui/accountstate.cpp | 38 +++++++--- src/gui/accountstate.h | 13 ++-- src/gui/activitywidget.cpp | 5 +- src/gui/application.cpp | 10 ++- src/gui/folder.cpp | 4 +- src/gui/folderman.cpp | 4 +- src/gui/models/activitylistmodel.cpp | 6 +- .../newaccountwizard/newaccountbuilder.cpp | 7 +- src/gui/newaccountwizard/newaccountbuilder.h | 2 +- src/gui/owncloudgui.cpp | 2 +- src/gui/protocolwidget.cpp | 2 +- src/gui/servernotificationhandler.cpp | 2 +- src/gui/settingsdialog.cpp | 8 +- src/gui/socketapi/socketapi.cpp | 15 ++-- src/libsync/account.cpp | 4 +- src/libsync/account.h | 4 +- test/testoauth.cpp | 8 +- test/testutils/syncenginetestutils.cpp | 8 +- test/testutils/syncenginetestutils.h | 2 +- test/testutils/testutils.cpp | 2 +- test/testutils/testutils.h | 2 +- 24 files changed, 136 insertions(+), 114 deletions(-) diff --git a/src/gui/accountmanager.cpp b/src/gui/accountmanager.cpp index e9b71a37b92..2aec3f25fdf 100644 --- a/src/gui/accountmanager.cpp +++ b/src/gui/accountmanager.cpp @@ -109,7 +109,7 @@ bool AccountManager::restore() if (auto acc = loadAccountHelper(*settings)) { acc->_groupIndex = accountIndex; if (auto accState = AccountState::loadFromSettings(acc, *settings)) { - addAccountState(std::move(accState)); + addAccountState(accState); } } settings->endGroup(); @@ -117,35 +117,35 @@ bool AccountManager::restore() return true; } - -AccountPtr AccountManager::createAccount(const NewAccountModel &model) + +Account *AccountManager::createAccount(const NewAccountModel &model) { - auto newAccountPtr = Account::create(QUuid::createUuid(), model.davUser(), model.effectiveUserInfoUrl()); + auto account = new Account(QUuid::createUuid(), model.davUser(), model.effectiveUserInfoUrl()); - newAccountPtr->setDavDisplayName(model.displayName()); + account->setDavDisplayName(model.displayName()); - Credentials *creds = new Credentials(model.authToken(), model.refreshToken(), newAccountPtr.get()); - newAccountPtr->setCredentials(creds); + Credentials *creds = new Credentials(model.authToken(), model.refreshToken(), account); + account->setCredentials(creds); - newAccountPtr->addApprovedCerts(model.trustedCertificates()); + account->addApprovedCerts(model.trustedCertificates()); QString syncRoot = model.defaultSyncRoot(); if (!syncRoot.isEmpty()) { - newAccountPtr->setDefaultSyncRoot(syncRoot); + account->setDefaultSyncRoot(syncRoot); if (!QFileInfo::exists(syncRoot)) { OC_ASSERT(QDir().mkpath(syncRoot)); } - Utility::markDirectoryAsSyncRoot(syncRoot, newAccountPtr->uuid()); + Utility::markDirectoryAsSyncRoot(syncRoot, account->uuid()); } - newAccountPtr->setCapabilities(model.capabilities()); + account->setCapabilities(model.capabilities()); - return newAccountPtr; + return account; } void AccountManager::save() { for (const auto &acc : std::as_const(_accounts)) { - saveAccount(acc->account().data()); + saveAccount(acc->account()); } qCInfo(lcAccountManager) << "Saved all account settings"; @@ -153,6 +153,9 @@ void AccountManager::save() void AccountManager::saveAccount(Account *account) { + if (!account) + return; + qCDebug(lcAccountManager) << "Saving account" << account->url().toString(); auto settings = ConfigFile::settingsWithGroup(accountsC()); settings->beginGroup(account->groupIndex()); @@ -206,9 +209,9 @@ QStringList AccountManager::accountNames() const bool AccountManager::accountForLoginExists(const QUrl &url, const QString &davUser) const { for (const auto &state : _accounts) { - if (state) { - AccountPtr account = state->account(); - if (account && account->url() == url && account->davUser() == davUser) { + if (state && state->account()) { + Account *account = state->account(); + if (account->url() == url && account->davUser() == davUser) { return true; } } @@ -221,18 +224,19 @@ const QList AccountManager::accounts() const return _accounts.values(); } -AccountPtr AccountManager::loadAccountHelper(QSettings &settings) +Account *AccountManager::loadAccountHelper(QSettings &settings) { QVariant urlConfig = settings.value(urlC()); if (!urlConfig.isValid()) { // No URL probably means a corrupted entry in the account settings qCWarning(lcAccountManager) << "No URL for account " << settings.group(); - return AccountPtr(); + return nullptr; } + QString user = settings.value(davUserC()).toString(); if (user.isEmpty()) { qCWarning(lcAccountManager) << "No user name provided for account " << settings.group(); - return AccountPtr(); + return nullptr; } QUrl url = urlConfig.toUrl(); @@ -260,7 +264,7 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings) if (key.contains(uid.toString(QUuid::WithoutBraces))) credsGroup->remove(key); } - return AccountPtr(); + return nullptr; } // 7.0 change - this special handling can be removed in 8.0 @@ -275,13 +279,13 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings) } - auto acc = Account::create(uid, user, url); + auto acc = new Account(uid, user, url); acc->setDavDisplayName(settings.value(davUserDisplyNameC()).toString()); acc->setCapabilities(caps); acc->setDefaultSyncRoot(settings.value(defaultSyncRootC()).toString()); - acc->setCredentials(new Credentials(acc.get())); + acc->setCredentials(new Credentials(acc)); // now the server cert, it is in the general group settings.beginGroup("General"); @@ -321,7 +325,7 @@ AccountState *AccountManager::accountState(const QUuid uuid) return _accounts.value(uuid); } -AccountState *AccountManager::addAccount(const AccountPtr &newAccount) +AccountState *AccountManager::addAccount(Account *newAccount) { auto id = newAccount->groupIndex(); if (id.isEmpty() || !isAccountIndexAvailable(id)) { @@ -330,7 +334,7 @@ AccountState *AccountManager::addAccount(const AccountPtr &newAccount) newAccount->_groupIndex = id; - return addAccountState(AccountState::fromNewAccount(newAccount)); + return addAccountState(new AccountState(newAccount)); } void AccountManager::deleteAccount(AccountState *account) @@ -412,23 +416,22 @@ QString AccountManager::generateFreeAccountIndex() const } } -AccountState *AccountManager::addAccountState(std::unique_ptr &&accountState) +AccountState *AccountManager::addAccountState(AccountState *accountState) { - AccountState *statePtr = accountState.release(); - if (!statePtr) // just bail. I have no idea why this is happening but fine. it's null and not usable - return statePtr; + if (!accountState || !accountState->account()) // just bail. I have no idea why this is happening but fine. it's null and not usable + return nullptr; + + _accounts.insert(accountState->account()->uuid(), accountState); - _accounts.insert(statePtr->account()->uuid(), statePtr); + Account *acc = accountState->account(); - auto *rawAccount = statePtr->account().get(); // this slot can't be connected until the account state exists because saveAccount uses the state - connect(rawAccount, &Account::wantsAccountSaved, this, [rawAccount, this] { saveAccount(rawAccount); }); + connect(acc, &Account::wantsAccountSaved, this, [acc, this] { saveAccount(acc); }); - Q_EMIT accountAdded(statePtr); + Q_EMIT accountAdded(accountState); Q_EMIT accountsChanged(); + accountState->checkConnectivity(); - statePtr->checkConnectivity(); - - return statePtr; + return accountState; } } diff --git a/src/gui/accountmanager.h b/src/gui/accountmanager.h index 9e18d49b0d4..980ba1ab9f7 100644 --- a/src/gui/accountmanager.h +++ b/src/gui/accountmanager.h @@ -53,7 +53,7 @@ class OWNCLOUDGUI_EXPORT AccountManager : public QObject * @param model contains the data used to set up the new account (usually collected from the new account wizard) * @return the AccountPtr associated with the new account. for now. */ - AccountPtr createAccount(const NewAccountModel &model); + Account *createAccount(const NewAccountModel &model); /** * Creates account objects from a given settings file. @@ -68,7 +68,7 @@ class OWNCLOUDGUI_EXPORT AccountManager : public QObject * Typically called from the wizard */ // todo: #28 - this should not be public and should not be called directly by any third party - AccountState *addAccount(const AccountPtr &newAccount); + AccountState *addAccount(Account *newAccount); /** * remove all accounts @@ -119,15 +119,16 @@ public Q_SLOTS: private: // saving and loading Account to settings void saveAccountHelper(Account *account, QSettings &settings, bool saveCredentials = true); - AccountPtr loadAccountHelper(QSettings &settings); + Account *loadAccountHelper(QSettings &settings); bool isAccountIndexAvailable(const QString &index) const; QString generateFreeAccountIndex() const; // Adds an account to the tracked list, emitting accountAdded() - AccountState *addAccountState(std::unique_ptr &&accountState); + AccountState *addAccountState(AccountState *accountState); AccountManager() {} + QMap _accounts; }; } diff --git a/src/gui/accountsettings.cpp b/src/gui/accountsettings.cpp index 65c83557bb4..e450520a95f 100644 --- a/src/gui/accountsettings.cpp +++ b/src/gui/accountsettings.cpp @@ -187,7 +187,7 @@ void AccountSettings::slotCustomContextMenuRequested(Folder *folder) if (folder->accountState()->account()->capabilities().privateLinkPropertyAvailable()) { QString path = folder->remotePathTrailingSlash(); menu->addAction(CommonStrings::showInWebBrowser(), [path, davUrl = folder->webDavUrl(), this] { - fetchPrivateLinkUrl(_accountState->account().get(), davUrl, path, this, [](const QUrl &url) { Utility::openBrowser(url, nullptr); }); + fetchPrivateLinkUrl(_accountState->account(), davUrl, path, this, [](const QUrl &url) { Utility::openBrowser(url, nullptr); }); }); } @@ -258,7 +258,7 @@ void AccountSettings::showSelectiveSyncDialog(Folder *folder) return; } - auto *selectiveSync = new SelectiveSyncWidget(_accountState->account().get(), this); + auto *selectiveSync = new SelectiveSyncWidget(_accountState->account(), this); selectiveSync->setDavUrl(folder->webDavUrl()); bool ok; selectiveSync->setFolderInfo( @@ -280,7 +280,7 @@ void AccountSettings::slotAddFolder() return; } - FolderWizard *folderWizard = new FolderWizard(_accountState->account().get(), this); + FolderWizard *folderWizard = new FolderWizard(_accountState->account(), this); folderWizard->setAttribute(Qt::WA_DeleteOnClose); connect(folderWizard, &QDialog::accepted, this, &AccountSettings::slotFolderWizardAccepted); @@ -553,7 +553,7 @@ void AccountSettings::slotAccountStateChanged(AccountState::State state) return; } - Account *account = _accountState->account().get(); + Account *account = _accountState->account(); qCDebug(lcAccountSettings) << "Account state changed to" << state << "for account" << account; FolderMan *folderMan = FolderMan::instance(); @@ -716,21 +716,21 @@ void AccountSettings::addModalLegacyDialog(QWidget *widget, ModalWidgetSizePolic Q_ASSERT(widget->testAttribute(Qt::WA_DeleteOnClose)); // Refactoring todo: eval this more completely - Q_ASSERT(_accountState); + Q_ASSERT(_accountState && _accountState->account()); connect(widget, &QWidget::destroyed, this, [this, outerWidget] { outerWidget->deleteLater(); if (!_goingDown) { - ocApp()->gui()->settingsDialog()->ceaseModality(_accountState->account().get()); + ocApp()->gui()->settingsDialog()->ceaseModality(_accountState->account()); } }); widget->setVisible(true); - ocApp()->gui()->settingsDialog()->requestModality(_accountState->account().get()); + ocApp()->gui()->settingsDialog()->requestModality(_accountState->account()); } void AccountSettings::addModalAccountWidget(AccountModalWidget *widget) { - if (!_accountState) { + if (!_accountState || !_accountState->account()) { return; } @@ -739,9 +739,9 @@ void AccountSettings::addModalAccountWidget(AccountModalWidget *widget) connect(widget, &AccountModalWidget::finished, this, [widget, this] { widget->deleteLater(); - ocApp()->gui()->settingsDialog()->ceaseModality(_accountState->account().get()); + ocApp()->gui()->settingsDialog()->ceaseModality(_accountState->account()); }); - ocApp()->gui()->settingsDialog()->requestModality(_accountState->account().get()); + ocApp()->gui()->settingsDialog()->requestModality(_accountState->account()); } uint AccountSettings::unsyncedSpaces() const diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index 770cf0bd0ed..f3812a39803 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -48,7 +48,7 @@ namespace OCC { Q_LOGGING_CATEGORY(lcAccountState, "gui.account.state", QtInfoMsg) -AccountState::AccountState(AccountPtr account) +AccountState::AccountState(Account *account) : QObject() , _account(account) , _queueGuard(_account->jobQueue()) @@ -115,9 +115,9 @@ void AccountState::connectNetworkInformation() connect(NetworkInformation::instance(), &NetworkInformation::isBehindCaptivePortalChanged, this, &AccountState::onBehindCaptivePortalChanged); } -std::unique_ptr AccountState::loadFromSettings(AccountPtr account, const QSettings &settings) +AccountState *AccountState::loadFromSettings(Account *account, const QSettings &settings) { - auto accountState = std::unique_ptr(new AccountState(account)); + auto accountState = new AccountState(account); const bool userExplicitlySignedOut = settings.value(userExplicitlySignedOutC(), false).toBool(); if (userExplicitlySignedOut) { // see writeToSettings below @@ -127,14 +127,14 @@ std::unique_ptr AccountState::loadFromSettings(AccountPtr account, return accountState; } -std::unique_ptr AccountState::fromNewAccount(AccountPtr account) +/*std::unique_ptr AccountState::fromNewAccount(AccountPtr account) { // todo: #22. In essence this function creates a unique pointer which in the caller is immediately added to the _accounts map // as a QPointer. Meanwhile the parent is a shared pointer (the account) so who knows when this thing will ever get cleaned up? // add to this that I rarely see anyone checking an account state to see if it's null. We need a consistent impl that reflects // the ownership status of these elements, and make the lifetime concrete return std::unique_ptr(new AccountState(account)); -} +}*/ void AccountState::writeToSettings(QSettings &settings) const { @@ -146,7 +146,13 @@ void AccountState::writeToSettings(QSettings &settings) const settings.setValue(userExplicitlySignedOutC(), _state == SignedOut); } -AccountPtr AccountState::account() const +// this is SOOO temporary +AccountPtr AccountState::accountShared() const +{ + return _account->sharedFromThis(); +} + +Account *AccountState::account() const { return _account; } @@ -168,6 +174,9 @@ AccountState::State AccountState::state() const void AccountState::setState(State state) { + if (!_account) + return; + const State oldState = _state; if (_state != state) { qCInfo(lcAccountState) << "AccountState state change: " << _state << "->" << state; @@ -200,7 +209,7 @@ void AccountState::setState(State state) // ensure the connection validator is done _queueGuard.unblock(); // update capabilities and fetch relevant settings - _fetchCapabilitiesJob = new FetchServerSettingsJob(account().get(), this); + _fetchCapabilitiesJob = new FetchServerSettingsJob(_account, this); connect(_fetchCapabilitiesJob.get(), &FetchServerSettingsJob::finishedSignal, this, [oldState, this] { // Lisa todo: I do not understand this logic at all - review it if (oldState == Connected || _state == Connected) { @@ -224,11 +233,14 @@ bool AccountState::isSignedOut() const void AccountState::signOutByUi() { - account()->credentials()->forgetSensitiveData(); - account()->clearCookieJar(); + if (!_account) + return; + + _account->credentials()->forgetSensitiveData(); + _account->clearCookieJar(); setState(SignedOut); // persist that we are signed out - Q_EMIT account()->wantsAccountSaved(account().data()); + Q_EMIT _account->wantsAccountSaved(_account); } void AccountState::freshConnectionAttempt() @@ -240,11 +252,13 @@ void AccountState::freshConnectionAttempt() void AccountState::signIn() { + if (!_account) + return; if (_state == SignedOut) { _waitingForNewCredentials = false; setState(Disconnected); // persist that we are no longer signed out - Q_EMIT account()->wantsAccountSaved(account().data()); + Q_EMIT account()->wantsAccountSaved(_account); } } @@ -564,7 +578,7 @@ void AccountState::slotCredentialsFetched() Account *AccountState::accountForQml() const { - return _account.data(); + return _account; } std::unique_ptr AccountState::settings() diff --git a/src/gui/accountstate.h b/src/gui/accountstate.h index 2f504cc7bfd..fb4955265e1 100644 --- a/src/gui/accountstate.h +++ b/src/gui/accountstate.h @@ -92,15 +92,15 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject /// The actual current connectivity status. typedef ConnectionValidator::Status ConnectionStatus; + /// Use the account as parent + explicit AccountState(Account *account); ~AccountState() override; /** Creates an account state from settings and an Account object. * * Use from AccountManager with a prepared QSettings object only. */ - static std::unique_ptr loadFromSettings(AccountPtr account, const QSettings &settings); - - static std::unique_ptr fromNewAccount(AccountPtr account); + static AccountState *loadFromSettings(Account *account, const QSettings &settings); /** Writes account state information to settings. * @@ -108,7 +108,8 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject */ void writeToSettings(QSettings &settings) const; - AccountPtr account() const; + AccountPtr accountShared() const; + Account *account() const; ConnectionStatus connectionStatus() const; QStringList connectionErrors() const; @@ -160,8 +161,6 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject void checkConnectivity(bool blockJobs = false); private: - /// Use the account as parent - explicit AccountState(AccountPtr account); void setState(State state); @@ -177,7 +176,7 @@ protected Q_SLOTS: private: Account *accountForQml() const; - AccountPtr _account; + QPointer _account; JobQueueGuard _queueGuard; State _state; ConnectionStatus _connectionStatus; diff --git a/src/gui/activitywidget.cpp b/src/gui/activitywidget.cpp index 9c284a403ba..7e02fc0fbaf 100644 --- a/src/gui/activitywidget.cpp +++ b/src/gui/activitywidget.cpp @@ -315,9 +315,10 @@ void ActivityWidget::slotSendNotificationRequest(const QString &accountName, con }; if (validVerbs.contains(verb)) { - if (auto acc = AccountManager::instance()->account(accountName)) { + auto acc = AccountManager::instance()->account(accountName); + if (acc && acc->account()) { // TODO: host validation? - auto *job = new NotificationConfirmJob(acc->account().get(), QUrl(link), verb, this); + auto *job = new NotificationConfirmJob(acc->account(), QUrl(link), verb, this); job->setWidget(theSender); connect(job, &NotificationConfirmJob::finishedSignal, this, [job, this] { diff --git a/src/gui/application.cpp b/src/gui/application.cpp index 1734a75de32..7d9fc8ecb6b 100644 --- a/src/gui/application.cpp +++ b/src/gui/application.cpp @@ -132,15 +132,17 @@ void Application::lastAccountStateRemoved() const void Application::slotAccountStateAdded(AccountState *accountState) const { + if (!accountState || !accountState->account()) + return; // Hook up the GUI slots to the account state's Q_SIGNALS: + Account *account = accountState->account(); + connect(accountState, &AccountState::stateChanged, _gui.data(), &ownCloudGui::slotComputeOverallSyncStatus); - connect(accountState->account().data(), &Account::serverVersionChanged, _gui.data(), - [account = accountState->account().data(), this] { _gui->slotTrayMessageIfServerUnsupported(account); }); + connect(account, &Account::serverVersionChanged, _gui.data(), [account, this] { _gui->slotTrayMessageIfServerUnsupported(account); }); // Hook up the folder manager slots to the account state's Q_SIGNALS: connect(accountState, &AccountState::isConnectedChanged, FolderMan::instance(), &FolderMan::slotIsConnectedChanged); - connect(accountState->account().data(), &Account::serverVersionChanged, FolderMan::instance(), - [account = accountState->account().data()] { FolderMan::instance()->slotServerVersionChanged(account); }); + connect(account, &Account::serverVersionChanged, FolderMan::instance(), [account] { FolderMan::instance()->slotServerVersionChanged(account); }); } void Application::slotCleanup() diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index c8dd74039ec..ca665eb1f0a 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -115,7 +115,7 @@ Folder::Folder(const FolderDefinition &definition, AccountState *accountState, s _journal.wipeErrorBlacklistCategory(SyncJournalErrorBlacklistRecord::Category::LocalSoftError); // todo: the engine needs to be created externally, presumably by the folderman, and passed in by injection // current impl can result in an invalid engine which is just a mess given the folder is useless without it - _engine.reset(new SyncEngine(_accountState->account().get(), webDavUrl(), path(), remotePath(), &_journal)); + _engine.reset(new SyncEngine(_accountState->account(), webDavUrl(), path(), remotePath(), &_journal)); // pass the setting if hidden files are to be ignored, will be read in csync_update _engine->setIgnoreHiddenFiles(ignoreHiddenFiles); @@ -502,7 +502,7 @@ void Folder::startVfs() return; } - VfsSetupParams vfsParams(_accountState->account(), webDavUrl(), groupInSidebar(), _engine.get()); + VfsSetupParams vfsParams(_accountState->accountShared(), webDavUrl(), groupInSidebar(), _engine.get()); vfsParams.filesystemPath = path(); vfsParams.remotePath = remotePathTrailingSlash(); vfsParams.journal = &_journal; diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 20441887b6f..32bb6c43f23 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -471,7 +471,7 @@ void FolderMan::slotServerVersionChanged(Account *account) << "pausing all folders on the account"; for (auto &f : std::as_const(_folders)) { - if (f->accountState()->account().data() == account) { + if (f->accountState()->account() == account) { f->setSyncPaused(true); } } @@ -644,7 +644,7 @@ QStringList FolderMan::findFileInLocalFolders(const QString &relPath, const Acco serverPath.prepend(QLatin1Char('/')); for (auto *folder : std::as_const(_folders)) { - if (acc && folder->accountState() && folder->accountState()->account().get() != acc) { + if (acc && folder->accountState() && folder->accountState()->account() != acc) { continue; } if (!serverPath.startsWith(folder->remotePath())) diff --git a/src/gui/models/activitylistmodel.cpp b/src/gui/models/activitylistmodel.cpp index 127ce9887e0..6e8bfbb3140 100644 --- a/src/gui/models/activitylistmodel.cpp +++ b/src/gui/models/activitylistmodel.cpp @@ -71,12 +71,12 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const return Utility::timeAgoInWords(a.dateTime()); } case ActivityRole::Path: { - QStringList list = FolderMan::instance()->findFileInLocalFolders(a.file(), accountState->account().get()); + QStringList list = FolderMan::instance()->findFileInLocalFolders(a.file(), accountState->account()); if (!list.isEmpty()) { return list.at(0); } // File does not exist anymore? Let's try to open its path - list = FolderMan::instance()->findFileInLocalFolders(QFileInfo(a.file()).path(), accountState->account().get()); + list = FolderMan::instance()->findFileInLocalFolders(QFileInfo(a.file()).path(), accountState->account()); if (!list.isEmpty()) { return list.at(0); } @@ -186,7 +186,7 @@ void ActivityListModel::startFetchJob(AccountState *accountState) if (!accountState || !accountState->isConnected() || !accountState->account()) { return; } - auto *job = new JsonApiJob(accountState->account().get(), QStringLiteral("ocs/v2.php/cloud/activity"), + auto *job = new JsonApiJob(accountState->account(), QStringLiteral("ocs/v2.php/cloud/activity"), {{QStringLiteral("page"), QStringLiteral("0")}, {QStringLiteral("pagesize"), QStringLiteral("100")}}, {}, this); QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, accountState, this] { diff --git a/src/gui/newaccountwizard/newaccountbuilder.cpp b/src/gui/newaccountwizard/newaccountbuilder.cpp index e0914b0a1b5..2de3571e28a 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.cpp +++ b/src/gui/newaccountwizard/newaccountbuilder.cpp @@ -29,6 +29,7 @@ NewAccountBuilder::NewAccountBuilder(const NewAccountModel &model, QObject *pare void NewAccountBuilder::buildAccount() { + Q_ASSERT(_account); // the account manager triggers the first checkConnection in the state when it's added - there should be no need to call it again explicitly // either in the application or the folderman, as was the case previously _accountState = AccountManager::instance()->addAccount(_account); @@ -57,7 +58,9 @@ void NewAccountBuilder::onAccountStateChanged(AccountState::State state) void NewAccountBuilder::completeAccountSetup() { - AccountManager::instance()->saveAccount(_account.get()); + Q_ASSERT(_account); + + AccountManager::instance()->saveAccount(_account); // the creds should always be ready when creating new account as we don't get here unless the auth and all other checks succeeded // the check is perfunctory. @@ -84,7 +87,7 @@ void NewAccountBuilder::completeAccountSetup() // if selective sync is selected, we need to run the folder wizard asap. if (_syncType == NewAccount::SyncType::SELECTIVE_SYNC) - Q_EMIT requestFolderWizard(_account.get()); + Q_EMIT requestFolderWizard(_account); deleteLater(); } diff --git a/src/gui/newaccountwizard/newaccountbuilder.h b/src/gui/newaccountwizard/newaccountbuilder.h index cd5dd3a486f..d9c206b4961 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.h +++ b/src/gui/newaccountwizard/newaccountbuilder.h @@ -40,7 +40,7 @@ class NewAccountBuilder : public QObject void onAccountStateChanged(AccountState::State state); void completeAccountSetup(); - AccountPtr _account = nullptr; + Account *_account = nullptr; AccountState *_accountState = nullptr; NewAccount::SyncType _syncType = NewAccount::SyncType::NONE; }; diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp index 04e4741ed49..56ce2d2cd81 100644 --- a/src/gui/owncloudgui.cpp +++ b/src/gui/owncloudgui.cpp @@ -319,7 +319,7 @@ void ownCloudGui::slotShowShareInBrowser(const QString &sharePath, const QString } if (folder->accountState()->account()->capabilities().filesSharing().sharing_roles) { - fetchPrivateLinkUrl(folder->accountState()->account().get(), folder->webDavUrl(), sharePath, this, [](const QUrl &url) { + fetchPrivateLinkUrl(folder->accountState()->account(), folder->webDavUrl(), sharePath, this, [](const QUrl &url) { const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("sharing")}}); Utility::openBrowser(queryUrl, nullptr); }); diff --git a/src/gui/protocolwidget.cpp b/src/gui/protocolwidget.cpp index a092b2161c9..39fc3209cad 100644 --- a/src/gui/protocolwidget.cpp +++ b/src/gui/protocolwidget.cpp @@ -134,7 +134,7 @@ void ProtocolWidget::showContextMenu(QWidget *parent, QTableView *table, Models: // "Open in Browser" action auto showInWebBrowserAction = menu->addAction(CommonStrings::showInWebBrowser()); showInWebBrowserAction->setEnabled(false); - fetchPrivateLinkUrl(data.folder()->accountState()->account().get(), data.folder()->webDavUrl(), data.folder()->remotePathTrailingSlash() + data.path(), + fetchPrivateLinkUrl(data.folder()->accountState()->account(), data.folder()->webDavUrl(), data.folder()->remotePathTrailingSlash() + data.path(), parent, [showInWebBrowserAction, parent, pos = menu->actions().size(), menu = QPointer(menu)](const QUrl &url) { // as fetchPrivateLinkUrl is async we need to check the menu still exists if (menu) { diff --git a/src/gui/servernotificationhandler.cpp b/src/gui/servernotificationhandler.cpp index ea6605ea2b8..f7896396a5c 100644 --- a/src/gui/servernotificationhandler.cpp +++ b/src/gui/servernotificationhandler.cpp @@ -52,7 +52,7 @@ void ServerNotificationHandler::slotFetchNotifications(AccountState *accountStat } // if the previous notification job has finished, start next. - auto *job = new JsonApiJob(accountState->account().get(), notificationsPath, {}, {}, this); + auto *job = new JsonApiJob(accountState->account(), notificationsPath, {}, {}, this); QObject::connect(job, &JsonApiJob::finishedSignal, this, [job, accountState, this] { slotNotificationsReceived(job, accountState); deleteLater(); diff --git a/src/gui/settingsdialog.cpp b/src/gui/settingsdialog.cpp index a0c6ac4098c..aea4dd60611 100644 --- a/src/gui/settingsdialog.cpp +++ b/src/gui/settingsdialog.cpp @@ -151,7 +151,7 @@ SettingsDialog::SettingsDialog(ownCloudGui *gui, QWidget *parent) onAccountAdded(accountState); } if (!_widgetForAccount.isEmpty()) { - setCurrentAccount(accounts.first()->account().get()); + setCurrentAccount(accounts.first()->account()); } connect(AccountManager::instance(), &AccountManager::accountAdded, this, &SettingsDialog::onAccountAdded); @@ -170,8 +170,8 @@ void SettingsDialog::onAccountAdded(AccountState *state) return; auto accountSettings = new AccountSettings(state, this); _ui->stack->addWidget(accountSettings); - _widgetForAccount.insert(state->account().data(), accountSettings); - setCurrentAccount(state->account().get()); + _widgetForAccount.insert(state->account(), accountSettings); + setCurrentAccount(state->account()); } void SettingsDialog::onAccountRemoved(AccountState *state) @@ -179,7 +179,7 @@ void SettingsDialog::onAccountRemoved(AccountState *state) if (!state || !state->account()) return; // todo: #37. using the account after we know it's been removed is not ok. - Account *acc = state->account().data(); + Account *acc = state->account(); if (AccountSettings *asw = _widgetForAccount.value(acc)) { _widgetForAccount.remove(acc); _ui->stack->removeWidget(asw); diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index 8297d1de480..1fc77f46d16 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -332,7 +332,7 @@ void SocketApi::unregisterAccount(AccountState *state) if (!state || !state->account()) return; - Account *a = state->account().get(); + Account *a = state->account(); if (!a->defaultSyncRoot().isEmpty()) { broadcastMessage(buildMessage(unregisterPathMessageC(), Utility::stripTrailingSlash(a->defaultSyncRoot()))); @@ -508,7 +508,7 @@ void SocketApi::fetchPrivateLinkUrlHelper(const QString &localFile, const std::f return; } - fetchPrivateLinkUrl(fileData.folder->accountState()->account().get(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, targetFun); + fetchPrivateLinkUrl(fileData.folder->accountState()->account(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, targetFun); } void SocketApi::command_COPY_PRIVATE_LINK(const QString &localFile, SocketListener *) @@ -530,11 +530,10 @@ void SocketApi::command_OPEN_PRIVATE_LINK_VERSIONS(const QString &localFile, Soc { const auto fileData = FileData::get(localFile); if (fileData.isValid() && fileData.folder->accountState()->account()->capabilities().filesSharing().sharing_roles) { - fetchPrivateLinkUrl( - fileData.folder->accountState()->account().get(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, [](const QUrl &url) { - const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("versions")}}); - Utility::openBrowser(queryUrl, nullptr); - }); + fetchPrivateLinkUrl(fileData.folder->accountState()->account(), fileData.folder->webDavUrl(), fileData.serverRelativePath, this, [](const QUrl &url) { + const auto queryUrl = Utility::concatUrlPath(url, QString(), {{QStringLiteral("details"), QStringLiteral("versions")}}); + Utility::openBrowser(queryUrl, nullptr); + }); } else { fetchPrivateLinkUrlHelper(localFile, [](const QUrl &link) { Utility::openBrowser(Utility::concatUrlPath(link, {}, {{QStringLiteral("details"), QStringLiteral("versionsTabView")}}), nullptr); @@ -663,7 +662,7 @@ Q_INVOKABLE void OCC::SocketApi::command_OPEN_APP_LINK(const QString &localFile, const auto &provider = data.folder->accountState()->account()->appProvider(); const auto record = data.journalRecord(); if (record.isValid()) { - provider.open(data.folder->accountState()->account().get(), localFile, record._fileId); + provider.open(data.folder->accountState()->account(), localFile, record._fileId); } } } diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 034d72dc300..9b4496cc28e 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -83,12 +83,12 @@ Account::Account(const QUuid &uuid, const QString &user, const QUrl &url, QObjec _spacesManager = new GraphApi::SpacesManager(this); } -AccountPtr Account::create(const QUuid &uuid, const QString &user, const QUrl &url) +/*AccountPtr Account::create(const QUuid &uuid, const QString &user, const QUrl &url) { AccountPtr acc = AccountPtr(new Account(uuid, user, url)); acc->setSharedThis(acc); return acc; -} +}*/ Account::~Account() { diff --git a/src/libsync/account.h b/src/libsync/account.h index 295632b6f3e..f2c9506bd32 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -89,7 +89,8 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject static void setCommonCacheDirectory(const QString &directory); static QString commonCacheDirectory(); - static AccountPtr create(const QUuid &uuid, const QString &user, const QUrl &url); + // static AccountPtr create(const QUuid &uuid, const QString &user, const QUrl &url); + Account(const QUuid &uuid, const QString &user, const QUrl &url, QObject *parent = nullptr); ~Account() override; void cleanupForRemoval(); @@ -254,7 +255,6 @@ public Q_SLOTS: // directory all newly created accounts store their various caches in static QString _customCommonCacheDirectory; - Account(const QUuid &uuid, const QString &user, const QUrl &url, QObject *parent = nullptr); void setSharedThis(AccountPtr sharedThis); QWeakPointer _sharedThis; diff --git a/test/testoauth.cpp b/test/testoauth.cpp index b5a33458fd4..77d9edf0697 100644 --- a/test/testoauth.cpp +++ b/test/testoauth.cpp @@ -125,19 +125,19 @@ class OAuthTestCase : public QObject QNetworkAccessManager realQNAM; QPointer browserReply = nullptr; QString code = QString::fromUtf8(generateEtag()); - OCC::AccountPtr account; + OCC::Account *account; std::unique_ptr oauth; virtual std::unique_ptr prepareOauth() { fakeAm = new FakeAM(nullptr); - account = Account::create(QUuid::createUuid(), "admin", sOpenIdBaseURL); + account = new Account(QUuid::createUuid(), "admin", sOpenIdBaseURL); // the account seizes ownership over the qnam in account->setCredentials(...) by keeping a shared pointer on it // therefore, we should never call fakeAm->setThis(...) // Lisa todo: ??? - account->setCredentials(new FakeCredentials(account.get(), fakeAm, account.get())); + account->setCredentials(new FakeCredentials(account, fakeAm, account)); fakeAm->setOverride([this](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *device) { if (req.url().path().endsWith(QLatin1String(".well-known/openid-configuration"))) { return this->wellKnownReply(op, req); @@ -155,7 +155,7 @@ class OAuthTestCase : public QObject QObject::connect(&desktopServiceHook, &DesktopServiceHook::hooked, this, &OAuthTestCase::openBrowserHook); - auto out = std::make_unique(account.get(), nullptr); + auto out = std::make_unique(account, nullptr); QObject::connect(out.get(), &OAuth::result, this, &OAuthTestCase::oauthResult); return out; } diff --git a/test/testutils/syncenginetestutils.cpp b/test/testutils/syncenginetestutils.cpp index e3ff394e42d..e79f65e2d13 100644 --- a/test/testutils/syncenginetestutils.cpp +++ b/test/testutils/syncenginetestutils.cpp @@ -790,11 +790,11 @@ OCC::TestUtils::TestUtilsPrivate::AccountStateRaii createDummyAccountWithFileSup // ensure we have an instance of folder man std::ignore = OCC::TestUtils::folderMan(); // don't use the account manager to create the account, it would try to use widgets - auto acc = OCC::Account::create(QUuid::createUuid(), QStringLiteral("admin"), QUrl(QStringLiteral("http://localhost/owncloud"))); + auto acc = new OCC::Account(QUuid::createUuid(), QStringLiteral("admin"), QUrl(QStringLiteral("http://localhost/owncloud"))); // todo: #41 FakeAM *am = new FakeAM(intialRoot, nullptr); - auto creds = new FakeCredentials(acc.get(), am, acc.get()); + auto creds = new FakeCredentials(acc, am, acc); acc->setCredentials(creds); acc->setDavDisplayName(QStringLiteral("fakename") + acc->uuid().toString(QUuid::WithoutBraces)); acc->setCapabilities({acc->url(), OCC::TestUtils::testCapabilities()}); @@ -807,8 +807,8 @@ OCC::TestUtils::TestUtilsPrivate::AccountStateRaii createDummyAccount() // ensure we have an instance of folder man std::ignore = OCC::TestUtils::folderMan(); // don't use the account manager to create the account, it would try to use widgets - auto acc = OCC::Account::create(QUuid::createUuid(), QStringLiteral("admin"), QUrl(QStringLiteral("http://localhost/owncloud"))); - auto creds = new FakeCredentials(acc.get(), acc.get()); + auto acc = new OCC::Account(QUuid::createUuid(), QStringLiteral("admin"), QUrl(QStringLiteral("http://localhost/owncloud"))); + auto creds = new FakeCredentials(acc, acc); acc->setCredentials(creds); acc->setDavDisplayName(QStringLiteral("fakename") + acc->uuid().toString(QUuid::WithoutBraces)); acc->setCapabilities({acc->url(), OCC::TestUtils::testCapabilities()}); diff --git a/test/testutils/syncenginetestutils.h b/test/testutils/syncenginetestutils.h index a45fe6614ca..10fd9dde391 100644 --- a/test/testutils/syncenginetestutils.h +++ b/test/testutils/syncenginetestutils.h @@ -575,7 +575,7 @@ class FakeFolder : public QObject void switchToVfs(QSharedPointer vfs); - OCC::Account *account() const { return _accountState->account().get(); } + OCC::Account *account() const { return _accountState->account(); } OCC::SyncEngine &syncEngine() const { return *_syncEngine; } OCC::SyncJournalDb &syncJournal() const { return *_journalDb; } diff --git a/test/testutils/testutils.cpp b/test/testutils/testutils.cpp index 1b10354edf7..133afe31bfd 100644 --- a/test/testutils/testutils.cpp +++ b/test/testutils/testutils.cpp @@ -11,7 +11,7 @@ namespace OCC { namespace TestUtils { // We have more than one of these? - FolderDefinition createDummyFolderDefinition(const AccountPtr &account, const QString &path) + FolderDefinition createDummyFolderDefinition(Account *account, const QString &path) { // TODO: legacy auto d = OCC::FolderDefinition::createNewFolderDefinition(account->davUrl(), {}); diff --git a/test/testutils/testutils.h b/test/testutils/testutils.h index ca82a121625..dae21821fff 100644 --- a/test/testutils/testutils.h +++ b/test/testutils/testutils.h @@ -21,7 +21,7 @@ namespace TestUtils { } FolderMan *folderMan(); - FolderDefinition createDummyFolderDefinition(const AccountPtr &account, const QString &path); + FolderDefinition createDummyFolderDefinition(Account *account, const QString &path); // TestUtilsPrivate::AccountStateRaii createDummyAccount(); bool writeRandomFile(const QString &fname, int size = -1); From 51098eceee98a5ef3e49af64c12a439da2f12dc9 Mon Sep 17 00:00:00 2001 From: modSpike Date: Wed, 1 Oct 2025 20:29:13 +0200 Subject: [PATCH 21/23] merged the vfs changes in, updated the accountPtr to raw pointer in vfs - still need to turn it into a qpointer and check for nulls but it works now (local tests passed too) so should be no problem. --- src/common/vfs.cpp | 2 +- src/common/vfs.h | 10 +++++++--- src/gui/folder.cpp | 2 +- test/testutils/syncenginetestutils.cpp | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/common/vfs.cpp b/src/common/vfs.cpp index 510a69599f1..e0e06f09664 100644 --- a/src/common/vfs.cpp +++ b/src/common/vfs.cpp @@ -276,7 +276,7 @@ const VfsPluginManager &VfsPluginManager::instance() return *_instance; } -VfsSetupParams::VfsSetupParams(const AccountPtr &account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine) +VfsSetupParams::VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine) : account(account) , _baseUrl(baseUrl) , _groupInSidebar(groupInSidebar) diff --git a/src/common/vfs.h b/src/common/vfs.h index abaad81b12a..c793001e7ab 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -29,17 +29,21 @@ #include + namespace OCC { -class Account; class SyncJournalDb; class SyncFileItem; class SyncEngine; +class Account; + /** Collection of parameters for initializing a Vfs instance. */ struct OCSYNC_EXPORT VfsSetupParams { - explicit VfsSetupParams(const AccountPtr &account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine); + // todo: the account is only used to get the displayNameWithHost + capabilities.preferredUploadChecksumType - both of which I would think are const + // once the account is created? if so, consider replacing the account ptr with those much simpler, safer values asap + explicit VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine); /** The full path to the folder on the local filesystem * * Always ends with /. @@ -53,7 +57,7 @@ struct OCSYNC_EXPORT VfsSetupParams QString remotePath; /// Account url, credentials etc. for network calls - AccountPtr account; + Account *account; /** Access to the sync folder's database. * diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index ca665eb1f0a..861e41f3abb 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -502,7 +502,7 @@ void Folder::startVfs() return; } - VfsSetupParams vfsParams(_accountState->accountShared(), webDavUrl(), groupInSidebar(), _engine.get()); + VfsSetupParams vfsParams(_accountState->account(), webDavUrl(), groupInSidebar(), _engine.get()); vfsParams.filesystemPath = path(); vfsParams.remotePath = remotePathTrailingSlash(); vfsParams.journal = &_journal; diff --git a/test/testutils/syncenginetestutils.cpp b/test/testutils/syncenginetestutils.cpp index e79f65e2d13..baeda988983 100644 --- a/test/testutils/syncenginetestutils.cpp +++ b/test/testutils/syncenginetestutils.cpp @@ -926,7 +926,7 @@ void FakeFolder::switchToVfs(QSharedPointer vfs) opts._vfs = vfs; _syncEngine->setSyncOptions(opts); - OCC::VfsSetupParams vfsParams(account()->sharedFromThis(), account()->davUrl(), false, &syncEngine()); + OCC::VfsSetupParams vfsParams(account(), account()->davUrl(), false, &syncEngine()); vfsParams.filesystemPath = localPath(); vfsParams.remotePath = QLatin1Char('/'); vfsParams.journal = _journalDb.get(); From 0ca983e8e3128eda1387de6c27f881c51897a2b3 Mon Sep 17 00:00:00 2001 From: modspike Date: Thu, 2 Oct 2025 13:44:56 +0200 Subject: [PATCH 22/23] removed accountfwd.h and all associated refs to AccountPtr - it's now gone. also did some basic cleanup on some includes as well as updating to #pragma once where I ran into it --- src/common/vfs.h | 13 ++++---- src/gui/accountstate.cpp | 15 ---------- src/gui/accountstate.h | 2 -- src/gui/activitywidget.cpp | 1 - src/gui/activitywidget.h | 12 ++++---- src/gui/connectionvalidator.h | 2 +- src/gui/creds/oauth.cpp | 1 - src/gui/creds/oauth.h | 4 ++- src/gui/folderman.cpp | 3 +- src/gui/folderstatusmodel.h | 7 ++--- src/gui/folderwizard/folderwizard.cpp | 6 ---- src/gui/folderwizard/folderwizard.h | 1 - src/gui/folderwizard/folderwizard_p.h | 5 ++-- .../newaccountwizard/newaccountbuilder.cpp | 4 +-- src/gui/newaccountwizard/newaccountbuilder.h | 3 +- src/gui/notificationconfirmjob.h | 6 ++-- src/gui/owncloudgui.h | 3 +- src/gui/servernotificationhandler.cpp | 1 + src/gui/socketapi/socketapi.cpp | 1 + src/gui/socketapi/socketapi.h | 5 +++- src/gui/spaces/spacesmodel.h | 2 -- src/libsync/abstractcorejob.cpp | 6 +--- src/libsync/abstractnetworkjob.h | 7 ++--- src/libsync/account.cpp | 21 +------------ src/libsync/account.h | 23 +++----------- src/libsync/accountfwd.h | 30 ------------------- src/libsync/appprovider.h | 5 ++-- src/libsync/creds/abstractcredentials.h | 2 +- src/libsync/graphapi/space.cpp | 1 - src/libsync/graphapi/space.h | 2 -- src/libsync/syncengine.h | 4 +-- 31 files changed, 50 insertions(+), 148 deletions(-) delete mode 100644 src/libsync/accountfwd.h diff --git a/src/common/vfs.h b/src/common/vfs.h index c793001e7ab..b5b88e90bb6 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -13,9 +13,12 @@ */ #pragma once +// if this is included, to support a qpointer on the account, all hell breaks loose wrt endless "cannot open include file: 'owncloudlib.h' +// No such file or directory" which is fundamental to the libsync exports +// so unfortunately we can't make a qpointer for the account here - do the update described below asap to get rid of the dependency +// #include "libsync/account.h" #include "assert.h" #include "csync/csync.h" -#include "libsync/accountfwd.h" #include "ocsynclib.h" #include "pinstate.h" #include "result.h" @@ -23,7 +26,7 @@ #include "utility.h" #include -#include +#include #include #include @@ -32,17 +35,15 @@ namespace OCC { +class Account; + class SyncJournalDb; class SyncFileItem; class SyncEngine; -class Account; - /** Collection of parameters for initializing a Vfs instance. */ struct OCSYNC_EXPORT VfsSetupParams { - // todo: the account is only used to get the displayNameWithHost + capabilities.preferredUploadChecksumType - both of which I would think are const - // once the account is created? if so, consider replacing the account ptr with those much simpler, safer values asap explicit VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine); /** The full path to the folder on the local filesystem * diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index f3812a39803..50e9a8c21b7 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -127,15 +127,6 @@ AccountState *AccountState::loadFromSettings(Account *account, const QSettings & return accountState; } -/*std::unique_ptr AccountState::fromNewAccount(AccountPtr account) -{ - // todo: #22. In essence this function creates a unique pointer which in the caller is immediately added to the _accounts map - // as a QPointer. Meanwhile the parent is a shared pointer (the account) so who knows when this thing will ever get cleaned up? - // add to this that I rarely see anyone checking an account state to see if it's null. We need a consistent impl that reflects - // the ownership status of these elements, and make the lifetime concrete - return std::unique_ptr(new AccountState(account)); -}*/ - void AccountState::writeToSettings(QSettings &settings) const { // The SignedOut state is the only state where the client should *not* ask for credentials, nor @@ -146,12 +137,6 @@ void AccountState::writeToSettings(QSettings &settings) const settings.setValue(userExplicitlySignedOutC(), _state == SignedOut); } -// this is SOOO temporary -AccountPtr AccountState::accountShared() const -{ - return _account->sharedFromThis(); -} - Account *AccountState::account() const { return _account; diff --git a/src/gui/accountstate.h b/src/gui/accountstate.h index fb4955265e1..3ab618247b3 100644 --- a/src/gui/accountstate.h +++ b/src/gui/accountstate.h @@ -19,7 +19,6 @@ #include "gui/owncloudguilib.h" #include "connectionvalidator.h" -#include "creds/abstractcredentials.h" #include "jobqueue.h" #include "account.h" @@ -108,7 +107,6 @@ class OWNCLOUDGUI_EXPORT AccountState : public QObject */ void writeToSettings(QSettings &settings) const; - AccountPtr accountShared() const; Account *account() const; ConnectionStatus connectionStatus() const; diff --git a/src/gui/activitywidget.cpp b/src/gui/activitywidget.cpp index 7e02fc0fbaf..da3badeb76a 100644 --- a/src/gui/activitywidget.cpp +++ b/src/gui/activitywidget.cpp @@ -17,7 +17,6 @@ #include "QProgressIndicator.h" -#include "account.h" #include "accountmanager.h" #include "accountstate.h" #include "activitywidget.h" diff --git a/src/gui/activitywidget.h b/src/gui/activitywidget.h index c8561f687a1..a6e747546cb 100644 --- a/src/gui/activitywidget.h +++ b/src/gui/activitywidget.h @@ -12,8 +12,7 @@ * for more details. */ -#ifndef ACTIVITYWIDGET_H -#define ACTIVITYWIDGET_H +#pragma once #include #include @@ -21,9 +20,7 @@ #include #include -#include "progressdispatcher.h" #include "owncloudgui.h" -#include "account.h" #include "activitydata.h" #include "models/models.h" @@ -35,8 +32,8 @@ class QVBoxLayout; namespace OCC { -class Account; -class AccountStatusPtr; +class AccountState; +//class AccountStatusPtr; class ProtocolWidget; class IssuesWidget; class JsonApiJob; @@ -162,7 +159,8 @@ private Q_SLOTS: IssuesWidget *_issuesWidget; QProgressIndicator *_progressIndicator; QTimer _notificationCheckTimer; + // todo: *never* hash on a pointer - this is so dangerous + // replace the pointer with the account uuid for safety QHash _timeSinceLastCheck; }; } -#endif // ActivityWIDGET_H diff --git a/src/gui/connectionvalidator.h b/src/gui/connectionvalidator.h index ade4516abdf..c14ce0cb5fd 100644 --- a/src/gui/connectionvalidator.h +++ b/src/gui/connectionvalidator.h @@ -17,7 +17,7 @@ #include "gui/owncloudguilib.h" #include "common/chronoelapsedtimer.h" -#include "libsync/accountfwd.h" +#include "libsync/account.h" #include #include diff --git a/src/gui/creds/oauth.cpp b/src/gui/creds/oauth.cpp index 9ff4ecc552a..4f09d228eff 100644 --- a/src/gui/creds/oauth.cpp +++ b/src/gui/creds/oauth.cpp @@ -15,7 +15,6 @@ #include "oauth.h" #include "accessmanager.h" -#include "account.h" #include "creds/credentialssupport.h" #include "gui/networkadapters/userinfoadapter.h" #include "libsync/creds/credentialmanager.h" diff --git a/src/gui/creds/oauth.h b/src/gui/creds/oauth.h index a27017d8aaa..b6d089c4260 100644 --- a/src/gui/creds/oauth.h +++ b/src/gui/creds/oauth.h @@ -13,9 +13,11 @@ */ #pragma once -#include "accountfwd.h" + #include "owncloudlib.h" +#include "account.h" + #include #include #include diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index 32bb6c43f23..c68813212d9 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -17,8 +17,8 @@ #include "account.h" #include "accountmanager.h" #include "accountstate.h" +#include "accessmanager.h" #include "common/asserts.h" -#include "common/depreaction.h" #include "configfile.h" #include "folder.h" #include "gui/networkinformation.h" @@ -39,7 +39,6 @@ #include #include #include -#include using namespace std::chrono; using namespace std::chrono_literals; diff --git a/src/gui/folderstatusmodel.h b/src/gui/folderstatusmodel.h index 72a3b7b5ae7..60c74d737ea 100644 --- a/src/gui/folderstatusmodel.h +++ b/src/gui/folderstatusmodel.h @@ -12,12 +12,12 @@ * for more details. */ -#ifndef FOLDERSTATUSMODEL_H -#define FOLDERSTATUSMODEL_H +#pragma once -#include "accountfwd.h" +#include "accountstate.h" #include "progressdispatcher.h" +#include #include #include #include @@ -91,4 +91,3 @@ private Q_SLOTS: } // namespace OCC -#endif // FOLDERSTATUSMODEL_H diff --git a/src/gui/folderwizard/folderwizard.cpp b/src/gui/folderwizard/folderwizard.cpp index 6ec2cd242ea..ef152aeba8c 100644 --- a/src/gui/folderwizard/folderwizard.cpp +++ b/src/gui/folderwizard/folderwizard.cpp @@ -27,7 +27,6 @@ #include "gui/settingsdialog.h" #include "theme.h" -#include "gui/accountstate.h" #include "gui/folderman.h" #include "libsync/graphapi/space.h" @@ -131,11 +130,6 @@ QString FolderWizardPrivate::displayName() const return _spacesPage->currentSpace()->displayName(); } -/*AccountState *FolderWizardPrivate::accountState() -{ - return _accountState; -}*/ - bool FolderWizardPrivate::useVirtualFiles() const { const auto mode = VfsPluginManager::instance().bestAvailableVfsMode(); diff --git a/src/gui/folderwizard/folderwizard.h b/src/gui/folderwizard/folderwizard.h index fed091781ce..59c938d8b0f 100644 --- a/src/gui/folderwizard/folderwizard.h +++ b/src/gui/folderwizard/folderwizard.h @@ -17,7 +17,6 @@ #include #include -#include "accountfwd.h" #include "gui/folderman.h" class QCheckBox; diff --git a/src/gui/folderwizard/folderwizard_p.h b/src/gui/folderwizard/folderwizard_p.h index cd68dca1b7e..f7a450f9cb2 100644 --- a/src/gui/folderwizard/folderwizard_p.h +++ b/src/gui/folderwizard/folderwizard_p.h @@ -20,8 +20,9 @@ #include "folderwizard.h" -#include "libsync/accountfwd.h" +#include "libsync/account.h" +#include #include #include #include @@ -52,8 +53,6 @@ class FolderWizardPrivate bool useVirtualFiles() const; QString displayName() const; - // AccountState *accountState(); - private: Q_DECLARE_PUBLIC(FolderWizard) FolderWizard *q_ptr; diff --git a/src/gui/newaccountwizard/newaccountbuilder.cpp b/src/gui/newaccountwizard/newaccountbuilder.cpp index 2de3571e28a..00053a2859d 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.cpp +++ b/src/gui/newaccountwizard/newaccountbuilder.cpp @@ -12,10 +12,10 @@ * for more details. */ #include "newaccountbuilder.h" -#include "account.h" + #include "accountmanager.h" #include "accountsettings.h" -#include "accountstate.h" +#include "creds/abstractcredentials.h" namespace OCC { diff --git a/src/gui/newaccountwizard/newaccountbuilder.h b/src/gui/newaccountwizard/newaccountbuilder.h index d9c206b4961..9363b82be51 100644 --- a/src/gui/newaccountwizard/newaccountbuilder.h +++ b/src/gui/newaccountwizard/newaccountbuilder.h @@ -16,13 +16,14 @@ #include "newaccountmodel.h" #include -#include "accountfwd.h" +#include "libsync/account.h" #include "accountstate.h" #include "newaccountenums.h" namespace OCC { + class NewAccountBuilder : public QObject { Q_OBJECT diff --git a/src/gui/notificationconfirmjob.h b/src/gui/notificationconfirmjob.h index df2a8b91327..9733572ec00 100644 --- a/src/gui/notificationconfirmjob.h +++ b/src/gui/notificationconfirmjob.h @@ -12,11 +12,9 @@ * for more details. */ -#ifndef NOTIFICATIONCONFIRMJOB_H -#define NOTIFICATIONCONFIRMJOB_H +#pragma once #include "abstractnetworkjob.h" -#include "accountfwd.h" #include "networkjobs/jsonjob.h" #include @@ -26,6 +24,7 @@ namespace OCC { +class Account; class NotificationWidget; /** @@ -67,4 +66,3 @@ class NotificationConfirmJob : public JsonApiJob }; } -#endif // NotificationConfirmJob_H diff --git a/src/gui/owncloudgui.h b/src/gui/owncloudgui.h index 44adeb813ad..a4a9e3443fe 100644 --- a/src/gui/owncloudgui.h +++ b/src/gui/owncloudgui.h @@ -14,7 +14,6 @@ #pragma once -#include "account.h" #include "gui/owncloudguilib.h" #include "progressdispatcher.h" #include "syncresult.h" @@ -29,6 +28,8 @@ namespace OCC { namespace Wizard { class SetupWizardController; } +class AccountState; +class Account; class Folder; class AboutDialog; diff --git a/src/gui/servernotificationhandler.cpp b/src/gui/servernotificationhandler.cpp index f7896396a5c..278603b22d9 100644 --- a/src/gui/servernotificationhandler.cpp +++ b/src/gui/servernotificationhandler.cpp @@ -15,6 +15,7 @@ #include "servernotificationhandler.h" #include "accountstate.h" #include "capabilities.h" +#include "creds/abstractcredentials.h" #include "networkjobs/jsonjob.h" #include diff --git a/src/gui/socketapi/socketapi.cpp b/src/gui/socketapi/socketapi.cpp index 1fc77f46d16..1a44645e2c7 100644 --- a/src/gui/socketapi/socketapi.cpp +++ b/src/gui/socketapi/socketapi.cpp @@ -21,6 +21,7 @@ #include "gui/commonstrings.h" #include "accountmanager.h" +#include "accountstate.h" #include "common/asserts.h" #include "common/syncjournalfilerecord.h" #include "common/version.h" diff --git a/src/gui/socketapi/socketapi.h b/src/gui/socketapi/socketapi.h index 2c851e60ad0..6484b554c57 100644 --- a/src/gui/socketapi/socketapi.h +++ b/src/gui/socketapi/socketapi.h @@ -18,9 +18,11 @@ #include "common/syncfilestatus.h" #include "common/syncjournalfilerecord.h" -#include "libsync/accountfwd.h" +#include "libsync/account.h" #include "syncfileitem.h" +#include + #if defined(Q_OS_MAC) #include "socketapisocket_mac.h" #else @@ -34,6 +36,7 @@ class QLocalSocket; namespace OCC { +class AccountState; class SyncFileStatus; class Folder; class SocketListener; diff --git a/src/gui/spaces/spacesmodel.h b/src/gui/spaces/spacesmodel.h index f9b801429ae..5a5ea10f201 100644 --- a/src/gui/spaces/spacesmodel.h +++ b/src/gui/spaces/spacesmodel.h @@ -13,8 +13,6 @@ */ #pragma once -#include "accountfwd.h" - #include namespace OCC::GraphApi { diff --git a/src/libsync/abstractcorejob.cpp b/src/libsync/abstractcorejob.cpp index f627a4571f8..09180592579 100644 --- a/src/libsync/abstractcorejob.cpp +++ b/src/libsync/abstractcorejob.cpp @@ -14,6 +14,7 @@ */ #include "abstractcorejob.h" +#include "common/asserts.h" using namespace OCC; @@ -26,11 +27,6 @@ AbstractCoreJobFactory::~AbstractCoreJobFactory() { } -/*QNetworkAccessManager *AbstractCoreJobFactory::nam() const -{ - return _nam; -}*/ - void AbstractCoreJobFactory::setJobResult(CoreJob *job, const QVariant &result) { job->setResult(result); diff --git a/src/libsync/abstractnetworkjob.h b/src/libsync/abstractnetworkjob.h index c54854bc811..42f855474f2 100644 --- a/src/libsync/abstractnetworkjob.h +++ b/src/libsync/abstractnetworkjob.h @@ -15,12 +15,9 @@ #pragma once -#include "accountfwd.h" -#include "jobqueue.h" - -#include "common/asserts.h" - #include "owncloudlib.h" +#include "account.h" +#include "jobqueue.h" #include #include diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 9b4496cc28e..5f9526d4e95 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -61,7 +61,7 @@ Account::Account(const QUuid &uuid, const QString &user, const QUrl &url, QObjec , _queueGuard(&_jobQueue) , _credentialManager(new CredentialManager(this)) { - qRegisterMetaType("AccountPtr"); + qRegisterMetaType("Account"); _cacheDirectory = QStringLiteral("%1/accounts/%2").arg(commonCacheDirectory(), _uuid.toString(QUuid::WithoutBraces)); QDir().mkpath(_cacheDirectory); @@ -83,13 +83,6 @@ Account::Account(const QUuid &uuid, const QString &user, const QUrl &url, QObjec _spacesManager = new GraphApi::SpacesManager(this); } -/*AccountPtr Account::create(const QUuid &uuid, const QString &user, const QUrl &url) -{ - AccountPtr acc = AccountPtr(new Account(uuid, user, url)); - acc->setSharedThis(acc); - return acc; -}*/ - Account::~Account() { } @@ -119,12 +112,6 @@ QString Account::davPath() const return QLatin1String("/remote.php/dav/files/") + davUser() + QLatin1Char('/'); } -// todo: #20. I'm speechless -void Account::setSharedThis(AccountPtr sharedThis) -{ - _sharedThis = sharedThis.toWeakRef(); -} - CredentialManager *Account::credentialManager() const { return _credentialManager; @@ -135,12 +122,6 @@ QUuid Account::uuid() const return _uuid; } -// todo: #20 -AccountPtr Account::sharedFromThis() -{ - return _sharedThis.toStrongRef(); -} - QString Account::davUser() const { return _davUser; diff --git a/src/libsync/account.h b/src/libsync/account.h index f2c9506bd32..3b42b3e88fb 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -16,43 +16,33 @@ #ifndef SERVERCONNECTION_H #define SERVERCONNECTION_H -#include "common/utility.h" +#include "owncloudlib.h" #include "appprovider.h" #include "capabilities.h" #include "jobqueue.h" +#include #include #include #include -#include -#include #include #include -#include -#include + #include -#include -#include -#include -#include #include #include #include class QSettings; class QNetworkReply; -class QUrl; class AccessManager; namespace OCC { class CredentialManager; class AbstractCredentials; -class Account; -typedef QSharedPointer AccountPtr; class AccessManager; -class SimpleNetworkJob; namespace GraphApi { class SpacesManager; @@ -95,8 +85,6 @@ class OWNCLOUDSYNC_EXPORT Account : public QObject void cleanupForRemoval(); - AccountPtr sharedFromThis(); - /** * The unique identifier for the account. * This value is immutable after construction @@ -255,9 +243,6 @@ public Q_SLOTS: // directory all newly created accounts store their various caches in static QString _customCommonCacheDirectory; - void setSharedThis(AccountPtr sharedThis); - - QWeakPointer _sharedThis; QString _groupIndex; QUuid _uuid; QString _davUser; @@ -286,7 +271,7 @@ public Q_SLOTS: }; } -Q_DECLARE_METATYPE(OCC::AccountPtr) +Q_DECLARE_METATYPE(OCC::Account) QDebug OWNCLOUDSYNC_EXPORT operator<<(QDebug debug, const OCC::Account *job); diff --git a/src/libsync/accountfwd.h b/src/libsync/accountfwd.h deleted file mode 100644 index 4e688bf99a8..00000000000 --- a/src/libsync/accountfwd.h +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (C) by Daniel Molkentin - * - * 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 - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY - * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * for more details. - */ - -#ifndef SERVERFWD_H -#define SERVERFWD_H - -#include - -namespace OCC { - -class Account; -class AccountState; - -using AccountPtr = QSharedPointer; - - -} // namespace OCC - -#endif //SERVERFWD diff --git a/src/libsync/appprovider.h b/src/libsync/appprovider.h index e883f2c4a24..2358d9001f9 100644 --- a/src/libsync/appprovider.h +++ b/src/libsync/appprovider.h @@ -17,8 +17,6 @@ */ #pragma once -#include "accountfwd.h" -#include "capabilities.h" #include "owncloudlib.h" #include @@ -28,6 +26,9 @@ #include namespace OCC { + +class Account; + class OWNCLOUDSYNC_EXPORT AppProvider { public: diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h index edde7df1577..a27d9697d52 100644 --- a/src/libsync/creds/abstractcredentials.h +++ b/src/libsync/creds/abstractcredentials.h @@ -18,7 +18,7 @@ #include #include "accessmanager.h" -#include "accountfwd.h" +#include "account.h" #include "owncloudlib.h" #include diff --git a/src/libsync/graphapi/space.cpp b/src/libsync/graphapi/space.cpp index 264de69b820..b572edf6729 100644 --- a/src/libsync/graphapi/space.cpp +++ b/src/libsync/graphapi/space.cpp @@ -14,7 +14,6 @@ #include "space.h" -#include "libsync/account.h" #include "libsync/graphapi/spacesmanager.h" #include "libsync/networkjobs.h" #include "libsync/networkjobs/resources.h" diff --git a/src/libsync/graphapi/space.h b/src/libsync/graphapi/space.h index db01f653a39..20f63e510de 100644 --- a/src/libsync/graphapi/space.h +++ b/src/libsync/graphapi/space.h @@ -16,8 +16,6 @@ #include "owncloudlib.h" -#include "libsync/accountfwd.h" - #include #include diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index f9450b856ac..fcfda13ae30 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -15,7 +15,7 @@ #pragma once -#include "accountfwd.h" +#include "account.h" #include "common/checksums.h" #include "common/chronoelapsedtimer.h" #include "discoveryphase.h" @@ -29,7 +29,7 @@ #include #include #include -#include +#include #include #include From a900d4ca2584b7d489a3e754a2fda44c1a1b19b6 Mon Sep 17 00:00:00 2001 From: modSpike Date: Thu, 2 Oct 2025 14:59:30 +0200 Subject: [PATCH 23/23] missed a nullptr check that crashed on quit immediately after adding account --- src/gui/folder.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index 861e41f3abb..37c6b40586a 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -249,7 +249,8 @@ SyncOptions Folder::loadSyncOptions() ConfigFile cfgFile; opt._moveFilesToTrash = cfgFile.moveToTrash(); - opt._parallelNetworkJobs = _accountState->account()->isHttp2Supported() ? 20 : 6; + // got a nullptr hit here - this is so shady but best I can do for now + opt._parallelNetworkJobs = (_accountState && _accountState->account() && _accountState->account()->isHttp2Supported()) ? 20 : 6; opt.fillFromEnvironmentVariables(); return opt;