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

Add support for iterating over all tags #255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gschulze
Copy link

Closes #243

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

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

One question I have is whether the strip/tile offsets and lengths should be included in the iteration over all tags. On one hand it is better for consistency, but they're also likely to be larger than the other tags and not especially helpful for users

@@ -895,6 +898,18 @@ impl<R: Read + Seek> Decoder<R> {
self.get_tag(tag)?.into_string()
}

pub fn tag_iter(&mut self) -> impl Iterator<Item = TiffResult<(Tag, ifd::Value)>> + '_ {
match self.image().ifd.as_ref() {
None => Either::Left(std::iter::empty()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know when this case happens? Wondering if we should return None or an error rather than an empty iterator

Copy link
Author

Choose a reason for hiding this comment

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

I have no idea, honestly. I think from a developer perspective, it is more convenient to have an empty iterator instead of having to deal with an Option.

.tag_iter()
.filter_map(Result::ok)
.collect::<Vec<_>>();
all_tags.sort_by(|(t1, _), (t2, _)| t1.to_u16().cmp(&t2.to_u16()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could probably use sort_by_key here

Copy link
Author

Choose a reason for hiding this comment

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

Agree.

pub fn tag_iter(&mut self) -> impl Iterator<Item = TiffResult<(Tag, ifd::Value)>> + '_ {
match self.image().ifd.as_ref() {
None => Either::Left(std::iter::empty()),
Some(ifd) => Either::Right(TagIter::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than constructing a TagIter here, I think we could directly return:

ifd.clone().into_iter().map(...)

Copy link
Author

Choose a reason for hiding this comment

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

I tried but failed to make the borrow checker happy with this approach. Happy to change it if you can provide a working solution.

@gschulze
Copy link
Author

I noticed tests are failing on CI. However, they run fine on my local machine (MacBook Pro M1). Maybe something is architecture/platform-dependent in the decoder @fintelia?

@fintelia
Copy link
Contributor

fintelia commented Nov 21, 2024

The CI works by merging the branch into main and testing the resulting version. You might have to merge the lest changes from main locally to get the same results?

Edit: This is what's happening. Your branch is three commits behind:
image

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.

Provide a way to retrieve all tags defined within a file
2 participants