Make lastfmcovers use pylast 0.4

[Ubuntuxer]

I rewrote lastfmcovers, so it uses pylast 0.4 to fetch the cover. I inserted the lib pylast 0.4 into the directory xl, so that in the future other plugins can use this lib, too. (e.g. contextinfo)
Besides I added in the class Track of pylast 0.4 a function named get_cover_image().

[reacocard (reacocard)]

Please don't embed libs into core that will only be used by plugins. Libs used only by plugins must either be external deps or embedded into the plugin itself. Modifying upstream libs, even when embedded, is also discouraged.

Also, I'm not sure you understand how find_covers is supposed to work. find_covers returns a list of strings you can pass to get_cover_data to retrieve the actual data, it does NOT itself return that data, as your code appears to.

[Ubuntuxer (johannes-schw)]

Can you tell me where I can insert the lib?
>Modifying upstream libs, even when embedded, is also discouraged.
I have noticed that with "track.getInfo" you get the best result in the cover search.
It's not a big change in the code, because I just copy the function from the class Album. Should I make an request by the upstream project?
I would like to adapt also lastfmdynamic to pylast 0.4, but I noticed the return value of the method similiar_artist is strange formatted, so actually I want to change the lib again.

>Also, I'm not sure you understand how find_covers is supposed to work. find_covers returns a list of strings you can pass to >get_cover_data to retrieve the actual data, it does NOT itself return that data, as your code appears to.
I don't understand, what you mean.
The method "track.get_cover_image()" returns the url as string; I use square bracket to return a list of strings
I tested my patch and it works fine for me.

[reacocard (reacocard)]

>Can you tell me where I can insert the lib?
Only in the plugin itself, as the contextinfo plugin currently does.

> Should I make an request by the upstream project?
yes, getting your changes merged upstream when possible is best. We try not to modify libs because that makes it easier to keep our copy in sync with upstream.

> The method "track.get_cover_image()" returns the url as string;
Then you should name it something else that doesn't suggest that it's data. get_cover_url, perhaps. its implicit that a cover is an image, so get_cover_image implies that you're getting the image itself, not a url.

[Ubuntuxer (johannes-schw)]

>>Can you tell me where I can insert the lib?
>Only in the plugin itself, as the contextinfo plugin currently does.
Does it make sense to use pylast for the small plugins, like lastfmcovers or lastfmdynamic, in this case?
The advantage of lib is that you load it once and reuse it then. In my opinion the powerfulness of the lib pylast bear no relation to the plugin.

[reacocard (reacocard)]

> The advantage of lib is that you load it once and reuse it then.
yes, but this is a limitation of our plugins system. deal with it. any embedded lib is going to have the same issue, hence why external libs are generally a good idea.

> Does it make sense to use pylast for the small plugins, like
> lastfmcovers or lastfmdynamic, in this case?
There has been some discussion that the proliferation of lastfm plugins could be merged into one plugin and then just given settings to toggle the components on and off, which would alleviate this issue somewhat.

[Ubuntuxer (johannes-schw)]

>yes, but this is a limitation of our plugins system. deal with it. any embedded lib is going to have the same issue, hence why >external libs are generally a good idea.
>There has been some discussion that the proliferation of lastfm plugins could be merged into one plugin and then just given >settings to toggle the components on and off, which would alleviate this issue somewhat.

Ok, I think you can close this bug report because in this case there isn't any advantage to use pylast0.4. I don't like the idea to merge the lastfm plugins into one plugin because in my opinion it isn't user friendly.