Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataSource] Support section expanding and collapsing #33

Open
jessesquires opened this issue Jul 6, 2021 · 27 comments
Open

[DataSource] Support section expanding and collapsing #33

jessesquires opened this issue Jul 6, 2021 · 27 comments

Comments

@jessesquires jessesquires changed the title Support section Expanding and Collapsing [DataSource] Support section expanding and collapsing May 24, 2024
@nuomi1
Copy link
Collaborator

nuomi1 commented Sep 4, 2024

I used sectionSnapshotHandlers to write a file browser demo. The item of NSDiffableDataSourceSectionSnapshot is multi-level (directory tree structure). How should we modify CellViewModel APIs to adapt this effect?

@nuomi1
Copy link
Collaborator

nuomi1 commented Sep 4, 2024

https://github.com/jessesquires/ReactiveCollectionsKit/tree/feature/sectionSnapshotHandlers-demo

Simulator.Screen.Recording.-.iPhone.15.-.2024-09-04.at.20.35.33.mp4

@jessesquires
Copy link
Owner Author

That video looks great!

How should we modify CellViewModel APIs to adapt this effect?

Hm... I was imagining this would be implemented at the section level, where we add some new configuration properties to SectionViewModel. I don't think we should be modifying CellViewModel for this.

However, I do not have any experience with these APIs. I see that the docs mentions a "parent item" — I'm assuming that's why you're suggesting modifying CellViewModel. It looks like the "parent item" needs to be a cell.

Ideas:

  1. Perhaps, as part of the configuration for SectionViewModel, if you opt-in to collapsing/expanding sections, then we ask users to provide this "parent item", which would be another CellViewModel but it would be separated from the array of cells for the section — similar to how we currently handle headers and footers separately.
  2. Or, could we use the header as the "parent item"?
  3. Or, maybe we do modify CellViewModel to add a property, isExpandableParentItem or something like that?

I'm not sure what's best until I play around with this a bit more.

@nuomi1
Copy link
Collaborator

nuomi1 commented Sep 5, 2024

From the video, sectionIndex is always 0 (Section.main), which looks similar to NSCollectionLayoutGroup in UICollectionViewCompositionalLayout. It is a virtual hierarchy, and the IndexPath is always sectionIndex and itemIndex, without additional real hierarchy. The actual practice of ExpandItem is to adjust the frame of UICollectionViewListCell.contentView to achieve the hierarchical effect.

@nuomi1
Copy link
Collaborator

nuomi1 commented Sep 5, 2024

Simulator.Screen.Recording.-.iPhone.15.-.2024-09-05.at.10.11.24.mp4

@nuomi1
Copy link
Collaborator

nuomi1 commented Sep 5, 2024

I will test the effect of custom Cell and see if it is the same as UICollectionViewListCell.

ExpandItem must be used with UICollectionViewListCell.

@bmarkowitz
Copy link
Contributor

bmarkowitz commented Oct 26, 2024

Hm... I was imagining this would be implemented at the section level, where we add some new configuration properties to SectionViewModel. I don't think we should be modifying CellViewModel for this.

However, I do not have any experience with these APIs. I see that the docs mentions a "parent item" — I'm assuming that's why you're suggesting modifying CellViewModel. It looks like the "parent item" needs to be a cell.

From my understanding of the APIs, that's correct - sections don't have a concept of expanding/collapsing. To achieve the effect, you must use section snapshots, and you have to append items to other items (or to nil for the root item) when populating the snapshot like @nuomi1 said.

IMO - and I preface this with the fact that I've only spent less than a day messing with the library lol - it seems like it'd be nice if the view model structure continued to match the end state, i.e.

let models = ColorModel.makeColors()

let cellViewModels = models.map {
    ColorCellViewModelList(
        color: $0,
        children: $0.children,      <----- these would also need to be CellViewModels, I think
        contextMenuConfiguration: nil
    )
}

but I don't know if this makes the most sense.

@bmarkowitz
Copy link
Contributor

@jessesquires I hacked together this branch of what I was roughly thinking

https://github.com/bmarkowitz/ReactiveCollectionsKit/tree/feature/expandable-sections

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-27.at.14.52.05.mp4

@nuomi1
Copy link
Collaborator

nuomi1 commented Oct 28, 2024

@jessesquires I hacked together this branch of what I was roughly thinking

https://github.com/bmarkowitz/ReactiveCollectionsKit/tree/feature/expandable-sections

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-27.at.14.52.05.mp4

It's great, I think there are some improvements that can be made:

  1. Use [AnyCellViewModel] for children.
  2. Add new methods to CollectionViewModel and SectionViewModel to find cell's children instead of directly modifying cellViewModel(for:).

@bmarkowitz
Copy link
Contributor

bmarkowitz commented Oct 29, 2024

  1. Use [AnyCellViewModel] for children.

Good call. That being said... I wonder if children should be any CellViewModel and _children should be AnyCellViewModel, similar to how SectionViewModel type erases its cells?

@nuomi1
Copy link
Collaborator

