From da7959cfc92817a3759e4fd4391aa812f7ca6c38 Mon Sep 17 00:00:00 2001 From: Frank Howie Date: Tue, 23 Apr 2024 17:23:19 +0200 Subject: [PATCH 1/4] [json] json-rpc/AddonsOperations.cpp: remove dead code --- xbmc/interfaces/json-rpc/AddonsOperations.cpp | 10 +++------- xbmc/interfaces/json-rpc/AddonsOperations.h | 3 +-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/xbmc/interfaces/json-rpc/AddonsOperations.cpp b/xbmc/interfaces/json-rpc/AddonsOperations.cpp index db6c90a191a21..3daac8cc0b7ed 100644 --- a/xbmc/interfaces/json-rpc/AddonsOperations.cpp +++ b/xbmc/interfaces/json-rpc/AddonsOperations.cpp @@ -11,7 +11,6 @@ #include "JSONUtils.h" #include "ServiceBroker.h" #include "TextureCache.h" -#include "addons/AddonDatabase.h" #include "addons/AddonManager.h" #include "addons/PluginSource.h" #include "addons/addoninfo/AddonInfo.h" @@ -122,9 +121,8 @@ JSONRPC_STATUS CAddonsOperations::GetAddons(const std::string &method, ITranspor int start, end; HandleLimits(parameterObject, result, addons.size(), start, end); - CAddonDatabase addondb; for (int index = start; index < end; index++) - FillDetails(addons.at(index), parameterObject["properties"], result["addons"], addondb, true); + FillDetails(addons.at(index), parameterObject["properties"], result["addons"], true); return OK; } @@ -138,8 +136,7 @@ JSONRPC_STATUS CAddonsOperations::GetAddonDetails(const std::string &method, ITr addon->Type() >= AddonType::MAX_TYPES) return InvalidParams; - CAddonDatabase addondb; - FillDetails(addon, parameterObject["properties"], result["addon"], addondb); + FillDetails(addon, parameterObject["properties"], result["addon"], false); return OK; } @@ -272,8 +269,7 @@ static CVariant Serialize(const AddonPtr& addon) void CAddonsOperations::FillDetails(const std::shared_ptr& addon, const CVariant& fields, CVariant& result, - CAddonDatabase& addondb, - bool append /* = false */) + bool append) { if (addon.get() == NULL) return; diff --git a/xbmc/interfaces/json-rpc/AddonsOperations.h b/xbmc/interfaces/json-rpc/AddonsOperations.h index 727f418999867..cf78a2dba0c97 100644 --- a/xbmc/interfaces/json-rpc/AddonsOperations.h +++ b/xbmc/interfaces/json-rpc/AddonsOperations.h @@ -35,7 +35,6 @@ namespace JSONRPC static void FillDetails(const std::shared_ptr& addon, const CVariant& fields, CVariant& result, - ADDON::CAddonDatabase& addondb, - bool append = false); + bool append); }; } From c7ad7622ed02c6815bcd33038cfe76e932d1f8d1 Mon Sep 17 00:00:00 2001 From: Frank Howie Date: Tue, 21 Nov 2023 06:57:53 +0100 Subject: [PATCH 2/4] follow up to clang-tidy 'modernize-use-emplace' make use of emplace / try_emplace for maps instead of insert. clang-tidy seems to have missed a few in previous runs --- xbmc/addons/AddonManager.h | 3 ++- xbmc/addons/AddonRepos.cpp | 6 ++--- xbmc/addons/AddonRepos.h | 10 ++------ xbmc/filesystem/AddonsDirectory.cpp | 38 ++++++++++++++--------------- xbmc/utils/test/TestVariant.cpp | 2 +- 5 files changed, 26 insertions(+), 33 deletions(-) diff --git a/xbmc/addons/AddonManager.h b/xbmc/addons/AddonManager.h index 10668125c042d..fd7ee70f66983 100644 --- a/xbmc/addons/AddonManager.h +++ b/xbmc/addons/AddonManager.h @@ -16,6 +16,7 @@ #include #include #include +#include #include namespace ADDON @@ -37,10 +38,10 @@ using ADDON_INFO_LIST = std::map; class IAddon; using AddonPtr = std::shared_ptr; +using AddonWithUpdate = std::pair, std::shared_ptr>; using VECADDONS = std::vector; struct AddonEvent; -struct AddonWithUpdate; struct DependencyInfo; struct RepositoryDirInfo; diff --git a/xbmc/addons/AddonRepos.cpp b/xbmc/addons/AddonRepos.cpp index 5455ca4487d2c..efca8c103b439 100644 --- a/xbmc/addons/AddonRepos.cpp +++ b/xbmc/addons/AddonRepos.cpp @@ -122,7 +122,7 @@ bool CAddonRepos::LoadAddonsFromDatabase(const std::string& addonId, { if (m_addonMgr.IsCompatible(addon)) { - m_addonsByRepoMap[addon->Origin()].insert({addon->ID(), addon}); + m_addonsByRepoMap[addon->Origin()].emplace(addon->ID(), addon); } } @@ -170,7 +170,7 @@ void CAddonRepos::AddAddonIfLatest( if (latestVersionByRepo == map.end()) // repo not found { - map[repoId].insert({addonToAdd->ID(), addonToAdd}); + map[repoId].emplace(addonToAdd->ID(), addonToAdd); } else { @@ -212,7 +212,7 @@ void CAddonRepos::BuildAddonsWithUpdateList( { if (DoAddonUpdateCheck(addon, update)) { - addonsWithUpdate.insert({addon->ID(), {addon, update}}); + addonsWithUpdate.try_emplace(addon->ID(), addon, update); } } } diff --git a/xbmc/addons/AddonRepos.h b/xbmc/addons/AddonRepos.h index bb9be3473c57d..4284674f01146 100644 --- a/xbmc/addons/AddonRepos.h +++ b/xbmc/addons/AddonRepos.h @@ -13,6 +13,7 @@ #include #include #include +#include #include namespace ADDON @@ -30,14 +31,7 @@ enum class CheckAddonPath CHOICE_NO = false, }; -/** - * Struct - CAddonWithUpdate - */ -struct AddonWithUpdate -{ - std::shared_ptr m_installed; - std::shared_ptr m_update; -}; +using AddonWithUpdate = std::pair, std::shared_ptr>; /** * Class - CAddonRepos diff --git a/xbmc/filesystem/AddonsDirectory.cpp b/xbmc/filesystem/AddonsDirectory.cpp index 19955b91dece4..aba97cae08458 100644 --- a/xbmc/filesystem/AddonsDirectory.cpp +++ b/xbmc/filesystem/AddonsDirectory.cpp @@ -790,29 +790,27 @@ void CAddonsDirectory::GenerateAddonListing(const CURL& path, addon->Version()); bool disabled = CServiceBroker::GetAddonMgr().IsAddonDisabled(addon->ID()); - std::function CheckOutdatedOrUpdate = [&](bool checkOutdated) -> bool { - auto mapEntry = addonsWithUpdate.find(addon->ID()); - if (mapEntry != addonsWithUpdate.end()) - { - const std::shared_ptr& checkedObject = - checkOutdated ? mapEntry->second.m_installed : mapEntry->second.m_update; - - return (checkedObject->Origin() == addon->Origin() && - checkedObject->Version() == addon->Version()); - } - return false; - }; - - bool isUpdate = CheckOutdatedOrUpdate(false); // check if it's an available update - bool hasUpdate = CheckOutdatedOrUpdate(true); // check if it's an outdated addon - + bool isUpdate{false}; + bool hasUpdate{false}; std::string validUpdateVersion; std::string validUpdateOrigin; - if (hasUpdate) + + auto _ = addonsWithUpdate.find(addon->ID()); + if (_ != addonsWithUpdate.end()) { - auto mapEntry = addonsWithUpdate.find(addon->ID()); - validUpdateVersion = mapEntry->second.m_update->Version().asString(); - validUpdateOrigin = mapEntry->second.m_update->Origin(); + auto [installed, update] = _->second; + + auto CheckAddon = [&addon](const std::shared_ptr& _) + { return _->Origin() == addon->Origin() && _->Version() == addon->Version(); }; + + isUpdate = CheckAddon(update); // check if listed add-on is update to an installed add-on + hasUpdate = CheckAddon(installed); // check if installed add-on has an update available + + if (hasUpdate) + { + validUpdateVersion = update->Version().asString(); + validUpdateOrigin = update->Origin(); + } } bool fromOfficialRepo = CAddonRepos::IsFromOfficialRepo(addon, CheckAddonPath::CHOICE_NO); diff --git a/xbmc/utils/test/TestVariant.cpp b/xbmc/utils/test/TestVariant.cpp index f5bf687ea6952..60eafddf04b84 100644 --- a/xbmc/utils/test/TestVariant.cpp +++ b/xbmc/utils/test/TestVariant.cpp @@ -294,7 +294,7 @@ TEST(TestVariant, empty) std::map strmap; EXPECT_TRUE(CVariant(strmap).empty()); - strmap.emplace(std::string("key"), std::string("value")); + strmap.emplace("key", "value"); EXPECT_FALSE(CVariant(strmap).empty()); std::string str; From c4ad9015ced01bcbc12fe07ce4ba9640d0b473ed Mon Sep 17 00:00:00 2001 From: Frank Howie Date: Fri, 12 Apr 2024 21:13:56 +0200 Subject: [PATCH 3/4] AddonRepos.cpp: use structured bindings where possible and don't take references to iterators --- xbmc/addons/AddonRepos.cpp | 81 ++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/xbmc/addons/AddonRepos.cpp b/xbmc/addons/AddonRepos.cpp index efca8c103b439..810814798145b 100644 --- a/xbmc/addons/AddonRepos.cpp +++ b/xbmc/addons/AddonRepos.cpp @@ -126,16 +126,12 @@ bool CAddonRepos::LoadAddonsFromDatabase(const std::string& addonId, } } - for (const auto& repo : m_addonsByRepoMap) + for (const auto& [repoId, addonsPerRepo] : m_addonsByRepoMap) { - CLog::LogFC(LOGDEBUG, LOGADDONS, "{} - {} addon(s) loaded", repo.first, repo.second.size()); + CLog::LogFC(LOGDEBUG, LOGADDONS, "{} - {} addon(s) loaded", repoId, addonsPerRepo.size()); - const auto& addonsPerRepo = repo.second; - - for (const auto& addonMapEntry : addonsPerRepo) + for (const auto& [addonId, addonToAdd] : addonsPerRepo) { - const auto& addonToAdd = addonMapEntry.second; - if (IsFromOfficialRepo(addonToAdd, CheckAddonPath::CHOICE_YES)) { AddAddonIfLatest(addonToAdd, m_latestOfficialVersions); @@ -146,7 +142,7 @@ bool CAddonRepos::LoadAddonsFromDatabase(const std::string& addonId, } // add to latestVersionsByRepo - AddAddonIfLatest(repo.first, addonToAdd, m_latestVersionsByRepo); + AddAddonIfLatest(repoId, addonToAdd, m_latestVersionsByRepo); } } @@ -156,8 +152,8 @@ bool CAddonRepos::LoadAddonsFromDatabase(const std::string& addonId, void CAddonRepos::AddAddonIfLatest(const std::shared_ptr& addonToAdd, std::map>& map) const { - const auto& latestKnown = map.find(addonToAdd->ID()); - if (latestKnown == map.end() || addonToAdd->Version() > latestKnown->second->Version()) + const auto latestKnownIt = map.find(addonToAdd->ID()); + if (latestKnownIt == map.end() || addonToAdd->Version() > latestKnownIt->second->Version()) map[addonToAdd->ID()] = addonToAdd; } @@ -166,19 +162,19 @@ void CAddonRepos::AddAddonIfLatest( const std::shared_ptr& addonToAdd, std::map>>& map) const { - const auto& latestVersionByRepo = map.find(repoId); + const auto latestVersionByRepoIt = map.find(repoId); - if (latestVersionByRepo == map.end()) // repo not found + if (latestVersionByRepoIt == map.end()) // repo not found { map[repoId].emplace(addonToAdd->ID(), addonToAdd); } else { - const auto& latestVersionEntryByRepo = latestVersionByRepo->second; - const auto& latestKnown = latestVersionEntryByRepo.find(addonToAdd->ID()); + const auto& latestVersionEntryByRepo = latestVersionByRepoIt->second; + const auto latestKnownIt = latestVersionEntryByRepo.find(addonToAdd->ID()); - if (latestKnown == latestVersionEntryByRepo.end() || - addonToAdd->Version() > latestKnown->second->Version()) + if (latestKnownIt == latestVersionEntryByRepo.end() || + addonToAdd->Version() > latestKnownIt->second->Version()) map[repoId][addonToAdd->ID()] = addonToAdd; } } @@ -253,10 +249,10 @@ bool CAddonRepos::DoAddonUpdateCheck(const std::shared_ptr& addon, else { // ...we check for updates in the origin repo only - const auto& repoEntry = m_latestVersionsByRepo.find(addon->Origin()); - if (repoEntry != m_latestVersionsByRepo.end()) + const auto repoEntryIt = m_latestVersionsByRepo.find(addon->Origin()); + if (repoEntryIt != m_latestVersionsByRepo.end()) { - if (!FindAddonAndCheckForUpdate(addon, repoEntry->second, update)) + if (!FindAddonAndCheckForUpdate(addon, repoEntryIt->second, update)) { return false; } @@ -280,14 +276,14 @@ bool CAddonRepos::FindAddonAndCheckForUpdate( const std::map>& map, std::shared_ptr& update) const { - const auto& remote = map.find(addonToCheck->ID()); - if (remote != map.end()) // is addon in the desired map? + const auto remoteIt = map.find(addonToCheck->ID()); + if (remoteIt != map.end()) // is addon in the desired map? { - if ((remote->second->Version() > addonToCheck->Version()) || + if ((remoteIt->second->Version() > addonToCheck->Version()) || m_addonMgr.IsAddonDisabledWithReason(addonToCheck->ID(), AddonDisabledReason::INCOMPATIBLE)) { // return addon update - update = remote->second; + update = remoteIt->second; return true; // update found } } @@ -300,10 +296,10 @@ bool CAddonRepos::GetLatestVersionByMap(const std::string& addonId, const std::map>& map, std::shared_ptr& addon) const { - const auto& remote = map.find(addonId); - if (remote != map.end()) // is addon in the desired map? + const auto remoteIt = map.find(addonId); + if (remoteIt != map.end()) // is addon in the desired map? { - addon = remote->second; + addon = remoteIt->second; return true; } @@ -356,14 +352,15 @@ void CAddonRepos::GetLatestAddonVersions(std::vector>& a // then we insert private addon versions if they don't exist in the official map // or installation from ANY_REPOSITORY is allowed and the private version is higher - for (const auto& privateVersion : m_latestPrivateVersions) + for (const auto& [privateVersionId, privateVersion] : m_latestPrivateVersions) { - const auto& officialVersion = m_latestOfficialVersions.find(privateVersion.first); - if (officialVersion == m_latestOfficialVersions.end() || + const auto officialVersionIt = m_latestOfficialVersions.find(privateVersionId); + + if (officialVersionIt == m_latestOfficialVersions.end() || (updateMode == AddonRepoUpdateMode::ANY_REPOSITORY && - privateVersion.second->Version() > officialVersion->second->Version())) + privateVersion->Version() > officialVersionIt->second->Version())) { - addonList.emplace_back(privateVersion.second); + addonList.emplace_back(privateVersion); } } } @@ -390,18 +387,18 @@ void CAddonRepos::GetLatestAddonVersionsFromAllRepos( // so we need to filter them out if (std::none_of(officialRepoInfos.begin(), officialRepoInfos.end(), - [&](const ADDON::RepoInfo& officialRepo) { - return repo.first == officialRepo.m_repoId; - })) + [&repo](const ADDON::RepoInfo& officialRepo) + { return repo.first == officialRepo.m_repoId; })) { - for (const auto& latestAddon : repo.second) + for (const auto& [latestAddonId, latestAddon] : repo.second) { - const auto& officialVersion = m_latestOfficialVersions.find(latestAddon.first); - if (officialVersion == m_latestOfficialVersions.end() || + const auto officialVersionIt = m_latestOfficialVersions.find(latestAddonId); + + if (officialVersionIt == m_latestOfficialVersions.end() || (updateMode == AddonRepoUpdateMode::ANY_REPOSITORY && - latestAddon.second->Version() > officialVersion->second->Version())) + latestAddon->Version() > officialVersionIt->second->Version())) { - addonList.emplace_back(latestAddon.second); + addonList.emplace_back(latestAddon); } } } @@ -469,10 +466,10 @@ bool CAddonRepos::FindDependencyByParentRepo(const std::string& dependsId, const std::string& parentRepoId, std::shared_ptr& dependencyToInstall) const { - const auto& repoEntry = m_latestVersionsByRepo.find(parentRepoId); - if (repoEntry != m_latestVersionsByRepo.end()) + const auto repoEntryIt = m_latestVersionsByRepo.find(parentRepoId); + if (repoEntryIt != m_latestVersionsByRepo.end()) { - if (GetLatestVersionByMap(dependsId, repoEntry->second, dependencyToInstall)) + if (GetLatestVersionByMap(dependsId, repoEntryIt->second, dependencyToInstall)) return true; } From 417f3a5236f26bc4c2704cd05a7ffe8560d2f8ba Mon Sep 17 00:00:00 2001 From: Frank Howie Date: Sun, 21 Apr 2024 18:07:24 +0200 Subject: [PATCH 4/4] simplify AddAddonIfLatest --- xbmc/addons/AddonRepos.cpp | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/xbmc/addons/AddonRepos.cpp b/xbmc/addons/AddonRepos.cpp index 810814798145b..362f051b97bd3 100644 --- a/xbmc/addons/AddonRepos.cpp +++ b/xbmc/addons/AddonRepos.cpp @@ -162,21 +162,23 @@ void CAddonRepos::AddAddonIfLatest( const std::shared_ptr& addonToAdd, std::map>>& map) const { - const auto latestVersionByRepoIt = map.find(repoId); + bool doInsert{true}; - if (latestVersionByRepoIt == map.end()) // repo not found - { - map[repoId].emplace(addonToAdd->ID(), addonToAdd); - } - else + const auto latestVersionByRepoIt = map.find(repoId); + if (latestVersionByRepoIt != map.end()) // we already have this repository in the outer map { const auto& latestVersionEntryByRepo = latestVersionByRepoIt->second; const auto latestKnownIt = latestVersionEntryByRepo.find(addonToAdd->ID()); - if (latestKnownIt == latestVersionEntryByRepo.end() || - addonToAdd->Version() > latestKnownIt->second->Version()) - map[repoId][addonToAdd->ID()] = addonToAdd; + if (latestKnownIt != latestVersionEntryByRepo.end() && + addonToAdd->Version() <= latestKnownIt->second->Version()) + { + doInsert = false; + } } + + if (doInsert) + map[repoId][addonToAdd->ID()] = addonToAdd; } void CAddonRepos::BuildUpdateOrOutdatedList(const std::vector>& installed,