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

Add support to TextProviders for CoreAnimation #1723

Merged
merged 22 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
12 changes: 8 additions & 4 deletions Sources/Private/CoreAnimation/CoreAnimationLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,17 @@ final class CoreAnimationLayer: BaseAnimationLayer {
/// The `AnimationTextProvider` that `TextLayer`'s use to retrieve texts,
/// that they should use to render their text context
var textProvider: AnimationTextProvider {
didSet { reloadTexts() }
didSet {
// We need to rebuild the current animation after registering a value provider,
// since any existing `CAAnimation`s could now be out of date.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// We need to rebuild the current animation after registering a value provider,
// since any existing `CAAnimation`s could now be out of date.
// We need to rebuild the current animation after updating the text provider,
// since this is used in `TextLayer.setupAnimations(context:)`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

7a71e9a βœ…

rebuildCurrentAnimation()
}
}

/// The `FontProvider` that `TextLayer`s use to retrieve the `CTFont`
/// that they should use to render their text content
var fontProvider: AnimationFontProvider {
didSet { reloadTexts() }
didSet { reloadFonts() }
}

/// Queues the animation with the given timing configuration
Expand Down Expand Up @@ -404,8 +408,8 @@ extension CoreAnimationLayer: RootAnimationLayer {
}
}

func reloadTexts() {
// When the text (or font) provider changes, we have to update all `TextLayer`s
func reloadFonts() {
// When the text provider changes, we have to update all `TextLayer`s
// so they can query the most up-to-date font from the new font provider.
for sublayer in allSublayers {
if let textLayer = sublayer as? TextLayer {
Expand Down
8 changes: 4 additions & 4 deletions Sources/Private/CoreAnimation/Layers/TextLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ final class TextLayer: BaseCompositionLayer {
override func setupAnimations(context: LayerAnimationContext) throws {
try super.setupAnimations(context: context)

// Question 1: Which keypath to pass?
_ = try context.currentKeypath.keys.map { key in
Copy link
Member

Choose a reason for hiding this comment

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

We should pass the entire keypath here, e.g. context.currentKeypath.fullPath

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this context.currentKeypath.keys.map -- we should be able to just remove it and just have the code on the inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of it in c8a84c2 βœ…

// Question 2: Where do we get the text layer text?
let customText = context.textProvider.textFor(keypathName: key, sourceText: "text layer text")
// Question 2.1: Should we get from somewhere source text? What if the user has custom name?
let customText = context.textProvider.textFor(keypathName: context.currentKeypath.fullPath, sourceText: "text layer text")
Copy link
Member

Choose a reason for hiding this comment

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

sourceText is just the text value specified in the animation json for this layer -- so, try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what if the JSON will have a different name for this text layer?
Would there be a way to pass some specific value to configure animation for such behavior?

// Question 2.2: If we don't have this .isEmpty check, then how do we decide when to set our custom text, and when to
// set the result of the `exactlyOneKeyFrame()`?
Copy link
Member

Choose a reason for hiding this comment

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

If you look at the behavior of the main thread rendering engine, it just unconditionally sets renderLayer.text to the result of textProvider.textFor(keypathName:sourceText:). Since that method doesn't return an optional, it looks like expected usage is to just return sourceText if you don't want to customize the text for that specific keypath. That's how DefaultTextProvider works, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the method in c8a84c2 🟑

Then we just set it like this? πŸ€”

if customText.isEmpty {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this isEmpty check -- the Main Thread rendering engine doesn't have this check (

let textString = textProvider.textFor(keypathName: keypathName, sourceText: text.text)
) and it seems like a valid use case to hide a piece of text by returning an empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the method in 5055f10 🟑

Okay, I see. But then I just don't understand how do we actually decide when to set this:
renderLayer.text = customText

and when do this:

renderLayer.text = try textLayerModel.text.exactlyOneKeyframe(context: context, description: "text layer text").text? πŸ€”

Copy link
Member

@calda calda Aug 19, 2022

Choose a reason for hiding this comment

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

we always just do renderLayer.text = customText πŸ˜„ https://github.com/airbnb/lottie-ios/pull/1723/files#r950221577

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, then it is this way already in the code! πŸŽ‰

renderLayer.text = customText
} else {
Expand Down Expand Up @@ -73,7 +74,6 @@ final class TextLayer: BaseCompositionLayer {
""")
}

renderLayer.text = text.text
renderLayer.font = context.fontProvider.fontFor(family: text.fontFamily, size: CGFloat(text.fontSize))

renderLayer.alignment = text.justification.textAlignment
Expand Down
4 changes: 2 additions & 2 deletions Tests/SnapshotConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ struct SnapshotConfiguration {
var customImageProvider: AnimationImageProvider?

/// A custom `AnimationTextProvider` to use when rendering this animatino
var customTextProvider: AnimationTextProvider?
var customTextProvider: AnimationTextProvider?
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to actually apply this text provider to the AnimationView in SnapshotConfiguration.makeAnimationView: https://github.com/airbnb/lottie-ios/blob/master/Tests/SnapshotTests.swift#L255

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool to know, thanks!
Done in c7a21ce βœ…


/// A custom `AnimationFontProvider` to use when rendering this animation
var customFontProvider: AnimationFontProvider?
Expand Down Expand Up @@ -74,7 +74,7 @@ extension SnapshotConfiguration {
"Nonanimating/_dog": .customImageProvider(HardcodedImageProvider(imageName: "Samples/Images/dog.png")),

// Test cases for `AnimatedTextProvider`
"": .customTextProvider(HardcodedTextProvider()),
"Issues/issue_1722": .customTextProvider(HardcodedTextProvider(text: "Bounce-bounce")),
Copy link
Member

Choose a reason for hiding this comment

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

When you're done with the code changes you can generate the snapshot images for this by running the unit test target on an iPhone 8 simulator (or by running bundle exec rake test:package on the command line).

Copy link
Member

Choose a reason for hiding this comment

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

fyi I think the snapshot tests are sensitive to specific Xcode versions -- right now I think they run correctly on Xcode 13.4.1 but not on Xcode 14. If you get a lot of spurious failures when you run the tests let me know and I can pull you branch and run them for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm unfortunately it always return this message during the test run:
No reference was found on disk. Automatically recorded snapshot:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you get a lot of spurious failures when you run the tests let me know and I can pull you branch and run them for you.

Yeah, Cal, that would be helpful, thanks a bunch! πŸ™πŸΌ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way the name of the text layer in the provided animation is Animation text layer πŸ™πŸΌ

Copy link
Member

Choose a reason for hiding this comment

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

Hm unfortunately it always return this message during the test run:
No reference was found on disk. Automatically recorded snapshot:

This is expected the first time you run the tests after you add a new sample JSON file. If you run the tests again this goes away, since now it compares the new snapshot with the previously-recorded one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm true. However, the snapshots anyway don't contain the text. Could it be because the text layer has a different name?

Copy link
Member

Choose a reason for hiding this comment

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

I think this was because we had to call textRenderLayer.sizeToFit() after updating textRenderLayer.text.


// Test cases for `AnimationFontProvider`
"Nonanimating/Text_Glyph": .customFontProvider(HardcodedFontProvider(font: UIFont(name: "Chalkduster", size: 36)!)),
Expand Down
13 changes: 7 additions & 6 deletions Tests/Utils/HardcodedTextProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ import UIKit

/// An `AnimationTextProvider` that always returns a specific hardcoded text
class HardcodedTextProvider: AnimationTextProvider {
// Question 4: What should we actually return here?
private let text: String

init(text: String) {
self.text = text
}

func textFor(keypathName: String, sourceText: String) -> String {
#if os(iOS)
return "text layer text"
#else
return nil
#endif
text
}
}