Skip to content
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

Added activity lifecycle handler to safe call android settings dialog #2088

Closed
wants to merge 3 commits into from

Conversation

caghp94
Copy link

@caghp94 caghp94 commented May 15, 2024

Description

One Line Summary

Added a safe call for AlertDialogPrepromptForAndroidSettings.show() using an activity lifecycle handler

Details

Motivation

This is a fix for #2014.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@jkasten2
Copy link
Member

jkasten2 commented May 30, 2024

@caghp94 Thanks for the PR!

I looked it over and I see 2 entries points to the code that is modified in this PR.

  • NotificationPermissionController.reject - I think these code changes will probably work with this, as I believe the PermissionsActivity is finishing and there will be a different Activity will become foregrounded soon. (This most likely fixes [Bug]: android.view.WindowManager$BadTokenException - Unable to add window #2014 as you noted)
  • NotificationPermissionController.prompt - This one fires for Android 12 and older devices. I don't believe any Activity focus changes are expected here, so I think your PR will break this case. Can you confirm?

@caghp94
Copy link
Author

caghp94 commented May 30, 2024

@caghp94 Thanks for the PR!

I looked it over and I see 2 entries points to the code that is modified in this PR.

  • NotificationPermissionController.reject - I think these code changes will probably work with this, as I believe the PermissionsActivity is finishing and there will be a different Activity will become foregrounded soon. (This most likely fixes [Bug]: android.view.WindowManager$BadTokenException - Unable to add window #2014 as you noted)
  • NotificationPermissionController.prompt - This one fires for Android 12 and older devices. I don't believe any Activity focus changes are expected here, so I think your PR will break this case. Can you confirm?

Hello @jkasten2 , the implementation of the addActivityLifecycleHandler method notifies if theres an activity currently available (the app is not in the background) not only the focus changes, so I think it wont break

override fun addActivityLifecycleHandler(handler: IActivityLifecycleHandler) {
        activityLifecycleNotifier.subscribe(handler)
        if (current != null) {
            handler.onActivityAvailable(current!!)
        }
    }

@jkasten2
Copy link
Member

jkasten2 commented May 30, 2024

@caghp94 ah yes you are correct, it won't break NotificationPermissionController.prompt as it does fire right away if there is an Activity already available, thanks for checking!

However with that information, looking at the NotificationPermissionController.prompt flow again I have a concern that the fallback dialog might not show in some cases, due to the AndroidUtils.isActivityFullyReady() check. It might be false and there is nothing done to check it again later (unless of course another Activity focus happens but there is no telling when that would happen). Thinking you may want to use waitUntilActivityReady() instead of AndroidUtils.isActivityFullyReady() as it takes care of the fallback.

@caghp94
Copy link
Author

caghp94 commented May 30, 2024

@jkasten2 you are right! just updated with the modification

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on Android 6.0 (but probably happens on any Android version) and the prompt is not showing;

  1. Opened app
  2. Disabled notifications for the app in the settings
  3. Attempted to prompt with requestPermission(true)
  4. Observe nothing happens in the app, and we get an error in the logcat.
2024-05-31 14:36:01.667  4250-4422  OneSignal               com.onesignal.sdktest                E  Exception on thread
                                                                                                    java.lang.RuntimeException: Can't create handler inside thread that has not called Looper.prepare()
                                                                                                    	at android.os.Handler.<init>(Handler.java:200)
                                                                                                    	at android.os.Handler.<init>(Handler.java:114)
                                                                                                    	at android.app.Dialog.<init>(Dialog.java:119)
                                                                                                    	at android.app.AlertDialog.<init>(AlertDialog.java:200)
                                                                                                    	at android.app.AlertDialog$Builder.create(AlertDialog.java:1086)
                                                                                                    	at android.app.AlertDialog$Builder.show(AlertDialog.java:1111)
                                                                                                    	at com.onesignal.core.internal.permissions.AlertDialogPrepromptForAndroidSettings$show$1$onActivityAvailable$1.invokeSuspend(AlertDialogPrepromptForAndroidSettings.kt:79)
                                                                                                    	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                                                                                                    	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
                                                                                                    	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:284)
                                                                                                    	at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:85)
                                                                                                    	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
                                                                                                    	at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source)
                                                                                                    	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
                                                                                                    	at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source)
                                                                                                    	at com.onesignal.common.threading.ThreadUtilsKt$suspendifyOnThread$1.invoke(ThreadUtils.kt:69)
                                                                                                    	at com.onesignal.common.threading.ThreadUtilsKt$suspendifyOnThread$1.invoke(ThreadUtils.kt:67)
                                                                                                    	at kotlin.concurrent.ThreadsKt$thread$thread$1.run(Thread.kt:30)

@jinliu9508 jinliu9508 closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants