Skip to content

Commit

Permalink
Fix crash when diffing to/from empty/nil states to/from non-empty/nil…
Browse files Browse the repository at this point in the history 
… states

Closes #124
Internal: IP-3275
  • Loading branch information
@jessesquires
jessesquires committed Jul 13, 2018
1 parent 01bfcf9 commit 826784d
Show file tree
Hide file tree
Showing 10 changed files with 132 additions and 122 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ The changelog for `ReactiveLists`. Also see the [releases](https://github.com/pl

- Removed undocumented initializers for `CollectionSectionViewModel` (the ones that received `headerHeight:` and/or `footerHeight:`) ([#123](https://github.com/plangrid/ReactiveLists/pull/123), [@jessesquires](https://github.com/jessesquires))

- `CollectionViewDriver.automaticDiffingEnabled` is no longer public ([#125](https://github.com/plangrid/ReactiveLists/pull/125), [@jessesquires](https://github.com/jessesquires))

### Fixed

- Fixed a crash in diffing when transitioning to or from empty/nil states ([#125](https://github.com/plangrid/ReactiveLists/pull/125), [@jessesquires](https://github.com/jessesquires))

- Fixed incorrect calculation for `TableViewModel.isEmpty`. It now correctly returns true only if all sections return `true` for `isEmpty`. ([#123](https://github.com/plangrid/ReactiveLists/pull/123), [@jessesquires](https://github.com/jessesquires))

### New
Expand Down
18 changes: 5 additions & 13 deletions ReactiveLists.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,33 +201,26 @@
path = Example;
sourceTree = "<group>";
};
257A97BB2017A5AA00164403 /* Fixtures */ = {
isa = PBXGroup;
children = (
257A97BC2017A5AA00164403 /* TestCollectionViewModels.swift */,
257A97BD2017A5AA00164403 /* TestTableViewModels.swift */,
);
path = Fixtures;
sourceTree = "<group>";
};
257A97C32017A80B00164403 /* TableView */ = {
isa = PBXGroup;
children = (
257A97C52017A80B00164403 /* TableViewDriverTests.swift */,
257A97C62017A80B00164403 /* TableViewDiffingTests.swift */,
257A97C52017A80B00164403 /* TableViewDriverTests.swift */,
257A97C42017A80B00164403 /* TableViewMocks.swift */,
257A97C72017A80B00164403 /* TableViewModelTests.swift */,
257A97BD2017A5AA00164403 /* TestTableViewModels.swift */,
);
path = TableView;
sourceTree = "<group>";
};
257A97C82017A80B00164403 /* CollectionView */ = {
isa = PBXGroup;
children = (
257A97DB2017AA3500164403 /* CollectionViewMocks.swift */,
25B1B0B720195F160036545F /* CollectionViewDriverDiffingTests.swift */,
257A97C92017A80B00164403 /* CollectionViewDriverTests.swift */,
257A97DB2017AA3500164403 /* CollectionViewMocks.swift */,
257A97CA2017A80B00164403 /* CollectionViewModelTests.swift */,
25B1B0B720195F160036545F /* CollectionViewDriverDiffingTests.swift */,
257A97BC2017A5AA00164403 /* TestCollectionViewModels.swift */,
);
path = CollectionView;
sourceTree = "<group>";
Expand Down Expand Up @@ -288,7 +281,6 @@
isa = PBXGroup;
children = (
257A97C82017A80B00164403 /* CollectionView */,
257A97BB2017A5AA00164403 /* Fixtures */,
258E319F1F0D8CBC00D6F324 /* Info.plist */,
257A97C32017A80B00164403 /* TableView */,
257A97CB2017A80B00164403 /* Utils */,
Expand Down
80 changes: 42 additions & 38 deletions Sources/CollectionViewDriver.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,15 @@ public class CollectionViewDriver: NSObject {
assert(Thread.isMainThread, "Must set \(#function) on main thread")
}
didSet {
self._collectionViewModelDidChange()
self._collectionViewModelDidChange(from: oldValue)
}
}

/// If this property is set to `true`, updating the `collectionViewModel` will always
/// automatically lead to updating the UI state of the `UICollectionView`, even if cells/sections
/// were moved/inserted/deleted.
///
/// For details, see the documentation for `CollectionViewDriver.collectionViewModel`.
public let automaticDiffingEnabled: Bool

private var _shouldDeselectUponSelection: Bool
private var _collectionViewDiffer: CollectionViewDiffCalculator<DiffingKey, DiffingKey>?
private var _didReceiveFirstNonNilValue = false

private var _differ: CollectionViewDiffCalculator<DiffingKey, DiffingKey>?
private let _automaticDiffingEnabled: Bool
private var _didReceiveFirstNonNilNonEmptyValue = false

// MARK: Initialization

Expand All @@ -76,16 +71,15 @@ public class CollectionViewDriver: NSObject {
collectionView: UICollectionView,
collectionViewModel: CollectionViewModel? = nil,
shouldDeselectUponSelection: Bool = true,
automaticDiffingEnabled: Bool = true
) {
automaticDiffingEnabled: Bool = true) {
self.collectionViewModel = collectionViewModel
self.collectionView = collectionView
self.automaticDiffingEnabled = automaticDiffingEnabled
self._automaticDiffingEnabled = automaticDiffingEnabled
self._shouldDeselectUponSelection = shouldDeselectUponSelection
super.init()
collectionView.dataSource = self
collectionView.delegate = self
self._collectionViewModelDidChange()
self._collectionViewModelDidChange(from: nil)
}

// MARK: Change and UI Update Handling
Expand Down Expand Up @@ -134,36 +128,45 @@ public class CollectionViewDriver: NSObject {

// MARK: Private

private func _collectionViewModelDidChange() {
guard let newModel = self.collectionViewModel else {
self.collectionView.reloadData()
return
private func _collectionViewModelDidChange(from: CollectionViewModel?) {
if let newModel = self.collectionViewModel {
self.collectionView.registerViews(for: newModel)
}

self.collectionView.registerViews(for: newModel)
let previousStateNilOrEmpty = (from == nil || from!.isEmpty)
let nextStateNilOrEmpty = (self.collectionViewModel == nil || self.collectionViewModel!.isEmpty)

if self.automaticDiffingEnabled {
if !self._didReceiveFirstNonNilValue {
// For the first non-nil value, we want to reload data, to avoid a weird
// animation where we animate in the initial state
self.collectionView.reloadData()
self._didReceiveFirstNonNilValue = true
// 1. we're moving *from* a nil/empty state
// or
// 2. we're moving *to* a nil/empty state
// in either case, simply reload and short-circuit, no need to diff
if previousStateNilOrEmpty || nextStateNilOrEmpty {
self.collectionView.reloadData()

if self._automaticDiffingEnabled
&& self.collectionViewModel != nil
&& !self._didReceiveFirstNonNilNonEmptyValue {
// Special case for the first non-nil value
// Now that we have this initial state, setup the differ with that initial state,
// so that the diffing works properly from here on out
self._collectionViewDiffer = CollectionViewDiffCalculator<DiffingKey, DiffingKey>(
self._didReceiveFirstNonNilNonEmptyValue = true
self._differ = CollectionViewDiffCalculator<DiffingKey, DiffingKey>(
collectionView: self.collectionView,
initialSectionedValues: newModel.diffingKeys
)
} else if self._didReceiveFirstNonNilValue {
// If the current collection view model is empty, default to an empty set of diffing keys
if let differ = self._collectionViewDiffer {
let diffingKeys = newModel.diffingKeys
differ.sectionedValues = diffingKeys
self.refreshViews()
} else {
self.refreshViews()
}
initialSectionedValues: self.collectionViewModel!.diffingKeys)
}
return
}

guard let newModel = self.collectionViewModel else { return }

if self._automaticDiffingEnabled && self._didReceiveFirstNonNilNonEmptyValue {

// If the current collection view model is empty, default to an empty set of diffing keys
if let differ = self._differ {
differ.sectionedValues = newModel.diffingKeys
self.refreshViews()
} else {
self.refreshViews()
}
} else {
self.refreshViews()
Expand Down Expand Up @@ -194,7 +197,8 @@ extension CollectionViewDriver: UICollectionViewDataSource {

/// :nodoc:
public func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
return self.collectionViewModel?[section]?.cellViewModels.count ?? 0
guard let sectionModel = self.collectionViewModel?[section] else { return 0 }
return sectionModel.cellViewModels.count
}

/// :nodoc:
Expand Down
83 changes: 44 additions & 39 deletions Sources/TableViewDriver.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ open class TableViewDriver: NSObject {
assert(Thread.isMainThread, "Must set \(#function) on main thread")
}
didSet {
self._tableViewModelDidChange()
self._tableViewModelDidChange(from: oldValue)
}
}

Expand All @@ -65,15 +65,10 @@ open class TableViewDriver: NSObject {
/// The animation for row deletions.
public var deletionAnimation: UITableViewRowAnimation = .fade

/// If this property is set to `true`, updating the `tableViewModel` will always
/// automatically lead to updating the UI state of the `UITableView`, even if cells/sections
/// were moved/inserted/deleted.
///
/// For details, see the documentation for `TableViewDriver.tableViewModel`.
private let _automaticDiffingEnabled: Bool

private let _shouldDeselectUponSelection: Bool
private var _tableViewDiffer: TableViewDiffCalculator<DiffingKey, DiffingKey>?

private var _differ: TableViewDiffCalculator<DiffingKey, DiffingKey>?
private let _automaticDiffingEnabled: Bool
private var _didReceiveFirstNonNilNonEmptyValue = false

/// Initializes a data source that drives a `UITableView` based on a `TableViewModel`.
Expand All @@ -90,16 +85,15 @@ open class TableViewDriver: NSObject {
tableView: UITableView,
tableViewModel: TableViewModel? = nil,
shouldDeselectUponSelection: Bool = true,
automaticDiffingEnabled: Bool = true
) {
automaticDiffingEnabled: Bool = true) {
self.tableViewModel = tableViewModel
self.tableView = tableView
self._automaticDiffingEnabled = automaticDiffingEnabled
self._shouldDeselectUponSelection = shouldDeselectUponSelection
super.init()
tableView.dataSource = self
tableView.delegate = self
self._tableViewModelDidChange()
self._tableViewModelDidChange(from: nil)
}

// MARK: Change and UI Update Handling
Expand Down Expand Up @@ -161,40 +155,51 @@ open class TableViewDriver: NSObject {

// MARK: Private

private func _tableViewModelDidChange() {
guard let newModel = self.tableViewModel, !newModel.isEmpty else {
self.tableView.reloadData()
return
private func _tableViewModelDidChange(from: TableViewModel?) {
if let newModel = self.tableViewModel {
self.tableView.registerViews(for: newModel)
}

self.tableView.registerViews(for: newModel)
let previousStateNilOrEmpty = (from == nil || from!.isEmpty)
let nextStateNilOrEmpty = (self.tableViewModel == nil || self.tableViewModel!.isEmpty)

if self._automaticDiffingEnabled {
if !self._didReceiveFirstNonNilNonEmptyValue {
// For the first non-nil value, we want to reload data, to avoid a weird
// animation where we animate in the initial state
self.tableView.reloadData()
self._didReceiveFirstNonNilNonEmptyValue = true
// 1. we're moving *from* a nil/empty state
// or
// 2. we're moving *to* a nil/empty state
// in either case, simply reload and short-circuit, no need to diff
if previousStateNilOrEmpty || nextStateNilOrEmpty {
self.tableView.reloadData()

if self._automaticDiffingEnabled
&& self.tableViewModel != nil
&& !self._didReceiveFirstNonNilNonEmptyValue {
// Special case for the first non-nil value
// Now that we have this initial state, setup the differ with that initial state,
// so that the diffing works properly from here on out
self._tableViewDiffer = TableViewDiffCalculator<DiffingKey, DiffingKey>(
self._didReceiveFirstNonNilNonEmptyValue = true
self._differ = TableViewDiffCalculator<DiffingKey, DiffingKey>(
tableView: self.tableView,
initialSectionedValues: newModel.diffingKeys
)
self._tableViewDiffer?.insertionAnimation = self.insertionAnimation
self._tableViewDiffer?.deletionAnimation = self.deletionAnimation
} else if self._didReceiveFirstNonNilNonEmptyValue {
// If the current table view model is empty, default to an empty set of diffing keys
if let differ = self._tableViewDiffer {
let diffingKeys = newModel.diffingKeys
let diff = Dwifft.diff(lhs: differ.sectionedValues, rhs: diffingKeys)
differ.sectionedValues = diffingKeys
let context: TableRefreshContext = !diff.isEmpty ? .rowsModified : .contentOnly
self.refreshViews(refreshContext: context)
} else {
self.refreshViews()
}
initialSectionedValues: self.tableViewModel!.diffingKeys)
}
return
}

guard let newModel = self.tableViewModel else { return }

if self._automaticDiffingEnabled && self._didReceiveFirstNonNilNonEmptyValue {

self._differ?.insertionAnimation = self.insertionAnimation
self._differ?.deletionAnimation = self.deletionAnimation

// If the current table view model is empty, default to an empty set of diffing keys
if let differ = self._differ {
let diffingKeys = newModel.diffingKeys
let diff = Dwifft.diff(lhs: differ.sectionedValues, rhs: diffingKeys)
differ.sectionedValues = diffingKeys
let context: TableRefreshContext = !diff.isEmpty ? .rowsModified : .contentOnly
self.refreshViews(refreshContext: context)
} else {
self.refreshViews()
}
} else {
self.refreshViews()
Expand Down
35 changes: 10 additions & 25 deletions Tests/CollectionView/CollectionViewDriverDiffingTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,8 @@ final class CollectionViewDriverDiffingTests: XCTestCase {

self.collectionViewDataSource.collectionViewModel = updatedModel

XCTAssertEqual(self.mockCollectionView.callsToInsertItems.count, 1)
XCTAssertEqual(
self.mockCollectionView.callsToInsertItems[0],
[IndexPath(row: 0, section: 0)]
)
XCTAssertEqual(self.mockCollectionView.callsToInsertItems.count, 0)
XCTAssertEqual(self.mockCollectionView.callsToReloadData, 3)
}

/// Tests that changes to individual sections result in the correct calls to update the
Expand All @@ -76,38 +73,26 @@ final class CollectionViewDriverDiffingTests: XCTestCase {
/// - Note: We're only testing one type of section update since this is sufficient to test the
/// communication between the diffing lib and the collection view. The diffing lib itself has
/// extensive tests for the various diffing scenarios.

/// This is broken. Dwifft algo does not work.
func testChangingSections() {
let section = CollectionSectionViewModel(cellViewModels: generateCollectionCellViewModels(), diffingKey: "2")

let initialModel = CollectionViewModel(
sectionModels: [
CollectionSectionViewModel(
cellViewModels: [],
diffingKey: "1"
),
CollectionSectionViewModel(
cellViewModels: [],
diffingKey: "2"
)
CollectionSectionViewModel( cellViewModels: generateCollectionCellViewModels(), diffingKey: "1"),
section,
]
)

self.collectionViewDataSource.collectionViewModel = initialModel

let updatedModel = CollectionViewModel(
sectionModels: [
CollectionSectionViewModel(
cellViewModels: [],
diffingKey: "2"
)
]
)
let updatedModel = CollectionViewModel(sectionModels: [section])

self.collectionViewDataSource.collectionViewModel = updatedModel

XCTAssertEqual(self.mockCollectionView.callsToDeleteSections.count, 1)
XCTAssertEqual(
self.mockCollectionView.callsToDeleteSections[0],
IndexSet(integer: 0)
)
XCTAssertEqual(self.mockCollectionView.callsToDeleteSections[0], IndexSet(integer: 0))
}
}

Expand Down
Loading

0 comments on commit 826784d

Please sign in to comment.