windows: updates: make UI represent underlying update states

This commit fixes several issues:
- the update confirmation dialog getting stuck in the open state
- secondary attempts to download an update possibly causing a crash
- poor error information presented in the case of missing content
- unhandled errors during MSI package installation

Gitlab: #1367
Change-Id: Ia8855b8268ab13b8e1cbbb15de75d41f593f947a

src/app/MainApplicationWindow.qml

@@ -268,7 +268,7 @@ ApplicationWindow {
     }
 
     function presentUpdateInfoDialog(infoText) {
-        viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
+        return viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
                 "title": JamiStrings.updateDialogTitle,
                 "infoText": infoText,
                 "buttonTitles": [JamiStrings.optionOk],
@@ -277,6 +277,36 @@ ApplicationWindow {
             });
     }
 
+    function presentUpdateConfirmInstallDialog(switchToBeta=false) {
+        return viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
+                "title": JamiStrings.updateDialogTitle,
+                "infoText": switchToBeta ? JamiStrings.confirmBeta : JamiStrings.updateFound,
+                "buttonTitles": [JamiStrings.optionUpgrade, JamiStrings.optionLater],
+                "buttonStyles": [SimpleMessageDialog.ButtonStyle.TintedBlue, SimpleMessageDialog.ButtonStyle.TintedBlue],
+                "buttonCallBacks": [function () {
+                        AppVersionManager.applyUpdates(switchToBeta);
+                    }]
+            });
+    }
+
+    function translateErrorToString(error) {
+        switch (error) {
+        case NetworkManager.DISCONNECTED:
+            return JamiStrings.networkDisconnected;
+        case NetworkManager.CONTENT_NOT_FOUND:
+            return JamiStrings.contentNotFoundError;
+        case NetworkManager.ACCESS_DENIED:
+            return JamiStrings.accessError;
+        case NetworkManager.SSL_ERROR:
+            return JamiStrings.updateSSLError;
+        case NetworkManager.CANCELED:
+            return JamiStrings.updateDownloadCanceled;
+        case NetworkManager.NETWORK_ERROR:
+        default:
+            return JamiStrings.updateNetworkError;
+        }
+    }
+
     Connections {
         target: AppVersionManager
 
@@ -288,41 +318,26 @@ ApplicationWindow {
 
         function onUpdateCheckReplyReceived(ok, found) {
             if (!ok) {
+                // Show an error dialog describing that we could not successfully check for an update.
                 presentUpdateInfoDialog(JamiStrings.updateCheckError);
                 return;
             }
             if (!found) {
+                // Show a dialog describing that no update was found.
                 presentUpdateInfoDialog(JamiStrings.updateNotFound);
             } else {
-                viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
+                // Show a dialog describing that an update were found, and offering to install it.
-                        "title": JamiStrings.updateDialogTitle,
+                presentUpdateConfirmInstallDialog()
-                        "infoText": JamiStrings.updateFound,
-                        "buttonTitles": [JamiStrings.optionUpgrade, JamiStrings.optionLater],
-                        "buttonStyles": [SimpleMessageDialog.ButtonStyle.TintedBlue, SimpleMessageDialog.ButtonStyle.TintedBlue],
-                        "buttonCallBacks": [function () {
-                                AppVersionManager.applyUpdates();
-                            }]
-                    });
             }
         }
 
-        function onUpdateErrorOccurred(error) {
+        function onNetworkErrorOccurred(error) {
-            presentUpdateInfoDialog((function () {
+            var errorStr = translateErrorToString(error);
-                        switch (error) {
+            presentUpdateInfoDialog(errorStr);
-                        case NetworkManager.ACCESS_DENIED:
+        }
-                            return JamiStrings.genericError;
+
-                        case NetworkManager.DISCONNECTED:
+        function onInstallErrorOccurred(errorMsg) {
-                            return JamiStrings.networkDisconnected;
+            presentUpdateInfoDialog(errorMsg);
-                        case NetworkManager.NETWORK_ERROR:
-                            return JamiStrings.updateNetworkError;
-                        case NetworkManager.SSL_ERROR:
-                            return JamiStrings.updateSSLError;
-                        case NetworkManager.CANCELED:
-                            return JamiStrings.updateDownloadCanceled;
-                        default:
-                            return {};
-                        }
-                    })());
         }
     }
 

src/app/appversionmanager.cpp

@@ -62,11 +62,11 @@ struct AppVersionManager::Impl : public QObject
             connect(&parent_,
                     &NetworkManager::errorOccurred,
                     &parent_,
-                    &AppVersionManager::updateErrorOccurred);
+                    &AppVersionManager::networkErrorOccurred);
 
         cleanUpdateFiles();
