Added metadata reader for reading exif data #2581

vanshikaarora · 2019-03-13T09:38:01Z

Reading EXIF Directories Exif IFD0, Exif SubIFD, Exif Thumbnail if it does not contains any of these disaplay a warning to the user

Fixes #97 Avoid copyright violations

Changes Made

Added a class(ReadExif.java) to read exif data
Modifies ImageUtils.java
Modified ImageProcessingService.java

…commons into no_exif

vanshikaarora · 2019-03-17T05:00:47Z

@maskaravivek @nicolas-raoul Can you please review this PR. It will help in working out with #2542

nicolas-raoul · 2019-03-18T07:38:11Z

app/src/main/java/fr/free/nrw/commons/upload/ReadEXIF.java

+import timber.log.Timber;
+
+@Singleton
+public class ReadEXIF {


Any particular reason for using a verb rather than a noun (which is the convention for Java classes)?
I would suggest EXIFReader.

nicolas-raoul · 2019-03-18T07:38:16Z

app/src/main/java/fr/free/nrw/commons/upload/ReadEXIF.java

+
+    }
+
+    public Single<Integer> processMetadata(String path) throws IOException {


vanshikaarora · 2019-03-18T09:24:19Z

@nicolas-raoul I have made the changes. Kindly review :)

codecov-io · 2019-03-18T09:40:46Z

Codecov Report

Merging #2581 into master will increase coverage by <.01%.
The diff coverage is 28%.

@@            Coverage Diff            @@
##           master   #2581      +/-   ##
=========================================
+ Coverage    2.77%   2.77%   +<.01%     
=========================================
  Files         259     261       +2     
  Lines       12373   12451      +78     
  Branches     1117    1130      +13     
=========================================
+ Hits          343     346       +3     
- Misses      12003   12078      +75     
  Partials       27      27

