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

Remove selected IndexPath before notifying delegate #165

Merged
merged 1 commit into from May 20, 2016
Merged

Remove selected IndexPath before notifying delegate #165

merged 1 commit into from May 20, 2016

Conversation

michaelzoech
Copy link

The contract for deselectItemAtIndexPath is to remove the index path
from the array before notifying the delegate. This allows the user
to check the selectedIndexes if there are remaining selections.

This patch applies this contract to reloadData where until now
selectedIndexes was cleaned after notifying the delegate.

@jwilling
Copy link
Owner

jwilling commented May 20, 2016

Thank you for putting this together! ✨

I'm debating whether this is the behavior that would be desired, however. Would it make more sense to remove all selected indexes first, then notify the delegate? That way when the delegate receives each message the state would be finalized. Here's what I'm thinking:

- (void)reloadData {
    _collectionViewFlags.wantsLayout = YES;

    NSArray *selectedIndexes = self.selectedIndexes.copy;   
    [self.selectedIndexes removeAllObjects];

    // Remove any selected indexes we've been tracking.
    if (_collectionViewFlags.delegateDidDeselect) {
        for (NSIndexPath *indexPath in selectedIndexes) {
            [self.delegate collectionView:self didDeselectItemAtIndexPath:indexPath];
        }
    }

    // ...
}

What do you think?

The contract for deselectItemAtIndexPath is to remove the index path
from the array before notifying the delegate. This allows the user
to check the selectedIndexes if there are remaining selections.

This patch applies this contract to reloadData where until now
selectedIndexes was cleaned after notifying the delegate.
@michaelzoech
Copy link
Author

Sounds good to me. I've updated the PR

@jwilling
Copy link
Owner

Thanks again! 🌴

@jwilling jwilling merged commit 38ec08a into jwilling:master May 20, 2016
@michaelzoech
Copy link
Author

np. thx for providing such an awesome library.

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.

None yet

2 participants