Pixel aligned CGFloat equality fix (#128)

airbnb · Mar 30, 2024 · f9b27b7 · f9b27b7
1 parent 8b41fe5
commit f9b27b7
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 29 deletions.
diff --git a/MagazineLayout/LayoutCore/Types/ScreenPixelAlignment.swift b/MagazineLayout/LayoutCore/Types/ScreenPixelAlignment.swift
@@ -22,10 +22,12 @@ extension CGFloat {
     (self * scale).rounded() / scale
   }
 
-  /// Tests `self` for approximate equality using the threshold value. For example, 1.48 equals 1.52 if the threshold is 0.05.
-  /// `threshold` will be treated as a positive value by taking its absolute value.
-  func isEqual(to rhs: CGFloat, threshold: CGFloat) -> Bool {
-    abs(self - rhs) <= abs(threshold)
+  /// Tests `self` for approximate equality, first rounding the operands to be pixel-aligned for a screen with the given
+  /// `screenScale`. For example, 1.48 equals 1.52 if the `screenScale` is `2`.
+  func isEqual(to rhs: CGFloat, screenScale: CGFloat) -> Bool {
+    let lhs = alignedToPixel(forScreenWithScale: screenScale)
+    let rhs = rhs.alignedToPixel(forScreenWithScale: screenScale)
+    return lhs == rhs
   }
 
 }
diff --git a/MagazineLayout/Public/MagazineLayout.swift b/MagazineLayout/Public/MagazineLayout.swift
@@ -584,22 +584,21 @@ public final class MagazineLayout: UICollectionViewLayout {
   }
 
   override public func shouldInvalidateLayout(forBoundsChange newBounds: CGRect) -> Bool {
-    // When using the topToBottom layout direction, we only want to invalidate the layout when the
-    // widths differ. When using the bottomToTop layout direction, we want to invalidate on any
-    // size change due to the requirement of  needing to preserve scroll position from the bottom
+    let isSameWidth = currentCollectionView.bounds.size.width.isEqual(
+      to: newBounds.size.width,
+      screenScale: scale)
     let shouldInvalidateDueToSize: Bool
     switch verticalLayoutDirection {
     case .topToBottom:
-      shouldInvalidateDueToSize = !currentCollectionView.bounds.size.width.isEqual(
-        to: newBounds.size.width,
-        threshold: 1 / scale)
+      shouldInvalidateDueToSize = !isSameWidth
     case .bottomToTop:
-      shouldInvalidateDueToSize = !(currentCollectionView.bounds.size.width.isEqual(
-        to: newBounds.size.width,
-        threshold: 1 / scale) &&
-      currentCollectionView.bounds.size.height.isEqual(
+      // When using the topToBottom layout direction, we only want to invalidate the layout when the
+      // widths differ. When using the bottomToTop layout direction, we want to invalidate on any
+      // size change due to the requirement of  needing to preserve scroll position from the bottom
+      let isSameHeight = currentCollectionView.bounds.size.height.isEqual(
         to: newBounds.size.height,
-        threshold: 1 / scale))
+        screenScale: scale)
+      shouldInvalidateDueToSize = !isSameWidth || !isSameHeight
     }
 
     return shouldInvalidateDueToSize || hasPinnedHeaderOrFooter
@@ -638,7 +637,7 @@ public final class MagazineLayout: UICollectionViewLayout {
 
     let isSameHeight = preferredAttributes.size.height.isEqual(
       to: originalAttributes.size.height,
-      threshold: 1 / scale)
+      screenScale: scale)
     let hasNewPreferredHeight = !isSameHeight
 
     switch (preferredAttributes.representedElementCategory, preferredAttributes.representedElementKind) {
@@ -655,7 +654,7 @@ public final class MagazineLayout: UICollectionViewLayout {
           at: preferredAttributes.indexPath)
         let isSameHeight = preferredAttributes.size.height.isEqual(
           to: currentPreferredHeight ?? -.greatestFiniteMagnitude,
-          threshold: 1 / scale)
+          screenScale: scale)
         return hasNewPreferredHeight && !isSameHeight
       case nil:
         return false
@@ -789,7 +788,7 @@ public final class MagazineLayout: UICollectionViewLayout {
     // because the collection view's width can change without a `contentSizeAdjustment` occurring.
     let isSameWidth = collectionView?.bounds.size.width.isEqual(
       to: cachedCollectionViewWidth ?? -.greatestFiniteMagnitude,
-      threshold: 1 / scale)
+      screenScale: scale)
       ?? false
     if !isSameWidth {
       prepareActions.formUnion([.updateLayoutMetrics, .cachePreviousWidth])

diff --git a/Tests/ScreenPixelAlignmentTests.swift b/Tests/ScreenPixelAlignmentTests.swift
@@ -62,17 +62,11 @@ final class ScreenPixelAlignmentTests: XCTestCase {
   // MARK: Approximate equality tests
 
   func testApproximateEquality() {
-    XCTAssert(CGFloat(1.48).isEqual(to: 1.52, threshold: 0.05))
-    XCTAssert(!CGFloat(1.48).isEqual(to: 1.53, threshold: 0.05))
-
-    XCTAssert(CGFloat(1).isEqual(to: 10, threshold: 9))
-    XCTAssert(!CGFloat(1).isEqual(to: 11, threshold: 9))
-
-    XCTAssert(CGFloat(1).isEqual(to: 10, threshold: 9))
-    XCTAssert(!CGFloat(1).isEqual(to: 11, threshold: 9))
-
-    XCTAssert(CGFloat(1.333).isEqual(to: 1.666, threshold: 1 / 3))
-    XCTAssert(!CGFloat(1.332).isEqual(to: 1.666, threshold: 1 / 3))
+    XCTAssert(CGFloat(1.48).isEqual(to: 1.52, screenScale: 2))
+    XCTAssert(!CGFloat(1).isEqual(to: 10, screenScale: 9))
+    XCTAssert(!CGFloat(1).isEqual(to: 10, screenScale: 9))
+    XCTAssert(!CGFloat(1).isEqual(to: 9, screenScale: 9))
+    XCTAssert(!CGFloat(1.333).isEqual(to: 1.666, screenScale: 3))
   }
 
 }