Add item-level moves to IGListCollectionContext #418

rnystrom · 2017-01-13T23:19:10Z

Changes in this pull request

Adding an API to do item-level (cell) moves on the collection view. This complicates things a little bit because of all the issues that moving sections have while in batch updates (e.g. simultaneous animation UICV bugs). Thankfully we use pretty strict types so the compiler does most of the work for us.

Closes #145

TODO

jessesquires · 2017-01-15T21:25:28Z

Is #146 the right issue??

rnystrom · 2017-01-16T03:22:09Z

Off by one 😅

facebook-github-bot · 2017-01-18T23:17:02Z

@rnystrom updated the pull request - view changes

facebook-github-bot · 2017-01-19T22:40:53Z

@rnystrom updated the pull request - view changes

facebook-github-bot · 2017-01-20T15:00:31Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-01-20T15:01:00Z

Ready for review!

facebook-github-bot · 2017-01-20T15:03:23Z

@rnystrom updated the pull request - view changes

Sherlouk · 2017-01-20T15:10:47Z

Can't offer much in terms of review for ObjC, sorry! 😅

Is this purely additive, or am I missing the breaking change here?

rnystrom · 2017-01-20T17:43:39Z

Nope shouldn't be anything breaking, all new APIs

zhubofei · 2017-01-20T17:55:16Z

Tests/IGListAdapterTests.m

@@ -134,7 +134,7 @@ - (void)test_whenQueryingIndexPaths_withSectionController_thatPathsAreEqual {
    IGListSectionController <IGListSectionType> * second = [self.adapter sectionControllerForObject:@1];
    NSArray *paths0 = [self.adapter indexPathsFromSectionController:second
                                                            indexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(2, 4)]
-                                               adjustForUpdateBlock:NO];
+                                               usePreviousSection:NO];


nit: off by one space?

zhubofei · 2017-01-20T17:55:37Z

Tests/IGListAdapterTests.m

