Add artwork and logo support for additional sources - #329
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
|
Hi! Thanks so much for your interest and for taking the time to contribute a PR, we really appreciate it! 🎉 Would you mind rebasing your changes onto our 2.0.0 development branch? Also, if possible, please try to keep the PR focused on a single change or feature. From our review perspective, several smaller, focused PRs are much easier to review and merge than one large PR with multiple unrelated changes. Thanks again for your contribution, and we're looking forward to reviewing your PR! I will have a closer look once you have rebased onto the development branch 🙃 |
…p, Discogs, and Beatport; return 404 for missing artwork
…fallback consistently
…ogs, and Beatport
|
Hello! Thanks for the guidance - I’ve now rebased the PR onto the release/2.0.0 branch and updated the relevant compatibility issues and tests as needed. I’m relatively new to rebasing, so please let me know if anything still needs adjusting. I’ll be sure to keep future contributions focused on a single feature but hopefully the commits in this PR are self-contained enough to make the changes easier to review. Thanks again, I appreciate you taking a look 😇 |
semohr
left a comment
semohr
left a comment
There was a problem hiding this comment.
First round of reviews 🙃
|
|
||
| images = data.get("images", []) | ||
| return images[0].get("uri150") if images else None | ||
| except ( |
There was a problem hiding this comment.
All theses functions seem to have similar error handling can we move the error handling into the redirect_external_art function or abstract it?
| Returns the url the the art. | ||
| """ | ||
|
|
||
| get_config() |
There was a problem hiding this comment.
Why is the config needed here?
| if plugin.name != "beatport4": | ||
| continue | ||
|
|
||
| client = getattr(plugin, "client", None) |
There was a problem hiding this comment.
Try to omit getattr calls. I think an instance check could work instead of relying on name matching.
| }); | ||
| return dataUrl; | ||
| ); | ||
| if (!response.ok) { |
There was a problem hiding this comment.
We overwrite fetch in api/common.ts, We should not need to handle errors here this is done centrally.
| SiDiscogs, | ||
| SiMusicbrainz, | ||
| SiSpotify, | ||
| } from '@icons-pack/react-simple-icons'; |
There was a problem hiding this comment.
Im not too keen on adding another icon library. Any way we can omit this?
I would prefer we just add the brand icons as svgs.
There was a problem hiding this comment.
@llttlltt do you know how the simpleicon lib ships the icons?
the motivation of the comment is to keep client side payloads as small as possible.
if only those icons that are imported end up in the compiled site, im in favour of adding the lib.
| ...props | ||
| }: { src: string } & Partial<BoxProps>) { | ||
| useEffect(() => { | ||
| if (!src.startsWith('blob:')) { |
|
Hey hey, also thanks from my side and welcome aboard! Ill drop some more comments - so far nothing critical, just small things to consider. |
| # Check that url is a valid spotify url | ||
| # Check that url is a valid supported source url | ||
| redirect_url: str | None = None | ||
| if "spotify" in url: |
There was a problem hiding this comment.
Maybe its time we check for actual url schemas, not just service names - with all the services we now support, we might hit a wrong one if it occurs in the (admittedly unusual) album name.
| SiDiscogs, | ||
| SiMusicbrainz, | ||
| SiSpotify, | ||
| } from '@icons-pack/react-simple-icons'; |
There was a problem hiding this comment.
@llttlltt do you know how the simpleicon lib ships the icons?
the motivation of the comment is to keep client side payloads as small as possible.
if only those icons that are imported end up in the compiled site, im in favour of adding the lib.
|
Hey both, thanks so much for the detailed feedback and openness to contributions, I'll get around to looking into the comments and addressing them during the week! |
3 participants
Hello!
First off I wanted to say thank you for all of your hard work on beets-flask - I’ve been using it for a while now and it’s made managing my library so much quicker and more enjoyable!
I’ve prepared a few improvements to the
backend/beets_flask/server/routes/art_preview.pyto fetch artwork from Bandcamp, Discogs, and Beatport (implemented as close to the specific beets plugins as possible) - this should resolve #274.I've also adjusted the cover-art flow so missing artwork behaves reliably and consistently, while hopefully reducing CORS issues by moving more of the external art resolution to the backend (I no longer see CORS errors in Safari so this should fix #275) and added simple-icons (https://simpleicons.org) for source logos in the frontend.
What’s changed
Backend
/art.404response from backend.Frontend
externalArtQueryOptionsto:/artresponse status before treating it as successHTTPErroron non-OK responsesURL.createObjectURL(blob)for successful image payloadsThis makes the existing
CoverArtErrorfallback path reliably render when artwork can’t be fetched.UI
simple-iconsinSourceTypeIcon.useSourceColors?: booleanso icon colouring is opt-in at some point.Why this helps
Testing
backend/tests/integration/test_routes/test_art_preview.py404for unresolved art URLsLinks / references
Environment
Used for beets-flask in
requirements.txt:Config for beets-flask in
config.yaml:Config for beets in
config.yaml: