GNOME Bugzilla – Bug 770806
fixes for 0.3.3 release
Last modified: 2016-10-21 21:04:28 UTC
Only one warning left here, for grl-opensubtitles [0] but can be fixed later on anyway. [0] grl-opensubtitles.c:221:3: warning: ‘soup_xmlrpc_request_new’ is deprecated: Use 'soup_xmlrpc_message_new' instead [-Wdeprecated-declarations]
Created attachment 334719 [details] [review] youtube: gdata_youtube_video_get_video_id is deprecated Not sure if this should work in versions prior 0.17.0 so #ifdef to avoid regressions. grl-youtube.c:507:5: warning: ‘gdata_youtube_video_get_video_id’ is deprecated: Use gdata_entry_get_id instead [-Wdeprecated-declarations] grl_media_set_id (media, gdata_youtube_video_get_video_id (video)); ^~~~~~~~~~~~~~~~
Created attachment 334720 [details] [review] youtube: gdata_youtube_video_look_up_content is deprecated It always return NULL as this is no longer supported by Google. grl-youtube.c: In function ‘build_media_from_entry’: grl-youtube.c:571:2: warning: ‘gdata_youtube_video_look_up_content’ is deprecated: Use gdata_youtube_video_get_player_uri instead [-Wdeprecated-declarations] gdata_youtube_video_look_up_content (video, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Created attachment 334721 [details] [review] youtube: fix a few compiler warnings grl-youtube.c:591:10: warning: unused variable ‘id’ [-Wunused-variable] gchar *id; ^~ grl-youtube.c:745:47: warning: passing argument 1 of gdata_youtube_service_get_categories_async’ from incompatible pointer gdata_youtube_service_get_categories_async (service, NULL, ^~~~~~~ grl-youtube.c:31: expected ‘GDataYouTubeService * but argument is of type ‘GDataService * void gdata_youtube_service_get_categories_async (GDataYouTubeService ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ grl-youtube.c: In function ‘produce_from_feed’: grl-youtube.c:1076:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘produce_from_category’: grl-youtube.c:1128:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘grl_youtube_source_search’: grl-youtube.c:1317:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (ss->text); ^
Created attachment 334722 [details] [review] tmdb: use GTask instead of GSimpleAsyncResult
Created attachment 334723 [details] [review] metrolyrics: fix html parser Seems that in some lyrics a new <div></div> can be included with some info. That was breaking the html parser. Instead on relying on ending </div> for the lyric, let's use something else that is present and not so common (<p class="writers") as ending point for the lyrics and remove what is not interesting for us. Leaving a g_debug on lyrics test to pinpoint faster what is the issue between the data we have stored and the lyrics from webservice.
Created attachment 334724 [details] [review] tests: fix url for mocked content in thegamesdb (test_games:5048): Grilo-WARNING **: [lua-library] grl-lua-library.c:504: Can't fetch element 1 (URL: http://thegamesdb.net/api/GetGamesList.php?name=Ast%C3%A9rix&platform=PC): 'Could not find mock content Key file does not have group 'http://thegamesdb.net/api/GetGamesList.php?name=Ast%C3%A9rix&platform=PC''
Review of attachment 334719 [details] [review]: Given that gdata_youtube_video_get_video_id() just calls gdata_entry_get_id(), and that since the YouTube v3 port (which we require to speak to upstream YouTube), no need for the #ifdef's
Review of attachment 334719 [details] [review]: Also note that the commit subject should be something like: "Replace deprecated ..." otherwise there's no way to know whether it's a replacement, a removal, a change of behaviour, from the shortlog.
Review of attachment 334720 [details] [review]: Ditto about the commit message, looks fine otherwise.
Review of attachment 334721 [details] [review]: Rest looks fine, but again with the subject ("Fix assignment compiler warnings"). ::: src/youtube/grl-youtube.c @@ -589,3 @@ GList *all = NULL, *iter; CategoryInfo *cat_info; - gchar *id; Separate patch for this ("Remove unused variable").
Review of attachment 334723 [details] [review]: Would be great to add the lyrics in question to the test suite to make sure we don't regress in the future. ::: tests/lua-factory/sources/test_lua_metrolyrics.c @@ +108,2 @@ if (g_ascii_strncasecmp (lyrics, data, size - 1) != 0) { + g_debug ("--- Lyrics stored in test -----\n%s", data); Was that needed/wanted? Should probably be in a separate patch.
Review of attachment 334724 [details] [review]: Does the test pass after that? If so, please push.
Review of attachment 334722 [details] [review]: If the test suite still passes, and is valgrind-clean, looks fine to me, but not as a "fix" for 0.3.3.
(In reply to Bastien Nocera from comment #11) > Review of attachment 334723 [details] [review] [review]: > > Would be great to add the lyrics in question to the test suite to make sure > we don't regress in the future. I catch this with the lyrics we already have in the test suite. > > ::: tests/lua-factory/sources/test_lua_metrolyrics.c > @@ +108,2 @@ > if (g_ascii_strncasecmp (lyrics, data, size - 1) != 0) { > + g_debug ("--- Lyrics stored in test -----\n%s", data); > > Was that needed/wanted? > > Should probably be in a separate patch. It was but I'll remove, we don't really need it.
Comment on attachment 334724 [details] [review] tests: fix url for mocked content in thegamesdb Attachment 334724 [details] pushed as a93945b - tests: fix url for mocked content in thegamesdb
Created attachment 334803 [details] [review] youtube: replace deprecated gdata_youtube_video_get_video_id grl-youtube.c:507:5: warning: ‘gdata_youtube_video_get_video_id’ is deprecated: Use gdata_entry_get_id instead [-Wdeprecated-declarations] grl_media_set_id (media, gdata_youtube_video_get_video_id (video)); ^~~~~~~~~~~~~~~~
Created attachment 334804 [details] [review] youtube: replace deprecated gdata_youtube_video_look_up_content It always return NULL as this is no longer supported by Google. grl-youtube.c: In function ‘build_media_from_entry’: grl-youtube.c:571:2: warning: ‘gdata_youtube_video_look_up_content’ is deprecated: Use gdata_youtube_video_get_player_uri instead [-Wdeprecated-declarations] gdata_youtube_video_look_up_content (video, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Created attachment 334805 [details] [review] youtube: fix a few compiler warnings grl-youtube.c:745:47: warning: passing argument 1 of gdata_youtube_service_get_categories_async’ from incompatible pointer gdata_youtube_service_get_categories_async (service, NULL, ^~~~~~~ grl-youtube.c:31: expected ‘GDataYouTubeService * but argument is of type ‘GDataService * void gdata_youtube_service_get_categories_async (GDataYouTubeService ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ grl-youtube.c: In function ‘produce_from_feed’: grl-youtube.c:1076:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘produce_from_category’: grl-youtube.c:1128:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘grl_youtube_source_search’: grl-youtube.c:1317:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (ss->text); ^
Created attachment 334806 [details] [review] youtube: fix unused variable grl-youtube.c:591:10: warning: unused variable ‘id’ [-Wunused-variable] gchar *id; ^~
Created attachment 334807 [details] [review] metrolyrics: fix html parser Seems that in some lyrics a new <div></div> can be included with some info. That was breaking the html parser. Instead on relying on ending </div> for the lyric, let's use something else that is present and not so common (<p class="writers") as ending point for the lyrics and remove what is not interesting for us.
Review of attachment 334719 [details] [review]: obsolete
Review of attachment 334720 [details] [review]: obsolete
Review of attachment 334803 [details] [review]: Sure.
Review of attachment 334804 [details] [review]: Yes.
Review of attachment 334805 [details] [review]: Subject line again, rest looks good.
Review of attachment 334806 [details] [review]: "Remove unused variable" "fixing" an unused variable could mean to start using it.
(In reply to Victor Toso from comment #14) > (In reply to Bastien Nocera from comment #11) > > Review of attachment 334723 [details] [review] [review] [review]: > > > > Would be great to add the lyrics in question to the test suite to make sure > > we don't regress in the future. > > I catch this with the lyrics we already have in the test suite. So the output was already wrong in the test suite. Something has to change, no? Either the input (the website changed, we need new test data files), or the output (the new output is cleaned up now).
(In reply to Bastien Nocera from comment #27) > (In reply to Victor Toso from comment #14) > > (In reply to Bastien Nocera from comment #11) > > > Review of attachment 334723 [details] [review] [review] [review] [review]: > > > > > > Would be great to add the lyrics in question to the test suite to make sure > > > we don't regress in the future. > > > > I catch this with the lyrics we already have in the test suite. > > So the output was already wrong in the test suite. Something has to change, > no? Either the input (the website changed, we need new test data files), or > the output (the new output is cleaned up now). The website changed but we don't have the html stored locally. grl-metrolyrics unit test does fetch the website every time and check if the result is the same as before.
(In reply to Bastien Nocera from comment #26) > Review of attachment 334806 [details] [review] [review]: > > "Remove unused variable" > > "fixing" an unused variable could mean to start using it. Okay :) Thanks again!
Attachment 334803 [details] pushed as 8d91159 - youtube: replace deprecated gdata_youtube_video_get_video_id Attachment 334804 [details] pushed as e425483 - youtube: replace deprecated gdata_youtube_video_look_up_content
Created attachment 334808 [details] [review] youtube: fix assignment compiler warnings grl-youtube.c:745:47: warning: passing argument 1 of gdata_youtube_service_get_categories_async’ from incompatible pointer gdata_youtube_service_get_categories_async (service, NULL, ^~~~~~~ grl-youtube.c:31: expected ‘GDataYouTubeService * but argument is of type ‘GDataService * void gdata_youtube_service_get_categories_async (GDataYouTubeService ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ grl-youtube.c: In function ‘produce_from_feed’: grl-youtube.c:1076:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘produce_from_category’: grl-youtube.c:1128:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (NULL); ^ grl-youtube.c: In function ‘grl_youtube_source_search’: grl-youtube.c:1317:9: warning: assignment from incompatible pointer query = gdata_youtube_query_new (ss->text); ^
Created attachment 334809 [details] [review] youtube: remove unused variable grl-youtube.c:591:10: warning: unused variable ‘id’ [-Wunused-variable] gchar *id; ^~
(In reply to Victor Toso from comment #28) > (In reply to Bastien Nocera from comment #27) > > (In reply to Victor Toso from comment #14) > > > (In reply to Bastien Nocera from comment #11) > > > > Review of attachment 334723 [details] [review] [review] [review] [review] [review]: > > > > > > > > Would be great to add the lyrics in question to the test suite to make sure > > > > we don't regress in the future. > > > > > > I catch this with the lyrics we already have in the test suite. > > > > So the output was already wrong in the test suite. Something has to change, > > no? Either the input (the website changed, we need new test data files), or > > the output (the new output is cleaned up now). > > The website changed but we don't have the html stored locally. > grl-metrolyrics unit test does fetch the website every time and check if the > result is the same as before. Ha, ok, will review now.
Review of attachment 334807 [details] [review]: Looks good.
Review of attachment 334808 [details] [review]: Yep.
Review of attachment 334809 [details] [review]: Sure.
I'll let the bug open for the tmdb patch which should be commited after freeze. Attachment 334807 [details] pushed as 1596afd - metrolyrics: fix html parser Attachment 334808 [details] pushed as d519b20 - youtube: fix assignment compiler warnings Attachment 334809 [details] pushed as b63fa21 - youtube: remove unused variable
Review of attachment 334805 [details] [review]: obsolete
Review of attachment 334806 [details] [review]: obsolete
Attachment 334722 [details] pushed as fb45b33 - tmdb: use GTask instead of GSimpleAsyncResult