Interactive Reordering #976

jverdi · 2017-10-24T05:02:03Z

I had a desire for interactive reordering in a personal project, so here's a first attempt at adding support in IGListKit.

I figured I might as well get a WIP PR up for comments before I continue further as there are a few aspects to interactive reordering that don't interplay perfectly with IGListKit.

As discussed in #291, I went after two prime use cases:

Moving items amongst a section
Rearranging whole sections

I also "disabled" moving items between sections by having those moves revert, to mimic interactive reordering cancellation as closely as possible.

You can see both in the Mixed Data example. Grid items can be moved within a section, while users can be moved to reorder whole sections. But trying to move a grid item out of a grid or a user item into a grid will auto-revert. The revert animation isn't as tight as it should be. It may be more desirable to disable the animation - though you lose the visual cue.

There is a also a new example, ReorderableViewController, that demonstrates 2 in its pure form (likely the most desired use case), where all sections are reorderable single rows.

Happy to take feedback -- this is my first experience working on IGListKit, so I would expect there to be gaps. (Ex. I haven't used IGListStackedSectionController, and its tests failed as I hadn't implemented reordering delegates for it. Those are simply stubbed out for now.)

Changes in this pull request

Issue fixed: #291

Checklist

All tests pass. Demo project builds and runs.
I added tests, an experiment, or detailed why my change isn't tested.
I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
I have reviewed the contributing guide

Additional TODO

Proper support in IGListStackedSectionController

TODO: - IGListStackedSectionController - Tests

iglistkit-bot · 2017-10-24T05:08:28Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

weyert · 2017-10-24T09:40:00Z

Thank you for this pull request @jverdi. Would this increase the minimal iOS version requirement from 8.0 to 9.0? As I think the interactive movement feature was introduced in iOS 9? The library is targeting 8.0 at the moment but maybe this can be changed @rnystrom if necessary? As Instagram itself seems to be iOS 9 or later.

jverdi · 2017-10-24T15:53:18Z

Hi @weyert - the PR shouldn't require changing the deployment target to iOS 9. The 9+ specific code for interactive reordering only gets implemented in the client apps (and IGListKit example projects). But, to take advantage of this optional feature would require iOS 9.

Common solutions to support interactive reordering on iOS <9 typically impose a specific UICollectionViewLayout like LXReorderableCollectionViewFlowLayout or DraggableCollectionView. I think this strikes a better balance for a library - but definitely up to the maintainers.

rnystrom · 2017-10-24T16:47:50Z

@jverdi incredible. I’m going to set aside some time to thoroughly review this. Been a feature I and others have wanted for a while. Really excited for this and want to make sure it gets merged!

_{Sent with GitHawk}

weyert · 2017-10-24T16:49:56Z

Great thank you, @jverdi. Sounds like a great addition to the project! (and sorry for the dumb question ☺️)

jessesquires · 2017-10-24T19:17:48Z

Thanks @jverdi ! 🙌

jessesquires · 2017-10-24T19:20:32Z

Source/IGListAdapterDataSource.h

@@ -60,7 +60,27 @@ NS_SWIFT_NAME(ListAdapterDataSource)
 adding the background view and maintaining its visibility.
 */
 - (nullable UIView *)emptyViewForListAdapter:(IGListAdapter *)listAdapter;
-
+
+@optional


just noting:

we can keep this for 3.2 to avoid breaking clients and remove for 4.0.

after this merges, let's be sure to open a new issue to track this.

@jverdi could we move this to just be a new delegate on IGListAdapter called IGListAdapterMoveDelegate? Then we:

Don't have to use @optional at all

Can let it be nil by default

Don't add any boilerplate to clients that don't use moving (all of them at the beginning). This will saves hundreds of empty implementations within Instagram 😉

rnystrom

Looks awesome, I think this is absolutely on the right track. Would love to land this and include in 3.2 announcement.

Other thing to start thinking about is testing. We will definitely need to cover this w/ some tests. Doesn't have to be incredibly exhaustive, but at least get some groundwork so if a regression happens we can add tests on top of the initial ones.

rnystrom · 2017-10-24T19:46:47Z

Source/IGListUpdatingDelegate.h

+ @param fromIndex The source index of the section to move.
+ @param toIndex The destination index of the section to move.
+ */
+- (void)moveSectionInCollectionView:(UICollectionView *)collectionView


Technically this is a breaking change, but I'm not sure if ppl are making their own updaters. @jessesquires thoughts?

