ronantakizawa/codereview-bench huggingface dataset:
Repository: airbnb/lottie-ios
File: Sources/Private/Utility/Extensions/BlendMode+Filter.swift (Swift)

Code before:
//
//  File.swift
//  
//
//  Created by Denis Koryttsev on 10.05.2022.
//

extension BlendMode {
    /// The Core Image filter name for this `BlendMode`, that can be applied to a `CALayer`'s `compositingFilter`.
    /// Supported compositing filters are defined here: https://developer.apple.com/library/archive/documentation/GraphicsImaging/Reference/CoreImageFilterReference/index.html#//apple_ref/doc/uid/TP30000136-SW71
    var filterName: String? {
        switch self {
        case .normal: return nil
        case .multiply: return "multiplyBlendMode"
        case .screen: return "screenBlendMode"
        case .overlay: return "overlayBlendMode"
        case .darken: return "darkenBlendMode"
        case .lighten: return "lightenBlendMode"
        case .colorDodge: return "colorDodgeBlendMode"
        case .colorBurn: return "colorBurnBlendMode"
        case .hardLight: return "hardLightBlendMode"
        case .softLight: return "softLightBlendMode"
        case .difference: return "differenceBlendMode"
        case .exclusion: return "exclusionBlendMode"
        case .hue: return "hueBlendMode"
        case .saturation: return "multiplyBlendMode"
        case .color: return "colorBlendMode"
        case .luminosity: return "luminosityBlendMode"
        }
    }
}



Review comment (suggestion):
Should this be:

```suggestion
        case .saturation: return "saturationBlendMode"
```

Code after:
//
//  File.swift
//
//
//  Created by Denis Koryttsev on 10.05.2022.
//

extension BlendMode {
  /// The Core Image filter name for this `BlendMode`, that can be applied to a `CALayer`'s `compositingFilter`.
  /// Supported compositing filters are defined here: https://developer.apple.com/library/archive/documentation/GraphicsImaging/Reference/CoreImageFilterReference/index.html#//apple_ref/doc/uid/TP30000136-SW71
  var filterName: String? {
    switch self {
    case .normal: return nil
    case .multiply: return "multiplyBlendMode"
    case .screen: return "screenBlendMode"
    case .overlay: return "overlayBlendMode"
    case .darken: return "darkenBlendMode"
    case .lighten: return "lightenBlendMode"
    case .colorDodge: return "colorDodgeBlendMode"
    case .colorBurn: return "colorBurnBlendMode"
    case .hardLight: return "hardLightBlendMode"
    case .softLight: return "softLightBlendMode"
    case .difference: return "differenceBlendMode"
    case .exclusion: return "exclusionBlendMode"
    case .hue: return "hueBlendMode"
    case .saturation: return "saturationBlendMode"
    case .color: return "colorBlendMode"
    case .luminosity: return "luminosityBlendMode"
    }
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/Utility/Interpolatable/KeyframeGroup+Extensions.swift (Swift)

Code before:
      return keyframes
    }

    var output = ContiguousArray<Keyframe<Vector1D>>()

    for idx in 1 ..< keyframes.count {
      let prev = keyframes[idx - 1]
      let curr = keyframes[idx]

      // The timing function is responsible for computing the expected progress
      let outTangent = prev.outTangent?.pointValue ?? .zero
      let inTangent = curr.inTangent?.pointValue ?? .init(x: 1, y: 1)
      let timingFunction = UnitBezier(controlPoint1: outTangent, controlPoint2: inTangent)

      // These values are used to compute new values in the adjusted keyframes
      let difference = curr.value.value - prev.value.value
      let startValue = prev.value.value
      let startTime = prev.time
      let duration = max(Int(curr.time - prev.time), 0)

      // Create one interpolated keyframe for each time in the duration
      for t in 0 ... duration {
        let progress = timingFunction.value(
          for: CGFloat(t) / CGFloat(duration),
          epsilon: 0.005)
        let value = startValue + progress * difference
        output.append(
          Keyframe<Vector1D>(
            value: Vector1D(value),
            time: startTime + CGFloat(t),
            isHold: false,
            inTangent: nil,
            outTangent: nil,
            spatialInTangent: nil,
            spatialOutTangent: nil))
      }
    }

    return output
  }
}


Review comment (suggestion):
There exists a similar method `Keyframe.interpolate(to:progress:)` but is either performing a different operation or giving less accurate results.

Code after:
    guard keyframes.count > 1 else {
      return keyframes
    }

    var output = ContiguousArray<Keyframe<Vector1D>>()

    for idx in 1 ..< keyframes.count {
      let prev = keyframes[idx - 1]
      let curr = keyframes[idx]

      // The timing function is responsible for computing the expected progress
      let outTangent = prev.outTangent?.pointValue ?? .zero
      let inTangent = curr.inTangent?.pointValue ?? .init(x: 1, y: 1)
      let timingFunction = UnitBezier(controlPoint1: outTangent, controlPoint2: inTangent)

      // These values are used to compute new values in the adjusted keyframes
      let difference = curr.value.value - prev.value.value
      let startValue = prev.value.value
      let startTime = prev.time
      let duration = max(Int(curr.time - prev.time), 0)

      // Create one interpolated keyframe for each time in the duration
      for t in 0 ... duration {
        let progress = timingFunction.value(
          for: CGFloat(t) / CGFloat(duration),
          epsilon: 0.005)
        let value = startValue + Double(progress) * difference
        output.append(
          Keyframe<Vector1D>(
            value: Vector1D(value),
            time: startTime + CGFloat(t),
            isHold: false,
            inTangent: nil,
            outTangent: nil,
            spatialInTangent: nil,
            spatialOutTangent: nil))
      }
    }

    return output
  }
}
Repository: airbnb/lottie-ios
File: Package.swift (Swift)

Code before:
// swift-tools-version:5.3
import PackageDescription

let package = Package(
  name: "Lottie",
  platforms: [.iOS("11.0"), .macOS("10.10"), .tvOS("11.0")],
  products: [.library(name: "Lottie", targets: ["Lottie"])],
  targets: [.target(name: "Lottie", path: "Sources")])

#if swift(>=5.6)
// Add the Airbnb Swift formatting plugin if possible
package.dependencies.append(.package(url: "https://github.com/airbnb/swift", .branch("cal--swiftversion")))
#endif


Review comment (suggestion):
I'll change this to `.upToNextMajor(from: "1.0.1")` after https://github.com/airbnb/swift/pull/180 is merged / released

Code after:
// swift-tools-version:5.3
import PackageDescription

let package = Package(
  name: "Lottie",
  platforms: [.iOS("11.0"), .macOS("10.10"), .tvOS("11.0")],
  products: [.library(name: "Lottie", targets: ["Lottie"])],
  targets: [.target(name: "Lottie", path: "Sources")])

#if swift(>=5.6)
// Add the Airbnb Swift formatting plugin if possible
package.dependencies.append(.package(url: "https://github.com/airbnb/swift", .upToNextMajor(from: "1.0.1")))
#endif
Repository: airbnb/lottie-ios
File: Sources/Private/CoreAnimation/Animations/EllipseAnimation.swift (Swift)

Code before:
      value: { keyframe in
        BezierPath.ellipse(
          size: keyframe.size.sizeValue,
          center: keyframe.position.pointValue,
          direction: ellipse.direction)
          .cgPath()
          .duplicated(times: pathMultiplier)
      },
      context: context)
  }
}

extension Ellipse {
  /// Data that represents how to render an ellipse at a specific point in time
  struct Keyframe {
    let size: Vector3D
    let position: Vector3D
  }

  /// Creates a single array of animatable keyframes from the separate arrays of keyframes in this Ellipse
  func combinedKeyframes(context: LayerAnimationContext) throws-> KeyframeGroup<Ellipse.Keyframe> {
    let combinedKeyframes = Keyframes.combinedIfPossible(size, position) { untypedValues in
      Keyframe(
        size: untypedValues[0] as! Vector3D,
        position: untypedValues[1] as! Vector3D)
    }

    if let combinedKeyframes = combinedKeyframes {
      return combinedKeyframes
    } else {
      // If we weren't able to combine all of the keyframes, we have to take the timing values
      // from one property and use a fixed value for the other properties.
      return try size.map { sizeValue in
        Keyframe(
          size: sizeValue,
          position: try position.exactlyOneKeyframe(context: context, description: "ellipse position"))
      }
    }
  }
}


Review comment (suggestion):
I'm a bit confused what's going on here, why are we force casting these to `Vector3D` if both params are of type `Vector3D`? Is this due to limitations around variadic parameters and generics? Wondering if we could instead just make multiple arities of this function with tuples, similar to how Combine works with operators, since this feels a bit brittle/unsafe as-is

Code after:
      value: { keyframe in
        BezierPath.ellipse(
          size: keyframe.size.sizeValue,
          center: keyframe.position.pointValue,
          direction: ellipse.direction)
          .cgPath()
          .duplicated(times: pathMultiplier)
      },
      context: context)
  }
}

extension Ellipse {
  /// Data that represents how to render an ellipse at a specific point in time
  struct Keyframe {
    let size: Vector3D
    let position: Vector3D
  }

  /// Creates a single array of animatable keyframes from the separate arrays of keyframes in this Ellipse
  func combinedKeyframes(context: LayerAnimationContext) throws-> KeyframeGroup<Ellipse.Keyframe> {
    let combinedKeyframes = Keyframes.combinedIfPossible(
      size, position,
      makeCombinedResult: Ellipse.Keyframe.init)

    if let combinedKeyframes = combinedKeyframes {
      return combinedKeyframes
    } else {
      // If we weren't able to combine all of the keyframes, we have to take the timing values
      // from one property and use a fixed value for the other properties.
      return try size.map { sizeValue in
        Keyframe(
          size: sizeValue,
          position: try position.exactlyOneKeyframe(context: context, description: "ellipse position"))
      }
    }
  }
}
Repository: airbnb/lottie-ios
File: Sources/Private/CoreAnimation/Animations/CombinedShapeAnimation.swift (Swift)

Code before:
    super.init(name: name, type: .shape, hidden: false)
  }

  required init(from _: Decoder) throws {
    fatalError("init(from:) has not been implemented")
  }

  required init(dictionary _: [String: Any]) throws {
    fatalError("init(dictionary:) has not been implemented")
  }

  // MARK: Internal

  let shapes: KeyframeGroup<[BezierPath]>

}

extension CombinedShapeItem {
  /// Manually combines the given shape keyframes by manually interpolating at each frame
  static func manuallyInterpolating(
    shapes: [KeyframeGroup<BezierPath>],
    name: String,
    context: LayerContext)
    -> CombinedShapeItem
  {
    let interpolators = shapes.map { KeyframeInterpolator<BezierPath>(keyframes: $0.keyframes) }

    let animationTimeRange = Int(context.animation.startFrame)...Int(context.animation.endFrame)
    let interpolatedKeyframes: [Keyframe<[BezierPath]>] = animationTimeRange.map { frame in
      let paths = interpolators.compactMap { $0.value(frame: AnimationFrameTime(frame)) as? BezierPath }
      return Keyframe(value: paths, time: AnimationFrameTime(frame))
    }

    return CombinedShapeItem(
      shapes: KeyframeGroup(keyframes: ContiguousArray(interpolatedKeyframes)),
      name: name)
  }
}


Review comment (suggestion):
Nice use of some infra from the main thread renderer! Wondering if we should move it to a shared place now

Code after:
    super.init(name: name, type: .shape, hidden: false)
  }

  required init(from _: Decoder) throws {
    fatalError("init(from:) has not been implemented")
  }

  required init(dictionary _: [String: Any]) throws {
    fatalError("init(dictionary:) has not been implemented")
  }

  // MARK: Internal

  let shapes: KeyframeGroup<[BezierPath]>

}

