-
Notifications
You must be signed in to change notification settings - Fork 483
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
Fujifilm enhancements #619
Conversation
- This commit adds the ability to retrieve the image actuation for Fuji cameras - Also completes the list of Film simulations - Also makes the method getMakerNoteVersionDescription public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one question/comment.
@Nullable | ||
public int getImageNumber() | ||
{ | ||
final Integer value = _directory.getInteger(TAG_IMAGE_NUMBER); | ||
return value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Methods on the *Descriptor
classes generally return a String
description. This method feels inconsistent with the rest of the API here. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a benefit of returning String descriptions exclusively? If it's a matter of feel, aesthetics, style, then I'd get that and it's up to you, really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Descriptor
classes are supposed to return presentable string versions. A method that gets a tag's raw value would be a better fit on the Directory
class, following the design that exists elsewhere here.
Apart from moving to the Directory
, this method will throw a NullPointerException
if the tag's not present in the image, from the implicit conversion from Integer
to int
. The @Nullable
attribute doesn't apply to int
either.
I think I would vote to remove this method. We generally don't have methods for fields like this anywhere else in the API apart from where they have some complex logic such as converting from an encoded representation into some other form that's useful (and not just a string description). In this case it's no different than any other integer tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the case. You're correct on all counts.
Fully agreed BTW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition, thanks!
This ports over the improvements made by @danielsz in drewnoakes/metadata-extractor#619
This commit brings the following:
Nice work! Thank you!