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: Edit metadata #5066

Merged
merged 31 commits into from Nov 30, 2023
Merged

feat: Edit metadata #5066

merged 31 commits into from Nov 30, 2023

Conversation

YFrendo
Copy link
Contributor

@YFrendo YFrendo commented Nov 15, 2023

This PR is "feature complete"

You can:

  • Change date
  • Change localization
  • Change in bulk

In order to do this the server create a sidecar file with 'exiftool-vendored', if there is already a sidecar the following value are overwrite if needed :

  • CreationDate
  • GPSLatitude
  • GPSLongitude

The metadata extraction job is run after this in order to actualize everything with the new data.

The Update of the frontend go through the websocket to update the front dynamically

The design of the frontend is very basic but I think it's the best I can do !

Screenshot

Change date:

Enregistrement.de.l.ecran.2023-11-29.a.13.15.07.mp4

Change location

Enregistrement.de.l.ecran.2023-11-29.a.13.16.34.mp4

Change in batch

Enregistrement.de.l.ecran.2023-11-29.a.13.17.43.mp4

Future improvement

  • custom date/time picker instead of the system one
  • Downlaod sidecar files alongside the asset
  • Global UI improvment
  • Send error message to the client if there is an issue (only in the logs curently)

But this can be add later in other PR!

@bo0tzz
Copy link
Member

bo0tzz commented Nov 15, 2023

Thanks for the PR! I think the general direction is looking good. I see you're writing to a separate sidecar file rather than using the one that is (maybe) already there. Is there a particular reason for that?

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 15, 2023

Yeah I don't want to override user sidecars, the goal is only to correct the graphical view and not to change the user sidecars. But if you think it's a better idea to use the sidecar of the user I can do it this way it will be easier!

@bo0tzz bo0tzz changed the title feat: edit-medata feat: Edit metadata Nov 15, 2023
@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 16, 2023

For now the server part is ready for change date and position for singular asset or in bulk.

For now it overide the preexisting sidecar (but create a save of the previous one), I assume that if a user changes the metadata he also wants to modify his sidecar. I don't see any case where that wouldn't be the case.

I need extensive testing also

I will start the frontend part

@Bouni
Copy link

Bouni commented Nov 17, 2023

🤩 Exactly the feature that I was looking for!
@YFrendo How do you plan to let the user set a location for a picture?
I have tons of old pictures without geolocation at all which I want to update with the rough location they were taken at.
Personaly I would prefer to select the location on a map rather that entering lat/lon manually.

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 17, 2023

Select on the map is the best way to go, but my skills in frontend are limited so I need to figure out how to do it !
For now I have this :
Capture vidéo du 17-11-2023 00:06:08.webm

@YFrendo YFrendo marked this pull request as ready for review November 17, 2023 14:53
@Wordsmith9091
Copy link

It's so cool to see that this is in development.

Would you consider also capturing and saving the (existing, editable) description line to the sidecars?

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 18, 2023

Maybe in a future PR, I think there is enough change for this PR !

I am focusing on clear the code and writing test and docs for now !

server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/infra/repositories/asset.repository.ts Outdated Show resolved Hide resolved
creationDate: event.detail,
},
});
notificationController.show({ message: 'Metadata updated please reload to apply', type: NotificationType.Info });
Copy link
Member

Choose a reason for hiding this comment

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

Can we just change the asset var and let svelte handle this? (cc @jrasm91 / @alextran1502 - maybe not because that var is handed down from the timeline?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember what used to be here, but I think it's need fixed now?

Copy link
Member

Choose a reason for hiding this comment

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

My intent was rather than asking the user to reload the page, can we just update it in place?

web/src/lib/components/asset-viewer/detail-panel.svelte Outdated Show resolved Hide resolved
web/src/lib/components/asset-viewer/detail-panel.svelte Outdated Show resolved Hide resolved
web/src/lib/components/asset-viewer/detail-panel.svelte Outdated Show resolved Hide resolved
@YFrendo YFrendo requested a review from bo0tzz November 20, 2023 14:58
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/metadata/metadata.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/domain/asset/asset.service.ts Outdated Show resolved Hide resolved
@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 21, 2023

I just want to thanks @danieldietzler who save this PR from the mess I did during the rebase !

I'm going to work to migrate the writing process inside a job

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.

The server stuff looks good to me. I made a few changes:

  • moved write tags repo method to the metadata repository (instead of the asset repository)
  • renamed the job to sidecar-write
  • refactored the logic to queue the write job into a private method since it is used in both the single update and bulk update methods
  • Added some e2e tests for writing gps
  • Added some unit tests for the handleSidecarWrite method

The web could probably still use a little work. I made the following changes:

  • Removed if/else blocks and instead always render the same component (with conditional props instead)
  • Updated the new modals to extend the confirm dialogue instead of copying all it's internals
  • Updated the dropdown list to show timezone names and offsets (although there are a lot, maybe too many 😄)
  • Updated the timezone to use the custom select, which required a few changes to make its position fixed, instead of absolute

A few things to do (now or later):

  • listen to websocket event for metadata update
  • update the UI for the detail panel to look like google photos
  • custom date/time picker instead of the system one
  • timezone typeahead filter

@alextran1502
Copy link
Contributor

Hi @YFrendo I and Jason was testing the PR last night and realize the websocket mechanism can trigger rerendering the timeline which is what we don't want, so we've decided to resort back to display a notification and let the user refresh the page to see the upload

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 29, 2023

Hi @alextran1502 !

Should I revert everything related to the Websocket ?

Where is the issue with the rerendering of the timeline ? Genuine question just to understand where is my mistake.

@alextran1502
Copy link
Contributor

@YFrendo Yes you can revert the websocket changes.

It is not your mistake but how the update works on the timeline. You can try to upload a few assets, and you can see the placeholder get rendered on the timeline even though the photo thumbnail hasn't been processed yet. We specifically don't want to have websocket's event rerenders the timeline because during the bulk upload action, it will constantly shift things around and make the timeline unusable.

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 29, 2023

Everything related to the websocket is now revert, I tried to update the display of the asset when you update it but it create some mess with the timezone.
The update of the city can't work too, so i think the best way is juste to reload.

@YFrendo
Copy link
Contributor Author

YFrendo commented Nov 29, 2023

This version is without websocket, but can we keep websocket for :

  • Message to the client
  • Update the asset inside the information part (I don't know if it triggers the issue)

Or is it a more global issue around the websocket inside this context

@alextran1502
Copy link
Contributor

@YFrendo After further testing and realized that @jrasm91 has updated the websocket event emitter only when sidecar-write happen, websocket is not getting in the way of the timeline rendering anymore.

I am reverting the change to put back the websocket. Thank you for your work :)

@alextran1502 alextran1502 enabled auto-merge (squash) November 30, 2023 03:51
@alextran1502 alextran1502 merged commit 644e52b into immich-app:main Nov 30, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants