Add move detection and support to ASLayoutTransition #1006

wiseoldduck · 2018-07-03T21:16:18Z

...and NSArray+Diffing.
Add some tests.

See #816 for more details on the existing shortcomings. This PR should allow z-order to be correct after nodes move during a transition.

...and NSArray+Diffing. Add some tests.

CLAassistant · 2018-07-03T21:16:25Z

All committers have signed the CLA.

appleguy · 2018-07-06T23:27:23Z

Source/ASDisplayNode+Layout.mm

@@ -680,7 +680,7 @@ - (void)transitionLayoutWithSizeRange:(ASSizeRange)constrainedSize
      }

      // Apply the subnode insertion immediately to be able to animate the nodes
-      [pendingLayoutTransition applySubnodeInsertions];
+      [pendingLayoutTransition applySubnodeInsertionsAndMoves];


Outside the scope of this change - but I've heard that CALayer .zPosition is animatable. If we wanted to, it might be possible to animate the move operations (not sure exactly what it looks like though).

I suppose doing the reordering at the beginning is better than at the end (usually this is less noticeable than a pop at the end), so I think this implementation is ideal for current needs.

That's great, maybe it is a cross-fade animation or something. But yes being able to animate that is a good win from this.
The second point is very related, as setting up animations that will run simultaneously also allows us to do the setup in whatever order makes life easiest for ourselves, not necessarily tied to the order in which anything plays out on the screen. But at present (where we are not animating z-order) I agree it looks nicer to have the z-order swap happen before the animations rather than at the end.

appleguy · 2018-07-06T23:29:00Z

Source/Details/NSArray+Diffing.h

@@ -35,4 +35,12 @@
 */
 - (void)asdk_diffWithArray:(NSArray *)array insertions:(NSIndexSet **)insertions deletions:(NSIndexSet **)deletions compareBlock:(BOOL (^)(id lhs, id rhs))comparison;

