Implement collectAndSendFeedback() #3816

lfkellogg · 2022-06-15T16:33:19Z

Adds FeedbackSender as a new Component, so that it can be accessed by the FeedbackActivity.

Screencast (submit fails because the upload URL mapping has not rolled out to GFE yet): https://screencast.googleplex.com/cast/NTQ2NDIyNjkzNzE3NjA2NHw0M2YwNjUxYS03Nw

google-oss-bot · 2022-06-15T16:38:49Z

Coverage Report ¹

Affected Products

`firebase-appdistribution`

Overall coverage changed from 79.73% (0dd2be1) to 78.21% (164729d) by -1.52%.

Filename	Base (`0dd2be1`)	Merge (164729d)	Diff
FeedbackActivity.java	?	0.00%	?
FeedbackSender.java	?	87.50%	?
FirebaseAppDistributionImpl.java	94.92%	93.18%	-1.74%
FirebaseAppDistributionRegistrar.java	92.59%	81.08%	-11.51%
ReleaseIdentifier.java	88.16%	88.00%	-0.16%
ScreenshotTaker.java	?	66.67%	?

firebase-appdistribution-api
Overall coverage changed from 64.08% (0dd2be1) to 62.26% (164729d) by -1.81%.
Filename Base (0dd2be1) Merge (164729d) Diff
FirebaseAppDistributionProxy.java 100.00% 84.62% -15.38%
FirebaseAppDistributionStub.java 83.33% 81.40% -1.94%

Test Logs

Notes

Commit (164729d) is created by Prow via merging PR base commit (0dd2be1) and head commit (dd88656).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

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

google-oss-bot · 2022-06-15T16:41:53Z

