PluginSpec: Naming and encapsulation improvements

Prefer enabled state as a property as opposed to disabled state, and
make setter correspond to getter. Also move setters into private.

Change-Id: I5d002a12f4e540d5b38cc5865490d056ec75f296
Reviewed-by: Daniel Teske <daniel.teske@theqtcompany.com>

src/libs/extensionsystem/optionsparser.cpp

@@ -30,6 +30,8 @@
 
 #include "optionsparser.h"
 
+#include "pluginspec_p.h"
+
 #include <QCoreApplication>
 
 using namespace ExtensionSystem;
@@ -151,7 +153,7 @@ bool OptionsParser::checkForLoadOption()
                     .arg(m_currentArg);
             m_hasError = true;
         } else {
-            spec->setForceEnabled(true);
+            spec->d->setForceEnabled(true);
             m_isDependencyRefreshNeeded = true;
         }
     }
@@ -170,7 +172,7 @@ bool OptionsParser::checkForNoLoadOption()
                                                              "The plugin \"%1\" does not exist.").arg(m_currentArg);
             m_hasError = true;
         } else {
-            spec->setForceDisabled(true);
+            spec->d->setForceDisabled(true);
             m_isDependencyRefreshNeeded = true;
         }
     }

src/libs/extensionsystem/pluginerroroverview.cpp

@@ -48,7 +48,7 @@ PluginErrorOverview::PluginErrorOverview(QWidget *parent) :
 
     foreach (PluginSpec *spec, PluginManager::plugins()) {
         // only show errors on startup if plugin is enabled.
-        if (spec->hasError() && spec->isEnabledInSettings() && !spec->isDisabledIndirectly()) {
+        if (spec->hasError() && spec->isEnabledBySettings() && !spec->isDisabledIndirectly()) {
             QListWidgetItem *item = new QListWidgetItem(spec->name());
             item->setData(Qt::UserRole, qVariantFromValue(spec));
             m_ui->pluginList->addItem(item);

src/libs/extensionsystem/pluginmanager.cpp

@@ -376,7 +376,7 @@ bool PluginManager::hasError()
 {
     foreach (PluginSpec *spec, plugins()) {
         // only show errors on startup if plugin is enabled.
-        if (spec->hasError() && spec->isEnabledInSettings() && !spec->isDisabledIndirectly())
+        if (spec->hasError() && spec->isEnabledBySettings() && !spec->isDisabledIndirectly())
             return true;
     }
     return false;
@@ -820,9 +820,9 @@ void PluginManagerPrivate::writeSettings()
     QStringList tempDisabledPlugins;
     QStringList tempForceEnabledPlugins;
     foreach (PluginSpec *spec, pluginSpecs) {
-        if (!spec->isDisabledByDefault() && !spec->isEnabledInSettings())
+        if (spec->isEnabledByDefault() && !spec->isEnabledBySettings())
             tempDisabledPlugins.append(spec->name());
-        if (spec->isDisabledByDefault() && spec->isEnabledInSettings())
+        if (!spec->isEnabledByDefault() && spec->isEnabledBySettings())
             tempForceEnabledPlugins.append(spec->name());
     }
 
@@ -1398,17 +1398,17 @@ void PluginManagerPrivate::readPluginPaths()
         }
         // defaultDisabledPlugins and defaultEnabledPlugins from install settings
         // is used to override the defaults read from the plugin spec
-        if (!spec->isDisabledByDefault() && defaultDisabledPlugins.contains(spec->name())) {
+        if (spec->isEnabledByDefault() && defaultDisabledPlugins.contains(spec->name())) {
-            spec->setDisabledByDefault(true);
+            spec->d->setEnabledByDefault(false);
-            spec->setEnabled(false);
+            spec->d->setEnabledBySettings(false);
-        } else if (spec->isDisabledByDefault() && defaultEnabledPlugins.contains(spec->name())) {
+        } else if (!spec->isEnabledByDefault() && defaultEnabledPlugins.contains(spec->name())) {
-            spec->setDisabledByDefault(false);
+            spec->d->setEnabledByDefault(true);
-            spec->setEnabled(true);
+            spec->d->setEnabledBySettings(true);
         }
-        if (spec->isDisabledByDefault() && forceEnabledPlugins.contains(spec->name()))
+        if (!spec->isEnabledByDefault() && forceEnabledPlugins.contains(spec->name()))
-            spec->setEnabled(true);
+            spec->d->setEnabledBySettings(true);
-        if (!spec->isDisabledByDefault() && disabledPlugins.contains(spec->name()))
+        if (spec->isEnabledByDefault() && disabledPlugins.contains(spec->name()))
-            spec->setEnabled(false);
+            spec->d->setEnabledBySettings(false);
 
         collection->addPlugin(spec);
         pluginSpecs.append(spec);
@@ -1452,10 +1452,10 @@ void PluginManagerPrivate::enableOnlyTestedSpecs()
         }
     }
     foreach (PluginSpec *spec, pluginSpecs)
-        spec->setForceDisabled(true);
+        spec->d->setForceDisabled(true);
     foreach (PluginSpec *spec, specsForTests) {
-        spec->setForceDisabled(false);
+        spec->d->setForceDisabled(false);
-        spec->setForceEnabled(true);
+        spec->d->setForceEnabled(true);
     }
 }
 

src/libs/extensionsystem/pluginspec.cpp

@@ -271,26 +271,25 @@ bool PluginSpec::isExperimental() const
 }
 
 /*!
-    Returns whether the plugin is disabled by default.
+    Returns whether the plugin is enabled by default.
-    This might be because the plugin is experimental, or because
+    A plugin might be disabled because the plugin is experimental, or because
-    the plugin manager's settings define it as disabled by default.
+    the install settings define it as disabled by default.
 */
