From 2eb18a735a5f75a77387a211f4311222d00c2d6c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 5 May 2016 19:19:52 +0900 Subject: Rewritten BaseJob to not depend on KJob. Some parts of the code were copied from the KCoreAddons sources - surprisingly few, in fact, mostly API with comments. With this commit, libqmatrixclient doesn't depend on KCoreAddons. --- jobs/basejob.cpp | 67 +++++++++++++++++++++++++++++------- jobs/basejob.h | 102 +++++++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 139 insertions(+), 30 deletions(-) (limited to 'jobs') diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 6c68ab66..2c95ef11 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -31,24 +31,21 @@ class BaseJob::Private { public: Private(ConnectionData* c, JobHttpType t, bool nt) - : connection(c), reply(nullptr), type(t), needsToken(nt) {} + : connection(c), reply(nullptr), type(t), needsToken(nt), errorCode(NoError) + {} ConnectionData* connection; QNetworkReply* reply; JobHttpType type; bool needsToken; + + int errorCode; + QString errorText; }; BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, QString name, bool needsToken) : d(new Private(connection, type, needsToken)) { - // Work around KJob inability to separate success and failure signals - connect(this, &BaseJob::result, [this]() { - if (error() == NoError) - emit success(this); - else - emit failure(this); - }); setObjectName(name); qDebug() << "Job" << objectName() << " created"; } @@ -119,6 +116,55 @@ void BaseJob::start() // this, &BaseJob::networkError ); // http://doc.qt.io/qt-5/qnetworkreply.html#error-1 } +void BaseJob::finishJob(bool emitResult) +{ + if( d->reply->isRunning() ) + d->reply->abort(); + + // Notify those that are interested in any completion of the job (including killing) + emit finished(this); + + if (emitResult) { + emit result(this); + if (error()) + emit failure(this); + else + emit success(this); + } + + deleteLater(); +} + +int BaseJob::error() const +{ + return d->errorCode; +} + +QString BaseJob::errorString() const +{ + return d->errorText; +} + +void BaseJob::setError(int errorCode) +{ + d->errorCode = errorCode; +} + +void BaseJob::setErrorText(QString errorText) +{ + d->errorText = errorText; +} + +void BaseJob::emitResult() +{ + finishJob(true); +} + +void BaseJob::abandon() +{ + finishJob(false); +} + void BaseJob::fail(int errorCode, QString errorString) { setError( errorCode ); @@ -134,11 +180,6 @@ QNetworkReply* BaseJob::networkReply() const return d->reply; } -// void BaseJob::networkError(QNetworkReply::NetworkError code) -// { -// fail( KJob::UserDefinedError+1, d->reply->errorString() ); -// } - void BaseJob::gotReply() { switch( d->reply->error() ) diff --git a/jobs/basejob.h b/jobs/basejob.h index 150d39e8..9d00c0ac 100644 --- a/jobs/basejob.h +++ b/jobs/basejob.h @@ -19,12 +19,6 @@ #ifndef QMATRIXCLIENT_BASEJOB_H #define QMATRIXCLIENT_BASEJOB_H -#ifdef USING_SYSTEM_KCOREADDONS -#include -#else -#include "kjob.h" -#endif // KCOREADDONS_FOUND - #include #include #include @@ -36,28 +30,75 @@ namespace QMatrixClient enum class JobHttpType { GetJob, PutJob, PostJob }; - class BaseJob: public KJob + class BaseJob: public QObject { Q_OBJECT public: + /* Just in case, the values are compatible with KJob + * (which BaseJob used to inherit from). */ + enum ErrorCode { NoError = 0, NetworkError = 100, + JsonParseError, TimeoutError, ContentAccessError, + UserDefinedError = 512 }; + BaseJob(ConnectionData* connection, JobHttpType type, QString name, bool needsToken=true); virtual ~BaseJob(); - void start() override; + void start(); - enum ErrorCode { NetworkError = KJob::UserDefinedError, - JsonParseError, TimeoutError, ContentAccessError, - UserDefinedError = 512 }; + /** + * Abandons the result of this job, arrived or unarrived. + * + * This aborts waiting for a reply from the server (if there was + * any pending) and deletes the job object. It is always done quietly + * (as opposed to KJob::kill() that can trigger emitting the result). + */ + void abandon(); + + int error() const; + virtual QString errorString() const; signals: /** - * Emitted together with KJob::result() but only if there's no error. + * Emitted when the job is finished, in any case. It is used to notify + * observers that the job is terminated and that progress can be hidden. + * + * This should not be emitted directly by subclasses; + * use emitResult() instead. + * + * In general, to be notified of a job's completion, client code + * should connect to success() and failure() + * rather than finished(), so that kill() is indeed quiet. + * However if you store a list of jobs and they might get killed + * silently, then you must connect to this instead of result(), + * to avoid dangling pointers in your list. + * + * @param job the job that emitted this signal + * @internal + * + * @see success, failure + */ + void finished(BaseJob* job); + + /** + * Emitted when the job is finished (except when killed). + * + * Use error to know if the job was finished with error. + * + * @param job the job that emitted this signal + * + * @see success, failure + */ + void result(BaseJob* job); + + /** + * Emitted together with result() but only if there's no error. */ void success(BaseJob*); + /** - * Emitted together with KJob::result() if there's an error. - * Same as result(), this won't be emitted in case of kill(Quietly). + * Emitted together with result() if there's an error. + * Same as result(), this won't be emitted in case of kill(). */ void failure(BaseJob*); @@ -70,6 +111,34 @@ namespace QMatrixClient virtual QJsonObject data() const; virtual void parseJson(const QJsonDocument& data); + /** + * Sets the error code. + * + * It should be called when an error is encountered in the job, + * just before calling emitResult(). Normally you might want to + * use fail() instead - it sets error code, error text, makes sure + * the job has finished and invokes emitResult after that. + * + * To extend the list of error codes, define an (anonymous) enum + * with additional values starting at BaseJob::UserDefinedError + * + * @param errorCode the error code + * @see emitResult(), fail() + */ + void setError(int errorCode); + void setErrorText(QString errorText); + + /** + * Utility function to emit the result signal, and suicide this job. + * It first notifies the observers to hide the progress for this job using + * the finished() signal. + * + * @note: Deletes this job using deleteLater(). + * + * @see result() + * @see finished() + */ + void emitResult(); void fail( int errorCode, QString errorString ); QNetworkReply* networkReply() const; @@ -79,10 +148,9 @@ namespace QMatrixClient void timeout(); void sslErrors(const QList& errors); - //void networkError(QNetworkReply::NetworkError code); - - private: + void finishJob(bool emitResult); + class Private; Private* d; }; -- cgit v1.2.3 From 3254963c1ea587921d2a434839aa703b9aaff6cc Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 26 Jul 2016 16:49:16 +0900 Subject: Split BaseJob::gotReply into checkReply and parseReply + internal tweaks to BaseJob 1. The externally (for derived classes) visible additions are checkReply() and parseReply() virtual methods, with gotReply becoming a mere dispatcher (and therefore a private method). Splitting gotReply() in that way allowed to remove boilerplate code from MediaThumbnailJob. 2. The internal tweak is using QScopedPointer<> to store pointers both to the Private object and to a QNetworkReply (with a special deleter that aborts the reply before destructing the object). This allows to remove desperate attempts to call reply->abort() wherever it's no more needed (and not aborting the in-flight replies seems to be a/the culprit of Quaternion after-exit hangs). --- jobs/basejob.cpp | 128 +++++++++++++++++++++++++-------------------- jobs/basejob.h | 61 +++++++++++++++++---- jobs/mediathumbnailjob.cpp | 12 +---- jobs/mediathumbnailjob.h | 3 +- 4 files changed, 125 insertions(+), 79 deletions(-) (limited to 'jobs') diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 2c95ef11..9ab43087 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -19,14 +19,25 @@ #include "basejob.h" #include -#include #include +#include +#include #include #include "../connectiondata.h" using namespace QMatrixClient; +struct NetworkReplyDeleter : public QScopedPointerDeleteLater +{ + static inline void cleanup(QNetworkReply* reply) + { + if (reply && reply->isRunning()) + reply->abort(); + QScopedPointerDeleteLater::cleanup(reply); + } +}; + class BaseJob::Private { public: @@ -35,7 +46,7 @@ class BaseJob::Private {} ConnectionData* connection; - QNetworkReply* reply; + QScopedPointer reply; JobHttpType type; bool needsToken; @@ -52,13 +63,6 @@ BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, QString name, boo BaseJob::~BaseJob() { - if( d->reply ) - { - if( d->reply->isRunning() ) - d->reply->abort(); - d->reply->deleteLater(); - } - delete d; qDebug() << "Job" << objectName() << " destroyed"; } @@ -77,11 +81,6 @@ QUrlQuery BaseJob::query() const return QUrlQuery(); } -void BaseJob::parseJson(const QJsonDocument& data) -{ - emitResult(); -} - void BaseJob::start() { QUrl url = d->connection->baseUrl(); @@ -100,26 +99,75 @@ void BaseJob::start() switch( d->type ) { case JobHttpType::GetJob: - d->reply = d->connection->nam()->get(req); + d->reply.reset( d->connection->nam()->get(req) ); break; case JobHttpType::PostJob: - d->reply = d->connection->nam()->post(req, data.toJson()); + d->reply.reset( d->connection->nam()->post(req, data.toJson()) ); break; case JobHttpType::PutJob: - d->reply = d->connection->nam()->put(req, data.toJson()); + d->reply.reset( d->connection->nam()->put(req, data.toJson()) ); break; } - connect( d->reply, &QNetworkReply::sslErrors, this, &BaseJob::sslErrors ); - connect( d->reply, &QNetworkReply::finished, this, &BaseJob::gotReply ); + connect( d->reply.data(), &QNetworkReply::sslErrors, this, &BaseJob::sslErrors ); + connect( d->reply.data(), &QNetworkReply::finished, this, &BaseJob::gotReply ); QTimer::singleShot( 120*1000, this, SLOT(timeout()) ); // connect( d->reply, static_cast(&QNetworkReply::error), // this, &BaseJob::networkError ); // http://doc.qt.io/qt-5/qnetworkreply.html#error-1 } +void BaseJob::gotReply() +{ + if (checkReply(d->reply.data())) + parseReply(d->reply->readAll()); + // FIXME: we should not hold parseReply()/parseJson() responsible for + // emitting the result; it should be done here instead. +} + +bool BaseJob::checkReply(QNetworkReply* reply) +{ + switch( reply->error() ) + { + case QNetworkReply::NoError: + return true; + + case QNetworkReply::AuthenticationRequiredError: + case QNetworkReply::ContentAccessDenied: + case QNetworkReply::ContentOperationNotPermittedError: + qDebug() << "Content access error, Qt error code:" << reply->error(); + fail( ContentAccessError, reply->errorString() ); + return false; + + default: + qDebug() << "NetworkError, Qt error code:" << reply->error(); + fail( NetworkError, reply->errorString() ); + return false; + } +} + +void BaseJob::parseReply(QByteArray data) +{ + QJsonParseError error; + QJsonDocument json = QJsonDocument::fromJson(data, &error); + if( error.error == QJsonParseError::NoError ) + parseJson(json); + else + fail( JsonParseError, error.errorString() ); +} + +void BaseJob::parseJson(const QJsonDocument&) +{ + // Do nothing by default + emitResult(); +} + void BaseJob::finishJob(bool emitResult) { - if( d->reply->isRunning() ) - d->reply->abort(); + if (!d->reply) + { + qWarning() << objectName() + << ": empty network reply (finish() called more than once?)"; + return; + } // Notify those that are interested in any completion of the job (including killing) emit finished(this); @@ -132,6 +180,7 @@ void BaseJob::finishJob(bool emitResult) emit success(this); } + d->reply.reset(); deleteLater(); } @@ -169,47 +218,10 @@ void BaseJob::fail(int errorCode, QString errorString) { setError( errorCode ); setErrorText( errorString ); - if( d->reply && d->reply->isRunning() ) - d->reply->abort(); qWarning() << "Job" << objectName() << "failed:" << errorString; emitResult(); } -QNetworkReply* BaseJob::networkReply() const -{ - return d->reply; -} - -void BaseJob::gotReply() -{ - switch( d->reply->error() ) - { - case QNetworkReply::NoError: - break; // All good, go to the normal flow after the switch() - - case QNetworkReply::AuthenticationRequiredError: - case QNetworkReply::ContentAccessDenied: - case QNetworkReply::ContentOperationNotPermittedError: - qDebug() << "Content access error, Qt error code:" << d->reply->error(); - fail( ContentAccessError, d->reply->errorString() ); - return; - - default: - qDebug() << "NetworkError, Qt error code:" << d->reply->error(); - fail( NetworkError, d->reply->errorString() ); - return; - } - - QJsonParseError error; - QJsonDocument data = QJsonDocument::fromJson(d->reply->readAll(), &error); - if( error.error != QJsonParseError::NoError ) - { - fail( JsonParseError, error.errorString() ); - return; - } - parseJson(data); -} - void BaseJob::timeout() { fail( TimeoutError, "The job has timed out" ); diff --git a/jobs/basejob.h b/jobs/basejob.h index 9d00c0ac..8e17e18e 100644 --- a/jobs/basejob.h +++ b/jobs/basejob.h @@ -19,10 +19,14 @@ #ifndef QMATRIXCLIENT_BASEJOB_H #define QMATRIXCLIENT_BASEJOB_H +#include #include #include #include -#include +#include + +class QNetworkReply; +class QSslError; namespace QMatrixClient { @@ -109,7 +113,35 @@ namespace QMatrixClient virtual QString apiPath() const = 0; virtual QUrlQuery query() const; virtual QJsonObject data() const; - virtual void parseJson(const QJsonDocument& data); + + /** + * Checks the received reply for sanity; calls setError/setErrorText + * respectively. setError() with any argument except NoError prevents + * further parseReply()/parseJson() invocations. + * + * @param reply the reply received from the server + */ + virtual bool checkReply(QNetworkReply* reply); + + /** + * Processes the reply. By default, parses the reply into + * a QJsonDocument and calls parseJson() if it's a valid JSON. + * Overrides MUST ensure that fail() or emitResult() is called + * on every execution path exactly once. + * + * @param data raw contents of a HTTP reply from the server (without headers) + */ + virtual void parseReply(QByteArray data); + + /** + * Processes the JSON document received from the Matrix server. + * By default emits a successful result without analysing the JSON. + * Overrides MUST ensure that fail() or emitResult() is called + * on every execution path exactly once. + * + * @param json valid JSON document received from the server + */ + virtual void parseJson(const QJsonDocument&); /** * Sets the error code. @@ -126,12 +158,16 @@ namespace QMatrixClient * @see emitResult(), fail() */ void setError(int errorCode); + /** + * Sets the error text. Usually is combined with a setError() call + * before it, as setErrorText() alone does not indicate the error status. + */ void setErrorText(QString errorText); /** - * Utility function to emit the result signal, and suicide this job. - * It first notifies the observers to hide the progress for this job using - * the finished() signal. + * Emits the result signal, and suicides this job. + * It first notifies the observers to hide the progress for this job + * using the finished() signal. * * @note: Deletes this job using deleteLater(). * @@ -139,20 +175,27 @@ namespace QMatrixClient * @see finished() */ void emitResult(); - void fail( int errorCode, QString errorString ); - QNetworkReply* networkReply() const; + /** + * Same as emitResult() but calls setError() and setErrorText() + * with respective arguments passed to it. Use it as a shortcut to + * finish the job with a failure status. + */ + void fail( int errorCode, QString errorString ); + protected slots: - virtual void gotReply(); void timeout(); void sslErrors(const QList& errors); + private slots: + void gotReply(); + private: void finishJob(bool emitResult); class Private; - Private* d; + QScopedPointer d; }; } diff --git a/jobs/mediathumbnailjob.cpp b/jobs/mediathumbnailjob.cpp index 48ed0ffb..3f5f9ebf 100644 --- a/jobs/mediathumbnailjob.cpp +++ b/jobs/mediathumbnailjob.cpp @@ -70,17 +70,9 @@ QUrlQuery MediaThumbnailJob::query() const return query; } -void MediaThumbnailJob::gotReply() +void MediaThumbnailJob::parseReply(QByteArray data) { - if( networkReply()->error() != QNetworkReply::NoError ) - { - qDebug() << "NetworkError!!!"; - qDebug() << networkReply()->errorString(); - fail( NetworkError, networkReply()->errorString() ); - return; - } - - if( !d->thumbnail.loadFromData( networkReply()->readAll() ) ) + if( !d->thumbnail.loadFromData(data) ) { qDebug() << "MediaThumbnailJob: could not read image data"; } diff --git a/jobs/mediathumbnailjob.h b/jobs/mediathumbnailjob.h index 541163ed..1af8ecd4 100644 --- a/jobs/mediathumbnailjob.h +++ b/jobs/mediathumbnailjob.h @@ -40,8 +40,7 @@ namespace QMatrixClient QString apiPath() const override; QUrlQuery query() const override; - protected slots: - void gotReply() override; + virtual void parseReply(QByteArray data) override; private: class Private; -- cgit v1.2.3 From 844d3362022d0278b07e1232460c1662e4fb61d2 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 27 Jul 2016 14:54:43 +0900 Subject: Introduce Status class + BaseJob::{checkReply,parseReply,parseJson} now return it This better fixes the contract for derived job classes and simplifies error reporting. Methods error() and errorString() are kept for back-compatibility; status() returns a combination of them, conveniently packed into a Status object. For a quick status check, Status::good() is provided. --- jobs/basejob.cpp | 76 ++++++++++++++++------------------ jobs/basejob.h | 101 +++++++++++++++++++++------------------------ jobs/checkauthmethods.cpp | 2 +- jobs/checkauthmethods.h | 2 +- jobs/joinroomjob.cpp | 15 +++---- jobs/joinroomjob.h | 2 +- jobs/mediathumbnailjob.cpp | 4 +- jobs/mediathumbnailjob.h | 2 +- jobs/passwordlogin.cpp | 6 +-- jobs/passwordlogin.h | 2 +- jobs/postmessagejob.cpp | 15 +++---- jobs/postmessagejob.h | 2 +- jobs/roommembersjob.cpp | 7 ++-- jobs/roommembersjob.h | 4 +- jobs/roommessagesjob.cpp | 4 +- jobs/roommessagesjob.h | 2 +- jobs/syncjob.cpp | 5 +-- jobs/syncjob.h | 2 +- 18 files changed, 118 insertions(+), 135 deletions(-) (limited to 'jobs') diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 9ab43087..3a0e1d3f 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -42,16 +42,16 @@ class BaseJob::Private { public: Private(ConnectionData* c, JobHttpType t, bool nt) - : connection(c), reply(nullptr), type(t), needsToken(nt), errorCode(NoError) + : connection(c), type(t), needsToken(nt) + , reply(nullptr), status(NoError) {} ConnectionData* connection; - QScopedPointer reply; JobHttpType type; bool needsToken; - int errorCode; - QString errorText; + QScopedPointer reply; + Status status; }; BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, QString name, bool needsToken) @@ -117,47 +117,45 @@ void BaseJob::start() void BaseJob::gotReply() { - if (checkReply(d->reply.data())) - parseReply(d->reply->readAll()); - // FIXME: we should not hold parseReply()/parseJson() responsible for - // emitting the result; it should be done here instead. + setStatus(checkReply(d->reply.data())); + if (status().good()) + setStatus(parseReply(d->reply->readAll())); + + finishJob(true); } -bool BaseJob::checkReply(QNetworkReply* reply) +BaseJob::Status BaseJob::checkReply(QNetworkReply* reply) const { switch( reply->error() ) { case QNetworkReply::NoError: - return true; + return NoError; case QNetworkReply::AuthenticationRequiredError: case QNetworkReply::ContentAccessDenied: case QNetworkReply::ContentOperationNotPermittedError: qDebug() << "Content access error, Qt error code:" << reply->error(); - fail( ContentAccessError, reply->errorString() ); - return false; + return { ContentAccessError, reply->errorString() }; default: qDebug() << "NetworkError, Qt error code:" << reply->error(); - fail( NetworkError, reply->errorString() ); - return false; + return { NetworkError, reply->errorString() }; } } -void BaseJob::parseReply(QByteArray data) +BaseJob::Status BaseJob::parseReply(QByteArray data) { QJsonParseError error; QJsonDocument json = QJsonDocument::fromJson(data, &error); if( error.error == QJsonParseError::NoError ) - parseJson(json); + return parseJson(json); else - fail( JsonParseError, error.errorString() ); + return { JsonParseError, error.errorString() }; } -void BaseJob::parseJson(const QJsonDocument&) +BaseJob::Status BaseJob::parseJson(const QJsonDocument&) { - // Do nothing by default - emitResult(); + return Success; } void BaseJob::finishJob(bool emitResult) @@ -165,7 +163,7 @@ void BaseJob::finishJob(bool emitResult) if (!d->reply) { qWarning() << objectName() - << ": empty network reply (finish() called more than once?)"; + << ": empty network reply (finishJob() called more than once?)"; return; } @@ -184,29 +182,34 @@ void BaseJob::finishJob(bool emitResult) deleteLater(); } -int BaseJob::error() const +BaseJob::Status BaseJob::status() const { - return d->errorCode; + return d->status; } -QString BaseJob::errorString() const +int BaseJob::error() const { - return d->errorText; + return d->status.code; } -void BaseJob::setError(int errorCode) +QString BaseJob::errorString() const { - d->errorCode = errorCode; + return d->status.message; } -void BaseJob::setErrorText(QString errorText) +void BaseJob::setStatus(Status s) { - d->errorText = errorText; + d->status = s; + if (!s.good()) + { + qWarning() << QString("Job %1 status: %2, code %3") + .arg(objectName()).arg(s.message).arg(s.code); + } } -void BaseJob::emitResult() +void BaseJob::setStatus(int code, QString message) { - finishJob(true); + setStatus({ code, message }); } void BaseJob::abandon() @@ -214,17 +217,10 @@ void BaseJob::abandon() finishJob(false); } -void BaseJob::fail(int errorCode, QString errorString) -{ - setError( errorCode ); - setErrorText( errorString ); - qWarning() << "Job" << objectName() << "failed:" << errorString; - emitResult(); -} - void BaseJob::timeout() { - fail( TimeoutError, "The job has timed out" ); + setStatus( TimeoutError, "The job has timed out" ); + finishJob(true); } void BaseJob::sslErrors(const QList& errors) diff --git a/jobs/basejob.h b/jobs/basejob.h index 8e17e18e..07b1f1dd 100644 --- a/jobs/basejob.h +++ b/jobs/basejob.h @@ -40,10 +40,38 @@ namespace QMatrixClient public: /* Just in case, the values are compatible with KJob * (which BaseJob used to inherit from). */ - enum ErrorCode { NoError = 0, NetworkError = 100, - JsonParseError, TimeoutError, ContentAccessError, - UserDefinedError = 512 }; + enum StatusCode { NoError = 0 // To be compatible with Qt conventions + , Success = 0 + , ErrorLevel = 100 // Errors have codes starting from this + , NetworkError = 100 + , JsonParseError + , TimeoutError + , ContentAccessError + , UserDefinedError = 200 + }; + /** + * This structure stores the status of a server call job. The status consists + * of a code, that is described (but not delimited) by the respective enum, + * and a freeform message. + * + * To extend the list of error codes, define an (anonymous) enum + * along the lines of StatusCode, with additional values + * starting at UserDefinedError + */ + class Status + { + public: + Status(StatusCode c) : code(c) { } + Status(int c, QString m) : code(c), message(m) { } + + bool good() const { return code < ErrorLevel; } + + int code; + QString message; + }; + + public: BaseJob(ConnectionData* connection, JobHttpType type, QString name, bool needsToken=true); virtual ~BaseJob(); @@ -59,6 +87,7 @@ namespace QMatrixClient */ void abandon(); + Status status() const; int error() const; virtual QString errorString() const; @@ -115,74 +144,40 @@ namespace QMatrixClient virtual QJsonObject data() const; /** - * Checks the received reply for sanity; calls setError/setErrorText - * respectively. setError() with any argument except NoError prevents - * further parseReply()/parseJson() invocations. + * Used by gotReply() slot to check the received reply for general + * issues such as network errors or access denial. + * Returning anything except NoError/Success prevents + * further parseReply()/parseJson() invocation. * * @param reply the reply received from the server + * @return the result of checking the reply + * + * @see gotReply */ - virtual bool checkReply(QNetworkReply* reply); + virtual Status checkReply(QNetworkReply* reply) const; /** * Processes the reply. By default, parses the reply into * a QJsonDocument and calls parseJson() if it's a valid JSON. - * Overrides MUST ensure that fail() or emitResult() is called - * on every execution path exactly once. * * @param data raw contents of a HTTP reply from the server (without headers) + * + * @see gotReply, parseJson */ - virtual void parseReply(QByteArray data); + virtual Status parseReply(QByteArray data); /** * Processes the JSON document received from the Matrix server. - * By default emits a successful result without analysing the JSON. - * Overrides MUST ensure that fail() or emitResult() is called - * on every execution path exactly once. + * By default returns succesful status without analysing the JSON. * * @param json valid JSON document received from the server - */ - virtual void parseJson(const QJsonDocument&); - - /** - * Sets the error code. * - * It should be called when an error is encountered in the job, - * just before calling emitResult(). Normally you might want to - * use fail() instead - it sets error code, error text, makes sure - * the job has finished and invokes emitResult after that. - * - * To extend the list of error codes, define an (anonymous) enum - * with additional values starting at BaseJob::UserDefinedError - * - * @param errorCode the error code - * @see emitResult(), fail() - */ - void setError(int errorCode); - /** - * Sets the error text. Usually is combined with a setError() call - * before it, as setErrorText() alone does not indicate the error status. - */ - void setErrorText(QString errorText); - - /** - * Emits the result signal, and suicides this job. - * It first notifies the observers to hide the progress for this job - * using the finished() signal. - * - * @note: Deletes this job using deleteLater(). - * - * @see result() - * @see finished() - */ - void emitResult(); - - /** - * Same as emitResult() but calls setError() and setErrorText() - * with respective arguments passed to it. Use it as a shortcut to - * finish the job with a failure status. + * @see parseReply */ - void fail( int errorCode, QString errorString ); + virtual Status parseJson(const QJsonDocument&); + void setStatus(Status s); + void setStatus(int code, QString message); protected slots: void timeout(); diff --git a/jobs/checkauthmethods.cpp b/jobs/checkauthmethods.cpp index 55d8632a..f471c135 100644 --- a/jobs/checkauthmethods.cpp +++ b/jobs/checkauthmethods.cpp @@ -57,7 +57,7 @@ QString CheckAuthMethods::apiPath() const return "_matrix/client/r0/login"; } -void CheckAuthMethods::parseJson(const QJsonDocument& data) +BaseJob::Status CheckAuthMethods::parseJson(const QJsonDocument& data) { // TODO } diff --git a/jobs/checkauthmethods.h b/jobs/checkauthmethods.h index 2951e27f..36eaa01a 100644 --- a/jobs/checkauthmethods.h +++ b/jobs/checkauthmethods.h @@ -35,7 +35,7 @@ namespace QMatrixClient protected: QString apiPath() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/joinroomjob.cpp b/jobs/joinroomjob.cpp index 799d4926..8848aa70 100644 --- a/jobs/joinroomjob.cpp +++ b/jobs/joinroomjob.cpp @@ -54,18 +54,15 @@ QString JoinRoomJob::apiPath() const return QString("_matrix/client/r0/join/%1").arg(d->roomAlias); } -void JoinRoomJob::parseJson(const QJsonDocument& data) +BaseJob::Status JoinRoomJob::parseJson(const QJsonDocument& data) { QJsonObject json = data.object(); - if( !json.contains("room_id") ) - { - fail( BaseJob::UserDefinedError, "Something went wrong..." ); - qDebug() << data; - return; - } - else + if( json.contains("room_id") ) { d->roomId = json.value("room_id").toString(); + return Success; } - emitResult(); + + qDebug() << data; + return { UserDefinedError, "No room_id in the JSON response" }; } diff --git a/jobs/joinroomjob.h b/jobs/joinroomjob.h index 1329ca56..a6f4af21 100644 --- a/jobs/joinroomjob.h +++ b/jobs/joinroomjob.h @@ -35,7 +35,7 @@ namespace QMatrixClient protected: QString apiPath() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/mediathumbnailjob.cpp b/jobs/mediathumbnailjob.cpp index 3f5f9ebf..1e434fbc 100644 --- a/jobs/mediathumbnailjob.cpp +++ b/jobs/mediathumbnailjob.cpp @@ -70,11 +70,11 @@ QUrlQuery MediaThumbnailJob::query() const return query; } -void MediaThumbnailJob::parseReply(QByteArray data) +BaseJob::Status MediaThumbnailJob::parseReply(QByteArray data) { if( !d->thumbnail.loadFromData(data) ) { qDebug() << "MediaThumbnailJob: could not read image data"; } - emitResult(); + return Success; } diff --git a/jobs/mediathumbnailjob.h b/jobs/mediathumbnailjob.h index 1af8ecd4..3babf845 100644 --- a/jobs/mediathumbnailjob.h +++ b/jobs/mediathumbnailjob.h @@ -40,7 +40,7 @@ namespace QMatrixClient QString apiPath() const override; QUrlQuery query() const override; - virtual void parseReply(QByteArray data) override; + Status parseReply(QByteArray data) override; private: class Private; diff --git a/jobs/passwordlogin.cpp b/jobs/passwordlogin.cpp index 231dcce5..c85e4c13 100644 --- a/jobs/passwordlogin.cpp +++ b/jobs/passwordlogin.cpp @@ -80,15 +80,15 @@ QJsonObject PasswordLogin::data() const return json; } -void PasswordLogin::parseJson(const QJsonDocument& data) +BaseJob::Status PasswordLogin::parseJson(const QJsonDocument& data) { QJsonObject json = data.object(); if( !json.contains("access_token") || !json.contains("home_server") || !json.contains("user_id") ) { - fail( BaseJob::UserDefinedError, "Unexpected data" ); + return { UserDefinedError, "No expected data" }; } d->returned_token = json.value("access_token").toString(); d->returned_server = json.value("home_server").toString(); d->returned_id = json.value("user_id").toString(); - emitResult(); + return Success; } diff --git a/jobs/passwordlogin.h b/jobs/passwordlogin.h index 75a45cae..d7e42725 100644 --- a/jobs/passwordlogin.h +++ b/jobs/passwordlogin.h @@ -38,7 +38,7 @@ namespace QMatrixClient protected: QString apiPath() const override; QJsonObject data() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/postmessagejob.cpp b/jobs/postmessagejob.cpp index cf9b94fd..0a38da62 100644 --- a/jobs/postmessagejob.cpp +++ b/jobs/postmessagejob.cpp @@ -61,14 +61,11 @@ QJsonObject PostMessageJob::data() const return json; } -void PostMessageJob::parseJson(const QJsonDocument& data) +BaseJob::Status PostMessageJob::parseJson(const QJsonDocument& data) { - QJsonObject json = data.object(); - if( !json.contains("event_id") ) - { - fail( BaseJob::UserDefinedError, "Something went wrong..." ); - qDebug() << data; - return; - } - emitResult(); + if( data.object().contains("event_id") ) + return Success; + + qDebug() << data; + return { UserDefinedError, "No event_id in the JSON response" }; } diff --git a/jobs/postmessagejob.h b/jobs/postmessagejob.h index 9d354240..73d72020 100644 --- a/jobs/postmessagejob.h +++ b/jobs/postmessagejob.h @@ -35,7 +35,7 @@ namespace QMatrixClient protected: QString apiPath() const override; QJsonObject data() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/roommembersjob.cpp b/jobs/roommembersjob.cpp index 002be75b..7fc44c63 100644 --- a/jobs/roommembersjob.cpp +++ b/jobs/roommembersjob.cpp @@ -56,10 +56,9 @@ QString RoomMembersJob::apiPath() const return QString("_matrix/client/r0/rooms/%1/members").arg(d->room->id()); } -void RoomMembersJob::parseJson(const QJsonDocument& data) +BaseJob::Status RoomMembersJob::parseJson(const QJsonDocument& data) { - QJsonObject obj = data.object(); - QJsonArray chunk = obj.value("chunk").toArray(); + QJsonArray chunk = data.object().value("chunk").toArray(); for( const QJsonValue& val : chunk ) { State* state = State::fromJson(val.toObject()); @@ -67,5 +66,5 @@ void RoomMembersJob::parseJson(const QJsonDocument& data) d->states.append(state); } qDebug() << "States: " << d->states.count(); - emitResult(); + return Success; } diff --git a/jobs/roommembersjob.h b/jobs/roommembersjob.h index ffae4309..04803d67 100644 --- a/jobs/roommembersjob.h +++ b/jobs/roommembersjob.h @@ -35,8 +35,8 @@ namespace QMatrixClient QList states(); protected: - virtual QString apiPath() const override; - virtual void parseJson(const QJsonDocument& data) override; + QString apiPath() const override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/roommessagesjob.cpp b/jobs/roommessagesjob.cpp index f1943d2c..ba075007 100644 --- a/jobs/roommessagesjob.cpp +++ b/jobs/roommessagesjob.cpp @@ -81,10 +81,10 @@ QUrlQuery RoomMessagesJob::query() const return query; } -void RoomMessagesJob::parseJson(const QJsonDocument& data) +BaseJob::Status RoomMessagesJob::parseJson(const QJsonDocument& data) { QJsonObject obj = data.object(); d->events = eventListFromJson(obj.value("chunk").toArray()); d->end = obj.value("end").toString(); - emitResult(); + return Success; } diff --git a/jobs/roommessagesjob.h b/jobs/roommessagesjob.h index c068f235..52a72f70 100644 --- a/jobs/roommessagesjob.h +++ b/jobs/roommessagesjob.h @@ -40,7 +40,7 @@ namespace QMatrixClient protected: QString apiPath() const override; QUrlQuery query() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; diff --git a/jobs/syncjob.cpp b/jobs/syncjob.cpp index 3a7573bc..3bd1f0c8 100644 --- a/jobs/syncjob.cpp +++ b/jobs/syncjob.cpp @@ -110,7 +110,7 @@ QUrlQuery SyncJob::query() const return query; } -void SyncJob::parseJson(const QJsonDocument& data) +BaseJob::Status SyncJob::parseJson(const QJsonDocument& data) { QJsonObject json = data.object(); d->nextBatch = json.value("next_batch").toString(); @@ -134,8 +134,7 @@ void SyncJob::parseJson(const QJsonDocument& data) } } - emitResult(); - qDebug() << objectName() << ": processing complete"; + return Success; } void SyncRoomData::EventList::fromJson(const QJsonObject& roomContents) diff --git a/jobs/syncjob.h b/jobs/syncjob.h index 95d7aacb..51d07944 100644 --- a/jobs/syncjob.h +++ b/jobs/syncjob.h @@ -73,7 +73,7 @@ namespace QMatrixClient protected: QString apiPath() const override; QUrlQuery query() const override; - void parseJson(const QJsonDocument& data) override; + Status parseJson(const QJsonDocument& data) override; private: class Private; -- cgit v1.2.3 From f6c623a27bcb5ec2fcc83930e500afb597a32a46 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Thu, 28 Jul 2016 18:54:45 +0900 Subject: Fixed double-emission of signal(s) on job timeout + general cleanup --- jobs/basejob.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) (limited to 'jobs') diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 3a0e1d3f..e0dff287 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -54,16 +54,21 @@ class BaseJob::Private Status status; }; +inline QDebug operator<<(QDebug dbg, BaseJob* j) +{ + return dbg << "Job" << j->objectName(); +} + BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, QString name, bool needsToken) : d(new Private(connection, type, needsToken)) { setObjectName(name); - qDebug() << "Job" << objectName() << " created"; + qDebug() << this << "created"; } BaseJob::~BaseJob() { - qDebug() << "Job" << objectName() << " destroyed"; + qDebug() << this << "destroyed"; } ConnectionData* BaseJob::connection() const @@ -134,11 +139,9 @@ BaseJob::Status BaseJob::checkReply(QNetworkReply* reply) const case QNetworkReply::AuthenticationRequiredError: case QNetworkReply::ContentAccessDenied: case QNetworkReply::ContentOperationNotPermittedError: - qDebug() << "Content access error, Qt error code:" << reply->error(); return { ContentAccessError, reply->errorString() }; default: - qDebug() << "NetworkError, Qt error code:" << reply->error(); return { NetworkError, reply->errorString() }; } } @@ -162,9 +165,12 @@ void BaseJob::finishJob(bool emitResult) { if (!d->reply) { - qWarning() << objectName() - << ": empty network reply (finishJob() called more than once?)"; - return; + qWarning() << this << "finishes with empty network reply"; + } + else if (d->reply->isRunning()) + { + qWarning() << this << "finishes without ready network reply"; + d->reply->disconnect(this); // Ignore whatever comes from the reply } // Notify those that are interested in any completion of the job (including killing) @@ -178,7 +184,6 @@ void BaseJob::finishJob(bool emitResult) emit success(this); } - d->reply.reset(); deleteLater(); } @@ -202,8 +207,7 @@ void BaseJob::setStatus(Status s) d->status = s; if (!s.good()) { - qWarning() << QString("Job %1 status: %2, code %3") - .arg(objectName()).arg(s.message).arg(s.code); + qWarning() << this << "status" << s.code << ":" << s.message; } } -- cgit v1.2.3