Initial code structure for App Distribution Android SDK #2754
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (10f53ba5) is created by Prow via merging commits: 202af1d 013e422. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (10f53ba5) is created by Prow via merging commits: 202af1d 013e422. |
Macrobenchmark ReportAffected SDKsMeasurements are for head commit (45167b3). Diffing against base commit (563781c) is working in progress.
|
|
/retest |
1 similar comment
|
/retest |
|
/retest |
...stribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionApi.java
Outdated
Show resolved
Hide resolved
...stribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionApi.java
Outdated
Show resolved
Hide resolved
| * @param updateProgressListener a callback function invoked as the update progresses | ||
| */ | ||
| @NonNull | ||
| public abstract Task<Void> updateApp(@Nullable UpdateProgressListener updateProgressListener); |
There was a problem hiding this comment.
it's a bit strange to me that lack of update is an exception, do you have an api review doc I can take a look at?
There was a problem hiding this comment.
This method is different from updateToLatestRelease, because it should be called only after checkForUpdate returns a truthy value. If it is called when no release is available, we'd throw the exception.
We'll share the API proposal with you over chat
| * @param updateProgressListener a callback function invoked as the update progresses | ||
| */ | ||
| @NonNull | ||
| public abstract Task<Void> updateApp(@Nullable UpdateProgressListener updateProgressListener); |
There was a problem hiding this comment.
This method is different from updateToLatestRelease, because it should be called only after checkForUpdate returns a truthy value. If it is called when no release is available, we'd throw the exception.
We'll share the API proposal with you over chat
...stribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionApi.java
Outdated
Show resolved
Hide resolved
|
/retest |
1 similar comment
|
/retest |
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
...tion/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistributionRegistrar.java
Outdated
Show resolved
Hide resolved
|
/retest |
|
/test device-check-changed |
1 similar comment
|
/test device-check-changed |
|
/test device-check-changed |
...p-distribution/src/main/java/com/google/firebase/appdistribution/AppDistributionRelease.java
Outdated
Show resolved
Hide resolved
...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
Outdated
Show resolved
Hide resolved
firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/UpdateProgress.java
Outdated
Show resolved
Hide resolved
| * @param updateProgressListener a callback function invoked as the update progresses | ||
| */ | ||
| @NonNull | ||
| public Task<Void> updateApp(@Nullable UpdateProgressListener updateProgressListener) { |
There was a problem hiding this comment.
This API looks a bit suspicious, on the one hand it's returning a task indicating that it will succeed in the future, otoh it also takes a listener to monitor progress.
We have a pattern for such APIs, e.g. storage uses CancellableTask that allows to listen to progress updates. It would be beneficial to do something similar here. cc @schmidt-sebastian
There was a problem hiding this comment.
Could you speak more about why returning a task is at odds with providing a progress listener? The idea behind a progress listener was to provide customers the flexibility of listening to progress updates (in our case APK download progress). Something like the CancellableTask is reasonable but we don't want to introduce update cancellation functionality yet.
There was a problem hiding this comment.
To add more context to the API - updateApp is a Task< Void > because we didn't think there is any actionable data to be returned to the customer if that task succeeds. But customers might be interested in knowing the progress of the current update (maybe to present a custom UI)
There was a problem hiding this comment.
We discussed this offline. We plan to update this method to return an UpdateTask, and follow the same pattern as the CancellableTask (but without providing cancellation functionality)
Change data classes to public final classes Add UpdateTask class Update FirebaseAppDistributionException to use an enum instead of an IntDef
| * updateToLatestRelease() | ||
| */ | ||
| public interface AppDistributionRelease { | ||
| public final class AppDistributionRelease { |
There was a problem hiding this comment.
non-blocking nit: you may want to add equals() and hashCode(), or use an @AutoValue altogether.
There was a problem hiding this comment.
Good call. We will add these in a follow-up PR
Initial commit for App Distribution new build alerts Android SDK