Fix for problems with input and output File Uri with "content" schema and Android 10 SAF new requirements.

fabio-blanco commented 3 years ago

List of changes:

Fix for problems with input and output File Uri with "content" schema.
Fix for using uCrop with SAF (Storage Access Framework) on Android 10 and newer.
Sample app changed for context file testing. Added a "choose file destination" button and some other associated controls.

Important note:

This is a fix for the non-native version since I don't even know if it is possible to deal with those things in native code.

Related and fixed issues:

This fix problems reported on issues #668, #718 and #666.

Explanation of the solution:

Basically I've adapted many of the existing code to rely more on the Uri whenever it is possible rather than using only the file paths.

The most significant changes were made on BitmapCropTask and ImageHeaderParser classes but that were made some minor changes on BitmapLoadTask too.

I had to change the way the exif info was been copied from the input file to the output file and because those were some delicate changes I tried to do this in a more conservative way allowing the behavior to change more for the newer Android versions were it was needed more and conserving the behavior where it wasn't necessary to change (e.g. when the output Uri hasn't the "content" schema).

How to test the changes:

In order to more easily be able to test those things I made some changes to the sample application by inserting in it some widgets to allow for easy testing of different types of output uri.

Seleção_123

A destination Uri path can be easily selected by using the "Choose destination" controls:

Seleção_124

A "Destination File" dialog was provided for Devices with Android versions that don't have the possibility to use a Document Provider file picker for choosing the destination file path:

Seleção_125

To test Uri with "file" or "content" is pretty straightforward:

Seleção_126

Seleção_127

I've made my tests varying content and file schema booth for input Uri and output Uri on those Android api: 16, 19, 22, 28, 29 and 30.

Limitations of the solution:

For Android API lower than Lollipop (21) with a output Uri image with schema "content" it is not possible to preserve the exif info due to ExifInterface limitations explained more deeply on this StackOverflow question. The file is successfully croped but without any exif info.

Pull request late fixes:

Typo on "Choose File Destination" button from Sample app
NullPointerException protection on weak reference of context get on crop method.
Refactorings on BitmapCropTask for improving readability, protections against null pointer exceptions and minor performance improvements
IllegalArgumentException in the copyFile method to inform that an unsupported URI was used.
Merge branch 'master-non-native' to bring commits from fix/non_native_ssl_handshake_exception branch

fabio-blanco commented 3 years ago

@warko-san You were the last one to commit a merge of a PR into the sources of this repository. Was this project abandoned by Yalantis folks? Should I wait for this PR to be properly reviewed by Yalantis folks?

codespair commented 3 years ago

This is an important fix, please review and merge it. Gr8 work @fabio-blanco

codespair commented 3 years ago

I have created a simple working example with your fix here: https://github.com/codespair/CameraImageUCropTutorial if changed from your fix to a release (in the gradle module file) the example breaks, with your fix it works.

fabio-blanco commented 3 years ago

I have created a simple working example with your fix here: https://github.com/codespair/CameraImageUCropTutorial if changed from your fix to a release (in the gradle module file) the example breaks, with your fix it works.

Good! It is nice to know that this work is been useful for someone! I'm still waiting for someone with write permissions in this repository to review and possibly merge my contribution. But till now, it seems that this repository has been abandoned. If this proves to be true, I will release a version as a forked project. Let's wait. If they accept my contribution, I can continue contributing in the future, otherwise I will continue with it in the way that I can in a different project but with backwards compatibility.

DeMoss15 commented 3 years ago

There is an issue on pre-Lollipop devices, that needs some investigation.

Currently in the sample app for random image used outdated url

https://unsplash.it/%d/%d/?random

, which leads to SSLHandshakeException (will be resolved with this PR). I've replaced unsplash.it link with

https://picsum.photos/%d/%d

to test this PR, and it led me to the next exception:

E/TransformImageView: onFailure: setImageUri
    java.lang.IllegalArgumentException: Bounds for bitmap could not be retrieved from the Uri: [file:///data/data/com.yalantis.ucrop.sample/cache/SampleCropImage.jpg]
        at com.yalantis.ucrop.task.BitmapLoadTask.doInBackground(BitmapLoadTask.java:110)
        at com.yalantis.ucrop.task.BitmapLoadTask.doInBackground(BitmapLoadTask.java:35)
        at android.os.AsyncTask$2.call(AsyncTask.java:288)
        at java.util.concurrent.FutureTask.run(FutureTask.java:237)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
        at java.lang.Thread.run(Thread.java:841)

I need time to investigate this issue.

UPD: seems it's ok, just typo in url. strange case.

fabio-blanco commented 3 years ago

@DeMoss15 I also have made the merge with branch 'master-non-native' to bring commits from fix/non_native_ssl_handshake_exception branch in order to easy regression tests and the merge of the PR

fabio-blanco commented 3 years ago

Very good, @DeMoss15 ! Is there any plans for a release? I'm particularly interested in a release with this PR and with #576 from the develop branch.

DeMoss15 commented 3 years ago

@fabio-blanco of course, your changes will be available in next non-native release (we'll try to rollout it today). but about translation I'm not sure. Because of strange format and not full translation for brazil in the PR.

fabio-blanco commented 3 years ago

@fabio-blanco of course, your changes will be available in next non-native release (we'll try to rollout it today).

Very good!

but about translation I'm not sure. Because of strange format and not full translation for brazil in the PR.

What is strange about the format? What is missing in the translation? If there is something missing I can fix it.

I think you should not avoid to release a translation just because one or other key is missing. It won't break the application. If a key is not translated de default is selected. The more translations/languages the lib suports, more users it will have.

Yalantis / uCrop