GNOME Bugzilla – Bug 679686
GRL_METADATA_KEY_CERTIFICATE doesn't consider regional ratings
Last modified: 2012-11-14 09:16:25 UTC
GRL_METADATA_KEY_CERTIFICATE seems under specified: Right now only the Apple Movie Trailer plugin is using that field and straightly copies the provide MPAA rating value to to. (e.g. "P", "PG", ...). To make that field universally useful it seems that country specific ratings must be considered. See http://help.themoviedb.org/kb/api/movie-release-info for an example: { "countries": [{ "certification": "PG", "iso_3166_1": "US", "release_date": "1977-05-25" }, { "certification": "U", "iso_3166_1": "GB", "release_date": "1977-12-27" }, { "certification": "12", "iso_3166_1": "DE", "release_date": "1978-02-02" }, { "certification": "U", "iso_3166_1": "FR", "release_date": "1977-10-19" }, { "certification": "", "iso_3166_1": "IT", "release_date": "1977-10-21" }, { "certification": "", "iso_3166_1": "PH", "release_date": "1977-08-23" }, { "certification": "", "iso_3166_1": "CZ", "release_date": "1991-07-25" }, { "certification": "12", "iso_3166_1": "HU", "release_date": "1979-08-16" }], "id": 11 } So how shall we support local movie ratings in that field? I see the following solutions: 1) Define a simple to parse text format that is stored in the field, e.g. "US:PG GB:U DE:12 FR:U HU:12", following the example above. 2) Deprecate GRL_METADATA_KEY_CERTIFICATE, and introduce a GHashTable based property, that uses the country code as key, and the country specific rating as value.
The problem with the hashtable is that at this moment we do not allow complex types as metadata. And it would require all sources to set up the country. An alternative solution that fits better in the current system is using multivalued elements: we keep GRL_METADATA_KEY_CERTIFICATE, that can have either one or more values, depending on the source. How to know the country for each certificate? Introducing a new key that stores the countries, GRL_METADATA_KEY_CERTIFICATE_REGION, for instance. And then making both keys related. Thus, for each value in the first key there will be a corresponding entry in the later. It allows also to handle by current sources, which would set only the first but not the former (you can interpret it as unknown / all regions). This is quite similar as the relation between URL and MIME: for each entry in URL there can be an entry in MIME. And for a specific URL, you can get the corresponding MIME.
Created attachment 226582 [details] [review] proposed patch
Created attachment 226634 [details] [review] tmdb patch #1
Created attachment 226635 [details] [review] tmdb patch #2
can i merge this?
Created attachment 226996 [details] [review] updated patch ...i've must have been tired...
Created attachment 227003 [details] [review] updated patch separate certificate and publication region merged into one related region key.
Created attachment 227006 [details] [review] update tmdb patch #1
Created attachment 227007 [details] [review] update tmdb patch #2
Created attachment 227008 [details] [review] updated patch made with git-format-patch instead of git-show
commit ee5513f6303412a0d3e7c4ccf2dcbd43132f8405 Author: Mathias Hasselmann <mathias@openismus.com> Date: Tue Oct 16 20:48:46 2012 +0200 Add region tags for publication and certification Both TMDB and IMDB publish region specific information for this metadata. This fixes https://bugzilla.gnome.org/show_bug.cgi?id=679686 bindings/vala/grilo-0.2-custom.vala | 2 ++ doc/grilo/grilo-sections.txt | 7 ++++++ src/data/grl-media.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- src/data/grl-media.h | 24 ++++++++++++++++++++ src/grl-metadata-key.c | 16 +++++++++++++- src/grl-metadata-key.h | 1 + 6 files changed, 202 insertions(+), 4 deletions(-)
commit fcfd3c496c1932d3ecd44d9bd81d070b151a3598 Author: Mathias Hasselmann <mathias@openismus.com> Date: Wed Oct 17 13:30:29 2012 +0200 tmdb: Remove "tmdb-age-certificates" key Use "certificate" and "certificate-region" instead. Related to https://bugzilla.gnome.org/show_bug.cgi?id=679686 src/tmdb/grl-tmdb.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------- 1 file changed, 50 insertions(+), 92 deletions(-) commit d916409114a8b90724ce154629903a583778aef0 Author: Mathias Hasselmann <mathias@openismus.com> Date: Wed Oct 17 13:29:46 2012 +0200 tmdb: Add grl_tmdb_request_get_list_with_filter() This is needed for region specific release dates. https://bugzilla.gnome.org/show_bug.cgi?id=679686 src/tmdb/grl-tmdb-request.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------- src/tmdb/grl-tmdb-request.h | 11 +++++++++-- 2 files changed, 100 insertions(+), 39 deletions(-)
I think this API addition/change in grilo really needs some better documentation. For instance, 1. What region's age certificate is now returned by grl_media_get_certificate(). Apparently its the 0th region (See grl_media_get_region_data_nth()), but that is arbitrary, so maybe this function should just be deprecated. 2. grl_media_get_region_data_nth(): How do I know how many there are. Don't we need a grl_media_get_region_data_count()? 3. Wouldn't it be more convenient to have: const gchar* grl_media_get_certificate_for_region(GrlMedia* media, const gchar* region) and gboolean grl_media_get_publication_date_for_region(GrlMedia* media, GDateTime **publication_date) ? I can provide a patch for all that, if you agree.
(In reply to comment #13) > 1. What region's age certificate is now returned by > grl_media_get_certificate(). Apparently its the 0th region (See > grl_media_get_region_data_nth()), but that is arbitrary, so maybe this function > should just be deprecated. Yes, the first element. And this is coherent with the other functions: for applications that are only interested in single-valued elements, they get the first element in case of having more than one values. That is why sources should put first the value that makes more sense for developers that are only interested in single values. > > 2. grl_media_get_region_data_nth(): How do I know how many there are. Don't we > need a grl_media_get_region_data_count()? grl_data_length (GRL_DATA (media), GRL_METADATA_KEY_REGION) > 3. Wouldn't it be more convenient to have: > const gchar* grl_media_get_certificate_for_region(GrlMedia* media, const gchar* > region) > and > gboolean grl_media_get_publication_date_for_region(GrlMedia* media, GDateTime > **publication_date) > ? > Yes, this would be a good addition. But I would use rather a different API: void grl_media_get_data_for_region (GrlMedia *media, const gchar *region, gchar **certificate, GDateTime **publication_date); So developer can specify which keys she need, and also getting a NULL value would mean the key has no value for that region
BTW, either if we add new API or not, the bug is already fixed.
(In reply to comment #14) > Yes, the first element. [snip] OK. Here's a patch to document the current interface then: https://bugzilla.gnome.org/show_bug.cgi?id=688301