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

Implements setting of cover image for Albums for #383 #3758

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

Conversation

Xirt
Copy link

@Xirt Xirt commented Sep 22, 2023

This implements the functionality to select an album cover from all photos in an Album as requested and discussed in #383. See discussion for details on implementation (incl. screenshots & confirmation on performance).

Acceptance Criteria:

  • Fully functional implementation for setting / selecting Album covers; code style in line with other PhotoPrism code styling.
  • Automated test added for changes to Album
  • Tested on various devices / dimensions with major browsers (e.g. Edge, Chrome & FireFox)
  • Translations provided for new label "Thumbnail"
  • No DB changes (leveraging existing fields Thumb / ThumbSrc)

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2023

CLA assistant check
All committers have signed the CLA.

@graciousgrey
Copy link
Member

Thank you for your contribution! We are currently preparing our next release and look forward to reviewing your PR after a short break.

@graciousgrey
Copy link
Member

Thank you very much for your work!

I have done some initial tests. Performance wise it works fine, at least with a few hundred photos in an album 👍 . I still need to do tests with very large albums (1000s of pictures).

I found a few things that we need to resolve/discuss before we can merge this:

  • It MUST NOT be possible to select pictures marked as private as cover.
  • If the photo that is selected as cover is marked as private at a later time, the cover MUST be set to another photo, even if the ThumbSrc is manual
  • If the photo that was selected as cover is later archived, the cover MUST be set to another photo, even if the ThumbSrc is manual.
  • If the photo that was selected as the cover is later removed from the album, the cover MUST be set to another photo, even if the ThumbSrc is manual.

We can happily take care of those once we have completed our current priority tasks (Open ID and 2FA).

Another thing that may not be a blocker, but definitely needs long term improvement, is that the thumbnails are very small. So people who can't see that well might have trouble setting a cover.

@graciousgrey graciousgrey added the work-in-progress Please don't merge just yet label Oct 9, 2023
@cyanue
Copy link

cyanue commented Nov 20, 2023

I see that the submitted method is to select a photo from an album as the cover. The problem with this approach is that when there are too many photos in the album, it may be slow or crash. Is it possible to use another method, such as adding a checkbox in the photo editing interface, to select the photo as the cover for the entire album? This way, we can avoid listing all the photos in the album.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Please don't merge just yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants