-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Feature/improve date selection #9160
Conversation
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.
Looks pretty good. Just two requests from you:
- Avoid loading each asset individually (especially when the keepTimeChanged flag isn't set to true)
- Add some unit tests for the logic you have added (asset.service.spec.ts)
const asset = await this.assetRepository.getById(id); | ||
const oldCreatedAtDate = asset?.fileCreatedAt && DateTime.fromJSDate(asset.fileCreatedAt); | ||
let newDateTimeString = dateTimeOriginal; | ||
if (dateTimeOriginal && keepTimeUnchanged && oldCreatedAtDate) { | ||
let newDateTime = DateTime.fromISO(dateTimeOriginal); | ||
|
||
newDateTime = newDateTime.set({ | ||
hour: oldCreatedAtDate.hour, | ||
minute: oldCreatedAtDate?.minute, | ||
second: oldCreatedAtDate.second, | ||
}); | ||
const newDateTimeStringWithNull = newDateTime?.toISO(); | ||
newDateTimeString = newDateTimeStringWithNull === null ? undefined : newDateTimeStringWithNull; | ||
} |
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.
This is inside a for loop, which might be called with 10k assets. Loading an asset from the database one at a time is going to be a problem. In hindsight, calling updateMetadata one at a time, which eventually calls upsertExif might not be the best either. I would probably recommend finding a more optimized solution. Ideally we move this logic into updateMetadata itself, and change that method to accept a list of ids to update. Only if the keepTimeUnchanged property is set, we can use the assetRepo.getByIds methods to load them in a single query.
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 could also move the "keepTimeUnchanged" logic inside the SIDECAR_WRITE job or create a new one for that case.
The problem I see with changing multiple assets in one Job is the error handling.
What should happen if one of the metadata updates fail?
And maybe one side question:
is await this.jobRepository.queue
waiting for the job to complete, or is it fire and forget. And what would happen if the SIDECAR_WRITE fails and the
await this.assetRepository.updateAll(ids, options); // asset.services.ts L 343
still is done.
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.
.queue is waiting for the job to be added to the queue. It is fine if the job takes longer to execute since it happens in the background.
Currently the bulk update (and single update) method optimistically update the database so the UI can refresh immediately and see the changes, but then persist the changes to a sidecar file so future metadata extraction doesn't erase/override the changes.
It is important that the initial implementation is fast, since the caller is waiting. Updating every asset individually makes that a little bit more difficult, especially if the asset needs to be read ahead of time.
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 options are to not do any update immediately, but defer it to the job. The user might be confused as the time isn't updated and the assets still appear in the wrong place for some time.
Or, update each asset one at a time, but optimize the loading to be in a single database request.
In the change date dialog the inital date is not now anymore but the date of the first selected asset
In the change date dialog there is now an option to ignore time change and set the original time of the asset
Co-authored-by: Daniel Dietzler <[email protected]>
6dd0a73
to
c6fc22b
Compare
Multiple PRs have been merged to improve date selection, and this PR has been stale. Thank you for your effort and apologize that this PR didn't get resolved |
This PR contains two changes, the smaller one sets the initial date of the date change dialog to the date of the first selected asset.
The bigger one adds an option to ignore the Time change and sets the original time.
The idea behind this PR is that I often upload images/videos from my GoPro or Drone. But both of them have sometimes a problem with the date and or the time.
So if I bulk change all the asses from one day, where date and time are off, I want to keep the "false" time because this will keep the assets in the right order. The smaller change is pure laziness, as I think it is easier in the date picket to get to "Today" than it is to go back "3 years" to the original asset date. I picked the first asset because I think most users will use the change date on similar dated assets.