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

@@ -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

src/lib/gui/core/CoreProcess.cpp

@@ -20,6 +20,7 @@
 #include <QFile>
 #include <QMessageBox>
 #include <QMutexLocker>
+#include <QProcess>
 #include <QRegularExpression>
 #include <QStandardPaths>
 #include <QTimer>
@@ -116,28 +117,18 @@ QString wrapIpv6(const QString &address)
   return address;
 }
 
-//
+QString getAppFilePath(const QString &name)
-// CoreProcess::Deps
-//
-
-QString CoreProcess::Deps::appPath(const QString &name) const
 {
   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> 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_ptr<Deps
       m_daemonIpcClient, &ipc::DaemonIpcClient::connectionFailed, this, &CoreProcess::daemonIpcClientConnectionFailed
   );
 
-  connect(&m_pDeps->process(), &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_ptr<Deps
 
 void CoreProcess::onProcessReadyReadStandardOutput()
 {
-  if (m_pDeps->process()) {
+  if (m_pProcess) {
-    handleLogLines(m_pDeps->process().readAllStandardOutput());
+    handleLogLines(m_pProcess->readAllStandardOutput());
   }
 }
 
 void CoreProcess::onProcessReadyReadStandardError()
 {
-  if (m_pDeps->process()) {
+  if (m_pProcess) {
-    handleLogLines(m_pDeps->process().readAllStandardError());
+    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<ProcessMode> 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;
   }

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 <memory>
 
 #include <QFileSystemWatcher>
 #include <QMutex>
 #include <QObject>
+#include <QProcess>
 #include <QString>
 #include <QStringList>
 #include <QTimer>
@@ -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> deps = std::make_shared<Deps>());
+  explicit CoreProcess(const IServerConfig &serverConfig);
 
   void extracted(QString &app, QStringList &args);
   void start(std::optional<ProcessMode> processMode = std::nullopt);
@@ -154,18 +137,19 @@ private:
 #endif
 
   const IServerConfig &m_serverConfig;
-  std::shared_ptr<Deps> 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<ProcessMode> 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

src/lib/gui/proxy/QProcessProxy.cpp

@@ -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<QProcess>();
-
-  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

src/lib/gui/proxy/QProcessProxy.h

@@ -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 <QObject>
-#include <QProcess>
-
-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<QProcess> m_pProcess;
-};
-
-} // namespace deskflow::gui::proxy

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()
 

src/unittests/legacytests/legacytests/gui/core/CoreProcessTests.cpp

@@ -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 <gmock/gmock.h>
-#include <gtest/gtest.h>
-
-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<QProcessProxyMock> 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<ServerConfigMock> m_serverConfig;
-  std::shared_ptr<NiceMock<DepsMock>> m_pDeps = std::make_shared<NiceMock<DepsMock>>();
-  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();
-}