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

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

Conversation

fabio-blanco
Copy link

@fabio-blanco fabio-blanco commented Jan 17, 2021

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

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.
@fabio-blanco
Copy link
Author

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

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

@codespair
Copy link

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

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

DeMoss15 commented May 20, 2021

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.

…against null pointer exceptions and minor performance improvements
…-destination-file

# Conflicts:
#	ucrop/src/main/java/com/yalantis/ucrop/task/BitmapLoadTask.java
@fabio-blanco
Copy link
Author

fabio-blanco commented May 24, 2021

@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

@DeMoss15 DeMoss15 merged commit 0ff3e6a into Yalantis:master-non-native May 25, 2021
@fabio-blanco
Copy link
Author

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

@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
Copy link
Author

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

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.

None yet

4 participants