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

[python-api] - several additions #9627

Merged
merged 4 commits into from Apr 19, 2016

Conversation

@phil65
Copy link
Member

commented Apr 15, 2016

  • getCurrentContainerId() allows to properly use the core viewtype management for WindowXML instances.
  • setInfo("dbid", xxx) allows to properly emulate database items. This is especially important for stuff like the dataprovider script from @BigNoid. Several context item add-ons depend on listitem.dbid for example.
  • getArt() should also be self-explaining.

@tamland

@ronie

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

allows to properly use the core viewtype management for WindowXML instances.

sounds cool... though i have no idea what it means. ;-)
care to elaborate?

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

Sure.
Most of the time script auhors add content to WindowXML list controls via Control.addItems(), but there is also another way to do this, namely WindowXML.addItem(). When using that, you basically get MediaWindow behaviour for that list (example: you can reference to the listitems via $INFO[ListItem.xxx] from outside of the container).
The problem is that the script author does not know which viewtype is currently active. By adding getCurrentContainerId(), that will become possible.
Main advantage is that skinners can add several viewtypes to their custom XML implementations by adding all the views they want to have to the < views > tag in the window header. They arent hardcoded anymore then by the script author.

EDIT: here is an example of a script with hardcoded viewtype ids: https://github.com/maloep/rom-collection-browser/blob/master/resources/lib/gui.py#L36
That wont be needed anymore after this change, skinners can use any id they want then by adding the id to < views >.

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

I appended another commit. While dealing with the new dbid setters and getters i noticed that items with a VideoinfoTag return -1 for dbid in case they dont have a dbid assigned. I think for skinners it is more convenient to check with IsEmpty() instead of isGreater(-1) for dbids. Agreed, @ronie ?

@phil65 phil65 force-pushed the phil65:python_additions branch from 56a67cb to 1672559 Apr 15, 2016

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

Doesn't the last commit break any skin comparing against -1?

Concerning the possibility for plugins to set the database id this might have much further implications than you might think. A lot of code uses the fact that an item has a database id to differentiate between library and plugin items. With this change that is not always true anymore.
Could you explain the exact use case how this would be used? A plugin must not set an arbitrary database id on an item that is not stored in the video or music database or some core logic ends up thinking that the item is an actual library item and overwrites the information in the database with the one from the plugin which might not be wanted.

@ronie

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

I think for skinners it is more convenient to check with IsEmpty() instead of isGreater(-1) for dbids. Agreed, @ronie ?

for sure, if no info is available it should return empty.

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2016

Doesn't the last commit break any skin comparing against -1?

In theory yes, but breakage will be minimal. We have much much bigger breakers in v17. :)
For me issues mainly came up when doing script work (I am enriching remote metadata with local metadata if available in my add-ons by cross-comparing with local lib)
For estuary for example this doesnt need any change at all.

Could you explain the exact use case how this would be used? A plugin must not set an arbitrary database id on an item that is not stored in the video or music database or some core logic ends up thinking that the item is an actual library item and overwrites the information in the database with the one from the plugin which might not be wanted.

A valid usecase for plugins (esp. for those which get integrated into skins via dynamic content) is to imitate library listitems. A good example probably is service.library.data.provider, which provides listitems for several widget types. Those scripts have to do hacks like https://github.com/BigNoid/service.library.data.provider/blob/master/default.py#L269 in order to allow skinners or other scripts to have the correct dbid at hand. (example for a script using this: https://github.com/phil65/context.extendedinfo.dialog/blob/krypton/addon.py#L23-L29 )
There are many other add-ons which rely on a dbid in order to work ( https://github.com/trakt/script.trakt/blob/master/script.py#L96 is another example )
This PR tries to get rid of those workarounds.

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

@phil65: I've never used service.library.data.provider or any of the other add-ons you mentioned so excuse my ignorance. Are the imitated library items based on real library items just with additional information? Or ar they fake items that don't exist in the database?

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

They are based on real library items. The aim for those plugins basically is to create listitems via python which are identical to the ones delivered via core, all infolabels should be identical.

* example:
* - poster = self.list.getSelectedItem().getArt('poster')
*/
String getArt(const char* key);

This comment has been minimized.

Copy link
@tamland

tamland Apr 16, 2016

Member

Could use some better documentation. 'It returns a value' A value of what? And list the available types please, as setArt does.

This comment has been minimized.

Copy link
@phil65

phil65 Apr 16, 2016

Author Member

Ok.

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

How do we protect core against add-ons using wrong/arbitrary database ids which could either overwrite data from existing library items or try to write library items that don't exist?

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2016

In my opinion a warning in docs should be fine that this should only be used for local items and in combination with the correct mediatype value, I can add that. Would that be sufficient for you?
I will also have that in mind when reviewing plug-ins.

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 16, 2016

Ideally there would be a way to prevent plugins from using invalid database ids in the python interface implementation but that's probably not possible right now or would be too much work so a warning is certainly nice.
Maybe we should change the method to set the id and the type to force plugin authors to set both at the same time.

@phil65 phil65 force-pushed the phil65:python_additions branch from 1672559 to 46f9051 Apr 16, 2016

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2016

@Montellese : I updated the docs, is it fine enough now?

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 17, 2016

Yes, better.

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2016

jenkins build this please.

@phil65 phil65 merged commit 65ce5ef into xbmc:master Apr 19, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
default Merged build finished.
Details

@Razzeee Razzeee added the v17 Krypton label Apr 19, 2016

@Razzeee Razzeee added this to the Krypton 17.0-alpha1 milestone Apr 19, 2016

@ghost

This comment has been minimized.

Copy link

commented Apr 22, 2016

This PR leads to an issue commented in:

65ce5ef

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Already fixed on estuary repository.

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

@phil65 you should push things like that to this repo because otherwise it will be left broken in nightly and for everyone doing development. Also, that's a private repo mapfau can't access..

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

We´re in pre-alpha, I noticed it after the merge, and there are also a lot of other small fixes I dont push on a daily basis....

@tamland

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

Sure, you don't have to push every small fix daily. But you can see how it's not ideal that issues are known and fixes exists, but can't be acquired yet right?

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Then the thing to do is probably to discuss whether the estuary repo should get public, and not instantly attacking someone for his / her workflow (which was agreed on by the team).

@phil65

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

@tamland

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

It was never my intention to attack anyone. sorry for bringing it up

@phil65 phil65 deleted the phil65:python_additions branch Apr 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.