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

Fix collection editing #1081

Merged
merged 27 commits into from
Sep 5, 2018
Merged

Conversation

wsdwsd0829
Copy link
Contributor

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.

[_changeSet moveItemAtIndexPath:indexPath toIndexPath:newIndexPath animationOptions:kASCollectionViewAnimationNone];
} completion:nil];
if (!_editing) {
[self performBatchUpdates:^{
Copy link
Member

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.

Copy link
Contributor Author

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?

* Set during beginInteractiveMovementForItemAtIndexPath and UIGestureRecognizerStateEnded
* (or UIGestureRecognizerStateFailed, UIGestureRecognizerStateCancelled.
*/
BOOL _editing;
Copy link
Member

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 ?

Copy link
Member

@Adlai-Holler Adlai-Holler left a 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.


- (BOOL)beginInteractiveMovementForItemAtIndexPath:(NSIndexPath *)indexPath {
_reordering = YES;
return [super beginInteractiveMovementForItemAtIndexPath:indexPath];
Copy link
Member

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.

@maicki
Copy link
Contributor

maicki commented Sep 5, 2018

@wsdwsd0829 Would you please add an entry to the CHANGELOG.md and than let's land it!

@wsdwsd0829
Copy link
Contributor Author

done, Thanks for reminding.

Copy link
Contributor

@maicki maicki left a 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!

@maicki maicki merged commit dbcf8ba into TextureGroup:master Sep 5, 2018
mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Oct 2, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants