From d9788b70fc0e0d03c77122ec41c0276b3dc13fe6 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sun, 12 Apr 2020 16:12:51 +0100 Subject: [PATCH] Remove enabled field from PluginMetadata It doesn't actually do anything, and fixing that turns out to be very complicated. Since nobody appears to have noticed, just remove the field entirely. --- docs/metadata/data_structures/plugin.rst | 7 -- include/loot/metadata/plugin_metadata.h | 21 +---- src/api/metadata/condition_evaluator.cpp | 1 - src/api/metadata/plugin_metadata.cpp | 14 +-- src/api/metadata/yaml/plugin_metadata.h | 9 -- .../internals/metadata/plugin_metadata_test.h | 90 ------------------- 6 files changed, 5 insertions(+), 137 deletions(-) diff --git a/docs/metadata/data_structures/plugin.rst b/docs/metadata/data_structures/plugin.rst index d85379f8..15e8c23e 100644 --- a/docs/metadata/data_structures/plugin.rst +++ b/docs/metadata/data_structures/plugin.rst @@ -11,12 +11,6 @@ This is the structure that brings all the others together, and forms the main co Regular expression plugin filenames must be written in `modified ECMAScript `_ syntax. -.. describe:: enabled - - ``boolean`` - - Enables or disables use of the plugin object. Used for user rules, but no reason to use it in the masterlist. If unspecified, defaults to ``true``. - .. describe:: group ``string`` @@ -120,7 +114,6 @@ Merging Behaviour Key Merge Behaviour (merging B into A) =============== ================================== name Not merged. -enabled Replaced by B's value. group Replaced by B's value only if A has no value set. after Merged. If B's file set contains an item that is equal to one already present in A's file set, B's item is discarded. req Merged. If B's file set contains an item that is equal to one already present in A's file set, B's item is discarded. diff --git a/include/loot/metadata/plugin_metadata.h b/include/loot/metadata/plugin_metadata.h index 91191817..04023e77 100644 --- a/include/loot/metadata/plugin_metadata.h +++ b/include/loot/metadata/plugin_metadata.h @@ -64,9 +64,8 @@ public: * Merge metadata from the given PluginMetadata object into this object. * * If an equal metadata object already exists in this PluginMetadata object, - * it is not duplicated. This object's enabled state is replaced by the given - * object's state. This object's group is replaced by the given object's group - * if the latter is explicit. + * it is not duplicated. This object's group is replaced by the given object's + * group if the latter is explicit. * @param plugin * The plugin metadata to merge. */ @@ -79,7 +78,7 @@ public: * The PluginMetadata object to compare against. * @return A PluginMetadata object containing the metadata in this object that * is not in the given object. The returned object inherits this - * object's enabled state and group. + * object's group. */ LOOT_API PluginMetadata NewMetadata(const PluginMetadata& plugin) const; @@ -89,12 +88,6 @@ public: */ LOOT_API std::string GetName() const; - /** - * Check if the plugin metadata is enabled for use during sorting. - * @return True if the metadata will be used during sorting, false otherwise. - */ - LOOT_API bool IsEnabled() const; - /** * Get the plugin's group. * @return An optional containing the name of the group this plugin belongs to @@ -160,13 +153,6 @@ public: LOOT_API std::vector GetSimpleMessages( const std::string& language) const; - /** - * Set whether the plugin metadata is enabled for use during sorting or not. - * @param enabled - * The value to set. - */ - LOOT_API void SetEnabled(const bool enabled); - /** * Set the plugin's group. * @param group @@ -281,7 +267,6 @@ public: private: std::string name_; - bool enabled_; std::optional group_; std::set loadAfter_; std::set requirements_; diff --git a/src/api/metadata/condition_evaluator.cpp b/src/api/metadata/condition_evaluator.cpp index 8068efbd..89177f94 100644 --- a/src/api/metadata/condition_evaluator.cpp +++ b/src/api/metadata/condition_evaluator.cpp @@ -121,7 +121,6 @@ bool ConditionEvaluator::Evaluate(const std::string& condition) { PluginMetadata ConditionEvaluator::EvaluateAll(const PluginMetadata& pluginMetadata) { PluginMetadata evaluatedMetadata(pluginMetadata.GetName()); - evaluatedMetadata.SetEnabled(pluginMetadata.IsEnabled()); evaluatedMetadata.SetLocations(pluginMetadata.GetLocations()); if (pluginMetadata.GetGroup()) { diff --git a/src/api/metadata/plugin_metadata.cpp b/src/api/metadata/plugin_metadata.cpp index 8a1be51f..29eb11f7 100644 --- a/src/api/metadata/plugin_metadata.cpp +++ b/src/api/metadata/plugin_metadata.cpp @@ -40,12 +40,10 @@ using std::set; using std::vector; namespace loot { -PluginMetadata::PluginMetadata() : - enabled_(true) {} +PluginMetadata::PluginMetadata() {} PluginMetadata::PluginMetadata(const std::string& n) : - name_(n), - enabled_(true) { + name_(n) { // If the name passed ends in '.ghost', that should be trimmed. if (boost::iends_with(name_, ".ghost")) name_ = name_.substr(0, name_.length() - 6); @@ -55,10 +53,6 @@ void PluginMetadata::MergeMetadata(const PluginMetadata& plugin) { if (plugin.HasNameOnly()) return; - // For 'enabled' and 'group' metadata, use the given plugin's values, - // but if the 'group' value is not explicit, ignore it. - enabled_ = plugin.IsEnabled(); - if (!group_.has_value() && plugin.GetGroup()) { group_ = plugin.GetGroup(); } @@ -171,8 +165,6 @@ PluginMetadata PluginMetadata::NewMetadata(const PluginMetadata& plugin) const { std::string PluginMetadata::GetName() const { return name_; } -bool PluginMetadata::IsEnabled() const { return enabled_; } - std::optional PluginMetadata::GetGroup() const { return group_; } std::set PluginMetadata::GetLoadAfterFiles() const { return loadAfter_; } @@ -210,8 +202,6 @@ std::vector PluginMetadata::GetSimpleMessages( return simpleMessages; } -void PluginMetadata::SetEnabled(const bool e) { enabled_ = e; } - void PluginMetadata::SetGroup(const std::string& group) { group_ = group; } diff --git a/src/api/metadata/yaml/plugin_metadata.h b/src/api/metadata/yaml/plugin_metadata.h index 3b5060e1..508947e8 100644 --- a/src/api/metadata/yaml/plugin_metadata.h +++ b/src/api/metadata/yaml/plugin_metadata.h @@ -52,9 +52,6 @@ struct convert { Node node; node["name"] = rhs.GetName(); - if (!rhs.IsEnabled()) - node["enabled"] = rhs.IsEnabled(); - if (rhs.GetGroup()) node["group"] = rhs.GetGroup().value(); @@ -102,9 +99,6 @@ struct convert { } } - if (node["enabled"]) - rhs.SetEnabled(node["enabled"].as()); - if (node["group"]) rhs.SetGroup(node["group"].as()); @@ -138,9 +132,6 @@ inline Emitter& operator<<(Emitter& out, const loot::PluginMetadata& rhs) { out << BeginMap << Key << "name" << Value << YAML::SingleQuoted << rhs.GetName(); - if (!rhs.IsEnabled()) - out << Key << "enabled" << Value << rhs.IsEnabled(); - if (rhs.GetGroup()) out << Key << "group" << Value << YAML::SingleQuoted << rhs.GetGroup().value(); diff --git a/src/tests/api/internals/metadata/plugin_metadata_test.h b/src/tests/api/internals/metadata/plugin_metadata_test.h index 714390e3..38dae6fd 100644 --- a/src/tests/api/internals/metadata/plugin_metadata_test.h +++ b/src/tests/api/internals/metadata/plugin_metadata_test.h @@ -55,7 +55,6 @@ TEST_P( PluginMetadata plugin; EXPECT_TRUE(plugin.GetName().empty()); - EXPECT_TRUE(plugin.IsEnabled()); EXPECT_FALSE(plugin.GetGroup()); } @@ -65,7 +64,6 @@ TEST_P( PluginMetadata plugin(blankEsm); EXPECT_EQ(blankEsm, plugin.GetName()); - EXPECT_TRUE(plugin.IsEnabled()); EXPECT_FALSE(plugin.GetGroup()); } @@ -115,31 +113,6 @@ TEST_P(PluginMetadataTest, mergeMetadataShouldNotChangeName) { EXPECT_EQ(blankEsm, plugin1.GetName()); } -TEST_P(PluginMetadataTest, - mergeMetadataShouldNotUseMergedEnabledStateIfMergedMetadataIsEmpty) { - PluginMetadata plugin1; - PluginMetadata plugin2; - - plugin2.SetEnabled(false); - ASSERT_TRUE(plugin2.HasNameOnly()); - plugin1.MergeMetadata(plugin2); - - EXPECT_TRUE(plugin1.IsEnabled()); -} - -TEST_P(PluginMetadataTest, - mergeMetadataShouldUseMergedEnabledStateIfMergedMetadataIsNotEmpty) { - PluginMetadata plugin1; - PluginMetadata plugin2; - - plugin2.SetEnabled(false); - plugin2.SetGroup("group1"); - ASSERT_FALSE(plugin2.HasNameOnly()); - plugin1.MergeMetadata(plugin2); - - EXPECT_FALSE(plugin1.IsEnabled()); -} - TEST_P(PluginMetadataTest, mergeMetadataShouldNotUseMergedGroupIfItAndCurrentGroupAreBothExplicit) { PluginMetadata plugin1; @@ -300,21 +273,6 @@ TEST_P(PluginMetadataTest, newMetadataShouldUseSourcePluginName) { EXPECT_EQ(blankEsm, newMetadata.GetName()); } -TEST_P(PluginMetadataTest, newMetadataShouldUseSourcePluginEnabledState) { - PluginMetadata plugin1; - PluginMetadata plugin2; - - plugin2.SetEnabled(false); - PluginMetadata newMetadata = plugin1.NewMetadata(plugin2); - - EXPECT_TRUE(newMetadata.IsEnabled()); - - plugin1.SetEnabled(false); - newMetadata = plugin1.NewMetadata(plugin2); - - EXPECT_FALSE(newMetadata.IsEnabled()); -} - TEST_P(PluginMetadataTest, newMetadataShouldUseSourcePluginGroupExplicitlyIfItIsExplicit) { PluginMetadata plugin1; @@ -529,14 +487,6 @@ TEST_P(PluginMetadataTest, EXPECT_TRUE(plugin.HasNameOnly()); } -TEST_P(PluginMetadataTest, - hasNameOnlyShouldBeTrueIfThePluginMetadataIsDisabled) { - PluginMetadata plugin(blankEsp); - plugin.SetEnabled(false); - - EXPECT_TRUE(plugin.HasNameOnly()); -} - TEST_P(PluginMetadataTest, hasNameOnlyShouldBeFalseIfTheGroupIsExplicit) { PluginMetadata plugin; plugin.SetGroup("group"); @@ -687,35 +637,6 @@ TEST_P(PluginMetadataTest, emitter.c_str()); } -TEST_P( - PluginMetadataTest, - emittingAsYamlShouldOutputAPluginThatIsDisabledAndIsNotNameOnlyCorrectly) { - PluginMetadata plugin(blankEsm); - plugin.SetGroup("group1"); - plugin.SetEnabled(false); - - YAML::Emitter emitter; - emitter << plugin; - - EXPECT_STREQ( - "name: 'Blank.esm'\n" - "enabled: false\n" - "group: 'group1'", - emitter.c_str()); -} - -TEST_P( - PluginMetadataTest, - emittingAsYamlShouldOutputAPluginThatIsDisabledAndIsNameOnlyAsAnEmptyString) { - PluginMetadata plugin(blankEsm); - plugin.SetEnabled(false); - - YAML::Emitter emitter; - emitter << plugin; - - EXPECT_STREQ("", emitter.c_str()); -} - TEST_P(PluginMetadataTest, emittingAsYamlShouldOutputAPluginWithLoadAfterMetadataCorrectly) { PluginMetadata plugin(blankEsp); @@ -847,7 +768,6 @@ TEST_P(PluginMetadataTest, encodingAsYamlShouldOmitAllUnsetFields) { node = plugin; EXPECT_EQ(plugin.GetName(), node["name"].as()); - EXPECT_FALSE(node["enabled"]); EXPECT_FALSE(node["after"]); EXPECT_FALSE(node["req"]); EXPECT_FALSE(node["inc"]); @@ -858,15 +778,6 @@ TEST_P(PluginMetadataTest, encodingAsYamlShouldOmitAllUnsetFields) { EXPECT_FALSE(node["url"]); } -TEST_P(PluginMetadataTest, encodingAsYamlShouldSetEnabledFieldIfItIsFalse) { - PluginMetadata plugin(blankEsp); - plugin.SetEnabled(false); - YAML::Node node; - node = plugin; - - EXPECT_FALSE(node["enabled"].as()); -} - TEST_P(PluginMetadataTest, encodingAsYamlShouldSetAfterFieldIfLoadAfterMetadataExists) { PluginMetadata plugin(blankEsp); @@ -946,7 +857,6 @@ TEST_P(PluginMetadataTest, encodingAsYamlShouldSetUrlFieldIfLocationsExist) { TEST_P(PluginMetadataTest, decodingFromYamlShouldStoreAllGivenData) { YAML::Node node = YAML::Load( "name: 'Blank.esp'\n" - "enabled: false\n" "after:\n" " - 'Blank.esm'\n" "req:\n"