Add the ability to store more than one online id

[phate89]

Right now online ids are pretty useless. There's no way to know where are they from. Since there are always more addons that needs to identify episodes, tvshows and movies with online info I thought to add a way to store those values in the db (from scrapers and addons).
I used the structure created from #1549 and I joined m_strIMDBNumber and m_strUniqueId in a map that can contain more than one online id.

At start I thought to move the id to details (to avoid yet another join in the views) since it's not a common display info but is already linked in LISTITEM_IMDBNUMBER (imdbnumber) so this will break current behavior. Is imdbnumber property really used?
When kodi upgrades the database it checks if the existing online id starts with "tt". If it does it sets the type to "imdb". It's the only type i can safely recognize from the id. If you know other types please tell me.

I'm not sure how to handle editing of online ids via json. there are 3 options (the default one is the one used from the scraper):

1. Total freedom. It's possible add, delete, edit and change the default one
2. Mixed. It's possible to edit all values (including the default one) but you can't change what the default one is. If you delete it we restore the old one.
3. Strict. You can delete, add and change the ones that are not default. You can't change the default one

@phil65 is this what you wanted? ;)

[MartijnKaijser]

not only phil65 wanted this but whole nations if devs :)

[phil65]

I was probably the one who annoyed phate long enough. :)
Very cool, thx for tacklin that request!

[Tolriq]

@phate89 can you describe more the feature ?
This sounds like something long wanted but JSON definition sounds quite strange. (Supporting multiple ids and a way to know what is the id, beside searching the tt)

Imdb number is used by nearly all remotes to either link to imdb or gather additional data. Is that value still populated ? How does updating this field via json / nfo update the copy in unique ids ?

And describe the JSON part that you wanted to implement ?
You add a Video.OnlineIDs definition but it's not used anywhere.

If it's only pairs of values, then the ids should be an object with 2 values like type / value and uniqueid be uniqueids and return an array of those.

cc @Montellese

Edit : Nice 10000 PR number

[phate89]

right now you have a "imdbnumber" field but you don't know what it contains. might be the imdb id or tvdb id or whatever the scraper set it. With this feature kodi will allow more than one uniqueid and also have a name to define it (so we now know where the id comes from).
So when the scraper and addons will support it for example the movie x men apocalypse will have via json:
uniqueid:{"tmdb":"246655", "imdb":"tt3385516"}
And addons will be able to add info too. So let's say trakt addon matches the movie to its database. It will add its id so we will have:
uniqueid:{"tmdb":"246655", "imdb":"tt3385516","trakt":"149999"}
I created OnlineIDs at start before I saw #1549 . It already created a structure for this feature planning it for the future (only in episode) so I used it .I just forgot to delete it..
The json result structure is like in the result above. Imho is cleaner than a list of objects..

[Tolriq]

Well uniqueid that contains multiple ids is a strange name ;) But yes this covers an often asked feature :)

About json you need to think also about parsing and documentation.

Proposal is it's an object that's all. No real definition no way to validate or really know what it returns.

Fixed defintion and arrays allow control and documentation. We can define what is the type of the keys and what is the type of the value too.

Same for filtering how would you define a filter on those without a definition ? Find movie with imdb xxxx or movies with a trakt id.

Anyway @Montellese call but I really do not think "object" is a correct way to describe an arrays of pairs.

[phate89]

Why a complex structure is better to validate? More simple it is easier it is. There's no way to get wrong a list of pairs It's already done for every property...It's actually harder and more error prone a complex object.. More you nest the info easier is to make a mistake..
Also if you have properties with a name you can only call it object.. What you want it's not for sure an array of pairs..
Of course it's (I guess) the only type defined this way but it's there since 2012 and montellese proposed this structure...
About filtering I'm not sure because I'm not familiar with our json filtering..
but ofc montellese have a better understanding of our json..

[Montellese]

