GNOME Bugzilla – Bug 754275
Fix metrolyrics source and test
Last modified: 2015-08-29 16:55:07 UTC
Finally... not sure if we should include the queen change upstream, looks like they got the lyrics wrong. Maybe we could remove queen from the test.
Created attachment 310239 [details] [review] tests: match lyrics in the test with metrolyrics
Created attachment 310240 [details] [review] metrolyrics: fix source by removing noise Some changes in the metrolyrics website included more html noise in the lyrics.
Created attachment 310241 [details] [review] metrolyrics: Do not crash when parser fails (lt-grilo-test-ui-0.2:8463): Grilo-WARNING **: [lua-library] grl-lua-library.c:509: calling source callback function fail (fetch_page_cb) grl-metrolyrics.lua:99: attempt to index a nil value (local 'feed')'
Created attachment 310242 [details] [review] metrolyrics: Do not crash when parser fails (lt-grilo-test-ui-0.2:8463): Grilo-WARNING **: [lua-library] grl-lua-library.c:509: calling source callback function fail (fetch_page_cb) grl-metrolyrics.lua:99: attempt to index a nil value (local 'feed')'
Review of attachment 310239 [details] [review]: Sure.
Review of attachment 310240 [details] [review]: Yep.
Review of attachment 310242 [details] [review]: Sure, but I don't think that's enough. You also need to check *before* whether feed is nil, eg. you got a 404. This can be done in another patch.
Attachment 310239 [details] pushed as 722e3fd - tests: match lyrics in the test with metrolyrics Attachment 310240 [details] pushed as 3874725 - metrolyrics: fix source by removing noise Attachment 310242 [details] pushed as a692b9e - metrolyrics: Do not crash when parser fails
(In reply to Bastien Nocera from comment #7) > Review of attachment 310242 [details] [review] [review]: > > Sure, but I don't think that's enough. You also need to check *before* > whether feed is nil, eg. you got a 404. This can be done in another patch. That should be covered by fetch_page_cb(feed), no? <snip> function fetch_page_cb(feed) local media = nil if feed and not feed:find("notfound") then media = metrolyrics_get_lyrics(feed) end grl.callback(media, 0) end </snip>
(In reply to Victor Toso from comment #9) > (In reply to Bastien Nocera from comment #7) > > Review of attachment 310242 [details] [review] [review] [review]: > > > > Sure, but I don't think that's enough. You also need to check *before* > > whether feed is nil, eg. you got a 404. This can be done in another patch. > > That should be covered by fetch_page_cb(feed), no? > > <snip> > function fetch_page_cb(feed) > local media = nil > if feed and not feed:find("notfound") then > media = metrolyrics_get_lyrics(feed) > end > grl.callback(media, 0) > end > </snip> You're right, yes.