-
Notifications
You must be signed in to change notification settings - Fork 625
Notifications: Ask permission upfront, show only one persistent notification #4179
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
Conversation
…d of cancelling and showing a new one as Activities pause/resume.
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected ProductsNo changes between base commit (4d64675) and merge commit (24f44b1).Test Logs |
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.
This looks great and works well for our test app so I think we should submit this as is. That said I just want to call out that I think when we move this to the SDK we could combine initialize
, requestPermission
, and enable
into one call, and maybe even remove disable
:
initialize()
needed to be called in theApplication.onCreate()
so we'd be able to hook into theActivity.onCreate()
using lifecycle callbacks. But that's not necessary anymore now that we're telling them to callrequestPermission()
in theActivity.onCreate()
. We can just combine the two.enable()
can probably also be combined into the same method. Our test app wants the ability to toggle this on and off but I don't see that being super useful in a real app. They probably just want to either enable it or not when the app starts. Also if they're requesting the permission they probably just want to go ahead and enable it.- For the same reason, I think we can consider removing
disable()
...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Show resolved
Hide resolved
* | ||
* @param activity the [Activity] object | ||
*/ | ||
@RequiresApi(Build.VERSION_CODES.TIRAMISU) |
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.
Nice!
...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt
Outdated
Show resolved
Hide resolved
Makes sense! |
No description provided.