Squish: Extract common message handling

Change-Id: I078046e52fa2e3580bfa978ebe74303fb02f1513
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/squish/CMakeLists.txt

@@ -14,6 +14,7 @@ add_qtc_plugin(Squish
     scripthelper.cpp scripthelper.h
     squish.qrc
     squishfilehandler.cpp squishfilehandler.h
+    squishmessages.cpp squishmessages.h
     squishnavigationwidget.cpp squishnavigationwidget.h
     squishoutputpane.cpp squishoutputpane.h
     squishperspective.cpp squishperspective.h

src/plugins/squish/objectsmapeditorwidget.cpp

@@ -7,6 +7,7 @@
 #include "objectsmapdocument.h"
 #include "objectsmaptreeitem.h"
 #include "propertyitemdelegate.h"
+#include "squishmessages.h"
 #include "squishtr.h"
 #include "symbolnameitemdelegate.h"
 
@@ -25,7 +26,6 @@
 #include <QItemSelectionModel>
 #include <QLabel>
 #include <QMenu>
-#include <QMessageBox>
 #include <QMimeData>
 #include <QModelIndex>
 #include <QPushButton>
@@ -467,10 +467,8 @@ void ObjectsMapEditorWidget::onRemoveSymbolicNameTriggered()
         newReference = dialog.selectedSymbolicName();
     } else {
         // Squish does not ask for removing objects without references, but we prefer to do it
-        if (QMessageBox::question(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("Do you really want to remove \"%1\"?").arg(symbolicName);
-                                  Tr::tr("Remove Symbolic Name"),
+        if (SquishMessages::simpleQuestion(Tr::tr("Remove Symbolic Name"), detail) != QMessageBox::Yes)
-                                  Tr::tr("Do you really want to remove \"%1\"?").arg(symbolicName))
-            != QMessageBox::Yes)
             return;
     }
 

src/plugins/squish/squish.qbs

@@ -35,6 +35,8 @@ QtcPlugin {
         "squishconstants.h",
         "squishfilehandler.cpp",
         "squishfilehandler.h",
+        "squishmessages.cpp",
+        "squishmessages.h",
         "squishnavigationwidget.cpp",
         "squishnavigationwidget.h",
         "squishoutputpane.cpp",

src/plugins/squish/squishfilehandler.cpp

@@ -5,7 +5,7 @@
 
 #include "opensquishsuitesdialog.h"
 #include "squishconstants.h"
-#include "squishplugin.h"
+#include "squishmessages.h"
 #include "squishsettings.h"
 #include "squishtesttreemodel.h"
 #include "squishtools.h"
@@ -327,12 +327,10 @@ void SquishFileHandler::runTestCase(const QString &suiteName, const QString &tes
 
     const Utils::FilePath suitePath = m_suites.value(suiteName).parentDir();
     if (!suitePath.exists() || !suitePath.isReadableDir()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
-                              Tr::tr("Test Suite Path Not Accessible"),
-                              Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
                                       "Refusing to run test case \"%2\".")
-                                  .arg(suitePath.toUserOutput())
+                .arg(suitePath.toUserOutput()).arg(testCaseName);
-                                  .arg(testCaseName));
+        SquishMessages::criticalMessage(Tr::tr("Test Suite Path Not Accessible"), detail);
         return;
     }
 
@@ -350,11 +348,9 @@ void SquishFileHandler::runTestSuite(const QString &suiteName)
     const Utils::FilePath suiteConf = m_suites.value(suiteName);
     const Utils::FilePath suitePath = suiteConf.parentDir();
     if (!suitePath.exists() || !suitePath.isReadableDir()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
-                              Tr::tr("Test Suite Path Not Accessible"),
+                                      "Refusing to run test cases.").arg(suitePath.toUserOutput());
-                              Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
+        SquishMessages::criticalMessage(Tr::tr("Test Suite Path Not Accessible"), detail);
-                                     "Refusing to run test cases.")
-                                  .arg(suitePath.toUserOutput()));
         return;
     }
 