I don't think it makes a real difference whether it's an object or a array of string pairs in this case. The object is well defined because a property name is always a string and the API definition states that any additional property must also be of type string so you definitly know what the types will be. And looping over all properties in an object or looping over an array of pairs isn't that different either. It might have been better to call the property "uniqueids" instead of "uniqueid". But that and changing it to an array would be backwards incompatible. I'm not sure if anyone is already using this so that might not be such a big deal.

[Tolriq]

Since already a few incompatible changes for 17 it's time to do things right :)

If going the object road, then we need to go the same as art, and still create an object type and an object.set type to allow proper updating and removing of values to try to stay consistent. Or define a way to add / update & remove properties.

About filtering how can it be handled ? I see a simple usage of get all movies with a trakt id to sync watched statuses.

[phate89]

I still don't understand (except for filtering that I don't know) where exactly is the problem with a very simple structure like { "imdb":"tt99999", "tvdb":"99999"}
Why should be so difficult to be consistent updating this? Almost every language has a type of data that matches this structure exactly that can be mirrored.. Is also simpler to parse/match because you don't have subfields to check.
If we want to keep it similar to the rest is ok but there aren't similar fields.. art and cast have more than 2 options..

@Montellese when you have some time (no hurry since the current window is closed) can you do a review?
@tamland I added uniqueid field to python. can you check if it's ok? (it's the first time I change python api). There's some bump to do?

[Tolriq]

@phate89 there's some misunderstanding here ;)

Art and your proposal are the exact same things, art just have some default fields set up and IMO your solution should have imdb by default since we have a default field imdbnumber. (And surely all the others you handle by default in the upgrade process)

Anyway arts defines "Media.Artwork" and "Media.Artwork.Set" and as you can see they have a big difference, set type allow null value for a field to delete it.

Since your define says string min len 1 you can't use null to delete an entry and your code does not support it too.

https://github.com/xbmc/xbmc/pull/10000/files#diff-bc14ddc0bd64f94097c9f8c3cf871345R1152

Does not provide a proper way to remove / update / add entries. Look at how art are updated.
And reading this I'm really not sure updating only imdbnumber does not affect all others (Well it seems updating just imdb does not update DB but unsure as no time to fully read everything)

Same you take the uniqueIds passed as argument and set that as new value, dropping every other value. (including imdbnumber is the app update the 2 at the same time)

Artwork works the correct way, you can pass just one artwork and this one will be added / updated without breaking all the others.
And you can delete an artwork by passing just it's name and null value.

Current proposal does not allow proper handling of this data as it requires any consumer to first get the previous data, play with all then send the full value, not how other things works and not error prone at all.

And last thing about your upgrade process, the fact that current scraper on a folder is X does not sounds enough to be sure about the current value of a field, scraper can have been changed or user can have used nfos that will may prevail for that field.

[zag2me]

Excellent new feature! I'd love to see this with music database as well in the future! Having ID's for different sites would be really useful.

[phate89]

Art has that unique method but all the string lists are handled in this way. They replace the entire content with what you provide. So you can delete / add / change what you want..Just like you doit for genres, directors etc etc etc.. If that's not " a proper way to remove / update / add entries" half of the fields are the same..
Imho is actually worse to have all the fields that replace the value except 2 or 3...

your solution should have imdb by default since we have a default field imdbnumber

we had as default field imdb. After this change scraper will set as default whatever they want.
Right now for example themoviesdb uses imdb for scraping and doesn't make sense. If you provide an id you should also provide a type name.. The old method is there only for backward compatibility

scraper can have been changed or user can have used nfos that will may prevail for that field.

Of course users could have changed it with nfo or json but that's a very corner case. The alternative is to identify only imdb tags and everything else is "unknown". it's 99.9% accurate

[Tolriq]

A string list is not a list of pairs.

You can not compare apples and oranges.

Obviously when you change the genres of something you change all the genres as genre is a single entity.

Uniques Ids are exactly like art, they are a collection of different data related to different things. With the only common thing being to be unique ids.
Art is a collection of different data related to different things. With the only common thing being to be art.