The public api surface has changed for the subproject firebase-appdistribution-api:
warning: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.collectAndSendFeedback() [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-06-15T16:45:14Z

The public api surface has changed for the subproject firebase-appdistribution-api:
warning: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.collectAndSendFeedback() [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-06-15T16:48:40Z

Size Report ¹

Affected Products

firebase-appdistribution
Type Base (0dd2be1) Merge (164729d) Diff
aar 129 kB 146 kB +17.2 kB (+13.4%)
apk (aggressive) 751 kB 878 kB +127 kB (+16.9%)
apk (release) 1.59 MB 2.01 MB +415 kB (+26.1%)
firebase-appdistribution-api
Type Base (0dd2be1) Merge (164729d) Diff
aar 14.4 kB 14.5 kB +96 B (+0.7%)
apk (release) 697 kB 697 kB +76 B (+0.0%)

Test Logs

Notes

Commit (164729d) is created by Prow via merging PR base commit (0dd2be1) and head commit (dd88656).

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

google-oss-bot · 2022-06-15T16:52:08Z

The public api surface has changed for the subproject firebase-appdistribution-api:
warning: Added method com.google.firebase.appdistribution.FirebaseAppDistribution.collectAndSendFeedback() [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.

...tion/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

kaibolay · 2022-06-15T17:20:56Z

...src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java

@@ -46,23 +46,39 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
        Component.builder(FirebaseAppDistribution.class)
            .add(Dependency.required(FirebaseApp.class))
            .add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
+            .add(Dependency.required(FeedbackSender.class))


Is component registry intended as a general purpose dependency injection framework?
If so, why use it just for FeedbackSender and not for things like FirebaseAppDistributionTesterApiClient, ReleaseIdentifier, etc.?

It's not. In this case the FeedbackActivity needs to get an instance of the class, which depends on a Provider and thus needs to be instantiated by the Firebase Components framework.

Not sure if this is the best way to do this though. I'll reach out to the ACore folks.

why does FirebaseAppDistribution actually depend on FeedbackSender? I don't see it used inside buildFirebaseAppDistribution

I was thinking that this captures the dependency that FeedbackActivity (launched by FirebaseAppDistribution) has on FeedbackSender... (EDIT: thinking more now I guess that doesn't make sense, disregard)

kaibolay · 2022-06-15T17:24:37Z

...-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java

+
+  /** Take a screenshot of the running host app. */
+  Task<Bitmap> takeScreenshot() {
+    // TODO(lkellogg): Actually take a screenshot


See http://google3/java/com/google/android/gmscore/integ/client/feedback/src/com/google/android/gms/feedback/FeedbackClient.java?rcl=433730242&l=148-188

Seems pretty straightforward, but you need an Activity or a View

Nice! We can get an activity using FirebaseAppDistributionActivityLifecycleNotifier.applyToForegroundActivity().

TODO: shorten the name of that class :)

I think we can add that in a followup.

...src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java

jladieu · 2022-06-15T18:16:37Z

...e-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FeedbackSender.java

+    this.testerApiClient = testerApiClient;
+  }
+
+  @NonNull


I noticed you removed this annotation in UpdateProgress. How do you decide it's needed here?

It wasn't needed in UpdateProgress because those return types (long) were not nullable.

Actually I don't think this is necessary either. In this case it's because this is not a publicly exposed API. I'll remove it.

jladieu · 2022-06-15T18:18:32Z

firebase-appdistribution/src/main/res/layout/activity_feedback.xml

@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="utf-8"?>


Is this file generated somehow?

No I built this. It's a very basic prototype UI. You can see it in the screencast in the PR description.

kaibolay · 2022-06-15T23:11:53Z

Task :firebase-appdistribution-api:verifyGoogleJavaFormat FAILED

The following files are not formatted properly:
/home/prow/go/src/github.com/firebase/firebase-android-sdk/firebase-appdistribution-api/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

kaibolay · 2022-06-15T23:12:35Z

/test smoke-tests

Add FeedbackActivity

792e775

google-oss-bot added the size/L label Jun 15, 2022

lfkellogg force-pushed the lk/feedback-activity branch from bdb53b8 to 6d5b7d1 Compare June 15, 2022 16:37

Add FeedbackSender and tests

51c8604

lfkellogg force-pushed the lk/feedback-activity branch from 6d5b7d1 to 51c8604 Compare June 15, 2022 16:42

lfkellogg requested review from kaibolay and rachaprince June 15, 2022 16:42

kaibolay reviewed Jun 15, 2022

View reviewed changes

Update api.txt

7307517

rachaprince reviewed Jun 15, 2022

View reviewed changes

...src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java Show resolved Hide resolved

Move FeedbackSender instantiation to getInstance()

05d4642

lfkellogg force-pushed the lk/feedback-activity branch from 861b763 to 05d4642 Compare June 15, 2022 17:46

Formatting

a99baec

jladieu approved these changes Jun 15, 2022

View reviewed changes

lfkellogg added 2 commits June 15, 2022 14:24

Add javadoc

791c68f

Remove a @nullable annotation and an unnecessary dependency

edbbc6f

lfkellogg requested review from vkryachko, kaibolay and rachaprince June 15, 2022 20:16

kaibolay approved these changes Jun 15, 2022

View reviewed changes

Formatting

dd88656

lfkellogg merged commit bd94e28 into master Jun 16, 2022

lfkellogg deleted the lk/feedback-activity branch June 16, 2022 16:58

firebase locked and limited conversation to collaborators Jul 17, 2022

Implement collectAndSendFeedback() #3816

Implement collectAndSendFeedback() #3816

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-bot commented Jun 15, 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

Notes

Uh oh!

google-oss-bot commented Jun 15, 2022

Uh oh!

google-oss-bot commented Jun 15, 2022

Uh oh!

google-oss-bot commented Jun 15, 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

Notes

Uh oh!

google-oss-bot commented Jun 15, 2022

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!

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

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!

kaibolay commented Jun 15, 2022

Uh oh!

kaibolay commented Jun 15, 2022

Uh oh!

Uh oh!

lfkellogg commented Jun 15, 2022 •

edited

Loading

google-oss-bot commented Jun 15, 2022 •

edited

Loading

Coverage Report ¹

`firebase-appdistribution`

`firebase-appdistribution-api`

google-oss-bot commented Jun 15, 2022 •

edited

Loading

Size Report ¹

`firebase-appdistribution`

`firebase-appdistribution-api`

lfkellogg Jun 15, 2022 •

edited

Loading