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

[WIP] Crop Image before upload #2542

Closed
wants to merge 28 commits into from

Conversation

vanshikaarora
Copy link
Collaborator

**Added crop image activity after images are selected for upload **

Fixes #1192 Option to edit the image within the app

Progress till now

  • Added the cropping library https://github.com/ArthurHub/Android-Image-Cropper
  • Implemented the feature in case of single image
  • Update Uri of uploaded item in case of image cropped
  • Created a grid view activity in case of editing multiple images
  • CropImage activity also implemented for multiple images

Change's yet to be made

  • A better UI for Image list in case of multiple images
  • Effective way for retrieving Uri of each cropped image in case of multiple images
  • Updating the UploadItem object with the Uri of cropped image effectively so as not to hamper the functioning of other code (in case of multiple images)

Screenshot for edit icon in Upload Activity:

whatsapp image 2019-03-05 at 10 13 23 pm

Crop Image

whatsapp image 2019-03-05 at 10 13 23 pm 1

@codecov-io
Copy link

codecov-io commented Mar 5, 2019

Codecov Report

Merging #2542 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2542      +/-   ##
=========================================
- Coverage     2.7%   2.68%   -0.02%     
=========================================
  Files         258     259       +1     
  Lines       12283   12356      +73     
  Branches     1113    1121       +8     
=========================================
  Hits          332     332              
- Misses      11925   11998      +73     
  Partials       26      26
Impacted Files Coverage Δ
...ava/fr/free/nrw/commons/upload/UploadActivity.java 0% <0%> (ø) ⬆️
...fr/free/nrw/commons/filepicker/UploadableFile.java 1.47% <0%> (-0.15%) ⬇️
.../java/fr/free/nrw/commons/upload/ImageAdapter.java 0% <0%> (ø)
...n/java/fr/free/nrw/commons/upload/UploadModel.java 31.14% <0%> (-0.7%) ⬇️
...va/fr/free/nrw/commons/upload/UploadPresenter.java 10.05% <0%> (-0.88%) ⬇️

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 44c18ac...ec42b3f. Read the comment docs.

@vanshikaarora
Copy link
Collaborator Author

@maskaravivek @nicolas-raoul @ujjwalagrawal17 Can you please review the current changes :)

Once these changes seem satisfactory I'll continue the work ahead to complete this PR :)

@nicolas-raoul
Copy link
Member

Looks good so far :-)

@vanshikaarora
Copy link
Collaborator Author

@nicolas-raoul I have updated the PR for multiple images. Here is a report of the progress so far.

Work Completed

  • Cropping image for single upload
  • Directing to a new activity (to display list of images) in case of multiple upload
  • Selecting images for crop and Cropping activity

Work yet to be completed

  • Improve UI of Image GridView
  • Finalise the position of Edit Icon in upload activity

Screenshots showing the change

WhatsApp Image 2019-03-11 at 5 56 13 PM (2)
WhatsApp Image 2019-03-11 at 5 56 13 PM (1)

@maskaravivek
Copy link
Member

Crop works perfectly for me.

@vanshikaarora Was there a discussion about putting the edit icon in the bottom sheet view? I thought initially we agreed about putting it in the middle view which contains a map icon.

@nicolas-raoul Should the cropped image contain the original EXIF?

@nicolas-raoul
Copy link
Member

I think the cropped image should contain the original EXIF, yes.

@nicolas-raoul
Copy link
Member

I don't think there was a discussion about it, but personally I love the edit icon in the bottom sheet view :-)
The right side map component could be put there too, in order to hide the image less.

@maskaravivek
Copy link
Member

maskaravivek commented Mar 15, 2019

I think the cropped image should contain the original EXIF, yes.

@vanshikaarora Can you check if original EXIF is being persisted.

You can either use any EXIF viewer android app.

I don't think there was a discussion about it, but personally I love the edit icon in the bottom sheet view :-)

Okay cool.

The right side map component could be put there too, in order to hide the image less.

