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

fix(server): Allow negative rating (for rejected images) #15699

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

chkuendig
Copy link
Contributor

@chkuendig chkuendig commented Jan 26, 2025

Resolves (partially) #14454. ( edit: also, see relevant discord discussion)

XMP/Exif Metadata allows for negative ratings, with -1 meaning "rejected". See e.g. https://exiftool.org/TagNames/XMP.html#xmp:

Tag Name Writable Values / Notes
Rating real (a value from 0 to 5, or -1 for "rejected")

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.

Will negative ratings already be extracted? Are they stored correctly? The frontend also needs to handle them

@chkuendig chkuendig changed the title Allow negative rating (for rejected images) fix(server) Allow negative rating (for rejected images) Jan 27, 2025
@chkuendig chkuendig changed the title fix(server) Allow negative rating (for rejected images) fix(server): Allow negative rating (for rejected images) Jan 27, 2025
@chkuendig
Copy link
Contributor Author

chkuendig commented Jan 27, 2025

Will negative ratings already be extracted? Are they stored correctly?

I missed a few things. Pushed it now to include all parts for extraction and storing. I tested this locally and it worked for me. Sorry for pushing the barebone commit yesterday.

The frontend also needs to handle them

I would add this as a separate part if possible. I don't even think these should necessarily be reflected in the ratings/star UI. Since it doesn't make much sense to show a negative star rating - maybe they should be flagged as isHidden instead? For now it would just be good to have the metadata in the file be represented in the database so users (me) can build processes around that (in my case I'll mark -1 as hidden and 5 as a favorite via the REST API).

@danieldietzler
Copy link
Member

@chkuendig As long as you have checked that a rating of -1 does not actually break something on the clients I'm happy with doing that in the future PR :)

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.

Looks good! Thanks for also writing tests!

@chkuendig
Copy link
Contributor Author

chkuendig commented Jan 27, 2025

@chkuendig As long as you have checked that a rating of -1 does not actually break something on the clients

I tested web and iOS which handled this just fine (showing as no rating). Don't have Android available to test unfortunately.

@alextran1502 alextran1502 merged commit fe1e09e into immich-app:main Jan 28, 2025
33 checks passed
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.

4 participants