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(server): use the earliest date between file creation and modification timestamps when missing exif tags #14874

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Chuckame
Copy link

I also applied the date extraction for motion asset creation, tell me if I need to rollback this part, maybe it was wanted to not re-use getDates.

Closes #14857

Copy link
Contributor

@alextran1502 alextran1502 left a comment

Choose a reason for hiding this comment

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

LGTM, please address the comments

@alextran1502 alextran1502 changed the title feat(server): Use the earliest date between file creation and modification timestamps when missing exif tags feat(server): use the earliest date between file creation and modification timestamps when missing exif tags Dec 23, 2024
Copy link
Contributor

📖 Documentation deployed to pr-14874.preview.immich.app

this.logger.warn(
`No valid date found in exif tags from asset ${asset.id}, falling back to earliest timestamp between file creation and file modification`,
);
// eslint-disable-next-line unicorn/prefer-math-min-max
Copy link
Member

Choose a reason for hiding this comment

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

Why not do Math.min(asset.fileModifiedAt, asset.fileCreatedAt)?

Copy link
Author

Choose a reason for hiding this comment

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

Because it returns a number, but I can do new Date(Math.min(asset.fileModifiedAt, asset.fileCreatedAt)) instead if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that, thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Sadly, ts linter reports it 😆
image
image
You have to choose the pill: the red or the green one ? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Date.valueOf() or Date.getTime() also work.

new Date(Math.min(asset.fileModifiedAt.valueOf(), asset.fileCreatedAt.valueOf()))

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to take either pill and instead just use luxon lmao. Do you think that's overkill or would it make things nicely readable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Funny enough luxon's min/max functions require a DateTime instead of a Date, so a conversion is going to have to happen somewhere anyway, which means something like:

DateTime.min(DateTime.fromJSDate(asset.fileModifiedAt), DateTime.fromJSDate(asset.fileCreatedAt)).toJSDate()

😅

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.

Date fallback should be the earliest between creation and modification dates
4 participants