Merge pull request #11664 from tamland/addons_broken_2

[addons] auto-disable broken addon on update

8 files changed, 46 insertions, 79 deletions

diff --git a/addons/resource.language.en_gb/resources/strings.po b/addons/resource.language.en_gb/resources/strings.po
index 7187ecda65..016aa88a09 100644
--- a/addons/resource.language.en_gb/resources/strings.po
+++ b/addons/resource.language.en_gb/resources/strings.po
@@ -14786,17 +14786,19 @@ msgctxt "#24093"
 msgid "Checking %s..."
 msgstr ""
 
-#empty string with id 24094
+#: xbmc/addons/AddonInstaller.cpp
+msgctxt "#24094"
+msgid "Add-on disabled due to being marked broken in repository."
+msgstr ""
 
 #: xbmc/addons/GUIDialogAddonInfo.cpp
 msgctxt "#24095"
 msgid "Local package cache"
 msgstr ""
 
-#: xbmc/addons/Repository.cpp
 #: addons/skin.estuary/xml/DialogAddonInfo.xml
 msgctxt "#24096"
-msgid "Add-on is incompatible or has been marked broken in repository."
+msgid "Add-on has been marked broken in repository."
 msgstr ""
 
 #: xbmc/addons/Repository.cpp
diff --git a/xbmc/addons/AddonDatabase.cpp b/xbmc/addons/AddonDatabase.cpp
index 67ad41d247..ad655d6ae8 100644
--- a/xbmc/addons/AddonDatabase.cpp
+++ b/xbmc/addons/AddonDatabase.cpp
@@ -539,7 +539,6 @@ bool CAddonDatabase::GetAvailableVersions(const std::string& addonId,
         "WHERE "
         "repo.checksum IS NOT NULL AND repo.checksum != '' "
         "AND EXISTS (SELECT * FROM installed WHERE installed.addonID=repoID AND installed.enabled=1) "
-        "AND NOT EXISTS (SELECT * FROM  broken WHERE broken.addonID=addons.addonID) "
         "AND addons.addonID='%s'", addonId.c_str());
 
     m_pDS->query(sql.c_str());
diff --git a/xbmc/addons/AddonInstaller.cpp b/xbmc/addons/AddonInstaller.cpp
index 9f2480ce88..55d300904d 100644
--- a/xbmc/addons/AddonInstaller.cpp
+++ b/xbmc/addons/AddonInstaller.cpp
@@ -228,14 +228,15 @@ void CAddonInstaller::Install(const std::string& addonId, const AddonVersion& ve
 }
 
 bool CAddonInstaller::DoInstall(const AddonPtr &addon, const RepositoryPtr& repo,
-    const std::string &hash /* = "" */, bool background /* = true */, bool modal /* = false */)
+    const std::string &hash /* = "" */, bool background /* = true */, bool modal /* = false */,
+    bool autoUpdate /* = false*/)
 {
   // check whether we already have the addon installing
   CSingleLock lock(m_critSection);
   if (m_downloadJobs.find(addon->ID()) != m_downloadJobs.end())
     return false;
 
-  CAddonInstallJob* installJob = new CAddonInstallJob(addon, repo, hash);
+  CAddonInstallJob* installJob = new CAddonInstallJob(addon, repo, hash, autoUpdate);
   if (background)
   {
     // Workaround: because CAddonInstallJob is blocking waiting for other jobs, it needs to be run
@@ -431,9 +432,18 @@ void CAddonInstaller::InstallUpdates()
   for (const auto& addon : updates)
   {
     if (!CAddonMgr::GetInstance().IsBlacklisted(addon->ID()))
-      CAddonInstaller::GetInstance().InstallOrUpdate(addon->ID());
+    {
+      AddonPtr toInstall;
+      RepositoryPtr repo;
+      std::string hash;
+      if (CAddonInstallJob::GetAddonWithHash(addon->ID(), repo, toInstall, hash))
+        DoInstall(toInstall, repo, hash, true, false, true);
+    }
   }
+}
 
+void CAddonInstaller::InstallUpdatesAndWait()
+{
   CSingleLock lock(m_critSection);
   if (!m_downloadJobs.empty())
   {
@@ -464,13 +474,15 @@ int64_t CAddonInstaller::EnumeratePackageFolder(std::map<std::string,CFileItemLi
   return size;
 }
 
-CAddonInstallJob::CAddonInstallJob(const AddonPtr &addon, const AddonPtr &repo, const std::string &hash /* = "" */)
+CAddonInstallJob::CAddonInstallJob(const AddonPtr &addon, const AddonPtr &repo,
+    const std::string &hash, bool isAutoUpdate)
   : m_addon(addon),
     m_repo(repo),
-    m_hash(hash)
+    m_hash(hash),
+    m_isAutoUpdate(isAutoUpdate)
 {
   AddonPtr dummy;
-  m_update = CAddonMgr::GetInstance().GetAddon(addon->ID(), dummy, ADDON_UNKNOWN, false);
+  m_isUpdate = CAddonMgr::GetInstance().GetAddon(addon->ID(), dummy, ADDON_UNKNOWN, false);
 }
 
 bool CAddonInstallJob::GetAddonWithHash(const std::string& addonID, RepositoryPtr& repo,
@@ -605,20 +617,30 @@ bool CAddonInstallJob::DoWork()
   g_localizeStrings.LoadAddonStrings(URIUtils::AddFileToFolder(m_addon->Path(), "resources/language/"),
       CServiceBroker::GetSettings().GetString(CSettings::SETTING_LOCALE_LANGUAGE), m_addon->ID());
 
-  ADDON::OnPostInstall(m_addon, m_update, IsModal());
+  ADDON::OnPostInstall(m_addon, m_isUpdate, IsModal());
 
   {
     CAddonDatabase database;
     database.Open();
     database.SetOrigin(m_addon->ID(), m_repo ? m_repo->ID() : "");
-    if (m_update)
+    if (m_isUpdate)
       database.SetLastUpdated(m_addon->ID(), CDateTime::GetCurrentDateTime());
   }
 
   CEventLog::GetInstance().Add(
-    EventPtr(new CAddonManagementEvent(m_addon, m_update ? 24065 : 24064)),
+    EventPtr(new CAddonManagementEvent(m_addon, m_isUpdate ? 24065 : 24064)),
     !IsModal() && CServiceBroker::GetSettings().GetBool(CSettings::SETTING_ADDONS_NOTIFICATIONS), false);
 
+  if (m_isAutoUpdate && !m_addon->Broken().empty())
+  {
+    CLog::Log(LOGDEBUG, "CAddonInstallJob[%s]: auto-disabling due to being marked as broken", m_addon->ID().c_str());
+    CAddonMgr::GetInstance().DisableAddon(m_addon->ID());
+    CEventLog::GetInstance().Add(
+        EventPtr(new CAddonManagementEvent(m_addon, 24094)),
+        CServiceBroker::GetSettings().GetBool(CSettings::SETTING_ADDONS_NOTIFICATIONS),
+        false);
+  }
+
   // and we're done!
   MarkFinished();
   return true;
@@ -725,7 +747,7 @@ bool CAddonInstallJob::Install(const std::string &installFrom, const AddonPtr& r
             return false;
           }
 
-          CAddonInstallJob dependencyJob(addon, repoForDep, hash);
+          CAddonInstallJob dependencyJob(addon, repoForDep, hash, false);
 
           // pass our progress indicators to the temporary job and don't allow it to
           // show progress or information updates (no progress, title or text changes)
diff --git a/xbmc/addons/AddonInstaller.h b/xbmc/addons/AddonInstaller.h
index 57a38c9624..d42617adf2 100644
--- a/xbmc/addons/AddonInstaller.h
+++ b/xbmc/addons/AddonInstaller.h
@@ -95,6 +95,7 @@ public:
   bool HasJob(const std::string& ID) const;
 
   /*! Install update and block until all updates have installed. */
+  void InstallUpdatesAndWait();
   void InstallUpdates();
 
   void OnJobComplete(unsigned int jobID, bool success, CJob* job);
@@ -129,7 +130,7 @@ private:
    \return true on successful install, false on failure.
    */
   bool DoInstall(const ADDON::AddonPtr &addon, const ADDON::RepositoryPtr &repo,
-      const std::string &hash = "", bool background = true, bool modal = false);
+      const std::string &hash = "", bool background = true, bool modal = false, bool autoUpdate = false);
 
   /*! \brief Check whether dependencies of an addon exist or are installable.
    Iterates through the addon's dependencies, checking they're installed or installable.
@@ -153,7 +154,8 @@ private:
 class CAddonInstallJob : public CFileOperationJob
 {
 public:
-  CAddonInstallJob(const ADDON::AddonPtr &addon, const ADDON::AddonPtr &repo, const std::string &hash = "");
+  CAddonInstallJob(const ADDON::AddonPtr& addon, const ADDON::AddonPtr& repo,
+      const std::string& hash, bool isAutoUpdate);
 
   virtual bool DoWork();
 
@@ -185,7 +187,8 @@ private:
   ADDON::AddonPtr m_addon;
   ADDON::AddonPtr m_repo;
   std::string m_hash;
-  bool m_update;
+  bool m_isUpdate;
+  bool m_isAutoUpdate;
 };
 
 class CAddonUnInstallJob : public CFileOperationJob
diff --git a/xbmc/addons/AddonManager.cpp b/xbmc/addons/AddonManager.cpp
index 4c7c1cd5a3..8dd7395c35 100644
--- a/xbmc/addons/AddonManager.cpp
+++ b/xbmc/addons/AddonManager.cpp
@@ -848,19 +848,7 @@ bool CAddonMgr::IsAddonInstalled(const std::string& ID)
 
 bool CAddonMgr::CanAddonBeInstalled(const AddonPtr& addon)
 {
-  if (addon == NULL)
-    return false;
-
-  CSingleLock lock(m_critSection);
-  // can't install already installed addon
-  if (IsAddonInstalled(addon->ID()))
-    return false;
-
-  // can't install broken addons
-  if (!addon->Broken().empty())
-    return false;
-
-  return true;
+  return addon != nullptr &&!IsAddonInstalled(addon->ID());
 }
 
 bool CAddonMgr::CanUninstall(const AddonPtr& addon)
diff --git a/xbmc/addons/AddonSystemSettings.cpp b/xbmc/addons/AddonSystemSettings.cpp
index 6f245936d5..f0f3dad990 100644
--- a/xbmc/addons/AddonSystemSettings.cpp
+++ b/xbmc/addons/AddonSystemSettings.cpp
@@ -148,7 +148,7 @@ std::vector<std::string> CAddonSystemSettings::MigrateAddons(std::function<void(
       CRepositoryUpdater::GetInstance().Await();
 
     CLog::Log(LOGINFO, "ADDON: waiting for add-ons to update...");
-    CAddonInstaller::GetInstance().InstallUpdates();
+    CAddonInstaller::GetInstance().InstallUpdatesAndWait();
   }
 
   auto incompatible = getIncompatible();
diff --git a/xbmc/addons/Repository.cpp b/xbmc/addons/Repository.cpp
index e64a3665c1..ce1e8ab7ed 100644
--- a/xbmc/addons/Repository.cpp
+++ b/xbmc/addons/Repository.cpp
@@ -257,48 +257,5 @@ bool CRepositoryUpdateJob::DoWork()
   }
 
   database.UpdateRepositoryContent(m_repo->ID(), m_repo->Version(), newChecksum, addons);
-
-  //Notify about broken status changes
-  for (const auto& addon : addons)
-  {
-    AddonPtr localAddon;
-    if (!CAddonMgr::GetInstance().GetAddon(addon->ID(), localAddon))
-      continue;
-
-    if (localAddon && localAddon->Version() > addon->Version())
-      //We have a newer version locally
-      continue;
-
-    AddonPtr oldAddon;
-    database.GetAddon(addon->ID(), oldAddon);
-
-    if (database.GetAddonVersion(addon->ID()).first > addon->Version())
-      //Newer version in db (ie. in a different repo)
-      continue;
-
-    std::string broken = addon->Broken();
-    bool isBroken = !addon->Broken().empty();
-    bool isBrokenInDb = oldAddon && !oldAddon->Broken().empty();
-    if (isBroken && !isBrokenInDb)
-    {
-      //newly broken
-      if (HELPERS::ShowYesNoDialogLines(CVariant{addon->Name()}, CVariant{24096}, CVariant{24097}, CVariant{""})
-        == DialogResponse::YES)
-      {
-        CAddonMgr::GetInstance().DisableAddon(addon->ID());
-      }
-
-      CLog::Log(LOGDEBUG, "CRepositoryUpdateJob[%s] addon '%s' marked broken. reason: \"%s\"",
-           m_repo->ID().c_str(), addon->ID().c_str(), broken.c_str());
-
-      CEventLog::GetInstance().Add(EventPtr(new CAddonManagementEvent(addon, 24096)));
-    }
-    else if (!isBroken && isBrokenInDb)
-    {
-      //Unbroken
-      CLog::Log(LOGDEBUG, "CRepositoryUpdateJob[%s] addon '%s' unbroken",
-          m_repo->ID().c_str(), addon->ID().c_str());
-    }
-  }
   return true;
 }
diff --git a/xbmc/addons/RepositoryUpdater.cpp b/xbmc/addons/RepositoryUpdater.cpp
index ca008ce8a5..659c39012d 100644
--- a/xbmc/addons/RepositoryUpdater.cpp
+++ b/xbmc/addons/RepositoryUpdater.cpp
@@ -87,11 +87,7 @@ void CRepositoryUpdater::OnJobComplete(unsigned int jobID, bool success, CJob* j
 
     if (CServiceBroker::GetSettings().GetInt(CSettings::SETTING_ADDONS_AUTOUPDATES) == AUTO_UPDATES_ON)
     {
-      for (const auto& addon : updates)
-      {
-        if (!CAddonMgr::GetInstance().IsBlacklisted(addon->ID()))
-          CAddonInstaller::GetInstance().InstallOrUpdate(addon->ID());
-      }
+      CAddonInstaller::GetInstance().InstallUpdates();
     }
 
     ScheduleUpdate();