Refactor maintenance tool writing conditions

This change fixes a bug caused by a blocking runInstaller() call
in PerformInstallationPage::entering() and also contains refactoring
to related parts of code.

Change-Id: Ic3309707c1f975a646937aa96fc407a3e5931359
Reviewed-by: Katja Marttila <katja.marttila@qt.io>

10 files changed, 87 insertions, 53 deletions

diff --git a/src/libs/installer/packagemanagercore.cpp b/src/libs/installer/packagemanagercore.cpp
index 913666068..d34f608cd 100644
--- a/src/libs/installer/packagemanagercore.cpp
+++ b/src/libs/installer/packagemanagercore.cpp
@@ -434,9 +434,13 @@ static bool componentMatches(const Component *component, const QString &name,
 */
 void PackageManagerCore::writeMaintenanceTool()
 {
+    if (d->m_disableWriteMaintenanceTool) {
+        qCDebug(QInstaller::lcInstallerInstallLog()) << "Maintenance tool writing disabled.";
+        return;
+    }
+
     if (d->m_needToWriteMaintenanceTool) {
         try {
-            emit titleMessageChanged(tr("Creating Maintenance Tool"));
             d->writeMaintenanceTool(d->m_performedOperationsOld + d->m_performedOperationsCurrentSession);
 
             bool gainedAdminRights = false;
@@ -448,14 +452,6 @@ void PackageManagerCore::writeMaintenanceTool()
             if (gainedAdminRights)
                 dropAdminRights();
             d->m_needToWriteMaintenanceTool = false;
-
-            // fake a possible wrong value to show a full progress bar
-            const int progress = ProgressCoordinator::instance()->progressInPercentage();
-            // usually this should be only the reserved one from the beginning
-            if (progress < 100)
-                ProgressCoordinator::instance()->addManualPercentagePoints(100 - progress);
-            ProgressCoordinator::instance()->emitLabelAndDetailTextChanged(tr("\nInstallation finished!"));
-            d->setStatus(PackageManagerCore::Success);
         } catch (const Error &error) {
             qCritical() << "Error writing Maintenance Tool: " << error.message();
             MessageBoxHandler::critical(MessageBoxHandler::currentBestSuitParent(),
@@ -474,6 +470,15 @@ void PackageManagerCore::writeMaintenanceConfigFiles()
 }
 
 /*!
+    Disables writing of maintenance tool for the current session.
+    \sa {installer::disableWriteMaintenanceTool}{installer.disableWriteMaintenanceTool}
+ */
+void PackageManagerCore::disableWriteMaintenanceTool(bool disable)
+{
+    d->m_disableWriteMaintenanceTool = disable;
+}
+
+/*!
     Resets the class to its initial state.
 */
 void PackageManagerCore::reset()
@@ -2247,8 +2252,8 @@ void PackageManagerCore::listInstalledPackages()
 /*!
     Updates the selected components \a componentsToUpdate without GUI.
     If essential components are found, then only those will be updated.
-    Returns \c true if components are updated and the maintenance tool needs
-    to be written, otherwise returns \c false.
+    Returns \c true if components are updated successfully or there are
+    no updates to perform, otherwise returns \c false.
 */
 bool PackageManagerCore::updateComponentsSilently(const QStringList &componentsToUpdate)
 {
@@ -2264,7 +2269,6 @@ bool PackageManagerCore::updateComponentsSilently(const QStringList &componentsT
 
     if (componentList.count() ==  0) {
         qCDebug(QInstaller::lcInstallerInstallLog) << "No updates available.";
-        return false;
     } else {
         // Check if essential components are available (essential components are disabled).
         // If essential components are found, update first essential updates,
@@ -2296,7 +2300,7 @@ bool PackageManagerCore::updateComponentsSilently(const QStringList &componentsT
             if (userSelectedComponents && componentsToBeUpdated.isEmpty()) {
                 qCDebug(QInstaller::lcInstallerInstallLog)
                     << "No updates available for selected components.";
-                return false;
+                return true;
             }
             foreach (Component *componentToUpdate, componentsToBeUpdated) {
                 const QModelIndex &idx = model->indexFromComponentName(componentToUpdate->name());
@@ -2304,12 +2308,13 @@ bool PackageManagerCore::updateComponentsSilently(const QStringList &componentsT
             }
         }
 
-        if (d->calculateComponentsAndRun()) {
-            if (essentialUpdatesFound)
-                qCDebug(QInstaller::lcInstallerInstallLog) << "Essential components updated successfully.";
-            else
-                qCDebug(QInstaller::lcInstallerInstallLog) << "Components updated successfully.";
-        }
+        if (!d->calculateComponentsAndRun())
+            return false;
+
+        if (essentialUpdatesFound)
+            qCDebug(QInstaller::lcInstallerInstallLog) << "Essential components updated successfully.";
+        else
+            qCDebug(QInstaller::lcInstallerInstallLog) << "Components updated successfully.";
     }
     return true;
 }
@@ -2321,8 +2326,8 @@ void PackageManagerCore::commitSessionOperations()
 
 /*!
     Uninstalls the selected components \a components without GUI.
-    Returns \c true if components are uninstalled and the maintenance tool
-    needs to be written, otherwise returns \c false.
+    Returns \c true if components are uninstalled successfully or
+    there are no components to uninstall, otherwise returns \c false.
 */
 bool PackageManagerCore::uninstallComponentsSilently(const QStringList& components)
 {
@@ -2349,12 +2354,12 @@ bool PackageManagerCore::uninstallComponentsSilently(const QStringList& componen
     }
 
     if (uninstallComponentFound) {
-        if (d->calculateComponentsAndRun()) {
-            qCDebug(QInstaller::lcInstallerUninstallLog) << "Components uninstalled successfully";
-            return true;
-        }
+        if (!d->calculateComponentsAndRun())
+            return false;
+
+        qCDebug(QInstaller::lcInstallerUninstallLog) << "Components uninstalled successfully";
     }
-    return false;
+    return true;
 }
 
 /*!
@@ -2375,8 +2380,8 @@ bool PackageManagerCore::removeInstallationSilently()
     Installs the selected components \a components without displaying a user
     interface. Virtual components cannot be installed unless made visible with
     --show-virtual-components. AutoDependOn nor non-checkable components cannot
-    be installed directly. Returns \c true if components are installed and the
-    maintenance tool needs to be written, otherwise returns \c false.
+    be installed directly. Returns \c true if components are installed or there
+    is nothing to install, otherwise returns \c false.
 */
 bool PackageManagerCore::installSelectedComponentsSilently(const QStringList& components)
 {
@@ -2416,12 +2421,12 @@ bool PackageManagerCore::installSelectedComponentsSilently(const QStringList& co
         }
     }
     if (installComponentsFound) {
-        if (d->calculateComponentsAndRun()) {
-            qCDebug(QInstaller::lcInstallerInstallLog) << "Components installed successfully";
-            return true;
-        }
+        if (!d->calculateComponentsAndRun())
+            return false;
+
+        qCDebug(QInstaller::lcInstallerInstallLog) << "Components installed successfully";
     }
-    return false;
+    return true;
 }
 
 /*!
diff --git a/src/libs/installer/packagemanagercore.h b/src/libs/installer/packagemanagercore.h
index e188ed34d..27fb20b0e 100644
--- a/src/libs/installer/packagemanagercore.h
+++ b/src/libs/installer/packagemanagercore.h
@@ -172,6 +172,8 @@ public:
     void writeMaintenanceTool();
     void writeMaintenanceConfigFiles();
 
+    void disableWriteMaintenanceTool(bool disable = true);
+
     QString maintenanceToolName() const;
     QString installerBinaryPath() const;
 
diff --git a/src/libs/installer/packagemanagercore_p.cpp b/src/libs/installer/packagemanagercore_p.cpp
index 2f78b5d00..5fdba4bd0 100644
--- a/src/libs/installer/packagemanagercore_p.cpp
+++ b/src/libs/installer/packagemanagercore_p.cpp
@@ -220,6 +220,7 @@ PackageManagerCorePrivate::PackageManagerCorePrivate(PackageManagerCore *core)
     , m_userSetBinaryMarker(false)
     , m_checkAvailableSpace(true)
     , m_autoAcceptLicenses(false)
+    , m_disableWriteMaintenanceTool(false)
 {
 }
 
@@ -255,6 +256,7 @@ PackageManagerCorePrivate::PackageManagerCorePrivate(PackageManagerCore *core, q
     , m_userSetBinaryMarker(false)
     , m_checkAvailableSpace(true)
     , m_autoAcceptLicenses(false)
+    , m_disableWriteMaintenanceTool(false)
 {
     foreach (const OperationBlob &operation, performedOperations) {
         QScopedPointer<QInstaller::Operation> op(KDUpdater::UpdateOperationFactory::instance()
@@ -1178,6 +1180,11 @@ void PackageManagerCorePrivate::writeMaintenanceToolBinaryData(QFileDevice *outp
 
 void PackageManagerCorePrivate::writeMaintenanceTool(OperationList performedOperations)
 {
+    if (m_disableWriteMaintenanceTool) {
+        qCDebug(QInstaller::lcInstallerInstallLog()) << "Maintenance tool writing disabled.";
+        return;
+    }
+
     bool gainedAdminRights = false;
     if (!directoryWritable(targetDir())) {
         m_core->gainAdminRights();
@@ -1610,7 +1617,24 @@ bool PackageManagerCorePrivate::runInstaller()
                 }
             }
         }
+        emit m_core->titleMessageChanged(tr("Creating Maintenance Tool"));
+
         m_needToWriteMaintenanceTool = true;
+        m_core->writeMaintenanceTool();
+
+        // fake a possible wrong value to show a full progress bar
+        const int progress = ProgressCoordinator::instance()->progressInPercentage();
+        // usually this should be only the reserved one from the beginning
+        if (progress < 100)
+            ProgressCoordinator::instance()->addManualPercentagePoints(100 - progress);
+
+        ProgressCoordinator::instance()->emitLabelAndDetailTextChanged(tr("\nInstallation finished!"));
+
+        if (adminRightsGained)
+            m_core->dropAdminRights();
+
+        setStatus(PackageManagerCore::Success);
+        emit installationFinished();
     } catch (const Error &err) {
         if (m_core->status() != PackageManagerCore::Canceled) {
             setStatus(PackageManagerCore::Failure);
@@ -2510,14 +2534,17 @@ bool PackageManagerCorePrivate::calculateComponentsAndRun()
 {
     QString htmlOutput;
     bool componentsOk = m_core->calculateComponents(&htmlOutput);
-    bool success = false;
     if (statusCanceledOrFailed()) {
         qCDebug(QInstaller::lcInstallerInstallLog) << "Installation canceled.";
     } else if (componentsOk && acceptLicenseAgreements()) {
         qCDebug(QInstaller::lcInstallerInstallLog).noquote() << htmlToString(htmlOutput);
-        success = m_core->run();
+        if (m_core->run()) {
+            // Write maintenance tool if required
+            m_core->writeMaintenanceTool();
+            return true;
+        }
     }
-    return success;
+    return false;
 }
 
 bool PackageManagerCorePrivate::acceptLicenseAgreements() const
diff --git a/src/libs/installer/packagemanagercore_p.h b/src/libs/installer/packagemanagercore_p.h
index 8e221fe44..d75a4a0ce 100644
--- a/src/libs/installer/packagemanagercore_p.h
+++ b/src/libs/installer/packagemanagercore_p.h
@@ -207,6 +207,7 @@ public:
     bool m_dependsOnLocalInstallerBinary;
     QStringList m_allowedRunningProcesses;
     bool m_autoAcceptLicenses;
+    bool m_disableWriteMaintenanceTool;
 
 private slots:
     void infoMessage(Job *, const QString &message) {
diff --git a/src/libs/installer/packagemanagergui.cpp b/src/libs/installer/packagemanagergui.cpp
index 751076f9a..e921ab978 100644
--- a/src/libs/installer/packagemanagergui.cpp
+++ b/src/libs/installer/packagemanagergui.cpp
@@ -2553,10 +2553,7 @@ void PerformInstallationPage::entering()
         setButtonText(QWizard::CommitButton, tr("&Install"));
         setColoredTitle(tr("Installing %1").arg(productName()));
 
-        if (packageManagerCore()->runInstaller()) {
-            packageManagerCore()->writeMaintenanceTool();
-            emit packageManagerCore()->installationFinished();
-        }
+        QTimer::singleShot(30, packageManagerCore(), SLOT(runInstaller()));
     }
 }
 
diff --git a/src/sdk/commandlineinterface.cpp b/src/sdk/commandlineinterface.cpp
index 0ddf973d2..75ab19cd5 100644
--- a/src/sdk/commandlineinterface.cpp
+++ b/src/sdk/commandlineinterface.cpp
@@ -151,9 +151,8 @@ int CommandLineInterface::updatePackages()
     if (!checkLicense())
         return EXIT_FAILURE;
     try {
-        if (m_core->updateComponentsSilently(m_positionalArguments))
-            m_core->writeMaintenanceTool();
-        return EXIT_SUCCESS;
+        return m_core->updateComponentsSilently(m_positionalArguments)
+            ? EXIT_SUCCESS : EXIT_FAILURE;
     } catch (const QInstaller::Error &err) {
         qCCritical(QInstaller::lcInstallerInstallLog) << err.message();
         return EXIT_FAILURE;
@@ -172,12 +171,12 @@ int CommandLineInterface::installPackages()
                 return EXIT_FAILURE;
             }
             // No packages provided, install default components
-            if (m_core->installDefaultComponentsSilently())
-                m_core->writeMaintenanceTool();
-        } else if (m_core->installSelectedComponentsSilently(m_positionalArguments)) {
-            m_core->writeMaintenanceTool();
+            return m_core->installDefaultComponentsSilently()
+                ? EXIT_SUCCESS : EXIT_FAILURE;
         }
-        return EXIT_SUCCESS;
+        // Normal installation
+        return m_core->installSelectedComponentsSilently(m_positionalArguments)
+            ? EXIT_SUCCESS : EXIT_FAILURE;
     } catch (const QInstaller::Error &err) {
         qCCritical(QInstaller::lcInstallerInstallLog) << err.message();
         return EXIT_FAILURE;
@@ -194,9 +193,8 @@ int CommandLineInterface::uninstallPackages()
     }
     m_core->setPackageManager();
     try {
-        if (m_core->uninstallComponentsSilently(m_positionalArguments))
-            m_core->writeMaintenanceTool();
-        return EXIT_SUCCESS;
+        return m_core->uninstallComponentsSilently(m_positionalArguments)
+            ? EXIT_SUCCESS : EXIT_FAILURE;
     } catch (const QInstaller::Error &err) {
         qCCritical(QInstaller::lcInstallerInstallLog) << err.message();
         return EXIT_FAILURE;
diff --git a/tests/auto/installer/commandlineupdate/tst_commandlineupdate.cpp b/tests/auto/installer/commandlineupdate/tst_commandlineupdate.cpp
index e260ab175..8d223ae09 100644
--- a/tests/auto/installer/commandlineupdate/tst_commandlineupdate.cpp
+++ b/tests/auto/installer/commandlineupdate/tst_commandlineupdate.cpp
@@ -90,7 +90,8 @@ private slots:
     void testUpdateNoUpdatesForSelectedPackage()
     {
         setRepository(":///data/installPackagesRepositoryUpdate");
-        QVERIFY(!core->updateComponentsSilently(QStringList() << "componentInvalid"));
+        // Succeeds as no updates available for component so nothing to do
+        QVERIFY(core->updateComponentsSilently(QStringList() << "componentInvalid"));
     }
 
     void testUpdateTwoPackageSilently()
diff --git a/tests/auto/installer/createshortcutoperation/tst_createshortcutoperation.cpp b/tests/auto/installer/createshortcutoperation/tst_createshortcutoperation.cpp
index f3c1132f3..a8826124e 100644
--- a/tests/auto/installer/createshortcutoperation/tst_createshortcutoperation.cpp
+++ b/tests/auto/installer/createshortcutoperation/tst_createshortcutoperation.cpp
@@ -70,6 +70,7 @@ private slots:
                                                           QString(), Protocol::DefaultAuthorizationKey, Protocol::Mode::Production,
                                                           QHash<QString, QString>(), true);
         core->setAllowedRunningProcesses(QStringList() << QCoreApplication::applicationFilePath());
+        core->disableWriteMaintenanceTool();
         QSet<Repository> repoList;
         Repository repo = Repository::fromUserInput(":///data/repository");
         repoList.insert(repo);
diff --git a/tests/auto/installer/messageboxhandler/tst_messageboxhandler.cpp b/tests/auto/installer/messageboxhandler/tst_messageboxhandler.cpp
index 9dc3367f8..83d93d647 100644
--- a/tests/auto/installer/messageboxhandler/tst_messageboxhandler.cpp
+++ b/tests/auto/installer/messageboxhandler/tst_messageboxhandler.cpp
@@ -96,6 +96,7 @@ private slots:
                                       QString(), Protocol::DefaultAuthorizationKey, Protocol::Mode::Production,
                                       QHash<QString, QString>(), true);
         core->setAllowedRunningProcesses(QStringList() << QCoreApplication::applicationFilePath());
+        core->disableWriteMaintenanceTool();
         m_installDir = QInstaller::generateTemporaryFileName();
         QVERIFY(QDir().mkpath(m_installDir));
         core->setValue(scTargetDir, m_installDir);
@@ -162,7 +163,7 @@ private slots:
         setRepository(":///data/invalidoperation");
         core->autoAcceptMessageBoxes();
         core->installDefaultComponentsSilently();
-        QCOMPARE(PackageManagerCore::Running, core->status());
+        QCOMPARE(PackageManagerCore::Success, core->status());
     }
 
     void invalidHashAutoReject()
diff --git a/tests/auto/installer/shared/packagemanager.h b/tests/auto/installer/shared/packagemanager.h
index 762edc61f..a62f13484 100644
--- a/tests/auto/installer/shared/packagemanager.h
+++ b/tests/auto/installer/shared/packagemanager.h
@@ -46,6 +46,7 @@ struct PackageManager
         PackageManagerCore *core = new PackageManagerCore(BinaryContent::MagicInstallerMarker, QList<OperationBlob> ());
         QString appFilePath = QCoreApplication::applicationFilePath();
         core->setAllowedRunningProcesses(QStringList() << appFilePath);
+        core->disableWriteMaintenanceTool();
         QSet<Repository> repoList;
         Repository repo = Repository::fromUserInput(repository);
         repoList.insert(repo);