@@ -379,12 +375,10 @@ void SquishFileHandler::recordTestCase(const QString &suiteName, const QString &
 
     const Utils::FilePath suitePath = m_suites.value(suiteName).parentDir();
     if (!suitePath.exists() || !suitePath.isReadableDir()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
-                              Tr::tr("Test Suite Path Not Accessible"),
-                              Tr::tr("The path \"%1\" does not exist or is not accessible.\n"
                                       "Refusing to record test case \"%2\".")
-                                  .arg(suitePath.toUserOutput())
+                .arg(suitePath.toUserOutput()).arg(testCaseName);
-                                  .arg(testCaseName));
+        SquishMessages::criticalMessage(Tr::tr("Test Suite Path Not Accessible"), detail);
         return;
     }
 
@@ -491,9 +485,7 @@ void SquishFileHandler::openObjectsMap(const QString &suiteName)
     QTC_ASSERT(conf.ensureObjectMapExists(), return);
 
     if (!Core::EditorManager::openEditor(objectsMapPath, Constants::OBJECTSMAP_EDITOR_ID)) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::criticalMessage(Tr::tr("Failed to open objects.map file at \"%1\".")
-                              Tr::tr("Error"),
-                              Tr::tr("Failed to open objects.map file at \"%1\".")
                                         .arg(objectsMapPath.toUserOutput()));
     }
 }

src/plugins/squish/squishmessages.cpp

@@ -0,0 +1,35 @@
+// Copyright (C) 2022 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include "squishmessages.h"
+
+#include "squishtr.h"
+
+#include <coreplugin/icore.h>
+
+namespace Squish::Internal::SquishMessages {
+
+void criticalMessage(const QString &title, const QString &details)
+{
+    QMessageBox::critical(Core::ICore::dialogParent(), title, details);
+}
+
+void criticalMessage(const QString &details)
+{
+    criticalMessage(Tr::tr("Error"), details);
+}
+
+void toolsInUnexpectedState(int state, const QString &additionalDetails)
+{
+    QString details = Tr::tr("Squish Tools in unexpected state (%1).").arg(state);
+    if (!additionalDetails.isEmpty())
+        details.append('\n').append(additionalDetails);
+    criticalMessage(details);
+}
+
+QMessageBox::StandardButton simpleQuestion(const QString &title, const QString &detail)
+{
+    return QMessageBox::question(Core::ICore::dialogParent(), title, detail);
+}
+
+} // Squish::Internal::SquishMessages

src/plugins/squish/squishmessages.h

@@ -0,0 +1,15 @@
+// Copyright (C) 2022 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#pragma once
+
+#include <QMessageBox>
+
+namespace Squish::Internal::SquishMessages {
+
+void criticalMessage(const QString &title, const QString &details);
+void criticalMessage(const QString &details);
+void toolsInUnexpectedState(int state, const QString &additionalDetails);
+QMessageBox::StandardButton simpleQuestion(const QString &title, const QString &detail);
+
+} // Squish::Internal::SquishMessages

src/plugins/squish/squishnavigationwidget.cpp

@@ -5,6 +5,7 @@
 
 #include "squishconstants.h"
 #include "squishfilehandler.h"
+#include "squishmessages.h"
 #include "squishplugin.h"
 #include "squishsettings.h"
 #include "squishtesttreemodel.h"
@@ -22,7 +23,6 @@
 
 #include <QHeaderView>
 #include <QMenu>
