Skip to content

Commit

Permalink
Stability, Layout, and Logging Enhancements; Added Support for Legacy…
Browse files Browse the repository at this point in the history 
… Edge Insets Functionality (#202)

* Stability, Layout, and Logging Enhancements; Added Support for Legacy Edge Insets Functionality

Squashed Commit of the following:

commit 89ef650
Author: Will Spurgeon <[email protected]>
Date:   Thu Nov 9 10:16:10 2017 -0500

    Added additional verbose logging to refresh/reloading of data.

commit cee70e8
Author: Will Spurgeon <[email protected]>
Date:   Thu Nov 2 15:05:39 2017 -0400

    Clear prefetch index paths upon invalidation. Written by nlobue

commit c760425
Author: Will Spurgeon <[email protected]>
Date:   Mon Oct 30 16:50:56 2017 -0400

    Fix for a crash that can occur if the first index is greater the number of items.

commit a838e8d
Author: Will Spurgeon <[email protected]>
Date:   Mon Oct 23 14:15:58 2017 -0400

    Updated height provider code to be a closure (#191). Fix by vlozko.

commit 5e21936
Author: Will Spurgeon <[email protected]>
Date:   Wed Oct 18 14:06:09 2017 -0400

    Fix for crash

commit 48846ee
Author: Will Spurgeon <[email protected]>
Date:   Fri Oct 13 10:04:59 2017 -0400

    Attempt to fix a crash

commit e6d3c6d
Author: Will Spurgeon <[email protected]>
Date:   Thu Oct 5 17:28:58 2017 -0400

    Added a flag that determines which version of the inset updating code is run.

* Fix crash when changing repeat counts; Add unit tests for it

* Line spacing fix, per PR comment

* Replace XCTAssertEqual with XCTAssertEqualWithAccuracy due to Swift version
  • Loading branch information
@mamccarthy @jay18001
mamccarthy authored and jay18001 committed Nov 20, 2017
1 parent 0b8607f commit daf9dd4
Show file tree
Hide file tree
Showing 6 changed files with 152 additions and 20 deletions.
2 changes: 1 addition & 1 deletion BrickKit.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@
93D9EBE21DA4057000D8C87A /* BrickUtils.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BrickUtils.swift; sourceTree = "<group>"; };
93D9EBE31DA4057000D8C87A /* FatalError.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FatalError.swift; sourceTree = "<group>"; };
93D9EBE51DA4057000D8C87A /* BrickCollectionView+BrickLayoutDataSource.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "BrickCollectionView+BrickLayoutDataSource.swift"; sourceTree = "<group>"; };
93D9EBE61DA4057000D8C87A /* BrickCollectionView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; lineEnding = 0; path = BrickCollectionView.swift; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.swift; };
93D9EBE61DA4057000D8C87A /* BrickCollectionView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; lineEnding = 0; path = BrickCollectionView.swift; sourceTree = "<group>"; };
93D9EBE71DA4057000D8C87A /* BrickViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BrickViewController.swift; sourceTree = "<group>"; };
93D9EC1A1DA4057900D8C87A /* CardLayoutBehaviorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CardLayoutBehaviorTests.swift; sourceTree = "<group>"; };
93D9EC1C1DA4057900D8C87A /* HideLayoutBehaviorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = HideLayoutBehaviorTests.swift; sourceTree = "<group>"; };
Expand Down
22 changes: 15 additions & 7 deletions Source/Cells/BrickCell.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,23 @@ open class BrickCell: BaseBrickCell {
return UIEdgeInsetsMake(defaultTopConstraintConstant, defaultLeftConstraintConstant, defaultBottomConstraintConstant, defaultRightConstraintConstant)
}

open var needsLegacyEdgeInsetFunctionality = false
private var didUpdateEdgeInsets: Bool = false
@objc open dynamic var edgeInsets: UIEdgeInsets = UIEdgeInsets.zero {
didSet {
if edgeInsets == oldValue {

if edgeInsets == oldValue && needsLegacyEdgeInsetFunctionality {
return
}

self.topSpaceConstraint?.constant = edgeInsets.top
self.bottomSpaceConstraint?.constant = edgeInsets.bottom
self.leftSpaceConstraint?.constant = edgeInsets.left
self.rightSpaceConstraint?.constant = edgeInsets.right
didUpdateEdgeInsets = true

if needsLegacyEdgeInsetFunctionality {
didUpdateEdgeInsets = true
}
}
}

Expand Down Expand Up @@ -232,13 +238,15 @@ open class BrickCell: BaseBrickCell {
return layoutAttributes
}

if !didUpdateEdgeInsets {
guard let brickAttributes = layoutAttributes as? BrickLayoutAttributes, brickAttributes.isEstimateSize else {
return layoutAttributes
if needsLegacyEdgeInsetFunctionality {
if !didUpdateEdgeInsets {
guard let brickAttributes = layoutAttributes as? BrickLayoutAttributes, brickAttributes.isEstimateSize else {
return layoutAttributes
}
}
didUpdateEdgeInsets = false
}
didUpdateEdgeInsets = false


let preferred = layoutAttributes

// We're inverting the frame because the given frame is already transformed
Expand Down
7 changes: 6 additions & 1 deletion Source/Layout/BrickFlowLayout.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,13 @@ open class BrickFlowLayout: UICollectionViewLayout, BrickLayout {

var updated = false
for section in currentSections {
// For deletion cases, we don't want to recalculate brick layaout sections for indexes
// that no longer exist in the collection view.
let sectionCount = _collectionView.numberOfItems(inSection: section.sectionIndex)
updateSection(section, updatedAttributes: updateAttributes, action: {
updated = section.continueCalculatingCells() || updated
if section.numberOfItems <= sectionCount {
updated = section.continueCalculatingCells() || updated
}
})
}

Expand Down
12 changes: 7 additions & 5 deletions Source/Layout/BrickLayoutSection.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,13 +330,15 @@ internal class BrickLayoutSection {

let numberOfItems = self.numberOfItems

for index in firstIndex..<numberOfItems {
// Create or Update an attribute at an index. If returned true, continue calculating. If not, break
if !createOrUpdateAttribute(at: index, with: dataSource, x: &x, y: &y, maxY: &maxY, force: false, invalidate: invalidate, frameOfInterest: frameOfInterest, updatedAttributes: updatedAttributes) {
break
if firstIndex < numberOfItems {
for index in firstIndex..<numberOfItems {
// Create or Update an attribute at an index. If returned true, continue calculating. If not, break
if !createOrUpdateAttribute(at: index, with: dataSource, x: &x, y: &y, maxY: &maxY, force: false, invalidate: invalidate, frameOfInterest: frameOfInterest, updatedAttributes: updatedAttributes) {
break
}
}
}

// If rows need to be aligned, make sure the previous lines are checked
handleRow(for: attributes.count - 1, maxHeight: maxY - y, updatedAttributes: updatedAttributes)

Expand Down
44 changes: 38 additions & 6 deletions Source/ViewControllers/BrickCollectionView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ open class BrickCollectionView: UICollectionView {

internal fileprivate(set) var isConfiguringCollectionBrick: Bool = false

internal fileprivate(set) var sectionsWithInsertion: [Int] = []

// MARK: - Overrides

open override var frame: CGRect {
Expand Down Expand Up @@ -260,6 +262,19 @@ open class BrickCollectionView: UICollectionView {
}, completion: completion)
}

open override func reloadData() {
BrickLogger.logVerbose("Reloading all data.")
super.reloadData()
}

open override func performBatchUpdates(_ updates: (() -> Void)?, completion: ((Bool) -> Void)? = nil) {
BrickLogger.logVerbose("Will perform batch updates.")
super.performBatchUpdates(updates, completion: { completed in
BrickLogger.logVerbose("Did finish perform batch updates.")
completion?(completed)
})
}

// MARK: - Private methods

/// Invalidate the layout for bounds change
Expand Down Expand Up @@ -294,6 +309,7 @@ open class BrickCollectionView: UICollectionView {
/// - parameter invalidate: Flag that indicates if the function should also invalidate the layout
/// Default to true, but could be set to false if it's part of a bigger invalidation
open func reloadBrickWithIdentifier(_ identifier: String, andIndex index: Int, invalidate: Bool = true) {
BrickLogger.logVerbose("Reload brick with identifier: \(identifier)")
let indexPaths = section.indexPathsForBricksWithIdentifier(identifier, index: index, in: collectionInfo)
self.reloadItems(at: indexPaths)

Expand All @@ -317,6 +333,7 @@ open class BrickCollectionView: UICollectionView {
}

fileprivate func updateItems(at index: Int, for identifier: String, itemCount: Int, updateAction: @escaping (_ indexPaths: [IndexPath]) -> Void, completion: ((_ completed: Bool, _ indexPaths: [IndexPath]) -> Void)? = nil) {
BrickLogger.logVerbose("Update items for identifier: \(identifier)")
guard let originIndexPath = indexPathsForBricksWithIdentifier(identifier, index: index).first else {
invalidateRepeatCounts()
return
Expand Down Expand Up @@ -357,6 +374,8 @@ open class BrickCollectionView: UICollectionView {
let lastItem = lastIndexPath.item
let section = lastIndexPath.section

self?.sectionsWithInsertion.append(section)

var offsetIndexPaths: [IndexPath] = []

for offset in 1...insertedIndexPaths.count {
Expand Down Expand Up @@ -432,6 +451,7 @@ open class BrickCollectionView: UICollectionView {
///
/// - parameter completion: Completion Block
open func invalidateRepeatCounts(reloadAllSections: Bool = false, completion: ((_ completed: Bool, _ insertedIndexPaths: [IndexPath], _ deletedIndexPaths: [IndexPath]) -> Void)? = nil) {
BrickLogger.logVerbose("Invalidate repeat counts.\(reloadAllSections ? " Reloading all sections." : ""))")
var insertedIndexPaths: [IndexPath]!
var deletedIndexPaths: [IndexPath]!

Expand All @@ -444,10 +464,18 @@ open class BrickCollectionView: UICollectionView {
}
}

fileprivate func clearPrefetchAttributes() {
for section in sectionsWithInsertion {
self.prefetchAttributeIndexPaths[section]?.removeAll()
}
}

fileprivate func invalidateRepeatCountsWithoutPerformBatchUpdates(_ reloadAllSections: Bool) -> (insertedIndexPaths: [IndexPath], deletedIndexPaths: [IndexPath]) {

let brickSection = self.section

clearPrefetchAttributes()

var insertedIndexPaths = [IndexPath]()
var deletedIndexPaths = [IndexPath]()
var reloadIndexPaths = [IndexPath]()
Expand Down Expand Up @@ -535,7 +563,7 @@ open class BrickCollectionView: UICollectionView {
var removedIndexPaths = [IndexPath]()
var reloadIndexPaths = [IndexPath]()
let sectionsToReload = NSMutableIndexSet()

BrickLogger.logVerbose("Reload Bricks with identifiers: \(identifiers).")
for identifier in identifiers {
let indexPaths = self.section.indexPathsForBricksWithIdentifier(identifier, in: collectionInfo)
for indexPath in indexPaths {
Expand All @@ -554,10 +582,6 @@ open class BrickCollectionView: UICollectionView {
}
}

if !shouldReloadCell {
return
}

if !insertedIndexPaths.isEmpty {
self.insertItems(at: insertedIndexPaths)
}
Expand All @@ -576,6 +600,7 @@ open class BrickCollectionView: UICollectionView {
}

fileprivate func reloadBrickSection(_ brickSection: BrickSection, insertedIndexPaths: inout [IndexPath], removedIndexPaths: inout [IndexPath], sectionsToReload: NSMutableIndexSet) {
BrickLogger.logVerbose("Reload brick section: \(brickSection.identifier)")
let updatedSections = reloadSectionWithIdentifier(brickSection.identifier)

for (section, count) in updatedSections {
Expand Down Expand Up @@ -607,7 +632,14 @@ open class BrickCollectionView: UICollectionView {
/// - returns: A dictionary with the section as the key and the value is the change count
fileprivate func reloadSectionWithIdentifier(_ identifier: String) -> [Int: Int] {
let oldCounts = self.section.currentSectionCounts(in: collectionInfo)
self.section.invalidateCounts(in: collectionInfo)

if let matchingIndex = self.indexPathsForBricksWithIdentifier(identifier).first,
let brickSectionCell = cellForItem(at: matchingIndex) as? BrickSectionCell,
let brickSection = brickSectionCell._brick as? BrickSection {

brickSection.invalidateCounts(in: collectionInfo)
}

let newCounts = self.section.currentSectionCounts(in: collectionInfo)

var changedSections: [Int: Int] = [:]
Expand Down
85 changes: 85 additions & 0 deletions Tests/ViewControllers/BrickViewControllerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,21 @@ class PreviewViewController: BrickViewController, BrickViewControllerPreviewing
}

class BrickViewControllerTests: XCTestCase {
private class RepeatCountDelegate: BrickRepeatCountDataSource {
var repeatCount = 0
var increment = 0

init(startingCount: Int, increment: Int) {
self.repeatCount = startingCount
self.increment = increment
}

func repeatCount(for identifier: String, with collectionIndex: Int, collectionIdentifier: String) -> Int {
let returnValue = repeatCount
repeatCount += increment
return returnValue
}
}

var brickViewController: BrickViewController!
var width:CGFloat!
Expand Down Expand Up @@ -565,6 +580,76 @@ class BrickViewControllerTests: XCTestCase {
XCTAssertEqual(brickViewController.collectionViewLayout.collectionViewContentSize, CGSize(width: width, height: 50))
}

func testChangingRepeatCountWhileInvalidatingAndReloading() {
let sectionIdentifier = "repeated brick section"
let brick = GenericBrick<UIView>("", width: .ratio(ratio: 1.0), height: .fixed(size: 50.0)) { _ in }
let sectionWithMultipleBricks = BrickSection(sectionIdentifier, bricks: [brick])
let repeatCountDelegate = RepeatCountDelegate(startingCount: 0, increment: 1)
sectionWithMultipleBricks.repeatCountDataSource = repeatCountDelegate
brickViewController.brickCollectionView.setupSectionAndLayout(BrickSection("", bricks: [sectionWithMultipleBricks]))

// Repeat counts incrementing by 1
brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: false)
brickViewController.brickCollectionView.invalidateRepeatCounts()

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: true)
brickViewController.brickCollectionView.invalidateRepeatCounts()

XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 2), 2)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 2), 2)

// Repeat counts decrementing by 1
repeatCountDelegate.repeatCount = 10
repeatCountDelegate.increment = -1

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: false)
brickViewController.brickCollectionView.invalidateRepeatCounts()

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: true)
brickViewController.brickCollectionView.invalidateRepeatCounts()

XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 2), 9)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 2), 9)

// Repeat counts incrementing by 5
XCTAssertNotNil(brickViewController)

repeatCountDelegate.repeatCount = 100
repeatCountDelegate.increment = 5

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: false)
brickViewController.brickCollectionView.invalidateRepeatCounts()

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: true)
brickViewController.brickCollectionView.invalidateRepeatCounts()

XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 2), 105)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 2), 105)

// Repeat counts decrementing by 5
XCTAssertNotNil(brickViewController)

repeatCountDelegate.repeatCount = 100
repeatCountDelegate.increment = -5

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: false)
brickViewController.brickCollectionView.invalidateRepeatCounts()

brickViewController.reloadBricksWithIdentifiers([sectionWithMultipleBricks.identifier], shouldReloadCell: true)
brickViewController.brickCollectionView.invalidateRepeatCounts()

// If the test method makes it this far, then it didn't crash for NSInternalConsistencyException, which is the main issue this method is testing for.
XCTAssertNotNil(brickViewController)
XCTAssertEqual(brickViewController.collectionView!.numberOfSections, 3)
XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 0), 1)
XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 1), 1)
XCTAssertEqual(brickViewController.collectionView!.numberOfItems(inSection: 2), 95)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfSections, 3)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 0), 1)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 1), 1)
XCTAssertEqual(brickViewController.brickCollectionView.numberOfItems(inSection: 2), 95)
}

func testUpdateFrame() {
let section = BrickSection("Test Section", bricks: [
DummyBrick("Brick 1", height: .fixed(size: 50)),
Expand Down

0 comments on commit daf9dd4

Please sign in to comment.