Add possibility to remove ValueProvider #2474

batanus · 2024-09-06T14:40:06Z

Summary

Previously, when the animation property was set, all ValueProviders for AnimatioView were reset (see #920). Now it's not so (see #2390)

calda · 2024-09-08T16:33:36Z

Sources/Public/Animation/LottieAnimationView.swift

@@ -673,6 +673,12 @@ open class LottieAnimationView: LottieAnimationViewBase {
    lottieAnimationLayer.setValueProvider(valueProvider, keypath: keypath)
  }

+  /// Sets a ValueProvider for the specified keypath. The value provider will be removed
+  /// on all properties that match the keypath.
+  public func removeValueProvider(keypath: AnimationKeypath) {


A better name for this would probably be:

Suggested change

public func removeValueProvider(keypath: AnimationKeypath) {

public func removeValueProvider(for keypath: AnimationKeypath) {

calda · 2024-09-08T16:37:15Z

Sources/Private/CoreAnimation/ValueProviderStore.swift

@@ -40,6 +40,11 @@ final class ValueProviderStore {
    valueProviders.append((keypath: keypath, valueProvider: valueProvider))
  }

+  /// Removes all ValueProviders for the given `AnimationKeypath`
+  func removeValueProvider(keypath: AnimationKeypath) {
+      valueProviders.removeAll(where: { $0.keypath == keypath })


What about if the given keypath is something like Layer.*? Should it remove key paths like Layer.Color? It seems like that probably wouldn't happen with the current implementation using ==.

Could you please add a test case for this to ValueProvidersTests.swift, with some cases like that where the the key path being removed isn't directly present in the list of providers and instead matches using wildcards?

Is that behavior really expected? The method is named removeValueProvider, which suggests that it should only remove the exact value provider for the specified keyPath, and not multiple ValueProviders.
Another point - in setValueProvider the value providers are removed the same way as is implemented in this method (see here - https://github.com/airbnb/lottie-ios/blob/master/Sources/Private/CoreAnimation/ValueProviderStore.swift#L39). If you anyway would like to have functionality for wildcard case (like Layer.*) - I'm not sure how exactly it should work, so help would be wanted.
I added test for the current removeValueProvider implementation.

Thank you

Is that behavior really expected?

Good question, I can see it making sense for the behavior to stay as-is in the Core Animation rendering engine.

However, I'm pretty sure that this behavior that I describe is the current behavior implemented in this PR for the Main Thread rendering engine. We should make sure the two implementations are consistent.

calda · 2024-09-08T16:40:31Z

You'll also need to run bundle exec rake format:swift, which automatically reformats the code to follow Airbnb's Swift Style Guide.

calda · 2024-09-09T13:47:43Z

Sources/Private/MainThread/LayerContainers/MainThreadAnimationLayer.swift

@@ -241,6 +241,17 @@ final class MainThreadAnimationLayer: CALayer, RootAnimationLayer {
    }
  }

+  func removeValueProvider(for keypath: AnimationKeypath) {
+    for layer in animationLayers {
+      if let foundProperties = layer.nodeProperties(for: keypath) {


Like I mentioned in #2474 (comment), I think this behavior is different than the behavior implemented for the Core Animation rendering engine in ValueProviderStore.

layer.nodeProperties(for: keypath) returns all of the properties that match the given key path, so for example if you add a provider with Layer.Color and then call removeValueProvider(for: "**.Color"), this implementation will remove the provider for Layer.Color (but the implementation in the Core Animation rendering engine will not).

I don't have a strong preference on which approach is better (removing all matches, or just removing exact matches) but I think the behavior of the Core Animation engine and Main Thread engine needs to be the same.

I have a feeling updating the behavior of ValueProviderStore to check for matches (using keypath.matches(...) instead of ==) would be easier than trying to change the implementation here to check for exact matches on the original input key path (which I don't think we have access to anymore).

Thanks, @calda, for your assistance!

calda · 2024-09-10T15:16:19Z

Tests/ValueProvidersTests.swift

+      context: animationContext)
+    XCTAssertNil(keyFramesQuery5)
+
+    // Test removing wildcard keypath


calda · 2024-09-10T15:16:37Z

Sources/Private/CoreAnimation/ValueProviderStore.swift

@@ -40,6 +40,11 @@ final class ValueProviderStore {
    valueProviders.append((keypath: keypath, valueProvider: valueProvider))
  }

+  /// Removes all ValueProviders for the given `AnimationKeypath`
+  func removeValueProvider(for keypath: AnimationKeypath) {
+    valueProviders.removeAll(where: { $0.keypath.matches(keypath) })


calda · 2024-09-10T15:17:15Z

Sources/Public/Animation/LottieAnimationLayer.swift

+  public func removeValueProvider(for keypath: AnimationKeypath) {
+    guard let animationLayer = rootAnimationLayer else { return }
+
+    valueProviders[keypath] = nil


I guess we should also do something like this here:

Suggested change

valueProviders[keypath] = nil

valueProviders.removeAll(where: { $0.key.matches(keypath) })

The removeAll(where:) method isn't available for Dictionaries, so I used .forEach with a necessary check. Thank you 👍

calda

Thanks for the update and test! Just one last comment before we merge this: https://github.com/airbnb/lottie-ios/pull/2474/files#r1752184422

calda

Thank you!

Add removeValueProvider method

e33fdb5

batanus changed the title ~~Add possibility to Remove ValueProvider~~ Add possibility to remove ValueProvider Sep 6, 2024

Naveen-C-Ramachandrappa approved these changes Sep 6, 2024

View reviewed changes

calda reviewed Sep 8, 2024

View reviewed changes

batanus added 3 commits September 9, 2024 11:39

Update removeValueProvider signature

fed12ef

Add Unit test

fcd4fca

Run rake format

b55cedb

calda reviewed Sep 9, 2024

View reviewed changes

batanus added 3 commits September 10, 2024 10:55

Use keypath.matches instead of ==

2e4503c

Add test for wildcard keypath removal

7cee759

Revert Package.resolved changes

cd0f3db

calda reviewed Sep 10, 2024

View reviewed changes

Tests/ValueProvidersTests.swift

context: animationContext)

XCTAssertNil(keyFramesQuery5)

// Test removing wildcard keypath

Copy link

Member

calda Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

calda reviewed Sep 10, 2024

View reviewed changes

calda approved these changes Sep 10, 2024

View reviewed changes

Remove value providers for matching keyPath

03c9113

calda approved these changes Sep 10, 2024

View reviewed changes

calda merged commit 7a8279b into airbnb:master Sep 10, 2024
12 checks passed

calda mentioned this pull request Sep 10, 2024

AnimationKeypath and ValueProvider not work as previous version #2428

Open

batanus deleted the add-possibility-to-remove-value-provider branch September 10, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to remove ValueProvider #2474

Add possibility to remove ValueProvider #2474

batanus commented Sep 6, 2024

calda Sep 8, 2024

calda Sep 8, 2024

calda Sep 8, 2024

batanus Sep 9, 2024

calda Sep 9, 2024

calda commented Sep 8, 2024

calda Sep 9, 2024

calda Sep 9, 2024

calda Sep 9, 2024

batanus Sep 10, 2024

calda Sep 10, 2024

calda Sep 10, 2024

calda Sep 10, 2024 •

edited

Loading

batanus Sep 10, 2024

calda left a comment

calda left a comment

	public func removeValueProvider(keypath: AnimationKeypath) {
	public func removeValueProvider(for keypath: AnimationKeypath) {

	valueProviders[keypath] = nil
	valueProviders.removeAll(where: { $0.key.matches(keypath) })

Add possibility to remove ValueProvider #2474

Add possibility to remove ValueProvider #2474

Conversation

batanus commented Sep 6, 2024

Summary

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

calda commented Sep 8, 2024

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

calda Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda left a comment

Choose a reason for hiding this comment

calda left a comment

Choose a reason for hiding this comment

calda Sep 10, 2024 •

edited

Loading