rnystrom · 2017-10-24T19:49:48Z

Source/IGListAdapterDataSource.h

+ @param sectionIndex The index of the section proposed to be moved.
+ @param index The index of the object proposed to be moved.
+ */
+- (BOOL)listAdapter:(IGListAdapter *)listAdapter canMoveObjectInSection:(NSInteger)sectionIndex atIndex:(NSInteger)index;


Is there a way we could make this a property (defaulted to NO) on the adapter? Adding another (eventually) required method like this will be a pretty big pain to upgrade.

Another option, (which I like a lot more), can we add a IGListAdapterMovingDelegate or something onto IGListAdapter? Then we can make all the methods required and not break any upgrades. Plus, if you don't use moving, it makes your data source implementation much cleaner.

Thoughts?

Removing this entirely based on later feedback.

rnystrom · 2017-10-24T19:51:27Z

Source/Internal/IGListAdapter+UICollectionView.m

+    // for ease of implementation, first we check with the listAdapter
+    if ([dataSource respondsToSelector:@selector(listAdapter:canMoveObjectInSection:atIndex:)]) {
+        return [dataSource listAdapter:self canMoveObjectInSection:sectionIndex atIndex:itemIndex];
+    }


I left a note above, but wondering if we can just skip this entirely? Maybe just rely on the section controller deciding if a move is allowed or not?

Sure -- I was thinking about whole section reordering here. My motivation was that for section controllers to be reusable, they might be re-orderable or not depending on the context of the collection/adapter they appear in.

But, if the delegates are all to be required eventually, yes, it's worth dropping this to reduce complexity.

rnystrom · 2017-10-24T19:51:35Z

Source/Internal/IGListAdapter+UICollectionView.m

+    IGAssert(dataSource != nil, @"Found a nil dataSource when requesting canMoveItemAtIndexPath for interactive reordering");
+
+    NSInteger sectionIndex = indexPath.section;
+    NSInteger itemIndex = indexPath.item;


nit: const on both

rnystrom · 2017-10-24T19:51:55Z

Source/Internal/IGListAdapter+UICollectionView.m

+    NSInteger sourceItemIndex = sourceIndexPath.item;
+    NSInteger destinationItemIndex = destinationIndexPath.item;
+    NSInteger sourceSectionIndex = sourceIndexPath.section;
+    NSInteger destinationSectionIndex = destinationIndexPath.section;


nit: const all of it

rnystrom · 2017-10-24T19:55:38Z

Source/Internal/IGListAdapter+UICollectionView.m

+            // the "item" representing our section was dropped
+            // into the beginning of a destination section rather than the end
+            // so it really belongs one section before the section where it landed
+            destinationSectionIndex -= 1;


rnystrom · 2017-10-24T19:56:00Z

Source/Internal/IGListAdapter+UICollectionView.m

+
+    // otherwise this is a move of an _item_ from one section to another section
+    // this is not currently supported, so we need to revert the change as it's too late to cancel
+    [self revertInvalidInteractiveMoveFromIndexPath:sourceIndexPath toIndexPath:destinationIndexPath];


You're comments are 💯

rnystrom · 2017-10-24T19:56:52Z

Source/IGListSectionController.h

+
+ @param index The index of the unhighlighted cell.
+
+ @note Interactive reordering is supported both for items within a single section, as well as for reordering sections themselves when sections contain only one item. The default implementation returns false.


nit: let's newline. I try to wrap comments at 120 chars (we should add a lint)

Also "false" should be "NO" with markdown code ticks

rnystrom · 2017-10-24T19:58:23Z

Source/IGListSectionController.m

+}
+
+- (void)moveObjectFromIndex:(NSInteger)sourceIndex toIndex:(NSInteger)destinationIndex {
+    IGFailAssert(@"Section controller %@ must override %s if interactive reordering is enabled.", self, __PRETTY_FUNCTION__);


rnystrom · 2017-10-24T20:01:04Z

Source/IGListAdapterDataSource.h

+ @param sourceIndex The starting index of the object.
+ @param destinationIndex The ending index of the object.
+ */
+- (void)listAdapter:(IGListAdapter *)listAdapter moveSectionAtIndex:(NSInteger)sourceIndex toIndex:(NSInteger)destinationIndex;


B/c data sources could be out of sync w/ what is being displayed (ultimately held by IGListAdapter internally), I think we should do one of the following:

Add params for to/from objects

Add to/from object arrays

facebook-github-bot · 2017-10-24T22:55:07Z