refactor(controller): revert to exit() instead of connect dialog quit…

… to exit, stop card event monitor thread immediately on cancel and on critical failure WE2-672 Signed-off-by: Mart Somermaa <[email protected]>
web-eid · Jun 13, 2022 · c64653b · c64653b
1 parent 437adea
commit c64653b
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 31 deletions.
diff --git a/src/controller/controller.cpp b/src/controller/controller.cpp
@@ -55,6 +55,8 @@ QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessag
 
 void interruptThread(QThread* thread)
 {
+    REQUIRE_NON_NULL(thread)
+
     qDebug() << "Interrupting thread" << uintptr_t(thread);
     thread->disconnect();
     thread->requestInterruption();
@@ -274,9 +276,6 @@ void Controller::onCommandHandlerConfirmCompleted(const QVariantMap& res)
 
     qDebug() << "Command completed";
 
-    // Schedule application exit when the UI dialog is destroyed.
-    connect(window, &WebEidUI::destroyed, this, &Controller::exit);
-
     try {
         _result = res;
         writeResponseToStdOut(isInStdinMode, res, commandHandler->commandType());
@@ -286,6 +285,7 @@ void Controller::onCommandHandlerConfirmCompleted(const QVariantMap& res)
     }
 
     window->quit();
+    exit();
 }
 
 void Controller::onRetry()
@@ -331,29 +331,31 @@ void Controller::onDialogOK(const CardCertificateAndPinInfo& cardCertAndPinInfo)
 
 void Controller::onDialogCancel()
 {
-    REQUIRE_NON_NULL(window)
+    stopCardEventMonitorThread();
 
     qDebug() << "User cancelled";
 
-    // Schedule application exit when the UI dialog is destroyed.
-    connect(window, &WebEidUI::destroyed, this, &Controller::exit);
-
     _result = makeErrorObject(RESP_USER_CANCEL, QStringLiteral("User cancelled"));
     writeResponseToStdOut(isInStdinMode, _result, commandType());
+
+    exit();
 }
 
 void Controller::onPinPadCancel()
 {
     REQUIRE_NON_NULL(window)
 
-    onDialogCancel();
     window->quit();
+    onDialogCancel();
 }
 
 void Controller::onCriticalFailure(const QString& error)
 {
+    stopCardEventMonitorThread();
+
     qCritical() << "Exiting due to command" << std::string(commandType())
                 << "fatal error:" << error;
+
     _result = makeErrorObject(RESP_TECH_ERROR, error);
     writeResponseToStdOut(isInStdinMode, _result, commandType());
     disposeUI();

diff --git a/src/controller/controller.hpp b/src/controller/controller.hpp
@@ -79,7 +79,7 @@ class Controller : public QObject
     void saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread);
     void stopCardEventMonitorThread();
     void disposeUI();
-    void exit();
+    void exit(); // private slot
     void waitForChildThreads();
     CommandType commandType();
 

diff --git a/src/ui/webeiddialog.cpp b/src/ui/webeiddialog.cpp
@@ -27,6 +27,7 @@
 #include "ui_dialog.h"
 
 #include <QButtonGroup>
+#include <QCloseEvent>
 #include <QDesktopServices>
 #include <QFile>
 #include <QMessageBox>
@@ -396,6 +397,15 @@ void WebEidDialog::reject()
     }
 }
 
+void WebEidDialog::closeEvent(QCloseEvent* event)
+{
+    if (closeUnconditionally) {
+        event->accept();
+    } else {
+        WebEidUI::closeEvent(event);
+    }
+}
+
 bool WebEidDialog::event(QEvent* event)
 {
     if (event->type() == QEvent::LanguageChange) {

diff --git a/src/ui/webeiddialog.hpp b/src/ui/webeiddialog.hpp
@@ -24,7 +24,7 @@
 
 #include "ui.hpp"
 
-#include <QCloseEvent>
+class QCloseEvent;
 
 // clang-format off
 /**
@@ -76,14 +76,7 @@ class WebEidDialog final : public WebEidUI
     bool event(QEvent* event) final;
     void reject() final;
 
-    void closeEvent(QCloseEvent* event) final
-    {
-        if (closeUnconditionally) {
-            event->accept();
-        } else {
-            WebEidUI::closeEvent(event);
-        }
-    }
+    void closeEvent(QCloseEvent* event) final;
 
     void connectOkToCachePinAndEmitSelectedCertificate(const CardCertificateAndPinInfo& certAndPin);
 

diff --git a/tests/mock-ui/mock-ui.hpp b/tests/mock-ui/mock-ui.hpp
@@ -53,20 +53,14 @@ class MockUI : public WebEidUI
 
     void onSigningCertificateMismatch() override {}
 
-    void onRetry(const RetriableError) override { emit rejected(); }
+    void onRetry(const RetriableError) override { reject(); }
 
     void onVerifyPinFailed(const electronic_id::VerifyPinFailed::Status, const qint8) override {}
 
     void onSmartCardStatusUpdate(const RetriableError) override
     {
-        emit rejected();
-        // Schedule invoking Controller::exit().
-        emit destroyed();
+        reject();
     }
 
-    void quit() final
-    {
-        // Schedule invoking Controller::exit().
-        emit destroyed();
-    }
+    void quit() final {}
 };
diff --git a/tests/tests/main.cpp b/tests/tests/main.cpp
@@ -75,7 +75,7 @@ private slots:
     void quit_exits();
 
 private:
-    void runEventLoopVerifySignalsEmitted(QSignalSpy& actionSpy, bool waitForQuit = true);
+    void runEventLoopVerifySignalsEmitted(QSignalSpy& actionSpy);
     void initGetCert();
     void initAuthenticate();
     void initCard(bool withSigningScript = true);
@@ -101,7 +101,7 @@ void WebEidTests::statusUpdate_withUnsupportedCard_hasExpectedStatus()
     QSignalSpy statusUpdateSpy(controller.get(), &Controller::statusUpdate);
 
     // act
-    runEventLoopVerifySignalsEmitted(statusUpdateSpy, false);
+    runEventLoopVerifySignalsEmitted(statusUpdateSpy);
 
     // assert
     const auto statusArgument = qvariant_cast<RetriableError>(statusUpdateSpy.first().at(0));
@@ -220,7 +220,7 @@ void WebEidTests::quit_exits()
     }
 }
 
-void WebEidTests::runEventLoopVerifySignalsEmitted(QSignalSpy& actionSpy, bool waitForQuit)
+void WebEidTests::runEventLoopVerifySignalsEmitted(QSignalSpy& actionSpy)
 {
     // Waits until Controller emits quit.
     QSignalSpy quitSpy(controller.get(), &Controller::quit);
@@ -230,7 +230,7 @@ void WebEidTests::runEventLoopVerifySignalsEmitted(QSignalSpy& actionSpy, bool w
 
     // Run the event loop, verify that signals were emitted.
     QVERIFY(actionSpy.wait());
-    if (waitForQuit && quitSpy.count() < 1) {
+    if (quitSpy.count() < 1) {
         QVERIFY(quitSpy.wait());
     }
 }