Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(controller): revert to exit() instead of connect window::destroyed → controller::exit, stop card event monitor thread immediately on cancel and on critical failure #201

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/controller/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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());
Expand All @@ -286,6 +285,7 @@ void Controller::onCommandHandlerConfirmCompleted(const QVariantMap& res)
}

window->quit();
exit();
}

void Controller::onRetry()
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/controller/controller.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
10 changes: 10 additions & 0 deletions src/ui/webeiddialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "ui_dialog.h"

#include <QButtonGroup>
#include <QCloseEvent>
#include <QDesktopServices>
#include <QFile>
#include <QMessageBox>
Expand Down Expand Up @@ -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) {
Expand Down
11 changes: 2 additions & 9 deletions src/ui/webeiddialog.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

#include "ui.hpp"

#include <QCloseEvent>
class QCloseEvent;

// clang-format off
/**
Expand Down Expand Up @@ -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);

Expand Down
12 changes: 3 additions & 9 deletions tests/mock-ui/mock-ui.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
};
8 changes: 4 additions & 4 deletions tests/tests/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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());
}
}
Expand Down