An addon that manage discart should not have to worry about another addon that deals with fanarts.
And this is done that way currently for that reason. This is logical and it was not made at random to be different than the rest.

It's different because it's a completely different use case. Art is not a single entity it's a collection of entities managed by different process / addons / entities / whatever.

An addon / software that wants to deal with it's own pair should not have to be worried about the others.

And since art usage = uniqueid usage, then they should behave the same. Handling uniqueid as a single entity is wrong considering the goal and definition of uniqueids.

About default / history IMDB :

1. Your current code does not update database when updating the field for a movie.
2. If it was, it would drop any other values set by any other scrapers / tools / API consumer.

So either remove the update function, and then the get as in all case there's already a few incompatible changes. Or consider it's another argument for the problem of current implementation.

About migration :
IMO something that can have mistakes that can't be easily fixed by user from a menu or existing tools should take the 100% sure path.

EDIT : And BTW genre / directors / ... are all arrays ;)

[phate89]

It's different because you decided to?
It's still lists. Doesn't matter if they're pair or string.
You want to add a genre? You get the genres and you add the genre you want.
you want to remove a genre? you get the genres and remove the one you want to remove.
You want to add a uniqueid? You get the uniqueids and you add the genre you want.
you want to remove a uniqueid? you get the uniqueids and remove the one you want to remove.
Same thing...

It's different because it's a completely different use case. Art is not a single entity it's a collection of entities managed by different process / addons / entities / whatever.
An addon / software that wants to deal with it's own pair should not have to be worried about the others.

Genre is a collection of entities.. genres.. They're composed of a single property, the name but is still a list..
Exactly why do you think only Art shouldn't deal with the others? Also genres would be probably nice to be able to add one without dealing with the rest. the same for actors, tags etc etc

You can't say it's a nice api where you have all the fields in a specific function that simply put what you set in the fields except with a field that behave in a completely different way

About default / history IMDB :

1. Your current code does not update database when updating the field for a movie.
2. If it was, it would drop any other values set by any other scrapers / tools / API consumer.

No. It replace everything but it checks that the default one is still there. If there isn't it's added back. I'm not sure if I also have to force the old value

[Tolriq]

Ok I give up, you have made your mind, and think different things are equals and equals things are different. If it's not different then why art was made like that ? Are uniqueids more like genre or like art ? Answer is quite easy to find.

If you do not see the difference between an array of strings (array = 1 object) and an object that contains a list of unrelated objects. Or the fact that all genres are genre but that an imdbid is not a tmdbid I can't do anything about that.

I'm just giving facts, genre / directors are an array of same object, art is a collection of unrelated things, a discart have nothing in common with a fanart. Same for uniqueid a tmdb have nothing in common with an anidb id. An addon could even store a full url as a value for an unique id.

And as I said genre / ... are all arrays ;) So preaching for my first proposal if you say it's like genre.

No arguments will change your mind I get it, personally I do not care as I will never write those fields, but at least there's a written arguments when problems will arise.

And your proposal does not fit anything currently present in Kodi. It's neither an array that overwrite or an object that properly update it's part.

@Montellese will have the last word let's hope he have time to read all.

[phate89]

but at least there's a written arguments when problems will arise.

