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

feat(web): add Exif-Rating #11580

Merged
merged 16 commits into from
Aug 9, 2024
Merged

feat(web): add Exif-Rating #11580

merged 16 commits into from
Aug 9, 2024

Conversation

stumpigit
Copy link
Contributor

This PR enables the rating tag to be read from the Exif data and saves it in the database. The rating can be displayed in the photo viewer using a star display and can also be customised with the appropriate rights.

Rating

This PR contains

  • Database migration
  • Adaptation to the DTOs
  • Adjustments to detail-panel.svelte with integration of the Svelte component svelte-star-rating

This PR fulfils parts of the following discussions:
#7100
#3619

As a further development, the option of filtering by rating will be implemented.

bo0tzz
bo0tzz previously requested changes Aug 5, 2024
Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

Nice!

web/package.json Outdated Show resolved Hide resolved
@alextran1502
Copy link
Contributor

Thank you for the PR. I have difficulty convincing myself to display the star count because it is tailored for photo editing rather than viewing/browsing software.

My thought process is that if you only show the star count in the Info panel, it doesn't help you find more "likes" when browsing the timeline, and if we show that information on the timeline, it gets cluttered.

My main concern is that it gets the UI clutter while doesn't serve a lot of purpose

@stumpigit
Copy link
Contributor Author

Thank you for the PR. I have difficulty convincing myself to display the star count because it is tailored for photo editing rather than viewing/browsing software.

My thought process is that if you only show the star count in the Info panel, it doesn't help you find more "likes" when browsing the timeline, and if we show that information on the timeline, it gets cluttered.

My main concern is that it gets the UI clutter while doesn't serve a lot of purpose

Thank you Alex for your feedback and for your work in general. From my point of view, the discussions in #3619 and also general discussions on the internet about the large cloud provider show the need for this function. As a photographer, it is very important to me that I can assign stars in my photo workflow. I want to have all photos online, but quickly filter only those that have more than 3 stars, for example. I then use these to create an album. A lot of image editing tools and many cameras support the rating and are based on this system.

In my opinion, Synology Photo is a good example of how this has been implemented (you can try it out online at https://demo.synology.com/de-de/dsm). The rating appears in the info panel and then in the filter for the photos. I think this way the user interface is not overloaded, is intuitive and gives many photographers a powerful tool at hand.

syno rating
syno rating2

@alextran1502
Copy link
Contributor

Thank you. Let's refactor it into its own component, and then I can help find a place for it in the INFO panel.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Can you add an e2e test for setting the rating on an asset as well? (e2e/ folder)

server/src/dtos/asset.dto.ts Show resolved Hide resolved
server/src/entities/exif.entity.ts Show resolved Hide resolved
web/package.json Outdated Show resolved Hide resolved
@stumpigit
Copy link
Contributor Author

I have just comitted the following adjustments:

  • The Svelte-Star-Rating dependency has been removed
  • Developed my own component "detail-panel-rating.svelte", which creates the stars with SVG and Tailwind. These are also somewhat finer than the first proposal. So I hope that they fit better into the design.
  • Proposal to place the rating just below the description (modelled on Synology Photos)

Screenshot 2024-08-05 203845

@stumpigit stumpigit requested a review from bo0tzz August 5, 2024 19:46
@mertalev
Copy link
Contributor

mertalev commented Aug 6, 2024

Just to offer another perspective: I never use this feature in photo apps, personally. It generally adds visual clutter and the way the stars tend to change color when you hover over them is a nuisance. This last version you shared doesn't look too intrusive, though.

@stumpigit stumpigit requested a review from jrasm91 August 6, 2024 11:56
@stumpigit stumpigit changed the title Add Exif-Rating feat(server,web): Add Exif-Rating Aug 6, 2024
@jrasm91
Copy link
Contributor

jrasm91 commented Aug 6, 2024

Another option is to have this be a setting the use can turn on our off

@alextran1502
Copy link
Contributor

Another option is to have this be a setting the use can turn on our off

yep, user's preference to the rescue

@stumpigit
Copy link
Contributor Author

So should I try to add it in the user settings? Under app settings? I'm welcome to try that.

@alextran1502
Copy link
Contributor

@stumpigit yeah, let do that. Please use the user preference setting mechanism for this, so that the user preference will be used across the clients

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

Looking pretty good so far! :)

@stumpigit
Copy link
Contributor Author

Now you can set in the User Settings under App Settings whether the star rating should be displayed.
Screenshot 2024-08-07 095958
Unfortunately, I'm not quite sure how the process works with the translations. I've comitted en.json and de.json.

@alextran1502
Copy link
Contributor

Sorry put in the wrong number for the issue :P

@mertalev
Copy link
Contributor

mertalev commented Aug 7, 2024

Wait, so a PR with the fixes keyword can close other PRs?

@alextran1502 alextran1502 changed the title feat(server,web): Add Exif-Rating feat(web): add Exif-Rating Aug 7, 2024
@alextran1502
Copy link
Contributor

Wait, so a PR with the fixes keyword can close other PRs?

YEP, TIL

@stumpigit
Copy link
Contributor Author

I have adapted the behaviour of the stars to the usual behaviour patterns. The design has also become a little more refined.

Bildschirmaufnahme.2024-08-07.215233.mp4

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for working on this

@stumpigit stumpigit requested a review from jrasm91 August 9, 2024 08:24
@mertalev mertalev dismissed bo0tzz’s stale review August 9, 2024 15:22

Removed star rating dependency

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

I think this is good to go now. I changed a few things with the code:

  • StarRating wasn't actually a rating component, it was just a single star icon. I changed it to be the full component with the interface of
<StarRating rating={3} count={5} onRating={(rating => ... } readOnly={true}/>
  • Swapped out the svg + path elements for <Icon/> which are use everywhere else for icons
  • Changed the color to immich-purple, which is obviously subjective we can change it to yellow if there is a consensus on the preferred color
  • Changed the hover functionality to only revert back to the set value when the mouse leaves the entire component - this prevents jank
  • Changed the conditional rendering logic to be inside of the Component instead of in the asset viewer, which is already big enough as it is
  • Moved the click/blur/tab handlers to the button instead of the svg path element
  • Fixed e2e formatting
  • Renamed app_settings to rating, etc.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

LGTM

@alextran1502 alextran1502 enabled auto-merge (squash) August 9, 2024 17:43
@alextran1502 alextran1502 merged commit f33dbdf into immich-app:main Aug 9, 2024
22 checks passed
@Orbitally
Copy link

I have adapted the behaviour of the stars to the usual behaviour patterns. The design has also become a little more refined.

Bildschirmaufnahme.2024-08-07.215233.mp4

I know this has already been merged but just figured I'd chime in quickly anyways with a tiny suggestion. The outline of the star you're hovering holding is a really clean way of displaying the interaction but my initial thought when I saw it was that it may make more sense if all the stars leading up to the one being hovered bolded as well. I wasn't sure if this would just help with continuity since they all become coloured once the star gets clicked as well as just a visual indicator that you're about to apply all the stars up to and including the one you click.

@stumpigit
Copy link
Contributor Author

Thank you for your feedback @Orbitally. @jrasm91 has already made some adjustments to the behaviour of the stars. In the merged version it now looks like this:

Bildschirmaufnahme.2024-08-10.200545.mp4

From my point of view it makes sense. I have seen very different behaviours of the stars in my research. But it certainly fits in well with the further operation of Immich.

@Orbitally
Copy link

Nice! I didn't catch that change but that looks great too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants