Updates for Android 13 by vbuberen · Pull Request #887 · ChuckerTeam/chucker

vbuberen · 2022-09-18T18:50:33Z

📷 Videos

notifications_success_scenario.webm
notifications_disable_scenario.webm

📄 Context

An alternative approach to #879 . I wanted to avoid forcing users to modify their code somehow for Android 13, so went with the way where notification permission is added into Chucker's AndroidManifest. From a user side the only required action is to open Chucker after adding (ideally, via shortcut which we provide by default) and grant the permission. The flow feels like not the most convenient one, but I don't see other easier ways at the moment to handle this new permission request.

📝 Changes

Bumped targetSDK and compileSDK to 33
Added notifications permission handling on devices with Android 13

🚫 Breaking

Nothing breaking is expected

🛠️ How to test

Run Chucker sample on a device/emulator with Android 13 and try to open Chucker directly.

⏱️ Next steps

As I suggest to use the shortcut which we add, we might want to remove the possibility of disabling shortcut creation from our API to not confuse users.

Closes #854

cortinico

Great stuff, I prefer this approach than mine 👍

The only thing is that I would save the:

    private fun shouldShowNotification() =
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
            !BaseChuckerActivity.isInForeground && notificationManager.areNotificationsEnabled()
        } else {
            !BaseChuckerActivity.isInForeground
        }

to be added inside NotificatioHelper.show as querying for areNotificationsEnabled seems to be the recommended way to verify if notifications are enabled 👍

cortinico · 2022-09-20T00:04:20Z

    minSdkVersion = 21
-    targetSdkVersion = 31
-    compileSdkVersion = 31
+    targetSdkVersion = 33


might be worth also bumping app compat to 1.5.x in this same PR?

Yes, can do that as well.

cortinico · 2022-09-20T00:05:55Z

+        if (!isPermissionGranted) {
+            showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted))
+            Logger.error("Notification permission denied. Can`t show transactions info")
+        }


Might be worth to also log here a success message?

I don't think it is worth it as logs usually already filled with enough noise by other apps, libraries.

Ok but you log a:

Logger.info("Notification permission granted")

Just below. Shoudl we remove that as well then?

Good catch. Didn't like empty braces initially. Removed.

cortinico · 2022-09-20T00:18:35Z

+        ActivityResultContracts.RequestPermission()
+    ) { isPermissionGranted: Boolean ->
+        if (!isPermissionGranted) {
+            showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted))


I would make this a SnackBar as well with Long Duration.

We show toasts for all other fault/errors states. The reason I used Snackbar for redirection to Settings is to have an action button, so it is convenient to just click on it and end up in settings. Here we don't have any action and to be consistent with other failed operations (like exporting stuff, etc.) I use Toast.

Mmm I understand.

For me it was a bit odd as, when playing with the app I saw both the Toast and the SnackBar with the same error message, so I thought it was a bug.

I'm fine having it as a toast, but I would still have Duration Long as I barely had the time to read it.

Made it long.

cortinico · 2022-09-20T00:21:41Z

+                Snackbar.make(
+                    mainBinding.root,
+                    applicationContext.getString(R.string.chucker_notifications_permission_not_granted),
+                    Snackbar.LENGTH_LONG
+                ).setAction(applicationContext.getString(R.string.chucker_change)) {
+                    Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply {
+                        addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)
+                        data = Uri.fromParts("package", packageName, null)
+                    }.also { intent ->
+                        startActivity(intent)
+                    }
+                }.show()


Here we're breaking a bit from the Google's guideline where the rationale should just explain why we need the permission and we should still allow the user to pick yes/no. If yes -> we should fire the permissionRequest.

Letting the user jump to the Settings screen sort of assumes that the user selected No on the past.

Yes, I did it on purpose as I assume that Chucker is used by devs, not regular users, so showing one more dialog to somebody, who disabled permission on purpose on first launch is going to be annoying. Devs know how and why permission is needed, especially, since showing notifications is what Chucker does.
Instead, you have a snackbar and can change settings if you wish or just mind your own business with Chucker without dismissing the dialog.

Co-authored-by: Nicola Corti <corti.nico@gmail.com>

…inActivity.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

…ucker into update/android_13

cortinico

LGTM 👍 Minor comments that needs to follow up, but this is good to merge on my end. Thanks for doing it!

cortinico · 2022-09-25T16:02:11Z

+        if (!isPermissionGranted) {
+            showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted))
+            Logger.error("Notification permission denied. Can`t show transactions info")
+        }


Ok but you log a:

Logger.info("Notification permission granted")

Just below. Shoudl we remove that as well then?

cortinico · 2022-09-25T16:04:03Z

+        ActivityResultContracts.RequestPermission()
+    ) { isPermissionGranted: Boolean ->
+        if (!isPermissionGranted) {
+            showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted))


Mmm I understand.

For me it was a bit odd as, when playing with the app I saw both the Toast and the SnackBar with the same error message, so I thought it was a bug.

I'm fine having it as a toast, but I would still have Duration Long as I barely had the time to read it.

yogiseralia · 2022-09-27T12:46:01Z

Eagerly waiting for this update, when can we expect a new release with these changes??

…ranted

…ucker into update/android_13

vbuberen added 4 commits September 18, 2022 20:39

Add notifications permissions request

fd3fb53

Add toast when notification permission is not granted

472b99b

Update README, add release notes

a2648f6

Add string resource

378fe36

vbuberen added the enhancement New feature or improvement to the library label Sep 18, 2022

vbuberen requested a review from cortinico September 18, 2022 18:50

cortinico reviewed Sep 20, 2022

View reviewed changes

vbuberen and others added 5 commits September 21, 2022 12:37

Update README.md

9bb63f6

Co-authored-by: Nicola Corti <corti.nico@gmail.com>

Update library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/Ma…

9592206

…inActivity.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

Bump AppCompat, unify Toast usage

eee7739

Add check if notifications are enabled

1d49e93

Merge branch 'update/android_13' of https://github.com/ChuckerTeam/ch…

95d55c6

…ucker into update/android_13

vbuberen requested a review from cortinico September 21, 2022 10:15

Merge branch 'develop' into update/android_13

8a8f205

cortinico approved these changes Sep 25, 2022

View reviewed changes

cortinico mentioned this pull request Sep 27, 2022

Bump activity-ktx from 1.5.1 to 1.6.0 #892

Merged

vbuberen added 2 commits September 29, 2022 11:18

Add toast length parameter, remove log entry about permission being g…

57f5a04

…ranted

Merge branch 'update/android_13' of https://github.com/ChuckerTeam/ch…

cf1b140

…ucker into update/android_13

vbuberen enabled auto-merge (squash) September 29, 2022 08:20

vbuberen merged commit 71e0cea into develop Sep 29, 2022

vbuberen deleted the update/android_13 branch September 29, 2022 08:22

vbuberen mentioned this pull request Sep 29, 2022

Add support for Android 13 #879

Closed

ink1804 mentioned this pull request Sep 29, 2022

bumping android version breaking changes #897

Closed

Uh oh!

Conversation

vbuberen commented Sep 18, 2022

📷 Videos

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbuberen Sep 21, 2022 • 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!

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!

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!

cortinico left a comment

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!

yogiseralia commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbuberen Sep 21, 2022 •

edited

Loading