feat(appdistribution): add in-app-feedback support #1463
Conversation
There was a problem hiding this comment.
Nice! Left some comments.
By the way, do you intend to update the README as well? https://github.com/firebase/quickstart-android/blob/master/appdistribution/README.md
...ution/app/src/main/java/com/google/firebase/appdistributionquickstart/java/MainActivity.java
Outdated
Show resolved
Hide resolved
...ution/app/src/main/java/com/google/firebase/appdistributionquickstart/java/MainActivity.java
Outdated
Show resolved
Hide resolved
...ution/app/src/main/java/com/google/firebase/appdistributionquickstart/java/MainActivity.java
Outdated
Show resolved
Hide resolved
...ution/app/src/main/java/com/google/firebase/appdistributionquickstart/java/MainActivity.java
Outdated
Show resolved
Hide resolved
|
@lfkellogg Great callout on the README. I updated it and its screenshot. PTAL |
...ution/app/src/main/java/com/google/firebase/appdistributionquickstart/java/MainActivity.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| protected void onDestroy() { | ||
| super.onDestroy(); | ||
| // Hide the notification once this activity is destroyed | ||
| mFirebaseAppDistribution.cancelFeedbackNotification(); | ||
| } |
There was a problem hiding this comment.
You shouldn't need this anymore now that we made the change to automatically hide the notification
There was a problem hiding this comment.
When does the automatic hide take place? I tried closing the Activity, but it didn't dismiss the notification. Is it when the app is closed?
|
|
||
| private void askNotificationPermission() { | ||
| // This is only necessary for API level >= 33 (TIRAMISU) | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { |
There was a problem hiding this comment.
I don't think you want this check here anymore. Even if they are on an older device we still want to show the notification. On older devices the checkSelfPermission() call will return true.
There was a problem hiding this comment.
cc: @marinacoelho
The check is due to the POST_NOTIFICATIONS field which was introduced in API 33.
I have added an else branch to display the feedback notification on older devices.
| override fun onDestroy() { | ||
| super.onDestroy() | ||
| // Hide the notification once this activity is destroyed | ||
| firebaseAppDistribution.cancelFeedbackNotification() |
There was a problem hiding this comment.
According to Lee's comment above, I think we don't need this line here as well.
|
|
||
| private fun askNotificationPermission() { | ||
| // This is only necessary for API level >= 33 (TIRAMISU) | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { |
There was a problem hiding this comment.
Same as Lee's comment above. And if we absolutely need to keep this check here, what do you think of changing this function a bit to use when instead of if / else if / else ? (Or switch / case on the Java file).
There was a problem hiding this comment.
Having a when here might work for Kotlin, since the language allows usage of when without a subject. But not sure if Java has a switch-case without a subject 🤔
No description provided.