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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Nov 2, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c21891
Commit with local test passing
greggiacovelli Oct 22, 2021
a9bcd9b
Expanding TextDelegate to provide similar functionality as iOS TextPr…
greggiacovelli Oct 22, 2021
c5ea9b4
Moving to using keyPathName as string to reduce allocations and be mo…
greggiacovelli Oct 22, 2021
52e1a29
Closes #1932 Adds context of keyPathName to TextDelegate and be more …
greggiacovelli Oct 22, 2021
2099504
Use layer name instead of keypathName
greggiacovelli Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions lottie/src/main/java/com/airbnb/lottie/TextDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
public class TextDelegate {

private final Map<String, String> stringMap = new HashMap<>();

@Nullable private final LottieAnimationView animationView;
@Nullable private final LottieDrawable drawable;
private boolean cacheText = true;
Expand All @@ -41,6 +42,17 @@ public TextDelegate(@SuppressWarnings("NullableProblems") LottieDrawable drawabl
animationView = null;
}

/**
* Override this to replace the animation text with something dynamic. This can be used for
* translations or custom data.
* @param keyPathName the name of the layer with text
* @param input the string at the layer with text
* @return a String to use for the specific data, by default this is the same as getText(input)
*/
public String getText(String keyPathName, String input) {
return getText(input);
}

/**
* Override this to replace the animation text with something dynamic. This can be used for
* translations or custom data.
Expand Down Expand Up @@ -82,11 +94,11 @@ public void invalidateAllText() {
}

@RestrictTo(RestrictTo.Scope.LIBRARY)
public final String getTextInternal(String input) {
public final String getTextInternal(String keyPathName, String input) {
if (cacheText && stringMap.containsKey(input)) {
return stringMap.get(input);
}
String text = getText(input);
String text = getText(keyPathName, input);
if (cacheText) {
stringMap.put(input, text);
}
Expand Down
3 changes: 2 additions & 1 deletion lottie/src/main/java/com/airbnb/lottie/model/layer/TextLayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.airbnb.lottie.model.DocumentData.Justification;
import com.airbnb.lottie.model.Font;
import com.airbnb.lottie.model.FontCharacter;
import com.airbnb.lottie.model.KeyPath;
import com.airbnb.lottie.model.animatable.AnimatableTextProperties;
import com.airbnb.lottie.model.content.ShapeGroup;
import com.airbnb.lottie.utils.Utils;
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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

}
fillPaint.setTypeface(typeface);
float textSize;
Expand Down