Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download missing album art from Lastfm #319

Merged
merged 5 commits into from
Nov 11, 2017

Conversation

rivaldi8
Copy link
Contributor

If an album's art isn't found in the disk or something fails while loading it, this adds downloading it from Lastfm as a fallback. It does so only when listing the albums of an artist (ArtistMusicFragment) and when showing the album (AlbumDetailFragment).

There are some more places where album art is displayed and could benefit from this change. As they are not as important, I've left them out to keep the pull request small. If this gets merged, I can work on them on another pull request. I would also like to implement downloading the images directly into the folder of the album instead of into the cache of the application.

@naman14
Copy link
Owner

naman14 commented Nov 3, 2017

This would need to be accompanied by a preference in settings similar to 'Load artist images' to disable this behaviour.

@rivaldi8
Copy link
Contributor Author

rivaldi8 commented Nov 3, 2017

Ok, I'll work on it.

@rivaldi8
Copy link
Contributor Author

Sorry, I misunderstood you about the "Load artist images" preference. I thought you meant it as a new preference, but I've found it already exists. I guess you meant to create another preference for album images instead.

In my opinion, if a user wants to load artist images, he or she will want album covers too. Or at least they won't care if they are downloaded too. Otherwise, the user would have to check/uncheck two options instead of one, which is bad from a usability point of view.

Currently this preference is already affecting album images. The ImageLoader is initialized here and this same instance used in rest of the application. Then, from the point of view of the code, it would be simpler to leave it as is. Just change the preference string to "Load artist/album images". Otherwise we would have to check somehow whether we are loading an artist or an album image.

What do you think?

@naman14
Copy link
Owner

naman14 commented Nov 10, 2017

@rivaldi8 I forgot that the ImageLoaderConfiguration is already using the load image preference.

I think we can change the preference string to "load artist/album images" and that should be fine. But i think we should add another preference "Always load album images from Last.fm". Summary for that can be "If unchecked, album art will be fetched locally from song file if available". And if its checked, then we should ignore the local album art and always fetch from lastfm

Though, its just my opinion, if you have better idea than feel free to implement that way

The preference to control downloading of artist images is also also used for album images.
Ignores images from disk when checked.
@rivaldi8
Copy link
Contributor Author

Done!

@naman14
Copy link
Owner

naman14 commented Nov 11, 2017

@rivaldi8 Thanks!

@naman14 naman14 merged commit 901c35c into naman14:master Nov 11, 2017
@rivaldi8 rivaldi8 deleted the download-album-covers branch November 11, 2017 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants