Storing of TVDB Episode ID #1549

Merged
merged 2 commits into from Oct 10, 2012

Projects

None yet

5 participants

@mizaki
mizaki commented Oct 5, 2012

Following on from talking to myself http://forum.xbmc.org/showthread.php?tid=131803

Episodes can be in aired, DVD or absolute order. With this PR it's now easier to identify the episode. This helps with external sites like trakt but should also help with updating the information of an episode.

@MartijnKaijser
Member

@jmarshallnz or @cptspiff
Good for this merge window?

@jmarshallnz
Member

The scraper would need to be done in the addons repo which is likely more up to date.

I don't like the fixed nature of the I'd being tvdb. It would be nice, at least for json-rpc to have a more general id with the source of the id attached, so that the imdb id could be tied into the same system?

@Montellese your thoughts?

@mizaki
mizaki commented Oct 7, 2012

@jmarshallnz Just so I understand you correctly. New to people actually checking what I've done.

Remove the changes to tvdb.xml because, as you say it's an addon change?

Originally I used the name "episodeid" but as Montellese pointed out that is confusing with the table and JSONRPC id of the same name. So I followed the IMDB for movies, which although is called IMDB does not have to actually be the IMDB number. I believe any addons that use it check for "tt" at the beginning. The TVShowID is also saved with no indication of where it came from. Although it would be nice, it would be a big change imo.

Thanks for your time, I appreciate you are all busy right now.

@Montellese
Member

I agree that "tvdbepisodeid" is not an ideal name but certainly less confusing than "episodeid" but if we can come up with a more general name I'd be happier as well. I guess the name of "imdbid" is historical.

And yup the changes to the tvdb scraper should be done outside of a PR because we always merge the changes to the official addons back into git from the ones in the addon repo so your changes would be overwritten. You'll want to contact olympia about it once the PR has been accepted/merged.

@mizaki
mizaki commented Oct 7, 2012

After talking it over with Montellese he is happy with "episodeuniqueid". This was preferred over "episodeuid" because of its closeness to "episodeid". I changed the internals to match.

All good now?

@jmarshallnz
Member

Personally I'd prefer a pair (perhaps even a map later on?) with information as to where the uniqueid is from (in this case thetvdb). I could see in future that both an imdb id, themoviedb id and perhaps other ids such as rottentomatoes etc. could be useful for movies for example.

Just something to keep in mind for the future - doesn't necessarily have to be done now.

@mizaki
mizaki commented Oct 8, 2012

This just brings episodes in line with the rest. I agree that it would be helpful. Movies especially as you say, there are a number of resources. I kept my ambition low for my first foray into C++ and XBMC :)

@mizaki
mizaki commented Oct 8, 2012

Sorry, thought I had nuked the tvdb.xml. Have now. All good to go in?

@jmarshallnz
Member

@Montellese: Up to you if you want to take this as-is, or prefer some additional element to denote the source. It's not as if we'll be able to merge the imdbid number into this, so perhaps not worth the effort at this point?

@Montellese
Member

I'm not really into our scraper code. I can see a benefit in also storing the scraper but I wouldn't see it as a blocker. Maybe @vdrfan can offer his opinion as he knows the scraper code better.

@mkortstiege
Member

Yea, not really a blocker and i clearly see the benefit here. IMO Long term solution is, like jm mentioned, a proper id <-> source mapping.

@Montellese
Member

So we've discussed this topic a bit on IRC and we came to the following conclusion. The way this is stored in the database and in CVideoInfoTag is good but in the future we probably want a map of ID and source instead of just a single ID. So something like { "tvdb": "12345", "imdb": "tt12345" } which will provide much more info and make it more useful. This serialization into that specific format (be it JSON or XML or whatever) can later be added to the videoinfoscanner and stored as a string in the database and in CVideoInfoTag so nothing to change there. But in JSON-RPC we'd want to return this as a map so to prevent future backward-compatibility-breakage it would be great if we could already return this as a map/object.
What that means for now would be to change https://github.com/xbmc/xbmc/pull/1549/files#L1R636 to

"episodeuniqueid": { "type": "object", "additionalProperties": { "type": "string", "minLength": 1 } },

to describe that "episodeuniqueid" will return an object/map with arbitrary properties of type string. Furthermore the serialization in CVideoInfoTag (see https://github.com/xbmc/xbmc/pull/1549/files#L5R471) needs to be changed to

value["episodeuniqueid"]["unknown"] = m_strEpisodeUniqueId;

I think that should be it for now. We'll then look into applying this to the tvdb scraper after merging this in.
@jmarshallnz @vdrfan Anything I forgot?

@jmarshallnz
Member

I wonder if it should be more general than episodeuniqueid - i.e. just uniqueid or some such - that way later on imdb could be conscripted once it's hooked up for movies/shows etc?

@mizaki
mizaki commented Oct 9, 2012

I've changed as per the instructions (thanks for those). I've tested and it all seems to work fine. Is it ready now? :)

@jmarshallnz
Member

You missed the ["unknown"] bit in the serialisation to CVariant. i.e. you want to serialize into the "unknown" field of the "uniqueid" object.

@Montellese Montellese commented on an outdated diff Oct 9, 2012
xbmc/video/VideoInfoTag.cpp
@@ -464,6 +468,7 @@ void CVideoInfoTag::Serialize(CVariant& value) const
value["year"] = m_iYear;
value["season"] = m_iSeason;
value["episode"] = m_iEpisode;
+ value["uniqueid"] = m_strUniqueId;
@Montellese
Montellese Oct 9, 2012 Team Kodi member

As @jmarshallnz mentioned this should be

value["uniqueid"]["unknown"] = m_strUniqueId;

for now.

@Montellese Montellese commented on the diff Oct 9, 2012
xbmc/video/VideoInfoTag.cpp
@@ -120,6 +121,7 @@ bool CVideoInfoTag::Save(TiXmlNode *node, const CStdString &tag, bool savePathIn
{
XMLUtils::SetInt(movie, "season", m_iSeason);
XMLUtils::SetInt(movie, "episode", m_iEpisode);
+ XMLUtils::SetString(movie, "uniqueid", m_strUniqueId);
@Montellese
Montellese Oct 9, 2012 Team Kodi member

@jmarshallnz Do you think we should do something special here as well similar to the CVariant serialization?

@jmarshallnz
jmarshallnz Oct 9, 2012 Team Kodi member

Later on it would be something like id

@mizaki
mizaki commented Oct 9, 2012

Okay, 20th time lucky? :)

@jmarshallnz jmarshallnz was assigned Oct 9, 2012
@Montellese
Member

Yup looks good now. Can you provide the scraper changes (was there something else) in a pastebin or something so that olympia can take care of that?

@mizaki
mizaki commented Oct 9, 2012

This is what I did to test: http://pastebin.com/BYDxpj1h Olympia might have a better way. I don't think there is anything else.

Thanks for sticking with me everyone :)

@Montellese Montellese was assigned Oct 10, 2012
@Montellese Montellese merged commit 8756dfb into xbmc:master Oct 10, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment