ronantakizawa/codereview-bench huggingface dataset:
Repository: airbnb/lottie-ios
File: Sources/Public/iOS/Compatibility/CompatibleAnimationView.swift (Swift)

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

  // MARK: Public

  @objc
  public var compatibleAnimation: CompatibleAnimation? {
    didSet {
      animationView.animation = compatibleAnimation?.animation
    }
  }

  @objc
  public var loopAnimationCount: CGFloat = 0 {
    didSet {
      animationView.loopMode = loopAnimationCount == -1 ? .loop : .repeat(Float(loopAnimationCount))
    }
  }

  @objc
  public var compatibleAnimationTextProvider: CompatibleAnimationTextProvider? {
    didSet {
      if let textProvider = compatibleAnimationTextProvider?.textProvider {
        animationView.textProvider = textProvider
      }
    }
  }

  @objc
  public override var contentMode: UIView.ContentMode {
    set { animationView.contentMode = newValue }
    get { animationView.contentMode }
  }

  @objc
  public var shouldRasterizeWhenIdle: Bool {
    set { animationView.shouldRasterizeWhenIdle = newValue }
    get { animationView.shouldRasterizeWhenIdle }
  }

  @objc
  public var currentProgress: CGFloat {
    set { animationView.currentProgress = newValue }
    get { animationView.currentProgress }
  }

  @objc
  public var duration: CGFloat {
    animationView.animation?.duration ?? 0.0
  }

Review comment (suggestion):
We probably want to handle the case where `compatibleAnimationTextProvider` is set to `nil`. Previously this setter did nothing in that case, so `animationView.textProvider` would still have the previous value.

```suggestion
      animationView.textProvider = compatibleAnimationTextProvider?.textProvider ?? DefaultTextProvider()
```

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

  // MARK: Public

  @objc
  public var compatibleAnimation: CompatibleAnimation? {
    didSet {
      animationView.animation = compatibleAnimation?.animation
    }
  }

  @objc
  public var loopAnimationCount: CGFloat = 0 {
    didSet {
      animationView.loopMode = loopAnimationCount == -1 ? .loop : .repeat(Float(loopAnimationCount))
    }
  }

  @objc
  public var compatibleDictionaryTextProvider: CompatibleDictionaryTextProvider? {
    didSet {
      animationView.textProvider = compatibleDictionaryTextProvider?.textProvider ?? DefaultTextProvider()
    }
  }

  @objc
  public override var contentMode: UIView.ContentMode {
    set { animationView.contentMode = newValue }
    get { animationView.contentMode }
  }

  @objc
  public var shouldRasterizeWhenIdle: Bool {
    set { animationView.shouldRasterizeWhenIdle = newValue }
    get { animationView.shouldRasterizeWhenIdle }
  }

  @objc
  public var currentProgress: CGFloat {
    set { animationView.currentProgress = newValue }
    get { animationView.currentProgress }
  }

  @objc
  public var duration: CGFloat {
    animationView.animation?.duration ?? 0.0
  }

  @objc
Repository: airbnb/lottie-ios
File: Example/iOS/Views/AnimatedButtonRow.swift (Swift)

Code before:
      let fromMarker: String
      let toMarker: String
      let event: UIControl.Event
    }
  }

  func setContent(_ content: Content, animated _: Bool) {
    self.content = content
    updateGroup()
  }

  // MARK: Private

  private enum DataID {
    case animatedButton
    case title
  }

  private var content: Content?
  private let group = HGroup(alignment: .fill, spacing: 24)

  private func updateGroup() {
    guard let content = content else { return }

    group.setItems {
      if let animationName = content.animationName {
        GroupItem<AnimatedButton>(
          dataID: DataID.animatedButton,
          content: content,
          make: {
            let animatedButton = AnimatedButton()
            animatedButton.translatesAutoresizingMaskIntoConstraints = false

            NSLayoutConstraint.activate([
              animatedButton.widthAnchor.constraint(equalToConstant: 80),
              animatedButton.heightAnchor.constraint(equalToConstant: 80),
            ])

            return animatedButton
          },
          setContent: { context, content in
            context.constrainable.animation = .named(animationName)
            context.constrainable.contentMode = .scaleAspectFit

            for playRange in content.playRanges {
              context.constrainable.setPlayRange(
                fromMarker: playRange.fromMarker,
                toMarker: playRange.toMarker,
                event: playRange.event)
            }
          })

Review comment (bug):
Weird! I wonder how this compiled before considering `content.animationName` is non-optional. Must have been a compiler bug.

Code after:
      let fromMarker: String
      let toMarker: String
      let event: UIControl.Event
    }
  }

  func setContent(_ content: Content, animated _: Bool) {
    self.content = content
    updateGroup()
  }

  // MARK: Private

  private enum DataID {
    case animatedButton
    case title
  }

  private var content: Content?
  private let group = HGroup(alignment: .fill, spacing: 24)

  private func updateGroup() {
    guard let content = content else { return }

    group.setItems {
      GroupItem<AnimatedButton>(
        dataID: DataID.animatedButton,
        content: content,
        make: {
          let animatedButton = AnimatedButton()
          animatedButton.translatesAutoresizingMaskIntoConstraints = false

          NSLayoutConstraint.activate([
            animatedButton.widthAnchor.constraint(equalToConstant: 80),
            animatedButton.heightAnchor.constraint(equalToConstant: 80),
          ])

          return animatedButton
        },
        setContent: { context, content in
          context.constrainable.animation = .named(content.animationName)
          context.constrainable.contentMode = .scaleAspectFit

          for playRange in content.playRanges {
            context.constrainable.setPlayRange(
              fromMarker: playRange.fromMarker,
              toMarker: playRange.toMarker,
              event: playRange.event)
          }
        })
Repository: airbnb/lottie-ios
File: Sources/Private/Model/DotLottie/DotLottieAnimation.swift (Swift)

Code before:
import Foundation

// MARK: - DotLottieAnimation

struct DotLottieAnimation: Codable {
  /// Id of Animation
  var id: String

  /// Loop enabled
  var loop: Bool? = false

  /// Animation Playback Speed
  var speed: Double? = 1

  /// 1 or -1
  var direction: Int? = 1

  /// mode - "bounce" | "normal"
  var mode: DotLottieAnimationMode? = .normal

  /// Loop mode for animation
  var loopMode: LottieLoopMode {
    switch mode {
    case .bounce:
      return .autoReverse
    default:
      return ((loop ?? false) ? .loop : .playOnce)
    }
  }

  /// Animation speed
  var animationSpeed: Double {
    (speed ?? 1) * Double(direction ?? 1)
  }

  /// Loads `LottieAnimation` from `animationUrl`
  /// - Returns: Deserialized `LottieAnimation`. Optional.
  func animation(url: URL) throws -> LottieAnimation {
    let animationUrl = url.appendingPathComponent("\(id).json")
    let data = try Data(contentsOf: animationUrl)
    return try LottieAnimation.from(data: data)
  }
}

// MARK: - DotLottieAnimationMode

enum DotLottieAnimationMode: String, Codable {
  case normal
  case bounce
}


Review comment (suggestion):
Can we switch exhaustively instead:

```suggestion
    case .normal, nil:
```

Code after:
import Foundation

// MARK: - DotLottieAnimation

struct DotLottieAnimation: Codable {
  /// Id of Animation
  var id: String

  /// Loop enabled
  var loop: Bool? = false

  /// Animation Playback Speed
  var speed: Double? = 1

  /// 1 or -1
  var direction: Int? = 1

  /// mode - "bounce" | "normal"
  var mode: DotLottieAnimationMode? = .normal

  /// Loop mode for animation
  var loopMode: LottieLoopMode {
    switch mode {
    case .bounce:
      return .autoReverse
    case .normal, nil:
      return ((loop ?? false) ? .loop : .playOnce)
    }
  }

  /// Animation speed
  var animationSpeed: Double {
    (speed ?? 1) * Double(direction ?? 1)
  }

  /// Loads `LottieAnimation` from `animationUrl`
  /// - Returns: Deserialized `LottieAnimation`. Optional.
  func animation(url: URL) throws -> LottieAnimation {
    let animationUrl = url.appendingPathComponent("\(id).json")
    let data = try Data(contentsOf: animationUrl)
    return try LottieAnimation.from(data: data)
  }
}

// MARK: - DotLottieAnimationMode

enum DotLottieAnimationMode: String, Codable {
  case normal
  case bounce
}
Repository: airbnb/lottie-ios
File: Sources/Private/Utility/Helpers/View+ValueChanged.swift (Swift)

Code before:
// Created by miguel_jimenez on 7/26/23.
// Copyright © 2023 Airbnb Inc. All rights reserved.

import Combine
import SwiftUI

@available(iOS 13.0, tvOS 13.0, macOS 10.15, *)
extension View {
  /// A backwards compatible wrapper for iOS 14 `onChange`
  @ViewBuilder
  func valueChanged(value: any Equatable, onChange: @escaping (any Equatable) -> Void) -> some View {
    if #available(iOS 14.0, *, macOS 11.0, tvOS 14.0) {
      self.onChange(of: value, perform: onChange)
    } else {
      onReceive(Just(value)) { value in
        onChange(value)
      }
    }
  }
}


Review comment (bug):
I think we'll need to use the old syntax here since Lottie still supports Swift 5.5. CI will probably fail with this written as-is.

```suggestion
  func valueChanged<T: Equatable>(value: T, onChange: @escaping (T) -> Void) -> some View {
```

Code after:
// Created by miguel_jimenez on 7/26/23.
// Copyright © 2023 Airbnb Inc. All rights reserved.

import Combine
import SwiftUI

@available(iOS 13.0, tvOS 13.0, macOS 10.15, *)
extension View {
  /// A backwards compatible wrapper for iOS 14 `onChange`
  @ViewBuilder
  func valueChanged<T: Equatable>(value: T, onChange: @escaping (T) -> Void) -> some View {
    if #available(iOS 14.0, *, macOS 11.0, tvOS 14.0) {
      self.onChange(of: value, perform: onChange)
    } else {
      onReceive(Just(value)) { value in
        onChange(value)
      }
    }
  }
}
Repository: airbnb/lottie-ios
File: Sources/Public/Animation/LottieView.swift (Swift)

Code before:
  private var showPlaceholderWhileReloading = false
  private var imageProvider: AnimationImageProvider?
  private var textProvider: AnimationTextProvider = DefaultTextProvider()
  private var fontProvider: AnimationFontProvider = DefaultFontProvider()
  private var configuration: LottieConfiguration = .shared
  private var logger: LottieLogger = .shared
  private var sizing = SwiftUIMeasurementContainerStrategy.automatic
  private let placeholder: (() -> Placeholder)?

  private func loadAnimationIfNecessary() {
    guard let loadAnimation = loadAnimation else { return }

    Task {
      do {
        animationSource = try await loadAnimation()
      } catch {
        logger.warn("Failed to load asynchronous Lottie animation with error: \(error)")
      }
    }
  }

  private func clearAnimationIfNecessary() {
    if loadAnimation != nil, showPlaceholderWhileReloading {
      animationSource = nil
    }
  }
}

// MARK: - DefaultLoadTrigger

/// The default load animation trigger token
private enum DefaultLoadTrigger: Hashable {
  case noneProvided
}


Review comment (suggestion):
I'm not sure about this specific method since it's tightly coupled to the `reloadAnimationTrigger` use case and it doesn't seem like we'd call it again for something else.

Maybe instead organization like this would make sense:

```suggestion
  private func reloadAnimationTriggerDidChange() {
    guard loadAnimation != nil else { return }
    
    if showPlaceholderWhileReloading {
      animationSource = nil
    }
    
    loadAnimationIfNecessary()
}
```

this feels more self-documenting to me, compared to `clearAnimationIfNecessary`

Code after:
    }
  }

  // MARK: Internal

  var configurations = [SwiftUIView<LottieAnimationView, Void>.Configuration]()

  // MARK: Private

  @State private var animationSource: LottieAnimationSource?
  private var reloadAnimationTrigger: AnyEquatable?
  private var loadAnimation: (() async throws -> LottieAnimationSource?)?
  private var showPlaceholderWhileReloading = false
  private var imageProvider: AnimationImageProvider?
  private var textProvider: AnimationTextProvider = DefaultTextProvider()
  private var fontProvider: AnimationFontProvider = DefaultFontProvider()
  private var configuration: LottieConfiguration = .shared
  private var logger: LottieLogger = .shared
  private var sizing = SwiftUIMeasurementContainerStrategy.automatic
  private let placeholder: (() -> Placeholder)?

  private func loadAnimationIfNecessary() {
    guard let loadAnimation = loadAnimation else { return }

    Task {
      do {
        animationSource = try await loadAnimation()
      } catch {
        logger.warn("Failed to load asynchronous Lottie animation with error: \(error)")
      }
    }
  }

  private func reloadAnimationTriggerDidChange() {
    guard loadAnimation != nil else { return }

    if showPlaceholderWhileReloading {
      animationSource = nil
    }

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

Code before:
//
//  LottieImageProvider.swift
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {
    
  var ignoreInternalImageCache: Bool { get set }
    
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

public extension AnimationImageProvider {
    var ignoreInternalImageCache: Bool {
        get { return false }
        set { }
    }
}


Review comment (performance):
Can we invert this to something like `cacheEligible` which defaults to `true`?

```suggestion
  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get set }
```

Code after:
//
//  LottieImageProvider.swift
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    true
  }
}
Repository: airbnb/lottie-ios
File: Sources/Public/ImageProvider/AnimationImageProvider.swift (Swift)

Code before:
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get set }

  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    get { true }
    set { }
  }
}


Review comment (suggestion):
We may as well add a doc comment here for consistency:

```suggestion
  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
```

Code after:
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    true
  }
}
Repository: airbnb/lottie-ios
File: Sources/Public/ImageProvider/AnimationImageProvider.swift (Swift)

Code before:
//
//  LottieImageProvider.swift
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get set }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    get { true }
    set { }
  }
}


Review comment (performance):
I don't think we want this to be mutable

```suggestion
  var cacheEligible: Bool { get }
```

Code after:
//
//  LottieImageProvider.swift
//  lottie-swift
//
//  Created by Brandon Withrow on 1/25/19.
//

import CoreGraphics
import Foundation

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    true
  }
}
Repository: airbnb/lottie-ios
File: Sources/Public/ImageProvider/AnimationImageProvider.swift (Swift)

Code before:

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get set }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    get { true }
    set { }
  }
}


Review comment (performance):
```suggestion
  public var cacheEligible: Bool {
    true
  }
```

Code after:

// MARK: - AnimationImageProvider

/// Image provider is a protocol that is used to supply images to `LottieAnimationView`.
///
/// Some animations require a reference to an image. The image provider loads and
/// provides those images to the `LottieAnimationView`.  Lottie includes a couple of
/// prebuilt Image Providers that supply images from a Bundle, or from a FilePath.
///
/// Additionally custom Image Providers can be made to load images from a URL,
/// or to Cache images.
public protocol AnimationImageProvider {

  /// Whether or not the resulting image of this image provider can be cached by Lottie. Defaults to true.
  /// If true, Lottie may internally cache the result of `imageForAsset`
  var cacheEligible: Bool { get }

  /// The image to display for the given `ImageAsset` defined in the `LottieAnimation` JSON file.
  func imageForAsset(asset: ImageAsset) -> CGImage?
}

extension AnimationImageProvider {
  public var cacheEligible: Bool {
    true
  }
}