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

MBS-13832: Don't skip PDF files for frontiest check #3417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reosarevok
Copy link
Member

Implement MBS-13832

Problem

Frontiest PDFs are not shown on the sidebar for releases/events (it just shows "we have no front, see all", even if we do have a front but it's a PDF). We originally skipped PDFs because they didn't use to support thumbnails (making them bad choices for the sidebar and the like). That changed many years ago though, and especially for events, it's not uncommon for the frontiest image to be a PDF.

Solution

This just removes the check in index_listing views that filters out PDF files.

Testing

None yet.

We originally skipped PDFs because they didn't use to support thumbnails
(making them bad choices for the sidebar and the like). That changed
many years ago though, and especially for events, it's not uncommon
for the frontiest image to be a PDF.
@reosarevok reosarevok added the New feature Non urgent new stuff label Nov 25, 2024
Copy link
Member

@mwiencek mwiencek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but do we want to continue excluding them from the API's /front endpoint? I suppose there is a chance of breaking clients that don't expect PDFs from there, so it would be safer to preserve the existing behavior (and I'm not sure anybody has requested it be changed). If so, an artwork-redirect PR should be deployed first.

@reosarevok
Copy link
Member Author

We don't have any documentation saying we do exclude them (https://musicbrainz.org/doc/Cover_Art_Archive/API / https://musicbrainz.org/doc/Event_Art_Archive/API). It makes sense IMO to have an option to skip (or include) pdfs, but I'm not sure how that would work - as far as I can tell artwork-redirect just uses is_front from index_listing when asked for the frontiest, and index_listing seems to set is_front with LIMIT 1 so it doesn't have an alternative non-PDF option for cases when a PDF is_front.

Would the suggestion be to implement such a thing on the MBS side, with more than one is_front option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature Non urgent new stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants