GNOME Bugzilla – Bug 761694
[lastfm] order thumbnails by size
Last modified: 2016-02-22 22:09:35 UTC
Created attachment 320595 [details] [review] sort albumart urls large->small I noticed with the new lua lastfm plugin, gnome-music retrieved very small albumart and upscaled it. It looks like the old plugin ordered the retrieved albumart urls from large to small, this patch does the same.
Created attachment 320630 [details] [review] v2 noticed the debug statement could have a null url in the new setup and this would generate a warning, so moved it inside the check.
Review of attachment 320595 [details] [review]: This is obsolete to v2
Review of attachment 320630 [details] [review]: Patch does not have comment, we need that ;) ::: src/lua-factory/sources/grl-lastfm-cover.lua @@ +74,2 @@ function fetch_page_cb(result, userdata) + local url As this is used only inside the for scope, I would define it there @@ +82,2 @@ userdata.media.thumbnail = {} + image_sizes = { "mega", "extralarge", "large", "medium", "small" } consider putting this outside of this scope as LASTFM_THUMBNAIL_SIZES or include the local keyword: `local image_sizes = ...` @@ +82,3 @@ userdata.media.thumbnail = {} + image_sizes = { "mega", "extralarge", "large", "medium", "small" } + for c, u in pairs(image_sizes) do you are not using c so you can switch it to _ also, consider changing u to 'image_size' or just 'size' @@ +85,3 @@ + url = string.match(result, '<image size="' .. u .. '">(.-)</image>') + + if (url ~= nil and url ~= '') then I don't mind () but the other conditionals in the code are not using it so bet remove it; @@ +91,1 @@ end Could you please also add a patch to fix the case where userdata.media.thumbnail could be empty? IIRC we discussed this at IRC and it was happening... if #userdata.media.thumbnail == 0 then grl.callback() return end
Created attachment 321726 [details] [review] order by size v3 Changes as requested, as well as returning an empty result set (see bug #761852 -> where the supposed attachment was missing).
Review of attachment 321726 [details] [review]: Could you split in two patches, add commit log and also the minor fix needed? Thanks! ::: src/lua-factory/sources/grl-lastfm-cover.lua @@ +88,3 @@ + if url ~= nil and url ~= '' then + grl.debug ('Image size ' .. size .. ' = ' .. url) + if v ~= '' then There is no 'v' now @@ +98,3 @@ + else + userdata.callback(userdata.media, 0) + end Yes! But it should be in a different patch
Created attachment 321876 [details] [review] lastfm: order by size Order by size large->small patch.
Created attachment 321877 [details] [review] lastfm: return empty Lastfm: return an empty result if no covers are found.
I forgot to git-bz it Pushed both patches: https://git.gnome.org/browse/grilo-plugins/commit/?id=ae252b6580037da6493734a34ce3738b02ca14b9 https://git.gnome.org/browse/grilo-plugins/commit/?id=15023e0137e5ed99ecc262747db313ef2f8695a0 Many thanks!