extension CombinedShapeItem {
  /// Manually combines the given shape keyframes by manually interpolating at each frame
  static func manuallyInterpolating(
    shapes: [KeyframeGroup<BezierPath>],
    name: String,
    context: LayerContext)
    -> CombinedShapeItem
  {
    let animationTimeRange = Int(context.animation.startFrame)...Int(context.animation.endFrame)

    let interpolators = shapes.map { shape in
      KeyframeInterpolator(keyframes: shape.keyframes)
    }

    let interpolatedKeyframes = animationTimeRange.map { frame in
      Keyframe(
        value: interpolators.compactMap { interpolator in
          interpolator.value(frame: AnimationFrameTime(frame)) as? BezierPath
        },
        time: AnimationFrameTime(frame))
    }

    return CombinedShapeItem(
      shapes: KeyframeGroup(keyframes: ContiguousArray(interpolatedKeyframes)),
      name: name)
  }
}
Repository: airbnb/lottie-ios
File: Tests/Utils/HardcodedTextProvider.swift (Swift)

Code before:
// Created by Igor Katselenbogen on 08/19/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

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

// MARK: - HardcodedImageProvider

/// An `AnimationTextProvider` that always returns a specific hardcoded text
class HardcodedTextProvider: AnimationTextProvider {
  // Question 4: What should we actually return here?
  func textFor(keypathName: String, sourceText: String) -> String {
    #if os(iOS)
    return "text layer text"
    #else
    return nil
    #endif
  }
}


Review comment (suggestion):
I think this should be:

```suggestion
class HardcodedTextProvider: AnimationTextProvider {

  init(text: String) {
    self.text = text
  }
  
  func textFor(keypathName _: String, sourceText _: String) -> String {
    text
  }
  
  private let text: String

}
```

Code after:
// Created by Igor Katselenbogen on 08/19/22.
// Copyright © 2022 Airbnb Inc. All rights reserved.

import Lottie

// MARK: - HardcodedTextProvider

/// An `AnimationTextProvider` that always returns a specific hardcoded text
class HardcodedTextProvider: AnimationTextProvider {

  // MARK: Lifecycle

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

  // MARK: Internal

  func textFor(keypathName _: String, sourceText _: String) -> String {
    text
  }

  // MARK: Private

  private let text: String

}
Repository: airbnb/lottie-ios
File: Sources/Public/LottieConfiguration.swift (Swift)

Code before:
  public var rawValue: String {
    switch self {
    case .mainThread:
      return "Main Thread"
    case .coreAnimation:
      return "Core Animation"
    }
  }

  public var description: String {
    rawValue
  }
}

// MARK: - DecodingStrategy

/// How animation files should be decoded
public enum DecodingStrategy: Hashable {
  /// Use Codable. This is was the default strategy introduced on Lottie 3, but should be rarely
  /// used as it's slower than `dictionaryBased`. Kept here for any possible compability issues
  /// that may come up, but consider it soft-deprecated.
  case codable

  /// Manually deserialize a dictionary into an Animation.
  /// This should be at least 2-3x faster than using Codable and due to that
  /// it's the new default on Lottie 4.x.
  case dictionaryBased
}


Review comment (suggestion):
```suggestion
  /// it's the default as of Lottie 4.x.
```

Code after:
  public var rawValue: String {
    switch self {
    case .mainThread:
      return "Main Thread"
    case .coreAnimation:
      return "Core Animation"
    }
  }

  public var description: String {
    rawValue
  }
}

// MARK: - DecodingStrategy

/// How animation files should be decoded
public enum DecodingStrategy: Hashable {
  /// Use Codable. This is was the default strategy introduced on Lottie 3, but should be rarely
  /// used as it's slower than `dictionaryBased`. Kept here for any possible compatibility issues
  /// that may come up, but consider it soft-deprecated.
  case legacyCodable

  /// Manually deserialize a dictionary into an Animation.
  /// This should be at least 2-3x faster than using Codable and due to that
  /// it's the default as of Lottie 4.x.
  case dictionaryBased
}
Repository: airbnb/lottie-ios
File: Sources/Public/LottieConfiguration.swift (Swift)

Code before:
      return nil
    }
  }

  // MARK: Public

  public var rawValue: String {
    switch self {
    case .mainThread:
      return "Main Thread"
    case .coreAnimation:
      return "Core Animation"
    }
  }

  public var description: String {
    rawValue
  }
}

