Notification trigger API #4183

lfkellogg · 2022-10-07T14:10:52Z

This adds a new API allowing developers to show a notification that, when tapped, will take a screenshot of the running application and open the feedback form. This way the developers don't need to add any new UI elements to their app to trigger feedback.

Other changes include:

The lifecycle notifier now tracks the previous activity in addition to the current one, so that we can ignore our own empty activity when taking a screenshot after the notification is tapped.
Adds the SDK-based notification trigger to the test app

google-oss-bot · 2022-10-07T14:15:27Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from 78.24% (765f9c8) to 77.88% (1d9d395) by -0.36%.

Filename	Base (`765f9c8`)	Merge (`1d9d395`)	Diff
FirebaseAppDistributionImpl.java	94.42%	91.92%	-2.50%
FirebaseAppDistributionLifecycleNotifier.java	98.00%	96.67%	-1.33%
FirebaseAppDistributionNotificationsManager.java	80.00%	80.95%	+0.95%
LogWrapper.java	62.50%	68.75%	+6.25%
ScreenshotTaker.java	66.67%	62.16%	-4.50%
TakeScreenshotAndStartFeedbackActivity.java	?	0.00%	?

firebase-appdistribution-api
Overall coverage changed from 57.76% (765f9c8) to 49.63% (1d9d395) by -8.13%.
Filename Base (765f9c8) Merge (1d9d395) Diff
FirebaseAppDistributionProxy.java 57.89% 44.00% -13.89%
FirebaseAppDistributionStub.java 76.09% 71.43% -4.66%
InterruptionLevel.java ? 0.00% ?

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/9noTGISHUt.html

google-oss-bot · 2022-10-07T14:19:07Z

