-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix collection editing #1081
Fix collection editing #1081
Conversation
Source/ASCollectionView.mm
Outdated
[_changeSet moveItemAtIndexPath:indexPath toIndexPath:newIndexPath animationOptions:kASCollectionViewAnimationNone]; | ||
} completion:nil]; | ||
if (!_editing) { | ||
[self performBatchUpdates:^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should performBatchUpdates itself change behavior when interactiveMovement is in progress?
Imagine a reorder starts at the same time that a new page of content comes in, creating an insertion. I'm not sure what's actually supposed to happen there, but maybe we would want to just have performBatchUpdates internally run the batch operations without actually calling performBatchUpdates on the parent view?
That said, doing this only for moves right now seems reasonable if there is not a clear answer to this other question. Ideally in the case of the insertion, we at least know the UI will end up in a good state and not crash, even if it forces the user out of interactive mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method is called on main thread and should not be happening at same time? One case should be blocked waiting to the other to finish?
Source/ASCollectionView.mm
Outdated
* Set during beginInteractiveMovementForItemAtIndexPath and UIGestureRecognizerStateEnded | ||
* (or UIGestureRecognizerStateFailed, UIGestureRecognizerStateCancelled. | ||
*/ | ||
BOOL _editing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this reordering, or maybe _interactiveMovementInProgress ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense.
Source/ASCollectionView.mm
Outdated
|
||
- (BOOL)beginInteractiveMovementForItemAtIndexPath:(NSIndexPath *)indexPath { | ||
_reordering = YES; | ||
return [super beginInteractiveMovementForItemAtIndexPath:indexPath]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If super returns NO
here, I think it's appropriate for us not to set our flag. If the user notices that interactive movement was refused, he may not ever end the movement.
@wsdwsd0829 Would you please add an entry to the |
done, Thanks for reminding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks also good to me. Let's land it!
* fix SIMULATE_WEB_RESPONSE not imported TextureGroup#449 * Fix to make rangeMode update in right time * remove uncessary assert * Fix collection cell editing bug for iOS 9 & 10 * Rename to reordering. * Adjust _reordering more acuratedly * Add change log
It's used to fix cell editing bug for iOS 9 & 10 where UICV moveItemToIndexPath will be called every time cell is dragged through other cells (before dataSource's collectionNode:moveItemAtIndexPath... is called). For these cases we should call super code to handle the cell swapping internally in UICV. (moveItemToIndexPath for iOS 11+ will not be called though, so the reasoning is that the code after iOS11 have abstracted the method call to internal without triggering through public APIs).
ASCollectionView example app's reordering cell is fixed by this PR.