you wrote hundreds of lines and you actually never bring an actual problem.
Instead of writing hundreds of lines of how different are art and uniqueid than the rest bring a simple example where the procedure everyone uses for (let's say) genres doesn't work for uniqueid or create problems.
You can't say "I told you so" because you said "there will be problems".
You point me to a concrete problem and I'll IMMEDIATELY switch it to your option (and then I will also have to change "ratings" because it works exactly the same)

Of course if someonelse agree with you especially montellese I'll switch immediately too..

[Tolriq]

The problem is consistency and logic and usability.

Genre is an array of string, from API point of view setting an array value set all the values.
And there's no API to delete a single entry because there's no real need for it.

uniqueId is an object, setting a property of an object should not erase all other properties.

From an SQL point of view, settings a field to X update that field and not the others. uniqueId is not a field it's a collection of fields setting one of those field should not erase the others.

Current implementation will generate for sure problems where API consumer will delete other's data.

It's not logical that someone that want to manage it's own data have to take care of other's data and users will not take care and delete the other's data.

Art are handled this way because of that. It would be insane to have anyone deleting others random data.

Implementation requires getting the values, check all pairs key update then resend the whole data. If the consumer have encoding problem then all data is corrupted. And users will never understand why an app A broke app B and will blame either Kodi or app B, not app A.

When a user use a tool that update a movie genre it expect the genre to be to the new value, if he makes a mistakes he understand.

A user try a trakt addon to sync it's watched status, then he loose all the other data, he will never understand the problem.

[Montellese]

@phate89: The way artwork works has been discussed a lot and it was the best approach that we could come up with because certain add-ons only care about certain artwork types and they don't want to look and or mess with any of the other artwork types. So the API is done in a way that such an add-on can simply provide a single type of artwork and not care about the current state of the rest of the artwork at all.
IMO the same logic does not apply to genres. It doesn't make sense to have an add-on that only cares about a single type of genre (e.g. "Action"). A movie has multiple genres assigned and they are somehow connected.

IMO for uniqueids the artwork approach applies because e.g. the trakt add-on shouldn't have to care at all about any other uniqueid. It should simply be able to add and/or remove the "trakt" unique id to any video without having to know if the video also has an imdbid or a tmdbid assigned.

The discussion is completely independent of whether the type in JSON is a list of strings or an object or a list of pairs of whatever. It's solely about what the type represents and about who is interested in doing what with that type.

[phate89]

@Montellese I updated the pr with something similar to artwork.
Do I have to do it also for ratings? Right now they are replaced... but they are exactly like uniqueids
I could change that to something like:

ratings:
{  
   "imdb":
   {  
      "value":7,
      "votes":45,
      "default":false
   },
   "tvdb": null
}

[phate89]

Actually if I'm not wrong for rescraping/refreshing it does a new search from name.. it's used for searching art (but we can request it there). The only difference is for skinning labels /python listitem. In that cases it won't always be available (only if we request full details)

[MartijnKaijser]

Anything left to get this in? :)

[phate89]

I'm checking if I can remove uniqueid from views...

@Montellese I checked and refreshing and getting art works even without uniqueid in the view..The only difference is that now you can't always get imdbnumber via GetProperty (before was in the view so it was always available)..
I'm not sure about uPNP..Can be dropped there the identifier or is it used? I can't simply ask more details in that case because it's retrieved via smartplaylist so it has the default details..
I created a second commit where uniqueid is not in the views..When you find some spare time (with no hurry) can you check if it's okay? I'm sorry to always bother you but I don't think there is anyone that knows this kodi part like you..

[phate89]

Since I'm not 100% sure we can drop the uniqueid because our upnp stuff might need I'll start to go with the reviewed part.. We can do the rest later..
jenkins build this please

[Montellese]

IIRC UPnP only retrieves the uniqueid to be able to pass it to the UPnP client. But this information is only interpretable by a Kodi UPnP client as it's not part of the standard.

[phate89]

@Montellese but it isn't used in your work for upnp importing or to provide kodi clients a way to scrape the info?
jenkins build this please

[Montellese]

@phate89: Well in one of my attempts to provide handling for multiple identical items from different sources (files, upnp, ...) I'm using the IMDB number to try to match items more reliably but that is heavily outdated. I might also be using it in my UPnP import work to find and match tvshows coming from UPnP with tvshows in the local library but I'm not 100% sure. But don't let this be held up by work that isn't ready and that I didn't have time to work on for months.

[phate89]

@Montellese I also split to get in at least the feature before the windows closes.. I can always pr after the second part..
jenkins build this please

[phate89]

jenkins build this please

[phate89]

Jenkins build this please