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

[IMAGING-105] Add methods for editing EXIF-value for rotation for jpeg-files #283

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

Conversation

snumlautoken
Copy link
Contributor

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Attention: Patch coverage is 72.88136% with 16 lines in your changes missing coverage. Please review.

Project coverage is 70.78%. Comparing base (ff13fa4) to head (0ab16f7).
Report is 858 commits behind head on master.

Files with missing lines Patch % Lines
...ing/formats/jpeg/exif/ExifOrientationRewriter.java 72.88% 13 Missing and 3 partials ⚠️
Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

@snumlautoken there are over thirty commits in this PR. I think something went wrong when you created the branch or committed your changes??

@snumlautoken
Copy link
Contributor Author

snumlautoken commented Mar 7, 2023

@kinow
We have been working as a group (school project), and have thus been aiming to work in a very fractured manner, with many small commits. So no error on that front.
But a squash may have been appropriate...

@kinow
Copy link
Member

kinow commented Mar 7, 2023

We have been working as a group (school project)

Interesting!

and have thus been aiming to work in a very fractured manner, with many small commits. So no error on that front.

Got it.

But a squash may have been appropriate...

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!

@snumlautoken
Copy link
Contributor Author

Should we send in a new, squashed, PR?
Or just let it be?

@kinow
Copy link
Member

kinow commented Mar 7, 2023

Should we send in a new, squashed, PR? Or just let it be?

We can use this PR. You should be able to squash the commits in this PR, then push -f to your branch.

Co-authored-by: YavizGuldalf <[email protected]>
Co-authored-by: Hasti Mohebali Zadeh <[email protected]>
Co-authored-by: chenyi <[email protected]>
Co-authored-by: regusta <[email protected]>
@snumlautoken
Copy link
Contributor Author

Maybe not the ideal way to add co-authors, but this will hopefully work

@kinow
Copy link
Member

kinow commented Mar 7, 2023

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 src/changes/changes.xml. That's where we record the entry for the issue (IMAGING-105). Alternatively, you can add multiple authors to a commit too - e.g. https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors (feel free to edit your commit and push -f to this branch as you did, until you are happy with the code 👍 )

@snumlautoken
Copy link
Contributor Author

e.g. https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

That was the way I added them, so it should be fine as it is!

@snumlautoken snumlautoken requested a review from kinow March 9, 2023 10:56
@snumlautoken
Copy link
Contributor Author

snumlautoken commented Mar 9, 2023

Re-requesting review as the change is still requested
Just making sure that a misunderstanding has not occured

@kinow
Copy link
Member

kinow commented Mar 9, 2023

No misunderstanding. Just busy, sorry. Thanks for checking though 👍

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

Missing javadoc with @since tag (see other classes).


public class ExifOrientationRewriter {

public enum Orientation {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
*
Copy link
Member

Choose a reason for hiding this comment

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

Missing @throws here...

}

/**
* Writes Bytesource to file with given path
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is never tested?

}

/***
* Get the orientation (enum) of the current image
Copy link
Member

Choose a reason for hiding this comment

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

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;
}
Copy link
Member

Choose a reason for hiding this comment

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

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<>())));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

@StefanOltmann
Copy link
Contributor

@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. :)

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.

4 participants