Support item expanding and collapsing #146
Conversation
bmarkowitz
force-pushed
the
feature/expandable-items
branch
from
1787986 to
238a869
Compare
| self.apply(destinationSectionSnapshot, to: $0.id, animatingDifferences: animated) { [weak self] in | ||
| self?._applySnapshotCompletion(source: source, | ||
| destination: destination, | ||
| completion) |
There was a problem hiding this comment.
Execute completion multiple times?
There was a problem hiding this comment.
ah, yup 🤦 Great catch.
Any ideas? Thinking a dispatch group might work here?
|
One other note - this warning appears whenever collapsing or expanding an item, and then any time the collection view updates after that. Only way I've been able to "fix" it is by disabling diffing on a background queue, but not sure why. 
|
|
Related to the above, found this super interesting response from Apple:
Also goes on to say that "applying snapshots from a background queue is almost always unnecessary in general." |
|
@jessesquires hey! Wanted to see if you had a chance to check this out? |
|
Hey @bmarkowitz! Sorry for the delay here. I really appreciate your work here and I do want to get this reviewed and merged. I've been very busy, then the holidays, now some travel. I'll try to review soon! |
No need to apologize at all, totally understood! Hope you're doing well! |
There was a problem hiding this comment.
@bmarkowitz I'm building an app that has disclosable views and want to use this library. Thanks for the starting point on supporting items expanding and collapsing; attempted to tackle some of the TODOs detailed in your PR's summary. Haven't tested thoroughly or with dynamic data, but the straightforward example app seems to work, including preserving expansion state!
Disclaimer: not an experienced iOS dev
| @@ -84,14 +87,42 @@ public struct CollectionViewModel: DiffableViewModel { | |||
| /// - Returns: The cell at `indexPath`. | |||
| /// | |||
| /// - Precondition: The specified `indexPath` must be valid. | |||
There was a problem hiding this comment.
| /// - Precondition: The specified `indexPath` must be valid. | |
| /// - Precondition: The specified `indexPath` must be valid and refer to a *visible* item. |
In strengthening this invariant, I'm making an assumption that did-prefixed UICollectionViewDelegate methods are guaranteed to be called after a snapshot's visibleItems is updated. Experientially, is this a fine assumption to make?
| guard let section = self._safeSectionViewModel(at: indexPath.section), | ||
| indexPath.item < section.cells.count else { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| guard let section = self._safeSectionViewModel(at: indexPath.section), | |
| indexPath.item < section.cells.count else { | |
| return nil | |
| } | |
| guard let section = self._safeSectionViewModel(at: indexPath.section), | |
| indexPath.item < section.cells.count | |
| else { | |
| return nil | |
| } | |
| // If not using DiffableDataSource or the section is not hierarchical, | |
| // section.cells[indexPath.item] is fine | |
| guard let diffableDataSource = collectionView.dataSource as? DiffableDataSource, | |
| section.cells.contains(where: { !$0.children.isEmpty }) | |
| else { | |
| return self.cellViewModel(at: indexPath, in: collectionView) | |
| } | |
| // Otherwise, section is hierarchical, so validate indexPath is a *visible* item | |
| guard indexPath.item < diffableDataSource.snapshot(for: section.id).visibleItems.count | |
| else { | |
| return nil | |
| } | |
| return self.cellViewModel(at: indexPath, in: collectionView) |
| guard let diffableDataSource = collectionView.dataSource as? DiffableDataSource | ||
| else { | ||
| return cells[indexPath.item] | ||
| } | ||
|
|
||
| let snapshot = diffableDataSource.snapshot(for: section.id) | ||
| let id = snapshot.visibleItems[indexPath.item] | ||
|
|
||
| guard let cellViewModel = cellViewModel(for: id) | ||
| else { | ||
| return cells[indexPath.item] | ||
| } | ||
|
|
||
| return cellViewModel | ||
| } |
There was a problem hiding this comment.
| guard let diffableDataSource = collectionView.dataSource as? DiffableDataSource | |
| else { | |
| return cells[indexPath.item] | |
| } | |
| let snapshot = diffableDataSource.snapshot(for: section.id) | |
| let id = snapshot.visibleItems[indexPath.item] | |
| guard let cellViewModel = cellViewModel(for: id) | |
| else { | |
| return cells[indexPath.item] | |
| } | |
| return cellViewModel | |
| } | |
| // If not using DiffableDataSource or the section is not hierarchical, | |
| // section.cells[indexPath.item] is fine | |
| guard let diffableDataSource = collectionView.dataSource as? DiffableDataSource, | |
| cells.contains(where: { !$0.children.isEmpty }) | |
| else { | |
| return section.cells[indexPath.item] | |
| } | |
| // Otherwise, section is hierarchical, so a valid indexPath isn't | |
| // necessarily 1:1 with visible items. Validate indexPath is a *visible* item. | |
| let snapshot = diffableDataSource.snapshot(for: section.id) | |
| precondition( | |
| indexPath.item < snapshot.visibleItems.count, | |
| "Invalid visible index path: item \(indexPath.item) is out of bounds for visible items" | |
| ) | |
| let itemId = snapshot.visibleItems[indexPath.item] | |
| // Find the cell recursively (it might be a nested child) | |
| guard let cellViewModel = cellViewModel(for: itemId) else { | |
| preconditionFailure("Could not find cell view model for identifier: \(itemId)") | |
| } | |
| return cellViewModel | |
| } |
There was a problem hiding this comment.
We need to replace allCellsByIdentifier:
func allCellsByIdentifier() -> [UniqueIdentifier: AnyCellViewModel] {
var allCellsDict = [UniqueIdentifier: AnyCellViewModel]()
var cellsToProcess: [AnyCellViewModel] = []
for section in self.sections {
cellsToProcess.append(contentsOf: section.cells)
}
while let cell = cellsToProcess.popLast() {
allCellsDict[cell.id] = cell
cellsToProcess.append(contentsOf: cell.children)
}
return allCellsDict
}
Note: maybe a place we can optimize performance? You have to call allCellsByIdentifier for both source and dest in _findItemsToReconfigure which size-wise can be >>> the # of cells you actually care about--visible items--which is probably around ~10 on mobile depending on ur design
| // If so, nesting is desired and we must use section snapshots, since UIKit only supports | ||
| // this behavior through the use of NSDiffableDataSourceSectionSnapshots. | ||
| // Otherwise, use a standard snapshot. | ||
| let childrenExist = destination.sections.first?.contains(where: \.children.isNotEmpty) ?? false |
There was a problem hiding this comment.
| let childrenExist = destination.sections.first?.contains(where: \.children.isNotEmpty) ?? false | |
| let childrenExist = destination.sections.contains { section in | |
| section.cells.contains(where: \.children.isNotEmpty) | |
| } |
| nonisolated private func _applySectionSnapshot( | ||
| from source: CollectionViewModel, | ||
| to destination: CollectionViewModel, | ||
| withVisibleItems visibleItemIdentifiers: Set<UniqueIdentifier>, | ||
| animated: Bool, | ||
| completion: SnapshotCompletion? | ||
| ) { | ||
| // For each section, build the destination section snapshot. | ||
| destination.sections.forEach { | ||
| let destinationSectionSnapshot = DiffableSectionSnapshot(viewModel: $0) | ||
|
|
||
| // Apply the section snapshot. | ||
| // | ||
| // Swift 6 complains about 'call to main actor-isolated instance method' here. | ||
| // However, call this method from a background thread is valid according to the docs. | ||
| self.apply(destinationSectionSnapshot, to: $0.id, animatingDifferences: animated) { [weak self] in | ||
| self?._applySnapshotCompletion(source: source, | ||
| destination: destination, | ||
| completion) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
| nonisolated private func _applySectionSnapshot( | |
| from source: CollectionViewModel, | |
| to destination: CollectionViewModel, | |
| withVisibleItems visibleItemIdentifiers: Set<UniqueIdentifier>, | |
| animated: Bool, | |
| completion: SnapshotCompletion? | |
| ) { | |
| // For each section, build the destination section snapshot. | |
| destination.sections.forEach { | |
| let destinationSectionSnapshot = DiffableSectionSnapshot(viewModel: $0) | |
| // Apply the section snapshot. | |
| // | |
| // Swift 6 complains about 'call to main actor-isolated instance method' here. | |
| // However, call this method from a background thread is valid according to the docs. | |
| self.apply(destinationSectionSnapshot, to: $0.id, animatingDifferences: animated) { [weak self] in | |
| self?._applySnapshotCompletion(source: source, | |
| destination: destination, | |
| completion) | |
| } | |
| } | |
| } | |
| nonisolated private func _applySectionSnapshot( | |
| from source: CollectionViewModel, | |
| to destination: CollectionViewModel, | |
| withVisibleItems visibleItemIdentifiers: Set<UniqueIdentifier>, | |
| animated: Bool, | |
| completion: SnapshotCompletion? | |
| ) { | |
| // STEP 1: Reconfigure items BEFORE applying section snapshots | |
| self._reconfigureItemsBeforeSectionSnapshots( | |
| from: source, | |
| to: destination, | |
| withVisibleItems: visibleItemIdentifiers | |
| ) { [weak self] in | |
| // STEP 2: Apply section snapshots after reconfiguration | |
| self?._applySectionSnapshotsAfterReconfiguration( | |
| destination: destination, | |
| animated: animated, | |
| completion: completion | |
| ) | |
| } | |
| } | |
| nonisolated private func _reconfigureItemsBeforeSectionSnapshots( | |
| from source: CollectionViewModel, | |
| to destination: CollectionViewModel, | |
| withVisibleItems visibleItemIdentifiers: Set<UniqueIdentifier>, | |
| completion: @escaping () -> Void | |
| ) { | |
| // The strat here is to reconfigure items before applying section snapshots. | |
| // We do this by creating a temporary snapshot from the *source* CollectionViewModel | |
| // and reconfiguring the necessary items. | |
| var tempSnapshot = DiffableSnapshot(viewModel: source) | |
| // Below, we utilize CollectionViewModel solely as a storage container to avoid modifying | |
| // method signatures like _findItemsToReconfigure. | |
| let allDestinationCells = destination.allCellsByIdentifier() | |
| let cellsStillVisibleInDestinationSnapshot = visibleItemIdentifiers.compactMap { | |
| allDestinationCells[$0] | |
| } | |
| let tempSection = SectionViewModel( | |
| id: UUID().uuidString, cells: cellsStillVisibleInDestinationSnapshot) | |
| let tempDestination = CollectionViewModel(id: UUID().uuidString, sections: [tempSection]) | |
| let itemsToReconfigure = self._findItemsToReconfigure( | |
| from: source, | |
| to: tempDestination, | |
| withVisibleItems: visibleItemIdentifiers | |
| ) | |
| if itemsToReconfigure.isNotEmpty { | |
| tempSnapshot.reconfigureItems(itemsToReconfigure) | |
| self.apply(tempSnapshot, animatingDifferences: false) { | |
| completion() | |
| } | |
| } else { | |
| completion() | |
| } | |
| } | |
| nonisolated private func _applySectionSnapshotsAfterReconfiguration( | |
| destination: CollectionViewModel, | |
| animated: Bool, | |
| completion: SnapshotCompletion? | |
| ) { | |
| let dispatchGroup = DispatchGroup() | |
| destination.sections.forEach { section in | |
| // Capture current expansion state before applying new snapshot | |
| let currentSnapshot = self.snapshot(for: section.id) | |
| var expandedItemIds = Set<AnyHashable>() | |
| if !currentSnapshot.items.isEmpty { | |
| currentSnapshot.items.forEach { item in | |
| if currentSnapshot.isExpanded(item) { | |
| expandedItemIds.insert(item) | |
| } | |
| } | |
| } | |
| var destinationSectionSnapshot = DiffableSectionSnapshot(viewModel: section) | |
| // Restore expansion state for items that still exist in the new snapshot | |
| if !expandedItemIds.isEmpty { | |
| for itemId in expandedItemIds { | |
| if destinationSectionSnapshot.items.contains(itemId) { | |
| destinationSectionSnapshot.expand([itemId]) | |
| } | |
| } | |
| } | |
| dispatchGroup.enter() | |
| self.apply(destinationSectionSnapshot, to: section.id, animatingDifferences: animated) { | |
| dispatchGroup.leave() | |
| } | |
| } | |
| dispatchGroup.notify(queue: .main) { [weak self] in | |
| self?._applySnapshotCompletion( | |
| source: destination, | |
| destination: destination, | |
| completion | |
| ) | |
| } | |
| } |
note: i dont think you can support diffing on background queue here because .apply will always run on completion on the main actor. Given the response from apple mentioned above, maybe deprecating diffOnBackgroundQueue is the right call? (or maybe restructure some of the nonisolated/other concurrency stuff)
Issue #33
Describe your changes
This is a WIP of an implementation to support nested items (i.e. nested CellViewModels), enabled by
NSDiffableDataSourceSectionSnapshots.What's been done so far
CellViewModelnow includes achildrenproperty, which allows us to provide an array ofAnyCellViewModels. Each of those can havechildren, and so on.DiffableSectionSnapshot, which is a typealias forNSDiffableDataSourceSectionSnapshot. The ability to append items to other items is only supported through section snapshots. The logic to do so is recursive - appending the items for each top level cell, then for each of those going into all of their children, and so on.DiffableDataSource, I added a check to see if we have at least one section with at least one cell that has non-empty children. If that's the case, we split off into creating section snapshots. Otherwise, we proceed with the existing standard snapshot approach.Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-16.at.12.06.50.mp4
What still needs to be done
self.snapshot(), then reconfigure from there.CollectionViewModelto ensure that we're referencing the snapshot'svisibleItemswhen we're grabbing aCellViewModelfor a given index path. The index paths all change when you expand/collapse an item, so we have to account for that.