@@ -153,7 +153,7 @@ - (void)test_whenQueryingIndexPaths_insideBatchUpdateBlock_thatPathsAreEqual {
    [self.adapter performBatchAnimated:YES updates:^{
        NSArray *paths = [self.adapter indexPathsFromSectionController:second
                                                               indexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(2, 2)]
-                                                  adjustForUpdateBlock:YES];
+                                                  usePreviousSection:YES];


facebook-github-bot · 2017-01-20T18:38:00Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-01-28T08:44:58Z

Looks like this needs a test covering move w/out batch updates:

https://coveralls.io/builds/9778932/source?filename=Source%2FIGListAdapterUpdater.m

facebook-github-bot · 2017-01-28T17:47:02Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-01-29T08:25:42Z

@jessesquires mind giving this a quick run through before I land?

rnystrom · 2017-02-02T16:57:06Z

ping @jessesquires

Adlai-Holler

Left a couple thoughts about naming.

Adlai-Holler · 2017-02-02T19:55:18Z

Source/Common/IGListBatchUpdateData.h

@@ -48,6 +49,11 @@ IGLK_SUBCLASSING_RESTRICTED
 @property (nonatomic, strong, readonly) NSSet<NSIndexPath *> *deleteIndexPaths;

 /**
+ Item delete index paths.


👍 we save doc updating for version cuts, keeps history cleaner

Ah I just mean, this should read Item move index paths.

Adlai-Holler · 2017-02-02T20:00:21Z

Source/IGListAdapter.m

@@ -552,22 +552,22 @@ - (BOOL)itemCountIsZero {
    return isZero;
 }

- (IGListSectionMap *)sectionMapAdjustForUpdateBlock:(BOOL)adjustForUpdateBlock {
+- (IGListSectionMap *)sectionMapAdjustForUpdateBlock:(BOOL)usePreviousSection {


Would - (IGListSectionMap *)sectionMapUsingPreviousIfInUpdateBlock:(BOOL)usePreviousMapIfInUpdateBlock or something be clearer?

🤔 I like that!

Adlai-Holler · 2017-02-02T20:01:09Z

Source/IGListAdapter.m

-    const NSInteger section = [self.sectionMap sectionForSectionController:controller];
+- (NSIndexPath *)indexPathForSectionController:(IGListSectionController *)controller
+                                         index:(NSInteger)index
+                            usePreviousSection:(BOOL)usePreviousSection {


Would usePreviousMap or usePreviousSectionMap be clearer?

I think I'll update this to match the previous comment so they are in sync. Good call outs!

jessesquires · 2017-02-03T01:01:57Z

Source/IGListAdapter.m

+    IGParameterAssert(fromIndex >= 0);
+    IGParameterAssert(toIndex >= 0);
+    UICollectionView *collectionView = self.collectionView;
+    IGAssert(collectionView != nil, @"Moving items from %@ without a collection view from index to %zi index %zi.",


assert message:

from index to %zi index %zi.

should be:

index %zi to index %zi.

jessesquires · 2017-02-03T01:02:40Z

Source/IGListAdapterUpdater.m

+        _deleteIndexPaths = [NSMutableSet new];
+        _insertIndexPaths = [NSMutableSet new];
+        _moveIndexPaths = [NSMutableSet new];
+        _reloadIndexPaths = [NSMutableSet new];


jessesquires · 2017-02-03T01:03:24Z

Source/IGListAdapterUpdater.m

+        [self.moveIndexPaths addObject:move];
+    } else {
+        [self.delegate listAdapterUpdater:self willMoveFromIndexPath:fromIndexPath toIndexPath:toIndexPath collectionView:collectionView];
+        [collectionView moveItemAtIndexPath:fromIndexPath toIndexPath:toIndexPath];


should we have a didMove: delegate method?

I think we're ok since we don't have any did* for insert/delete/reload

jessesquires · 2017-02-03T01:07:50Z

Source/IGListAdapterUpdaterDelegate.h

- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater didPerformBatchUpdates:(IGListBatchUpdateData *)updates withCollectionView:(UICollectionView *)collectionView;
+- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater
+    didPerformBatchUpdates:(IGListBatchUpdateData *)updates
+        withCollectionView:(UICollectionView *)collectionView;


nit:

this is the only delegate method spelled withCollectionView:. 😞 the others are simply spelled collectionView:.

I'd prefer to remove the "with" here. (could be another diff. will require changelog entry)

👍 filed #466 to follow up

jessesquires · 2017-02-03T01:10:04Z

Source/IGListUpdatingDelegate.h

+
+ @param collectionView The collection view on which to perform the transition.
+ @param fromIndexPath  The original index path of the item to move.
+ @param toIndexPath    The index path to move the item to.


how about:

@param fromIndexPath The source index path of the item to move.
@param toIndexPath The destination index path of the item to move.

jessesquires · 2017-02-03T01:10:41Z

Source/Internal/IGListAdapterInternal.h

+                        usePreviousSection:(BOOL)usePreviousSection;
+- (nullable NSIndexPath *)indexPathForSectionController:(IGListSectionController *)controller
+                                         index:(NSInteger)index
+                            usePreviousSection:(BOOL)usePreviousSection;


them : 😄

jessesquires · 2017-02-03T01:11:17Z

Tests/IGListAdapterE2ETests.m

@@ -1189,4 +1189,152 @@ - (void)test_whenQueuingUpdate_withSectionControllerBatchUpdate_thatSectionContr
    XCTAssertNil(weakSectionController);
 }

+- (void)test_whenMovingItems_withObjectMoving_thatCollectionViewWorks {


"thatCollectionViewWorks" lol

jessesquires · 2017-02-03T01:13:01Z

looks pretty good to me! 😎

facebook-github-bot · 2017-02-06T16:50:31Z

@rnystrom updated the pull request - view changes

facebook-github-bot · 2017-02-07T15:51:32Z

@rnystrom updated the pull request - view changes

facebook-github-bot · 2017-02-07T15:52:15Z

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Started work on the plane to get this moving since #418 is up and ready to land. We'll likely need to spend some time fleshing out the API of this, and I think I'll split it up into a couple different PRs once ready for review. Putting this up now to get early feedback. This adds an auto-diffing section controller as outlined in #38. There are several key parts: - Subclass a new section controller `IGListAutoSectionController` (naming wip) - Connect a data source - Implement the data source methods that do 3 things: - Given a top-level object, transform it into an array of **diffable** view models - Given a view model, return a cell - Given a view model, return a size for a cell - A new protocol for the cell `IGListBindable` so that we can control when the cell is updated w/ the view model. - The most important part of this is that it unlocks moving and reloading a cell, which you can't do w/ `UICollectionView` - [ ] Unit test `reloadObjects:` - [x] Add Closes #494 Reviewed By: amonshiz Differential Revision: D4696966 Pulled By: rnystrom fbshipit-source-id: f21b8341b3ed4389f2a4a106d0d316f481ba6943

rnystrom self-assigned this Jan 13, 2017

facebook-github-bot added the CLA Signed label Jan 13, 2017

jessesquires added this to the 3.0.0 milestone Jan 15, 2017

This was referenced Jan 18, 2017

Remove reloadIndexPaths: from IGListBatchUpdateData #429

Closed

Drop section moves when the section was deleted in IGListBatchUpdateData #430

Closed

rnystrom changed the title ~~WIP: Add item-level moves to IGListCollectionContext~~ Add item-level moves to IGListCollectionContext Jan 20, 2017

rnystrom requested review from jessesquires and Sherlouk January 20, 2017 15:00

rnystrom force-pushed the move-items branch from a1e3021 to 0e83b2c Compare January 20, 2017 15:02

rnystrom requested review from zhubofei and removed request for Sherlouk January 20, 2017 17:43

zhubofei suggested changes Jan 20, 2017

View reviewed changes

zhubofei approved these changes Jan 20, 2017

View reviewed changes

rnystrom mentioned this pull request Jan 27, 2017

WIP: Auto diffing section controller #449

Closed

8 tasks

rnystrom force-pushed the move-items branch from a7b537b to 761dedc Compare January 28, 2017 17:46

Adlai-Holler reviewed Feb 2, 2017

View reviewed changes

jessesquires reviewed Feb 3, 2017

View reviewed changes

Ryan Nystrom added 10 commits February 6, 2017 11:03

Add item-level moves to IGListCollectionContext

fb2354b

tests pass

293bb9b

add batch update data unit tests

0d345a7

Add adapter tests

7e6dd86

test that actually moves cells, fix tests

3694e9f

stacked section controller tests

6382d27

add changelog entry

4c57dd2

colon formatting

374cf1c

add test to move without batch updates

03d5f78

rebase, naming nits, formatting, docs

faf77a6

rnystrom force-pushed the move-items branch from 761dedc to faf77a6 Compare February 6, 2017 16:50

Merge branch 'master' into move-items

1acdc41

facebook-github-bot closed this in 4dec244 Feb 11, 2017

rnystrom mentioned this pull request Feb 15, 2017

Auto diffing section controller #494

Closed

10 tasks

jessesquires deleted the move-items branch February 23, 2017 23:49

Add item-level moves to IGListCollectionContext #418

Add item-level moves to IGListCollectionContext #418

Conversation

rnystrom commented Jan 13, 2017 • edited Loading

Changes in this pull request

TODO

jessesquires commented Jan 15, 2017

rnystrom commented Jan 16, 2017

facebook-github-bot commented Jan 18, 2017

facebook-github-bot commented Jan 19, 2017

facebook-github-bot commented Jan 20, 2017

rnystrom commented Jan 20, 2017

facebook-github-bot commented Jan 20, 2017

Sherlouk commented Jan 20, 2017

rnystrom commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 20, 2017

rnystrom commented Jan 28, 2017

facebook-github-bot commented Jan 28, 2017

rnystrom commented Jan 29, 2017

rnystrom commented Feb 2, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesquires Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesquires commented Feb 3, 2017

facebook-github-bot commented Feb 6, 2017

facebook-github-bot commented Feb 7, 2017

facebook-github-bot commented Feb 7, 2017

rnystrom commented Jan 13, 2017 •

edited

Loading

jessesquires Feb 3, 2017 •

edited

Loading