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

Set cover image for album/calendar/folder/moment #924

Closed
wants to merge 9 commits into from

Conversation

kvalev
Copy link
Contributor

@kvalev kvalev commented Jan 19, 2021

Tasks:

  • backend changes
  • handle cover caching
  • UI changes
  • UI styling
  • unit tests
  • UI/integration/e2e tests
  • translations

Closes #383

@kvalev
Copy link
Contributor Author

kvalev commented Jan 20, 2021

@lastzero I think I am mostly done, would you mind taking a look. Tests and translations are missing, tests should be doable, but I am not sure how to handle the translations? Should I wait until someone translates it into every supported language? Do I just translate it with DeepL?

@kvalev
Copy link
Contributor Author

kvalev commented Jan 25, 2021

@lastzero Did you had a chance to take a look?

@lastzero
Copy link
Member

Not yet, sorry. Might take a couple of days. Had to take care of a few bug reports and UX issues first (full log didn't fit on screen):

commits

@graciousgrey
Copy link
Member

@kvalev I checked out your branch (cover-image) to test. I see the button in the UI but the album cover is not updated for me and I get no error

@kvalev
Copy link
Contributor Author

kvalev commented Feb 17, 2021

If you refresh the page (browser reload, not pp reload) you should see the updated cover. For some reason pp does not fetch the updated covers for a page, which was already visited (e.g. going from folders to a specific folder/album, updating the cover there and then going back to folders).

@graciousgrey
Copy link
Member

Ok, I can confirm it works when reloading and emptying the cache of the browser.

Thanks for your work!

I still see the following to dos:

  • Album cover MUST update when clicking reload in PhotoPrism or directly
  • The option in the context menu MUST only be shown inside of albums/moments/calendar/states/folders and originals and not in sections like search or video because those do not have a cover
  • Nice to have: Set cover of a label as well

@kvalev
Copy link
Contributor Author

kvalev commented Feb 18, 2021

The first two are done. The third one should be fairly easy, but it requires extending the database schema and my understanding is that you prefer not to make any database changes at this time.

@graciousgrey
Copy link
Member

Thank you! I will have a look later today or tomorrow :)

@graciousgrey
Copy link
Member

Functionality looks good :) @lastzero will have a look at the code as well as soon as possible, we need to find out why the build is failing

Thanks for your work!

@kvalev kvalev marked this pull request as ready for review March 2, 2021 16:33
@kvalev
Copy link
Contributor Author

kvalev commented Mar 20, 2021

Functionality looks good :) @lastzero will have a look at the code as well as soon as possible, we need to find out why the build is failing

The failing build has been fixed

@graciousgrey
Copy link
Member

Thank you, unfortunately we won't find time in March. In the meantime you can send us an email to hello@photoprism.app so that we can send you instructions on how to enable sponsor/contributor features, if you like :)

@lastzero lastzero self-assigned this Apr 11, 2021
@lastzero
Copy link
Member

lastzero commented Sep 4, 2021

We have thumb columns in our database for this now, so that no UID lookup is required anymore, plus you can set a custom cover if you change the src to any value <> ''

@kvalev kvalev closed this Sep 6, 2021
@kvalev kvalev deleted the kvv-cover-image branch September 6, 2021 18:49
@fehervalentin
Copy link

Hi! What about this PR? I am looking for this functionality too. What was the issue with the branch/commits?

plus you can set a custom cover if you change the src to any value <> ''

@lastzero you mean directly in the database, right?

@graciousgrey
Copy link
Member

graciousgrey commented Nov 27, 2021

@q3flat

The problem is missing time and resources.

We have high quality standards and merging pull requests requires a lot of work and time on our side.

  • The feature has to be fully implemented - in the case of "set cover image", this means the ability to set cover images for all types of albums and people
  • Unit and acceptance tests have to be written
  • The feature must work with sqlite and mysql
  • The UI must be fully responsive
  • It needs to be tested on multiple browsers and devices
  • Translations need to be created
  • Docs need to be updated

We would love to grow our team to be able to work faster but we can't even cover our own expenses yet --> https://github.com/sponsors/photoprism

To set a cover image in the database, you need to add the filehash of the file you want to have as cover in the "thumbs" column. Additionally you need to set "thumb_src" to "manual".

@lastzero
Copy link
Member

Check out the Release Notes if you wonder why we didn't have time yet:

https://docs.photoprism.org/release-notes/

@fehervalentin
Copy link

fehervalentin commented Nov 27, 2021

Thanks for the responses! I understand that.

So technically PP is capable of this "feature" by now. It is great for me. Thanks!

(My previous nickname was q3flat)

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.

Albums: Set cover image for album/calendar/folder/moment/state/person
4 participants