Make lastfmcovers use pylast 0.4

Bug #506060 reported by Ubuntuxer
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Exaile
Invalid
Undecided
Unassigned

Bug Description

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().

Revision history for this message
Ubuntuxer (johannes-schw) wrote :
Revision history for this message
reacocard (reacocard) wrote :

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.

Changed in exaile:
milestone: none → 0.3.2
Revision history for this message
Ubuntuxer (johannes-schw) wrote :

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.

Revision history for this message
reacocard (reacocard) wrote :

>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.

Revision history for this message
Ubuntuxer (johannes-schw) wrote :

>>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.

Revision history for this message
reacocard (reacocard) wrote :

> 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.

Revision history for this message
Ubuntuxer (johannes-schw) wrote :

>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.

Changed in exaile:
status: New → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.