-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added NetworkFetcher and NetworkCache customization #1629
Added NetworkFetcher and NetworkCache customization #1629
Conversation
@egroden Thanks for this PR. Really helpful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting in some work on this!
private static volatile NetworkFetcher networkFetcher; | ||
private static volatile NetworkCache networkCache; | ||
|
||
public static void initialize(@NonNull final LottieConfig lottieConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a private constructor so this class can't be instantiated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
@NonNull | ||
public static NetworkFetcher networkFetcher(@NonNull Context context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these to L.java
. L
is where Lottie keeps static configuration for its own internal use. (Initialize should remail here though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
import androidx.annotation.NonNull; | ||
|
||
public interface CacheProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc. Also indicate when getCacheDir()
is called and whether it is called every time or cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name this LottieNetworkCacheProvider
and in the docs specify that this is the cache where animations downloaded via url are saved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
import androidx.annotation.NonNull; | ||
|
||
public class Result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup. Looking great. Just a few small comments.
/** | ||
* Interface for custom fetcher | ||
*/ | ||
public interface Fetcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name this LottieNetworkFetcher
to make it explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
import androidx.annotation.NonNull; | ||
|
||
public class DefaultFetcher implements Fetcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would become DefaultLottieNetworkFetcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
import androidx.annotation.NonNull; | ||
|
||
public interface CacheProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name this LottieNetworkCacheProvider
and in the docs specify that this is the cache where animations downloaded via url are saved.
lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkCacheProvider.java
Show resolved
Hide resolved
@@ -12,4 +12,6 @@ | |||
@WorkerThread | |||
@NonNull | |||
LottieNetworkResult fetchSync(@NonNull String url) throws IOException; | |||
|
|||
void disconnect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
import androidx.annotation.NonNull; | ||
|
||
/** | ||
* Interface for providing the custom cache directory where animations downloaded via url are saved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@gpeal Hi! Friendly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just one more comment then I'll land this! Thanks for putting in the work and iterations.
@Nullable final LottieNetworkFetcher networkFetcher; | ||
@Nullable final LottieNetworkCacheProvider cacheProvider; | ||
|
||
public LottieConfig(@NonNull Context applicationContext, @Nullable LottieNetworkFetcher networkFetcher, @Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant for this to be private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
lottie/src/main/java/com/airbnb/lottie/network/LottieNetworkCacheProvider.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close. We just need to clarify the new LottieFetchResult interface.
this.connection = connection; | ||
} | ||
|
||
@Override public boolean isSuccessful() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can just wrap IOException and return false right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
import androidx.annotation.Nullable; | ||
|
||
public interface LottieFetchResult extends Closeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a javadoc if you are adding a new public api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
boolean isSuccessful() throws IOException; | ||
int resultCode() throws IOException; | ||
@Nullable | ||
String message() throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted it
@@ -6,15 +6,39 @@ | |||
|
|||
import androidx.annotation.Nullable; | |||
|
|||
/** | |||
* The result of the operation of obtaining a lotty animation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lottie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return connection.getResponseCode(); | ||
} | ||
|
||
@Nullable @Override public InputStream bodyByteStream() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what situation would this return null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marked it NonNull
if (fetchResult.isSuccessful()) { | ||
InputStream inputStream = fetchResult.bodyByteStream(); | ||
String contentType = fetchResult.contentType(); | ||
LottieResult<LottieComposition> result = fromInputStream(url, inputStream, contentType, cacheKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You marked bodyByteStream as nullable but pass it into this not-null method. Does your IDE not warn you about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marked it NonNull
try { | ||
fetchResult.close(); | ||
} catch (IOException e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.printStackTrace();
should not be called in Android. You can use Logger.warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
@NonNull | ||
public Builder setCacheDir(@NonNull final File file) { | ||
this.cacheProvider = new LottieNetworkCacheProvider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw IllegalArgumentExcception
if !file.isDirectory
Also, this.cacheProvider
can be cacheProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
} | ||
|
||
@Override public int resultCode() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultCode() is only used as an implementation detail of this class so you can remove it from the interface's API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return connection.getContentType(); | ||
} | ||
|
||
@Nullable @Override public String error() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would throw throw IOException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
@Override | ||
@NonNull | ||
public LottieFetchResult fetchSync(@NonNull String url) throws IOException { | ||
final HttpURLConnection connection = (HttpURLConnection) new URL(url).openConnection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be HttpsUrlConnection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpUrlConnection was already in your library. If we change it to HttpsUrlConnection, won't anything break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpsUrlConnection is also part of the Android SDK but it extends HttpUrlConnection so you're good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
No description provided.