LottieCompositionFactory: Avoid NPE when animation contains a Font Family and Context is null. #2530
Conversation
…mily and Context is null.
vanniktech
force-pushed
the
fromZipStreamSyncInternal-context
branch
from
051a6b0
to
6caf73f
Compare
| @@ -538,7 +538,7 @@ public static LottieResult<LottieComposition> fromZipStreamSync(@Nullable Contex | |||
| } | |||
|
|
|||
| @WorkerThread | |||
| private static LottieResult<LottieComposition> fromZipStreamSyncInternal(Context context, ZipInputStream inputStream, @Nullable String cacheKey) { | |||
| private static LottieResult<LottieComposition> fromZipStreamSyncInternal(@Nullable Context context, ZipInputStream inputStream, @Nullable String cacheKey) { | |||
There was a problem hiding this comment.
My concern here is that a lot of animations are loaded from the server and this may lead to runtime crashes where the engineer doesn't pass context and then somebody else at some point in the future sends an animation that has fonts and then it leads to crashes. It's a tough situation to recover from because they can fix it but won't be able to add the animation because it'll continue to crash older apps.
Given that this can be an application context, what is the use case in which you can't get access to at least the application context?
There was a problem hiding this comment.
This crash would also happen right now with the current code that we have since the line after:
File tempFile = new File(context.getCacheDir(), fileName);would just crash with a NPE. I just saw this and wanted to improve the behavior by at least telling you why it didn't succeed. Do you want me to throw instead of returning a LottieResult?
There was a problem hiding this comment.
I see. IIRC, the intention here was that context is @NonNull, There is an overloaded version that omits the context parameter and clearly states in the docs that fonts will not be loaded.
There was a problem hiding this comment.
clearly states in the docs that fonts will not be loaded.
This isn't the case though as it will crash.
No description provided.