From 67c748d62aeea43f75d27f320a3b071be3968183 Mon Sep 17 00:00:00 2001 From: Dean Lee Date: Mon, 29 Nov 2021 18:19:08 +0800 Subject: [PATCH] refactor httprequest: emit single signals (#23039) * refactor httprequest * Trigger Build old-commit-hash: 3fd02649601856b2780168d36c49940ff49f4de2 --- selfdrive/ui/qt/api.cc | 35 +++++++++++++------------- selfdrive/ui/qt/api.h | 12 ++++----- selfdrive/ui/qt/maps/map_settings.cc | 18 ++++++------- selfdrive/ui/qt/maps/map_settings.h | 3 +-- selfdrive/ui/qt/request_repeater.cc | 6 ++--- selfdrive/ui/qt/setup/setup.cc | 2 +- selfdrive/ui/qt/widgets/drive_stats.cc | 6 +++-- selfdrive/ui/qt/widgets/drive_stats.h | 2 +- selfdrive/ui/qt/widgets/prime.cc | 8 +++--- selfdrive/ui/qt/widgets/prime.h | 2 +- selfdrive/ui/qt/widgets/ssh_keys.cc | 29 +++++++++++---------- selfdrive/ui/replay/route.cc | 6 ++--- 12 files changed, 63 insertions(+), 66 deletions(-) diff --git a/selfdrive/ui/qt/api.cc b/selfdrive/ui/qt/api.cc index 6339884aec..98236e5014 100644 --- a/selfdrive/ui/qt/api.cc +++ b/selfdrive/ui/qt/api.cc @@ -70,10 +70,14 @@ HttpRequest::HttpRequest(QObject *parent, bool create_jwt, int timeout) : create connect(networkTimer, &QTimer::timeout, this, &HttpRequest::requestTimeout); } -bool HttpRequest::active() { +bool HttpRequest::active() const { return reply != nullptr; } +bool HttpRequest::timeout() const { + return reply && reply->error() == QNetworkReply::OperationCanceledError; +} + void HttpRequest::sendRequest(const QString &requestURL, const HttpRequest::Method method) { if (active()) { qDebug() << "HttpRequest is active"; @@ -110,29 +114,26 @@ void HttpRequest::requestTimeout() { reply->abort(); } -// This function should always emit something void HttpRequest::requestFinished() { - bool success = false; - if (reply->error() != QNetworkReply::OperationCanceledError) { - networkTimer->stop(); - QString response = reply->readAll(); - - if (reply->error() == QNetworkReply::NoError) { - success = true; - emit receivedResponse(response); - } else { - emit failedResponse(reply->errorString()); + networkTimer->stop(); + if (reply->error() == QNetworkReply::NoError) { + emit requestDone(reply->readAll(), true); + } else { + QString error; + if (reply->error() == QNetworkReply::OperationCanceledError) { + networkAccessManager->clearAccessCache(); + networkAccessManager->clearConnectionCache(); + error = "Request timed out"; + } else { if (reply->error() == QNetworkReply::ContentAccessDenied || reply->error() == QNetworkReply::AuthenticationRequiredError) { qWarning() << ">> Unauthorized. Authenticate with tools/lib/auth.py <<"; } + error = reply->errorString(); } - } else { - networkAccessManager->clearAccessCache(); - networkAccessManager->clearConnectionCache(); - emit timeoutResponse("timeout"); + emit requestDone(error, false); } - emit requestDone(success); + reply->deleteLater(); reply = nullptr; } diff --git a/selfdrive/ui/qt/api.h b/selfdrive/ui/qt/api.h index 1c7c333ea0..6890a7957d 100644 --- a/selfdrive/ui/qt/api.h +++ b/selfdrive/ui/qt/api.h @@ -27,7 +27,11 @@ public: explicit HttpRequest(QObject* parent, bool create_jwt = true, int timeout = 20000); void sendRequest(const QString &requestURL, const Method method = Method::GET); - bool active(); + bool active() const; + bool timeout() const; + +signals: + void requestDone(const QString &response, bool success); protected: QNetworkReply *reply = nullptr; @@ -40,10 +44,4 @@ private: private slots: void requestTimeout(); void requestFinished(); - -signals: - void requestDone(bool success); - void receivedResponse(const QString &response); - void failedResponse(const QString &errorString); - void timeoutResponse(const QString &errorString); }; diff --git a/selfdrive/ui/qt/maps/map_settings.cc b/selfdrive/ui/qt/maps/map_settings.cc index 60a7812265..e130a9a1ec 100644 --- a/selfdrive/ui/qt/maps/map_settings.cc +++ b/selfdrive/ui/qt/maps/map_settings.cc @@ -127,8 +127,7 @@ MapPanel::MapPanel(QWidget* parent) : QWidget(parent) { { QString url = CommaApi::BASE_URL + "/v1/navigation/" + *dongle_id + "/locations"; RequestRepeater* repeater = new RequestRepeater(this, url, "ApiCache_NavDestinations", 30, true); - QObject::connect(repeater, &RequestRepeater::receivedResponse, this, &MapPanel::parseResponse); - QObject::connect(repeater, &RequestRepeater::failedResponse, this, &MapPanel::failedResponse); + QObject::connect(repeater, &RequestRepeater::requestDone, this, &MapPanel::parseResponse); } // Destination set while offline @@ -137,8 +136,8 @@ MapPanel::MapPanel(QWidget* parent) : QWidget(parent) { RequestRepeater* repeater = new RequestRepeater(this, url, "", 10, true); HttpRequest* deleter = new HttpRequest(this); - QObject::connect(repeater, &RequestRepeater::receivedResponse, [=](QString resp) { - if (resp != "null") { + QObject::connect(repeater, &RequestRepeater::requestDone, [=](const QString &resp, bool success) { + if (success && resp != "null") { if (params.get("NavDestination").empty()) { qWarning() << "Setting NavDestination from /next" << resp; params.put("NavDestination", resp.toStdString()); @@ -183,7 +182,12 @@ void MapPanel::updateCurrentRoute() { current_widget->setVisible(dest.size() && !doc.isNull()); } -void MapPanel::parseResponse(const QString &response) { +void MapPanel::parseResponse(const QString &response, bool success) { + if (!success) { + stack->setCurrentIndex(1); + return; + } + QJsonDocument doc = QJsonDocument::fromJson(response.trimmed().toUtf8()); if (doc.isNull()) { qDebug() << "JSON Parse failed on navigation locations"; @@ -283,10 +287,6 @@ void MapPanel::parseResponse(const QString &response) { repaint(); } -void MapPanel::failedResponse(const QString &response) { - stack->setCurrentIndex(1); -} - void MapPanel::navigateTo(const QJsonObject &place) { QJsonDocument doc(place); params.put("NavDestination", doc.toJson().toStdString()); diff --git a/selfdrive/ui/qt/maps/map_settings.h b/selfdrive/ui/qt/maps/map_settings.h index ec409c4962..03720edee7 100644 --- a/selfdrive/ui/qt/maps/map_settings.h +++ b/selfdrive/ui/qt/maps/map_settings.h @@ -17,8 +17,7 @@ public: explicit MapPanel(QWidget* parent = nullptr); void navigateTo(const QJsonObject &place); - void parseResponse(const QString &response); - void failedResponse(const QString &response); + void parseResponse(const QString &response, bool success); void updateCurrentRoute(); void clear(); diff --git a/selfdrive/ui/qt/request_repeater.cc b/selfdrive/ui/qt/request_repeater.cc index c11698d6f1..d2c0f9bc30 100644 --- a/selfdrive/ui/qt/request_repeater.cc +++ b/selfdrive/ui/qt/request_repeater.cc @@ -15,10 +15,10 @@ RequestRepeater::RequestRepeater(QObject *parent, const QString &requestURL, con if (!cacheKey.isEmpty()) { prevResp = QString::fromStdString(params.get(cacheKey.toStdString())); if (!prevResp.isEmpty()) { - QTimer::singleShot(500, [=]() { emit receivedResponse(prevResp); }); + QTimer::singleShot(500, [=]() { emit requestDone(prevResp, true); }); } - QObject::connect(this, &HttpRequest::receivedResponse, [=](const QString &resp) { - if (resp != prevResp) { + QObject::connect(this, &HttpRequest::requestDone, [=](const QString &resp, bool success) { + if (success && resp != prevResp) { params.put(cacheKey.toStdString(), resp.toStdString()); prevResp = resp; } diff --git a/selfdrive/ui/qt/setup/setup.cc b/selfdrive/ui/qt/setup/setup.cc index a855813688..bd494327cc 100644 --- a/selfdrive/ui/qt/setup/setup.cc +++ b/selfdrive/ui/qt/setup/setup.cc @@ -169,7 +169,7 @@ QWidget * Setup::network_setup() { // setup timer for testing internet connection HttpRequest *request = new HttpRequest(this, false, 2500); - QObject::connect(request, &HttpRequest::requestDone, [=](bool success) { + QObject::connect(request, &HttpRequest::requestDone, [=](const QString &, bool success) { cont->setEnabled(success); if (success) { const bool cell = networking->wifi->currentNetworkType() == NetworkType::CELL; diff --git a/selfdrive/ui/qt/widgets/drive_stats.cc b/selfdrive/ui/qt/widgets/drive_stats.cc index bd1dc71766..f4c8f502a3 100644 --- a/selfdrive/ui/qt/widgets/drive_stats.cc +++ b/selfdrive/ui/qt/widgets/drive_stats.cc @@ -48,7 +48,7 @@ DriveStats::DriveStats(QWidget* parent) : QFrame(parent) { if (auto dongleId = getDongleId()) { QString url = CommaApi::BASE_URL + "/v1.1/devices/" + *dongleId + "/stats"; RequestRepeater* repeater = new RequestRepeater(this, url, "ApiCache_DriveStats", 30); - QObject::connect(repeater, &RequestRepeater::receivedResponse, this, &DriveStats::parseResponse); + QObject::connect(repeater, &RequestRepeater::requestDone, this, &DriveStats::parseResponse); } setStyleSheet(R"( @@ -76,7 +76,9 @@ void DriveStats::updateStats() { update(json["week"].toObject(), week_); } -void DriveStats::parseResponse(const QString& response) { +void DriveStats::parseResponse(const QString& response, bool success) { + if (!success) return; + QJsonDocument doc = QJsonDocument::fromJson(response.trimmed().toUtf8()); if (doc.isNull()) { qDebug() << "JSON Parse failed on getting past drives statistics"; diff --git a/selfdrive/ui/qt/widgets/drive_stats.h b/selfdrive/ui/qt/widgets/drive_stats.h index 40ecbdeaf2..9446294516 100644 --- a/selfdrive/ui/qt/widgets/drive_stats.h +++ b/selfdrive/ui/qt/widgets/drive_stats.h @@ -21,5 +21,5 @@ private: } all_, week_; private slots: - void parseResponse(const QString &response); + void parseResponse(const QString &response, bool success); }; diff --git a/selfdrive/ui/qt/widgets/prime.cc b/selfdrive/ui/qt/widgets/prime.cc index 665466df5e..ab6dc67d36 100644 --- a/selfdrive/ui/qt/widgets/prime.cc +++ b/selfdrive/ui/qt/widgets/prime.cc @@ -161,7 +161,7 @@ PrimeUserWidget::PrimeUserWidget(QWidget* parent) : QWidget(parent) { if (auto dongleId = getDongleId()) { QString url = CommaApi::BASE_URL + "/v1/devices/" + *dongleId + "/owner"; RequestRepeater *repeater = new RequestRepeater(this, url, "ApiCache_Owner", 6); - QObject::connect(repeater, &RequestRepeater::receivedResponse, this, &PrimeUserWidget::replyFinished); + QObject::connect(repeater, &RequestRepeater::requestDone, this, &PrimeUserWidget::replyFinished); } } @@ -291,14 +291,14 @@ SetupWidget::SetupWidget(QWidget* parent) : QFrame(parent) { QString url = CommaApi::BASE_URL + "/v1.1/devices/" + *dongleId + "/"; RequestRepeater* repeater = new RequestRepeater(this, url, "ApiCache_Device", 5); - QObject::connect(repeater, &RequestRepeater::failedResponse, this, &SetupWidget::show); - QObject::connect(repeater, &RequestRepeater::receivedResponse, this, &SetupWidget::replyFinished); + QObject::connect(repeater, &RequestRepeater::requestDone, this, &SetupWidget::replyFinished); } hide(); // Only show when first request comes back } -void SetupWidget::replyFinished(const QString &response) { +void SetupWidget::replyFinished(const QString &response, bool success) { show(); + if (!success) return; QJsonDocument doc = QJsonDocument::fromJson(response.toUtf8()); if (doc.isNull()) { diff --git a/selfdrive/ui/qt/widgets/prime.h b/selfdrive/ui/qt/widgets/prime.h index 6044f06104..f7470fe441 100644 --- a/selfdrive/ui/qt/widgets/prime.h +++ b/selfdrive/ui/qt/widgets/prime.h @@ -68,5 +68,5 @@ private: PrimeUserWidget *primeUser; private slots: - void replyFinished(const QString &response); + void replyFinished(const QString &response, bool success); }; diff --git a/selfdrive/ui/qt/widgets/ssh_keys.cc b/selfdrive/ui/qt/widgets/ssh_keys.cc index ca360a3054..7630e65731 100644 --- a/selfdrive/ui/qt/widgets/ssh_keys.cc +++ b/selfdrive/ui/qt/widgets/ssh_keys.cc @@ -41,23 +41,22 @@ void SshControl::refresh() { void SshControl::getUserKeys(const QString &username) { HttpRequest *request = new HttpRequest(this, false); - QObject::connect(request, &HttpRequest::receivedResponse, [=](const QString &resp) { - if (!resp.isEmpty()) { - params.put("GithubUsername", username.toStdString()); - params.put("GithubSshKeys", resp.toStdString()); + QObject::connect(request, &HttpRequest::requestDone, [=](const QString &resp, bool success) { + if (success) { + if (!resp.isEmpty()) { + params.put("GithubUsername", username.toStdString()); + params.put("GithubSshKeys", resp.toStdString()); + } else { + ConfirmationDialog::alert(QString("Username '%1' has no keys on GitHub").arg(username), this); + } } else { - ConfirmationDialog::alert(QString("Username '%1' has no keys on GitHub").arg(username), this); + if (request->timeout()) { + ConfirmationDialog::alert("Request timed out", this); + } else { + ConfirmationDialog::alert(QString("Username '%1' doesn't exist on GitHub").arg(username), this); + } } - refresh(); - request->deleteLater(); - }); - QObject::connect(request, &HttpRequest::failedResponse, [=] { - ConfirmationDialog::alert(QString("Username '%1' doesn't exist on GitHub").arg(username), this); - refresh(); - request->deleteLater(); - }); - QObject::connect(request, &HttpRequest::timeoutResponse, [=] { - ConfirmationDialog::alert("Request timed out", this); + refresh(); request->deleteLater(); }); diff --git a/selfdrive/ui/replay/route.cc b/selfdrive/ui/replay/route.cc index caaef4de5c..b3736fa75f 100644 --- a/selfdrive/ui/replay/route.cc +++ b/selfdrive/ui/replay/route.cc @@ -35,10 +35,8 @@ bool Route::load() { bool Route::loadFromServer() { QEventLoop loop; HttpRequest http(nullptr, !Hardware::PC()); - QObject::connect(&http, &HttpRequest::failedResponse, [&] { loop.exit(0); }); - QObject::connect(&http, &HttpRequest::timeoutResponse, [&] { loop.exit(0); }); - QObject::connect(&http, &HttpRequest::receivedResponse, [&](const QString &json) { - loop.exit(loadFromJson(json)); + QObject::connect(&http, &HttpRequest::requestDone, [&](const QString &json, bool success) { + loop.exit(success ? loadFromJson(json) : 0); }); http.sendRequest("https://api.commadotai.com/v1/route/" + route_.str + "/files"); return loop.exec();