GNOME Bugzilla – Bug 740871
opensubtitles: Add plugin
Last modified: 2014-12-08 22:50:06 UTC
.
Created attachment 291742 [details] [review] opensubtitles: Add plugin Add a plugin that will get a list of available subtitles available for a video, given its gibest hash.
Created attachment 291751 [details] [review] opensubtitles: Add plugin Add a plugin that will get a list of available subtitles available for a video, given its gibest hash.
Created attachment 291833 [details] [review] opensubtitles: Add plugin Add a plugin that will get a list of available subtitles available for a video, given its gibest hash.
Review of attachment 291833 [details] [review]: Some comments. ::: src/Makefile.am @@ +118,3 @@ apple-trailers bliptv bookmarks dleyna dmap filesystem flickr freebox gravatar jamendo \ lastfm-albumart local-metadata lua-factory magnatune metadata-store optical-media \ + pocket podcasts raitv shoutcast thetvdb tmdb tracker vimeo youtube opensubtitles Could you sort it? ::: src/opensubtitles/Makefile.am @@ +4,3 @@ +# Author: Guillaume Emont <gemont@igalia.com> +# +# Copyright (C) 2010-2011 Igalia S.L. All rights reserved. Fix header. You should be the author. ::: src/opensubtitles/grl-opensubtitles.c @@ +445,3 @@ + parse_results (rs->media, msg); + + rs->callback (rs->source, rs->operation_id, rs->media, rs->user_data, NULL); In case of errors, shouldn't we send a GError? @@ +493,3 @@ + while ((rs = g_async_queue_try_pop (priv->queue))) { + if (failed) { + rs->callback (rs->source, rs->operation_id, rs->media, rs->user_data, NULL); If failed, shouldn't we send a GError? @@ +512,3 @@ + static GList *keys = NULL; + if (!keys) { + keys = grl_metadata_key_list_new (GRL_OPENSUBTITLES_METADATA_KEY_SUBTITLES_URL, Shouldn't add also GRL_OPENSUBTITLES_METADATA_KEY_SUBTITLES_LANG? @@ +519,3 @@ + +static void +ensure_hash_keyid (GrlOpenSubtitlesSourcePriv *priv) This is not part of the API. Should be moved to above "utilities" section @@ +551,3 @@ + if (!grl_data_has_key (GRL_DATA (media), priv->hash_keyid)) + return FALSE; + If it doesn't contain key, but missing_keys is not NULL, we need to add that key in missing_keys, to inform that we can't resolve due missing that key. @@ +598,3 @@ + msg = new_search_message (priv->token, + grl_data_get_string (GRL_DATA (rs->media), priv->hash_keyid), + grl_media_get_size (rs->media)); If we require size to compute get the subtitle, we should add that key in the list of required keys, in may_resolve.
(In reply to comment #4) > Review of attachment 291833 [details] [review]: > > Some comments. > > ::: src/Makefile.am > @@ +118,3 @@ > apple-trailers bliptv bookmarks dleyna dmap filesystem flickr freebox > gravatar jamendo \ > lastfm-albumart local-metadata lua-factory magnatune metadata-store > optical-media \ > + pocket podcasts raitv shoutcast thetvdb tmdb tracker vimeo youtube > opensubtitles > > Could you sort it? Done. > ::: src/opensubtitles/Makefile.am > @@ +4,3 @@ > +# Author: Guillaume Emont <gemont@igalia.com> > +# > +# Copyright (C) 2010-2011 Igalia S.L. All rights reserved. > > Fix header. You should be the author. I just removed it. I haven't seen anyone use copyrights for Makefiles in any project I've been working on. > ::: src/opensubtitles/grl-opensubtitles.c > @@ +445,3 @@ > + parse_results (rs->media, msg); > + > + rs->callback (rs->source, rs->operation_id, rs->media, rs->user_data, NULL); > > In case of errors, shouldn't we send a GError? Sure, done. > @@ +493,3 @@ > + while ((rs = g_async_queue_try_pop (priv->queue))) { > + if (failed) { > + rs->callback (rs->source, rs->operation_id, rs->media, rs->user_data, > NULL); > > If failed, shouldn't we send a GError? Fixed. > @@ +512,3 @@ > + static GList *keys = NULL; > + if (!keys) { > + keys = grl_metadata_key_list_new > (GRL_OPENSUBTITLES_METADATA_KEY_SUBTITLES_URL, > > Shouldn't add also GRL_OPENSUBTITLES_METADATA_KEY_SUBTITLES_LANG? Done. > @@ +519,3 @@ > + > +static void > +ensure_hash_keyid (GrlOpenSubtitlesSourcePriv *priv) > > This is not part of the API. Should be moved to above "utilities" section Merging the register_keys patches would fix that problem... > @@ +551,3 @@ > + if (!grl_data_has_key (GRL_DATA (media), priv->hash_keyid)) > + return FALSE; > + > > If it doesn't contain key, but missing_keys is not NULL, we need to add that > key in missing_keys, to inform that we can't resolve due missing that key. Done. > @@ +598,3 @@ > + msg = new_search_message (priv->token, > + grl_data_get_string (GRL_DATA (rs->media), > priv->hash_keyid), > + grl_media_get_size (rs->media)); > > If we require size to compute get the subtitle, we should add that key in the > list of required keys, in may_resolve. Done.
Created attachment 292208 [details] [review] opensubtitles: Add plugin Add a plugin that will get a list of available subtitles available for a video, given its gibest hash.
Review of attachment 292208 [details] [review]: Nice!
Attachment 292208 [details] pushed as 5a96850 - opensubtitles: Add plugin