From fc226efce5b9d691ab58a3d022f9f82ee113f2b6 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Sat, 20 Aug 2016 13:47:42 +0100 Subject: [PATCH] Don't select messages' strings during condition eval This simplifies the API, clarifies the semantics of condition evaluation, and allows API users to query plugins for messages in several languages after condition evaluation (so they know the message is applicable). --- include/loot/database_interface.h | 8 +-- src/api/api_database.cpp | 6 +- src/api/api_database.h | 2 +- src/backend/metadata/message.cpp | 7 -- src/backend/metadata/message.h | 4 -- src/backend/metadata/plugin_metadata.cpp | 4 +- src/backend/metadata/plugin_metadata.h | 2 +- src/backend/metadata_list.cpp | 8 +-- src/backend/metadata_list.h | 2 +- src/backend/plugin/plugin_sorter.cpp | 2 +- src/gui/query_handler.cpp | 4 +- src/tests/api/database_interface_test.h | 26 +------ src/tests/backend/metadata/message_test.h | 67 ++----------------- .../backend/metadata/plugin_metadata_test.h | 2 +- src/tests/backend/metadata_list_test.h | 2 +- 15 files changed, 26 insertions(+), 120 deletions(-) diff --git a/include/loot/database_interface.h b/include/loot/database_interface.h index e29aa6c8..6cd2c11a 100644 --- a/include/loot/database_interface.h +++ b/include/loot/database_interface.h @@ -66,10 +66,8 @@ public: * @brief Evaluates all conditions and regular expression metadata entries. * @details Repeated calls re-evaluate the metadata from scratch. This * function affects the output of all the database access functions. - * @param language - * The language code that is used for message language comparisons. */ - virtual void EvalLists(const LanguageCode language) = 0; + virtual void EvalLists() = 0; /** * @} @@ -172,9 +170,7 @@ public: * The filename of the plugin to look up messages for. * @param language * The language to use when choosing which message content strings - * to return. This has no effect if `EvalLists` has been called, - * as it selects content strings, discarding non-selected strings, - * during its operation. + * to return. * @returns A vector of messages associated with the specified plugin. Empty * if the plugin has no messages associated with it. */ diff --git a/src/api/api_database.cpp b/src/api/api_database.cpp index 6e2706f6..24827149 100644 --- a/src/api/api_database.cpp +++ b/src/api/api_database.cpp @@ -67,7 +67,7 @@ void ApiDatabase::LoadLists(const std::string& masterlistPath, unevaluatedUserlist_ = userTemp; } -void ApiDatabase::EvalLists(const LanguageCode language) { +void ApiDatabase::EvalLists() { // Clear caches before evaluating conditions. game_.ClearCachedConditions(); @@ -75,8 +75,8 @@ void ApiDatabase::EvalLists(const LanguageCode language) { MetadataList userTemp = unevaluatedUserlist_; // Refresh active plugins before evaluating conditions. - temp.EvalAllConditions(game_, Language(LanguageCode(language)).GetCode()); - userTemp.EvalAllConditions(game_, Language(LanguageCode(language)).GetCode()); + temp.EvalAllConditions(game_); + userTemp.EvalAllConditions(game_); game_.GetMasterlist() = temp; game_.GetUserlist() = userTemp; diff --git a/src/api/api_database.h b/src/api/api_database.h index e278b5c2..f0104d5c 100644 --- a/src/api/api_database.h +++ b/src/api/api_database.h @@ -41,7 +41,7 @@ struct ApiDatabase : public DatabaseInterface { void LoadLists(const std::string& masterlist_path, const std::string& userlist_path = ""); - void EvalLists(const LanguageCode language); + void EvalLists(); std::vector SortPlugins(const std::vector& plugins); diff --git a/src/backend/metadata/message.cpp b/src/backend/metadata/message.cpp index 4c1a1f3a..6e83c7f6 100644 --- a/src/backend/metadata/message.cpp +++ b/src/backend/metadata/message.cpp @@ -64,13 +64,6 @@ bool Message::operator == (const Message& rhs) const { return (content_ == rhs.GetContent()); } -bool Message::EvalCondition(loot::Game& game, const LanguageCode language) { - BOOST_LOG_TRIVIAL(trace) << "Choosing message content for language: " << Language(language).GetName(); - content_.assign({GetContent(language)}); - - return ConditionalMetadata::EvalCondition(game); -} - MessageType Message::GetType() const { return type_; } diff --git a/src/backend/metadata/message.h b/src/backend/metadata/message.h index 159bfba0..40340990 100644 --- a/src/backend/metadata/message.h +++ b/src/backend/metadata/message.h @@ -37,8 +37,6 @@ #include "backend/metadata/message_content.h" namespace loot { -class Game; - class Message : public ConditionalMetadata { public: Message(); @@ -50,8 +48,6 @@ public: bool operator < (const Message& rhs) const; bool operator == (const Message& rhs) const; - bool EvalCondition(Game& game, const LanguageCode language); - MessageType GetType() const; std::vector GetContent() const; MessageContent GetContent(const LanguageCode language) const; diff --git a/src/backend/metadata/plugin_metadata.cpp b/src/backend/metadata/plugin_metadata.cpp index 6aec7bb6..0ddd035f 100644 --- a/src/backend/metadata/plugin_metadata.cpp +++ b/src/backend/metadata/plugin_metadata.cpp @@ -345,7 +345,7 @@ void PluginMetadata::Locations(const std::set& locations) { locations_ = locations; } -PluginMetadata& PluginMetadata::EvalAllConditions(Game& game, const LanguageCode language) { +PluginMetadata& PluginMetadata::EvalAllConditions(Game& game) { for (auto it = loadAfter_.begin(); it != loadAfter_.end();) { if (!it->EvalCondition(game)) loadAfter_.erase(it++); @@ -368,7 +368,7 @@ PluginMetadata& PluginMetadata::EvalAllConditions(Game& game, const LanguageCode } for (auto it = messages_.begin(); it != messages_.end();) { - if (!it->EvalCondition(game, language)) + if (!it->EvalCondition(game)) it = messages_.erase(it); else ++it; diff --git a/src/backend/metadata/plugin_metadata.h b/src/backend/metadata/plugin_metadata.h index f0b927f7..f7e32b47 100644 --- a/src/backend/metadata/plugin_metadata.h +++ b/src/backend/metadata/plugin_metadata.h @@ -89,7 +89,7 @@ public: void CleanInfo(const std::set& info); void Locations(const std::set& locations); - PluginMetadata& EvalAllConditions(Game& game, const LanguageCode language); + PluginMetadata& EvalAllConditions(Game& game); bool HasNameOnly() const; bool IsRegexPlugin() const; diff --git a/src/backend/metadata_list.cpp b/src/backend/metadata_list.cpp index 95b0416d..1551fb88 100644 --- a/src/backend/metadata_list.cpp +++ b/src/backend/metadata_list.cpp @@ -148,19 +148,19 @@ void MetadataList::AppendMessage(const Message& message) { messages_.push_back(message); } -void MetadataList::EvalAllConditions(Game& game, const LanguageCode language) { +void MetadataList::EvalAllConditions(Game& game) { std::unordered_set replacementSet; for (auto &plugin : plugins_) { PluginMetadata p(plugin); - p.EvalAllConditions(game, language); + p.EvalAllConditions(game); replacementSet.insert(p); } plugins_ = replacementSet; for (auto &plugin : regexPlugins_) { - plugin.EvalAllConditions(game, language); + plugin.EvalAllConditions(game); } for (auto &message : messages_) { - message.EvalCondition(game, language); + message.EvalCondition(game); } } } diff --git a/src/backend/metadata_list.h b/src/backend/metadata_list.h index 8c5aa47e..ef6a33f0 100644 --- a/src/backend/metadata_list.h +++ b/src/backend/metadata_list.h @@ -57,7 +57,7 @@ public: void AppendMessage(const Message& message); // Eval plugin conditions. - void EvalAllConditions(Game& game, const LanguageCode language); + void EvalAllConditions(Game& game); protected: std::set bashTags_; diff --git a/src/backend/plugin/plugin_sorter.cpp b/src/backend/plugin/plugin_sorter.cpp index 5b212464..be40abda 100644 --- a/src/backend/plugin/plugin_sorter.cpp +++ b/src/backend/plugin/plugin_sorter.cpp @@ -210,7 +210,7 @@ void PluginSorter::AddPluginVertices(Game& game, const LanguageCode language) { //Now that items are merged, evaluate any conditions they have. BOOST_LOG_TRIVIAL(trace) << "Evaluate conditions for merged plugin data."; try { - graph_[v].EvalAllConditions(game, language); + graph_[v].EvalAllConditions(game); } catch (std::exception& e) { BOOST_LOG_TRIVIAL(error) << "\"" << graph_[v].Name() << "\" contains a condition that could not be evaluated. Details: " << e.what(); list messages(graph_[v].Messages()); diff --git a/src/gui/query_handler.cpp b/src/gui/query_handler.cpp index 68950f03..f467a3da 100644 --- a/src/gui/query_handler.cpp +++ b/src/gui/query_handler.cpp @@ -929,7 +929,7 @@ std::vector QueryHandler::GetGeneralMessages() const { BOOST_LOG_TRIVIAL(info) << "Using message language: " << lootState_.getLanguage().GetName(); auto it = begin(messages); while (it != end(messages)) { - if (!it->EvalCondition(lootState_.getCurrentGame(), lootState_.getLanguage().GetCode())) + if (!it->EvalCondition(lootState_.getCurrentGame())) it = messages.erase(it); else ++it; @@ -954,7 +954,7 @@ YAML::Node QueryHandler::GenerateDerivedMetadata(const Plugin& file, const Plugi //Evaluate any conditions BOOST_LOG_TRIVIAL(trace) << "Evaluate conditions for merged plugin data."; try { - tempPlugin.EvalAllConditions(lootState_.getCurrentGame(), lootState_.getLanguage().GetCode()); + tempPlugin.EvalAllConditions(lootState_.getCurrentGame()); } catch (std::exception& e) { BOOST_LOG_TRIVIAL(error) << "\"" << tempPlugin.Name() << "\" contains a condition that could not be evaluated. Details: " << e.what(); list messages(tempPlugin.Messages()); diff --git a/src/tests/api/database_interface_test.h b/src/tests/api/database_interface_test.h index 0c536a9f..dbe84e34 100644 --- a/src/tests/api/database_interface_test.h +++ b/src/tests/api/database_interface_test.h @@ -122,35 +122,15 @@ TEST_P(DatabaseInterfaceTest, loadListsShouldSucceedIfTheMasterlistAndUserlistAr EXPECT_NO_THROW(db_->LoadLists(masterlistPath.string(), userlistPath_.string())); } -TEST_P(DatabaseInterfaceTest, evalListsShouldReturnOkForAllLanguagesWithNoListsLoaded) { - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::english)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::english)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::spanish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::russian)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::french)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::chinese)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::polish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::brazilian_portuguese)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::finnish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::german)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::danish)); +TEST_P(DatabaseInterfaceTest, evalListsShouldReturnOkWithNoListsLoaded) { + EXPECT_NO_THROW(db_->EvalLists()); } TEST_P(DatabaseInterfaceTest, evalListsShouldReturnOKForAllLanguagesWithAMasterlistLoaded) { ASSERT_NO_THROW(GenerateMasterlist()); ASSERT_NO_THROW(db_->LoadLists(masterlistPath.string(), "")); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::english)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::english)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::spanish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::russian)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::french)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::chinese)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::polish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::brazilian_portuguese)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::finnish)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::german)); - EXPECT_NO_THROW(db_->EvalLists(LanguageCode::danish)); + EXPECT_NO_THROW(db_->EvalLists()); } TEST_P(DatabaseInterfaceTest, sortPluginsShouldSucceedIfPassedValidArguments) { diff --git a/src/tests/backend/metadata/message_test.h b/src/tests/backend/metadata/message_test.h index 3eee8fac..45964969 100644 --- a/src/tests/backend/metadata/message_test.h +++ b/src/tests/backend/metadata/message_test.h @@ -94,7 +94,7 @@ TEST_P(MessageTest, messagesWithEqualContentStringsShouldBeEqual) { EXPECT_TRUE(message1 == message2); } -TEST_P(MessageTest, LessThanOperatorShouldUseCaseInsensitiveLexicographicalContentStringComparison) { +TEST_P(MessageTest, lessThanOperatorShouldUseCaseInsensitiveLexicographicalContentStringComparison) { Message message1(MessageType::say, MessageContents({MessageContent("content1", LanguageCode::english)}), "condition1"); Message message2(MessageType::warn, MessageContents({MessageContent("content1", LanguageCode::french)}), "condition2"); EXPECT_FALSE(message1 < message2); @@ -106,63 +106,12 @@ TEST_P(MessageTest, LessThanOperatorShouldUseCaseInsensitiveLexicographicalConte EXPECT_FALSE(message2 < message1); } -TEST_P(MessageTest, evalConditionShouldCreateADefaultContentObjectIfNoneExists) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - +TEST_P(MessageTest, getContentShouldReturnADefaultContentObjectIfNoneExists) { Message message; - EXPECT_TRUE(message.EvalCondition(game, LanguageCode::english)); - EXPECT_EQ(MessageContents({MessageContent()}), message.GetContent()); -} - -TEST_P(MessageTest, evalConditionShouldSelectTheStringForTheGivenLanguageIfOneExists) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - - Message message(MessageType::say, MessageContents({ - MessageContent("content1", LanguageCode::german), - MessageContent("content2", LanguageCode::english), - MessageContent("content3", LanguageCode::french), - })); - - EXPECT_TRUE(message.EvalCondition(game, LanguageCode::french)); - EXPECT_EQ(1, message.GetContent().size()); - EXPECT_EQ(MessageContent("content3", LanguageCode::french), message.GetContent()[0]); -} - -TEST_P(MessageTest, evalConditionShouldLeaveTheContentUnchangedIfOnlyOneStringExists) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - MessageContent content("content1", LanguageCode::english); - Message message(MessageType::say, MessageContents({content})); - - EXPECT_TRUE(message.EvalCondition(game, LanguageCode::french)); - EXPECT_EQ(MessageContents({content}), message.GetContent()); -} - -TEST_P(MessageTest, evalConditionShouldSelectTheEnglishStringIfNoStringExistsForTheGivenLanguage) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - - MessageContent content("content1", LanguageCode::english); - Message message(MessageType::say, MessageContents({ - content, - MessageContent("content1", LanguageCode::german), - })); - - EXPECT_TRUE(message.EvalCondition(game, LanguageCode::french)); - EXPECT_EQ(MessageContents({content}), message.GetContent()); + EXPECT_EQ(MessageContent(), message.GetContent(LanguageCode::english)); } TEST_P(MessageTest, getContentShouldSelectTheEnglishStringIfThereIsNoStringForTheGivenLanguage) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - Message message(MessageType::say, MessageContents({ MessageContent("content1", LanguageCode::german), MessageContent("content2", LanguageCode::english), @@ -173,10 +122,6 @@ TEST_P(MessageTest, getContentShouldSelectTheEnglishStringIfThereIsNoStringForTh } TEST_P(MessageTest, getContentShouldSelectTheGivenLanguageStringIfItExists) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - Message message(MessageType::say, MessageContents({ MessageContent("content1", LanguageCode::german), MessageContent("content2", LanguageCode::english), @@ -186,11 +131,7 @@ TEST_P(MessageTest, getContentShouldSelectTheGivenLanguageStringIfItExists) { EXPECT_EQ("content3", message.GetContent(LanguageCode::french).GetText()); } -TEST_P(MessageTest, getTextShouldSelectTheContentStringIfOnlyOneExists) { - Game game(GetParam()); - game.SetGamePath(dataPath.parent_path()); - ASSERT_NO_THROW(game.Init(false, localPath)); - +TEST_P(MessageTest, getContentShouldSelectTheContentStringIfOnlyOneExists) { Message message(MessageType::say, MessageContents({ MessageContent("content1", LanguageCode::german), })); diff --git a/src/tests/backend/metadata/plugin_metadata_test.h b/src/tests/backend/metadata/plugin_metadata_test.h index b0b4ce57..e481db97 100644 --- a/src/tests/backend/metadata/plugin_metadata_test.h +++ b/src/tests/backend/metadata/plugin_metadata_test.h @@ -659,7 +659,7 @@ TEST_P(PluginMetadataTest, evalAllConditionsShouldEvaluateAllMetadataConditions) plugin.DirtyInfo({info1, info2}); plugin.CleanInfo({info1, info2}); - EXPECT_NO_THROW(plugin.EvalAllConditions(game, LanguageCode::english)); + EXPECT_NO_THROW(plugin.EvalAllConditions(game)); std::set expectedFiles({file1}); EXPECT_EQ(expectedFiles, plugin.LoadAfter()); diff --git a/src/tests/backend/metadata_list_test.h b/src/tests/backend/metadata_list_test.h index fe2132b9..040fb764 100644 --- a/src/tests/backend/metadata_list_test.h +++ b/src/tests/backend/metadata_list_test.h @@ -299,7 +299,7 @@ TEST_P(MetadataListTest, evalAllConditionsShouldEvaluateTheConditionsForThePlugi ASSERT_EQ(blankEsp, plugin.Name()); ASSERT_FALSE(plugin.HasNameOnly()); - EXPECT_NO_THROW(metadataList.EvalAllConditions(game, LanguageCode::english)); + EXPECT_NO_THROW(metadataList.EvalAllConditions(game)); plugin = metadataList.FindPlugin(PluginMetadata(blankEsm)); EXPECT_EQ(std::list({