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

Multi-selection #184

Closed
rnystrom opened this issue Nov 11, 2016 · 5 comments
Closed

Multi-selection #184

rnystrom opened this issue Nov 11, 2016 · 5 comments

Comments

@rnystrom
Copy link
Contributor

Been asked this quiet a lot internally. It's easy to do but different than traditional UICollectionView. The whole "deselect" API is not what we want to be using for this.

@cdoncarroll
Copy link

I obviously don't have the proper context into why we don't want to use UICollectionView's deselection API. Can you elaborate?

@rnystrom rnystrom changed the title Multi-selection example Multi-selection Nov 11, 2016
@rnystrom
Copy link
Contributor Author

@cdoncarroll It's not a bad API, but UICollectionView does a lot of (private) state preservation for you, and all the state it saves revolves around "index paths".

I whipped up a sample project illustrating a few of these ideas: UICollectionViewDeselection.zip

Where you have -[UICollectionViewCell selected], we could have -[IGListSectionController selected] but what if only 1 of 2 cells in the section controller were actually selected? There's also -[UICollectionView indexPathsForSelectedItems]. Again we could do [IGListAdapter selectedObjects] but we have the same "1 of 2 cells in the section selected" problem.

Then there's reloadData which nukes selection state. I'm not sure what we do there. Maybe we internally track cell selection and restore it when cells are used? Possible. However if we do that, now we're out of sync w/ -[UICollectionView indexPathsForSelectedItems], so we replace that w/ our own tracking.

Also you have to override -[UICollectionViewCell setSelected:] to change a cell's appearance to use custom hide/show selectors. You can use -[UICollectionView selectedBackgroundView] but I don't imagine that'll be super useful.

Putting this all together, we end up reimplementing the selection API and introduce some weird UICollectionView behavior (e.g. in the example, select a cell, tap "Reload", now try deselecting the cell. No select or deselect methods are called. Dunno why.).

In the end we do a lot of complicated things and possibly introduce some weird behavior all to save clients tracking state themselves, which shouldn't be hard.

@kronik
Copy link

kronik commented Nov 16, 2016

@rnystrom so what's the right way of managing selected cells (setting a state, getting a list of selected) for IGListCollectionView?

@rnystrom
Copy link
Contributor Author

@kronik I'll make sure to get an example up, but for now probably manually tracking selection in the section controllers. The trickier part is then getting a list of selected stuff. There are two options that I've seen work:

  • At the VC level, iterate all of your section controllers. Check for your selectable SC type, then check if the SC is marked as selected (again using your custom property)
  • Create a delegate on the section controller that forwards select/deselect events up to the VC
    • IMO this is more tedious

I'd probably go w/ the first option.

However @cdoncarroll and I were chatting a lot last week about this and we might try to find a way to bake support into IGListKit, but to fully support selection it'll be a bigger change, so nothing concrete planned.

@kronik
Copy link

kronik commented Nov 17, 2016

@rnystrom Got it. Thanks.

@rnystrom rnystrom closed this as completed Feb 6, 2017
@rnystrom rnystrom mentioned this issue Jul 12, 2017
3 tasks
facebook-github-bot pushed a commit that referenced this issue Jul 17, 2017
Summary:
Adding support for a cell deselection API. Trying to make some headway to move and drag+drop support, but also want better stock `UICollectionView` API support. Will also assist eventual `UITableView` support.

- Added overridable API to `IGListSectionController`
- Support for stacked SC
- Breaking, required protocol for binding SC

Assists #524 and #184

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes.
Closes #853

Reviewed By: jeremycohen

Differential Revision: D5425414

Pulled By: rnystrom

fbshipit-source-id: 0b25c125b1f171979a15c3095095fc18b4108be6
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