The public api surface has changed for the subproject firebase-appdistribution-api:
error: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.showFeedbackNotification(int,int) [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2022-10-11T18:11:00Z

The public api surface has changed for the subproject firebase-appdistribution-api:
error: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.requestNotificationPermissions(T) [AddedAbstractMethod]
error: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.showFeedbackNotification(CharSequence,int) [AddedAbstractMethod]
error: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.showFeedbackNotification(int,int) [AddedAbstractMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2022-10-13T14:03:39Z

Size Report ¹

Affected Products

firebase-appdistribution
Type Base (765f9c8) Merge (1d9d395) Diff
aar 154 kB 159 kB +5.03 kB (+3.3%)
apk (aggressive) 882 kB 896 kB +13.9 kB (+1.6%)
apk (release) 2.01 MB 2.01 MB +4.76 kB (+0.2%)
firebase-appdistribution-api
Type Base (765f9c8) Merge (1d9d395) Diff
aar 14.7 kB 15.8 kB +1.18 kB (+8.0%)
apk (release) 689 kB 689 kB +352 B (+0.1%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/yFZx2a9Zbz.html

firebase-appdistribution-api/api.txt

...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

.../java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionLifecycleNotifier.java

...va/com/google/firebase/appdistribution/impl/FirebaseAppDistributionNotificationsManager.java

...in/java/com/google/firebase/appdistribution/impl/TakeScreenshotAndStartFeedbackActivity.java

...pp/src/main/java/com/googletest/firebase/appdistribution/testapp/AppDistroTestApplication.kt

...ution/test-app/src/main/java/com/googletest/firebase/appdistribution/testapp/MainActivity.kt

kaibolay · 2022-10-14T03:53:18Z

...src/main/java/com/googletest/firebase/appdistribution/testapp/NotificationFeedbackTrigger.kt

@@ -221,9 +221,9 @@ class TakeScreenshotAndTriggerFeedbackActivity : Activity() {
      activity.openFileOutput(SCREENSHOT_FILE_NAME, Context.MODE_PRIVATE).use { outputStream ->
        bitmap.compress(Bitmap.CompressFormat.PNG, /* quality = */ 100, outputStream)
      }


Silly GitHub doesn't let me comment on line 213... Anyhow, should there be a finish() at the end of onResume()?

True. Out of curiosity - any reason you split the behavior between onCreate and onResume? Or could it all go in the onCreate like it is in the SDK?

No good reason. It was just the first thing that worked. So let's make it mirror what the SDK does since that's simpler.

firebase-appdistribution-api/api.txt

vkryachko · 2022-10-14T14:09:09Z

...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+   *
+   * <p>On Android 8 and above, the notification will be created in its own notification channel.
+   *
+   * @param infoTextResourceId string resource ID of text to display to the tester before collecting


Does the underlying api require a resource id? if not, do we want to restrict to only allow resource ids as opposed to allowing passing in custom strings like constants?

If resourceId is preferred, please make sure to annotate this param with @StringRes.

We alternatively allow them to pass in a CharSequence, below. Supporting resource id is mostly for convenience, so they don't have to call Context.getText() or Context.getString(). This matches the options we already provide with startFeedback(int) vs startFeedback(CharSequence). If this is creating too much clutter in the API, maybe we just support CharSequence? It's not that big a deal to call getText.

My preference is in the other direction. If we eliminate one, I'd vote for the overload with CharSequence and only keep @StringRes int. The latter is a best practice. Hard-coded strings are smelly. Or we keep both, but add JavaDoc which explains that @StringRes int is preferred.

That's a good point, Kai. I actually just noticed that if you use TextView.setText(CharSequence) you get a compiler warning: "String literal in setText can not be translated. Use Android resources instead."

So I'm tempted to remove the CharSequence option, but that would prevent the dev from using format strings: https://developer.android.com/guide/topics/resources/string-resource#formatting-strings

I feel like we should support that.

Would it make sense to add an overload that would mirror getResource(int, Object...)? i.e. something like

void showFeedbackNotification( @NonNull InterruptionLevel interruptionLevel, @StringRes int infoTextResourceId, Object... formatArgs);

We could, but if they want to use a format string AND html elements like <a> tags they would have to do some extra steps and then final pass us a styled CharSequence: https://developer.android.com/guide/topics/resources/string-resource#StylingWithHTML

This might seem like an edge case, but because this text will likely include links to terms and conditions I expect anyone who wants to use a format string will have to do this.

vkryachko · 2022-10-14T14:10:23Z

...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+   *     NotificationChannel#setImportance}. On platforms below Android 8, the importance will be
+   *     translated into a comparable notification priority (see {@link


If we are doing some translation, would it make sense to create an enum as opposed to relying on the platform constants being passed in?

I think that might make more sense, actually. I was thinking about the scenario where Android adds a new importance level. If we just accept an importance level here, the nice thing is devs could start using it immediately without any update from us. The downside is that we would fall back to PRIORITY_DEFAULT on older devices. That might be significantly higher or lower priority than expected.

While the translation is super obvious (MIN -> MIN, DEFAULT -> DEFAULT, etc) it would still be nice to be explicit.

Here's what that would look like: https://github.com/firebase/firebase-android-sdk/pull/4205/files

Let me know if you think we should go through an API review for this.

I like it (just left one comment for simplification). I don't think we need an API review now. But after EAP we should go through an API review (again) for everything. Many small details changed - and might keep changing - e.g. whether to accept CharSequence or @StringRes int or both.

vkryachko · 2022-10-14T14:11:57Z

...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+   *     translated into a comparable notification priority (see {@link
+   *     NotificationCompat.Builder#setPriority}).
+   */
+  void showFeedbackNotification(int infoTextResourceId, int importance);


For my understanding, what will happen if called from background? do Android background restrictions apply to it?

Good question - I just tested it and it works, even after navigating away from the app. This makes sense - notifications are actually the recommended way for apps to present UI to the user when in the background: https://developer.android.com/guide/components/activities/background-starts#display-notification

vkryachko · 2022-10-14T14:20:30Z

.../java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionLifecycleNotifier.java

+          // Store a reference to the previous activity in case the current activity is ignored
+          previousActivity = currentActivity;


If the current activity is an ignored one, do you even need to store it?

I should make this comment clearer - what I mean is that the currentActivity may be ignored later when applyToNullableForegroundActivity(classToIgnore, ...) is called.

@nonnull

* Add InterruptionLevel to API * Move interruption and priority levels to InterruptionLevel enum * Mark interruptionLevel as @nonnull

vkryachko · 2022-10-21T13:22:02Z

...tribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

+   *
+   * <p>On Android 8 and above, the notification will be created in its own notification channel.
+   *
+   * @param infoTextResourceId string resource ID of text to display to the tester before collecting


Would it make sense to add an overload that would mirror getResource(int, Object...)? i.e. something like

void showFeedbackNotification( @NonNull InterruptionLevel interruptionLevel, @StringRes int infoTextResourceId, Object... formatArgs);

vkryachko · 2022-10-21T13:25:26Z

.../java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionLifecycleNotifier.java

+          // Store a reference to the previous activity in case the current activity is ignored
+          // later in call to applyToNullableForegroundActivity()
+          previousActivity = currentActivity;


I'm not sure if it's actually a problem in practice in you use case, but in general holding a reference to currentActivity should be safe, while it's not necessarily the case with previousActivity given that that activity may get destroyed while currentActivity is active. Is that a possible scenario here? What would happen if you try to use that activity?

That's not a possible scenario because if previousActivity gets destroyed, we remove the reference to it on line 343 (in onActivityDestroyed).

google-oss-bot added do-not-merge/work-in-progress size/S labels Oct 7, 2022

lfkellogg force-pushed the lk/notification-trigger-api branch from db7e5c0 to db7fe42 Compare October 11, 2022 18:01

google-oss-bot added size/L and removed size/S labels Oct 11, 2022

lfkellogg force-pushed the lk/notification-trigger-api branch 2 times, most recently from 08ad235 to 1148499 Compare October 12, 2022 17:40

google-oss-bot added size/XL and removed size/L labels Oct 12, 2022

lfkellogg force-pushed the lk/notification-trigger-api branch 5 times, most recently from deb39fe to 87302ba Compare October 13, 2022 13:57

lfkellogg force-pushed the lk/notification-trigger-api branch from 87302ba to ca6ec70 Compare October 13, 2022 21:35

lfkellogg changed the title ~~WIP - Notification trigger API~~ Notification trigger API Oct 13, 2022

google-oss-bot removed the do-not-merge/work-in-progress label Oct 13, 2022

lfkellogg force-pushed the lk/notification-trigger-api branch from ca6ec70 to fd44b91 Compare October 13, 2022 21:49

Add notification default trigger to in-app feedback

ae8db00

lfkellogg force-pushed the lk/notification-trigger-api branch from fd44b91 to ae8db00 Compare October 13, 2022 21:52

lfkellogg requested a review from kaibolay October 13, 2022 21:52

kaibolay reviewed Oct 14, 2022

View reviewed changes

firebase-appdistribution-api/api.txt Outdated Show resolved Hide resolved

kaibolay reviewed Oct 14, 2022

View reviewed changes

lfkellogg added 2 commits October 14, 2022 09:48

Address Kai's feedback

50f4148

Update api.txt

d633ffb

lfkellogg commented Oct 14, 2022

View reviewed changes

firebase-appdistribution-api/api.txt Outdated Show resolved Hide resolved

lfkellogg requested a review from vkryachko October 14, 2022 14:12

vkryachko reviewed Oct 14, 2022

View reviewed changes

kaibolay approved these changes Oct 14, 2022

View reviewed changes

Address some feedback

bfa8ca1

lfkellogg mentioned this pull request Oct 14, 2022

Add InterruptionLevel to notification trigger API #4205

Merged

lfkellogg and others added 2 commits October 17, 2022 09:24

Add InterruptionLevel to notification trigger API (#4205)

dd1967f

* Add InterruptionLevel to API * Move interruption and priority levels to InterruptionLevel enum * Mark interruptionLevel as @nonnull

Add @StringRes annotation

97322d5

vkryachko reviewed Oct 21, 2022

View reviewed changes

vkryachko approved these changes Oct 21, 2022

View reviewed changes

lfkellogg merged commit 644483e into fad/in-app-feedback Oct 21, 2022

lfkellogg deleted the lk/notification-trigger-api branch October 21, 2022 14:08

firebase locked and limited conversation to collaborators Nov 21, 2022

		* NotificationChannel#setImportance}. On platforms below Android 8, the importance will be
		* translated into a comparable notification priority (see {@link

		// Store a reference to the previous activity in case the current activity is ignored
		previousActivity = currentActivity;

Notification trigger API #4183

Notification trigger API #4183

Uh oh!

Conversation

lfkellogg commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-appdistribution

firebase-appdistribution-api

Test Logs

Uh oh!

google-oss-bot commented Oct 7, 2022

Uh oh!

google-oss-bot commented Oct 11, 2022

Uh oh!

google-oss-bot commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-appdistribution

firebase-appdistribution-api

Test Logs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lfkellogg commented Oct 7, 2022 •

edited

Loading

google-oss-bot commented Oct 7, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

`firebase-appdistribution-api`

google-oss-bot commented Oct 13, 2022 •

edited

Loading

Size Report ¹

`firebase-appdistribution`

`firebase-appdistribution-api`