Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add InstallAddon builtin #8419

Merged
merged 1 commit into from Dec 5, 2015

Conversation

Projects
None yet
7 participants
@ronie
Copy link
Member

ronie commented Nov 22, 2015

instead of 'forcing' skins to ship with a dozen addons,
give them an option to install those addons on demand.

forum discussion:
http://forum.kodi.tv/showthread.php?tid=231104

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Nov 22, 2015

Question now comes who should throw the install this add-on yes/no dialog, skin or core :)
Blindly doing installation without asking might be a bit too much?

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Nov 22, 2015

Core should throw a yesno dialog askin the user for confirmation in my opinion.
Nice new tool btw, thx for this.

@ronie

This comment has been minimized.

Copy link
Member Author

ronie commented Nov 22, 2015

InstallAddon will always show a yes/no popup before installation.
it's enabled by default in core: https://github.com/xbmc/xbmc/blob/master/xbmc/addons/AddonInstaller.cpp#L156

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Nov 22, 2015

Another nice addition, thanks ronie.

CAddonInstaller::GetInstance().InstallModal(addonid, addon);
}
else
CLog::Log(LOGERROR, "InstallAddon called with an addonid of an installed addon.");

This comment has been minimized.

@Razzeee

Razzeee Nov 22, 2015

Member

I don't think this should be handled as an error, maybe notice would be enough? If we even have that log level?


AddonPtr addon;
if (!CAddonMgr::GetInstance().GetAddon(addonid, addon))
{

This comment has been minimized.

@Razzeee

Razzeee Nov 22, 2015

Member

Remove this bracket and the closing one

@ronie ronie force-pushed the ronie:installaddon branch from 4f4ca29 to b8ba959 Nov 22, 2015

@ronie

This comment has been minimized.

Copy link
Member Author

ronie commented Nov 22, 2015

updated as suggested by @Razzeee

@Razzeee

This comment has been minimized.

Copy link
Member

Razzeee commented Nov 22, 2015

Code seems fine to me.

*/
static int InstallAddon(const std::vector<std::string>& params)
{
if (params.size())

This comment has been minimized.

@tamland

tamland Nov 23, 2015

Member

Unnecessary. the number you enter in GetOperations ensures the parameter is there


AddonPtr addon;
if (!CAddonMgr::GetInstance().GetAddon(addonid, addon))
CAddonInstaller::GetInstance().InstallModal(addonid, addon);

This comment has been minimized.

@tamland

tamland Nov 23, 2015

Member

Second parameter is an out parameter. so remove line 58

if (!CAddonMgr::GetInstance().GetAddon(addonid, addon))
CAddonInstaller::GetInstance().InstallModal(addonid, addon);
else
CLog::Log(LOGNOTICE, "InstallAddon called with an addonid of an installed addon.");

This comment has been minimized.

@tamland

tamland Nov 23, 2015

Member

This too seems unnecessary to me. Log errors and debug info. There's no need to log that something expected happened..

@tamland

This comment has been minimized.

Copy link
Member

tamland commented Nov 23, 2015

Frankly, Im a bit worried this will just be yet another thing for skins to abuse when you give them tools to explicitly install addon. Wouldn't be better to extend the functionally we already have for plugins that prompts for install when you attempt to use it?

@jeroenpardon

This comment has been minimized.

Copy link
Contributor

jeroenpardon commented Nov 24, 2015

Yet another thing?

Some functions provided by add-ons aren't explicitly started by the user but used by skins to enhance functionality or information. There's no way to prompt for those functions.

This provides a way for the skin to offer "out of the box functionality" without forcing the installation of add-ons. Coupled with a dialog explaining the user why the installation of the add-on is requested provides a much better process than the way it is currently handled. A welcome change imo.

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Nov 24, 2015

Plus the fact that these addons can be uninstalled if the user so wishes - something that can't be done if they're required in the addon.xml.

@ronie ronie force-pushed the ronie:installaddon branch from b8ba959 to c8f7480 Nov 27, 2015

@ronie

This comment has been minimized.

Copy link
Member Author

ronie commented Nov 27, 2015

thanx for the review @tamland , code updated as suggested.

no worries about the possible abuse. currently we have to 'abuse' runplugin() to install addons, when we don't even want to actually run the addon. we just want to install it.
so let's get a built-in that does exactly that and nothing more.

this can be used for other types of addons as well (resources for instance) that can't be covered by runscript() and the likes.

@MartijnKaijser MartijnKaijser modified the milestone: K***** 17.0-alpha1 Dec 4, 2015

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Dec 5, 2015

jenkins build this please

MartijnKaijser added a commit that referenced this pull request Dec 5, 2015

@MartijnKaijser MartijnKaijser merged commit b8705e3 into xbmc:master Dec 5, 2015

1 check passed

default Merged build finished.
Details

@ronie ronie deleted the ronie:installaddon branch Feb 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.