This can be picked as a separate issue.

@vanshikaarora
Copy link
Collaborator Author

Can you check if original EXIF is being persisted.

I'll do that 👍

This can be picked as a separate issue.

@maskaravivek @nicolas-raoul Shall I create a separate issue for that?

@nicolas-raoul
Copy link
Member

Shall I create a separate issue for that?

Yes thanks no need to ask :-)

@vanshikaarora
Copy link
Collaborator Author

@vanshikaarora Can you check if original EXIF is being persisted.

@maskaravivek @nicolas-raoul I just verified the code and found that the EXIF data isn't persisted in the image.

Since we need to check for EXIF we can call the function validate image the very first time image was uploaded.
Shall I update the code this way or you have other opinion in mind. Please share your opinion :)

@nicolas-raoul
Copy link
Member

The EXIF is important for the future after the image, even years after it has been uploaded.
This might give some hints: https://stackoverflow.com/questions/8972357/manipulate-an-image-without-deleting-its-exif-data

vanshikaarora added a commit to vanshikaarora/apps-android-commons that referenced this pull request Mar 15, 2019
@vanshikaarora
Copy link
Collaborator Author

The EXIF is important for the future after the image, even years after it has been uploaded.
This might give some hints: https://stackoverflow.com/questions/8972357/manipulate-an-image-without-deleting-its-exif-data

@nicolas-raoul I have gone through the code I think it would be better to add the functions inside the ReadEXIF.java class. It would be good if #2581 is merged before I work upon exif changes here. Can you please review that PR :)

@vanshikaarora
Copy link
Collaborator Author

@nicolas-raoul Also for adding this feature i need to add a new library org.apache.commons.imaging. Do you have any other alternative or should I proceed forward with this?

maskaravivek pushed a commit that referenced this pull request Mar 17, 2019
* resolved issue #2542

* removed commented code
@domdomegg domdomegg changed the title Crop Image before upload [WIP] [WIP] Crop Image before upload Mar 17, 2019
@vanshikaarora
Copy link
Collaborator Author

@nicolas-raoul the first solution uses ImageIO which is not a part of the java standard library :(

Shall I add the ImageIO library to the project?

@nicolas-raoul
Copy link
Member

Isn't it this standard Java library?
https://docs.oracle.com/javase/7/docs/api/javax/imageio/ImageIO.html

@vanshikaarora
Copy link
Collaborator Author

Isn't it this standard Java library?
https://docs.oracle.com/javase/7/docs/api/javax/imageio/ImageIO.html

No, I can't import this.
Also the answer at stack overflow https://stackoverflow.com/a/16813721/10595971

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Mar 18, 2019 via email

@vanshikaarora
Copy link
Collaborator Author

@nicolas-raoul For either of the solutions we need to download the jar file. So we can continue with the second solution

@vanshikaarora
Copy link
Collaborator Author

Hey @maskaravivek this PR works perfectly for me. However travis-ci fails because of migration to androidx and android.enableJetifier=true do you have any solution for this?

@vanshikaarora
Copy link
Collaborator Author

Hey @maskaravivek @nicolas-raoul, the present Issues with this PR is

android.useAndroidX=true
android.enableJetifier=true

in gradle.properties

I tried to blacklist it using the comment 9 here https://issuetracker.google.com/issues/119135578#comment9
But that couldn't resolve it

WARNING: The option setting 'android.jetifier.blacklist=commons-imaging-1.0-R1401825.jar' is experimental and unsupported.

Can you please look into this?

@neslihanturan
Copy link
Collaborator

Seems like we are a lot late to merge this perfectly working and quite complex pull request. This requires somonel to fix te conflix and should be retested again. But overall it is very ready to be merged.

@madhurgupta10
Copy link
Collaborator

@vanshikaarora Let us know if you are still working on this.

@neslihanturan
Copy link
Collaborator

Closing for now, please feel free to reopen :)

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.

Transform picture (crop/rotate/etc) within the app
6 participants