From e24576d8a843ceb097c0e805f702b70f99521486 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 18 Feb 2022 22:49:44 +0000 Subject: [PATCH] Initialise load order handler on construction And use a std::unique_ptr to avoid hitting the rule of five. --- src/api/game/game.cpp | 3 +- src/api/game/load_order_handler.cpp | 54 ++++----- src/api/game/load_order_handler.h | 18 +-- .../internals/game/load_order_handler_test.h | 103 +++++++----------- 4 files changed, 65 insertions(+), 113 deletions(-) diff --git a/src/api/game/game.cpp b/src/api/game/game.cpp index f59fb9ed..837f975d 100644 --- a/src/api/game/game.cpp +++ b/src/api/game/game.cpp @@ -59,6 +59,7 @@ Game::Game(const GameType gameType, const std::filesystem::path& localDataPath) : type_(gameType), gamePath_(gamePath), + loadOrderHandler_(type_, gamePath_, localDataPath), conditionEvaluator_( std::make_shared(Type(), DataPath())), database_(ApiDatabase(conditionEvaluator_)) { @@ -68,8 +69,6 @@ Game::Game(const GameType gameType, (int)type_, gamePath_.u8string()); } - - loadOrderHandler_.Init(type_, gamePath_, localDataPath); } GameType Game::Type() const { return type_; } diff --git a/src/api/game/load_order_handler.cpp b/src/api/game/load_order_handler.cpp index f391031a..7b5efc2f 100644 --- a/src/api/game/load_order_handler.cpp +++ b/src/api/game/load_order_handler.cpp @@ -55,28 +55,13 @@ unsigned int mapGameId(GameType gameType) { } } -LoadOrderHandler::LoadOrderHandler() : gh_(nullptr) {} - -LoadOrderHandler::LoadOrderHandler(LoadOrderHandler&& other) : gh_(other.gh_) { - other.gh_ = nullptr; -} - -LoadOrderHandler::~LoadOrderHandler() { lo_destroy_handle(gh_); } - -LoadOrderHandler& LoadOrderHandler::operator=(LoadOrderHandler&& other) { - if (&other != this) { - lo_destroy_handle(gh_); - - gh_ = other.gh_; - other.gh_ = nullptr; - } - - return *this; -} - -void LoadOrderHandler::Init(const GameType& gameType, - const std::filesystem::path& gamePath, - const std::filesystem::path& gameLocalAppData) { +LoadOrderHandler::LoadOrderHandler( + const GameType& gameType, + const std::filesystem::path& gamePath, + const std::filesystem::path& gameLocalAppData) : + gh_(std::unique_ptr::type, + decltype(&lo_destroy_handle)>(nullptr, + lo_destroy_handle)) { if (gamePath.empty()) { throw std::invalid_argument("Game path is not initialised."); } @@ -86,18 +71,18 @@ void LoadOrderHandler::Init(const GameType& gameType, if (!tempPathString.empty()) gameLocalDataPath = tempPathString.c_str(); - // If the handle has already been initialised, close it and open another. - if (gh_ != nullptr) { - lo_destroy_handle(gh_); - gh_ = nullptr; - } + lo_game_handle handle = nullptr; - int ret = lo_create_handle(&gh_, + int ret = lo_create_handle(&handle, mapGameId(gameType), gamePath.u8string().c_str(), gameLocalDataPath); HandleError("create a game handle", ret); + + gh_ = + std::unique_ptr::type, + decltype(&lo_destroy_handle)>(handle, lo_destroy_handle); } void LoadOrderHandler::LoadCurrentState() { @@ -106,7 +91,7 @@ void LoadOrderHandler::LoadCurrentState() { logger->info("Loading the current load order state."); } - const unsigned int ret = lo_load_current_state(gh_); + const unsigned int ret = lo_load_current_state(gh_.get()); HandleError("load the current load order state", ret); } @@ -119,7 +104,7 @@ bool LoadOrderHandler::IsPluginActive(const std::string& pluginName) const { bool result = false; const unsigned int ret = - lo_get_plugin_active(gh_, pluginName.c_str(), &result); + lo_get_plugin_active(gh_.get(), pluginName.c_str(), &result); HandleError("check if a plugin is active", ret); @@ -135,7 +120,8 @@ std::vector LoadOrderHandler::GetLoadOrder() const { char** pluginArr = nullptr; size_t pluginArrSize = 0; - const unsigned int ret = lo_get_load_order(gh_, &pluginArr, &pluginArrSize); + const unsigned int ret = + lo_get_load_order(gh_.get(), &pluginArr, &pluginArrSize); HandleError("get the load order", ret); @@ -156,7 +142,7 @@ std::vector LoadOrderHandler::GetActivePlugins() const { size_t pluginArrSize = 0; const unsigned int ret = - lo_get_active_plugins(gh_, &pluginArr, &pluginArrSize); + lo_get_active_plugins(gh_.get(), &pluginArr, &pluginArrSize); HandleError("get active plugins", ret); @@ -177,7 +163,7 @@ std::vector LoadOrderHandler::GetImplicitlyActivePlugins() const { size_t pluginArrSize = 0; const unsigned int ret = - lo_get_implicitly_active_plugins(gh_, &pluginArr, &pluginArrSize); + lo_get_implicitly_active_plugins(gh_.get(), &pluginArr, &pluginArrSize); HandleError("get implicitly active plugins", ret); @@ -206,7 +192,7 @@ void LoadOrderHandler::SetLoadOrder( } const unsigned int ret = - lo_set_load_order(gh_, plugins.data(), plugins.size()); + lo_set_load_order(gh_.get(), plugins.data(), plugins.size()); HandleError("set the load order", ret); diff --git a/src/api/game/load_order_handler.h b/src/api/game/load_order_handler.h index 7885e0aa..ac971da5 100644 --- a/src/api/game/load_order_handler.h +++ b/src/api/game/load_order_handler.h @@ -37,17 +37,9 @@ namespace loot { class LoadOrderHandler { public: - explicit LoadOrderHandler(); - LoadOrderHandler(const LoadOrderHandler&) = delete; - LoadOrderHandler(LoadOrderHandler&& other); - ~LoadOrderHandler(); - - LoadOrderHandler& operator=(const LoadOrderHandler&) = delete; - LoadOrderHandler& operator=(LoadOrderHandler&& other); - - void Init(const GameType& game, - const std::filesystem::path& gamePath, - const std::filesystem::path& gameLocalAppData = ""); + explicit LoadOrderHandler(const GameType& game, + const std::filesystem::path& gamePath, + const std::filesystem::path& gameLocalAppData = ""); void LoadCurrentState(); @@ -64,7 +56,9 @@ public: private: void HandleError(const std::string& operation, unsigned int returnCode) const; - lo_game_handle gh_; + std::unique_ptr::type, + decltype(&lo_destroy_handle)> + gh_; }; } diff --git a/src/tests/api/internals/game/load_order_handler_test.h b/src/tests/api/internals/game/load_order_handler_test.h index bc6c6149..3f2815b4 100644 --- a/src/tests/api/internals/game/load_order_handler_test.h +++ b/src/tests/api/internals/game/load_order_handler_test.h @@ -51,9 +51,8 @@ protected: } } - void initialiseHandler() { - ASSERT_NO_THROW( - loadOrderHandler_.Init(GetParam(), dataPath.parent_path(), localPath)); + LoadOrderHandler createHandler() { + return LoadOrderHandler(GetParam(), dataPath.parent_path(), localPath); } std::vector getImplicitlyActivePlugins() { @@ -99,7 +98,6 @@ protected: return activePlugins; } - LoadOrderHandler loadOrderHandler_; std::vector loadOrderToSet_; }; @@ -115,119 +113,94 @@ INSTANTIATE_TEST_SUITE_P(, GameType::fo4, GameType::tes5se)); -TEST_P(LoadOrderHandlerTest, initShouldThrowIfNoGamePathIsSet) { - EXPECT_THROW(loadOrderHandler_.Init(GetParam(), ""), std::invalid_argument); - EXPECT_THROW(loadOrderHandler_.Init(GetParam(), ""), std::invalid_argument); - EXPECT_THROW(loadOrderHandler_.Init(GetParam(), "", localPath), +TEST_P(LoadOrderHandlerTest, constructorShouldThrowIfNoGamePathIsSet) { + EXPECT_THROW(LoadOrderHandler(GetParam(), ""), std::invalid_argument); + EXPECT_THROW(LoadOrderHandler(GetParam(), ""), std::invalid_argument); + EXPECT_THROW(LoadOrderHandler(GetParam(), "", localPath), std::invalid_argument); - EXPECT_THROW(loadOrderHandler_.Init(GetParam(), "", localPath), + EXPECT_THROW(LoadOrderHandler(GetParam(), "", localPath), std::invalid_argument); } #ifndef _WIN32 -TEST_P(LoadOrderHandlerTest, initShouldThrowOnLinuxIfNoLocalPathIsSet) { - EXPECT_THROW(loadOrderHandler_.Init(GetParam(), dataPath.parent_path()), +TEST_P(LoadOrderHandlerTest, constructorShouldThrowOnLinuxIfNoLocalPathIsSet) { + EXPECT_THROW(LoadOrderHandler(GetParam(), dataPath.parent_path()), std::system_error); } #endif TEST_P(LoadOrderHandlerTest, - initShouldNotThrowIfAValidGameIdAndGamePathAndLocalPathAreSet) { + constructorShouldNotThrowIfAValidGameIdAndGamePathAndLocalPathAreSet) { EXPECT_NO_THROW( - loadOrderHandler_.Init(GetParam(), dataPath.parent_path(), localPath)); -} - -TEST_P(LoadOrderHandlerTest, - isPluginActiveShouldThrowIfTheHandlerHasNotBeenInitialised) { - EXPECT_THROW(loadOrderHandler_.IsPluginActive(masterFile), std::system_error); + LoadOrderHandler(GetParam(), dataPath.parent_path(), localPath)); } TEST_P(LoadOrderHandlerTest, isPluginActiveShouldReturnFalseIfLoadOrderStateHasNotBeenLoaded) { - initialiseHandler(); + auto loadOrderHandler = createHandler(); - EXPECT_FALSE(loadOrderHandler_.IsPluginActive(masterFile)); - EXPECT_FALSE(loadOrderHandler_.IsPluginActive(blankEsm)); - EXPECT_FALSE(loadOrderHandler_.IsPluginActive(blankEsp)); + EXPECT_FALSE(loadOrderHandler.IsPluginActive(masterFile)); + EXPECT_FALSE(loadOrderHandler.IsPluginActive(blankEsm)); + EXPECT_FALSE(loadOrderHandler.IsPluginActive(blankEsp)); } TEST_P(LoadOrderHandlerTest, isPluginActiveShouldReturnCorrectPluginStatesAfterInitialisation) { - initialiseHandler(); - loadOrderHandler_.LoadCurrentState(); + auto loadOrderHandler = createHandler(); + loadOrderHandler.LoadCurrentState(); - EXPECT_TRUE(loadOrderHandler_.IsPluginActive(masterFile)); - EXPECT_TRUE(loadOrderHandler_.IsPluginActive(blankEsm)); - EXPECT_FALSE(loadOrderHandler_.IsPluginActive(blankEsp)); -} - -TEST_P(LoadOrderHandlerTest, - getLoadOrderShouldThrowIfTheHandlerHasNotBeenInitialised) { - EXPECT_THROW(loadOrderHandler_.GetLoadOrder(), std::system_error); + EXPECT_TRUE(loadOrderHandler.IsPluginActive(masterFile)); + EXPECT_TRUE(loadOrderHandler.IsPluginActive(blankEsm)); + EXPECT_FALSE(loadOrderHandler.IsPluginActive(blankEsp)); } TEST_P(LoadOrderHandlerTest, getLoadOrderShouldReturnAnEmptyVectorIfStateHasNotBeenLoaded) { - initialiseHandler(); + auto loadOrderHandler = createHandler(); - EXPECT_TRUE(loadOrderHandler_.GetLoadOrder().empty()); + EXPECT_TRUE(loadOrderHandler.GetLoadOrder().empty()); } TEST_P(LoadOrderHandlerTest, getLoadOrderShouldReturnTheCurrentLoadOrder) { - initialiseHandler(); - loadOrderHandler_.LoadCurrentState(); + auto loadOrderHandler = createHandler(); + loadOrderHandler.LoadCurrentState(); - ASSERT_EQ(getLoadOrder(), loadOrderHandler_.GetLoadOrder()); -} - -TEST_P(LoadOrderHandlerTest, - getActivePluginsShouldThrowIfTheHandlerHasNotBeenInitialised) { - EXPECT_THROW(loadOrderHandler_.GetActivePlugins(), std::system_error); + ASSERT_EQ(getLoadOrder(), loadOrderHandler.GetLoadOrder()); } TEST_P(LoadOrderHandlerTest, getActivePluginsShouldReturnAnEmptyVectorIfStateHasNotBeenLoaded) { - initialiseHandler(); + auto loadOrderHandler = createHandler(); - EXPECT_TRUE(loadOrderHandler_.GetActivePlugins().empty()); + EXPECT_TRUE(loadOrderHandler.GetActivePlugins().empty()); } + TEST_P(LoadOrderHandlerTest, getActivePluginsShouldReturnOnlyActivePlugins) { - initialiseHandler(); - loadOrderHandler_.LoadCurrentState(); + auto loadOrderHandler = createHandler(); + loadOrderHandler.LoadCurrentState(); - ASSERT_EQ(getActivePlugins(), loadOrderHandler_.GetActivePlugins()); + ASSERT_EQ(getActivePlugins(), loadOrderHandler.GetActivePlugins()); } -TEST_P(LoadOrderHandlerTest, - getImplicitlyActivePluginsShouldThrowIfTheHandlerHasNotBeenInitialised) { - EXPECT_THROW(loadOrderHandler_.GetImplicitlyActivePlugins(), - std::system_error); -} TEST_P( LoadOrderHandlerTest, getImplicitlyActivePluginsShouldReturnValidDataEvenIfStateHasNotBeenLoaded) { - initialiseHandler(); + auto loadOrderHandler = createHandler(); ASSERT_EQ(getImplicitlyActivePlugins(), - loadOrderHandler_.GetImplicitlyActivePlugins()); + loadOrderHandler.GetImplicitlyActivePlugins()); - loadOrderHandler_.LoadCurrentState(); + loadOrderHandler.LoadCurrentState(); ASSERT_EQ(getImplicitlyActivePlugins(), - loadOrderHandler_.GetImplicitlyActivePlugins()); -} - -TEST_P(LoadOrderHandlerTest, - setLoadOrderShouldThrowIfTheHandlerHasNotBeenInitialised) { - EXPECT_THROW(loadOrderHandler_.SetLoadOrder(loadOrderToSet_), - std::system_error); + loadOrderHandler.GetImplicitlyActivePlugins()); } TEST_P(LoadOrderHandlerTest, setLoadOrderShouldSetTheLoadOrder) { - initialiseHandler(); - loadOrderHandler_.LoadCurrentState(); + auto loadOrderHandler = createHandler(); + loadOrderHandler.LoadCurrentState(); - EXPECT_NO_THROW(loadOrderHandler_.SetLoadOrder(loadOrderToSet_)); + EXPECT_NO_THROW(loadOrderHandler.SetLoadOrder(loadOrderToSet_)); if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) loadOrderToSet_.erase(begin(loadOrderToSet_));