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

refactor(server): decouple generated images from image formats #8246

Merged
merged 14 commits into from Apr 2, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Mar 24, 2024

Description

The codebase currently hardcodes the formats of preview and thumbnail images, down to the thumbnail path being webpPath in the DB. This PR does two things:

  1. Standardize the naming of generated images into preview and thumbnail without assuming their format
  2. Change target image paths to have the type of the image in the name, e.g. asset-id-preview.jpeg and asset-id-thumbnail.jpeg
    • This is necessary to allow the same format for both types

These changes unblock future PRs to configure the target image format and support additional formats.

How Has This Been Tested?

Tested browsing, generating thumbnails, changing image resolution, running the migration job for the new image paths and installing and using a fresh app from the app store.

Copy link

cloudflare-pages bot commented Mar 24, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: a367174
Status:⚡️  Build in progress...

View logs

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.

There seem to be a lot of mess ups where GENERATE_JPEG_THUMBNAIL becomes GENERATE_THUMBNAIL and vice versa (I stopped looking for them when I hit media.service.spec) . I assume those aren't intentional changes?
Also, this is a breaking change, correct? The mobile app will break completely?

server/src/services/job.service.spec.ts Outdated Show resolved Hide resolved
server/src/services/job.service.spec.ts Outdated Show resolved Hide resolved
server/src/services/job.service.spec.ts Outdated Show resolved Hide resolved
server/src/services/job.service.ts Outdated Show resolved Hide resolved
server/src/services/job.service.ts Outdated Show resolved Hide resolved
server/src/services/job.service.ts Outdated Show resolved Hide resolved
server/src/services/job.service.ts Outdated Show resolved Hide resolved
@alextran1502
Copy link
Contributor

How is the current version on the app store of the mobile app behave with these changes?

@mertalev
Copy link
Contributor Author

mertalev commented Mar 24, 2024

I'm not sure. I haven't tested mobile, but there didn't seem to be anything that relies specifically on the name of webpPath or resizePath.

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'm fine changing the internal usage, but the API changes are breaking changes and I had planned on moving these API calls to a media controller, so it would be a bit annoying to break it twice.

@mertalev
Copy link
Contributor Author

The app seems to work fine for me with this PR.

@mertalev
Copy link
Contributor Author

Coming back to this PR, is the concern here that it'll break third-party code? I suppose I could revert the DB / config file changes, leaving the renamed job names, enum names and adding -preview / -thumbnail to generated images.

@jrasm91
Copy link
Contributor

jrasm91 commented Mar 28, 2024

I am fine with everything, including configuration changes, with the exception of the public facing server API changes. And only because I would love to migrate to a media controller with endpoints like /media/* and suggest that we implement the breaking API changes as new endpoints rather than updating the current ones. What do you think?

@mertalev
Copy link
Contributor Author

I think the only API-facing change is renaming the DB columns. I think we could have the DTOs continue accepting the old names (marked deprecated) and map them internally, keeping the PR the same otherwise.

@jrasm91
Copy link
Contributor

jrasm91 commented Mar 28, 2024

If there are no (breaking) changes to the open API spec file then I would say it is fine.

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.

Nice work! Quite hard to not mix it up all the time - both when reviewing and especially when migrating it I'd imagine 😅

@mertalev mertalev force-pushed the refactor/decouple-image-format branch 2 times, most recently from d062024 to cf0b00e Compare March 31, 2024 05:18
@mertalev mertalev force-pushed the refactor/decouple-image-format branch from 171bf29 to 84d6497 Compare March 31, 2024 05:40
@mertalev mertalev requested a review from jrasm91 March 31, 2024 05:44
@alextran1502 alextran1502 enabled auto-merge (squash) April 2, 2024 04:48
@alextran1502 alextran1502 merged commit 8edc2fb into main Apr 2, 2024
23 of 24 checks passed
@alextran1502 alextran1502 deleted the refactor/decouple-image-format branch April 2, 2024 04:56
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.

None yet

4 participants