// MARK: - DecodingStrategy

/// How animation files should be decoded
public enum DecodingStrategy: Hashable {
  /// Use Codable. This is was the default strategy introduced on Lottie 3, but should be rarely
  /// used as it's slower than `dictionaryBased`. Kept here for any possible compability issues
  /// that may come up, but consider it soft-deprecated.
  case codable

  /// Manually deserialize a dictionary into an Animation.
  /// This should be at least 2-3x faster than using Codable and due to that
  /// it's the new default on Lottie 4.x.
  case dictionaryBased
}


Review comment (performance):
```suggestion
  /// used as it's slower than `dictionaryBased`. Kept here for any possible compatibility issues
```

Code after:
      return nil
    }
  }

  // MARK: Public

  public var rawValue: String {
    switch self {
    case .mainThread:
      return "Main Thread"
    case .coreAnimation:
      return "Core Animation"
    }
  }

  public var description: String {
    rawValue
  }
}

// MARK: - DecodingStrategy

/// How animation files should be decoded
public enum DecodingStrategy: Hashable {
  /// Use Codable. This is was the default strategy introduced on Lottie 3, but should be rarely
  /// used as it's slower than `dictionaryBased`. Kept here for any possible compatibility issues
  /// that may come up, but consider it soft-deprecated.
  case legacyCodable

  /// Manually deserialize a dictionary into an Animation.
  /// This should be at least 2-3x faster than using Codable and due to that
  /// it's the default as of Lottie 4.x.
  case dictionaryBased
}
Repository: airbnb/lottie-ios
File: Sources/Private/Model/DotLottie/DotLottieImageProvider.swift (Swift)

Code before:
//
//  DotLottieImageProvider.swift
//  Lottie
//
//  Created by Evandro Hoffmann on 20/10/22.
//

import Foundation
#if os(iOS) || os(tvOS) || os(watchOS) || targetEnvironment(macCatalyst)
import UIKit
#elseif os(macOS)
import AppKit
#endif

// MARK: - DotLottieImageProvider

/// An image provide that loads the images from a DotLottieFile into memory
class DotLottieImageProvider: AnimationImageProvider {

  // MARK: Lifecycle

  /// Initializes an image provider with a specific filepath.
  ///
  /// - Parameter filepath: The absolute filepath containing the images.
  ///
  init(filepath: String) {
    self.filepath = URL(fileURLWithPath: filepath)
    loadImages()
  }

  init(filepath: URL) {
    self.filepath = filepath
    loadImages()
  }

  // MARK: Internal

  let filepath: URL

  func imageForAsset(asset: ImageAsset) -> CGImage? {
    images[asset.name]
  }

Review comment (suggestion):
```suggestion
/// An image provider that loads the images from a DotLottieFile into memory
```

Code after:
//
//  DotLottieImageProvider.swift
//  Lottie
//
//  Created by Evandro Hoffmann on 20/10/22.
//

import Foundation
#if os(iOS) || os(tvOS) || os(watchOS) || targetEnvironment(macCatalyst)
import UIKit
#elseif os(macOS)
import AppKit
#endif

// MARK: - DotLottieImageProvider

/// An image provider that loads the images from a DotLottieFile into memory
class DotLottieImageProvider: AnimationImageProvider {

  // MARK: Lifecycle

  /// Initializes an image provider with a specific filepath.
  ///
  /// - Parameter filepath: The absolute filepath containing the images.
  ///
  init(filepath: String) {
    self.filepath = URL(fileURLWithPath: filepath)
    loadImages()
  }

  init(filepath: URL) {
    self.filepath = filepath
    loadImages()
  }

  // MARK: Internal

  let filepath: URL

  func imageForAsset(asset: ImageAsset) -> CGImage? {
    images[asset.name]
  }
Repository: airbnb/lottie-ios
File: Sources/Private/CoreAnimation/Layers/ShapeLayer.swift (Swift)

Code before:
      if allAlphaAnimationsAreIdentical {
        return [group]
      }