-        QUrl versionUrl {isBeta ? QUrl::fromUserInput(baseUrlString_ + betaVersionSubUrl)
+        const QUrl versionUrl {isBeta ? QUrl::fromUserInput(baseUrlString_ + betaVersionSubUrl)
-                                : QUrl::fromUserInput(baseUrlString_ + versionSubUrl)};
+                                      : QUrl::fromUserInput(baseUrlString_ + versionSubUrl)};
         parent_.sendGetRequest(versionUrl, [this, quiet](const QByteArray& latestVersionString) {
             if (latestVersionString.isEmpty()) {
                 qWarning() << "Error checking version";
@@ -76,14 +76,15 @@ struct AppVersionManager::Impl : public QObject
             }
             auto currentVersion = QString(VERSION_STRING).toULongLong();
             auto latestVersion = latestVersionString.toULongLong();
-            qDebug() << "latest: " << latestVersion << " current: " << currentVersion;
+            const QString channelStr = isBeta ? "beta" : "stable";
-            if (latestVersion > currentVersion) {
+            const auto newVersionFound = latestVersion > currentVersion;
-                qDebug() << "New version found";
+            qInfo().noquote() << "--------- Version info ------------"
+                              << QString("\n - Current: %1 (%2)").arg(currentVersion).arg(channelStr);
+            if (newVersionFound) {
+                qDebug() << " - Latest: " << latestVersion;
                 Q_EMIT parent_.updateCheckReplyReceived(true, true);
-            } else {
+            } else if (!quiet) {
-                qDebug() << "No new version found";
+                Q_EMIT parent_.updateCheckReplyReceived(true, false);
-                if (!quiet)
-                    Q_EMIT parent_.updateCheckReplyReceived(true, false);
             }
         });
     };
@@ -94,35 +95,56 @@ struct AppVersionManager::Impl : public QObject
         connect(&parent_,
                 &NetworkManager::errorOccurred,
                 &parent_,
-                &AppVersionManager::updateErrorOccurred);
+                &AppVersionManager::networkErrorOccurred);
 
         const QUrl downloadUrl {(beta || isBeta)
                                     ? QUrl::fromUserInput(baseUrlString_ + betaMsiSubUrl)
                                     : QUrl::fromUserInput(baseUrlString_ + msiSubUrl)};
 
-        int uuid = parent_.downloadFile(
+        const auto lastDownloadReplyId = parent_.replyId_;
+        parent_.replyId_ = parent_.downloadFile(
             downloadUrl,
-            *(parent_.replyId_),
+            lastDownloadReplyId,
             [this, downloadUrl](bool success, const QString& errorMessage) {
                 Q_UNUSED(success)
                 Q_UNUSED(errorMessage)
-                const QProcess process;
+                QProcess process;
                 auto basePath = tempPath_ + QDir::separator();
                 auto msiPath = QDir::toNativeSeparators(basePath + downloadUrl.fileName());
                 auto logPath = QDir::toNativeSeparators(basePath + "jami_x64_install.log");
-                process.startDetached("msiexec",
+                connect(&process, &QProcess::errorOccurred, this, [&](QProcess::ProcessError error) {
-                                      QStringList() << "/i" << msiPath << "/passive"
+                    QString errorMsg;
-                                                    << "/norestart"
+                    if (error == QProcess::ProcessError::Timedout) {
-                                                    << "WIXNONUILAUNCH=1"
+                        errorMsg = tr("The installer process has timed out.");
-                                                    << "/L*V" << logPath);
+                    } else {
+                        errorMsg = process.readAllStandardError();
+                        if (errorMsg.isEmpty())
+                            errorMsg = tr("The installer process has failed.");
+                    }
+                    Q_EMIT parent_.installErrorOccurred(errorMsg);
+                });
+                connect(&process,
+                        &QProcess::finished,
+                        this,
+                        [&](int exitCode, QProcess::ExitStatus exitStatus) {
+                            if (exitStatus != QProcess::ExitStatus::NormalExit || exitCode != 0) {
+                                auto errorMsg = process.readAllStandardOutput();
+                                Q_EMIT parent_.installErrorOccurred(errorMsg);
+                            }
+                        });
+                process.start("msiexec",
+                              QStringList() << "/i" << msiPath << "/passive"
+                                            << "/norestart"
+                                            << "WIXNONUILAUNCH=1"
+                                            << "/L*V" << logPath);
+                process.waitForFinished();
             },
             tempPath_);
-        parent_.replyId_.reset(&uuid);
     };
 
     void cancelUpdate()
     {
-        parent_.cancelDownload(*(parent_.replyId_));
+        parent_.cancelDownload(parent_.replyId_);
     };
 
     void setAutoUpdateCheck(bool state)
