builtins: pass arguments of RunAddon() to ActivateWindow() (fixes JSON-RPC's Addons.ExecuteAddon) #2072

Merged
merged 1 commit into from Nov 11, 2013

Conversation

Projects
None yet
6 participants
Owner

Montellese commented Jan 13, 2013

This makes sure to pass on any parameters provided in the call to the RunAddon() builtin function on to any resulting ActivateWindow() call and therefore to the plugin being executed. We already pass on the parameters for any non-media addons.

This fixes a bug reported in the forum when using JSON-RPC's Addons.ExecuteAddon on a video or music addon with specific parameters (e.g. to jump to a specific section of the youtube plugin).

@ghost

ghost commented Apr 6, 2013

i don't think we support such urls.

Owner

Montellese commented Apr 6, 2013

So is there some "standard" way or does every plugin do it the way they want?

EDIT: Either way there's probably no easy way to distinguish between an URL path and an URL GET parameter passed in so not sure how to handle that.

@ghost

ghost commented Apr 6, 2013

damn, we do.

Member

jmarshallnz commented Apr 6, 2013

Every plugin does it the way they want. Though one presumes the worst is the URL I gave, so the most general is probably a (relative) path + params.

Owner

Montellese commented Apr 7, 2013

Should the (relative) path + params be an extra parameter or just appended to the plugin:/// URL? That probably means that I have to deprecate something in the JSON-RPC API but I'll have to take a closer look.

Member

jmarshallnz commented Apr 7, 2013

The builtin will need it as another param I guess. Kinda messy as it screws around with the multiple-parameter version of RunPlugin() in that same block? IIRC @ulion has some work somewhere for named params for builtins.

Contributor

ulion commented Apr 7, 2013

why not json-rpc construct the full pluginurl and then RunAddon(fullpluginurl) ?

Owner

Montellese commented Apr 7, 2013

Because then we'd have to extract the addon's ID from the URL to be able to check if it's valid and it would break the current API of Addons.ExecuteAddon in JSON-RPC.

Contributor

ulion commented Apr 7, 2013

then we may need keep the ExecuteAddon's interface but add ability to parse the addonid in the format 'plugin://foo.bar/some/directory/structure/some_file', it's easy by using a CURL object. and finally construct the full plugin url call RunAddon(fullpluginurl). but for the param list style params which finally be called with RunScript in RunAddon execution code, I'm not sure how to distinguish it.

Hallo, any news here? It would be great if this pull request will be merge into master. We need it for cross add-on communication.

Owner

Montellese commented Sep 14, 2013

Since quite a few people have inquired about this problem in the last few weeks I've decided to look into it again.
What I've come up with is an extension of the JSON schema of Addons.ExecuteAddon which allows to pass in a string that is simply appended to the plugin:// URL. If the parameters are provided through an array or a key-value map they are passed on to RunAddon() as separate parameters and there they are combined into ?key1=value1&key2=value2.

There is an assumption in the code that a full URL path passed in by a JSON-RPC client starts with either "/" or "?" to be able to distinguish between a full URL path and a simple key-value parameter. But if the full URL path wouldn't start with "/" or "?" it wouldn't really be a valid URL.

Owner

MartijnKaijser commented Oct 12, 2013

@jmarshallnz to review

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Oct 13, 2013

xbmc/interfaces/json-rpc/AddonsOperations.cpp
@@ -190,6 +190,13 @@ JSONRPC_STATUS CAddonsOperations::ExecuteAddon(const CStdString &method, ITransp
argv += StringUtils::Paramify(it->asString());
}
}
+ else if (params.isString())
+ {
+ if (params.empty())
+ params.clear();
@jmarshallnz

jmarshallnz Oct 13, 2013

Member

This code looks a bit weird - I'm guessing .empty() checks if the string is empty, and .clear() then resets it from a string type?

@Montellese

Montellese Oct 21, 2013

Owner

TBH I have no clue what I was thinking when I wrote this. It doesn't do anything to params. I'll remove it.

Member

jmarshallnz commented Oct 13, 2013

Looks OK - can't think of a better way to do it. Perhaps with time we can sort out a unified way to pass params to and from plugins rather than allowing them to do whatever you want.

jmarshallnz was assigned Oct 17, 2013

Owner

Montellese commented Nov 11, 2013

Completely forgot about this. Fixed the last comment from @jmarshallnz and rebased onto master. jenkins build this please.

Owner

Montellese commented Nov 11, 2013

Hm are the ATV2 and OSX builds on jenkins broken in general? Doesn't look like a problem specific to this PR.

Owner

MartijnKaijser commented Nov 11, 2013

@Montellese
no idea why iOS is failing. It seems to happen for all PRs. Maybe @Memphiz or @davilla can have a look.

Owner

Memphiz commented Nov 11, 2013

on it...

Owner

Montellese commented Nov 11, 2013

jenkins build this please

Owner

Montellese commented Nov 11, 2013

@jmarshallnz: ok to inject?

jmarshallnz merged commit 688a43a into xbmc:master Nov 11, 2013

1 check passed

default Merged build #572 succeeded in 49 min
Details

Montellese deleted the Montellese:builtins_runaddon_fix branch Nov 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment