[IMAGING-105] Add methods for editing EXIF-value for rotation for jpeg-files #283
Conversation
Codecov Report
@@ Coverage Diff @@
## master #283 +/- ##
============================================
+ Coverage 70.74% 70.78% +0.03%
- Complexity 3369 3381 +12
============================================
Files 332 333 +1
Lines 16942 17001 +59
Branches 2647 2655 +8
============================================
+ Hits 11986 12034 +48
- Misses 3910 3918 +8
- Partials 1046 1049 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
@snumlautoken there are over thirty commits in this PR. I think something went wrong when you created the branch or committed your changes??
|
@kinow |
Interesting!
Got it.
Yes, please it is alright to have multiple commits as long as it is intentional and with a good reason.but otherwise squashed it is. Thanks! |
|
Should we send in a new, squashed, PR? |
We can use this PR. You should be able to squash the commits in this PR, then |
Co-authored-by: YavizGuldalf <skeletrox1@hotmail.com> Co-authored-by: Hasti Mohebali Zadeh <Hastimazadeh@gmail.com> Co-authored-by: chenyi <whale0827@icloud.com> Co-authored-by: regusta <rebeckagustavsson1@yahoo.se>
snumlautoken
force-pushed
the
IMAGING-105
branch
from
ff4045d
to
0ab16f7
Compare
|
Maybe not the ideal way to add co-authors, but this will hopefully work |
If you would like to credit others, we can add it to the |
|
That was the way I added them, so it should be fine as it is! |
|
Re-requesting review as the change is still requested |
|
No misunderstanding. Just busy, sorry. Thanks for checking though 👍 |
There was a problem hiding this comment.
The code is in excellent condition, @snumlautoken ! I added some comments about Javadoc and other things related to the current code style. Also some comments/questions about some design choices for the ExifOrientationRewriter. Sorry for the delay 👋
-Bruno
| import org.apache.commons.imaging.formats.tiff.write.TiffOutputField; | ||
| import org.apache.commons.imaging.formats.tiff.write.TiffOutputSet; | ||
|
|
||
| public class ExifOrientationRewriter { |
There was a problem hiding this comment.
Missing javadoc with @since tag (see other classes).
|
|
||
| public class ExifOrientationRewriter { | ||
|
|
||
| public enum Orientation { |
There was a problem hiding this comment.
At least everything public must have a Javadoc 👇 (tests should be fine as we don't have that in other tests, unless you have time to add it too).
| * @return Orientation enum | ||
| * @throws IOException | ||
| * @throws ImageReadException | ||
| * @throws ImageWriteException |
There was a problem hiding this comment.
These also need a comment explaining the exceptions.
| /** | ||
| * A method that sets a new value to the orientation field in the EXIF metadata of a JPEG file. | ||
| * @param orientation the value as a enum of the direction to set as the new EXIF orientation | ||
| * |
| } | ||
|
|
||
| /** | ||
| * Writes Bytesource to file with given path |
There was a problem hiding this comment.
Extra space before Writes, and s/Bytesource/ByteSource
| * @param path String of the path in which the file is written | ||
| * @throws IOException | ||
| */ | ||
| public void getOutput(String path) |
| } | ||
|
|
||
| /*** | ||
| * Get the orientation (enum) of the current image |
There was a problem hiding this comment.
Needs a dot. The other sentence can come directly after this one, or leave it in the next line (or if you intended to have two lines, use HTML to add a break line, new paragraph, etc.).
|
|
||
| if (metadata == null) { | ||
| return Orientation.HORIZONTAL; | ||
| } |
There was a problem hiding this comment.
Isn't that dangerous to return horizontal? Wouldn't it be better to return null instead? This way you have the three valid states, 1) data has horizontal orientation, 2) data has vertical orientation, or 3) no metadata about orientation.
| JpegImageMetadata metadata = (JpegImageMetadata) Imaging.getMetadata(this.fileSrc.getAll()); | ||
|
|
||
| if (metadata == null) { | ||
| metadata = new JpegImageMetadata(null, new TiffImageMetadata(new TiffContents(new TiffHeader(ByteOrder.BIG_ENDIAN, 0, 0), new ArrayList<>(), new ArrayList<>()))); |
There was a problem hiding this comment.
I think ExifRewriter allows big and little endian. But it looks like ExifOrientationRewriter uses big endian if no metadata found? Wouldn't be better if we had the same behavior as in ExifRewriter? Letting the user choose the endianess.
There was a problem hiding this comment.
s/new ArrayList()/Collections.emptyList()
That should create that empty EmptyList instead of an ArrayList, and I think further down the pipe the new ArrayList will become an unmodifiable collection, so that should be fine.
There was a problem hiding this comment.
Did you try to design the EXIF rotation as part of ExifRewriter, @snumlautoken ? What was the rationale for creating a new class instead? I guess that could mean that for other metadata rewriting/editing we would need other Exif$MetadataRewriter? Ditto for other metadata/formats? That sounds like it could become problematic in the long run, I think? WDYT?
|
@snumlautoken I share your opinion about the importance about the orientation flag. That’s why I recently wrote a logic dedicated to finding and updating that flag. You may want to rewrite https://github.com/Ashampoo/kim/blob/main/src/commonMain/kotlin/com/ashampoo/kim/format/jpeg/JpegOrientationOffsetFinder.kt in Java and contribute this. :) |
This PR is for an issue whose 10th anniversary was a week ago, so might not be the most important issue.
Nevertheless, we feel as though, among metadata operations, rotation is probably the one most commonly used, and thus most deserving of a more streamlined process.
We also feel that our contribution could serve as a base for other streamlinings of exif-data manipulation, such as for GPS-data.