Kotlin conversion of AboutActivity with tests also in Kotlin #2360

juuce79 · 2025-02-26T23:18:29Z

Converted AboutActivity to kotlin from java.
Have tried to keep the code as close to the original as possible while using kotlin code instead of java code.
Added tests for AboutActivity.
Manually tested with APIs 24, 30 and 35 in Virtual Device in Android Studio.
This is part of my university project, to contribute to an open source project, and part of developer's plan to convert app to firstly Kotlin and eventually Jetpack Compose

Have tried to keep the code as close to the original as possible while using kotlin code instead of java code.

…bout_activity_kt_conversion

theimpulson · 2025-02-27T05:22:38Z

Related PRs that should be closed if this supersedes them:

theimpulson · 2025-02-27T05:33:23Z

app/src/main/java/protect/card_locker/AboutActivity.kt

+            OpenWebLinkHandler().openBrowser(this, tag)
+        }
+    }
+}


Missing new line EOF

updated with @theimpulson's suggestion, now:
Applied the suggestion! Here's the updated code block showing the change:

143 private fun openExternalBrowser(view: View) { 144 val tag = view.tag 145 if (tag is String && tag.startsWith("https://")) { 146 OpenWebLinkHandler().openBrowser(this, tag) 147 } 148 } 149 } 150

theimpulson · 2025-02-27T05:47:37Z

app/src/main/java/protect/card_locker/AboutActivity.kt

+        if (item.itemId == android.R.id.home) {
+            finish()
+        }
+        return super.onOptionsItemSelected(item)


You can consider using a when block here for a cleaner and easily extensible code.

Suggested change

if (item.itemId == android.R.id.home) {

finish()

}

return super.onOptionsItemSelected(item)

return when (item.itemId) {

android.R.id.home -> finish()

else -> super.onOptionsItemSelected(item)

}

updated with @theimpulson's suggestion, now:

56 override fun onOptionsItemSelected(item: MenuItem): Boolean { 57 return when (item.itemId) { 58 android.R.id.home -> { 59 finish() 60 true 61 } 62 63 else -> super.onOptionsItemSelected(item) 64 } 65 }

theimpulson · 2025-02-27T05:48:42Z

app/src/main/java/protect/card_locker/AboutActivity.kt

+            // Hide Google Play rate button if not on Google Play
+            rate.visibility = if (BuildConfig.showRateOnGooglePlay) View.VISIBLE else View.GONE
+            // Hide donate button on Google Play (Google Play doesn't allow donation links)
+            donate.visibility = if (BuildConfig.showDonate) View.VISIBLE else View.GONE


You can use the isVisible extension method from the core-ktx library here to make the logic more clean and simple.

updated with @theimpulson's suggestion, now:

47 // Hide Google Play rate button if not on Google Play 48 rate.isVisible = BuildConfig.showRateOnGooglePlay 49 // Hide donate button on Google Play (Google Play doesn't allow donation links) 50 donate.isVisible = BuildConfig.showDonate

theimpulson · 2025-02-27T05:49:14Z

app/src/main/java/protect/card_locker/AboutActivity.kt

+    private var _binding: AboutActivityBinding? = null
+    private val binding get() = _binding!!


This pattern of view binding is used for fragments and not activities. Please see this guide on patterns for activities.

updated with @theimpulson's suggestion, now:

22 private lateinit var binding: AboutActivityBinding

theimpulson · 2025-02-27T05:55:02Z

app/src/main/java/protect/card_locker/AboutActivity.kt

+    override fun onDestroy() {
+        super.onDestroy()
+        content.destroy()
+        clearClickListeners()
+        _binding = null
+    }


These methods are not required for an Activity and should be dropped. They are only relevant for Fragment.

updated with @theimpulson's suggestion, now:

67 override fun onDestroy() { 68 super.onDestroy() 69 content.destroy() 70 clearClickListeners() 71 }

juuce79 · 2025-02-27T07:44:39Z

@theimpulson I have updated the code with your suggestions and I was wondering, can I update this pull request with a new commit of the updated code (I haven't pushed or committed it to my fork yet so I don't mess anything up), or do I need to make a new pull request?
Regarding this code

    override fun onDestroy() {
        super.onDestroy()
        content.destroy()
        clearClickListeners()
        _binding = null
    }

I have just removed the _binding = null.

As this is being done as part of my university program and I am documenting the process, here is a PDF of the conversion process with the new updated code included:
catima_kotlin_conversion (2).pdf

Thanks for the help by the way, I and my partner (we are 2 working on this project) are still at university and have only had one Kotlin course so we are still new to Kotlin, and Java to be honest, although we have had quite a few Java courses so far. We are currently in our third year of our bachelors degree.

- added new line at EOF. - changed if-else statement to `when` block for cleaner and more easily extensible code (lines 57-63). - now using `isVisible` extension method from the core-ktx library to make the logic more clean and simple (lines 48 & 50). - now using correct view binding pattern for activities (line 22). - removed `_binding = null` from `onDestroy()` function (line 71).

juuce79 · 2025-02-27T13:33:27Z

Updated with Aayush Gupta's suggestions:

added new line at EOF.
changed if-else statement to when block for cleaner and more easily extensible code (lines 57-63).
now using isVisible extension method from the core-ktx library to make the logic more clean and simple (lines 48 & 50).
now using correct view binding pattern for activities (line 22).
removed _binding = null from onDestroy() function (line 71).

TheLastProject · 2025-02-27T17:20:09Z

Looks sensible to me. If @theimpulson thinks it's good now I'll do some quick on-device testing and hit merge :)