@@ -138,7 +160,7 @@ struct AppVersionManager::Impl : public QObject
     void cleanUpdateFiles()
     {
         // Delete all logs and msi in the temporary directory before launching.
-        QString dir = QDir::tempPath();
+        const QString dir = QDir::tempPath();
         QDir log_dir(dir, {"jami*.log"});
         for (const QString& filename : log_dir.entryList()) {
             log_dir.remove(filename);
@@ -166,13 +188,13 @@ AppVersionManager::AppVersionManager(const QString& url,
                                      LRCInstance* instance,
                                      QObject* parent)
     : NetworkManager(cm, parent)
-    , replyId_(new int(0))
+    , replyId_(0)
     , pimpl_(std::make_unique<Impl>(url, instance, *this))
 {}
 
 AppVersionManager::~AppVersionManager()
 {
-    cancelDownload(*replyId_);
+    cancelDownload(replyId_);
 }
 
 void

src/app/appversionmanager.h

@@ -47,10 +47,11 @@ Q_SIGNALS:
     void appCloseRequested();
     void updateCheckReplyReceived(bool ok, bool found = false);
     void updateDownloadProgressChanged(qint64 bytesRead, qint64 totalBytes);
-    void updateErrorOccurred(const NetworkManager::GetError& error);
+    void networkErrorOccurred(const NetworkManager::GetError& error);
+    void installErrorOccurred(const QString& errorMsg);
 
 private:
-    QScopedPointer<int> replyId_;
+    int replyId_;
     struct Impl;
     friend struct Impl;
     std::unique_ptr<Impl> pimpl_;

src/app/constant/JamiStrings.qml

@@ -513,6 +513,8 @@ Item {
     property string updateDownloading: "Downloading"
     property string confirmBeta: qsTr("This will uninstall your current Release version and you can always download the latest Release version on our website")
     property string networkDisconnected: qsTr("Network disconnected")
+    property string accessError: qsTr("Content access error")
+    property string contentNotFoundError: qsTr("Content not found")
     property string genericError: qsTr("Something went wrong")
 
     //Troubleshoot Settings

src/app/networkmanager.cpp

@@ -25,6 +25,28 @@
 #include <QtNetwork>
 #include <QScopedPointer>
 
+namespace {
+NetworkManager::GetError
+translateErrorCode(QNetworkReply::NetworkError error)
+{
+    // From qnetworkreply.h:
+    // network layer errors (1-99): / proxy errors (101-199):
+    // content errors (201-299): ContentAccessDenied = 201,
+    // protocol errors / Server side errors (401-499)
+    static auto inRange = [](int value, int min, int max) -> bool {
+        return (value >= min && value <= max);
+    };
+    if (inRange(error, 1, 199))
+        return NetworkManager::NETWORK_ERROR;
+    if (inRange(error, 201, 201))
+        return NetworkManager::ACCESS_DENIED;
+    if (inRange(error, 202, 299))
+        return NetworkManager::CONTENT_NOT_FOUND;
+    return NetworkManager::NETWORK_ERROR;
+}
+
+} // namespace
+
 NetworkManager::NetworkManager(ConnectivityMonitor* cm, QObject* parent)
     : QObject(parent)
     , manager_(new QNetworkAccessManager(this))
@@ -45,7 +67,7 @@ NetworkManager::NetworkManager(ConnectivityMonitor* cm, QObject* parent)
             });
 #endif
     connect(connectivityMonitor_, &ConnectivityMonitor::connectivityChanged, this, [this] {
-        auto connected = connectivityMonitor_->isOnline();
+        const auto connected = connectivityMonitor_->isOnline();
         if (connected && !lastConnectionState_) {
             manager_->deleteLater();
             manager_ = new QNetworkAccessManager(this);
@@ -59,7 +81,7 @@ void
 NetworkManager::sendGetRequest(const QUrl& url,
                                std::function<void(const QByteArray&)>&& onDoneCallback)
 {
-    QNetworkRequest request = QNetworkRequest(url);
+    const QNetworkRequest request = QNetworkRequest(url);
     sendGetRequest(request, std::move(onDoneCallback));
 }
 
@@ -98,9 +120,8 @@ NetworkManager::downloadFile(const QUrl& url,
                              const QString& filePath,
                              const QString& extension)
 {
-    // If there is already a download in progress, return.
+    // Don't replace the download if there is already a download in progress for this id.
-    if ((downloadReplies_.value(replyId) != NULL || !(replyId == 0))
+    if (downloadReplies_.contains(replyId) && downloadReplies_.value(replyId)->isRunning()) {
-        && downloadReplies_[replyId]->isRunning()) {
         qWarning() << Q_FUNC_INFO << "Download already in progress";
         return replyId;
     }
@@ -121,6 +142,7 @@ NetworkManager::downloadFile(const QUrl& url,
     }
 
     // set the id for the request
+    // NOLINTNEXTLINE(misc-const-correctness)
     std::uniform_int_distribution<int> dist(1, std::numeric_limits<int>::max());
     auto uuid = dist(rng_);
 
@@ -167,7 +189,7 @@ NetworkManager::downloadFile(const QUrl& url,
                 resetDownload(uuid);
                 qWarning() << Q_FUNC_INFO
                            << QMetaEnum::fromType<QNetworkReply::NetworkError>().valueToKey(error);
-                Q_EMIT errorOccurred(GetError::NETWORK_ERROR);
+                Q_EMIT errorOccurred(translateErrorCode(error));
             });
 
     connect(reply, &QNetworkReply::finished, this, [this, uuid, onDoneCallback, reply, file]() {

src/app/networkmanager.h

@@ -35,7 +35,14 @@ public:
     explicit NetworkManager(ConnectivityMonitor* cm, QObject* parent = nullptr);
     virtual ~NetworkManager() = default;
 
-    enum GetError { DISCONNECTED, NETWORK_ERROR, ACCESS_DENIED, SSL_ERROR, CANCELED };
+    enum GetError {
+        DISCONNECTED,
+        CONTENT_NOT_FOUND,
+        ACCESS_DENIED,
+        SSL_ERROR,
+        CANCELED,
+        NETWORK_ERROR,
+    };
     Q_ENUM(GetError)
 
     void sendGetRequest(const QUrl& url, std::function<void(const QByteArray&)>&& onDoneCallback);
@@ -48,7 +55,7 @@ public:
                      int replyId,
                      std::function<void(bool, const QString&)>&& onDoneCallback,
                      const QString& filePath,
-                     const QString& extension = "");
+                     const QString& extension = {});
     void resetDownload(int replyId);
     void cancelDownload(int replyId);
 Q_SIGNALS:

src/app/settingsview/components/UpdateDownloadDialog.qml

@@ -36,8 +36,7 @@ SimpleMessageDialog {
     Connections {
         target: AppVersionManager
 
-        function onErrorOccurred(error, msg) {
+        function onNetworkErrorOccurred(error) {
-            console.warn("Error while downloading update: " + error + " - " + msg);
             downloadDialog.close();
         }
 

src/app/settingsview/components/UpdateSettingsPage.qml

@@ -32,28 +32,6 @@ SettingsPageBase {
 
     title: JamiStrings.updatesTitle
 
-    function presentInfoDialog(infoText) {
-        viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
-                "title": JamiStrings.updateDialogTitle,
-                "infoText": infoText,
-                "buttonTitles": [JamiStrings.optionOk],
-                "buttonStyles": [SimpleMessageDialog.ButtonStyle.TintedBlue],
-                "buttonCallBacks": []
-            });
-    }
-
-    function presentConfirmInstallDialog(infoText, beta) {
-        viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
-                "title": JamiStrings.updateDialogTitle,
-                "infoText": infoText,
-                "buttonTitles": [JamiStrings.optionUpgrade, JamiStrings.optionLater],
-                "buttonStyles": [SimpleMessageDialog.ButtonStyle.TintedBlue, SimpleMessageDialog.ButtonStyle.TintedBlue],
-                "buttonCallBacks": [function () {
-                        AppVersionManager.applyUpdates(beta);
-                    }]
-            });
-    }
-
     flickableContent: ColumnLayout {
         id: manageAccountEnableColumnLayout
         width: contentFlickableWidth
@@ -119,15 +97,7 @@ SettingsPageBase {
             toolTipText: JamiStrings.betaInstall
             text: JamiStrings.betaInstall
 
-            onClicked: viewCoordinator.presentDialog(appWindow, "commoncomponents/SimpleMessageDialog.qml", {
+            onClicked: appWindow.presentUpdateConfirmInstallDialog(true)
-                    "title": JamiStrings.updateDialogTitle,
-                    "infoText": JamiStrings.confirmBeta,
-                    "buttonTitles": [JamiStrings.optionUpgrade, JamiStrings.optionLater],
-                    "buttonStyles": [SimpleMessageDialog.ButtonStyle.TintedBlue, SimpleMessageDialog.ButtonStyle.TintedBlue],
-                    "buttonCallBacks": [function () {
-                            AppVersionManager.applyUpdates(true);
-                        }]
-                })
         }
     }
 }