Skip to content
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

Provide a second overload to TextDelegate.getText that provides layerName #1931

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

greggiacovelli
Copy link
Contributor

@greggiacovelli greggiacovelli commented Oct 22, 2021

In the iOS version of lottie, the AnimationTextProvider can look up text with the context of the keypath and the text within the layer. This doesn't seem to be possible in the Android version.

Closes #1932
Given the following example though there doesn't seem to be a way to address the following pseudo animation the same way across platforms.

layer_1 with text "READ IT" // Let's say in context it's a commanding statement
layer_2 with text "READ IT" // Let's say in context it's a statement representing the past

The default implementations aside (DictionaryTextProvider and TextDelegate), if an implementation was needed to disambiguate strings that were duplicated inside an animation, this would not be possible in the Android library today simply because getText(String input) is all that is provided. This just expands getText to also have access to the keyPath.

I am not sure if this has already been covered so please refer me to such past decisions if this is an inappropriate pull request.

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@@ -245,7 +246,7 @@ private void drawTextWithFont(
String text = documentData.text;
TextDelegate textDelegate = lottieDrawable.getTextDelegate();
if (textDelegate != null) {
text = textDelegate.getTextInternal(text);
text = textDelegate.getTextInternal(getName(), text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't a key path name. This is the layerName. Key path would be the fully qualified list of elements up to the root composition

Copy link
Contributor Author

@greggiacovelli greggiacovelli Nov 2, 2021

Choose a reason for hiding this comment

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

I see that, if I am understanding this is similar to iOS though looking at the implementation. It is only the layer name. Please let me know what would be best

Copy link
Contributor Author

@greggiacovelli greggiacovelli Nov 2, 2021

Choose a reason for hiding this comment

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

For more context, this is what I am referring to.

https://github.com/airbnb/lottie-ios/blob/64564c12ba0c258f5a5d9b1e81455b1f1da2315d/lottie-swift/src/Private/LayerContainers/CompLayers/CompositionLayer.swift#L63

CompositionLayer {
  init(layer: LayerModel, size: CGSize) {
      ...
      self.keypathName = layer.name
      ...
  }
}

I didn't think this was the true keypath either, but just a string representing the layer name, I was just trying to use consistent naming between the codebases

Copy link
Collaborator

Choose a reason for hiding this comment

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

@greggiacovelli Interesting. It does seem like iOS is using the name slightly inconsistently. For the same of consistency within the Android library, I'd prefer layerName here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will do. Thanks for the guidance and I will append the PR in a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 2099504

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Thanks!

@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

@gpeal gpeal changed the title Gives TextDelegate similar context as iOS AnimationTextProvider Provide a second overload to TextDelegate.getText that provides layerName Nov 2, 2021
@gpeal gpeal merged commit 8fd567b into airbnb:master Nov 2, 2021
@greggiacovelli greggiacovelli deleted the allow-text-delegate-more-info branch February 11, 2022 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Keypath to TextDelegate in order to have same cross platform behavior as iOS AnimationTextProvider.
3 participants