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

Ignore exceptions trying to read IPTC-IIM data #344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StephenMcConnel
Copy link

Having TagLibSharp just throw an exception when it can't read an IPTC-IIM segment is causing problems for some of our users.
This is the least important of the metadata segments, generally speaking, and the TagLibSharp code only pulls a handful of values
from it in any case. Our bug report at https://issues.bloomlibrary.org/youtrack/issue/BL-11933 has a jpeg image that shows
the problem. I can get around not being able to use the image at all by catching the exception in one of our libraries that
uses TagLibSharp, but it prevents other metadata from the file from being available, which causes some problems with suboptimal handling of the image file.

@StephenMcConnel
Copy link
Author

StephenMcConnel commented Jul 30, 2024

I've added a unit test after noticing this requirement in the README.
I'd really appreciate having a yes or no response on this PR. I'd understand a "no" response, but being left in limbo is not good.

@decriptor
Copy link
Collaborator

@StephenMcConnel Sorry, for the delay. There are some complications I'm trying to sort out, but will try to be more on top of things in the meantime.

using System.Threading.Tasks;
using TaglibSharp.Tests.Images.Validators;
using TagLib.IFD;
using TagLib.Xmp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you sort these with System at the top. Also, there are a couple duplicates in this list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you can fix this, I'm good with the changes and will merge them.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, none of the System using statements were needed according to VS 2022, so I removed them along with the other redundant or otherwise unnecessary using statements.

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.

3 participants