feat:extra_tags_registry #114
Conversation
|
Hi @feefladder, I'm super busy right now (upcoming conference + travel), so won't have the headspace to review until late August. Maybe @kylebarron can have a look if he has time. |
…ing in ifd::from_tags
| if extra_tags_registry.contains(&t) { | ||
| extra_tags_registry[&t].process_tag(t, value).map_err(|e| { | ||
| if let AsyncTiffError::InternalTIFFError(err) = e { | ||
| err | ||
| } else { | ||
| // TODO fix error handling. This is bad | ||
| TiffError::IntSizeError | ||
| } |
There was a problem hiding this comment.
For most purposes, the error will come from Value::into_<something>, which is InternalTiffError, so there it shouldn't be a problem. An alternative would be to panic!, or to do this outside of the match block.
| // (using thin pointer equality: https://stackoverflow.com/a/67114787/14681457 ; https://github.com/rust-lang/rust/issues/46139#issuecomment-346971153) | ||
| if seen.insert(Arc::as_ptr(extra_tags) as *const ()) { | ||
| if let Err(e) = new_registry.register(extra_tags.clone_arc()) { | ||
| panic!("{e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we only expect different arcs to come from register, which is Arc::clone, fat pointers should also compare equal, but there is a bit of a rabbit-hole with there being no guarantee on vtable layouts (IIRC)
There was a problem hiding this comment.
I was a bit unsure to put this here or in tests/image_tiff/images, since it's strictly speaking not from there (I do like the example being exif)
2 participants
closes #100
This PR only does the registry and the needed API. There should be a separate PR coming that moves all geo-tag parsing code to use this system. I thought there'd be enough changes in this PR already
notes:
Arc::clones, but it's a bit of a large possible footgun that's easily avoided.Cloneobject-safe, so there's the blanketExtraTagsCloneArctrait.IFDReader::read;IFDReader::read_all_ifds())fn tags(&self) -> &'static [Tag]is a bit ugly, also in the tests&selfis needed because of object safetystaticorconstvariableifd::from_tagsread_all_tagsis not currently testedGeoTagstoExtraTagsRegistry::default()@weiji14 you gave 👀, feel like reviewing? :)