-#include <QMessageBox>
 #include <QVBoxLayout>
 
 namespace Squish {
@@ -185,8 +185,7 @@ void SquishNavigationWidget::contextMenuEvent(QContextMenuEvent *event)
         menu.addAction(closeAllSuites);
 
         connect(closeAllSuites, &QAction::triggered, this, [this] {
-            if (QMessageBox::question(this,
+            if (SquishMessages::simpleQuestion(Tr::tr("Close All Test Suites"),
-                                      Tr::tr("Close All Test Suites"),
                                                Tr::tr("Close all test suites?"
                                                       /*"\nThis will close all related files as well."*/))
                 == QMessageBox::Yes)
@@ -273,13 +272,10 @@ void SquishNavigationWidget::onRemoveSharedFolderTriggered(int row, const QModel
     const auto folder = Utils::FilePath::fromVariant(m_sortModel->index(row, 0, parent).data(LinkRole));
     QTC_ASSERT(!folder.isEmpty(), return );
 
-    if (QMessageBox::question(Core::ICore::dialogParent(),
+    const QString detail = Tr::tr("Remove \"%1\" from the list of shared folders?")
-                              Tr::tr("Remove Shared Folder"),
+            .arg(folder.toUserOutput());
-                              Tr::tr("Remove \"%1\" from the list of shared folders?")
+    if (SquishMessages::simpleQuestion(Tr::tr("Remove Shared Folder"), detail) != QMessageBox::Yes)
-                                  .arg(folder.toUserOutput()))
-        != QMessageBox::Yes) {
         return;
-    }
 
     const QModelIndex &realIdx = m_sortModel->mapToSource(m_sortModel->index(row, 0, parent));
     if (SquishFileHandler::instance()->removeSharedFolder(folder))
@@ -288,10 +284,8 @@ void SquishNavigationWidget::onRemoveSharedFolderTriggered(int row, const QModel
 
 void SquishNavigationWidget::onRemoveAllSharedFolderTriggered()
 {
-    if (QMessageBox::question(Core::ICore::dialogParent(),
+    if (SquishMessages::simpleQuestion(Tr::tr("Remove All Shared Folders"),
-                              Tr::tr("Remove All Shared Folders"),
+                                       Tr::tr("Remove all shared folders?")) != QMessageBox::Yes) {
-                              Tr::tr("Remove all shared folders?"))
-        != QMessageBox::Yes) {
         return;
     }
 
@@ -320,9 +314,7 @@ void SquishNavigationWidget::onNewTestCaseTriggered(const QModelIndex &index)
     QTC_ASSERT(settings, return);
 
     if (!settings->squishPath.filePath().pathAppended("scriptmodules").exists()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::criticalMessage(Tr::tr("Set up a valid Squish path to be able to create "
-                              Tr::tr("Error"),
-                              Tr::tr("Set up a valid Squish path to be able to create "
                                                "a new test case.\n(Edit > Preferences > Squish)"));
         return;
     }

src/plugins/squish/squishsettings.cpp

@@ -4,6 +4,7 @@
 #include "squishsettings.h"
 
 #include "squishconstants.h"
+#include "squishmessages.h"
 #include "squishtools.h"
 #include "squishtr.h"
 
@@ -20,7 +21,6 @@
 #include <QDialogButtonBox>
 #include <QFrame>
 #include <QHeaderView>
-#include <QMessageBox>
 #include <QPushButton>
 #include <QSettings>
 #include <QVBoxLayout>
@@ -691,10 +691,9 @@ SquishServerSettingsDialog::SquishServerSettingsDialog(QWidget *parent)
 
 void SquishServerSettingsDialog::configWriteFailed(QProcess::ProcessError error)
 {
-    QMessageBox::critical(Core::ICore::dialogParent(),
+    const QString detail = Tr::tr("Failed to write configuration changes.\n"
-                          Tr::tr("Error"),
+                                  "Squish server finished with process error %1.").arg(error);
-                          Tr::tr("Failed to write configuration changes.\n"
+    SquishMessages::criticalMessage(detail);
-                                 "Squish server finished with process error %1.").arg(error));
 }
 
 } // namespace Internal

src/plugins/squish/squishtools.cpp

@@ -4,6 +4,7 @@
 #include "squishtools.h"
 
 #include "scripthelper.h"
+#include "squishmessages.h"
 #include "squishoutputpane.h"
 #include "squishplugin.h"
 #include "squishsettings.h"
@@ -31,7 +32,6 @@
 #include <QFile>
 #include <QFileSystemWatcher>
 #include <QLoggingCategory>
-#include <QMessageBox>
 #include <QTimer>
 #include <QWindow>
 
@@ -198,17 +198,12 @@ void SquishTools::runTestCases(const FilePath &suitePath,
     if (m_shutdownInitiated)
         return;
     if (m_state != Idle) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::toolsInUnexpectedState(m_state, Tr::tr("Refusing to run a test case."));
-                              Tr::tr("Error"),
-                              Tr::tr("Squish Tools in unexpected state (%1).\n"
-                                     "Refusing to run a test case.")
-                                .arg(m_state));
         return;
     }
     // create test results directory (if necessary) and return on fail
     if (!resultsDirectory.ensureWritableDir()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::criticalMessage(
-                              Tr::tr("Error"),
                     Tr::tr("Could not create test results folder. Canceling test run."));
         return;
     }
@@ -264,11 +259,7 @@ void SquishTools::queryServer(RunnerQuery query)
         return;
 
     if (m_state != Idle) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::toolsInUnexpectedState(m_state, Tr::tr("Refusing to execute server query."));
-                              Tr::tr("Error"),
-                              Tr::tr("Squish Tools in unexpected state (%1).\n"
-                                     "Refusing to execute server query.")
-                                  .arg(m_state));
         return;
     }
     m_perspective.setPerspectiveMode(SquishPerspective::Querying);
@@ -283,11 +274,7 @@ void SquishTools::recordTestCase(const FilePath &suitePath, const QString &testC
     if (m_shutdownInitiated)
         return;
     if (m_state != Idle) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::toolsInUnexpectedState(m_state, Tr::tr("Refusing to record a test case."));
-                              Tr::tr("Error"),
-                              Tr::tr("Squish Tools in unexpected state (%1).\n"
-                                     "Refusing to record a test case.")
-                                .arg(m_state));
         return;
     }
 
@@ -306,10 +293,7 @@ void SquishTools::writeServerSettingsChanges(const QList<QStringList> &changes)
     if (m_shutdownInitiated)
         return;
     if (m_state != Idle) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::toolsInUnexpectedState(m_state, Tr::tr("Refusing to write configuration changes."));
-                              Tr::tr("Error"),
-                              Tr::tr("Squish Tools in unexpected state (%1).\n"
-                                     "Refusing to write configuration changes.").arg(m_state));
         return;
     }
     m_serverConfigChanges = changes;
@@ -435,8 +419,7 @@ void SquishTools::onRunnerStateChanged(SquishProcessState state)
         break;
     case StartFailed:
         logAndChangeToolsState(SquishTools::RunnerStartFailed);
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        SquishMessages::criticalMessage(Tr::tr("Squish Runner Error"),
-                              Tr::tr("Squish Runner Error"),
                                         Tr::tr("Squish runner failed to start within given timeframe."));
         onRunnerStopped();
         break;
@@ -473,7 +456,7 @@ void SquishTools::onRunnerStopped()
                                                  m_suitePath.fileName(),
                                                  &error);
         if (!error.isEmpty())
-            QMessageBox::critical(Core::ICore::dialogParent(), Tr::tr("Error"), error);
+            SquishMessages::criticalMessage(error);
         logrotateTestResults();
     } else {
         m_xmlOutputHandler->clearForNextRun();
@@ -494,12 +477,10 @@ void SquishTools::onRunnerError(SquishRunnerProcess::RunnerError error)
         }
         break;
     case SquishRunnerProcess::MappedAutMissing:
-        QMessageBox::critical(Core::ICore::dialogParent(), Tr::tr("Error"),
+        SquishMessages::criticalMessage(
-                              Tr::tr("Squish could not find the AUT \"%1\" to start. "
+                    Tr::tr("Squish could not find the AUT \"%1\" to start. Make sure it has been "
-                                     "Make sure it has been added as a Mapped AUT in the "
+                           "added as a Mapped AUT in the squishserver settings.\n"
-                                     "squishserver settings.\n"
+                           "(Tools > Squish > Server Settings...)").arg(m_suiteConf.aut()));
-                                     "(Tools > Squish > Server Settings...)")
-                              .arg(m_suiteConf.aut()));
         break;
     }
 }
