add ActivityResultContracts #145

connyduck · 2021-06-16T19:43:48Z

close #26

Description:

This adds ActivityResultContracts to make opening the file picker and the image cropper nice after onActivityResult has been deprecated

Check list for the Code Reviewer:

CHANGELOG
README
Wiki
Version Number

Open questions:

Should the old ActivityBuilder be marked @Deprecated?

connyduck · 2021-06-16T19:45:13Z

This is a draft PR so we can discuss if this is the API we want. After we decided on the API we can add tests and cleanup the code/changelog/wiki etc.

Canato

This is some real quality work, thanks! Will help a lot the library an it users. 🎉

About the changes, before we have CropImage that would be responsible for all the calls from the user. Now we have CropImageContract and OpenChooser that make it split.

I would be keen to find a way the users could use the same contract for call the ImageCropper or just the ImagePicker.

Not sure how this would be possible, probably something like:

fun cropImageOptions(
    uri: Uri? = null,
    builder: CropImageContractOptions.() -&gt; (Unit) = {}
): CropImageContractOptions {
    val options = CropImageContractOptions(uri, null, CropImageOptions())
    options.run(builder)
    return options
}

and

fun pickerImageOptions(
    includeCamera: Boolean = true,
): CropImageContractOptions {
    val options = CropImageContractOptions(null, includeCamera, CropImageOptions())
    options.run(builder)
    return options
}

where

data class CropImageContractOptions @JvmOverloads constructor(
     val uri: Uri?,
     val includeCamera: Boolean,
     val options: CropImageOptions,
}

