ExtensionSystem: Split update into remove and install

Change-Id: Ib048d5c889bb5ac463309ea15486e3f226d84a2e
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/app/main.cpp

@@ -839,6 +839,8 @@ int main(int argc, char **argv)
     // Make sure we honor the system's proxy settings
     QNetworkProxyFactory::setUseSystemConfiguration(true);
 
+    PluginManager::removePluginsAfterRestart();
+
     // We need to install plugins before we scan for them.
     PluginManager::installPluginsAfterRestart();
 
@@ -847,9 +849,6 @@ int main(int argc, char **argv)
     PluginManager::setPluginPaths(
         getPluginPaths() + Utils::transform(pluginPaths, &FilePath::fromUserInput));
 
-    // We need to remove plugins once we have scanned for them.
-    PluginManager::removePluginsAfterRestart();
-
     QMap<QString, QString> foundAppOptions;
     if (pluginArguments.size() > 1) {
         QMap<QString, bool> appOptions;

src/libs/extensionsystem/pluginmanager.cpp

@@ -357,15 +357,15 @@ void PluginManager::installPluginsAfterRestart()
     d->installPluginsAfterRestart();
 }
 
-void PluginManager::removePluginOnRestart(const QString &id)
+Result PluginManager::removePluginOnRestart(const QString &id)
 {
-    d->removePluginOnNextRestart(id);
+    return d->removePluginOnRestart(id);
 }
 
 void PluginManager::installPluginOnRestart(
     const Utils::FilePath &source, const Utils::FilePath &destination)
 {
-    d->installPluginOnNextRestart(source, destination);
+    d->installPluginOnRestart(source, destination);
 }
 
 /*!
@@ -1710,11 +1710,6 @@ PluginSpec *PluginManager::specForPlugin(IPlugin *plugin)
     return findOrDefault(d->pluginSpecs, equal(&PluginSpec::plugin, plugin));
 }
 
-bool PluginManager::takePluginIdForRemoval(const QString &id)
-{
-    return d->m_pluginsToRemove.remove(id);
-}
-
 static QString pluginListString(const QSet<PluginSpec *> &plugins)
 {
     QStringList names = Utils::transform<QList>(plugins, &PluginSpec::name);
@@ -1925,36 +1920,6 @@ static const FilePaths pluginFiles(const FilePaths &pluginPaths)
     return pluginFiles;
 }
 
-bool PluginManagerPrivate::removePlugin(const QString &pluginId)
-{
-    PluginSpec *existingSpec
-        = Utils::findOrDefault(pluginSpecs, Utils::equal(&PluginSpec::id, pluginId));
-
-    if (existingSpec) {
-        QTC_ASSERT(existingSpec->state() == PluginSpec::State::Resolved, return false);
-
-        const Result removeResult = existingSpec->removePluginFiles();
-        if (!removeResult) {
-            qCWarning(pluginLog) << "Failed to remove plugin files for" << pluginId << ":"
-                                 << removeResult.error();
-            return false;
-        }
-
-        for (QList<PluginSpec *> &category : pluginCategories) {
-            category.removeOne(existingSpec);
-        }
-
-        if (pluginSpecs.removeOne(existingSpec)) {
-            delete existingSpec;
-            return true;
-        }
-    }
-
-    // This is used by other plugin managers like Lua that is not loaded yet.
-    m_pluginsToRemove << pluginId;
-    return true;
-}
-
 void PluginManagerPrivate::addPlugins(const PluginSpecs &specs)
 {
     pluginSpecs += specs;
@@ -1987,11 +1952,24 @@ void PluginManagerPrivate::addPlugins(const PluginSpecs &specs)
 static const char PLUGINS_TO_INSTALL_KEY[] = "PluginsToInstall";
 static const char PLUGINS_TO_REMOVE_KEY[] = "PluginsToRemove";
 
-void PluginManagerPrivate::removePluginOnNextRestart(const QString &pluginId)
+Result PluginManagerPrivate::removePluginOnRestart(const QString &pluginId)
 {
-    settings->setValue(
+    const PluginSpec *pluginSpec
-        PLUGINS_TO_REMOVE_KEY, settings->value(PLUGINS_TO_REMOVE_KEY).toStringList() << pluginId);
+        = findOrDefault(pluginSpecs, Utils::equal(&PluginSpec::id, pluginId));
+
+    if (!pluginSpec)
+        return Result::Error(Tr::tr("Plugin not found."));
+
+    const expected_str<FilePaths> filePaths = pluginSpec->filesToUninstall();
+    if (!filePaths)
+        return Result::Error(filePaths.error());
+
+    const QVariantList list = Utils::transform(*filePaths, &FilePath::toVariant);
+
+    settings->setValue(PLUGINS_TO_REMOVE_KEY, settings->value(PLUGINS_TO_REMOVE_KEY).toList() + list);
+
     settings->sync();
+    return Result::Ok;
 }
 
 static QList<QPair<FilePath, FilePath>> readPluginInstallList(QtcSettings *settings)
@@ -2009,7 +1987,7 @@ static QList<QPair<FilePath, FilePath>> readPluginInstallList(QtcSettings *setti
     return installList;
 }
 
-void PluginManagerPrivate::installPluginOnNextRestart(
+void PluginManagerPrivate::installPluginOnRestart(
     const Utils::FilePath &src, const Utils::FilePath &dest)
 {
     const QList<QPair<FilePath, FilePath>> list = readPluginInstallList(settings)
@@ -2028,9 +2006,19 @@ void PluginManagerPrivate::installPluginOnNextRestart(
 
 void PluginManagerPrivate::removePluginsAfterRestart()
 {
-    const QStringList removeList = settings->value(PLUGINS_TO_REMOVE_KEY).toStringList();
+    const FilePaths removeList
-    for (const QString &pluginId : removeList)
+        = Utils::transform(settings->value(PLUGINS_TO_REMOVE_KEY).toList(), &FilePath::fromVariant);
-        removePlugin(pluginId);
+
+    for (const FilePath &path : removeList) {
+        Result r = Result::Error(Tr::tr("It does not exist."));
+        if (path.isFile())
+            r = path.removeFile();
+        else if (path.isDir())
+            r = path.removeRecursively();
+
+        if (!r)
+            qCWarning(pluginLog()) << "Failed to remove" << path << ":" << r.error();
+    }
 
     settings->remove(PLUGINS_TO_REMOVE_KEY);
 }

src/libs/extensionsystem/pluginmanager.h

@@ -80,13 +80,12 @@ public:
     static const QSet<PluginSpec *> pluginsRequiredByPlugin(PluginSpec *spec);
     static void checkForProblematicPlugins();
     static PluginSpec *specForPlugin(IPlugin *plugin);
-    static bool takePluginIdForRemoval(const QString &id);
 
     static void addPlugins(const QVector<PluginSpec *> &specs);
 
     static void reInstallPlugins();
 
-    static void removePluginOnRestart(const QString &id);
+    static Utils::Result removePluginOnRestart(const QString &id);
     static void installPluginOnRestart(
         const Utils::FilePath &source, const Utils::FilePath &destination);
 

src/libs/extensionsystem/pluginmanager_p.h

@@ -52,8 +52,6 @@ public:
     void loadPluginsAtRuntime(const QSet<PluginSpec *> &plugins);
     void addPlugins(const QVector<PluginSpec *> &specs);
 
-    bool removePlugin(const QString &pluginId);
-
     void shutdown();
     void setPluginPaths(const Utils::FilePaths &paths);
     const QVector<ExtensionSystem::PluginSpec *> loadQueue();
@@ -77,8 +75,8 @@ public:
     void removePluginsAfterRestart();
     void installPluginsAfterRestart();
 
-    void removePluginOnNextRestart(const QString &pluginId);
+    Utils::Result removePluginOnRestart(const QString &pluginId);
-    void installPluginOnNextRestart(const Utils::FilePath &src, const Utils::FilePath &dest);
+    void installPluginOnRestart(const Utils::FilePath &src, const Utils::FilePath &dest);
 
     class TestSpec {
     public:
@@ -152,8 +150,6 @@ public:
 
     PluginManager::ProcessData m_creatorProcessData;
 
-    QSet<QString> m_pluginsToRemove;
-
 private:
     PluginManager *q;
 

src/libs/extensionsystem/pluginspec.cpp

@@ -1453,27 +1453,27 @@ QList<PluginSpec *> pluginSpecsFromArchive(const Utils::FilePath &path)
     return results;
 }
 
-Utils::Result PluginSpec::removePluginFiles() const
+expected_str<FilePaths> PluginSpec::filesToUninstall() const
 {
     if (isSystemPlugin())
-        return Result::Error(Tr::tr("Cannot remove system plugins."));
+        return make_unexpected(Tr::tr("Cannot remove system plugins."));
 
     // Try to figure out where we are ...
-    const Utils::FilePaths pluginPaths = PluginManager::pluginPaths();
+    const FilePaths pluginPaths = PluginManager::pluginPaths();
 
     for (const FilePath &pluginPath : pluginPaths) {
         if (location().isChildOf(pluginPath)) {
             const FilePath rootFolder = location().relativeChildPath(pluginPath);
             if (rootFolder.isEmpty())
-                return Result::Error(Tr::tr("Could not determine root folder."));
+                return make_unexpected(Tr::tr("Could not determine root folder."));
 
             const FilePath pathToDelete = pluginPath
                                           / rootFolder.pathComponents().first().toString();
-            return pathToDelete.removeRecursively();
+            return FilePaths{pathToDelete};
         }
     }
 
-    return filePath().removeFile();
+    return FilePaths{filePath()};
 }
 
 bool PluginSpec::isSystemPlugin() const

src/libs/extensionsystem/pluginspec.h

@@ -161,7 +161,7 @@ public:
 
     virtual Utils::FilePath installLocation(bool inUserFolder) const = 0;
 
-    virtual Utils::Result removePluginFiles() const;
+    virtual Utils::expected_str<Utils::FilePaths> filesToUninstall() const;
     virtual bool isSystemPlugin() const;
 
 protected:

src/plugins/coreplugin/plugininstallwizard.cpp

@@ -515,6 +515,14 @@ InstallResult executePluginInstallWizard(const FilePath &archive, bool prepareFo
                                          / extensionId(data.pluginSpec.get());
 
             if (prepareForUpdate) {
+                const Result result = ExtensionSystem::PluginManager::removePluginOnRestart(
+                    data.pluginSpec->id());
+                if (!result) {
+                    qWarning() << "Failed to remove plugin" << data.pluginSpec->id() << ":"
+                               << result.error();
+                    return false;
+                }
+
                 if (hasLibSuffix(data.sourcePath)) {
                     ExtensionSystem::PluginManager::installPluginOnRestart(
                         data.pluginSpec->filePath(), installPath);

src/plugins/lua/luaplugin.cpp

@@ -446,19 +446,6 @@ public:
                     continue;
                 }
 
-                if (PluginManager::takePluginIdForRemoval((*result)->id())) {
-                    auto removeResult = (*result)->removePluginFiles();
-                    if (!removeResult) {
-                        qWarning() << "Failed to remove plugin files" << script << ":"
-                                   << removeResult.error();
-                        MessageManager::writeFlashing(
-                            Tr::tr("Failed to remove plugin files of %1: %2")
-                                .arg((*result)->id())
-                                .arg(removeResult.error()));
-                    }
-                    continue;
-                }
-
                 plugins.insert(*result);
             }
         }