Implement notification trigger #4159
Conversation
Coverage Report 1Affected ProductsNo changes between base commit (b124fac) and merge commit (4c4d65d).Test Logs |
Size Report 1Affected ProductsNo changes between base commit (b124fac) and merge commit (4c4d65d).Test Logs |
| val channel = NotificationChannel( | ||
| FEEBACK_NOTIFICATION_CHANNEL_ID, | ||
| application.getString(R.string.feedback_notification_channel_name), | ||
| NotificationManager.IMPORTANCE_HIGH |
There was a problem hiding this comment.
I thought we were going to use a low importance so that it didn't emit a sound or anything.
There was a problem hiding this comment.
Yes, but unfortunately low importance messages sometimes just don't show up at all 😬 So I decided to err on the side of visibility - making sure the notification is seen. Since this is sample code selecting the appropriate importance is ultimately up to the developers integrating code based on this example into their app.
There was a problem hiding this comment.
Got it. I think it could be worth trying to make sure there's a level that makes sense though. If all of the levels either 1) make a sound or pop down over the current view, or 2) show up inconsistently, then it might not make sense to even supply sample code for a notification trigger.
There was a problem hiding this comment.
I guess it's up to the developer even if it makes sounds. Some developers might really want to get feedback 😄
...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } else { | ||
| Log.w(TAG, "Not listening for screenshots because this activity can't register for permission request results: $activity") |
There was a problem hiding this comment.
Nit: "Not listening for notification clicks" ?
I'm actually realizing that this logic (and thus my logic from the screenshot detector) is a bit off. If the activity can't get register for an activity result then we'd want to make sure we don't try and request the permission at all. But we're not doing that in either trigger.
There was a problem hiding this comment.
I've updated the log message.
I've added some code which now checks whether requestPermissionLauncher is null (and sets it to nuill when an activity is paused). I'm now also checking for !hasRequestedPermission before registering the launcher - because if we already have the permission, there's no need for the launcher.
That being said, the logic around requesting permissions seems pretty janky to me. We should probably reword it (for both of our triggers) so it's robust and follows the same pattern. Let's do that after merging this code?
There was a problem hiding this comment.
For example, I don't understand why registering the launcher happens in onCreateActivity() instead of in requestPermission() itself.
There was a problem hiding this comment.
Sounds good let's have a look. I'd love to put it in some shared component but that goes against the idea of having an entire trigger in one nice copyable file.
But regardless unfortunately you have to register the launcher when the activity is initialized or else vague bad things happen... https://developer.android.com/reference/androidx/activity/result/ActivityResultCaller#registerForActivityResult(androidx.activity.result.contract.ActivityResultContract%3CI,O%3E,androidx.activity.result.ActivityResultCallback%3CO%3E)
|
|
||
| private fun requestPermission(activity: Activity) { | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) { | ||
| return // no need |
There was a problem hiding this comment.
Before Tiramisu, would ContextCompat.checkSelfPermission() always return PERMISSION_GRANTED? If so then we would never reach this condition.
Even if we could reach this condition we'd just want to go ahead and show the notification, right? As is, we'd do nothing and set hasRequestedPermission to true.
There was a problem hiding this comment.
This was intended as defense-in-depth, but I can see how it was confusing. So I've changed it.
...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt
Show resolved
Hide resolved
...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt
Outdated
Show resolved
Hide resolved
| override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) {} | ||
|
|
||
| fun takeScreenshot() { | ||
| val activity = currentActivity |
There was a problem hiding this comment.
So it looks like you are able to take a screenshot of a paused activity then, very cool. Out of curiosity, did you try taking a screenshot of the TakeScreenshotAndTriggerFeedbackActivity itself, and if so what happened?
There was a problem hiding this comment.
It just took a screenshot of the empty activity.
| versionName "2.0" | ||
| versionCode 2 |
There was a problem hiding this comment.
Just checking: do you want to keep this in?
There was a problem hiding this comment.
Yes, I think we should get into the habit of increasing those numbers.
Adding the feedback functionality seems to enough for a major version bump.
…/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt Co-authored-by: Lee Kellogg <[email protected]>
| } | ||
| } | ||
|
|
||
| class TakeScreenshotAndTriggerFeedbackActivity : AppCompatActivity() { |
There was a problem hiding this comment.
Just a note: if we're serious about supporting this use case we could add this activity to the SDK (along with tracking the previous activity so we could take the screenshot). Then they could just launch the intent for the activity and we could handle everything else. That might call for an API review though.
No description provided.