From 4a16931c5d3e990839cd197af67115315bbf9f2b Mon Sep 17 00:00:00 2001 From: Nick Bolton Date: Fri, 22 Aug 2025 10:33:05 +0100 Subject: [PATCH] refactor: Remove QProcessProxy and simplify CoreProcess Making a proxy class for QProcess to make GTest work is an excellent example of over-abstraction making simple things hard to do. I just wanted to call a function on QProcess and had to jump through hoops to do it. Bye bye QProcessProxy! We can do much better with QTest. --- src/lib/gui/CMakeLists.txt | 2 - src/lib/gui/core/CoreProcess.cpp | 62 ++++---- src/lib/gui/core/CoreProcess.h | 26 +--- src/lib/gui/proxy/QProcessProxy.cpp | 65 --------- src/lib/gui/proxy/QProcessProxy.h | 38 ----- src/unittests/legacytests/CMakeLists.txt | 5 - .../legacytests/gui/core/CoreProcessTests.cpp | 133 ------------------ 7 files changed, 31 insertions(+), 300 deletions(-) delete mode 100644 src/lib/gui/proxy/QProcessProxy.cpp delete mode 100644 src/lib/gui/proxy/QProcessProxy.h delete mode 100644 src/unittests/legacytests/legacytests/gui/core/CoreProcessTests.cpp diff --git a/src/lib/gui/CMakeLists.txt b/src/lib/gui/CMakeLists.txt index 01bf8cfb7..af34c88f8 100644 --- a/src/lib/gui/CMakeLists.txt +++ b/src/lib/gui/CMakeLists.txt @@ -88,8 +88,6 @@ add_library(${target} STATIC dialogs/SettingsDialog.ui ipc/DaemonIpcClient.cpp ipc/DaemonIpcClient.h - proxy/QProcessProxy.cpp - proxy/QProcessProxy.h tls/TlsCertificate.cpp tls/TlsCertificate.h tls/TlsUtility.cpp diff --git a/src/lib/gui/core/CoreProcess.cpp b/src/lib/gui/core/CoreProcess.cpp index 42b6d2675..1f6827470 100644 --- a/src/lib/gui/core/CoreProcess.cpp +++ b/src/lib/gui/core/CoreProcess.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -116,28 +117,18 @@ QString wrapIpv6(const QString &address) return address; } -// -// CoreProcess::Deps -// - -QString CoreProcess::Deps::appPath(const QString &name) const +QString getAppFilePath(const QString &name) { QDir dir(QCoreApplication::applicationDirPath()); return dir.filePath(name); } -bool CoreProcess::Deps::fileExists(const QString &path) const -{ - return QFile::exists(path); -} - // // CoreProcess // -CoreProcess::CoreProcess(const IServerConfig &serverConfig, std::shared_ptr deps) +CoreProcess::CoreProcess(const IServerConfig &serverConfig) : m_serverConfig(serverConfig), - m_pDeps(deps), m_daemonIpcClient{new ipc::DaemonIpcClient(this)} { connect(m_daemonIpcClient, &ipc::DaemonIpcClient::connected, this, &CoreProcess::daemonIpcClientConnected); @@ -145,16 +136,6 @@ CoreProcess::CoreProcess(const IServerConfig &serverConfig, std::shared_ptrprocess(), &QProcessProxy::finished, this, &CoreProcess::onProcessFinished); - - connect( - &m_pDeps->process(), &QProcessProxy::readyReadStandardOutput, this, &CoreProcess::onProcessReadyReadStandardOutput - ); - - connect( - &m_pDeps->process(), &QProcessProxy::readyReadStandardError, this, &CoreProcess::onProcessReadyReadStandardError - ); - connect(&m_retryTimer, &QTimer::timeout, this, [this] { if (m_processState == ProcessState::RetryPending) { start(); @@ -166,15 +147,15 @@ CoreProcess::CoreProcess(const IServerConfig &serverConfig, std::shared_ptrprocess()) { - handleLogLines(m_pDeps->process().readAllStandardOutput()); + if (m_pProcess) { + handleLogLines(m_pProcess->readAllStandardOutput()); } } void CoreProcess::onProcessReadyReadStandardError() { - if (m_pDeps->process()) { - handleLogLines(m_pDeps->process().readAllStandardError()); + if (m_pProcess) { + handleLogLines(m_pProcess->readAllStandardError()); } } @@ -250,9 +231,9 @@ void CoreProcess::startForegroundProcess(const QString &app, const QStringList & const auto quoted = makeQuotedArgs(app, args); qInfo("running command: %s", qPrintable(quoted)); - m_pDeps->process().start(app, args); + m_pProcess->start(app, args); - if (m_pDeps->process().waitForStarted()) { + if (m_pProcess->waitForStarted()) { setProcessState(Started); } else { setProcessState(Stopped); @@ -284,15 +265,15 @@ void CoreProcess::stopForegroundProcess() const qFatal("core process must be in stopping state"); } - if (!m_pDeps->process()) { + if (!m_pProcess) { qFatal("process not set, cannot stop"); } qInfo("stopping core desktop process"); - if (m_pDeps->process().state() == QProcess::ProcessState::Running) { + if (m_pProcess->state() == QProcess::ProcessState::Running) { qDebug("process is running, closing"); - m_pDeps->process().close(); + m_pProcess->close(); } else { qDebug("process is not running, skipping terminate"); } @@ -365,7 +346,16 @@ void CoreProcess::start(std::optional processModeOption) setConnectionState(ConnectionState::Connecting); if (processMode == ProcessMode::Desktop) { - m_pDeps->process().create(); + m_pProcess = new QProcess(this); + connect(m_pProcess, &QProcess::finished, this, &CoreProcess::onProcessFinished, Qt::UniqueConnection); + connect( + m_pProcess, &QProcess::readyReadStandardOutput, this, &CoreProcess::onProcessReadyReadStandardOutput, + Qt::UniqueConnection + ); + connect( + m_pProcess, &QProcess::readyReadStandardError, this, &CoreProcess::onProcessReadyReadStandardError, + Qt::UniqueConnection + ); } QString app; @@ -489,9 +479,9 @@ bool CoreProcess::addGenericArgs(QStringList &args, const ProcessMode processMod bool CoreProcess::addServerArgs(QStringList &args, QString &app) { - app = m_pDeps->appPath(Settings::value(Settings::Server::Binary).toString()); + app = getAppFilePath(Settings::value(Settings::Server::Binary).toString()); - if (!m_pDeps->fileExists(app)) { + if (!QFile::exists(app)) { qFatal("core server binary does not exist"); return false; } @@ -535,9 +525,9 @@ bool CoreProcess::addServerArgs(QStringList &args, QString &app) bool CoreProcess::addClientArgs(QStringList &args, QString &app) { - app = m_pDeps->appPath(Settings::value(Settings::Client::Binary).toString()); + app = getAppFilePath(Settings::value(Settings::Client::Binary).toString()); - if (!m_pDeps->fileExists(app)) { + if (!QFile::exists(app)) { qFatal("core client binary does not exist"); return false; } diff --git a/src/lib/gui/core/CoreProcess.h b/src/lib/gui/core/CoreProcess.h index e2b1b1643..b9122919c 100644 --- a/src/lib/gui/core/CoreProcess.h +++ b/src/lib/gui/core/CoreProcess.h @@ -9,13 +9,11 @@ #include "common/Settings.h" #include "gui/FileTail.h" #include "gui/config/IServerConfig.h" -#include "gui/proxy/QProcessProxy.h" - -#include #include #include #include +#include #include #include #include @@ -30,25 +28,10 @@ class CoreProcess : public QObject { using ProcessMode = Settings::ProcessMode; using IServerConfig = deskflow::gui::IServerConfig; - using QProcessProxy = deskflow::gui::proxy::QProcessProxy; Q_OBJECT public: - struct Deps - { - virtual ~Deps() = default; - virtual QProcessProxy &process() - { - return m_process; - } - virtual QString appPath(const QString &name) const; - virtual bool fileExists(const QString &path) const; - - private: - QProcessProxy m_process; - }; - enum class Error { AddressMissing, @@ -70,7 +53,7 @@ public: Listening }; - explicit CoreProcess(const IServerConfig &serverConfig, std::shared_ptr deps = std::make_shared()); + explicit CoreProcess(const IServerConfig &serverConfig); void extracted(QString &app, QStringList &args); void start(std::optional processMode = std::nullopt); @@ -154,18 +137,19 @@ private: #endif const IServerConfig &m_serverConfig; - std::shared_ptr m_pDeps; QString m_address; ProcessState m_processState = ProcessState::Stopped; ConnectionState m_connectionState = ConnectionState::Disconnected; Settings::CoreMode m_mode = Settings::CoreMode::None; QMutex m_processMutex; - QString m_secureSocketVersion = ""; + QString m_secureSocketVersion; std::optional m_lastProcessMode = std::nullopt; QTimer m_retryTimer; int m_connections = 0; deskflow::gui::ipc::DaemonIpcClient *m_daemonIpcClient = nullptr; FileTail *m_daemonFileTail = nullptr; + QProcess *m_pProcess = nullptr; + QString m_appPath; }; } // namespace deskflow::gui diff --git a/src/lib/gui/proxy/QProcessProxy.cpp b/src/lib/gui/proxy/QProcessProxy.cpp deleted file mode 100644 index c753d365a..000000000 --- a/src/lib/gui/proxy/QProcessProxy.cpp +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Deskflow -- mouse and keyboard sharing utility - * SPDX-FileCopyrightText: (C) 2024 Symless Ltd. - * SPDX-License-Identifier: GPL-2.0-only WITH LicenseRef-OpenSSL-Exception - */ - -#include "QProcessProxy.h" - -namespace deskflow::gui::proxy { - -void QProcessProxy::create() -{ - m_pProcess = std::make_unique(); - - connect(m_pProcess.get(), &QProcess::finished, this, [this](int exitCode, QProcess::ExitStatus exitStatus) { - Q_EMIT finished(exitCode, exitStatus); - }); - - connect( - m_pProcess.get(), &QProcess::readyReadStandardOutput, // - this, [this]() { Q_EMIT readyReadStandardOutput(); } - ); - - connect( - m_pProcess.get(), &QProcess::readyReadStandardError, // - this, [this]() { Q_EMIT readyReadStandardError(); } - ); -} - -QProcessProxy::operator bool() const -{ - return m_pProcess.get(); -} - -void QProcessProxy::start(const QString &program, const QStringList &arguments) -{ - m_pProcess->start(program, arguments); -} - -bool QProcessProxy::waitForStarted() -{ - return m_pProcess->waitForStarted(); -} - -QProcess::ProcessState QProcessProxy::state() const -{ - return m_pProcess->state(); -} - -void QProcessProxy::close() -{ - m_pProcess->close(); -} - -QString QProcessProxy::readAllStandardOutput() -{ - return m_pProcess->readAllStandardOutput(); -} - -QString QProcessProxy::readAllStandardError() -{ - return m_pProcess->readAllStandardError(); -} - -} // namespace deskflow::gui::proxy diff --git a/src/lib/gui/proxy/QProcessProxy.h b/src/lib/gui/proxy/QProcessProxy.h deleted file mode 100644 index f18b81e21..000000000 --- a/src/lib/gui/proxy/QProcessProxy.h +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Deskflow -- mouse and keyboard sharing utility - * SPDX-FileCopyrightText: (C) 2024 Symless Ltd. - * SPDX-License-Identifier: GPL-2.0-only WITH LicenseRef-OpenSSL-Exception - */ - -#pragma once - -#include -#include - -namespace deskflow::gui::proxy { - -class QProcessProxy : public QObject -{ - Q_OBJECT - -public: - ~QProcessProxy() override = default; - explicit virtual operator bool() const; - virtual void create(); - virtual void start(const QString &program, const QStringList &arguments = {}); - virtual bool waitForStarted(); - virtual QProcess::ProcessState state() const; - virtual void close(); - virtual QString readAllStandardOutput(); - virtual QString readAllStandardError(); - -Q_SIGNALS: - void finished(int exitCode, QProcess::ExitStatus exitStatus); - void readyReadStandardOutput(); - void readyReadStandardError(); - -private: - std::unique_ptr m_pProcess; -}; - -} // namespace deskflow::gui::proxy diff --git a/src/unittests/legacytests/CMakeLists.txt b/src/unittests/legacytests/CMakeLists.txt index f65e7e43c..ce3f26c5c 100644 --- a/src/unittests/legacytests/CMakeLists.txt +++ b/src/unittests/legacytests/CMakeLists.txt @@ -62,11 +62,6 @@ macro(set_sources) list(APPEND sources ${PROJECT_SOURCE_DIR}/src/apps/res/deskflow.qrc) - # Hack this test is broken on CI ARM64 - if(WIN32 AND BUILD_ARCHITECTURE MATCHES "arm64") - list(REMOVE_ITEM sources ${test_base_dir}/legacytests/gui/core/CoreProcessTests.cpp) - endif() - replace_platform_sources() replace_arch_sources() diff --git a/src/unittests/legacytests/legacytests/gui/core/CoreProcessTests.cpp b/src/unittests/legacytests/legacytests/gui/core/CoreProcessTests.cpp deleted file mode 100644 index 3ac2fca03..000000000 --- a/src/unittests/legacytests/legacytests/gui/core/CoreProcessTests.cpp +++ /dev/null @@ -1,133 +0,0 @@ -/* - * Deskflow -- mouse and keyboard sharing utility - * SPDX-FileCopyrightText: (C) 2024 Symless Ltd. - * SPDX-License-Identifier: GPL-2.0-only WITH LicenseRef-OpenSSL-Exception - */ - -#include "common/Settings.h" -#include "gui/core/CoreProcess.h" -#include "gui/proxy/QProcessProxy.h" -#include "unittests/legacytests/shared/gui/mocks/ServerConfigMock.h" - -#include "gmock/gmock.h" -#include -#include - -using namespace deskflow::gui; -using namespace testing; - -namespace { - -class QProcessProxyMock : public proxy::QProcessProxy -{ -public: - operator bool() const override - { - return toBool(); - } - - QProcessProxyMock() - { - ON_CALL(*this, toBool()).WillByDefault(Return(true)); - ON_CALL(*this, state()).WillByDefault(Return(QProcess::ProcessState::Running)); - ON_CALL(*this, waitForStarted()).WillByDefault(Return(true)); - } - - MOCK_METHOD(bool, toBool, (), (const)); - MOCK_METHOD(void, create, (), (override)); - MOCK_METHOD(void, start, (const QString &program, const QStringList &arguments), (override)); - MOCK_METHOD(bool, waitForStarted, (), (override)); - MOCK_METHOD(QProcess::ProcessState, state, (), (const, override)); - MOCK_METHOD(void, close, (), (override)); - MOCK_METHOD(QString, readAllStandardOutput, (), (override)); - MOCK_METHOD(QString, readAllStandardError, (), (override)); -}; - -class DepsMock : public CoreProcess::Deps -{ -public: - DepsMock() - { - ON_CALL(*this, process()).WillByDefault(ReturnRef(m_process)); - ON_CALL(*this, appPath(_)).WillByDefault(Return("stub app path")); - ON_CALL(*this, fileExists(_)).WillByDefault(Return(true)); - } - - MOCK_METHOD(proxy::QProcessProxy &, process, (), (override)); - MOCK_METHOD(QString, appPath, (const QString &name), (const, override)); - MOCK_METHOD(bool, fileExists, (const QString &path), (const, override)); - - NiceMock m_process; -}; - -class CoreProcessTests : public Test -{ -public: - CoreProcessTests() : m_coreProcess(m_serverConfig, m_pDeps) - { - Settings::setValue(Settings::Server::ExternalConfig, true); - Settings::setValue(Settings::Server::ExternalConfigFile, m_configFile); - Settings::setValue(Settings::Core::ProcessMode, Settings::ProcessMode::Desktop); - } - - NiceMock m_serverConfig; - std::shared_ptr> m_pDeps = std::make_shared>(); - CoreProcess m_coreProcess; - -private: - const QString m_configFile = "tmp/deskflow-server.conf"; -}; - -} // namespace - -TEST_F(CoreProcessTests, start_serverDesktop_callsProcessStart) -{ - m_coreProcess.setMode(Settings::CoreMode::Server); - - EXPECT_CALL(m_pDeps->m_process, start(_, _)).Times(1); - - m_coreProcess.start(Settings::ProcessMode::Desktop); -} - -TEST_F(CoreProcessTests, start_clientDesktop_callsProcessStart) -{ - m_coreProcess.setMode(Settings::CoreMode::Client); - m_coreProcess.setAddress("stub address"); - - EXPECT_CALL(m_pDeps->m_process, start(_, _)).Times(1); - - m_coreProcess.start(Settings::ProcessMode::Desktop); -} - -TEST_F(CoreProcessTests, stop_serverDesktop_callsProcessClose) -{ - m_coreProcess.setMode(Settings::CoreMode::Server); - m_coreProcess.start(); - - EXPECT_CALL(m_pDeps->m_process, close()).Times(1); - - m_coreProcess.stop(Settings::ProcessMode::Desktop); -} - -TEST_F(CoreProcessTests, stop_clientDesktop_callsProcessClose) -{ - m_coreProcess.setMode(Settings::CoreMode::Client); - m_coreProcess.setAddress("stub address"); - m_coreProcess.start(); - - EXPECT_CALL(m_pDeps->m_process, close()).Times(1); - - m_coreProcess.stop(Settings::ProcessMode::Desktop); -} - -TEST_F(CoreProcessTests, restart_serverDesktop_callsProcessStart) -{ - Settings::setValue(Settings::Core::ProcessMode, Settings::ProcessMode::Desktop); - m_coreProcess.setMode(Settings::CoreMode::Server); - m_coreProcess.start(); - - EXPECT_CALL(m_pDeps->m_process, close()).Times(1); - EXPECT_CALL(m_pDeps->m_process, start(_, _)).Times(1); - - m_coreProcess.restart(); -}