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

[JSON]Add "dynpath" as property returned by Player.GetItem #15585

Merged
merged 1 commit into from Mar 18, 2019

Conversation

Projects
None yet
6 participants
@DaveTBlake
Copy link
Member

commented Feb 22, 2019

A non-breaking addition to JSON API - add "dynpath" as property returned by Player.GetItem

Dynpath was added to FileItem much earlier but not exposed to API, and it is useful for JSON consumers to be sure able to fetch the actual path of the currenly playing item rather than a virtual path etc.

@garbear

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Where does the documentation for this json field live?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

Where does the documentation for this json field live?

@garbear strictly in the JSON schema (either methods or types), but actually none of List.Item.All currently has descriptions

@rmrector

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

The name confuses me. Looking through the code it looks like this is the real actual path to whatever is playing, would it make sense to use dynpath as the fallback to file, which is very often the same thing? Or replace file logic completely with dynpath?

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

@rmrector no it would not make sense to use "dynpath" as the fallback to "file", and replacing "file" with it would be a breaking change for no gain. They are different things, or can be e.g. when what is playing is from a plugin.

Yes "dynpath" is the real actual path to whatever is playing (at least that is my understanding of CFileitem.dynpath). IMO naming it the same as the internal fileitem property makes it clear exactly where it comes from.

I do recognise there is a gap in the JSON self documentation, none of the properties returned by Player.GetItem are described. This is an historic issue that I don't know how best to solve, but don't want to hold up this addition because of it.

@rmrector

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

Plugin paths and UPNP are the only virtual paths I see that differs from file, but there are more (videodb and musicdb off the top of my head). #13771 explains this behavior, it changed the way plugins and UPNP set a path, but that had side-effects in this JSON-RPC. Kodi 17.6 did always return a real actual path with file because that's what it is for.

And I see this as more evidence that file should always be dynpath, not just the final fallback. JSON-RPC is older than dynpath and the dynpath update never made it here.

JSON-RPC consumers may have a use for virtual paths, but it should be clearly named (like virtualpath) and work for all items with a virtual path, not just plugins.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2019

Yes the intention is for "file" to contain the actual path, however I'm not sure it always succeeded. It tries to pick "file" value from different item properties depending on the item, and that is vulnerable to mistakes given the diversity of things that can be playing and how they are created and added to the queue. It has also been too easy to change underlying core use and forget to update JSON.

And I see this as more evidence that file should always be dynpath, not just the final fallback. JSON-RPC is older than dynpath and the dynpath update never made it here.

@rmrector you almost talked me around to your way of seeing it. My understanding of CFileitem.dynpath was that is was real actual path to whatever is playing, which is just what we want for "file". I tried playing library music, and even an example plugin (Shoutcast2) and dynpath was the actual file playing, great. Then I tried playing a .m3u playlist of music files
"file": "C:\BMusicTest\NuSound\Moods\05 Sahara Sun.mp3"
"dynpath": "musicdb://songs/131248.mp3"

That was unexpected. CFileitem.GetDynPath() is returning path value, in this case a virtual path, because m_strDynPath has not been set. Odd because the item it is processing is CApplication.m_itemCurrentFile which should be definitve, perhaps our understanding of dynpath is wrong.

So what to do for API consumers? I hope we can do something useful, even if that is simply exposing what we have for both dynpath and path.

@rmrector

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

however I'm not sure it always succeeded.

I agree with that. I'm just wary about adding a dynpath if the intention is redundant (even if not every case is covered), removing it will be difficult later. I think we can mostly adjust file now, then revisit a larger adjustment when we can accept a probably-breaking change.

So what to do for API consumers?

For now, how about adding another if for plugin and UPNP paths to file (to cover the noted PR) to return the DynPath, leave the fallback to GetPath, and don't add a separate dynpath property. That covers the major unintended change from 17 that I am aware of, and we've probably been there for awhile so without another specific example it's a safe place to stay until 19.

This could also give us time to consider the best way to get the real actual file, and maybe a separate param for Kodi's virtual path. These two options seem more useful to external API users than exposing DynPath and Path directly, however they differ.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

If we are attempting to fix "file" then an alternative to an if for plugins and Upnp is to replace
object["file"] = item->GetPath().c_str();
with
object["file"] = item->GetDynPath().c_str();
reflecting the general changes made elsewhere in Kodi when DynPath was introduced.

I don't think you or I can judge easily what API consumers need, but I anticipate that if something is useful internally to Kodi it also has a use externally. While the API must control what data can be written, there is no reason to hide the item data Kodi has. A good API would keep properties clean and consistent no matter what happened internally, but we have already failed to do that. They don't really know for sure what will appear in "file", the edge-cases are hard to spot and I can't answer with 100% certainty.

Exposing DynPath and Path (you inspired that) directly gives API consumers the flexibility to accommodate any thing we may have missed in trying to get "file" right. That way the API does not lose information with v18, and I don't think it will become redundant any time soon. I don't think any API consumer would complain about Kodi providing too much information, they only have problems when we hide things.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:JSONAddDynpath branch from 51ec85d to 16ce398 Mar 9, 2019

@rmrector
Copy link
Contributor

left a comment

It is what it is.

@DaveTBlake DaveTBlake merged commit ceb4ce2 into xbmc:master Mar 18, 2019

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:JSONAddDynpath branch Mar 18, 2019

"displaylyricist": { "type": "string"}
"displaylyricist": { "type": "string"},
"mediapath": { "type": "string", "description": "Media source path that identifies the item"},
"dynpath": { "type": "string", "description": "An experimental property for debug purposes, often same as mediapath but when different gives the actual file playing that should also be in file property"}

This comment has been minimized.

Copy link
@ksooo

ksooo Mar 18, 2019

Member

The description is wrong. dynpath is neither experimental nor a debug property. Sorry, but have not spotted this earlier.

@enen92

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

And in some cases we might be exposing sensible information to external clients. For instance, dynpah will expose the resolved URL of a plugin and that might enable easy download of the content when all the author would like to expose is the actual vfs path of the item. Something tells me that if we need to expose this outside of Kodi, something might be wrong

@ksooo

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@FernetMenta any thoughts from you on this? AFAIR, you introduced dynpath and you should have the whole picture.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I have never been a friend of exposing all and everything via an API. I agree with @enen92 that dynpath can hold sensitive information that should not be exposed. It seems that the incorrect information given for the field dynpath refers to the intent of exposing it rather to what the field actually is and does.

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.