-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[WIP] Crop Image before upload #2542
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@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 :) |
Looks good so far :-) |
@nicolas-raoul I have updated the PR for multiple images. Here is a report of the progress so far. Work Completed
Work yet to be completed
Screenshots showing the change |
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? |
I think the cropped image should contain the original EXIF, yes. |
I don't think there was a discussion about it, but personally I love the edit icon in the bottom sheet view :-) |
@vanshikaarora Can you check if original EXIF is being persisted. You can either use any EXIF viewer android app.
Okay cool.
This can be picked as a separate issue. |
I'll do that 👍
@maskaravivek @nicolas-raoul Shall I create a separate issue for that? |
Yes thanks no need to ask :-) |
@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. |
The EXIF is important for the future after the image, even years after it has been uploaded. |
@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 :) |
@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? |
* resolved issue #2542 * removed commented code
ec42b3f
to
72c7d11
Compare
@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? |
Isn't it this standard Java library? |
No, I can't import this. |
Oh :-/
I guess we have no choice but to use the second solution then?
The JAR is 690KB.
|
@nicolas-raoul For either of the solutions we need to download the jar file. So we can continue with the second solution |
Hey @maskaravivek this PR works perfectly for me. However travis-ci fails because of migration to androidx and |
Hey @maskaravivek @nicolas-raoul, the present Issues with this PR is
in gradle.properties I tried to blacklist it using the comment 9 here https://issuetracker.google.com/issues/119135578#comment9
Can you please look into this? |
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. |
@vanshikaarora Let us know if you are still working on this. |
Closing for now, please feel free to reopen :) |
**Added crop image activity after images are selected for upload **
Fixes #1192 Option to edit the image within the app
Progress till now
Change's yet to be made
Screenshot for edit icon in Upload Activity:
Crop Image