-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(server): crop and rotate #11700
base: main
Are you sure you want to change the base?
feat(server): crop and rotate #11700
Conversation
web/src/lib/components/asset-viewer/actions/rotate-action.svelte
Outdated
Show resolved
Hide resolved
e2e/test-assets
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like on main this points to an outdated commit: e2e/test-assets points to https://github.com/immich-app/test-assets/tree/39f25a96f13f743c96cdb7c6d93b031fcb61b83c but the commit hash has changed when the PR was merged
// force the image to refresh the thumbnail | ||
const oldSrc = new URL($photoViewer!.src); | ||
oldSrc.searchParams.set('t', Date.now().toString()); | ||
$photoViewer!.src = oldSrc.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We rely on the checksum field to detect changes in the thumbnail and bypass the cache. Refreshing based on the time might work in this component, but it will cause caching issues everywhere else. Mobile might have a similar caching issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's not pretty; but @jrasm91 told me on Discord the ideal way would through a web socket event but that since we don't currently have one for thumbnail generation, refreshing like that was a good enough hack until it's done.
How would you do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally fine just having a workaround here and then we'll hopefully have websocket events soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how a websocket event will solve caching issues in other components/pages. We include the checksum
for thumbnail requests, but the checksum doesn't change in this case so the cached thumbnail is still used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should use a counter/nonce instead of the checksum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry now I get your point. You're absolutely right, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, uh, sorry, but what should I change here?
web/src/lib/components/asset-viewer/actions/rotate-action.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks very good to me
3312b1d
to
b506ec6
Compare
5811c33
to
73795b2
Compare
This PR adds support for using the Orientation EXIF field from the sidecar (which can be modified without touching the original file).
It also adds two actions to the web viewer that "demo" that feature:

Issues:
img
tag's source field after 500 milliseconds.