Merge pull request #25013 from howie-f/v22-smallimp

[Kodi P*] small improvements and cleanup

7 files changed, 75 insertions, 88 deletions

diff --git a/xbmc/addons/AddonManager.h b/xbmc/addons/AddonManager.h
index 10668125c0..fd7ee70f66 100644
--- a/xbmc/addons/AddonManager.h
+++ b/xbmc/addons/AddonManager.h
@@ -16,6 +16,7 @@
 #include <mutex>
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace ADDON
@@ -37,10 +38,10 @@ using ADDON_INFO_LIST = std::map<std::string, AddonInfoPtr>;
 
 class IAddon;
 using AddonPtr = std::shared_ptr<IAddon>;
+using AddonWithUpdate = std::pair<std::shared_ptr<IAddon>, std::shared_ptr<IAddon>>;
 using VECADDONS = std::vector<AddonPtr>;
 
 struct AddonEvent;
-struct AddonWithUpdate;
 struct DependencyInfo;
 struct RepositoryDirInfo;
 
diff --git a/xbmc/addons/AddonRepos.cpp b/xbmc/addons/AddonRepos.cpp
index 5455ca4487..362f051b97 100644
--- a/xbmc/addons/AddonRepos.cpp
+++ b/xbmc/addons/AddonRepos.cpp
@@ -122,20 +122,16 @@ 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);
     }
   }
 
-  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<IAddon>& addonToAdd,
                                    std::map<std::string, std::shared_ptr<IAddon>>& 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,21 +162,23 @@ void CAddonRepos::AddAddonIfLatest(
     const std::shared_ptr<IAddon>& addonToAdd,
     std::map<std::string, std::map<std::string, std::shared_ptr<IAddon>>>& map) const
 {
-  const auto& latestVersionByRepo = map.find(repoId);
+  bool doInsert{true};
 
-  if (latestVersionByRepo == map.end()) // repo not found
-  {
-    map[repoId].insert({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 = 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())
-      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<std::shared_ptr<IAddon>>& installed,
@@ -212,7 +210,7 @@ void CAddonRepos::BuildAddonsWithUpdateList(
   {
     if (DoAddonUpdateCheck(addon, update))
     {
-      addonsWithUpdate.insert({addon->ID(), {addon, update}});
+      addonsWithUpdate.try_emplace(addon->ID(), addon, update);
     }
   }
 }
@@ -253,10 +251,10 @@ bool CAddonRepos::DoAddonUpdateCheck(const std::shared_ptr<IAddon>& 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 +278,14 @@ bool CAddonRepos::FindAddonAndCheckForUpdate(
     const std::map<std::string, std::shared_ptr<IAddon>>& map,
     std::shared_ptr<IAddon>& 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 +298,10 @@ bool CAddonRepos::GetLatestVersionByMap(const std::string& addonId,
                                         const std::map<std::string, std::shared_ptr<IAddon>>& map,
                                         std::shared_ptr<IAddon>& 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 +354,15 @@ void CAddonRepos::GetLatestAddonVersions(std::vector<std::shared_ptr<IAddon>>& 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 +389,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 +468,10 @@ bool CAddonRepos::FindDependencyByParentRepo(const std::string& dependsId,
                                              const std::string& parentRepoId,
                                              std::shared_ptr<IAddon>& 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;
   }
 
diff --git a/xbmc/addons/AddonRepos.h b/xbmc/addons/AddonRepos.h
index bb9be3473c..4284674f01 100644
--- a/xbmc/addons/AddonRepos.h
+++ b/xbmc/addons/AddonRepos.h
@@ -13,6 +13,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace ADDON
@@ -30,14 +31,7 @@ enum class CheckAddonPath
   CHOICE_NO = false,
 };
 
-/**
- * Struct - CAddonWithUpdate
- */
-struct AddonWithUpdate
-{
-  std::shared_ptr<IAddon> m_installed;
-  std::shared_ptr<IAddon> m_update;
-};
+using AddonWithUpdate = std::pair<std::shared_ptr<IAddon>, std::shared_ptr<IAddon>>;
 
 /**
  * Class - CAddonRepos
diff --git a/xbmc/filesystem/AddonsDirectory.cpp b/xbmc/filesystem/AddonsDirectory.cpp
index 19955b91de..aba97cae08 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<bool(bool)> CheckOutdatedOrUpdate = [&](bool checkOutdated) -> bool {
-      auto mapEntry = addonsWithUpdate.find(addon->ID());
-      if (mapEntry != addonsWithUpdate.end())
-      {
-        const std::shared_ptr<IAddon>& 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<IAddon>& _)
+      { 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/interfaces/json-rpc/AddonsOperations.cpp b/xbmc/interfaces/json-rpc/AddonsOperations.cpp
index db6c90a191..3daac8cc0b 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::IAddon>& 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 727f418999..cf78a2dba0 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::IAddon>& addon,
                             const CVariant& fields,
                             CVariant& result,
-                            ADDON::CAddonDatabase& addondb,
-                            bool append = false);
+                            bool append);
   };
 }
diff --git a/xbmc/utils/test/TestVariant.cpp b/xbmc/utils/test/TestVariant.cpp
index f5bf687ea6..60eafddf04 100644
--- a/xbmc/utils/test/TestVariant.cpp
+++ b/xbmc/utils/test/TestVariant.cpp
@@ -294,7 +294,7 @@ TEST(TestVariant, empty)
 
   std::map<std::string, std::string> 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;