+/**
+ * @abstract Compares two arrays, providing the insertion, deletion, and move indexes needed to transform into the target array.
+ * @discussion This compares the equality of each object with `isEqual:`.


I wonder if we should add to the method above that it will not transform into the same order, only to the same contents?

ahh but it will, you only need the moves if you want to animate them.

I will add some comments on this, as it was not clear to me until close to the end that this was actually true (that the -/+ information is sufficient and accurate to do the reordering without moves). It requires applying the diff in a certain way (basically in order into a sparse array) that was neither documented nor being done.

appleguy · 2018-07-06T23:31:57Z

Source/Details/NSArray+Diffing.h

+ * @discussion This compares the equality of each object with `isEqual:`.
+ * This diffing algorithm uses a bottom-up memoized longest common subsequence solution to identify differences.
+ * It runs in O(mn) complexity.
+ * The moves are returned in ascending order of their destination index.


Usually, the moves are returned in a form that applies either before any changes, after insertions, or after deletions - in other words, the order of insert / delete being applied (if at all) does matter for the move indices being valid.

Could you expand a bit on the comment to describe the exact order of application for insertions, deletions, and moves that results in the correct transformation?

Will do - these are returned as simple (previousIndex -> newIndex) moves, which is easy to work with and consistent with what vanilla [UICollectionView performBatchUpdates:completion:] wants. (This would be "before any changes")

appleguy · 2018-07-06T23:39:32Z

Source/Details/NSArray+Diffing.mm

+  NSMutableArray<NSIndexPath *> *moveIndexPaths = nil;
+  NSMutableIndexSet *insertionIndexes = nil, *deletionIndexes = nil;
+  if (moves) {
+    potentialMoves = std::unique_ptr<move_map>(new move_map());


Can you change this to stack allocation, matching the style of other unordered_map's in Texture? I think we can skip the typedef in that case too, as it is fairly clear in that style.

This will allocate it in some cases where it is not needed, but as a stack allocation, it is quite efficient.

Adlai-Holler

Hi @wiseoldduck I don't think we've met!

Adlai-Holler · 2018-07-07T00:55:11Z

Source/Details/NSArray+Diffing.mm

+        (*potentialMoves)[hash].push(i);
+      }
+
+    }


In this case it might be worth using fast enumeration and manually keeping track of i. That'll avoid N retain/releases and message sends.

Adlai-Holler · 2018-07-07T00:55:54Z

Source/Details/NSArray+Diffing.mm

+        }
+        if (movedFrom != j) {
+          NSUInteger indexes[] = {movedFrom, j};
+          [moveIndexPaths addObject:[NSIndexPath indexPathWithIndexes:indexes length:2]];


This is a really novel application of NSIndexPath! It's pretty unusual but it gives us tagged pointer support on 64-bit which is a nice freebie.

Prefer +indexPathForItem:section: since that method uses tagged pointers on 64-bit, whereas indexPathWithIndexes:length: does not. Obviously the semantics here are a little wonky but if you add a comment explaining the rationale then no problem.

Ummmmm yeah that's exactly what I was thinking...:p

Haha no I had not .mm-ed the file yet and was in need of something like a std::pair of Indexes. There were all these NSIndexSets floating around, and NSIndexPath jumped right to mind. It seems more natural to me, now, to go ahead and switch to a pair, but this tagged pointer detail is very interesting! I will leave it as is for now and educate myself on tagged pointers ;)

Adlai-Holler · 2018-07-07T00:57:56Z

Source/Details/NSArray+Diffing.mm

+    insertionIndexes = [NSMutableIndexSet indexSet];
+    NSArray *commonObjects = [self objectsAtIndexes:commonIndexes];
+    BOOL moveFound;
+    NSUInteger movedFrom = NSNotFound;


It looks like these two variables can both be moved inside the for loop? And possibly combined – can we use movedFrom != NSNotFound in place of moveFound e.g. NSUInteger movedFrom = (potentialMoves.empty() ? NSNotFound : potentialMoves.front()); at the top of the loop.

Definitely can go in the loop. The BOOL was added in a vain attempt to make this more readable lol!

Adlai-Holler · 2018-07-07T00:58:33Z

Source/Details/NSArray+Diffing.mm

+  dispatch_once(&onceToken, ^{
+    defaultCompare = [^BOOL(id lhs, id rhs) {
+      return [lhs isEqual:rhs];
+    } copy];


Explicitly copying blocks isn't worth doing under ARC since it takes care of that.

Adlai-Holler · 2018-07-07T01:06:15Z

Source/Details/NSArray+Diffing.mm

+          [moveIndexPaths addObject:[NSIndexPath indexPathWithIndexes:indexes length:2]];
+        }
+      }
+        if (i < commonObjects.count && j < array.count && comparison(commonObjects[i], array[j])) {


Indentation here

Adlai-Holler · 2018-07-07T01:08:33Z

Source/Private/ASLayoutTransition.mm

@@ -254,7 +300,7 @@ - (ASSizeRange)transitionContext:(_ASTransitionContext *)context constrainedSize
  for (ASLayout *sublayout in layout.sublayouts) {
    if (idx > lastIndex) { break; }
    if (idx >= firstIndex && [indexes containsIndex:idx]) {
-      ASDisplayNode *node = (ASDisplayNode *)sublayout.layoutElement;
+      ASDisplayNode *node = ASDynamicCast(sublayout.layoutElement, ASDisplayNode);


Is this change necessary? Not that this is a particularly hot path but ASDynamicCast incurs a runtime cost whereas a static cast is free (if you know it's safe.)

Haha this has nothing to do with the rest of this does it. But I noticed the comment below which was was claiming a behavior that simple type casting doesn't actually have!

// Ignore the odd case in which a non-node sublayout is accessed and the type cast fails if (node != nil) {

I will move this into a separate #trivial PR as there are already plenty of moving parts here to keep straight ;)

Adlai-Holler · 2018-07-07T01:16:05Z

Source/Private/ASLayoutTransition.mm

+    _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes);
+    for (IGListMoveIndex *move in result.moves) {
+      id subnode = previousLayout.sublayouts[static_cast<NSUInteger>(move.from)].layoutElement;
+      _subnodeMoves.push_back(std::pair<id, NSUInteger>(subnode, move.to));


A C-cast ((NSUInteger)move.from) is probably sufficient instead of a static_cast (which is usually used for C++ types). Is any kind of cast actually necessary though – the implicit conversion shouldn't be dangerous.

This is a good place to use unowned id subnode to avoid the extra retain/release pair that ARC will insert otherwise – we will retain the object into the std::pair on the next line so it's safe.

The cast is entirely for "clarity", but I don't really know why I thought it helped :-| :D

Adlai-Holler · 2018-07-07T01:17:10Z

Source/Private/ASLayoutTransition.mm

@@ -67,6 +73,7 @@ @implementation ASLayoutTransition {
  NSArray<ASDisplayNode *> *_removedSubnodes;
  std::vector<NSUInteger> _insertedSubnodePositions;
  std::vector<NSUInteger> _removedSubnodePositions;
+  std::vector<std::pair<id, NSUInteger>> _subnodeMoves;


Let's use ASDisplayNode * here for some type-safety.

Adlai-Holler · 2018-07-07T01:18:02Z

Source/Private/ASLayoutTransition.mm

-  for (ASDisplayNode *node in _insertedSubnodes) {
+  NSUInteger j = 0;
+  for (j = 0; j < _subnodeMoves.size(); ++j) {
+    [_subnodeMoves[j].first _removeFromSupernodeIfEqualTo:_node];


I think we can do for (auto const &move : _subnodeMoves) { [move.first _remove…] } and use C++ enumeration below (during the insertions) as well.

I don't think it is so straightforward below (insertions/moves), as we are interleaving the two vectors and consuming from only one or the other each time.

Adlai-Holler · 2018-07-07T01:19:26Z

Source/Private/ASLayoutTransition.mm

+    // These should arrive sorted in ascending order of move destinations.
+    for (NSIndexPath *move in moves) {
+      id subnode = previousLayout.sublayouts[static_cast<NSUInteger>([move indexAtPosition:0])].layoutElement;
+      _subnodeMoves.push_back(std::pair<id, NSUInteger>(subnode, [move indexAtPosition:1]));


Is a cast here necessary? NSArray indexing should do the conversion implicitly.

Same as above, it was for clarity, but I don't even agree with past-me about this one - it already is a NSUInteger!

wiseoldduck · 2018-07-07T02:54:26Z

Thanks guys, the comments I didn't reply to are all "Will do"s!

* Use `unordered_multimap` on stack instead of unordered_map<id,queue> on heap * Remove notFound BOOL (use NSNotFound sentinel value) and put some vars inside the if (insertions/moves) loop * Don't copy defaultCompare block (redundant under ARC) * Whitespace * Remove unneeded mutableCopy-s in ArrayDiffingTests

* Type _subnodeMoves pair.first to ASDisplayNode * instead of id * C++ enumeration * unowned refs for adding previousLayout nodes to _subnodeMoves * Remove unreleated ASDynamicCast that is probably right though

Adlai-Holler

Diff is shaping up nicely! Couple bits of polish

Adlai-Holler · 2018-07-10T18:39:25Z

Source/Private/ASLayoutTransition.mm

+    _removedSubnodePositions = findNodesInLayoutAtIndexes(previousLayout, result.deletes, &_removedSubnodes);
+    for (IGListMoveIndex *move in result.moves) {
+      unowned ASDisplayNode *subnode = previousLayout.sublayouts[move.from].layoutElement;
+      _subnodeMoves.push_back(std::pair<ASDisplayNode *, NSUInteger>(subnode, move.to));


Import <utility> and use std::make_pair(subnode, move.to). This:

Uses type inference to remove code.

Makes the pair <unowned ASDisplayNode *, NSUInteger>, skipping the extra retain/release.

Fascinating! So it will infer unowned? Is that a general rule for C++ type inference of ObjC types?

Adlai-Holler · 2018-07-10T18:46:58Z

Source/Private/ASLayoutTransition.mm

+    std::sort(_subnodeMoves.begin(), _subnodeMoves.end(), [](std::pair<id<ASLayoutElement>, NSUInteger> a,
+            std::pair<ASDisplayNode *, NSUInteger> b) {
+      return a.second < b.second;
+    });


Use auto a, auto b to save space

"Generic lambdas are not supported by the compiler"

What? That's crazy, it works in a sample project! OK must be something contextual, no worries

It should be available starting with C++ 14. We should double check what version we are run on.

cc @wiseoldduck hey - could you check on your side?

It appears to be set to c++0x in the .xcodeproj, maybe it is being built differently by BUCK?

Adlai-Holler · 2018-07-10T18:47:25Z

Source/Private/ASLayoutTransition.mm

+    // These should arrive sorted in ascending order of move destinations.
+    for (NSIndexPath *move in moves) {
+      unowned ASDisplayNode *subnode = previousLayout.sublayouts[([move indexAtPosition:0])].layoutElement;
+      _subnodeMoves.push_back(std::pair<id, NSUInteger>(subnode, [move indexAtPosition:1]));


Is it valid to remove the parens on 213? previousLayout.sublayouts[[move indexAtPosition:0]].layoutElement

Same make_pair game to safe space and retain/release traffic

Adlai-Holler · 2018-07-10T18:49:21Z

Source/Layout/ASLayout+IGListKit.mm

+  ASLayout *otherLayout = ASDynamicCast(other, ASLayout);
+  if (!otherLayout) return NO;
+
+  return [otherLayout.layoutElement isEqual:self.layoutElement];


Nit: remove nullability annotations from implementation file to be consistent with the rest of the framework style.

If it's easy to do, import the header that exposes the _layoutElement instance variable and use that directly to avoid extra retain/release on these elements. If not, it's not a blocker for this diff.

Adlai-Holler

One more thing before we land

Adlai-Holler · 2018-07-11T19:22:37Z

Source/Layout/ASLayout+IGListKit.mm

+
+- (id <NSObject>)diffIdentifier
+{
+  return [NSValue valueWithPointer: (__bridge void*) self->_layoutElement];


Rather than allocating a new object here, can we just return _layoutElement;? It may mean the element survives longer (inside the IGListDiff) but probably worth it on balance.

Adlai-Holler · 2018-07-13T01:40:51Z

Source/Layout/ASLayout+IGListKit.h

+@interface ASLayout(IGListKit) <IGListDiffable>
+@end
+
+#endif // AS_IG_LIST_KIT


Now that we're diffing nodes, not layouts, can we remove this?

We talked on Slack, moving the IGListKit approach to diff node-arrays rather than layouts could come in a separate diff.

ghost · 2018-07-13T02:09:53Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

* Add move detection and support to ASLayoutTransition ...and NSArray+Diffing. Add some tests. * Update CHANGELOG.md * Update CHANGELOG.md * Update ASLayout+IGListKit.h * Update ASLayout+IGListKit.mm * Use std collections to avoid NSNumber boxing * Update ASLayoutTransition.mm * Code review updates. * Use `unordered_multimap` on stack instead of unordered_map<id,queue> on heap * Remove notFound BOOL (use NSNotFound sentinel value) and put some vars inside the if (insertions/moves) loop * Don't copy defaultCompare block (redundant under ARC) * Whitespace * Remove unneeded mutableCopy-s in ArrayDiffingTests * Code review updates. * Type _subnodeMoves pair.first to ASDisplayNode * instead of id * C++ enumeration * unowned refs for adding previousLayout nodes to _subnodeMoves * Remove unreleated ASDynamicCast that is probably right though * Add commentary to NSArray+Diffing.h; make multimap elements unowned * Use std::make_pair, optimize ASLayout+IGListKit * Oops I thought I had added these headers but nope * Simplify simplify * Diff subnodes instead of sublayouts * Another randomized test with actual ASLayouts

Add move detection and support to ASLayoutTransition

b0c715d

...and NSArray+Diffing. Add some tests.

wiseoldduck added 5 commits July 3, 2018 14:20

Update CHANGELOG.md

f189d1b

Update CHANGELOG.md

09d43ae

Update ASLayout+IGListKit.h

baba616

Update ASLayout+IGListKit.mm

beb204e

Use std collections to avoid NSNumber boxing

d4ce1bc

wiseoldduck force-pushed the handle-moves branch from 88f0874 to d4ce1bc Compare July 6, 2018 20:11

appleguy reviewed Jul 6, 2018

View reviewed changes

Adlai-Holler reviewed Jul 7, 2018

View reviewed changes

wiseoldduck added 3 commits July 7, 2018 01:05

Update ASLayoutTransition.mm

18bf81a

Code review updates.

9898f0e

* Type _subnodeMoves pair.first to ASDisplayNode * instead of id * C++ enumeration * unowned refs for adding previousLayout nodes to _subnodeMoves * Remove unreleated ASDynamicCast that is probably right though

Adlai-Holler reviewed Jul 10, 2018

View reviewed changes

wiseoldduck added 3 commits July 10, 2018 12:21

Add commentary to NSArray+Diffing.h; make multimap elements unowned

a8cf065

Use std::make_pair, optimize ASLayout+IGListKit

14150e1

Oops I thought I had added these headers but nope

b400cac

TextureGroup deleted a comment Jul 11, 2018

Adlai-Holler approved these changes Jul 11, 2018

View reviewed changes

wiseoldduck force-pushed the handle-moves branch from 610f05e to d343adc Compare July 12, 2018 00:00

Simplify simplify

0c949c4

wiseoldduck force-pushed the handle-moves branch from d343adc to 0c949c4 Compare July 12, 2018 00:10

TextureGroup deleted a comment Jul 12, 2018

wiseoldduck added 2 commits July 12, 2018 16:04

Diff subnodes instead of sublayouts

067aa85

Another randomized test with actual ASLayouts

d2ca43e

wiseoldduck force-pushed the handle-moves branch from 31ce2dc to d2ca43e Compare July 12, 2018 23:11

Adlai-Holler reviewed Jul 13, 2018

View reviewed changes

Adlai-Holler merged commit 8986838 into TextureGroup:master Jul 13, 2018

wiseoldduck mentioned this pull request Jul 23, 2018

Remove misleading comment and add assertion #trivial #1027

Merged

Add move detection and support to ASLayoutTransition #1006

Add move detection and support to ASLayoutTransition #1006

Conversation

wiseoldduck commented Jul 3, 2018

CLAassistant commented Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiseoldduck Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiseoldduck Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

wiseoldduck Jul 7, 2018 • edited Loading

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

wiseoldduck commented Jul 7, 2018

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

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

ghost commented Jul 13, 2018

CLAassistant commented Jul 3, 2018 •

edited

Loading

wiseoldduck Jul 7, 2018 •

edited

Loading

wiseoldduck Jul 7, 2018 •

edited

Loading

wiseoldduck Jul 7, 2018 •

edited

Loading