Convert CropImageOptions to data class with proper default parameters. #396

vanniktech · 2022-06-25T22:47:37Z

What do you think of this?

It's way less code.
We don't need to do the Parcelable dance ourselves.
CropImageContractOptions would get much simpler too
Unifies the API Consistent names #387

Wanted to get your opinion first, before continuing

github-advanced-security

Found 29 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 29 potential problems in the proposed changes. Check the Files changed tab for more details.

PaulWoitaschek · 2022-06-27T13:47:54Z

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


    /**
     * The radius of the touchable area around the handle. (in pixels)<br></br>
     * We are basing this value off of the recommended 48dp Rhythm.<br></br>
     * See: http://developer.android.com/design/style/metrics-grids.html#48dp-rhythm
     */
    @JvmField
-    var touchRadius: Float
+    val touchRadius: Float = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 24f, Resources.getSystem().displayMetrics),


This should never be used as it might be different from the current window.
Alternatively I'd just rename this to touchRadiusDp, set 24 as the default value and do the conversion at a later stage.

agreed, but would suggest another PR to do the changes, so we keep it self contained

Agreed, I just inlined the previous defaults. I'd change that in a follow up. Also there's the #387

PaulWoitaschek · 2022-06-27T13:48:35Z

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


    /**
     * The radius of the touchable area around the handle. (in pixels)<br></br>
     * We are basing this value off of the recommended 48dp Rhythm.<br></br>
     * See: http://developer.android.com/design/style/metrics-grids.html#48dp-rhythm
     */
    @JvmField
-    var touchRadius: Float
+    val touchRadius: Float = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 24f, Resources.getSystem().displayMetrics),

    /** whether the guidelines should be on, off, or only showing when resizing.  */
    @JvmField


I don't see the benefit of JvmField here.

I believe we need this to work on Java, right?

When you're with Java you'd need to call getGuideliness here without it

For java it's also a best practice to use a getter in order to be able to change the implementation without needing to change the api.

Up to you. I can remove all JvmFields.

Canato

This is a great PR, thanks for the work. Will help everyone who uses and maintain the library.

Keep the good work 🚀

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

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

Canato · 2022-06-28T08:40:42Z

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


    /**
     * The radius of the touchable area around the handle. (in pixels)<br></br>
     * We are basing this value off of the recommended 48dp Rhythm.<br></br>
     * See: http://developer.android.com/design/style/metrics-grids.html#48dp-rhythm
     */
    @JvmField
-    var touchRadius: Float
+    val touchRadius: Float = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 24f, Resources.getSystem().displayMetrics),

    /** whether the guidelines should be on, off, or only showing when resizing.  */
    @JvmField


I believe we need this to work on Java, right?

Canato · 2022-06-28T08:41:50Z

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


    /**
     * The radius of the touchable area around the handle. (in pixels)<br></br>
     * We are basing this value off of the recommended 48dp Rhythm.<br></br>
     * See: http://developer.android.com/design/style/metrics-grids.html#48dp-rhythm
     */
    @JvmField
-    var touchRadius: Float
+    val touchRadius: Float = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 24f, Resources.getSystem().displayMetrics),


agreed, but would suggest another PR to do the changes, so we keep it self contained

Canato · 2022-06-28T10:00:05Z

@vanniktech please fix the CI issues so I can test ^^.

Maybe the contributing guide can help with ktlint

vanniktech · 2022-06-28T10:28:28Z

Mobile data is terrible where I am currently. I only have ktlint 0.46.0 & 0.46.1 locally and we can't seem to update this project #398, will run ktlint later

github-advanced-security

Found 65 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 67 potential problems in the proposed changes. Check the Files changed tab for more details.

vanniktech · 2022-06-28T21:52:16Z

@Canato updated the PR with ktlint fixes.

Canato

Nice work! need some rebase with the latest merged PRs

CONTRIBUTING.md

Canato · 2022-06-29T13:01:28Z

sample/src/main/java/com/canhub/cropper/sample/MainActivity.kt

