move MergePath flag to new FeatureFlag API #2
Conversation
Fixes airbnb#2493 Co-authored-by: chaoluo10 <chaoluo10@iflytek.com>
Currently, I am encountering a similar issue to [airbnb#1200](airbnb#1200), but in the case where the asset is a base64 image, it has not been fixed yet.
LottieAnimationView extends AppCompatImageView, therefore the latter is part of the former's ABI.
| * Merge paths currently don't work if the the operand shape is entirely contained within the | ||
| * first shape. If you need to cut out one shape from another shape, use an even-odd fill type | ||
| * instead of using merge paths. | ||
| */ | ||
| public void enableMergePathsForKitKatAndAbove(boolean enable) { |
There was a problem hiding this comment.
We will probably deprecate this method and incentivise use of the more generic enableFeatureFlag function. I was wondering if we pipe it in through here LottieDrawable, or do we do all of our flag setting through LottieComposition.
There was a problem hiding this comment.
Settled on LottieDrawable to allow different drawables different flags.
| @@ -142,7 +142,8 @@ private enum OnVisibleAction { | |||
| FontAssetDelegate fontAssetDelegate; | |||
| @Nullable | |||
| TextDelegate textDelegate; | |||
| private boolean enableMergePaths; | |||
| private LottieFeatureFlags lottieFeatureFlags = LottieFeatureFlags.getInstance(); | |||
There was a problem hiding this comment.
I agree that this should be on a per-composition basis. However, I think we should create a new instance of the flags rather than having a singleton because you may want a different set of flags on different drawables.
We'll also need to mirror this API for compose but I can help with that once we've settled on something.
| * first shape. If you need to cut out one shape from another shape, use an even-odd fill type | ||
| * instead of using merge paths. | ||
| */ | ||
| MergePath_19(Build.VERSION_CODES.KITKAT); |
There was a problem hiding this comment.
I'd like to avoid underscores in the name if possible. Maybe MergePathsApi19?
|
|
||
| @SuppressLint("DefaultLocale") | ||
| public void enableFlag(FeatureFlag flag, boolean enable) { | ||
| if (Objects.equals(flagValues.get(flag), enable)) { |
There was a problem hiding this comment.
Does this avoid autoboxing of a nullable boolean or does it always autobox boolean into Boolean because the equals param is Object?
There was a problem hiding this comment.
Ack. It did work to avoid the Boolean Object issues, but I changed to a Set backing (if the flag is present, it is enabled).
No description provided.