theimpulson · 2025-02-28T05:01:01Z

app/src/test/java/protect/card_locker/AboutActivityTest.kt

+        val rateButton = activity.findViewById<View>(R.id.rate)
+        assertEquals(rateButton.visibility,
+            if (BuildConfig.showRateOnGooglePlay) View.VISIBLE else View.GONE)
+
+        // Test donate button visibility based on BuildConfig
+        val donateButton = activity.findViewById<View>(R.id.donate)
+        assertEquals(donateButton.visibility,
+            if (BuildConfig.showDonate) View.VISIBLE else View.GONE)


These could be switched to isVisible extension method as well like above for better readability.

updated with @theimpulson's suggestion. Now:

51 @Test 52 fun testDisplayOptionsBasedOnConfig() { 53 activityController.create().start().resume() 54 55 // Test Google Play rate button visibility based on BuildConfig 56 val rateButton = activity.findViewById<View>(R.id.rate) 57 assertEquals(BuildConfig.showRateOnGooglePlay, rateButton.isVisible) 58 59 // Test donate button visibility based on BuildConfig 60 val donateButton = activity.findViewById<View>(R.id.donate) 61 assertEquals(BuildConfig.showDonate, donateButton.isVisible) 62 }

theimpulson · 2025-02-28T05:03:43Z

app/src/test/java/protect/card_locker/AboutActivityTest.kt

+        // Get the current values from BuildConfig
+        val showRateOnGooglePlay = BuildConfig.showRateOnGooglePlay
+        val showDonate = BuildConfig.showDonate
+
+        // Test that the visibility matches the BuildConfig values
+        assertEquals(if (showRateOnGooglePlay) View.VISIBLE else View.GONE,
+            activity.findViewById<View>(R.id.rate).visibility)
+        assertEquals(if (showDonate) View.VISIBLE else View.GONE,
+            activity.findViewById<View>(R.id.donate).visibility)


Similar suggestion to use isVisible like above

updated with @theimpulson's suggestion. Now:

145 @Test 146 fun testButtonVisibilityBasedOnBuildConfig() { 147 activityController.create().start().resume() 148 149 // Get the current values from BuildConfig 150 val showRateOnGooglePlay = BuildConfig.showRateOnGooglePlay 151 val showDonate = BuildConfig.showDonate 152 153 // Test that the visibility matches the BuildConfig values 154 assertEquals(showRateOnGooglePlay, activity.findViewById<View>(R.id.rate).isVisible) 155 assertEquals(showDonate, activity.findViewById<View>(R.id.donate).isVisible) 156 }

…tonVisibilityBasedOnBuildConfig` to use `isVisible` instead of checking visibility property directly with `View.VISIBLE/GONE` comparisons. This makes tests more consistent with implementation code in `AboutActivity.kt` and improves test readability.

juuce79 · 2025-02-28T06:25:29Z

@theimpulson @TheLastProject Updated test functions testDisplayOptionsBasedOnConfig and testButtonVisibilityBasedOnBuildConfig to use isVisible instead of checking visibility property directly with View.VISIBLE/GONE comparisons. This makes tests more consistent with implementation code in AboutActivity.kt and improves test readability.

theimpulson

Looks good to me. Thank you.

TheLastProject · 2025-03-02T12:29:57Z

Tested it and it works great. Thanks for the rewrite @juuce79 (and also thank you for the PDF file, it made understanding your changes a lot easier!). And thank you for the review, @theimpulson! :)

juuce79 added 5 commits February 21, 2025 01:13

Rename .java to .kt

7ee3ac8

Converted AboutActivity to kotlin from java.

0625d2b

Have tried to keep the code as close to the original as possible while using kotlin code instead of java code.

Added comments to mirror original java code

9c7add0

added tests for AboutActivity

f111a46

Merge remote-tracking branch 'upstream/HEAD' into kotlin_conversion/a…

b8e7303

…bout_activity_kt_conversion

theimpulson suggested changes Feb 27, 2025

View reviewed changes

theimpulson suggested changes Feb 28, 2025

View reviewed changes

theimpulson approved these changes Mar 2, 2025

View reviewed changes

TheLastProject merged commit 1a892b2 into CatimaLoyalty:main Mar 2, 2025
2 checks passed

TheLastProject mentioned this pull request Mar 2, 2025

Feature/about kotlin #1122

Closed

TheLastProject mentioned this pull request May 3, 2025

Migration to Jetpack Compose #2483

Open

27 tasks

		private var _binding: AboutActivityBinding? = null
		private val binding get() = _binding!!

Uh oh!

Kotlin conversion of AboutActivity with tests also in Kotlin #2360

Kotlin conversion of AboutActivity with tests also in Kotlin #2360

Uh oh!

Conversation

juuce79 commented Feb 26, 2025

Uh oh!

theimpulson commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juuce79 commented Feb 27, 2025

Uh oh!

TheLastProject commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuce79 commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theimpulson left a comment

Choose a reason for hiding this comment

Uh oh!

TheLastProject commented Mar 2, 2025

Uh oh!

Uh oh!

Uh oh!

juuce79 Feb 27, 2025 •

edited

Loading

juuce79 Feb 27, 2025 •

edited

Loading

juuce79 Feb 27, 2025 •

edited

Loading

juuce79 Feb 27, 2025 •

edited

Loading

juuce79 Feb 27, 2025 •

edited

Loading

juuce79 commented Feb 27, 2025 •

edited

Loading

juuce79 commented Feb 28, 2025 •

edited

Loading