@@ -18,7 +18,6 @@ internal class MainActivity : AppCompatActivity() {
        binding.sampleCropImageView.setOnClickListener { SampleUsingImageView.newInstance().show() }
        binding.sampleCustomActivity.setOnClickListener { SampleCustomActivity.start(this) }
        binding.sampleCropImage.setOnClickListener { SampleCrop.newInstance().show() }
-        binding.sampleCropImageJava.setOnClickListener { SampleCropJava.newInstance().show() }


Why remove this?

I don't see much benefit of providing it. We should not be the one teaching people how to use Kotlin from Java.

We should not be the one teaching people how to use Kotlin from Java\

Why should we not show how to use the library in Java?

There are many requests to use the library in Java code, we have documentation only for this and is by far the most amount of issues I get in the repo.

I need a way to manually test if is working, in the case someone spot a bug in the library.

this and is by far the most amount of issues I get in the repo.

Seems like we have different motivations. I'll eventually get around of it and update it. In the meantime, I'll build a version myself.

I need a way to manually test if is working, in the case someone spot a bug in the library.

And this can't be done using Kotlin only? What's a bug that is only targeting Java?

I'm just in favor of reducing maintenance cost here. We don't need similar things twice. Kotlin and Java.

Kotlin is the industry standard.

Javs usage is very simple and you get autocompletion. Also it's pretty well documented how interop is working in general.
https://kotlinlang.org/docs/java-interop.html

I believe it should not be a task of a library to document how kotlin-java interop works.

[Feat] - does a java version exist ? #338

[BUG] - No method find when java migrating to Can Hub #317

[BUG] - Cannot find getActivityResult() while migrating from ArthurHub to CanHub #316

[BUG] - "4.0.0"->App crash when clicking on "SAMPLE CALLING CROP IMAGE JAVA->TAKE CAMERA PICTURE BEFORE CALLING LIBRARY WITH URI" #301

[Feat] - how to use in java #290

[hacktoberfest] - Add Java samples #227

[Feat] - use in java #224

Any one please share a java version....! #210

Hello please java code provide #192

Implementation using java, with alternative for deprecated onActivityResult() #169

https://github.com/CanHub/Android-Image-Cropper/discussions/359

https://github.com/CanHub/Android-Image-Cropper/discussions/236

https://github.com/CanHub/Android-Image-Cropper/discussions/286

https://github.com/CanHub/Android-Image-Cropper/discussions/170

These are some of the cases.

Most of the time I'm maintaining the library by myself.
In other great times, good developers like you two (@PaulWoitaschek and @vanniktech) appear to provide valid help.
But so far no one took the step further of becoming responsible for the library too.

I believe it should not be a task of a library to document how kotlin-java interop works.

I strongly believe we can support the community with examples of library usage. And if we can use Kotlin and Java why not have both?

Sadly not all developers who use this library are good developers or even know how to use Kotlin.
There are some developers blocked on the versions before the v4, because they don't know how to use ActivityContracts or they don't have the skills to update Gradle and Java.

My goal with this library is to help the Android community, having it in Java helps a lot.
I already changed everything to Kotlin, fix issues and update to the latest Gradle, OS and Java.

I believe we can take time and help new developers on the path to becoming better Android developers instead of excluding them from using the library because they don't know Kotlin yet.

That said, if the community that uses this library wanna go in this direction, I will not oppose it. But we need someone to take ownership of the Java questions.

Canato · 2022-06-29T13:02:04Z

sample/src/main/java/com/canhub/cropper/sample/SampleCrop.kt

        customCropImage.launch(
-            options {
-                setImageSource(
-                    includeGallery = includeGallery,
-                    includeCamera = includeCamera,
+            CropImageContractOptions(
+                uri = null,
+                CropImageOptions(


We need to update the README, documentation, wiki etc.

Canato · 2022-06-29T13:02:28Z

sample/src/main/java/com/canhub/cropper/sample/SampleCrop.kt

-                setAllowRotation(true)
-                setNoOutputImage(false)
-                setFixAspectRatio(false)
-                // Odd Settings


Where is the new odd settings?

Removed, people can look at the type itself to figure out all options

Can you please return the odd settings? I use this for testing when people make changes to the library.

Or if you really want to remove we can add CI tests that check is this values are been setup.

Canato · 2022-06-29T13:02:59Z

sample/src/main/res/layout/activity_main.xml

-        <Button
-            style="@style/Sample.Button"
-            android:id="@+id/sampleCropImageJava"
-            android:layout_width="wrap_content"
-            android:layout_height="wrap_content"
-            android:layout_gravity="center"
-            android:text="@string/sample_calling_cropimage_java"/>


Why this was removed?

github-advanced-security

Found 67 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 67 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 67 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 67 potential problems in the proposed changes. Check the Files changed tab for more details.

vanniktech · 2022-07-25T12:05:09Z

Let's keep this mess organised! This issue has been automatically marked as stale because it has not had recent activity. =( It will be closed if no further activity occurs. Thank you for your contributions.

Convert CropImageOptions to data class with proper default parameters.

7fa4234

vanniktech requested a review from a team as a code owner June 25, 2022 22:47

github-advanced-security bot found potential problems Jun 25, 2022

View reviewed changes

github-advanced-security bot found potential problems Jun 26, 2022

View reviewed changes

PaulWoitaschek reviewed Jun 27, 2022

View reviewed changes

Canato requested changes Jun 28, 2022

View reviewed changes

Canato linked an issue Jun 28, 2022 that may be closed by this pull request

Consistent names #387

Closed

vanniktech added 3 commits June 28, 2022 11:43

Update dependencies. (#394)

3930666

The rest.

c81b5aa

Merge branch 'main' into better-CropImageOptions

882bffb

vanniktech requested a review from Canato June 28, 2022 09:44

Update contributing.

4879f89

github-advanced-security bot found potential problems Jun 28, 2022

View reviewed changes

Run ktlintFormat.

2488119

github-advanced-security bot found potential problems Jun 28, 2022

View reviewed changes

Canato requested changes Jun 29, 2022

View reviewed changes

vanniktech added 2 commits June 29, 2022 16:20

Merge branch 'main' into better-CropImageOptions

08fd0cd

Fix CONTRIBUTING.md

746d775

github-advanced-security bot found potential problems Jun 29, 2022

View reviewed changes

github-advanced-security bot found potential problems Jul 3, 2022

View reviewed changes

github-advanced-security bot found potential problems Jul 10, 2022

View reviewed changes

github-advanced-security bot found potential problems Jul 17, 2022

View reviewed changes

vanniktech closed this Jul 25, 2022

vanniktech deleted the better-CropImageOptions branch July 25, 2022 12:05

vanniktech mentioned this pull request Sep 26, 2022

Correctly close resources in BitmapUtils. #440

Merged

Convert CropImageOptions to data class with proper default parameters. #396

Convert CropImageOptions to data class with proper default parameters. #396

Conversation

vanniktech commented Jun 25, 2022

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Canato commented Jun 28, 2022

vanniktech commented Jun 28, 2022

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

vanniktech commented Jun 28, 2022

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

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

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

vanniktech commented Jul 25, 2022