Added NetworkFetcher and NetworkCache customization #1629

egroden · 2020-09-11T16:33:10Z

No description provided.

LottieSnapshotBot · 2020-09-11T16:58:25Z

Snapshot Tests
28: Report Diff

otopba · 2020-09-11T17:23:37Z

@egroden Thanks for this PR. Really helpful

gpeal

Thanks for putting in some work on this!

lottie/src/main/java/com/airbnb/lottie/Lottie.java

gpeal · 2020-09-12T22:56:03Z

lottie/src/main/java/com/airbnb/lottie/Lottie.java

+  private static volatile NetworkFetcher networkFetcher;
+  private static volatile NetworkCache networkCache;
+
+  public static void initialize(@NonNull final LottieConfig lottieConfig) {


Add a private constructor so this class can't be instantiated

gpeal · 2020-09-12T23:02:20Z

lottie/src/main/java/com/airbnb/lottie/Lottie.java

+  }
+
+  @NonNull
+  public static NetworkFetcher networkFetcher(@NonNull Context context) {


Can you move these to L.java. L is where Lottie keeps static configuration for its own internal use. (Initialize should remail here though)

gpeal · 2020-09-12T23:04:02Z

lottie/src/main/java/com/airbnb/lottie/network/CacheProvider.java

+
+import androidx.annotation.NonNull;
+
+public interface CacheProvider {


Javadoc. Also indicate when getCacheDir() is called and whether it is called every time or cached.

Let's name this LottieNetworkCacheProvider
and in the docs specify that this is the cache where animations downloaded via url are saved.

gpeal · 2020-09-12T23:17:44Z

lottie/src/main/java/com/airbnb/lottie/network/Result.java

+
+import androidx.annotation.NonNull;
+
+public class Result {


Let's rename this. Result will have lots of naming conflicts with other libraries that will be annoying when trying to use autocomplete. Can you reuse LottieResult or give this class a name that's specific to it being a network result?

LottieSnapshotBot · 2020-09-14T10:43:44Z

Snapshot Tests
28: Report Diff

gpeal

Thanks for the cleanup. Looking great. Just a few small comments.

gpeal · 2020-09-14T17:08:29Z

lottie/src/main/java/com/airbnb/lottie/network/Fetcher.java

+/**
+ * Interface for custom fetcher
+ */
+public interface Fetcher {


Let's name this LottieNetworkFetcher to make it explicit.

gpeal · 2020-09-14T17:08:42Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultFetcher.java

+
+import androidx.annotation.NonNull;
+
+public class DefaultFetcher implements Fetcher {


This would become DefaultLottieNetworkFetcher

gpeal · 2020-09-14T17:10:46Z

lottie/src/main/java/com/airbnb/lottie/network/CacheProvider.java

+
+import androidx.annotation.NonNull;
+
+public interface CacheProvider {


Let's name this LottieNetworkCacheProvider
and in the docs specify that this is the cache where animations downloaded via url are saved.

LottieSnapshotBot · 2020-09-14T20:53:59Z

Snapshot Tests
28: Report Diff

LottieSnapshotBot · 2020-09-14T22:09:32Z

Snapshot Tests
28: Report Diff

lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkCacheProvider.java

gpeal · 2020-09-15T15:07:38Z

lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkFetcher.java

@@ -12,4 +12,6 @@
    @WorkerThread
    @NonNull
    LottieNetworkResult fetchSync(@NonNull String url) throws IOException;
+
+    void disconnect();


Can you provide a javadoc for people who want to use this interface? When and why would you call disconnect?
Could you also add a more complete javadoc for the class? Something like:

Implement this interface to handle network fetching manually when animations are requested via url. By default, Lottie will use an HttpUrlConnection under the hood but this enables you to hook into your own network stack. By default, Lottie will also handle caching the animations but if you want to provide your own cache directory, you may implement `LottieNetworkCacheProvider`. @see Lottie#initialize

gpeal · 2020-09-15T15:10:58Z

lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkCacheProvider.java

+import androidx.annotation.NonNull;
+
+/**
+ * Interface for providing the custom cache directory where animations downloaded via url are saved


Add a period at the end of your doc and @see Lottie#initialize so people know what to do with this once they have implemented it.

LottieSnapshotBot · 2020-09-16T09:12:38Z

Snapshot Tests
28: Report Diff

otopba · 2020-09-18T17:28:25Z

@gpeal Hi! Friendly ping

gpeal

Looks good to me! Just one more comment then I'll land this! Thanks for putting in the work and iterations.

gpeal · 2020-09-20T02:56:31Z

lottie/src/main/java/com/airbnb/lottie/LottieConfig.java

+  @Nullable final LottieNetworkFetcher networkFetcher;
+  @Nullable final LottieNetworkCacheProvider cacheProvider;
+
+  public LottieConfig(@NonNull Context applicationContext, @Nullable LottieNetworkFetcher networkFetcher, @Nullable


I think you meant for this to be private

lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkCacheProvider.java

LottieSnapshotBot · 2020-09-20T11:37:52Z

Snapshot Tests
28: Report Diff

LottieSnapshotBot · 2020-09-20T12:10:14Z

Snapshot Tests
28: Report Diff

LottieSnapshotBot · 2020-09-29T18:36:37Z

Snapshot Tests
28: Report Diff

LottieSnapshotBot · 2020-10-01T08:26:58Z

Snapshot Tests
28: Report Diff

LottieSnapshotBot · 2020-10-01T10:46:46Z

Snapshot Tests
28: Report Diff

gpeal

Getting close. We just need to clarify the new LottieFetchResult interface.

gpeal · 2020-10-01T17:03:27Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultLottieFetchResult.java

+    this.connection = connection;
+  }
+
+  @Override public boolean isSuccessful() throws IOException {


I think this can just wrap IOException and return false right?

gpeal · 2020-10-01T17:04:22Z

lottie/src/main/java/com/airbnb/lottie/network/LottieFetchResult.java

+
+import androidx.annotation.Nullable;
+
+public interface LottieFetchResult extends Closeable {


Please add a javadoc if you are adding a new public api

gpeal · 2020-10-01T17:04:38Z

lottie/src/main/java/com/airbnb/lottie/network/LottieFetchResult.java

+  boolean isSuccessful() throws IOException;
+  int resultCode() throws IOException;
+  @Nullable
+  String message() throws IOException;


What is the purpose of message?

LottieSnapshotBot · 2020-10-05T10:31:38Z

Snapshot Tests
28: Report Diff

gpeal · 2020-10-12T01:14:35Z

lottie/src/main/java/com/airbnb/lottie/network/LottieFetchResult.java

@@ -6,15 +6,39 @@

 import androidx.annotation.Nullable;

+/**
+ * The result of the operation of obtaining a lotty animation


gpeal · 2020-10-12T01:17:08Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultLottieFetchResult.java

+    return connection.getResponseCode();
+  }
+
+  @Nullable @Override public InputStream bodyByteStream() throws IOException {


Under what situation would this return null?

marked it NonNull

gpeal · 2020-10-12T01:17:50Z

lottie/src/main/java/com/airbnb/lottie/network/NetworkFetcher.java

+      if (fetchResult.isSuccessful()) {
+        InputStream inputStream = fetchResult.bodyByteStream();
+        String contentType = fetchResult.contentType();
+        LottieResult<LottieComposition> result = fromInputStream(url, inputStream, contentType, cacheKey);


You marked bodyByteStream as nullable but pass it into this not-null method. Does your IDE not warn you about this?

marked it NonNull

gpeal · 2020-10-12T01:19:16Z

lottie/src/main/java/com/airbnb/lottie/network/NetworkFetcher.java

+        try {
+          fetchResult.close();
+        } catch (IOException e) {
+          e.printStackTrace();


e.printStackTrace(); should not be called in Android. You can use Logger.warning

gpeal · 2020-10-12T01:25:47Z

lottie/src/main/java/com/airbnb/lottie/LottieConfig.java

+
+    @NonNull
+    public Builder setCacheDir(@NonNull final File file) {
+      this.cacheProvider = new LottieNetworkCacheProvider() {


Throw IllegalArgumentExcception if !file.isDirectory

Also, this.cacheProvider can be cacheProvider

gpeal · 2020-10-12T01:28:07Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultLottieFetchResult.java

+    }
+  }
+
+  @Override public int resultCode() throws IOException {


resultCode() is only used as an implementation detail of this class so you can remove it from the interface's API.

gpeal · 2020-10-12T01:29:18Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultLottieFetchResult.java

+    return connection.getContentType();
+  }
+
+  @Nullable @Override public String error() throws IOException {


When would throw throw IOException?

gpeal · 2020-10-12T01:31:15Z

lottie/src/main/java/com/airbnb/lottie/network/DefaultLottieNetworkFetcher.java

+  @Override
+  @NonNull
+  public LottieFetchResult fetchSync(@NonNull String url) throws IOException {
+    final HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection();


Could this be HttpsUrlConnection?

HttpUrlConnection was already in your library. If we change it to HttpsUrlConnection, won't anything break?

HttpsUrlConnection is also part of the Android SDK but it extends HttpUrlConnection so you're good here.

LottieSnapshotBot · 2020-10-19T09:44:07Z

Snapshot Tests
28: Report Diff

gpeal

Thanks for taking the time to write and iterate on this :) I have some other changes planned for 3.5 but this should be released pretty soon.

Denis Egorov added 5 commits September 11, 2020 18:31

added customization of NetworkFetcher and NetworkCache

fddbd40

added customization of NetworkFetcher and NetworkCache

a082a50

added customization of NetworkFetcher and NetworkCache

d9c9d4b

added customization of NetworkFetcher and NetworkCache

b5bda21

added customization of NetworkFetcher and NetworkCache

0fd9dd3

gpeal reviewed Sep 12, 2020

View reviewed changes

fix

8992855

gpeal reviewed Sep 14, 2020

View reviewed changes

fix

b452427

fix SocketTimeoutException

d3dc480

gpeal reviewed Sep 15, 2020

View reviewed changes

javadoc

2b30715

gpeal reviewed Sep 20, 2020

View reviewed changes

private constructor

edb096b

delete context

c77366d

Denis Egorov added 2 commits September 29, 2020 20:57

fix problems with connections

a76fe2b

fix problems with connections

f3a8568

fix problems with connections

b70f6a7

refactoring

d0d6723

gpeal reviewed Oct 1, 2020

View reviewed changes

refactoring

8a45420

gpeal reviewed Oct 12, 2020

View reviewed changes

refactoring

099a275

gpeal approved these changes Oct 19, 2020

View reviewed changes

gpeal merged commit 81b4e78 into airbnb:master Oct 19, 2020


		import androidx.annotation.NonNull;

		public interface CacheProvider {


		import androidx.annotation.NonNull;

		public class DefaultFetcher implements Fetcher {


		import androidx.annotation.Nullable;

		public interface LottieFetchResult extends Closeable {

Added NetworkFetcher and NetworkCache customization #1629

Added NetworkFetcher and NetworkCache customization #1629

Conversation

egroden commented Sep 11, 2020

LottieSnapshotBot commented Sep 11, 2020

otopba commented Sep 11, 2020

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Sep 14, 2020

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Sep 14, 2020

LottieSnapshotBot commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Sep 16, 2020

otopba commented Sep 18, 2020

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Sep 20, 2020

LottieSnapshotBot commented Sep 20, 2020

LottieSnapshotBot commented Sep 29, 2020

LottieSnapshotBot commented Oct 1, 2020

LottieSnapshotBot commented Oct 1, 2020

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LottieSnapshotBot commented Oct 19, 2020

gpeal left a comment

Choose a reason for hiding this comment