From 4f45cbadedb2968b30db1b68725450d21721c0d8 Mon Sep 17 00:00:00 2001 From: Christian Heimlich Date: Thu, 14 Nov 2024 15:19:57 -0500 Subject: [PATCH] Improve directive handling Make a dedicated type for directives that need a response, RequestDirective, along with related concepts. Have response return from the postDirective() call for RequestDirective types. Use ADL to simplify visit pattern in frontend. --- app/gui/src/frontend/gui.cpp | 43 +++++++++---------- app/gui/src/frontend/gui.h | 28 ++++++------ lib/backend/include/kernel/directive.h | 30 +++++++++---- lib/backend/include/kernel/driver.h | 1 + lib/backend/src/command/c-link.cpp | 4 +- lib/backend/src/command/c-update.cpp | 3 +- lib/backend/src/kernel/core.cpp | 6 +-- lib/backend/src/kernel/director.h | 36 +++++++++++++--- lib/backend/src/kernel/directorate.h | 12 +++--- lib/backend/src/kernel/driver.cpp | 1 + lib/backend/src/task/t-download.cpp | 4 +- .../include/frontend/framework.h | 31 +++++++------ .../src/frontend/framework.cpp | 33 +++++++------- 13 files changed, 131 insertions(+), 101 deletions(-) diff --git a/app/gui/src/frontend/gui.cpp b/app/gui/src/frontend/gui.cpp index 3af7056..d620886 100644 --- a/app/gui/src/frontend/gui.cpp +++ b/app/gui/src/frontend/gui.cpp @@ -78,16 +78,16 @@ QIcon& FrontendGui::trayExitIconFromResources() { static QIcon ico(u":/frontend/ //-Instance Functions------------------------------------------------------------------------------------------------------ //Private: -void FrontendGui::handleMessage(const DMessage& d) +void FrontendGui::handleDirective(const DMessage& d) { auto mb = prepareMessageBox(d); mb->setAttribute(Qt::WA_DeleteOnClose); mb->show(); } -void FrontendGui::handleError(const DError& d) { Qx::postError(d.error); } +void FrontendGui::handleDirective(const DError& d) { Qx::postError(d.error); } -void FrontendGui::handleProcedureStart(const DProcedureStart& d) +void FrontendGui::handleDirective(const DProcedureStart& d) { // Set label mProgressDialog.setLabelText(d.label); @@ -96,7 +96,7 @@ void FrontendGui::handleProcedureStart(const DProcedureStart& d) mProgressDialog.setValue(0); } -void FrontendGui::handleProcedureStop(const DProcedureStop& d) +void FrontendGui::handleDirective(const DProcedureStop& d) { Q_UNUSED(d); /* Always reset the dialog regardless of whether it is visible or not as it may not be currently visible, @@ -106,49 +106,46 @@ void FrontendGui::handleProcedureStop(const DProcedureStop& d) mProgressDialog.reset(); } -void FrontendGui::handleProcedureProgress(const DProcedureProgress& d) { mProgressDialog.setValue(d.current); } -void FrontendGui::handleProcedureScale(const DProcedureScale& d) { mProgressDialog.setMaximum(d.max); } -void FrontendGui::handleStatusUpdate(const DStatusUpdate& d) { mStatusHeading = d.heading; mStatusMessage = d.message; } +void FrontendGui::handleDirective(const DProcedureProgress& d) { mProgressDialog.setValue(d.current); } +void FrontendGui::handleDirective(const DProcedureScale& d) { mProgressDialog.setMaximum(d.max); } +void FrontendGui::handleDirective(const DStatusUpdate& d) { mStatusHeading = d.heading; mStatusMessage = d.message; } // Sync directive handlers -void FrontendGui::handleBlockingMessage(const DBlockingMessage& d) +void FrontendGui::handleDirective(const DBlockingMessage& d) { auto mb = prepareMessageBox(d); mb->exec(); delete mb; } -void FrontendGui::handleBlockingError(const DBlockingError& d) +// Request directive handlers +void FrontendGui::handleDirective(const DBlockingError& d, DBlockingError::Choice* response) { - Q_ASSERT(d.response && d.choices != DBlockingError::Choice::NoChoice); + Q_ASSERT(d.choices != DBlockingError::Choice::NoChoice); auto btns = choicesToButtons(d.choices); auto def = smChoiceButtonMap.toRight(d.defaultChoice); int rawRes = Qx::postBlockingError(d.error, btns, def); - *d.response = smChoiceButtonMap.toLeft(static_cast(rawRes)); + *response = smChoiceButtonMap.toLeft(static_cast(rawRes)); } -void FrontendGui::handleSaveFilename(const DSaveFilename& d) +void FrontendGui::handleDirective(const DSaveFilename& d, QString* response) { - Q_ASSERT(d.response); - *d.response = QFileDialog::getSaveFileName(nullptr, d.caption, d.dir, d.filter, d.selectedFilter); + *response = QFileDialog::getSaveFileName(nullptr, d.caption, d.dir, d.filter, d.selectedFilter); } -void FrontendGui::handleExistingDir(const DExistingDir& d) +void FrontendGui::handleDirective(const DExistingDir& d, QString* response) { - Q_ASSERT(d.response); - *d.response = QFileDialog::getExistingDirectory(nullptr, d.caption, d.startingDir); + *response = QFileDialog::getExistingDirectory(nullptr, d.caption, d.startingDir); } -void FrontendGui::handleItemSelection(const DItemSelection& d) +void FrontendGui::handleDirective(const DItemSelection& d, QString* response) { - Q_ASSERT(d.response); - *d.response = QInputDialog::getItem(nullptr, d.caption, d.label, d.items, 0, false); + *response = QInputDialog::getItem(nullptr, d.caption, d.label, d.items, 0, false); } -void FrontendGui::handleYesOrNo(const DYesOrNo& d) +void FrontendGui::handleDirective(const DYesOrNo& d, bool* response) { - Q_ASSERT(d.response); - *d.response = QMessageBox::question(nullptr, QString(), d.question) == QMessageBox::Yes; + *response = QMessageBox::question(nullptr, QString(), d.question) == QMessageBox::Yes; } bool FrontendGui::aboutToExit() diff --git a/app/gui/src/frontend/gui.h b/app/gui/src/frontend/gui.h index 3082309..fb66221 100644 --- a/app/gui/src/frontend/gui.h +++ b/app/gui/src/frontend/gui.h @@ -54,21 +54,23 @@ static QIcon& trayExitIconFromResources(); //-Instance Functions------------------------------------------------------------------------------------------------------ private: // Async directive handlers - void handleMessage(const DMessage& d) override; - void handleError(const DError& d) override; - void handleProcedureStart(const DProcedureStart& d) override; - void handleProcedureStop(const DProcedureStop& d) override; - void handleProcedureProgress(const DProcedureProgress& d) override; - void handleProcedureScale(const DProcedureScale& d) override; - void handleStatusUpdate(const DStatusUpdate& d) override; + void handleDirective(const DMessage& d) override; + void handleDirective(const DError& d) override; + void handleDirective(const DProcedureStart& d) override; + void handleDirective(const DProcedureStop& d) override; + void handleDirective(const DProcedureProgress& d) override; + void handleDirective(const DProcedureScale& d) override; + void handleDirective(const DStatusUpdate& d) override; // Sync directive handlers - void handleBlockingMessage(const DBlockingMessage& d) override; - void handleBlockingError(const DBlockingError& d) override; - void handleSaveFilename(const DSaveFilename& d) override; - void handleExistingDir(const DExistingDir& d) override; - void handleItemSelection(const DItemSelection& d) override; - void handleYesOrNo(const DYesOrNo& d) override; + void handleDirective(const DBlockingMessage& d) override; + + // Request directive handlers + void handleDirective(const DBlockingError& d, DBlockingError::Choice* response) override; + void handleDirective(const DSaveFilename& d, QString* response) override; + void handleDirective(const DExistingDir& d, QString* response) override; + void handleDirective(const DItemSelection& d, QString* response) override; + void handleDirective(const DYesOrNo& d, bool* response) override; // Control bool aboutToExit() override; diff --git a/lib/backend/include/kernel/directive.h b/lib/backend/include/kernel/directive.h index a690fc0..255058a 100644 --- a/lib/backend/include/kernel/directive.h +++ b/lib/backend/include/kernel/directive.h @@ -99,6 +99,12 @@ struct DBlockingMessage bool selectable = false; }; +using SyncDirective = std::variant; + +template +concept SyncDirectiveT = requires(SyncDirective sd, T t) { sd = t; }; + +//-Blocking Directives With Response------------------------------------------------------- struct DBlockingError { enum class Choice @@ -113,7 +119,8 @@ struct DBlockingError Qx::Error error; Choices choices = Choice::Ok; Choice defaultChoice = Choice::No; - Choice* response = nullptr; + + using response_type = Choice; }; Q_DECLARE_OPERATORS_FOR_FLAGS(DBlockingError::Choices); @@ -123,14 +130,16 @@ struct DSaveFilename QString dir; QString filter; QString* selectedFilter = nullptr; - QString* response = nullptr; + + using response_type = QString; }; struct DExistingDir { QString caption; QString startingDir; - QString* response = nullptr; + + using response_type = QString; }; struct DItemSelection @@ -138,17 +147,18 @@ struct DItemSelection QString caption; QString label; QStringList items; - QString* response = nullptr; + + using response_type = QString; }; struct DYesOrNo { QString question; - bool* response = nullptr; + + using response_type = bool; }; -using SyncDirective = std::variant< - DBlockingMessage, +using RequestDirective = std::variant< DBlockingError, DSaveFilename, DExistingDir, @@ -157,14 +167,16 @@ using SyncDirective = std::variant< >; template -concept SyncDirectiveT = requires(SyncDirective sd, T t) { sd = t; }; +concept RequestDirectiveT = requires(RequestDirective rd, T t) { rd = t; typename T::response_type; } && + !std::same_as; //-Any--------------------------------------------------------------------- template -concept DirectiveT = AsyncDirectiveT || SyncDirectiveT; +concept DirectiveT = AsyncDirectiveT || SyncDirectiveT || RequestDirectiveT; //-Metatype Declarations----------------------------------------------------------------------------------------- Q_DECLARE_METATYPE(AsyncDirective); Q_DECLARE_METATYPE(SyncDirective); +Q_DECLARE_METATYPE(RequestDirective); #endif // DIRECTIVE_H diff --git a/lib/backend/include/kernel/driver.h b/lib/backend/include/kernel/driver.h index 563cb61..878fd88 100644 --- a/lib/backend/include/kernel/driver.h +++ b/lib/backend/include/kernel/driver.h @@ -46,6 +46,7 @@ public slots: // Director forwarders void asyncDirectiveAccounced(const AsyncDirective& aDirective); void syncDirectiveAccounced(const SyncDirective& sDirective); + void requestDirectiveAccounced(const RequestDirective& rDirective, void* response); }; #endif // DRIVER_H diff --git a/lib/backend/src/command/c-link.cpp b/lib/backend/src/command/c-link.cpp index 7e09540..fe059cb 100644 --- a/lib/backend/src/command/c-link.cpp +++ b/lib/backend/src/command/c-link.cpp @@ -102,11 +102,9 @@ Qx::Error CLink::perform() logEvent(LOG_EVENT_NO_PATH); // Prompt user for path - QString selectedPath; // Default is empty dir - postDirective(DExistingDir{ + QString selectedPath = postDirective(DExistingDir{ .caption = DIAG_CAPTION, .startingDir = QStandardPaths::writableLocation(QStandardPaths::DesktopLocation), - .response = &selectedPath }); if(selectedPath.isEmpty()) diff --git a/lib/backend/src/command/c-update.cpp b/lib/backend/src/command/c-update.cpp index a6db691..f2a2f02 100644 --- a/lib/backend/src/command/c-update.cpp +++ b/lib/backend/src/command/c-update.cpp @@ -320,8 +320,7 @@ CUpdateError CUpdate::checkAndPrepareUpdate() const return CUpdateError(); } - bool shouldUpdate = false; - postDirective(QUES_UPDATE.arg(rd.name), &shouldUpdate); + bool shouldUpdate = postDirective(QUES_UPDATE.arg(rd.name)); if(shouldUpdate) { logEvent(LOG_EVENT_UPDATE_ACCEPED); diff --git a/lib/backend/src/kernel/core.cpp b/lib/backend/src/kernel/core.cpp index 791061b..2bb1d1e 100644 --- a/lib/backend/src/kernel/core.cpp +++ b/lib/backend/src/kernel/core.cpp @@ -205,15 +205,13 @@ Qx::Error Core::searchAndFilterEntity(QUuid& returnBuffer, QString name, bool ex } // Get user choice - QString userChoice = idChoices.front(); // Default - postDirective(DItemSelection{ + QString userChoice = postDirective(DItemSelection{ .caption = MULTI_TITLE_SEL_CAP, .label = MULTI_TITLE_SEL_LABEL, .items = idChoices, - .response = &userChoice }); - if(userChoice.isNull()) + if(userChoice.isNull()) // Default if diag is silenced logEvent(LOG_EVENT_TITLE_SEL_CANCELED); else { diff --git a/lib/backend/src/kernel/director.h b/lib/backend/src/kernel/director.h index 606291e..a078f94 100644 --- a/lib/backend/src/kernel/director.h +++ b/lib/backend/src/kernel/director.h @@ -108,6 +108,14 @@ class Director : public QObject bool isLogOpen() const; void logQtMessage(QtMsgType type, const QMessageLogContext& context, const QString& msg); + // Helper + template + auto ddr() // Default directive return helper + { + if constexpr(RequestDirectiveT) + return typename T::response_type{}; + } + public: // Data Verbosity verbosity() const; @@ -122,30 +130,43 @@ class Director : public QObject // Directives template - void postDirective(const QString& src, const T& directive) + auto postDirective(const QString& src, const T& directive) { // Special handling if constexpr(Qx::any_of) { logError(src, directive.error); if(mVerbosity == Verbosity::Silent || (mVerbosity == Verbosity::Quiet && directive.error.severity() != Qx::Critical)) - return; + return ddr(); } else { if(mVerbosity != Verbosity::Full) - return; + return ddr(); } // Send if constexpr(AsyncDirectiveT) - { emit announceAsyncDirective(directive); - } + else if constexpr(SyncDirectiveT) + emit announceSyncDirective(directive); else { - static_assert(SyncDirectiveT); - emit announceSyncDirective(directive); + static_assert(RequestDirectiveT); + /* Normally using a void pointer is basically obsolete, and the much newer std::any should be used + * instead; however, here we must use a pointer to get a result back since this is a signal, and the + * type safety of std::any is less important since the pointer is cast automatically by our scaffolding + * based on ResponseDirectiveT::response_type. So, mind as well take advantage of the performance of + * void* then anyway. + * + * Also: If a default response is needed other than the default provided by value-initialization, we + * can add a static member "DEFAULT_RESPONSE" to each RequestDirectiveT struct and use that here. If + * various defaults are needed on a case-by-case basis, that gets tricky as we'd need another postDirective() + * overload. + */ + auto response = ddr(); + emit announceRequestDirective(directive, &response); + return response; } } @@ -153,6 +174,7 @@ class Director : public QObject signals: void announceAsyncDirective(const AsyncDirective& aDirective); void announceSyncDirective(const SyncDirective& sDirective); + void announceRequestDirective(const RequestDirective& rDirective, void* response); }; #endif // DIRECTOR_H diff --git a/lib/backend/src/kernel/directorate.h b/lib/backend/src/kernel/directorate.h index bd5c42f..0aa28f6 100644 --- a/lib/backend/src/kernel/directorate.h +++ b/lib/backend/src/kernel/directorate.h @@ -34,18 +34,18 @@ class Directorate void logError(const Qx::Error& error) const; void logEvent(const QString& event) const; + // These two only return a value when a RequestDirective is used, otherwise 'return' is ignored template - void postDirective(const T& t) const + [[nodiscard]] auto postDirective(const T& t) const { Q_ASSERT(mDirector); - mDirector->postDirective(name(), t); + return mDirector->postDirective(name(), t); } - template - void postDirective(Args&&... args) const + template + [[nodiscard]] auto postDirective(Args&&... args) const { - Q_ASSERT(mDirector); - mDirector->postDirective(name(), T{std::forward(args)...}); + return postDirective(T{std::forward(args)...}); } protected: diff --git a/lib/backend/src/kernel/driver.cpp b/lib/backend/src/kernel/driver.cpp index ef8aaa2..ccb3c85 100644 --- a/lib/backend/src/kernel/driver.cpp +++ b/lib/backend/src/kernel/driver.cpp @@ -85,6 +85,7 @@ void DriverPrivate::init() }); QObject::connect(dtor, &Director::announceAsyncDirective, q, &Driver::asyncDirectiveAccounced); QObject::connect(dtor, &Director::announceSyncDirective, q, &Driver::syncDirectiveAccounced); + QObject::connect(dtor, &Director::announceRequestDirective, q, &Driver::requestDirectiveAccounced); //-Setup deferred process manager------ /* NOTE: It looks like the manager should just be a stack member of TExec that is constructed diff --git a/lib/backend/src/task/t-download.cpp b/lib/backend/src/task/t-download.cpp index 2b0e1aa..f5aae9f 100644 --- a/lib/backend/src/task/t-download.cpp +++ b/lib/backend/src/task/t-download.cpp @@ -52,12 +52,10 @@ TDownload::TDownload(Core& core) : // Download event handlers connect(&mDownloadManager, &Qx::AsyncDownloadManager::sslErrors, this, [this](Qx::Error errorMsg, bool* ignore) { - DBlockingError::Choice choice; - postDirective(DBlockingError{ + auto choice = postDirective(DBlockingError{ .error = errorMsg, .choices = DBlockingError::Choice::Yes | DBlockingError::Choice::No, .defaultChoice = DBlockingError::Choice::No, - .response = &choice }); *ignore = choice == DBlockingError::Choice::Yes; }); diff --git a/lib/frontend_framework/include/frontend/framework.h b/lib/frontend_framework/include/frontend/framework.h index fc9a907..70d397a 100644 --- a/lib/frontend_framework/include/frontend/framework.h +++ b/lib/frontend_framework/include/frontend/framework.h @@ -63,22 +63,24 @@ class FrontendFramework : public QObject //-Instance Functions------------------------------------------------------------------------------------------------------ protected: // Async directive handlers - virtual void handleMessage(const DMessage& d) = 0; - virtual void handleError(const DError& d) = 0; - virtual void handleProcedureStart(const DProcedureStart& d) = 0; - virtual void handleProcedureStop(const DProcedureStop& d) = 0; - virtual void handleProcedureProgress(const DProcedureProgress& d) = 0; - virtual void handleProcedureScale(const DProcedureScale& d) = 0; - virtual void handleClipboardUpdate(const DClipboardUpdate& d); - virtual void handleStatusUpdate(const DStatusUpdate& d) = 0; + virtual void handleDirective(const DMessage& d) = 0; + virtual void handleDirective(const DError& d) = 0; + virtual void handleDirective(const DProcedureStart& d) = 0; + virtual void handleDirective(const DProcedureStop& d) = 0; + virtual void handleDirective(const DProcedureProgress& d) = 0; + virtual void handleDirective(const DProcedureScale& d) = 0; + virtual void handleDirective(const DClipboardUpdate& d); + virtual void handleDirective(const DStatusUpdate& d) = 0; // Sync directive handlers - virtual void handleBlockingMessage(const DBlockingMessage& d) = 0; - virtual void handleBlockingError(const DBlockingError& d) = 0; - virtual void handleSaveFilename(const DSaveFilename& d) = 0; - virtual void handleExistingDir(const DExistingDir& d) = 0; - virtual void handleItemSelection(const DItemSelection& d) = 0; - virtual void handleYesOrNo(const DYesOrNo& d) = 0; + virtual void handleDirective(const DBlockingMessage& d) = 0; + + // Request directive handlers + virtual void handleDirective(const DBlockingError& d, DBlockingError::Choice* response) = 0; + virtual void handleDirective(const DSaveFilename& d, QString* response) = 0; + virtual void handleDirective(const DExistingDir& d, QString* response) = 0; + virtual void handleDirective(const DItemSelection& d, QString* response) = 0; + virtual void handleDirective(const DYesOrNo& d, bool* response) = 0; // Control virtual bool aboutToExit(); @@ -96,6 +98,7 @@ private slots: void threadFinishHandler(); void asyncDirectiveHandler(const AsyncDirective& aDirective); void syncDirectiveHandler(const SyncDirective& sDirective); + void requestDirectiveHandler(const RequestDirective& rDirective, void* response); }; #endif // FRAMEWORK_H diff --git a/lib/frontend_framework/src/frontend/framework.cpp b/lib/frontend_framework/src/frontend/framework.cpp index 71632db..5023383 100644 --- a/lib/frontend_framework/src/frontend/framework.cpp +++ b/lib/frontend_framework/src/frontend/framework.cpp @@ -61,6 +61,7 @@ FrontendFramework::FrontendFramework(QGuiApplication* app) : // Register metatypes qRegisterMetaType(); qRegisterMetaType(); + qRegisterMetaType(); // Create driver Driver* driver = new Driver(mApp->arguments()); @@ -76,6 +77,7 @@ FrontendFramework::FrontendFramework(QGuiApplication* app) : // Connect driver - Directives connect(driver, &Driver::asyncDirectiveAccounced, this, &FrontendFramework::asyncDirectiveHandler); connect(driver, &Driver::syncDirectiveAccounced, this, &FrontendFramework::syncDirectiveHandler, Qt::BlockingQueuedConnection); + connect(driver, &Driver::requestDirectiveAccounced, this, &FrontendFramework::requestDirectiveHandler, Qt::BlockingQueuedConnection); // Store driver for use later mDriver = driver; @@ -123,7 +125,7 @@ const QIcon& FrontendFramework::appIconFromResources() { static QIcon ico(u":/fr //-Instance Functions-------------------------------------------------------------------------- //Protected: /* This implementation needs to be moved to FrontendGui if this ever drops the QGui dependency */ -void FrontendFramework::handleClipboardUpdate(const DClipboardUpdate& d) { mSystemClipboard->setText(d.text); } +void FrontendFramework::handleDirective(const DClipboardUpdate& d) { mSystemClipboard->setText(d.text); } bool FrontendFramework::aboutToExit() { @@ -167,26 +169,23 @@ void FrontendFramework::threadFinishHandler() void FrontendFramework::asyncDirectiveHandler(const AsyncDirective& aDirective) { - std::visit(qxFuncAggregate{ - [this](DMessage d){ handleMessage(d); }, - [this](DError d) { handleError(d); }, - [this](DProcedureStart d) { handleProcedureStart(d); }, - [this](DProcedureStop d) { handleProcedureStop(d); }, - [this](DProcedureProgress d) { handleProcedureProgress(d); }, - [this](DProcedureScale d) { handleProcedureScale(d); }, - [this](DClipboardUpdate d) { handleClipboardUpdate(d); }, - [this](DStatusUpdate d) { handleStatusUpdate(d); }, + std::visit([this](const auto& d) { + handleDirective(d); // ADL dispatches to the correct handle function }, aDirective); } void FrontendFramework::syncDirectiveHandler(const SyncDirective& sDirective) { - std::visit(qxFuncAggregate{ - [this](DBlockingMessage d){ handleBlockingMessage(d); }, - [this](DBlockingError d){ handleBlockingError(d); }, - [this](DSaveFilename d) { handleSaveFilename(d); }, - [this](DExistingDir d) { handleExistingDir(d); }, - [this](DItemSelection d) { handleItemSelection(d); }, - [this](DYesOrNo d) { handleYesOrNo(d); } + std::visit([this](const auto& d) { + handleDirective(d); // ADL dispatches to the correct handle function }, sDirective); } + +void FrontendFramework::requestDirectiveHandler(const RequestDirective& rDirective, void* response) +{ + Q_ASSERT(response); + std::visit([this, response](const auto& d) { + using ResponseT = std::decay_t::response_type; // Could use template lambda to avoid decltype + handleDirective(d, static_cast(response)); // ADL dispatches to the correct handle function + }, rDirective); +}