Impacted Files	Coverage Δ
...ain/java/fr/free/nrw/commons/utils/ImageUtils.java	`0% <ø> (ø)`	⬆️
...in/java/fr/free/nrw/commons/upload/EXIFReader.java	`0% <0%> (ø)`
...ree/nrw/commons/upload/ImageProcessingService.java	`85.48% <100%> (+0.73%)`	⬆️
...n/java/fr/free/nrw/commons/CommonsApplication.java	`0% <0%> (ø)`	⬆️
...e/nrw/commons/category/CategoryImagesActivity.java	`0% <0%> (ø)`	⬆️
...rw/commons/mwapi/ApacheHttpClientMediaWikiApi.java	`0% <0%> (ø)`	⬆️
...r/free/nrw/commons/contributions/MainActivity.java	`0% <0%> (ø)`	⬆️
...java/fr/free/nrw/commons/upload/UploadService.java	`0% <0%> (ø)`	⬆️
...ava/fr/free/nrw/commons/upload/UploadActivity.java	`0% <0%> (ø)`	⬆️
...rw/commons/explore/categories/ExploreActivity.java	`0% <0%> (ø)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 341f961...1c509d6. Read the comment docs.

nicolas-raoul

A few minor things :-)

nicolas-raoul · 2019-03-18T10:58:00Z

app/src/main/java/fr/free/nrw/commons/upload/EXIFReader.java

+
+    }
+
+    public Single<Integer> processMetadata(String path) throws IOException {


I have added the doc for this just above the class declaration. Shall I put it above the function

Both the class and the method need javadoc. At least say briefly what the method does, and what are the inputs and outputs of the method.

nicolas-raoul · 2019-03-18T10:59:14Z

app/src/main/java/fr/free/nrw/commons/upload/EXIFReader.java

+public class EXIFReader {
+    @Inject
+    public EXIFReader() {
+


A common practice is to write // Empty in empty methods so that readers know it is not a mistake :-)

nicolas-raoul · 2019-03-18T11:00:47Z

app/src/main/java/fr/free/nrw/commons/upload/EXIFReader.java

+        try {
+            readMetadata = ImageMetadataReader.readMetadata(new File(path));
+        } catch (ImageProcessingException e) {
+            e.printStackTrace();


How about logging the error to logcat? https://stackoverflow.com/a/8217563/226958

With Timber I guess.

nicolas-raoul · 2019-03-18T11:03:21Z

app/src/main/java/fr/free/nrw/commons/upload/EXIFReader.java

+        }
+        if (readMetadata != null) {
+            for (Directory directory : readMetadata.getDirectories()) {
+                if (directory.getName().equals("Exif IFD0") || directory.getName().equals("Exif SubIFD") || directory.getName().equals("Exif Thumbnail")) {


Please put each equals on its own line, and add at the end of the line a comment explaining where this kind of directory is usually found.
Otherwise future readers will have a very hard time knowing what it means.
Thanks! :-)

nicolas-raoul · 2019-03-18T11:05:40Z

app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java

-        });
+        return Single.zip(zipResult,checkFBMD,checkEXIF, (zip,fbmd,exif)->{
+            Timber.d("zip:"+zip+"fbmd:"+fbmd+"exif:"+exif);
+            return zip| fbmd| exif;


Better put a space before |.
For this kind of conventions a good read is https://www.oracle.com/technetwork/java/codeconventions-150003.pdf

Sure thanks for the resource @nicolas-raoul :)

vanshikaarora · 2019-03-18T13:01:02Z

@nicolas-raoul I have made the changes. Kindly review :)

nicolas-raoul · 2019-03-19T02:39:09Z

app/src/main/java/fr/free/nrw/commons/upload/EXIFReader.java

+                // In case of internet downloaded image these three fields are not present
+                if (directory.getName().equals("Exif IFD0") //Contains information about the device capturing the photo
+                        || directory.getName().equals("Exif SubIFD") //contains information like date, time and pixels of the image
+                        || directory.getName().equals("Exif Thumbnail")) //contains information about Image THumbnail like compression and reolution


about Image THumbnail like compression and reolution -> about image thumbnails like compression and resolution

nicolas-raoul · 2019-03-19T02:39:46Z

app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java

-            Timber.d("zip:" + zip + "fbmd:" + fbmd);
-            return zip | fbmd;
-        });
+        return Single.zip(zipResult,checkFBMD,checkEXIF, (zip,fbmd,exif)->{


space after each comma

nicolas-raoul · 2019-03-19T02:39:59Z

app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java

-            return zip | fbmd;
-        });
+        return Single.zip(zipResult,checkFBMD,checkEXIF, (zip,fbmd,exif)->{
+            Timber.d("zip:"+zip+"fbmd:"+fbmd+"exif:"+exif);


space before and after each +

nicolas-raoul

Looks great!

vanshikaarora · 2019-03-19T13:44:03Z

Thanks for approving the PR @nicolas-raoul @neslihanturan @ujjwalagrawal17

vanshikaarora added 5 commits March 12, 2019 19:26

initial commit

03e6af1

Merge branch 'master' of https://github.com/commons-app/apps-android-…

c455cc0

…commons into no_exif

initial commit

543bdfb

reading 3 exif directories

21eabbd

minor changes

eeb175b

vanshikaarora mentioned this pull request Mar 13, 2019

Avoid copyright violations #97

Closed

fixed unit tests

74ab4ee

vanshikaarora mentioned this pull request Mar 15, 2019

[WIP] Crop Image before upload #2542

Closed

nicolas-raoul requested changes Mar 18, 2019

View reviewed changes

vanshikaarora added 2 commits March 18, 2019 14:48

reviwied requested changes

956499a

resolved merge conflicts

ae926f9

nicolas-raoul requested changes Mar 18, 2019

View reviewed changes

minor changes

40831f6

nicolas-raoul reviewed Mar 19, 2019

View reviewed changes

nicolas-raoul approved these changes Mar 19, 2019

View reviewed changes

minor changes

1c509d6

neslihanturan approved these changes Mar 19, 2019

View reviewed changes

ujjwalagrawal17 self-requested a review March 19, 2019 13:32

ujjwalagrawal17 approved these changes Mar 19, 2019

View reviewed changes

ujjwalagrawal17 merged commit 3417d71 into commons-app:master Mar 19, 2019

This was referenced Mar 21, 2019

Potential problems with this image : No problem #2692

Closed

Picture thought to be downloaded from the Internet even though it is not #2704

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added metadata reader for reading exif data #2581

Added metadata reader for reading exif data #2581

vanshikaarora commented Mar 13, 2019

vanshikaarora commented Mar 17, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

vanshikaarora commented Mar 18, 2019

codecov-io commented Mar 18, 2019 •

edited

Loading

nicolas-raoul left a comment

nicolas-raoul Mar 18, 2019

vanshikaarora Mar 18, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

nicolas-raoul Mar 18, 2019

vanshikaarora Mar 18, 2019

vanshikaarora commented Mar 18, 2019

nicolas-raoul Mar 19, 2019

nicolas-raoul Mar 19, 2019

nicolas-raoul Mar 19, 2019

nicolas-raoul left a comment

vanshikaarora commented Mar 19, 2019


		}

		public Single<Integer> processMetadata(String path) throws IOException {

Added metadata reader for reading exif data #2581

Added metadata reader for reading exif data #2581

Conversation

vanshikaarora commented Mar 13, 2019

Changes Made

vanshikaarora commented Mar 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanshikaarora commented Mar 18, 2019

codecov-io commented Mar 18, 2019 • edited Loading

Codecov Report

nicolas-raoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanshikaarora commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-raoul left a comment

Choose a reason for hiding this comment

vanshikaarora commented Mar 19, 2019

codecov-io commented Mar 18, 2019 •

edited

Loading