nuomi1 commented Oct 29, 2024

Good call. That being said... I wonder if children should be any CellViewModel and _children should be AnyCellViewModel, similar to how SectionViewModel type erases its cells?

protocol CellViewModel {
    var children: [AnyCellViewModel] { get }
}

struct AnyCellViewModel: CellViewModel {
    var children: [AnyCellViewModel] { self._children }
    private let _children: [AnyCellViewModel]
}

@bmarkowitz
Copy link
Contributor

bmarkowitz commented Oct 29, 2024

Yeah, that definitely makes sense.

But, I think that would force a consumer to always have to type erase the children. Whereas when you're providing cells to a SectionViewModel, you only have to type erase when providing mixed types.

So, something like this:


protocol CellViewModel {
    var children: [any CellViewModel] { get }
}

struct AnyCellViewModel: CellViewModel {
    var children: [any CellViewModel] { self._children }
    private let _children: [AnyCellViewModel]

    public init<T: CellViewModel>(_ viewModel: T) {
        self._children = children.map { $0.eraseToAnyViewModel() }
    }
}

@nuomi1
Copy link
Collaborator

nuomi1 commented Oct 29, 2024

I'm not sure if there is any performance difference between [any CellViewModel] and [AnyCellViewModel], it would be better to test it (likes TestDiffableSnapshot.test_init_perf()).

@bmarkowitz
Copy link
Contributor

I added some additional rough commits to this branch, continuing to play around with this. Some notes:

  • adf7d36 and fca07ca - we need a way to recursively traverse all of the children when looking for a matching cell view model, and to make sure we append all children to the snapshot
  • 0001c52 - this is just a hack for now, but I think we should only be considering the data source's visibleItems when grabbing a cell view model. In this video, we can see that the index path's change when you expand/collapse an item, so we have to account for that.

Would love feedback here as these are just some rough commits to get things at least working.

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-29.at.22.12.47.mp4

@nuomi1
Copy link
Collaborator

nuomi1 commented Oct 30, 2024

Very nice. The tree structure requires the use of recursion.

@jessesquires
Copy link
Owner Author

jessesquires commented Nov 11, 2024

Hey @bmarkowitz and @nuomi1 -- sorry for the delay here and thanks for all of your thought and effort on this!

I glanced through the code and I have a new idea. I haven't experimented with this yet, so I'm not sure if it would work.

I'm still apprehensive to add children to CellViewModel because of how that complicates the API and structure of the collection. There's a lot of complexity there. I'm thinking we can still manage this at the section level — which will (in my opinion) provide a better API than UIKit. Here's what I'm thinking:

  1. We add a isExpandableCollapsible property to SectionViewModel. (Still not sure about naming here. We can brainstorm.)

  2. When sections have this property set to true, then when we build the snapshot, we use the first item in section.cells as the parent item and the rest of the cells (i.e., cells.dropFirst()) as the children items in the snapshot. In other words, it is a bit "implicit" that the first item is the parent and the rest are the children — but I think this will make building collapsible lists significantly easier.

I haven't tested this, but here's a rough draft for the snapshot code (building on what @bmarkowitz has already done):

typealias DiffableSnapshot = NSDiffableDataSourceSnapshot<AnyHashable, AnyHashable>
typealias DiffableSectionSnapshot = NSDiffableDataSourceSectionSnapshot<AnyHashable>

extension DiffableSnapshot {
    init(viewModel: CollectionViewModel) {
        self.init()

        let allSectionIdentifiers = viewModel.sections.map(\.id)
        self.appendSections(allSectionIdentifiers)

        viewModel.sections.forEach {
            let allCellIdentifiers = $0.cells.map(\.id)

            // NEW: check the property and only add the first item
            if $0.isExpandableCollapsible, let first = allCellIdentifiers.first {
                self.appendItems([first], toSection: $0.id)
            } else {
                self.appendItems(allCellIdentifiers, toSection: $0.id)
            }
        }
    }
}

extension DiffableSectionSnapshot {
    init(viewModel: SectionViewModel) {
        self.init()

        let allCellIdentifiers = viewModel.cells.map(\.id)
        
        // NEW: for sections, check the property and build the snapshot accordingly
        if viewModel.isExpandableCollapsible,
           viewModel.count > 1,
           let first = allCellIdentifiers.first {

            self.append([first])

            let children = Array(allCellIdentifiers.dropFirst())
            self.append(children, to: first)
        } else {
            self.append(allCellIdentifiers)
        }
    }
}
  1. Then we'll need to do some work in DiffableDataSource to correctly apply these section snapshots.

I think this should work as expected, right? Or am I missing something?

@bmarkowitz
Copy link
Contributor

@jessesquires No worries - thanks for taking a look!

I think this would definitely work - I'm just a little unclear on the change in the DiffableSnapshot extension, specifically this part:

// NEW: check the property and only add the first item
if $0.isExpandableCollapsible, let first = allSectionIdentifiers.first {
    self.appendItems([first], toSection: $0.id)
}
  1. It looks like we're appending a section identifier to a section - did you mean for that to be allCellIdentifiers.first?
  2. I believe the expand/collapse functionality is only supported by section snapshots, so I think DiffableSnapshot would remain unchanged. I might be misunderstanding the intention here though.

