New feature: show artist images#1107
Conversation
… generic name to be used for both, artist and album artwork
# Conflicts: # package-lock.json # src/app/app.module.ts # src/app/services/indexing/indexing.service.spec.ts # src/app/services/indexing/indexing.service.ts # src/app/ui/components/manage-collection/manage-collection.component.html # src/assets/i18n/he.json
|
As mentioned above, the last.fm API sometimes doesn't provide a MusicBrainz-Id or it provides an invalid Id. I improved this by adding the MusicBrainz API as fallback. I don't like using so many different APIs but the MusicBrainz API returns not only one artist, but several findings that also include aliases. |
# Conflicts: # src/app/common/application/i18n.spec.ts # src/app/data/database-migrator.ts # src/app/services/indexing/indexing.service.spec.ts # src/app/services/indexing/indexing.service.ts # src/assets/i18n/pt-PT.json
|
@FranzDeschler Sorry for the late reply and thank you for this PR! The feature looks really nice. Please give me some time to review it. Don't desperate if it takes little while. Sometimes live throws me off track. I'll definitely come back to it. |
@digimezzo I'm glad to hear you like it :-) |
There was a problem hiding this comment.
Great work on this pull request, and thank you for taking the time to implement this feature while staying consistent with the existing architecture and logic.
I've left a few review comments. Could you please take a look at them and either address them or let me know if you have concerns, alternative suggestions, or reasons for keeping the current implementation?
I also noticed that recent changes on master have introduced some merge conflicts. Could you please merge the latest master and resolve those conflicts as well?
Thank you!
# Conflicts: # src/app/app.module.ts # src/app/data/database-migrator.ts # src/app/data/migrations/migration11.ts # src/app/services/indexing/indexing.service.spec.ts # src/app/services/indexing/indexing.service.ts # src/app/ui/components/collection/collection-artists/artist-browser/artist-browser.component.ts # src/assets/i18n/vi.json
# Conflicts: # src/assets/i18n/ja-JP.json
|
@FranzDeschler Many many thanks for this PR. You've delivered high quality work. I appreciate the care that you took in implementing this feature and in respecting the established logic. I finished reviewing your changes and I'll now merge them. |
|
Thanks! I´m happy to hear that you appreciate my work. I´m already working on additions to make that feature more complete. |
2 participants
Dopamine is great! But I´m missing artist images so I added this feature by myself and I hope you like it too.
Startup
During startup, when the user is asked whether to download album images, there is now the option to download artist images as well. This option is disabled by default so Dopamine stays the same until it´s enabled explicitly.
Artists & Albums settings
The option can also be enabled or disabled in the "Artists & Albums" section (previously just called "Albums" - but I thought this is the right place for this option).
Here you can also find the options to reload the images in the same way as the album images.
Artists section
When enabled, artist images are downloaded from the internet and displayed in the "Artists" section.
Known Issues
I´m using the existing APIs for loading artist metadata and images. I noticed, that in some cases no image can be found because the last.fm API doesn´t provide a MusicBrainz-Id or the artist is ambiguous. I will try to find a solution for that.
Technical details
Here is an overview over the technical changes I made:
ArtistArtworkthat holds artists and the corresponding artwork ID.Tracktable, there is an additional column calledArtistsKey. That key contains the individual artists of the track respecting theartistSplitSeparatorsandartistSplitExceptionssettings.showArtistImagesoption is enabled, the images of each individual artist is downloaded and stored in the cache. The cacheId is stored asArtworkIdin the database.ArtistArtworktable and the application retries to download the image the next time.ArtistArtworktable contains a default cache-Id. This defauld-Id simply indicates that there is no artist image and so, an empty image is shown. This solution avoids reloading images that just can´t be found.No AI - almost
I used AI to do the translations. Everything else is 100% natural intelligence ;-)
Database tests
I noticed that there are no database tests. And I also noticed why. The regular unit tests are executed in the node runtime but the sqlite database is compiled for the electron runtime. I solved this by splitting the tests into the regular tests and separate database-tests.
npm run test-database-integration) only execute the database tests. In this case, Jest is executed inside the electron runtime. This allows to use an in-memory sqlite database for the tests without the need of rebuilding sqlite.I know there are a lot of changes. So take your time reviewing and testing.
I gave my best to match your code style and technical approaches. I hope you appreciate my effort and I´m looking forward to see this feature in production one day :-)