[addons] make binary addon management independent #12299

AlwinEsch · 2017-06-14T05:58:43Z

Description

This changes add a new binary addon manager who handle the
addons on request.

This way allow the use of several instances on the same addon.

Currently only used on Visualization and Screensaver, but other binary
addon types becomes on next steps changed to use them here.

Further becomes on next changes everything related to binary addons
moved to the new folder "./xbmc/addons/binary-addons".

The old system is not changed!

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the Code guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the CONTRIBUTING document
I have added tests to cover my change
All new and existing tests passed

tamland · 2017-06-14T16:14:52Z

Yes, architecturally I think this is fine.

Ideally GetInstalledBinaryAddonPaths would return a CAddon with all the info CBinaryAddonManger needs instead of the paths directly. But if you can't make it work, that's ok. It will be easy to fix later once CAddonDll and CAddon are separated.

AlwinEsch · 2017-06-15T07:38:30Z

The request is now complete updated and use a working way.

ksooo

some questions and suggestions...

xbmc/ServiceManager.cpp

+  m_binaryAddonManager.reset(new ADDON::CBinaryAddonManager());
+  if (!m_binaryAddonManager->Init())
+  {
+    CLog::Log(LOGFATAL, "CServiceManager::Init: Unable to start CBinaryAddonManager");


xbmc/ServiceManager.cpp

+  return *m_binaryAddonManager.get();
+}
+
+


xbmc/addons/AddonManager.cpp

+  std::vector<CAddonBuilder> builders;
+  m_database.GetInstalled(builders);
+
+  for (auto& builder : builders)


xbmc/addons/AddonManager.h

@@ -117,6 +117,8 @@ namespace ADDON

    bool GetInstallableAddons(VECADDONS& addons, const TYPE &type);

+    bool GetInstalledBinaryAddonPaths(std::vector<std::pair<std::string, bool>>& binaryAddonList);


xbmc/addons/binary-addons/BinaryAddonBase.cpp