      // Create a new group for each stroke / fill
      return strokesAndFills.map { strokeOrFill in
        ShapeRenderGroup(
          pathItems: group.pathItems,
          otherItems: [strokeOrFill] + otherItems)
      }
    }

    var unusedItems = [ShapeItemLayer.Item]()
    for index in renderGroups.indices.reversed() {
      let renderGroup = renderGroups[index]

      // All valid render groups must have a path, otherwise the items wouldn't be rendered
      if renderGroup.pathItems.isEmpty {
        unusedItems.append(contentsOf: renderGroup.otherItems)
        renderGroups.remove(at: index)
      }
    }

    return (validGroups: renderGroups, unusedItems: unusedItems)
  }
}


Review comment (bug):
One example of how the previous implementation could behave incorrectly:
 - start with [Fill (1), Fill (2), Shape Transform]
 - split into two render groups: [Fill (1), Shape Transform] and [Fill (2), Shape Transform]
 - this would return four unused items: [Fill (1), Shape Transform, Fill (2), Shape Transform]
 - instead it should only return three unused items, with the initial order preserved

Code after:
      if allAlphaAnimationsAreIdentical {
        return [group]
      }

      // Create a new group for each stroke / fill
      return strokesAndFills.map { strokeOrFill in
        ShapeRenderGroup(
          pathItems: group.pathItems,
          otherItems: [strokeOrFill] + otherItems)
      }
    }

    // All valid render groups must have a path, otherwise the items wouldn't be rendered
    renderGroups = renderGroups.filter { renderGroup in
      !renderGroup.pathItems.isEmpty
    }

    let itemsInValidRenderGroups = NSSet(
      array: renderGroups.lazy
        .flatMap { $0.pathItems + $0.otherItems }
        .map { $0.item })

    // `unusedItems` should only include each original item a single time,
    // and should preserve the existing order
    let itemsNotInValidRenderGroups = filter { item in
      !itemsInValidRenderGroups.contains(item.item)
    }

    return (validGroups: renderGroups, unusedItems: itemsNotInValidRenderGroups)
  }
}
Repository: airbnb/lottie-ios
File: Sources/Public/iOS/Compatibility/CompatibleAnimationView.swift (Swift)

Code before:
    animationView.topAnchor.constraint(equalTo: topAnchor).isActive = true
    animationView.trailingAnchor.constraint(equalTo: trailingAnchor).isActive = true
    animationView.leadingAnchor.constraint(equalTo: leadingAnchor).isActive = true
    animationView.bottomAnchor.constraint(equalTo: bottomAnchor).isActive = true
  }
}

/// An Objective-C compatible wrapper around Lottie's AnimationTextProvider.
/// Use in tandem with CompatibleAnimationView to supply text to LottieAnimationView
/// when using Lottie in Objective-C.
@objc
public final class CompatibleAnimationTextProvider: NSObject {

  // MARK: Lifecycle

  @objc
  public init(values: [String: String])
  {
    self.values = values
    super.init()
  }

  // MARK: Internal

  internal var textProvider: AnimationTextProvider? {
    return DictionaryTextProvider(self.values);
  }

  // MARK: Private

  private let values: [String: String]
}
#endif


Review comment (suggestion):
```suggestion
    return DictionaryTextProvider(self.values)
```

Code after:
    animationView.leadingAnchor.constraint(equalTo: leadingAnchor).isActive = true
    animationView.bottomAnchor.constraint(equalTo: bottomAnchor).isActive = true
  }
}

/// An Objective-C compatible wrapper around Lottie's DictionaryTextProvider.
/// Use in tandem with CompatibleAnimationView to supply text to LottieAnimationView
/// when using Lottie in Objective-C.
@objc
public final class CompatibleDictionaryTextProvider: NSObject {

  // MARK: Lifecycle

  @objc
  public init(values: [String: String]) {
    self.values = values
    super.init()
  }

  // MARK: Internal

  internal var textProvider: AnimationTextProvider? {
    DictionaryTextProvider(values)
  }

  // MARK: Private

  private let values: [String: String]
}
#endif