From 479b2b38fcc834cbcd41093e5c488a3bf11d2408 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thor=20Fr=C3=B8lich?= Date: Tue, 3 Dec 2019 12:17:39 +0100 Subject: [PATCH] Fix NSInternalInconsistencyException crash (#6) * Added unit test to verify updates sent to UIKit views * Added safeguard against incremental updates likely to crash UITableView * Pinned SwiftyJSON pod to version 4.2 * Added naive safeguard against `NSInternalInconsistencyException` during updates of UICollectionView * Removed SwiftyJSON pod version pinning --- Sources/BasicData/BasicDataSource.swift | 1 - .../UICollectionView+Updates.swift | 34 +-- Sources/UITableView/UITableView+Updates.swift | 35 +-- Sources/Updates.swift | 13 +- Tests/UIKitViewUpdateTests.swift | 216 ++++++++++++++++++ 5 files changed, 267 insertions(+), 32 deletions(-) create mode 100644 Tests/UIKitViewUpdateTests.swift diff --git a/Sources/BasicData/BasicDataSource.swift b/Sources/BasicData/BasicDataSource.swift index 727196d..76f01bd 100644 --- a/Sources/BasicData/BasicDataSource.swift +++ b/Sources/BasicData/BasicDataSource.swift @@ -1,5 +1,4 @@ import Foundation -import Dwifft public class BasicDataSource
: DataSourceType where Section: SectionType { diff --git a/Sources/UICollectionView/UICollectionView+Updates.swift b/Sources/UICollectionView/UICollectionView+Updates.swift index 0431104..1aff036 100644 --- a/Sources/UICollectionView/UICollectionView+Updates.swift +++ b/Sources/UICollectionView/UICollectionView+Updates.swift @@ -7,24 +7,26 @@ extension UICollectionView { public var defaultViewUpdate: IndexedUpdateHandler.Observer { return { [weak self] update in - guard let _ = self?.window else { - self?.reloadData() + guard let self = self else { return } + + if self.window == nil || update.isLikelyToCrashUIKitViews { + self.reloadData() return } - self?.performBatchUpdates({ - switch update { - case let .delta(insertedSections, updatedSections, deletedSections, insertedRows, updatedRows, deletedRows): - guard let strongSelf = self else { return } - strongSelf.insertSections(insertedSections) - strongSelf.deleteSections(deletedSections) - strongSelf.reloadSections(updatedSections) - strongSelf.insertItems(at: insertedRows) - strongSelf.deleteItems(at: deletedRows) - strongSelf.reloadItems(at: updatedRows) - case .full: - self?.reloadData() - } - }, completion: { _ in }) + + switch update { + case let .delta(insertedSections, updatedSections, deletedSections, insertedRows, updatedRows, deletedRows): + self.performBatchUpdates({ + self.insertSections(insertedSections) + self.deleteSections(deletedSections) + self.reloadSections(updatedSections) + self.insertItems(at: insertedRows) + self.deleteItems(at: deletedRows) + self.reloadItems(at: updatedRows) + }, completion: { _ in }) + case .full: + self.reloadData() + } } } } diff --git a/Sources/UITableView/UITableView+Updates.swift b/Sources/UITableView/UITableView+Updates.swift index d03c233..24fa4a8 100644 --- a/Sources/UITableView/UITableView+Updates.swift +++ b/Sources/UITableView/UITableView+Updates.swift @@ -7,26 +7,33 @@ extension UITableView { public func defaultViewUpdate(with animation: UITableView.RowAnimation = .fade) -> IndexedUpdateHandler.Observer { return { [weak self] update in - guard let _ = self?.window else { - self?.reloadData() + guard let self = self else { return } + + guard self.window != nil else { + self.reloadData() return } + switch update { case .delta(let insertedSections, let updatedSections, let deletedSections, let insertedRows, let updatedRows, let deletedRows): - self?.beginUpdates() - self?.insertSections(insertedSections, with: animation) - self?.deleteSections(deletedSections, with: animation) - self?.reloadSections(updatedSections, with: animation) - self?.insertRows(at: insertedRows, with: animation) - self?.deleteRows(at: deletedRows, with: animation) - self?.reloadRows(at: updatedRows, with: animation) - self?.endUpdates() + if update.isLikelyToCrashUIKitViews { break } + self.beginUpdates() + self.insertSections(insertedSections, with: animation) + self.deleteSections(deletedSections, with: animation) + self.reloadSections(updatedSections, with: animation) + self.insertRows(at: insertedRows, with: animation) + self.deleteRows(at: deletedRows, with: animation) + self.reloadRows(at: updatedRows, with: animation) + self.endUpdates() + return case .full: - guard let `self` = self else { return } - UIView.transition(with: self, duration: 0.4, options: .transitionCrossDissolve, animations: { - self.reloadData() - }, completion: nil) + break } + + // Fall-back animated full reload + UIView.transition(with: self, duration: 0.4, options: .transitionCrossDissolve, animations: { + self.reloadData() + }, completion: nil) } } } diff --git a/Sources/Updates.swift b/Sources/Updates.swift index 378af7b..1cb06d6 100644 --- a/Sources/Updates.swift +++ b/Sources/Updates.swift @@ -1,6 +1,6 @@ import UIKit -public enum IndexedUpdate { +public enum IndexedUpdate: Equatable { case delta( insertedSections: IndexSet, updatedSections: IndexSet, @@ -28,6 +28,17 @@ public enum IndexedUpdate { return false } } + + var isLikelyToCrashUIKitViews: Bool { + switch self { + case let .delta(insertedSections, updatedSections, deletedSections, insertedRows, updatedRows, deletedRows): + let hasItemUpdates = !(insertedRows.isEmpty && updatedRows.isEmpty && deletedRows.isEmpty) + let hasSectionUpdates = !(insertedSections.isEmpty && updatedSections.isEmpty && deletedSections.isEmpty) + return hasItemUpdates && hasSectionUpdates + case .full: + return false + } + } } public class IndexedUpdateHandler { diff --git a/Tests/UIKitViewUpdateTests.swift b/Tests/UIKitViewUpdateTests.swift new file mode 100644 index 0000000..fd48166 --- /dev/null +++ b/Tests/UIKitViewUpdateTests.swift @@ -0,0 +1,216 @@ +import UIKit +import Quick +import Nimble +@testable import SimpleSource + +protocol DataSourceUpdateable: UIViewController { + typealias Section = BasicIdentifiableSection + var dataSource: BasicDataSource
{ get } + var onViewUpdate: ((IndexedUpdate) ->Void)? { get set } + var viewNumberOfSections: Int { get } + init(sections: [Section]) + func viewNumberOfRows(inSection: Int) -> Int +} + +class UIKitViewUpdateTests: QuickSpec { + typealias Section = BasicIdentifiableSection + typealias DataSource = BasicDataSource
+ + static let cellIdentifier = "Cell" + + struct Item: Equatable, IdentifiableItem { + let itemIdentifier: String + var value: Bool + } + + override func spec() { + let vcTypes: [DataSourceUpdateable.Type] = [TableViewController.self, CollectionViewController.self] + for type in vcTypes { + + describe("A \(type) with a data source") { + + var initialSections: [Section]! + var vc: DataSourceUpdateable! + var window: UIWindow! + + beforeEach { + initialSections = stride(from: 0, to: 3, by: 1).map { index -> Section in + let item = Item(itemIdentifier: "\(index)", value: true) + return Section(sectionIdentifier: "\(index)", items: [item]) + } + + vc = type.init(sections: initialSections) + window = UIWindow() + window.rootViewController = vc + vc.loadViewIfNeeded() + window.makeKeyAndVisible() + } + + + it("updates its view") { + expect(vc.viewNumberOfSections).toEventually(equal(initialSections.count)) + expect(vc.viewNumberOfRows(inSection: 0)).toEventually(equal(initialSections[0].items.count)) + } + + it("performs a simple diff'ed update") { + var sections = initialSections! + sections[0].items[0].value.toggle() + sections[2].items[0].value.toggle() + + var didPerformDiffedUpdate = false + var updateIsLikelyToCrashUITableView = true + vc.onViewUpdate = { update in + updateIsLikelyToCrashUITableView = update.isLikelyToCrashUIKitViews + switch update { + case .delta: + didPerformDiffedUpdate = true + case .full: + break + } + } + + DispatchQueue.main.async { + vc.dataSource.sections = sections + } + + expect(updateIsLikelyToCrashUITableView).toEventually(equal(false)) + expect(didPerformDiffedUpdate).toEventually(equal(true)) + } + + it("survives a complex update") { + // Move all items from section 1 to section 0 and delete section 1 + var sections = initialSections! + sections[0].items.append(contentsOf: sections[1].items) + sections.remove(at: 1) + + // Change the value of the first item in the (previous section 2 but now) section 1 + sections[1].items[0].value.toggle() + + var updateIsLikelyToCrashUITableView = false + vc.onViewUpdate = { update in + updateIsLikelyToCrashUITableView = update.isLikelyToCrashUIKitViews + } + + DispatchQueue.main.async { + vc.dataSource.sections = sections + } + + expect(updateIsLikelyToCrashUITableView).toEventually(equal(true)) + expect(vc.viewNumberOfSections).toEventually(equal(sections.count)) + } + } + } + } +} + +extension UIKitViewUpdateTests { + fileprivate final class TableViewController: UITableViewController, DataSourceUpdateable { + typealias ViewDataSource = TableViewDataSource> + typealias Cell = UITableViewCell + + let sections: [Section] + + var onViewUpdate: ((IndexedUpdate) ->Void)? + + lazy var viewDataSource: ViewDataSource = { + let dataSource = DataSource(sections: self.sections) + let viewFactory = TableViewFactory { _,_ in UIKitViewUpdateTests.cellIdentifier } + let configuration: (Cell, Item, IndexPath) -> Void = { (_, _, _) in } + viewFactory.registerCell( + method: .classBased(Cell.self), + reuseIdentifier: UIKitViewUpdateTests.cellIdentifier, + in: tableView, + configuration: configuration + ) + let defaultViewUpdate = tableView.defaultViewUpdate() + let viewUpdate: IndexedUpdateHandler.Observer = { [weak self] update in + self?.onViewUpdate?(update) + defaultViewUpdate(update) + } + return ViewDataSource( + dataSource: dataSource, + viewFactory: viewFactory, + viewUpdate: viewUpdate + ) + }() + + var dataSource: BasicDataSource> { + return viewDataSource.dataSource + } + + var viewNumberOfSections: Int { return tableView!.numberOfSections } + + func viewNumberOfRows(inSection: Int) -> Int { + return tableView.numberOfRows(inSection: inSection) + } + + init(sections: [Section]) { + self.sections = sections + super.init(nibName: nil, bundle: nil) + } + + required init?(coder: NSCoder) { + fatalError("init(coder:) has not been implemented") + } + + override func viewDidLoad() { + super.viewDidLoad() + tableView.dataSource = viewDataSource + } + } + + fileprivate final class CollectionViewController: UICollectionViewController, DataSourceUpdateable { + typealias ViewDataSource = CollectionViewDataSource> + typealias Cell = UICollectionViewCell + + let sections: [Section] + + var onViewUpdate: ((IndexedUpdate) ->Void)? + + lazy var viewDataSource: ViewDataSource = { + let dataSource = DataSource(sections: self.sections) + let viewFactory = CollectionViewFactory { _,_ in UIKitViewUpdateTests.cellIdentifier } + let configuration: (Cell, Item, IndexPath) -> Void = { (_, _, _) in } + viewFactory.registerCell( + method: .classBased(Cell.self), + reuseIdentifier: UIKitViewUpdateTests.cellIdentifier, + in: collectionView, + configuration: configuration + ) + let defaultViewUpdate = collectionView.defaultViewUpdate + let viewUpdate: IndexedUpdateHandler.Observer = { [weak self] update in + self?.onViewUpdate?(update) + defaultViewUpdate(update) + } + return ViewDataSource( + dataSource: dataSource, + viewFactory: viewFactory, + viewUpdate: viewUpdate + ) + }() + + var dataSource: BasicDataSource> { + return viewDataSource.dataSource + } + + var viewNumberOfSections: Int { return collectionView!.numberOfSections } + + func viewNumberOfRows(inSection: Int) -> Int { + return collectionView.numberOfItems(inSection: inSection) + } + + init(sections: [Section]) { + self.sections = sections + super.init(collectionViewLayout: UICollectionViewFlowLayout()) + } + + required init?(coder: NSCoder) { + fatalError("init(coder:) has not been implemented") + } + + override func viewDidLoad() { + super.viewDidLoad() + collectionView.dataSource = viewDataSource + } + } +}