-
-
Notifications
You must be signed in to change notification settings - Fork 339
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
Updates for Android 13 #887
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
@@ -101,8 +101,8 @@ task clean(type: Delete) { | |||
|
|||
ext { | |||
minSdkVersion = 21 | |||
targetSdkVersion = 31 | |||
compileSdkVersion = 31 | |||
targetSdkVersion = 33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth also bumping app compat to 1.5.x in this same PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can do that as well.
library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/MainActivity.kt
Outdated
Show resolved
Hide resolved
if (!isPermissionGranted) { | ||
showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted)) | ||
Logger.error("Notification permission denied. Can`t show transactions info") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth to also log here a success message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is worth it as logs usually already filled with enough noise by other apps, libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but you log a:
Logger.info("Notification permission granted")
Just below. Shoudl we remove that as well then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Didn't like empty braces initially. Removed.
ActivityResultContracts.RequestPermission() | ||
) { isPermissionGranted: Boolean -> | ||
if (!isPermissionGranted) { | ||
showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this a SnackBar as well with Long Duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it long.
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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
…inActivity.kt Co-authored-by: Nicola Corti <[email protected]>
…ucker into update/android_13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Minor comments that needs to follow up, but this is good to merge on my end. Thanks for doing it!
if (!isPermissionGranted) { | ||
showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted)) | ||
Logger.error("Notification permission denied. Can`t show transactions info") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but you log a:
Logger.info("Notification permission granted")
Just below. Shoudl we remove that as well then?
ActivityResultContracts.RequestPermission() | ||
) { isPermissionGranted: Boolean -> | ||
if (!isPermissionGranted) { | ||
showToast(applicationContext.getString(R.string.chucker_notifications_permission_not_granted)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Eagerly waiting for this update, when can we expect a new release with these changes?? |
📷 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
targetSDK
andcompileSDK
to 33🚫 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