@jessesquires
Copy link
Owner Author

  1. It looks like we're appending a section identifier to a section - did you mean for that to be allCellIdentifiers.first?

Oops. Yes. That was a typo.

Fixed.

  1. I believe the expand/collapse functionality is only supported by section snapshots, so I think DiffableSnapshot would remain unchanged. I might be misunderstanding the intention here though.

Ah, that might be true? Again, I didn't test this yet. I made some assumptions:

In DiffableSnapshot, if a section is collapsible then:

  1. we should only append the first item (the "parent item") to the section
  2. we need to avoid appending all items to the section because the children items will be appended to the parent item

Perhaps those assumptions are incorrect?

@bmarkowitz
Copy link
Contributor

Yeah, that's my understanding based on the overview here and the fact that this API only exists on section snapshots. This is where I started leaning towards building on CellViewModel rather than SectionViewModel.

The underlying diffable data source API is built on the idea that items are expandable/collapsable, and items are nested within items, rather than sections. So just thinking that if someone comes in knowing how that works, this would be a different way of thinking.

I'm also not sure if we want to assume for the consumer that the first item is the one that things will be nested within.

Maybe we're fine with those things, but just throwing it out there - I kinda liked the idea that having CellViewModels nested within CellViewModels would more closely match how things would actually look in the UI (thinking of the file directory example from earlier).

@bmarkowitz
Copy link
Contributor

bmarkowitz commented Nov 12, 2024

To clarify, you could decide to literally have only one section with 50 items in it, and all 50 of those items could be expandable with varying amounts of children, and so on.

@jessesquires
Copy link
Owner Author

@bmarkowitz Ohh I just realized — nested items can have nested items. So if we use my "section hack" then we would be limiting users to a single level of nesting. 🤦🏼‍♂️

Ok, I agree the right approach here is for CellViewModel to have children.

Let's use @nuomi1's suggestion above:

protocol CellViewModel {
    var children: [AnyCellViewModel] { get }
}

struct AnyCellViewModel: CellViewModel {
    var children: [AnyCellViewModel] { self._children }
    private let _children: [AnyCellViewModel]
}

Note: we don't think we can use any CellViewModel here, for the same reasons we do not do that elsewhere.

It would also be nice to have:

extension CellViewModel {
    var isParent: Bool {
        self.children.isNotEmpty
    }
}

@jessesquires
Copy link
Owner Author

As for a PR for this:

Maybe let's go ahead and open one up? Then we can move our discussion about implementation details there.

Also, for the example app for this PR:

  1. Let's add a new "Outline" tab to exercise this new functionality
  2. Per [Example App] stress testing #105, I've been wanting to add a new NumberModel in the example. Maybe we can use this for the outline view? Just generate tons of numbers and nest them. Perhaps each section increases by an order of magnitude. For example, section one would be 0-9, section two would be 10-99, section three would be 100-999, etc.

@bmarkowitz
Copy link
Contributor

bmarkowitz commented Nov 12, 2024

Sounds good! One last question for you -

My only confusion with any CellViewModel vs. AnyCellViewModel was that the former would allow (I think) a user to not have to type erase the children upfront for a homogeneous array of children. It would be handled internally similar to how SectionViewModel type erases its cells within its public init.

Do we want to require them to be type erased up front, or should it be handled when the CellViewModel is turned into an AnyCellViewModel by the section?

@jessesquires
Copy link
Owner Author

@bmarkowitz Great question.

Re-posting your snippet above for reference:

protocol CellViewModel {
    var children: [any CellViewModel] { get }
}

struct AnyCellViewModel: CellViewModel {
    var children: [any CellViewModel] { self._children }
    private let _children: [AnyCellViewModel]

    public init<T: CellViewModel>(_ viewModel: T) {
        self._children = children.map { $0.eraseToAnyViewModel() }
    }
}

My only confusion with any CellViewModel vs. AnyCellViewModel was that the former would allow (I think) a user to not have to type erase the children upfront for a homogeneous array of children. It would be handled internally similar to how SectionViewModel type erases its cells within its public init.

Do we want to require them to be type erased up front, or should it be handled when the CellViewModel is turned into an AnyCellViewModel by the section?

This is correct about optimizing a homogeneous array of children. And I do like this idea.

However, the downside here is that if we use any CellViewModel, then clients cannot get Equatable and Hashable for free because any T cannot be Hashable.

In fact, I don't think [any CellViewModel] would compile because of the requirements inherited from DiffableViewModel.

@bmarkowitz
Copy link
Contributor

Gotcha, gotcha. Great point, I hadn't thought about that. I have a lot to learn about existentials 😄

@bmarkowitz
Copy link
Contributor

I'll do my best to get an initial PR up relatively soon, so we can start discussing some more 👍

@jessesquires
Copy link
Owner Author

@bmarkowitz No rush! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants