ronantakizawa/codereview-bench huggingface dataset:
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Animations/VisibilityAnimation.swift (Swift)

Code before:
    outFrame: AnimationFrameTime,
    context: LayerAnimationContext)
  {
    let animation = CAKeyframeAnimation(keyPath: "hidden")
    animation.calculationMode = .discrete

    animation.values = [
      true, // hidden, before `inFrame`
      false, // visible
      true, // hidden, after `outFrame`
    ]

    // From the documentation of `keyTimes`:
    //  - If the calculationMode is set to discrete, the first value in the array
    //    must be 0.0 and the last value must be 1.0. The array should have one more
    //    entry than appears in the values array. For example, if there are two values,
    //    there should be three key times.
    animation.keyTimes = [
      NSNumber(value: 0.0),
      NSNumber(value: Double(context.relativeTime(of: inFrame))),
      NSNumber(value: Double(context.relativeTime(of: outFrame))),
      NSNumber(value: 1.0),
    ]

    animation.configureTiming(with: context)
    add(animation, forKey: "hidden")
  }
}


Review comment (suggestion):
```suggestion
    add(animation, forKey: #keyPath(CALayer.isHidden))
```

Code after:
    outFrame: AnimationFrameTime,
    context: LayerAnimationContext)
  {
    let animation = CAKeyframeAnimation(keyPath: #keyPath(isHidden))
    animation.calculationMode = .discrete

    animation.values = [
      true, // hidden, before `inFrame`
      false, // visible
      true, // hidden, after `outFrame`
    ]

    // From the documentation of `keyTimes`:
    //  - If the calculationMode is set to discrete, the first value in the array
    //    must be 0.0 and the last value must be 1.0. The array should have one more
    //    entry than appears in the values array. For example, if there are two values,
    //    there should be three key times.
    animation.keyTimes = [
      NSNumber(value: 0.0),
      NSNumber(value: Double(context.relativeTime(of: inFrame))),
      NSNumber(value: Double(context.relativeTime(of: outFrame))),
      NSNumber(value: 1.0),
    ]

    animation.configureTiming(with: context)
    add(animation, forKey: #keyPath(isHidden))
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Layers/SolidLayer.swift (Swift)

Code before:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import QuartzCore

// MARK: - SolidLayer

final class SolidLayer: BaseCompositionLayer {

  // MARK: Lifecycle

  init(_ solidLayer: SolidLayerModel) {
    self.solidLayer = solidLayer
    super.init(layerModel: solidLayer)

    backgroundColor = solidLayer.colorHex.cgColor
  }

  required init?(coder _: NSCoder) {
    fatalError("init(coder:) has not been implemented")
  }

  /// Called by CoreAnimation to create a shadow copy of this layer
  /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
  override init(layer: Any) {
    guard let layer = layer as? Self else {
      fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
    }

    solidLayer = layer.solidLayer
    super.init(layer: layer)
  }

  // MARK: Private

  private let solidLayer: SolidLayerModel

}


Review comment (suggestion):
This is unrelated, but fixes this issue that's been bothering me:

<img width="755" alt="image" src="https://user-images.githubusercontent.com/1811727/148451823-f02aac05-b8bd-4e2c-855c-ce816fb1c3bb.png">


Code after:
  required init?(coder _: NSCoder) {
    fatalError("init(coder:) has not been implemented")
  }

  /// Called by CoreAnimation to create a shadow copy of this layer
  /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
  override init(layer: Any) {
    guard let layer = layer as? Self else {
      fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
    }

    solidLayer = layer.solidLayer
    super.init(layer: layer)
  }

  // MARK: Private

  private let solidLayer: SolidLayerModel

  private func setupContentLayer() {
    // Render the fill color in a child `CAShapeLayer`
    //  - Using a `CAShapeLayer` specifically, instead of a `CALayer` with a `backgroundColor`,
    //    allows the size of the fill shape to be different from `contentsLayer.size`.
    let shapeLayer = CAShapeLayer()
    shapeLayer.fillColor = solidLayer.colorHex.cgColor
    shapeLayer.path = CGPath(rect: .init(x: 0, y: 0, width: solidLayer.width, height: solidLayer.height), transform: nil)
    contentsLayer.addSublayer(shapeLayer)
  }

}
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Layers/MaskCompositionLayer.swift (Swift)

Code before:

  private let maskLayers: [MaskLayer]

}

// MARK: AnimationLayer

extension MaskCompositionLayer: AnimationLayer {
  func setupAnimations(context: LayerAnimationContext) {
    for maskLayer in maskLayers {
      maskLayer.setupAnimations(context: context)
    }
  }
}

// MARK: - MaskLayer

extension MaskCompositionLayer {
  final class MaskLayer: CAShapeLayer {

    // MARK: Lifecycle

    init(mask: Mask) {
      maskModel = mask
      super.init()
      fillColor = "#000000".cgColor
    }

    required init?(coder _: NSCoder) {
      fatalError("init(coder:) has not been implemented")
    }

    /// Called by CoreAnimation to create a shadow copy of this layer
    /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
    override init(layer: Any) {
      guard let layer = layer as? Self else {
        fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
      }

      maskModel = layer.maskModel
      super.init(layer: layer)
    }

    // MARK: Private

    private let maskModel: Mask

  }
}

// MARK: - MaskCompositionLayer.MaskLayer + AnimationLayer

Review comment (suggestion):
nit: why not `CGColor.black`? Seems like we'll pay for string processing overhead here
```suggestion
      fillColor = .black
```

Code after:

  private let maskLayers: [MaskLayer]

}

// MARK: AnimationLayer

extension MaskCompositionLayer: AnimationLayer {
  func setupAnimations(context: LayerAnimationContext) {
    for maskLayer in maskLayers {
      maskLayer.setupAnimations(context: context)
    }
  }
}

// MARK: - MaskLayer

extension MaskCompositionLayer {
  final class MaskLayer: CAShapeLayer {

    // MARK: Lifecycle

    init(mask: Mask) {
      maskModel = mask
      super.init()
      fillColor = .rgb(0, 0, 0)
    }

    required init?(coder _: NSCoder) {
      fatalError("init(coder:) has not been implemented")
    }

    /// Called by CoreAnimation to create a shadow copy of this layer
    /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
    override init(layer: Any) {
      guard let layer = layer as? Self else {
        fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
      }

      maskModel = layer.maskModel
      super.init(layer: layer)
    }

    // MARK: Private

    private let maskModel: Mask

  }
}

// MARK: - MaskCompositionLayer.MaskLayer + AnimationLayer
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/Layers/MaskCompositionLayer.swift (Swift)

Code before:
// Created by Cal Stephens on 1/6/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import QuartzCore

// MARK: - MaskCompositionLayer

/// The CALayer type responsible for rendering the `Mask` of a `BaseCompositionLayer`
final class MaskCompositionLayer: CALayer {

  // MARK: Lifecycle

  init(masks: [Mask]) {
    maskLayers = masks.map { MaskLayer(mask: $0) }
    super.init()

    for maskLayer in maskLayers {
      addSublayer(maskLayer)
    }
  }

  required init?(coder _: NSCoder) {
    fatalError("init(coder:) has not been implemented")
  }

  /// Called by CoreAnimation to create a shadow copy of this layer
  /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
  override init(layer: Any) {
    guard let layer = layer as? Self else {
      fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
    }

    maskLayers = layer.maskLayers
    super.init(layer: layer)
  }

  // MARK: Internal

  override func layoutSublayers() {

Review comment (suggestion):
nit:
```suggestion
    maskLayers = masks.map(MaskLayer.init(mask:))
```

Code after:
// Created by Cal Stephens on 1/6/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import QuartzCore

// MARK: - MaskCompositionLayer

/// The CALayer type responsible for rendering the `Mask` of a `BaseCompositionLayer`
final class MaskCompositionLayer: CALayer {

  // MARK: Lifecycle

  init(masks: [Mask]) {
    maskLayers = masks.map(MaskLayer.init(mask:))
    super.init()

    for maskLayer in maskLayers {
      addSublayer(maskLayer)
    }
  }

  required init?(coder _: NSCoder) {
    fatalError("init(coder:) has not been implemented")
  }

  /// Called by CoreAnimation to create a shadow copy of this layer
  /// More details: https://developer.apple.com/documentation/quartzcore/calayer/1410842-init
  override init(layer: Any) {
    guard let layer = layer as? Self else {
      fatalError("\(Self.self).init(layer:) incorrectly called with \(type(of: layer))")
    }

    maskLayers = layer.maskLayers
    super.init(layer: layer)
  }

  // MARK: Internal

  override func layoutSublayers() {
Repository: airbnb/lottie-ios
File: Sources/Private/Experimental/ExperimentalFeatureConfiguration.swift (Swift)

Code before:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

/// Configuration for experimental features that are in development
///
/// Experimental features are not considered stable and are subject
/// to change or be removed at any time.
public struct ExperimentalFeatureConfiguration {

  /// The singleton configuration for experimental features,
  /// which applies to all `AnimationView`s by default.
  public static var shared = ExperimentalFeatureConfiguration()

  /// Whether or not to use the new, experimental, rendering engine,
  /// which leverages the Core Animation render server to
  /// animate without executing on the main thread every frame.
  public var useNewRenderingEngine = false

  public init(useNewRenderingEngine: Bool = false) {
    self.useNewRenderingEngine = useNewRenderingEngine
  }

}


Review comment (suggestion):
nit: remove default value since it's specified in the initializer so there's a single source of truth
```suggestion
  public var useNewRenderingEngine: Bool
```

Code after:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

/// Configuration for experimental features that are in development
///
/// Experimental features are not considered stable and are subject
/// to change or be removed at any time.
public struct ExperimentalFeatureConfiguration {

  public init(useNewRenderingEngine: Bool = false) {
    self.useNewRenderingEngine = useNewRenderingEngine
  }

  /// The singleton configuration for experimental features,
  /// which applies to all `AnimationView`s by default.
  public static var shared = ExperimentalFeatureConfiguration()

  /// Whether or not to use the new, experimental, rendering engine,
  /// which leverages the Core Animation render server to
  /// animate without executing on the main thread every frame.
  public var useNewRenderingEngine: Bool

}
Repository: airbnb/lottie-ios
File: Tests/Utils/HardcodedImageProvider.swift (Swift)

Code before:
// Created by Cal Stephens on 1/19/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import Lottie
import QuartzCore
#if os(iOS)
import UIKit
#endif

// MARK: - HardcodedImageProvider

/// An `AnimationImageProvider` that always returns a specific hardcoded image of a dog (hi Marley)
struct HardcodedImageProvider: AnimationImageProvider {
  func imageForAsset(asset _: ImageAsset) -> CGImage? {
    #if os(iOS)
    return UIImage(named: "Samples/Images/dog.png", in: .module, compatibleWith: nil)?.cgImage
    #else
    return nil
    #endif
  }
}


Review comment (suggestion):
nit: add a `name` property to this provider so we can use it in other places?
```suggestion
struct HardcodedImageProvider: AnimationImageProvider {
  var name: String
  
  func imageForAsset(asset _: ImageAsset) -> CGImage? {
    #if os(iOS)
    return UIImage(named: name, in: .module, compatibleWith: nil)?.cgImage
    #else
    return nil
    #endif
  }
}
```

Code after:
// Created by Cal Stephens on 1/19/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import Lottie
import QuartzCore
#if os(iOS)
import UIKit
#endif

// MARK: - HardcodedImageProvider

/// An `AnimationImageProvider` that always returns a specific hardcoded image of a dog (hi Marley)
struct HardcodedImageProvider: AnimationImageProvider {
  let imageName: String

  func imageForAsset(asset _: ImageAsset) -> CGImage? {
    #if os(iOS)
    return UIImage(named: imageName, in: .module, compatibleWith: nil)?.cgImage
    #else
    return nil
    #endif
  }
}
Repository: airbnb/lottie-ios
File: Example/iOS/Configuration.swift (Swift)

Code before:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import Foundation
import Lottie

final class Configuration {

  /// Whether or not to use the new, experimental rendering engine
  static var useNewRenderingEngine: Bool {
    get { UserDefaults.standard.bool(forKey: #function) }
    set {
      UserDefaults.standard.set(newValue, forKey: #function)
      applyCurrentConfiguration()
    }
  }

  /// Applies the current configuration (stored in UserDefaults)
  /// to the singleton `LottieConfiguration.shared`
  static func applyCurrentConfiguration() {
    if useNewRenderingEngine {
      LottieConfiguration.shared.renderingEngine = .coreAnimation
    } else {
      LottieConfiguration.shared.renderingEngine = .mainThread
    }
  }

}


Review comment (suggestion):
nit:
```suggestion
  LottieConfiguration.shared.renderingEngine = useNewRenderingEngine ? .coreAnimation : .mainThread
```

Code after:
// Created by Cal Stephens on 12/13/21.
// Copyright © 2021 Airbnb Inc. All rights reserved.

import Foundation
import Lottie

final class Configuration {

  /// Whether or not to use the new, experimental rendering engine
  static var useNewRenderingEngine: Bool {
    get { UserDefaults.standard.bool(forKey: #function) }
    set {
      UserDefaults.standard.set(newValue, forKey: #function)
      applyCurrentConfiguration()
    }
  }

  /// Applies the current configuration (stored in UserDefaults)
  /// to the singleton `LottieConfiguration.shared`
  static func applyCurrentConfiguration() {
    LottieConfiguration.shared.renderingEngine = useNewRenderingEngine ? .coreAnimation : .mainThread
  }

}
Repository: airbnb/lottie-ios
File: Tests/PerformanceTests.swift (Swift)

Code before:
// Created by Cal Stephens on 2/14/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import Foundation
import XCTest

@testable import Lottie

// MARK: - PerformanceTests

final class PerformanceTests: XCTestCase {

  // MARK: Internal

  func testAnimationViewSetup_simpleAnimation() {
    // Compare the performance of displaying this simple animation in the two animation engines
    let ratio = compareEngineSetupPerformance(for: simpleAnimation, iterations: 2000)

    // This is basically a snapshot test for the performance of the Core Animation engine
    // compared to the Main Thread engine. Currently, the Core Animation engine is
    // about 1.6x slower than the Main Thread engine in this example.
    XCTAssertEqual(ratio, 1.6, accuracy: 0.3)
  }

  func testAnimationViewSetup_complexAnimation() {
    let ratio = compareEngineSetupPerformance(for: complexAnimation, iterations: 500)
    XCTAssertEqual(ratio, 1.6, accuracy: 0.3)
  }

  func testAnimationViewScrubbing_simpleAnimation() {
    let ratio = compareEngineScrubbingPerformance(for: simpleAnimation, iterations: 1000)
    XCTAssertEqual(ratio, 0.1, accuracy: 0.1)
  }

  func testAnimationViewScrubbing_complexAnimation() {
    let ratio = compareEngineScrubbingPerformance(for: complexAnimation, iterations: 1000)

Review comment (performance):
Do you think it would make sense to support running these with the `XCTest` `measure` APIs as well? Might be nice to support the local Xcode performance measurement APIs, even if the results aren't collected in CI?

Code after:
// Created by Cal Stephens on 2/14/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import Foundation
import XCTest

@testable import Lottie

// MARK: - PerformanceTests

final class PerformanceTests: XCTestCase {

  // MARK: Internal

  func testAnimationViewSetup_simpleAnimation() {
    // Compare the performance of displaying this simple animation in the two animation engines
    let ratio = compareEngineSetupPerformance(for: simpleAnimation, iterations: 2000)

    // This is basically a snapshot test for the performance of the Core Animation engine
    // compared to the Main Thread engine. Currently, the Core Animation engine is
    // about the same speed as the Main Thread engine in this example.
    XCTAssertEqual(ratio, 1.0, accuracy: 0.15)
  }

  func testAnimationViewSetup_complexAnimation() {
    let ratio = compareEngineSetupPerformance(for: complexAnimation, iterations: 500)

    // The Core Animation engine is currently about 1.6x slower than the
    // Main Thread engine in this example.
    XCTAssertEqual(ratio, 1.6, accuracy: 0.3)
  }

  func testAnimationViewScrubbing_simpleAnimation() {
    let ratio = compareEngineScrubbingPerformance(for: simpleAnimation, iterations: 2000)
    XCTAssertEqual(ratio, 0.002, accuracy: 0.001)
  }
Repository: airbnb/lottie-ios
File: Sources/Private/LayerContainers/AnimationContainer.swift (Swift)

Code before:
      let animation = CABasicAnimation(keyPath: event)
      animation.timingFunction = CAMediaTimingFunction(name: CAMediaTimingFunctionName.linear)
      animation.fromValue = presentation()?.currentFrame
      return animation
    }
    return super.action(forKey: event)
  }

  public override func display() {
    guard Thread.isMainThread else { return }
    var newFrame: CGFloat
    if
      let animationKeys = animationKeys(),
      !animationKeys.isEmpty
    {
      newFrame = presentation()?.currentFrame ?? currentFrame
    } else {
      // We ignore the presentation's frame if there's no animation in the layer.
      newFrame = currentFrame
    }
    if respectAnimationFrameRate {
      newFrame = floor(newFrame)
    }
    animationLayers.forEach {
      $0.displayWithFrame(frame: newFrame, forceUpdates: false)
    }
  }

  // MARK: Internal

  /// The animatable Current Frame Property
  @NSManaged var currentFrame: CGFloat

  var animationLayers: ContiguousArray<CompositionLayer>

  var primaryAnimationKey: AnimationKey {
    .managed
  }

  var _animationLayers: [CALayer] {
    Array(animationLayers)
  }

  var imageProvider: AnimationImageProvider {
    get {
      layerImageProvider.imageProvider
    }
    set {
      layerImageProvider.imageProvider = newValue
    }
  }

Review comment (suggestion):
nit: `forEach` feels non-idiomatic here, especially when used with `$0`
```suggestion
    for layer in animationLayers {
      layer.displayWithFrame(frame: newFrame, forceUpdates: false)
    }
```

Code after:
      let animation = CABasicAnimation(keyPath: event)
      animation.timingFunction = CAMediaTimingFunction(name: CAMediaTimingFunctionName.linear)
      animation.fromValue = presentation()?.currentFrame
      return animation
    }
    return super.action(forKey: event)
  }

  public override func display() {
    guard Thread.isMainThread else { return }
    var newFrame: CGFloat
    if
      let animationKeys = animationKeys(),
      !animationKeys.isEmpty
    {
      newFrame = presentation()?.currentFrame ?? currentFrame
    } else {
      // We ignore the presentation's frame if there's no animation in the layer.
      newFrame = currentFrame
    }
    if respectAnimationFrameRate {
      newFrame = floor(newFrame)
    }
    animationLayers.forEach { $0.displayWithFrame(frame: newFrame, forceUpdates: false) }
  }

  // MARK: Internal

  /// The animatable Current Frame Property
  @NSManaged var currentFrame: CGFloat

  var animationLayers: ContiguousArray<CompositionLayer>

  var primaryAnimationKey: AnimationKey {
    .managed
  }

  var _animationLayers: [CALayer] {
    Array(animationLayers)
  }

  var imageProvider: AnimationImageProvider {
    get {
      layerImageProvider.imageProvider
    }
    set {
      layerImageProvider.imageProvider = newValue
    }
  }

  var renderScale: CGFloat = 1 {
Repository: airbnb/lottie-ios
File: Sources/Private/Model/Extensions/Bundle.swift (Swift)

Code before:
import UIKit
#endif

extension Bundle {
  func getAnimationData(_ name: String, subdirectory: String? = nil) throws -> Data? {
    // Check for files in the bundle at the given path
    let name = name.removingJSONSuffix()
    if let url = url(forResource: name, withExtension: "json", subdirectory: subdirectory) {
      return try Data(contentsOf: url)
    }

    // Check for data assets (not available on macOS)
    #if os(iOS) || os(tvOS) || os(watchOS) || targetEnvironment(macCatalyst)
    let assetKey = subdirectory != nil ? "\(subdirectory ?? "")/\(name)" : name
    return NSDataAsset(name: assetKey, bundle: self)?.data
    #else
    return nil
    #endif
  }
}

extension String {
  fileprivate func removingJSONSuffix() -> String {
    guard hasSuffix(".json") else {
      return self
    }

    return (self as NSString).deletingPathExtension
  }
}


Review comment (suggestion):
A comment explaining why we specifically only do this for `.json` files would be nice (plus maybe mentioning that this is for backwards compatibility with Lottie 2.x)

Code after:
import UIKit
#endif

extension Bundle {
  func getAnimationData(_ name: String, subdirectory: String? = nil) throws -> Data? {
    // Check for files in the bundle at the given path
    let name = name.removingJSONSuffix()
    if let url = url(forResource: name, withExtension: "json", subdirectory: subdirectory) {
      return try Data(contentsOf: url)
    }

    // Check for data assets (not available on macOS)
    #if os(iOS) || os(tvOS) || os(watchOS) || targetEnvironment(macCatalyst)
    let assetKey = subdirectory != nil ? "\(subdirectory ?? "")/\(name)" : name
    return NSDataAsset(name: assetKey, bundle: self)?.data
    #else
    return nil
    #endif
  }
}

extension String {
  fileprivate func removingJSONSuffix() -> String {
    // Allow filenames to be passed with a ".json" extension (but not other extensions)
    // to keep the behavior from Lottie 2.x - instead of failing to load the animation
    guard hasSuffix(".json") else {
      return self
    }

    return (self as NSString).deletingPathExtension
  }
}