So, if include != null we have picker we have
CropImage.getPickImageChooserIntent(...

Tell me what you think

README.md

cropper/src/main/java/com/canhub/cropper/CropImageContracts.kt

connyduck · 2021-06-17T16:53:47Z

OpenChooser is meant as a replacement for CropImage.getPickImageChooserIntent and that one only has the includeCamera options so I don't think we need a helper function like for the CropImageContractOptions.
If you want to have the camera option configurable when opening the CropImageActivity (currently it defaults to true), that should be a different PR.

Canato · 2021-06-18T13:42:15Z

OpenChooser is meant as a replacement for CropImage.getPickImageChooserIntent and that one only has the includeCamera options so I don't think we need a helper function like for the CropImageContractOptions.
If you want to have the camera option configurable when opening the CropImageActivity (currently it defaults to true), that should be a different PR.

Right now we have just on class with different methods CropImage.something I want to avoid different contracts for the same library, increase the complexity about what to use. My goal here is that we could keep it all in one contract.

connyduck · 2021-06-18T20:06:16Z

Right now we have just on class with different methods CropImage.something I want to avoid different contracts for the same library, increase the complexity about what to use. My goal here is that we could keep it all in one contract.

I disagree, there are two very distinct usecases here

Show an image picker. Get back a uri.
Crop an image. Either provide the image or have the user choose one. Get back a result with information about the crop.

Putting both usecases into one contract makes that contract way more complex, it is easier to comprehend two small contracts.

If you want to keep things simple, we can discuss if the first usecase should even be part of this library. AndroidX provides the GetContent contract which does almost the same (minus the camera option).

Canato · 2021-06-21T08:09:18Z

Right now we have just on class with different methods CropImage.something I want to avoid different contracts for the same library, increase the complexity about what to use. My goal here is that we could keep it all in one contract.

I disagree, there are two very distinct usecases here

Show an image picker. Get back a uri.

Crop an image. Either provide the image or have the user choose one. Get back a result with information about the crop.

Putting both usecases into one contract makes that contract way more complex, it is easier to comprehend two small contracts.

If you want to keep things simple, we can discuss if the first usecase should even be part of this library. AndroidX provides the GetContent contract which does almost the same (minus the camera option).

I like this discussion, maybe we should extract the picker library, so the user could decide what to import in his project.

Anyway, this is no the PR for this.
We have CropImageContract let's use PickerImageContract for consistency. And we can keep both.

We will just need to update readme, docs and wiki later.

Canato

We are getting closer =D

cropper/src/main/java/com/canhub/cropper/CropImage.kt

cropper/src/main/java/com/canhub/cropper/CropImageContract.kt

Canato

We have a great solution.

Now is about update the sample apps and documentation.
If you want access to wiki update let me know or you can point where I need to update something and I will do it.

Congratulations for the work, version 4.0 coming.

Canato

👍 nice so far.

Just to keep track, what next steps are you planning to do?

Maybe we could add some checklist in the description, so we keep the track.

cropper/src/test/java/com/canhub/cropper/PickImageContractTest.kt

connyduck · 2021-06-24T18:32:50Z

I think I am pretty much done.

I would appreciate it if you could update the Wiki yourself @Canato. Basically all occurences of the old methods have to be replaced with the new contracts. And I guess a migration guide would also be nice.

Canato

Some tasks we need to finish before merge:

Need to update SCameraFragment and SCameraFragmentJava to the new Contract and remove the deprecated methods.
Change method onActivityResult in SCameraContract, SCameraContractJava
Need to update CropImage.kt documentation where it say Result will be received in onActivityResult...

Don't worry I will update the wiki!

cropper/src/test/java/com/canhub/cropper/PickImageContractTest.kt

connyduck · 2021-06-25T19:42:19Z

Need to update CropImage.kt documentation where it say Result will be received in onActivityResult...

No, this is still true if you use those methods. I updated the other things.

Canato · 2021-06-26T03:24:19Z

Need to update CropImage.kt documentation where it say Result will be received in onActivityResult...

No, this is still true if you use those methods. I updated the other things.

You are not wrong, but would prefer the documentation and examples/samples always related to the latest changes. So it explain to the user the way the library should be used

connyduck · 2021-06-26T05:55:29Z

You are not wrong, but would prefer the documentation and examples/samples always related to the latest changes. So it explain to the user the way the library should be used

The samples are up to date now, and the old methods are deprecated with an explanation what to use instead. That should be enough.

Canato

Some small points before we can merge.

The most important one is the test and return of UriFilePath since this give so much headache this year. More comments on the classes.

Please on the SCamera changes, apply to the Java code where make sense too please.

Canato · 2021-06-26T16:17:44Z

sample/src/main/java/com/canhub/cropper/sample/camera/presenter/SCameraPresenter.kt

-                    val bitmap = context?.let { CropImage.getActivityResult(data)?.getBitmap(it) }
-                    Log.v(
-                        "File Path",
-                        context
-                            ?.let { CropImage.getActivityResult(data)?.getUriFilePath(it) }
-                            .toString()
-                    )


I know this two don't have direct value for the code, but we need to keep the logic for method testing and user example of how getting the bitmap and UriFilePath (big issues on this latest one)

So we would need:

val bitmap = result.bitmap Log.v("File Path", context?.let { result.getUriFilePath(it) }.toString())

Canato · 2021-06-26T16:21:10Z

sample/src/main/java/com/canhub/cropper/sample/camera/presenter/SCameraPresenter.kt

+
+    override fun onPickImageResult(resultUri: Uri?) {
+        if (resultUri != null) {
+            Log.v("File Path", resultUri.toString())


I just realise we are not returning UriFilePath just UriContent. Will add a comment into PickImageContract class

Canato · 2021-06-26T16:26:50Z

cropper/src/main/java/com/canhub/cropper/PickImageContract.kt

+        if (intent != null) {
+            context?.let {
+                context = null
+                return getPickImageResultUriContent(it, intent)


This method is returning only UriContent missing UriFilePath. We need to return both.

Sorry I should have spotted this before.

To give a little more context. With the Android 10/11 changes UriContent doesn't represent a file path anymore, using the content:// schema for it URI. What is right by definition.
But many users of this library and other ones, got used of using URI as file path for saving the crop image.
So we split UriContent and UriFilePath as user request. In previous versions of Android UriContent can be in the file schema, in recent ones in content schema.
So now we have UriFilePath that always represent a file schema.

Why not put all in this file schema? To save space/memory, since some users doesn't need it.

So what do we do? Have it return a custom class with two uris?

Is better that we avoid the filePath generation, if the user will not use it.
But would be nice to make the method available for usage, so we generate only if the user need.

No idea the best way of doing this =/

We could make PickImageContract an open class so users can override the parseResult method and parse the result differently

Canato · 2021-06-26T16:30:19Z

sample/src/main/java/com/canhub/cropper/sample/camera/app/SCameraFragment.kt

@@ -56,6 +53,15 @@ internal class SCameraFragment :
        ActivityResultContracts.RequestPermission()
    ) { isGranted: Boolean -> presenter.onPermissionResult(isGranted) }

+    private val pickImage =
+        registerForActivityResult(PickImageContract(), presenter::onPickImageResult)


Please, let's avoid to use the reflection ::. I agree is much better in an aesthetics perspective, I tend to avoid reflection in general + some times Android Studio go crazy not finding it usages making harder for debug.

registerForActivityResult(PickImageContract()) { presenter.onPickImageResult(it) }

Canato

Day for celebration!!! 🎉 🎉

Amazing work @connyduck
👏 👏 👏 👏 👏

Will update the Wiki before release, hopefully will do this week

connyduck added 2 commits June 16, 2021 21:37

add ActivityResultContracts for cropping and opening chooser

c8a27e2

Merge branch 'main' into activity_result_contract

1b9f0be

Canato requested changes Jun 17, 2021

View reviewed changes

connyduck added 4 commits June 18, 2021 21:32

deprecate old code in CropImage

a96f2ea

have CropImageContract return non-null result

c6f6513

remove default parameters from CropImageContractOptions

d529075

fix ktlint

67d4846

Canato requested changes Jun 21, 2021

View reviewed changes

cropper/src/main/java/com/canhub/cropper/CropImage.kt Outdated Show resolved Hide resolved

cropper/src/main/java/com/canhub/cropper/CropImageContract.kt Outdated Show resolved Hide resolved

cropper/src/main/java/com/canhub/cropper/CropImageContract.kt Outdated Show resolved Hide resolved

connyduck added 4 commits June 21, 2021 19:42

extract CropImageContractOptions into its own file

6d130c5

rename OpenChooser to PickImageContract

1d480e9

add DEGREES_360 constant

34d8ef7

add code comments

1485ed3

Canato reviewed Jun 22, 2021

View reviewed changes

connyduck added 4 commits June 24, 2021 19:13

add CropImageContractTest

a041ea4

add PickImageContractTest

4c405b6

improve CropImageContractTest

90e0e72

update changelog

2efd302

connyduck marked this pull request as ready for review June 24, 2021 18:12

connyduck requested a review from a team as a code owner June 24, 2021 18:12

Canato reviewed Jun 24, 2021

View reviewed changes

cropper/src/test/java/com/canhub/cropper/PickImageContractTest.kt Outdated Show resolved Hide resolved

update CropImageActivity

4a71b55

connyduck force-pushed the activity_result_contract branch from 7167e73 to 4a71b55 Compare June 24, 2021 18:32

Canato requested changes Jun 24, 2021

View reviewed changes

cropper/src/test/java/com/canhub/cropper/PickImageContractTest.kt Outdated Show resolved Hide resolved

deprecate CropImage.getActivityResult

a751359

connyduck added 3 commits June 25, 2021 21:31

update sample app

a145e60

fix bug in CropImageContract

d2e2895

update test names

8b31c7a

fix ktlint

703b951

Canato requested changes Jun 26, 2021

View reviewed changes

connyduck added 2 commits June 29, 2021 19:16

make PickImageContract open and extend sample

385ec3c

improve sample

15621fb

Canato approved these changes Jul 1, 2021

View reviewed changes

Canato merged commit 630ed8f into CanHub:main Jul 1, 2021

Canato mentioned this pull request Nov 24, 2021

Release 4.0.0 #275

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ActivityResultContracts #145

add ActivityResultContracts #145

connyduck commented Jun 16, 2021 •

edited by Canato

Loading

connyduck commented Jun 16, 2021

Canato left a comment

connyduck commented Jun 17, 2021

Canato commented Jun 18, 2021

connyduck commented Jun 18, 2021

Canato commented Jun 21, 2021

Canato left a comment

Canato left a comment

Canato left a comment

connyduck commented Jun 24, 2021

Canato left a comment

connyduck commented Jun 25, 2021

Canato commented Jun 26, 2021

connyduck commented Jun 26, 2021

Canato left a comment

Canato Jun 26, 2021

Canato Jun 26, 2021

Canato Jun 26, 2021

connyduck Jun 26, 2021

Canato Jun 28, 2021

connyduck Jun 28, 2021 •

edited

Loading

Canato Jun 29, 2021

Canato Jun 26, 2021

connyduck Jun 26, 2021

Canato left a comment

add ActivityResultContracts #145

add ActivityResultContracts #145

Conversation

connyduck commented Jun 16, 2021 • edited by Canato Loading

Description:

Check list for the Code Reviewer:

connyduck commented Jun 16, 2021

Canato left a comment

Choose a reason for hiding this comment

connyduck commented Jun 17, 2021

Canato commented Jun 18, 2021

connyduck commented Jun 18, 2021

Canato commented Jun 21, 2021

Canato left a comment

Choose a reason for hiding this comment

Canato left a comment

Choose a reason for hiding this comment

Canato left a comment

Choose a reason for hiding this comment

connyduck commented Jun 24, 2021

Canato left a comment

Choose a reason for hiding this comment

connyduck commented Jun 25, 2021

Canato commented Jun 26, 2021

connyduck commented Jun 26, 2021

Canato 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

connyduck Jun 28, 2021 • edited Loading

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

Canato left a comment

Choose a reason for hiding this comment

connyduck commented Jun 16, 2021 •

edited by Canato

Loading

connyduck Jun 28, 2021 •

edited

Loading