-bool PluginSpec::isDisabledByDefault() const
+bool PluginSpec::isEnabledByDefault() const
 {
-    return d->disabledByDefault;
+    return d->enabledByDefault;
 }
 
 /*!
-    Returns whether the plugin should be loaded at startup. True by default.
+    Returns whether the plugin should be loaded at startup,
-
+    taking into account the default enabled state, and the user's settings.
-    The user can change it from the Plugin settings.
 
     \note This function returns true even if a plugin is disabled because its
     dependencies were not loaded, or an error occurred during loading it.
 */
-bool PluginSpec::isEnabledInSettings() const
+bool PluginSpec::isEnabledBySettings() const
 {
-    return d->enabledInSettings;
+    return d->enabledBySettings;
 }
 
 /*!
@@ -300,7 +299,7 @@ bool PluginSpec::isEnabledInSettings() const
 bool PluginSpec::isEffectivelyEnabled() const
 {
     if (d->disabledIndirectly
-        || (!d->enabledInSettings && !d->forceEnabled)
+        || (!d->enabledBySettings && !d->forceEnabled)
         || d->forceDisabled) {
         return false;
     }
@@ -485,8 +484,8 @@ namespace {
 PluginSpecPrivate::PluginSpecPrivate(PluginSpec *spec)
     : required(false),
       experimental(false),
-      disabledByDefault(false),
+      enabledByDefault(true),
-      enabledInSettings(true),
+      enabledBySettings(true),
       disabledIndirectly(false),
       forceEnabled(false),
       forceDisabled(false),
@@ -535,28 +534,28 @@ bool PluginSpecPrivate::read(const QString &fileName)
     return true;
 }
 
-void PluginSpec::setEnabled(bool value)
+void PluginSpecPrivate::setEnabledBySettings(bool value)
 {
-    d->enabledInSettings = value;
+    enabledBySettings = value;
 }
 
-void PluginSpec::setDisabledByDefault(bool value)
+void PluginSpecPrivate::setEnabledByDefault(bool value)
 {
-    d->disabledByDefault = value;
+    enabledByDefault = value;
 }
 
-void PluginSpec::setForceEnabled(bool value)
+void PluginSpecPrivate::setForceEnabled(bool value)
 {
-    d->forceEnabled = value;
+    forceEnabled = value;
     if (value)
-        d->forceDisabled = false;
+        forceDisabled = false;
 }
 
-void PluginSpec::setForceDisabled(bool value)
+void PluginSpecPrivate::setForceDisabled(bool value)
 {
     if (value)
-        d->forceEnabled = false;
+        forceEnabled = false;
-    d->forceDisabled = value;
+    forceDisabled = value;
 }
 
 /*!
@@ -687,12 +686,12 @@ bool PluginSpecPrivate::readMetaData(const QJsonObject &metaData)
     value = pluginInfo.value(QLatin1String(PLUGIN_DISABLED_BY_DEFAULT));
     if (!value.isUndefined() && !value.isBool())
         return reportError(msgValueIsNotABool(PLUGIN_DISABLED_BY_DEFAULT));
-    disabledByDefault = value.toBool(false);
+    enabledByDefault = !value.toBool(false);
-    qCDebug(pluginLog) << "disabledByDefault =" << disabledByDefault;
+    qCDebug(pluginLog) << "enabledByDefault =" << enabledByDefault;
 
     if (experimental)
-        disabledByDefault = true;
+        enabledByDefault = false;
-    enabledInSettings = !disabledByDefault;
+    enabledBySettings = enabledByDefault;
 
     value = pluginInfo.value(QLatin1String(VENDOR));
     if (!value.isUndefined() && !value.isString())
@@ -913,7 +912,7 @@ bool PluginSpecPrivate::resolveDependencies(const QList<PluginSpec *> &specs)
 
 void PluginSpecPrivate::disableIndirectlyIfDependencyDisabled()
 {
-    if (!enabledInSettings)
+    if (!enabledBySettings)
         return;
 
     if (disabledIndirectly)

src/libs/extensionsystem/pluginspec.h

@@ -45,11 +45,15 @@ QT_END_NAMESPACE
 namespace ExtensionSystem {
 
 namespace Internal {
-    class PluginSpecPrivate;
+
-    class PluginManagerPrivate;
+class OptionsParser;
-}
+class PluginSpecPrivate;
+class PluginManagerPrivate;
+
+} // Internal
 
 class IPlugin;
+class PluginItem;
 
 struct EXTENSIONSYSTEM_EXPORT PluginDependency
 {
@@ -97,8 +101,8 @@ public:
     bool isAvailableForHostPlatform() const;
     bool isRequired() const;
     bool isExperimental() const;
-    bool isDisabledByDefault() const;
+    bool isEnabledByDefault() const;
-    bool isEnabledInSettings() const;
+    bool isEnabledBySettings() const;
     bool isEffectivelyEnabled() const;
     bool isDisabledIndirectly() const;
     bool isForceEnabled() const;
@@ -112,11 +116,6 @@ public:
     QString location() const;
     QString filePath() const;
 
-    void setEnabled(bool value);
-    void setDisabledByDefault(bool value);
-    void setForceEnabled(bool value);
-    void setForceDisabled(bool value);
-
     QStringList arguments() const;
     void setArguments(const QStringList &arguments);
     void addArgument(const QString &argument);
@@ -138,6 +137,8 @@ private:
     PluginSpec();
 
     Internal::PluginSpecPrivate *d;
+    friend class PluginItem;
+    friend class Internal::OptionsParser;
     friend class Internal::PluginManagerPrivate;
     friend class Internal::PluginSpecPrivate;
 };

src/libs/extensionsystem/pluginspec_p.h

@@ -66,6 +66,11 @@ public:
     IPlugin::ShutdownFlag stop();
     void kill();
 
+    void setEnabledBySettings(bool value);
+    void setEnabledByDefault(bool value);
+    void setForceEnabled(bool value);
+    void setForceDisabled(bool value);
+
     QPluginLoader loader;
 
     QString name;
@@ -73,7 +78,7 @@ public:
     QString compatVersion;
     bool required;
     bool experimental;
-    bool disabledByDefault;
+    bool enabledByDefault;
     QString vendor;
     QString copyright;
     QString license;
@@ -82,7 +87,7 @@ public:
     QString category;
     QRegExp platformSpecification;
     QVector<PluginDependency> dependencies;
-    bool enabledInSettings;
+    bool enabledBySettings;
     bool disabledIndirectly;
     bool forceEnabled;
     bool forceDisabled;

src/libs/extensionsystem/pluginview.cpp

@@ -31,6 +31,7 @@
 #include "pluginview.h"
 #include "pluginmanager.h"
 #include "pluginspec.h"
+#include "pluginspec_p.h"
 #include "plugincollection.h"
 
 #include <utils/algorithm.h>
@@ -130,7 +131,7 @@ public:
                     return PluginView::tr("Plugin is required.");
             } else {
                 if (role == Qt::CheckStateRole)
-                    return m_spec->isEnabledInSettings() ? Qt::Checked : Qt::Unchecked;
+                    return m_spec->isEnabledBySettings() ? Qt::Checked : Qt::Unchecked;
                 if (role == Qt::ToolTipRole)
                     return PluginView::tr("Load on startup");
             }
@@ -153,7 +154,7 @@ public:
     bool setData(int column, const QVariant &data, int role)
     {
         if (column == LoadedColumn && role == Qt::CheckStateRole) {
-            m_spec->setEnabled(data.toBool());
+            m_spec->d->setEnabledBySettings(data.toBool());
             updateColumn(column);
             parent()->updateColumn(column);
             emit m_view->pluginSettingsChanged(m_spec);
@@ -167,7 +168,7 @@ public:
         if (m_spec->isRequired() || !m_spec->isAvailableForHostPlatform())
             return false;
         foreach (PluginSpec *spec, m_view->m_pluginDependencies.value(m_spec))
-            if (!spec->isEnabledInSettings())
+            if (!spec->isEnabledBySettings())
                 return false;
         return true;
     }
@@ -213,7 +214,7 @@ public:
                 foreach (PluginSpec *spec, m_plugins) {
                     if (spec->hasError())
                         return icon(ErrorIcon);
-                    if (!spec->isEnabledInSettings())
+                    if (!spec->isEnabledBySettings())
                         return icon(NotLoadedIcon);
                 }
                 return icon(OkIcon);
@@ -226,7 +227,7 @@ public:
             if (role == Qt::CheckStateRole) {
                 int checkedCount = 0;
                 foreach (PluginSpec *spec, m_plugins) {
-                    if (spec->isEnabledInSettings())
+                    if (spec->isEnabledBySettings())
                         ++checkedCount;
                 }