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

Metadata mapping and consolidation #868

Merged
merged 7 commits into from
Mar 30, 2024
Merged

Metadata mapping and consolidation #868

merged 7 commits into from
Mar 30, 2024

Conversation

grasdk
Copy link
Contributor

@grasdk grasdk commented Mar 30, 2024

  • Removed the SideCar Type to make reading sidecar data just as generic as reading the embedded exif data
  • Moved metadata-mapping into separate function to prepare for reuse
  • Most sidecar metadata is now read with the same function as embedded metadata (for photos). Fixed a bug with rating: 0 being ignored
  • added new test for special chars and timezone
  • merge incoming changes
  • xmp rating fix
  • refactored mapping of metadata into smaller functions
  • consolidated GPS coordinate mapping
  • simplification of sidecar date reading logic
  • Moved date mapping code into one function - fixed testdata
  • CreationDateOffset is now mapped from Sidecar timestamps. Tests updated accordingly.
  • Changed sidecar reading for video to shared reader and fixed tests accordingly (more data is read now)

* Removed the SideCar Type to make reading sidecar data just as generic as reading the embedded exif data

* Moved metadata-mapping into separate function to prepare for reuse

* Most sidecar metadata is now read with the same function as embedded metadata (for photos). Fixed a bug with rating: 0 being ignored

* added new test for special chars and timezone

* merge incoming changes

* xmp rating fix - 0 is a value. prep for xmp timestamp handling

* refactored mapping of metadata into smaller functions

* consolidated GPS coordinate mapping

* simplification of sidecar date reading logic

* Moved date mapping code into one function - fixed testdata - did not yet resolve offset bug

* CreationDateOffset is now mapped from Sidecar timestamps. Tests updated accordingly.


Total: changed sidecar reading for video to shared reader and fixed tests accordingly (more data is read now)
@grasdk
Copy link
Contributor Author

grasdk commented Mar 30, 2024

For some in depth info, take a look at each commit. It shows the "journey" in smaller steps:
master...grasdk:pigallery2:feature/metadata-sidecar-reading-cleanup

@grasdk
Copy link
Contributor Author

grasdk commented Mar 30, 2024

Hey @bpatrik
So this was meant to make it easier to add future fixes (new tags to read metadata from, or even new metadata), in a way that it will both embedded data in photoes and sidecar data for both photos and videos.

In the process I also stumbled over xmp.rating, which I asked a question about. I took the liberty of updating according to the adobe spec (adding -1 as legal value) in this pull request, but it can be easily undone, if you would rather not have that.

  • if keeping it, should I bump the database version again to ensure that running installations get their databases rebuilt?

@grasdk
Copy link
Contributor Author

grasdk commented Mar 30, 2024

I found out I'm missing the -1 min value in query-entry.search.gallery.component.html. I will add this of course, unless you'd rather want the rating part reverted.

Extra question: Should I also add the "no rating" means rating 0 in pigallery, is the adobe specification suggests is the intention?

@bpatrik
Copy link
Owner

bpatrik commented Mar 30, 2024

Huge thanks for the refactoring! This an addition that also wanted to do myself, but I find it hard recently spend time on this project and also I'm getting lost in the extension configuration labyrinth.

Would you mind separating the -1 change into a separate PR from the refactoring part?

For PRs I preffer having as small as possible as it's easier to review, but at least as big as merging it still makes production coherent (lot of people uses the edge build, so if it builds it should be consistent).

For the -1 handling I will answer in separate thread.

@bpatrik
Copy link
Owner

bpatrik commented Mar 30, 2024

Hey @bpatrik
So this was meant to make it easier to add future fixes (new tags to read metadata from, or even new metadata), in a way that it will both embedded data in photoes and sidecar data for both photos and videos.

In the process I also stumbled over xmp.rating, which I asked a question about. I took the liberty of updating according to the adobe spec (adding -1 as legal value) in this pull request, but it can be easily undone, if you would rather not have that.

  • if keeping it, should I bump the database version again to ensure that running installations get their databases rebuilt?

Yes if reindexing is needed (which in this case, would), that should be bumped

@grasdk
Copy link
Contributor Author

grasdk commented Mar 30, 2024

Ok, xmp.rating now works as before, 0 and below maps to "undefined" (also for embedded metadata). Values larger than 5 maps to 5. In case of decimal ratings, I added rounding as well.
This should be in line with your comment, and a suitable part of the refactoring :)

Based on this I think rating should work as follows:

  • Undefined, null and 0 are equivalent and metadat.rating should not be set.
    • searching for 0 rating should list all of these photos
  • Valid ratings are 1-5. UI only shows rating if its between 1-5.
  • -1 mapped to 0

Originally posted by @bpatrik in #865 (comment)

Details: b071b6d and 9aab44b (doh, forgot that "<=" instead of "<")

@@ -132,7 +132,7 @@ export class MediaMetadataEntity implements MediaMetadata {
@Column(() => PositionMetaDataEntity)
positionData: PositionMetaDataEntity;

@Column('tinyint', {unsigned: true})
@Column('tinyint', {unsigned: false})
Copy link
Owner

Choose a reason for hiding this comment

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

This should stay unsigned.

@bpatrik bpatrik merged commit 086bc47 into bpatrik:master Mar 30, 2024
5 checks passed
@bpatrik
Copy link
Owner

bpatrik commented Mar 30, 2024

Great, thank you very much! :)

@grasdk
Copy link
Contributor Author

grasdk commented Mar 30, 2024

Thank you too :)

Another step for this old enhancement request, since the refactor adds more support for video metadata, due to the consolidation of metadata handling. :)

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.

2 participants