diff --git a/Chatto/Source/ChatController/BaseChatViewController+Changes.swift b/Chatto/Source/ChatController/BaseChatViewController+Changes.swift index 457d166..66a9b54 100644 --- a/Chatto/Source/ChatController/BaseChatViewController+Changes.swift +++ b/Chatto/Source/ChatController/BaseChatViewController+Changes.swift @@ -36,6 +36,11 @@ extension BaseChatViewController: ChatDataSourceDelegateProtocol { public func enqueueModelUpdate(updateType updateType: UpdateType) { let newItems = self.chatDataSource?.chatItems ?? [] + + if self.updatesConfig.coalesceUpdates { + self.updateQueue.flushQueue() + } + self.updateQueue.addTask({ [weak self] (completion) -> () in guard let sSelf = self else { return } @@ -87,79 +92,94 @@ extension BaseChatViewController: ChatDataSourceDelegateProtocol { func updateVisibleCells(changes: CollectionChanges) { // Datasource should be already updated! - func updateCellIfVisible(atIndexPath cellIndexPath: NSIndexPath, newDataIndexPath: NSIndexPath) { - if let cell = self.collectionView.cellForItemAtIndexPath(cellIndexPath) { - let presenter = self.presenterForIndexPath(newDataIndexPath) - presenter.configureCell(cell, decorationAttributes: self.decorationAttributesForIndexPath(newDataIndexPath)) - presenter.cellWillBeShown(cell) // `createModelUpdates` may have created a new presenter instance for existing visible cell so we need to tell it that its cell is visible - } + // Afer performBatchUpdates, indexPathForCell may return a cell refering to the state before the update + // When doing very fast updates this can lead to giving a presenter the wrong cell below in presenter.configureCell(_:, decorationAttributes:) + // For that reason when self.updatesConfig.fastUpdates is enabled, it's also recomended to enable self.updatesConfig.trackVisibleCells + // See more: https://github.com/diegosanchezr/UICollectionViewStressing + + // When self.updatesConfig.fastUpdates is disabled we haven't seen this inconsistency so it's recomended to disable `trackVisibleCells` + + let cellsToUpdate: [NSIndexPath: UICollectionViewCell] + + if self.updatesConfig.trackVisibleCells { + var updatedVisibleCells = updated(collection: self.visibleCells, withChanges: changes) + + updatedVisibleCells.forEach({ (indexPath, cell) in + if cell.hidden || cell.superview == nil { + updatedVisibleCells[indexPath] = nil + } + }) + self.visibleCells = updatedVisibleCells + cellsToUpdate = updatedVisibleCells + } else { + var visibleCells: [NSIndexPath: UICollectionViewCell] = [:] + self.collectionView.indexPathsForVisibleItems().forEach({ (indexPath) in + if let cell = self.collectionView.cellForItemAtIndexPath(indexPath) { + visibleCells[indexPath] = cell + } + }) + cellsToUpdate = updated(collection: visibleCells, withChanges: changes) } - let visibleIndexPaths = Set(self.collectionView.indexPathsForVisibleItems().filter { (indexPath) -> Bool in - return !changes.insertedIndexPaths.contains(indexPath) && !changes.deletedIndexPaths.contains(indexPath) - }) - - var updatedIndexPaths = Set() - for move in changes.movedIndexPaths { - updatedIndexPaths.insert(move.indexPathOld) - updateCellIfVisible(atIndexPath: move.indexPathOld, newDataIndexPath: move.indexPathNew) - } - - // Update remaining visible cells - let remaining = visibleIndexPaths.subtract(updatedIndexPaths) - for indexPath in remaining { - updateCellIfVisible(atIndexPath: indexPath, newDataIndexPath: indexPath) + cellsToUpdate.forEach { (indexPath, cell) in + let presenter = self.presenterForIndexPath(indexPath) + presenter.configureCell(cell, decorationAttributes: self.decorationAttributesForIndexPath(indexPath)) + presenter.cellWillBeShown(cell) // `createModelUpdates` may have created a new presenter instance for existing visible cell so we need to tell it that its cell is visible } } - func performBatchUpdates( - updateModelClosure updateModelClosure: () -> Void, - changes: CollectionChanges, - updateType: UpdateType, - completion: () -> Void) { - let shouldScrollToBottom = updateType != .Pagination && self.isScrolledAtBottom() - let (oldReferenceIndexPath, newReferenceIndexPath) = self.referenceIndexPathsToRestoreScrollPositionOnUpdate(itemsBeforeUpdate: self.chatItemCompanionCollection, changes: changes) - let oldRect = self.rectAtIndexPath(oldReferenceIndexPath) - let myCompletion = { - // Found that cells may not match correct index paths here yet! (see comment below) - // Waiting for next loop seems to fix the issue - dispatch_async(dispatch_get_main_queue(), { () -> Void in - completion() - }) - } + func performBatchUpdates(updateModelClosure updateModelClosure: () -> Void, + changes: CollectionChanges, + updateType: UpdateType, + completion: () -> Void) { + let shouldScrollToBottom = updateType != .Pagination && self.isScrolledAtBottom() + let (oldReferenceIndexPath, newReferenceIndexPath) = self.referenceIndexPathsToRestoreScrollPositionOnUpdate(itemsBeforeUpdate: self.chatItemCompanionCollection, changes: changes) + let oldRect = self.rectAtIndexPath(oldReferenceIndexPath) + let fastUpdates = self.updatesConfig.fastUpdates + var myCompletionExecuted = false + let myCompletion = { + if myCompletionExecuted { return } + myCompletionExecuted = true - if updateType == .Normal { + dispatch_async(dispatch_get_main_queue(), { () -> Void in + // Reduces inconsistencies before next update: https://github.com/diegosanchezr/UICollectionViewStressing + completion() + }) + } - UIView.animateWithDuration(self.constants.updatesAnimationDuration, animations: { () -> Void in - self.collectionView.performBatchUpdates({ () -> Void in - // We want to update visible cells to support easy removal of bubble tail or any other updates that may be needed after a data update - // Collection view state is not constistent after performBatchUpdates. It can happen that we ask a cell for an index path and we still get the old one. - // Visible cells can be either updated in completion block (easier but with delay) or before, taking into account if some cell is gonna be moved - updateModelClosure() - self.updateVisibleCells(changes) + if updateType == .Normal { - self.collectionView.deleteItemsAtIndexPaths(Array(changes.deletedIndexPaths)) - self.collectionView.insertItemsAtIndexPaths(Array(changes.insertedIndexPaths)) - for move in changes.movedIndexPaths { - self.collectionView.moveItemAtIndexPath(move.indexPathOld, toIndexPath: move.indexPathNew) - } - }) { (finished) -> Void in - myCompletion() + UIView.animateWithDuration(self.constants.updatesAnimationDuration, animations: { () -> Void in + self.collectionView.performBatchUpdates({ () -> Void in + updateModelClosure() + self.updateVisibleCells(changes) // For instace, to support removal of tails + + self.collectionView.deleteItemsAtIndexPaths(Array(changes.deletedIndexPaths)) + self.collectionView.insertItemsAtIndexPaths(Array(changes.insertedIndexPaths)) + for move in changes.movedIndexPaths { + self.collectionView.moveItemAtIndexPath(move.indexPathOld, toIndexPath: move.indexPathNew) } - }) - } else { - updateModelClosure() - self.collectionView.reloadData() - self.collectionView.collectionViewLayout.prepareLayout() - myCompletion() - } + }) { (finished) -> Void in + myCompletion() + } + }) + } else { + self.visibleCells = [:] + updateModelClosure() + self.collectionView.reloadData() + self.collectionView.collectionViewLayout.prepareLayout() + } - if shouldScrollToBottom { - self.scrollToBottom(animated: updateType == .Normal) - } else { - let newRect = self.rectAtIndexPath(newReferenceIndexPath) - self.scrollToPreservePosition(oldRefRect: oldRect, newRefRect: newRect) - } + if shouldScrollToBottom { + self.scrollToBottom(animated: updateType == .Normal) + } else { + let newRect = self.rectAtIndexPath(newReferenceIndexPath) + self.scrollToPreservePosition(oldRefRect: oldRect, newRefRect: newRect) + } + + if updateType != .Normal || fastUpdates { + myCompletion() + } } private func updateModels(newItems newItems: [ChatItemProtocol], oldItems: ChatItemCompanionCollection, updateType: UpdateType, completion: () -> Void) { diff --git a/Chatto/Source/ChatController/BaseChatViewController+Presenters.swift b/Chatto/Source/ChatController/BaseChatViewController+Presenters.swift index c929d1c..4c0e796 100644 --- a/Chatto/Source/ChatController/BaseChatViewController+Presenters.swift +++ b/Chatto/Source/ChatController/BaseChatViewController+Presenters.swift @@ -45,6 +45,19 @@ extension BaseChatViewController: ChatCollectionViewLayoutDelegate { self.presentersByCell.removeObjectForKey(cell) oldPresenterForCell.cellWasHidden(cell) } + + if self.updatesConfig.trackVisibleCells { + if let visibleCell = self.visibleCells[indexPath] where visibleCell === cell { + self.visibleCells[indexPath] = nil + } else { + self.visibleCells.forEach({ (indexPath, storedCell) in + if cell === storedCell { + // Inconsistency found, likely due to very fast updates + self.visibleCells[indexPath] = nil + } + }) + } + } } public func collectionView(collectionView: UICollectionView, willDisplayCell cell: UICollectionViewCell, forItemAtIndexPath indexPath: NSIndexPath) { @@ -52,6 +65,9 @@ extension BaseChatViewController: ChatCollectionViewLayoutDelegate { let presenter = self.presenterForIndexPath(indexPath) self.presentersByCell.setObject(presenter, forKey: cell) + if self.updatesConfig.trackVisibleCells { + self.visibleCells[indexPath] = cell + } if self.isAdjustingInputContainer { UIView.performWithoutAnimation({ diff --git a/Chatto/Source/ChatController/BaseChatViewController.swift b/Chatto/Source/ChatController/BaseChatViewController.swift index fb0e4f4..20f8201 100644 --- a/Chatto/Source/ChatController/BaseChatViewController.swift +++ b/Chatto/Source/ChatController/BaseChatViewController.swift @@ -39,6 +39,14 @@ public class BaseChatViewController: UIViewController, UICollectionViewDataSourc public var constants = Constants() + public struct UpdatesConfig { + public var trackVisibleCells = false // When updating very fast, UICollectionView.cellForItemAtIndexPath is not reliable, may return cell from the previous state (*) + public var fastUpdates = false // Allows another performBatchUpdates to be called before completion of a previous one (not recommended). When enabling this it's also recommended to enable `trackVisibleCells` as (*) may happen. + public var coalesceUpdates = false // If receiving data source updates too fast, while an update it's being processed, only the last one will be executed + } + + public var updatesConfig = UpdatesConfig() + public private(set) var collectionView: UICollectionView! public final internal(set) var chatItemCompanionCollection: ChatItemCompanionCollection = ReadOnlyOrderedDictionary(items: []) private var _chatDataSource: ChatDataSourceProtocol? @@ -236,6 +244,7 @@ public class BaseChatViewController: UIViewController, UICollectionViewDataSourc var presenterFactory: ChatItemPresenterFactoryProtocol! let presentersByCell = NSMapTable(keyOptions: .WeakMemory, valueOptions: .WeakMemory) var updateQueue: SerialTaskQueueProtocol = SerialTaskQueue() + var visibleCells: [NSIndexPath: UICollectionViewCell] = [:] // @see UpdatesConfig.trackVisibleCells /** - You can use a decorator to: diff --git a/Chatto/Source/ChatController/Collaborators/CollectionChanges.swift b/Chatto/Source/ChatController/Collaborators/CollectionChanges.swift index 5152859..abed033 100644 --- a/Chatto/Source/ChatController/Collaborators/CollectionChanges.swift +++ b/Chatto/Source/ChatController/Collaborators/CollectionChanges.swift @@ -96,3 +96,26 @@ func generateChanges(oldCollection oldCollection: [UniqueIdentificable], newColl return CollectionChanges(insertedIndexPaths: insertedIndexPaths, deletedIndexPaths: deletedIndexPaths, movedIndexPaths: movedIndexPaths) } + +func updated(collection collection: [NSIndexPath: T], withChanges changes: CollectionChanges) -> [NSIndexPath: T] { + var result = collection + + changes.deletedIndexPaths.forEach { (indexPath) in + result[indexPath] = nil + } + + var movedDestinations = Set() + changes.movedIndexPaths.forEach { (move) in + result[move.indexPathNew] = collection[move.indexPathOld] + movedDestinations.insert(move.indexPathNew) + if !movedDestinations.contains(move.indexPathOld) { + result[move.indexPathOld] = nil + } + } + + changes.insertedIndexPaths.forEach { (indexPath) in + result[indexPath] = nil + } + + return result +} diff --git a/Chatto/Source/SerialTaskQueue.swift b/Chatto/Source/SerialTaskQueue.swift index b208bc2..6d2fc91 100644 --- a/Chatto/Source/SerialTaskQueue.swift +++ b/Chatto/Source/SerialTaskQueue.swift @@ -30,6 +30,7 @@ protocol SerialTaskQueueProtocol { func addTask(task: TaskClosure) func start() func stop() + func flushQueue() var isEmpty: Bool { get } } @@ -52,6 +53,10 @@ final class SerialTaskQueue: SerialTaskQueueProtocol { self.isStopped = true } + func flushQueue() { + self.tasksQueue.removeAll() + } + var isEmpty: Bool { return self.tasksQueue.isEmpty } diff --git a/Chatto/Tests/ChatController/BaseChatViewControllerTestHelpers.swift b/Chatto/Tests/ChatController/BaseChatViewControllerTestHelpers.swift index 0b1cea1..5a6f168 100644 --- a/Chatto/Tests/ChatController/BaseChatViewControllerTestHelpers.swift +++ b/Chatto/Tests/ChatController/BaseChatViewControllerTestHelpers.swift @@ -129,9 +129,9 @@ class FakeChatItem: ChatItemProtocol { final class SerialTaskQueueTestHelper: SerialTaskQueueProtocol { var onAllTasksFinished: (() -> Void)? - private var isBusy = false - private var isStopped = true - private var tasksQueue = [TaskClosure]() + var isBusy = false + var isStopped = true + var tasksQueue = [TaskClosure]() func addTask(task: TaskClosure) { self.tasksQueue.append(task) @@ -151,6 +151,10 @@ final class SerialTaskQueueTestHelper: SerialTaskQueueProtocol { return self.tasksQueue.isEmpty } + func flushQueue() { + self.tasksQueue.removeAll() + } + private func maybeExecuteNextTask() { if !self.isStopped && !self.isBusy { if !self.isEmpty { diff --git a/Chatto/Tests/ChatController/BaseChatViewControllerTests.swift b/Chatto/Tests/ChatController/BaseChatViewControllerTests.swift index 216d3a2..2e19bf2 100644 --- a/Chatto/Tests/ChatController/BaseChatViewControllerTests.swift +++ b/Chatto/Tests/ChatController/BaseChatViewControllerTests.swift @@ -207,6 +207,39 @@ class ChatViewControllerTests: XCTestCase { XCTAssertEqual(900, controller.view.convertRect(controller.chatInputView.bounds, fromView: controller.chatInputView).maxY) } + func testThat_GivenCoalescingIsEnabled_WhenMultipleUpdatesAreRequested_ThenUpdatesAreCoalesced() { + let controller = TesteableChatViewController() + controller.updatesConfig.coalesceUpdates = true + self.fakeDidAppearAndLayout(controller: controller) + let fakeDataSource = FakeDataSource() + let updateQueue = SerialTaskQueueTestHelper() + controller.updateQueue = updateQueue + + controller.setChatDataSource(fakeDataSource, triggeringUpdateType: .None) + controller.chatDataSourceDidUpdate(fakeDataSource) // running + controller.chatDataSourceDidUpdate(fakeDataSource) // discarded + controller.chatDataSourceDidUpdate(fakeDataSource) // discarded + controller.chatDataSourceDidUpdate(fakeDataSource) // queued + + XCTAssertEqual(1, updateQueue.tasksQueue.count) + } + + func testThat_GivenCoalescingIsDisabled_WhenMultipleUpdatesAreRequested_ThenUpdatesAreQueued() { + let controller = TesteableChatViewController() + self.fakeDidAppearAndLayout(controller: controller) + let fakeDataSource = FakeDataSource() + let updateQueue = SerialTaskQueueTestHelper() + controller.updateQueue = updateQueue + + updateQueue.start() + controller.setChatDataSource(fakeDataSource, triggeringUpdateType: .None) + controller.chatDataSourceDidUpdate(fakeDataSource) // running + controller.chatDataSourceDidUpdate(fakeDataSource) // queued + controller.chatDataSourceDidUpdate(fakeDataSource) // queued + controller.chatDataSourceDidUpdate(fakeDataSource) // queued + + XCTAssertEqual(3, updateQueue.tasksQueue.count) + } // MARK: helpers diff --git a/Chatto/Tests/ChatController/CollectionChangesTests.swift b/Chatto/Tests/ChatController/CollectionChangesTests.swift index fe174d5..623eacc 100644 --- a/Chatto/Tests/ChatController/CollectionChangesTests.swift +++ b/Chatto/Tests/ChatController/CollectionChangesTests.swift @@ -83,6 +83,44 @@ class CollectionChangesTests: XCTestCase { XCTAssertEqual(changes.insertedIndexPaths, [NSIndexPath(forItem: 0, inSection: 0)]) XCTAssertEqual(Set(changes.movedIndexPaths), [Move(0, to: 2), Move(2, to: 1)]) } + + func testThatAppliesChangesToCollection() { + // (0, 1, 2, 3, 4) -> (2, 3, new, 4) + + let indexPath0 = NSIndexPath(forItem: 0, inSection: 0) + let indexPath1 = NSIndexPath(forItem: 1, inSection: 0) + let indexPath2 = NSIndexPath(forItem: 2, inSection: 0) + let indexPath3 = NSIndexPath(forItem: 3, inSection: 0) + let indexPath4 = NSIndexPath(forItem: 4, inSection: 0) + + let collection = [ + indexPath0: 0, + indexPath1: 1, + indexPath2: 2, + indexPath3: 3, + indexPath4: 4, + ] + + let deletions = Set([indexPath0, indexPath1]) + let insertions = Set([indexPath2]) + let movements = [ + CollectionChangeMove(indexPathOld: indexPath2, indexPathNew: indexPath0), + CollectionChangeMove(indexPathOld: indexPath3, indexPathNew: indexPath1), + CollectionChangeMove(indexPathOld: indexPath4, indexPathNew: indexPath3) + ] + + let changes = CollectionChanges(insertedIndexPaths: insertions, deletedIndexPaths: deletions, movedIndexPaths:movements) + let result = updated(collection: collection, withChanges: changes) + + let expected = [ + indexPath0: 2, + indexPath1: 3, + // indexPath2: new + indexPath3: 4, + ] + + XCTAssertEqual(result, expected) + } } func Item(uid: String) -> UniqueIdentificable {