@@ -534,11 +515,9 @@ void SquishTools::startSquishServer(Request request)
     const FilePath squishServer = Environment::systemEnvironment().searchInPath(
         toolsSettings.serverPath.toString());
     if (!squishServer.isExecutableFile()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("\"%1\" could not be found or is not executable.\nCheck the "
-                              Tr::tr("Squish Server Error"),
+                                      "settings.").arg(toolsSettings.serverPath.toUserOutput());
-                              Tr::tr("\"%1\" could not be found or is not executable.\n"
+        SquishMessages::criticalMessage(Tr::tr("Squish Server Error"), detail);
-                                     "Check the settings.")
-                                  .arg(toolsSettings.serverPath.toUserOutput()));
         setIdle();
         return;
     }
@@ -1070,13 +1049,13 @@ void SquishTools::terminateRunner()
 
 void SquishTools::handleSquishServerAlreadyRunning()
 {
-    if (QMessageBox::question(Core::ICore::dialogParent(),
+    const QString detail = Tr::tr("There is still an old Squish server instance running.\n"
-                              Tr::tr("Squish Server Already Running"),
-                              Tr::tr("There is still an old Squish server instance running.\n"
                                   "This will cause problems later on.\n\n"
                                   "If you continue, the old instance will be terminated.\n"
-                                     "Do you want to continue?"))
+                                  "Do you want to continue?");
-        == QMessageBox::Yes) {
+    if (SquishMessages::simpleQuestion(Tr::tr("Squish Server Already Running"), detail) != QMessageBox::Yes)
+        return;
+
     switch (m_request) {
     case RunTestRequested:
         m_request = KillOldBeforeRunRunner;
@@ -1088,15 +1067,11 @@ void SquishTools::handleSquishServerAlreadyRunning()
         m_request = KillOldBeforeQueryRunner;
         break;
     default:
-            QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("Unexpected state or request while starting Squish server. "
-                                  Tr::tr("Error"),
+                                      "(state: %1, request: %2)").arg(m_state).arg(m_request);
-                                  Tr::tr("Unexpected state or request while starting Squish "
+        SquishMessages::criticalMessage(detail);
-                                         "server. (state: %1, request: %2)")
-                                      .arg(m_state)
-                                      .arg(m_request));
     }
     stopSquishServer();
-    }
 }
 
 QStringList SquishTools::serverArgumentsFromSettings() const