+CBinaryAddonBase::CBinaryAddonBase(const std::string& addonPath)
+{
+
+  m_usable = false;


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+CBinaryAddonManager::CBinaryAddonManager()
+{
+
+}


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+CBinaryAddonManager::~CBinaryAddonManager()
+{
+
+}


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+  std::vector<std::pair<std::string, bool>> binaryAddonList;
+  if (!CAddonMgr::GetInstance().GetInstalledBinaryAddonPaths(binaryAddonList))
+  {
+    CLog::Log(LOGERROR, "Binary Addons: failed to get list");


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+
+  for (auto addon : binaryAddonList)
+  {
+    BinaryAddonBase base = std::make_shared<CBinaryAddonBase>(addon.first);


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+      continue;
+    m_installedAddons[base->ID()] = base;
+    if (addon.second)
+      m_enabledAddons[base->ID()] = base;


ksooo

Here we go. Lots of stuff...

xbmc/ServiceManager.cpp

  m_addonMgr.reset(new ADDON::CAddonMgr());
  if (!m_addonMgr->Init())
  {
    CLog::Log(LOGFATAL, "CServiceManager::Init: Unable to start CAddonMgr");
    return false;
  }

+  if (!m_binaryAddonManager->Init())
+  {
+    CLog::Log(LOGFATAL, "CServiceManager::Init: Unable to start CBinaryAddonManager");


xbmc/addons/AddonInfo.h

@@ -143,40 +101,31 @@ namespace ADDON
    static TYPE TranslateSubContent(const std::string &content);
    //@}

-    InfoMap extrainfo; // defined here, but removed in future with cpluff
-
-  private:


xbmc/addons/AddonManager.h

@@ -117,6 +117,8 @@ namespace ADDON

    bool GetInstallableAddons(VECADDONS& addons, const TYPE &type);

+    bool GetInstalledBinaryAddonPaths(std::vector<std::tuple<std::string, bool, std::string>>& binaryAddonList);


xbmc/addons/binary-addons/AddonDll.cpp

 }

+


xbmc/addons/binary-addons/AddonDll.cpp

+  AddonPtr addon = CServiceBroker::GetBinaryAddonManager().GetRunningAddon(ID());
+  if (addon)
+    return addon;
+  return AddonPtr();


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+      }
+      m_installedAddons[base->ID()] = base;
+      if (std::get<1>(addon))
+        m_enabledAddons[base->ID()] = base;


xbmc/addons/binary-addons/BinaryAddonManager.h

+     * @param[in] type Add-on type to check installed
+     * @return true if given type is installed
+     */
+    bool HasInstalledAddons(const TYPE &type);


xbmc/addons/binary-addons/BinaryAddonManager.h

+     * @param[in] type Add-on type to check enabled
+     * @return true if given type is enabled
+     */
+    bool HasEnabledAddons(const TYPE &type);


xbmc/addons/binary-addons/BinaryAddonManager.h

+     *                        If a type id becomes added are only add-ons
+     *                        returned who match them. Default is for all types.
+     */
+    void GetAddonInfos(BinaryAddonBaseList& addonInfos, bool enabledOnly, const TYPE &type);


xbmc/addons/binary-addons/BinaryAddonManager.h

+     *                 not a nullptr is returned
+     * @return add-on information pointer of installed add-on
+     */
+    const BinaryAddonBasePtr GetInstalledAddonInfo(const std::string& addonId, const TYPE &type = ADDON_UNKNOWN);


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+  CSingleLock lock(m_critSection);
+
+  BINARY_ADDON_LIST binaryAddonList;
+  CAddonMgr::GetInstance().GetInstalledBinaryAddonPaths(binaryAddonList);


AlwinEsch · 2017-06-16T04:05:04Z

Is complete updated, instead of the CAddon use it CAddonInfo who was created by CAddonBuilder.

ksooo

Very nice. Just some nitpicking...

xbmc/addons/AddonBuilder.h

@@ -61,6 +61,8 @@ class CAddonBuilder
  const std::string& GetId() const { return m_addonInfo.m_id; }
  const AddonVersion& GetVersion() const { return m_addonInfo.m_version; }

+  const CAddonInfo& GetAddonInfo() { return m_addonInfo; }


xbmc/ServiceManager.cpp

  m_addonMgr.reset(new ADDON::CAddonMgr());
  if (!m_addonMgr->Init())
  {
    CLog::Log(LOGFATAL, "CServiceManager::Init: Unable to start CAddonMgr");
    return false;
  }

+  if (!m_binaryAddonManager->Init())
+  {
+    CLog::Log(LOGFATAL, "CServiceManager::Init: Unable to initialize CBinaryAddonManager");


xbmc/addons/AddonManager.cpp

@@ -564,6 +567,40 @@ bool CAddonMgr::FindInstallableById(const std::string& addonId, AddonPtr& result
  return true;
 }

+bool CAddonMgr::GetInstalledBinaryAddons(BINARY_ADDON_LIST& binaryAddonList)


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+  CSingleLock lock(m_critSection);
+
+  auto addon = m_installedAddons.find(addonId);
+  if (addon != m_installedAddons.end() && addon->second)


xbmc/addons/binary-addons/BinaryAddonType.cpp

+  return URIUtils::AddFileToFolder(m_path, m_libname);
+}
+
+const char* CBinaryAddonType::GetPlatformLibraryName(const TiXmlElement* element)


tamland · 2017-06-16T07:32:09Z

Is complete updated, instead of the CAddon use it CAddonInfo who was created by CAddonBuilder.

Why didn't using CAddon work?

AlwinEsch · 2017-06-16T07:58:54Z

Make no sense to use, it need only to have the CAddonInfo (who becomes also stored) for binary addon creation.

CAddon becomes created with CAddonInfo as value, the binary part need then only to read the CAddonInfo and destroy the CAddon.

With that he gets it directly:

binaryAddonList.push_back(BINARY_ADDON_LIST_ENTRY(!IsAddonDisabled(cp_addon->identifier), std::move(builder.GetAddonInfo())));

Otherwise must be the

AddonPtr addon;
...
addon = builder.Build();
binaryAddonList.push_back(BINARY_ADDON_LIST_ENTRY(!IsAddonDisabled(cp_addon->identifier), addon));
...

added who then make never needed work.

AlwinEsch · 2017-06-16T11:57:06Z

jenkins build this please

Needed from external places if something on CAddon construction need to changed.

Is for binary addons who do this job byself and not needed from here.

tamland · 2017-06-17T10:36:09Z

Make no sense to use, it need only to have the CAddonInfo (who becomes also stored) for binary addon creation.

CAddon becomes created with CAddonInfo as value, the binary part need then only to read the CAddonInfo and destroy the CAddon.

Everything else that interface with addon manager use IAddon as the 'info' so it makes sense to do the same for binary addons. Even if you don't to directly need it. When you don't set an extension point there is 0 overhead of Build() (it just calls the CAddon constructor). If it's impossible to do now, at the very least be prepared to fix it later

xbmc/addons/binary-addons/BinaryAddonManager.cpp

+  }
+
+  for (auto addon : binaryAddonList)
+    AddAddonBaseEntry(addon);


xbmc/addons/binary-addons/BinaryAddonManager.cpp

+bool CBinaryAddonManager::AddAddonBaseEntry(BINARY_ADDON_LIST_ENTRY& entry)
+{
+  BinaryAddonBasePtr base = std::make_shared<CBinaryAddonBase>(entry.second);
+  if (!base->Create())


This changes add a new binary addon manager who handle the addons on request. This way allow the use of several instances on the same addon. Currently only used on Visualization and Screensaver, but other binary addon types becomes on next steps changed to use them here. Further becomes on next changes everything related to binary addons moved to the new folder "./xbmc/addons/binary-addons". The old system is not changed!

AlwinEsch · 2017-06-17T12:32:36Z

jenkins build this please

AlwinEsch changed the title ~~[addons] add CBinaraAddonManager (currently only to ask)~~ [addons] add CBinaryAddonManager (currently only to ask) Jun 14, 2017

AlwinEsch force-pushed the another-addon-way branch from 0c9152d to 0bf12ba Compare June 14, 2017 06:32

AlwinEsch force-pushed the another-addon-way branch 2 times, most recently from cf05f24 to bfab227 Compare June 15, 2017 07:19

AlwinEsch changed the title ~~[addons] add CBinaryAddonManager (currently only to ask)~~ [addons] make binary addon management independent Jun 15, 2017

AlwinEsch requested review from ksooo, tamland and Paxxi June 15, 2017 07:22

AlwinEsch added Component: Binary add-ons Type: Feature non-breaking change which adds functionality v18 Leia labels Jun 15, 2017

AlwinEsch added this to the L 18.0-alpha1 milestone Jun 15, 2017

ksooo requested changes Jun 15, 2017

View reviewed changes

AlwinEsch force-pushed the another-addon-way branch from bfab227 to a94c70f Compare June 15, 2017 11:27

tamland reviewed Jun 15, 2017

View reviewed changes

AlwinEsch force-pushed the another-addon-way branch 2 times, most recently from b70b429 to 0bb9b15 Compare June 16, 2017 03:55

ksooo requested changes Jun 16, 2017

View reviewed changes

AlwinEsch force-pushed the another-addon-way branch from 0bb9b15 to 5119f63 Compare June 16, 2017 08:27

AlwinEsch added 2 commits June 17, 2017 03:27

[addons] update on CAddonInfo setters to set values

eda1d9a

Needed from external places if something on CAddon construction need to changed.

[addons} add option to ignore extension parse on manager

63ccea5

Is for binary addons who do this job byself and not needed from here.

AlwinEsch force-pushed the another-addon-way branch 2 times, most recently from 2ce6afe to 31e90a1 Compare June 17, 2017 07:08

AlwinEsch force-pushed the another-addon-way branch from 31e90a1 to fa02f65 Compare June 17, 2017 08:17

tamland reviewed Jun 17, 2017

View reviewed changes

xbmc/addons/binary-addons/BinaryAddonManager.cpp Outdated

}

for (auto addon : binaryAddonList)

AddAddonBaseEntry(addon);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tamland reviewed Jun 17, 2017

View reviewed changes

xbmc/addons/binary-addons/BinaryAddonManager.cpp

bool CBinaryAddonManager::AddAddonBaseEntry(BINARY_ADDON_LIST_ENTRY& entry)

{

BinaryAddonBasePtr base = std::make_shared<CBinaryAddonBase>(entry.second);

if (!base->Create())

This comment was marked as spam.

Sign in to view

AlwinEsch force-pushed the another-addon-way branch from fa02f65 to 105c23b Compare June 17, 2017 12:06

AlwinEsch merged commit 5e81f25 into xbmc:master Jun 17, 2017

AlwinEsch deleted the another-addon-way branch June 27, 2017 20:20

		@@ -117,6 +117,8 @@ namespace ADDON

		bool GetInstallableAddons(VECADDONS& addons, const TYPE &type);

		bool GetInstalledBinaryAddonPaths(std::vector<std::pair<std::string, bool>>& binaryAddonList);

[addons] make binary addon management independent #12299

[addons] make binary addon management independent #12299

Conversation

AlwinEsch commented Jun 14, 2017 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

tamland commented Jun 14, 2017 • edited

AlwinEsch commented Jun 15, 2017

ksooo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ksooo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

AlwinEsch commented Jun 16, 2017

ksooo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tamland commented Jun 16, 2017

AlwinEsch commented Jun 16, 2017 • edited

AlwinEsch commented Jun 16, 2017

tamland commented Jun 17, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

AlwinEsch commented Jun 17, 2017

AlwinEsch commented Jun 14, 2017 •

edited

tamland commented Jun 14, 2017 •

edited

AlwinEsch commented Jun 16, 2017 •

edited