From f638c58545ee0a159cae3e91e1ccb3089945930e Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 6 May 2016 22:11:08 +0900 Subject: Add separate success() and failure() signals to BaseJob This is to facilitate processing of job results (see further commits). --- jobs/basejob.cpp | 7 +++++++ jobs/basejob.h | 13 ++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index 519e1517..e24db012 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -42,6 +42,13 @@ class BaseJob::Private BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, 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); + }); } BaseJob::~BaseJob() diff --git a/jobs/basejob.h b/jobs/basejob.h index 95cf4232..f343c769 100644 --- a/jobs/basejob.h +++ b/jobs/basejob.h @@ -46,7 +46,18 @@ namespace QMatrixClient void start() override; enum ErrorCode { NetworkError = KJob::UserDefinedError, JsonParseError, UserDefinedError }; - + + signals: + /** + * Emitted together with KJob::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). + */ + void failure(BaseJob*); + protected: ConnectionData* connection() const; -- cgit v1.2.3 From a3be7715910434cd8ef9d626b8c9c518e2d5fc40 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 6 May 2016 22:35:59 +0900 Subject: Make sure the QNetworkReply object is aborted at any failure and even silent destruction of the job --- jobs/basejob.cpp | 9 +++++++-- jobs/basejob.h | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index e24db012..bcfd5875 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -54,7 +54,11 @@ BaseJob::BaseJob(ConnectionData* connection, JobHttpType type, bool needsToken) BaseJob::~BaseJob() { if( d->reply ) + { + if( d->reply->isRunning() ) + d->reply->abort(); d->reply->deleteLater(); + } delete d; } @@ -115,6 +119,8 @@ void BaseJob::fail(int errorCode, QString errorString) { setError( errorCode ); setErrorText( errorString ); + if( d->reply->isRunning() ) + d->reply->abort(); emitResult(); } @@ -149,8 +155,7 @@ void BaseJob::gotReply() void BaseJob::timeout() { qDebug() << "Timeout!"; - if( d->reply->isRunning() ) - d->reply->abort(); + fail( TimeoutError, "The job has timed out" ); } void BaseJob::sslErrors(const QList& errors) diff --git a/jobs/basejob.h b/jobs/basejob.h index f343c769..f1ad66d1 100644 --- a/jobs/basejob.h +++ b/jobs/basejob.h @@ -45,7 +45,8 @@ namespace QMatrixClient void start() override; - enum ErrorCode { NetworkError = KJob::UserDefinedError, JsonParseError, UserDefinedError }; + enum ErrorCode { NetworkError = KJob::UserDefinedError, + JsonParseError, TimeoutError, UserDefinedError }; signals: /** -- cgit v1.2.3 From c1de050c143a79361e6b183bbfa84fc2fdaef959 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 6 May 2016 21:26:37 +0900 Subject: Enhanced logging of job failures --- jobs/basejob.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp index bcfd5875..cfdf8a28 100644 --- a/jobs/basejob.cpp +++ b/jobs/basejob.cpp @@ -121,6 +121,7 @@ void BaseJob::fail(int errorCode, QString errorString) setErrorText( errorString ); if( d->reply->isRunning() ) d->reply->abort(); + qWarning() << this << "failed:" << errorString; emitResult(); } @@ -138,7 +139,7 @@ void BaseJob::gotReply() { if( d->reply->error() != QNetworkReply::NoError ) { - qDebug() << "NetworkError!!!" << d->reply->error(); + qDebug() << "NetworkError:" << d->reply->error(); fail( NetworkError, d->reply->errorString() ); return; } -- cgit v1.2.3 From 0b7bee1e2600ee80554aa85b18d30ca1964535a0 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 6 May 2016 21:31:14 +0900 Subject: Use lambda captures instead of throwing job pointers around --- connection.cpp | 30 +++++++++++-- connectionprivate.cpp | 122 +++++++++++++++++++++++++------------------------- connectionprivate.h | 8 ++-- room.cpp | 41 +++++++++-------- user.cpp | 25 ++++------- 5 files changed, 120 insertions(+), 106 deletions(-) diff --git a/connection.cpp b/connection.cpp index e04de7aa..bf19a16e 100644 --- a/connection.cpp +++ b/connection.cpp @@ -62,7 +62,13 @@ void Connection::resolveServer(QString domain) void Connection::connectToServer(QString user, QString password) { PasswordLogin* loginJob = new PasswordLogin(d->data, user, password); - connect( loginJob, &PasswordLogin::result, d, &ConnectionPrivate::connectDone ); + connect( loginJob, &PasswordLogin::success, [=] () { + qDebug() << "Our user ID: " << d->userId; + connectWithToken(loginJob->id(), loginJob->token()); + }); + connect( loginJob, &PasswordLogin::failure, [=] () { + emit loginError(loginJob->errorString()); + }); loginJob->start(); d->username = user; // to be able to reconnect d->password = password; @@ -79,7 +85,14 @@ void Connection::connectWithToken(QString userId, QString token) void Connection::reconnect() { PasswordLogin* loginJob = new PasswordLogin(d->data, d->username, d->password ); - connect( loginJob, &PasswordLogin::result, d, &ConnectionPrivate::reconnectDone ); + connect( loginJob, &PasswordLogin::success, [=] () { + d->userId = loginJob->id(); + emit reconnected(); + }); + connect( loginJob, &PasswordLogin::failure, [=] () { + emit loginError(loginJob->errorString()); + d->isConnected = false; + }); loginJob->start(); } @@ -89,7 +102,13 @@ SyncJob* Connection::sync(int timeout) SyncJob* syncJob = new SyncJob(d->data, d->data->lastEvent()); syncJob->setFilter(filter); syncJob->setTimeout(timeout); - connect( syncJob, &SyncJob::result, d, &ConnectionPrivate::syncDone ); + connect( syncJob, &SyncJob::success, [=] () { + d->data->setLastEvent(syncJob->nextBatch()); + d->processRooms(syncJob->roomData()); + emit syncDone(); + }); + connect( syncJob, &SyncJob::failure, + [=] () { emit connectionError(syncJob->errorString());}); syncJob->start(); return syncJob; } @@ -110,7 +129,10 @@ PostReceiptJob* Connection::postReceipt(Room* room, Event* event) void Connection::joinRoom(QString roomAlias) { JoinRoomJob* job = new JoinRoomJob(d->data, roomAlias); - connect( job, &JoinRoomJob::result, d, &ConnectionPrivate::gotJoinRoom ); + connect( job, &SyncJob::success, [=] () { + if ( Room* r = d->provideRoom(job->roomId()) ) + emit joinedRoom(r); + }); job->start(); } diff --git a/connectionprivate.cpp b/connectionprivate.cpp index 851e3d58..bcca8744 100644 --- a/connectionprivate.cpp +++ b/connectionprivate.cpp @@ -116,67 +116,67 @@ Room* ConnectionPrivate::provideRoom(QString id) return room; } -void ConnectionPrivate::connectDone(KJob* job) -{ - PasswordLogin* realJob = static_cast(job); - if( !realJob->error() ) - { - isConnected = true; - userId = realJob->id(); - qDebug() << "Our user ID: " << userId; - emit q->connected(); - } - else { - emit q->loginError( job->errorString() ); - } -} - -void ConnectionPrivate::reconnectDone(KJob* job) -{ - PasswordLogin* realJob = static_cast(job); - if( !realJob->error() ) - { - userId = realJob->id(); - emit q->reconnected(); - } - else { - emit q->loginError( job->errorString() ); - isConnected = false; - } -} - -void ConnectionPrivate::syncDone(KJob* job) -{ - SyncJob* syncJob = static_cast(job); - if( !syncJob->error() ) - { - data->setLastEvent(syncJob->nextBatch()); - processRooms(syncJob->roomData()); - emit q->syncDone(); - } - else { - if( syncJob->error() == BaseJob::NetworkError ) - emit q->connectionError( syncJob->errorString() ); - else - qDebug() << "syncJob failed, error:" << syncJob->error(); - } -} - -void ConnectionPrivate::gotJoinRoom(KJob* job) -{ - qDebug() << "gotJoinRoom"; - JoinRoomJob* joinJob = static_cast(job); - if( !joinJob->error() ) - { - if ( Room* r = provideRoom(joinJob->roomId()) ) - emit q->joinedRoom(r); - } - else - { - if( joinJob->error() == BaseJob::NetworkError ) - emit q->connectionError( joinJob->errorString() ); - } -} +//void ConnectionPrivate::connectDone(KJob* job) +//{ +// PasswordLogin* realJob = static_cast(job); +// if( !realJob->error() ) +// { +// isConnected = true; +// userId = realJob->id(); +// qDebug() << "Our user ID: " << userId; +// emit q->connected(); +// } +// else { +// emit q->loginError( job->errorString() ); +// } +//} + +//void ConnectionPrivate::reconnectDone(KJob* job) +//{ +// PasswordLogin* realJob = static_cast(job); +// if( !realJob->error() ) +// { +// userId = realJob->id(); +// emit q->reconnected(); +// } +// else { +// emit q->loginError( job->errorString() ); +// isConnected = false; +// } +//} + +//void ConnectionPrivate::syncDone(KJob* job) +//{ +// SyncJob* syncJob = static_cast(job); +// if( !syncJob->error() ) +// { +// data->setLastEvent(syncJob->nextBatch()); +// processRooms(syncJob->roomData()); +// emit q->syncDone(); +// } +// else { +// if( syncJob->error() == BaseJob::NetworkError ) +// emit q->connectionError( syncJob->errorString() ); +// else +// qDebug() << "syncJob failed, error:" << syncJob->error(); +// } +//} + +//void ConnectionPrivate::gotJoinRoom(KJob* job) +//{ +// qDebug() << "gotJoinRoom"; +// JoinRoomJob* joinJob = static_cast(job); +// if( !joinJob->error() ) +// { +// if ( Room* r = provideRoom(joinJob->roomId()) ) +// emit q->joinedRoom(r); +// } +// else +// { +// if( joinJob->error() == BaseJob::NetworkError ) +// emit q->connectionError( joinJob->errorString() ); +// } +//} void ConnectionPrivate::gotRoomMembers(KJob* job) { diff --git a/connectionprivate.h b/connectionprivate.h index c538cef5..8e37a934 100644 --- a/connectionprivate.h +++ b/connectionprivate.h @@ -60,10 +60,10 @@ namespace QMatrixClient QString userId; public slots: - void connectDone(KJob* job); - void reconnectDone(KJob* job); - void syncDone(KJob* job); - void gotJoinRoom(KJob* job); +// void connectDone(KJob* job); +// void reconnectDone(KJob* job); +// void syncDone(KJob* job); +// void gotJoinRoom(KJob* job); void gotRoomMembers(KJob* job); }; } diff --git a/room.cpp b/room.cpp index ac4275af..07c94987 100644 --- a/room.cpp +++ b/room.cpp @@ -73,6 +73,7 @@ class Room::Private: public QObject QList membersLeft; QHash lastReadEvent; QString prevBatch; + RoomMessagesJob* roomMessagesJob; bool gettingNewContent; // Convenience methods to work with the membersMap and usersLeft. addMember() @@ -86,6 +87,8 @@ class Room::Private: public QObject void renameMember(User* u, QString oldName); void removeMember(User* u); + void getPreviousContent(); + private: QString calculateDisplayname() const; QString roomNameFromMemberNames(const QList& userlist) const; @@ -100,7 +103,7 @@ Room::Room(Connection* connection, QString id) d->id = id; d->connection = connection; d->joinState = JoinState::Join; - d->gettingNewContent = false; + d->roomMessagesJob = nullptr; qDebug() << "New Room:" << id; //connection->getMembers(this); // I don't think we need this anymore in r0.0.1 @@ -373,31 +376,27 @@ void Room::updateData(const SyncRoomData& data) void Room::getPreviousContent() { - if( !d->gettingNewContent ) - { - d->gettingNewContent = true; - RoomMessagesJob* job = d->connection->getMessages(this, d->prevBatch); - connect( job, &RoomMessagesJob::result, d, &Room::Private::gotMessages ); - } + d->getPreviousContent(); } -void Room::Private::gotMessages(KJob* job) +void Room::Private::getPreviousContent() { - RoomMessagesJob* realJob = static_cast(job); - if( realJob->error() ) - { - qDebug() << realJob->errorString(); - } - else + if( !roomMessagesJob ) { - for( Event* event: realJob->events() ) - { - q->processMessageEvent(event); - emit q->newMessage(event); - } - prevBatch = realJob->end(); + roomMessagesJob = connection->getMessages(q, prevBatch); + connect( roomMessagesJob, &RoomMessagesJob::result, [=]() { + if( !roomMessagesJob->error() ) + { + for( Event* event: roomMessagesJob->events() ) + { + q->processMessageEvent(event); + emit q->newMessage(event); + } + prevBatch = roomMessagesJob->end(); + } + roomMessagesJob = nullptr; + }); } - gettingNewContent = false; } Connection* Room::connection() diff --git a/user.cpp b/user.cpp index f9529db3..5018fe83 100644 --- a/user.cpp +++ b/user.cpp @@ -28,7 +28,7 @@ using namespace QMatrixClient; -class User::Private: public QObject +class User::Private { public: User* q; @@ -45,8 +45,6 @@ class User::Private: public QObject QHash,QPixmap> scaledMap; void requestAvatar(); - public slots: - void gotAvatar(KJob* job); }; User::User(QString userId, Connection* connection) @@ -135,17 +133,12 @@ void User::Private::requestAvatar() { MediaThumbnailJob* job = connection->getThumbnail(avatarUrl, requestedWidth, requestedHeight); - connect( job, &MediaThumbnailJob::result, this, &User::Private::gotAvatar ); -} - -void User::Private::gotAvatar(KJob* job) -{ - avatarOngoingRequest = false; - avatarValid = true; - avatar = - static_cast(job)->thumbnail() - .scaled(requestedWidth, requestedHeight, - Qt::KeepAspectRatio, Qt::SmoothTransformation); - scaledMap.clear(); - emit q->avatarChanged(q); + connect( job, &MediaThumbnailJob::success, [=]() { + avatarOngoingRequest = false; + avatarValid = true; + avatar = job->thumbnail().scaled(requestedWidth, requestedHeight, + Qt::KeepAspectRatio, Qt::SmoothTransformation); + scaledMap.clear(); + emit q->avatarChanged(q); + }); } -- cgit v1.2.3 From 5a0e6080a6245aa2c68f254d7105f19629a5a654 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sat, 7 May 2016 18:13:51 +0900 Subject: Minors: one fix, one cleanup --- connection.cpp | 2 +- room.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/connection.cpp b/connection.cpp index bf19a16e..a219f1da 100644 --- a/connection.cpp +++ b/connection.cpp @@ -63,7 +63,7 @@ void Connection::connectToServer(QString user, QString password) { PasswordLogin* loginJob = new PasswordLogin(d->data, user, password); connect( loginJob, &PasswordLogin::success, [=] () { - qDebug() << "Our user ID: " << d->userId; + qDebug() << "Our user ID: " << loginJob->id(); connectWithToken(loginJob->id(), loginJob->token()); }); connect( loginJob, &PasswordLogin::failure, [=] () { diff --git a/room.cpp b/room.cpp index 07c94987..81b64c54 100644 --- a/room.cpp +++ b/room.cpp @@ -74,7 +74,6 @@ class Room::Private: public QObject QHash lastReadEvent; QString prevBatch; RoomMessagesJob* roomMessagesJob; - bool gettingNewContent; // Convenience methods to work with the membersMap and usersLeft. addMember() // and removeMember() emit respective Room:: signals after a succesful -- cgit v1.2.3