@@ -1165,37 +1140,26 @@ QStringList SquishTools::runnerArgumentsFromSettings()
 bool SquishTools::isValidToStartRunner()
 {
     if (!m_serverProcess.isRunning()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("Squish server does not seem to be running.\n(state: %1, "
-                              Tr::tr("No Squish Server"),
+                                      "request: %2)\nTry again.").arg(m_state).arg(m_request);
-                              Tr::tr("Squish server does not seem to be running.\n"
+        SquishMessages::criticalMessage(Tr::tr("No Squish Server"), detail);
-                                     "(state: %1, request: %2)\n"
-                                     "Try again.")
-                                  .arg(m_state)
-                                  .arg(m_request));
         setIdle();
         return false;
     }
     if (m_serverProcess.port() == -1) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("Failed to get the server port.\n(state: %1, request: %2)\n"
-                              Tr::tr("No Squish Server Port"),
+                                      "Try again.").arg(m_state).arg(m_request);
-                              Tr::tr("Failed to get the server port.\n"
+        SquishMessages::criticalMessage(Tr::tr("No Squish Server Port"), detail);
-                                     "(state: %1, request: %2)\n"
-                                     "Try again.")
-                                  .arg(m_state)
-                                  .arg(m_request));
         // setting state to ServerStartFailed will terminate/kill the current unusable server
         onServerStateChanged(StartFailed);
         return false;
     }
 
     if (m_primaryRunner && m_primaryRunner->state() != QProcess::NotRunning) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("Squish runner seems to be running already.\n(state: %1, "
-                              Tr::tr("Squish Runner Running"),
+                                      "request: %2)\nWait until it has finished and try again.")
-                              Tr::tr("Squish runner seems to be running already.\n"
+                .arg(m_state).arg(m_request);
-                                     "(state: %1, request: %2)\n"
+        SquishMessages::criticalMessage(Tr::tr("Squish Runner Running"), detail);
-                                     "Wait until it has finished and try again.")
-                                  .arg(m_state)
-                                  .arg(m_request));
         return false;
     }
     return true;
@@ -1206,11 +1170,9 @@ bool SquishTools::setupRunnerPath()
     const FilePath squishRunner = Environment::systemEnvironment().searchInPath(
         toolsSettings.runnerPath.toString());
     if (!squishRunner.isExecutableFile()) {
-        QMessageBox::critical(Core::ICore::dialogParent(),
+        const QString detail = Tr::tr("\"%1\" could not be found or is not executable.\nCheck the "
-                              Tr::tr("Squish Runner Error"),
+                                      "settings.").arg(toolsSettings.runnerPath.toUserOutput());
-                              Tr::tr("\"%1\" could not be found or is not executable.\n"
+        SquishMessages::criticalMessage(Tr::tr("Squish Runner Error"), detail);
-                                     "Check the settings.")
-                                  .arg(toolsSettings.runnerPath.toUserOutput()));
         onRunnerStateChanged(Stopped);
         return false;
     }