lxcid / LXReorderableCollectionViewFlowLayout

Extends `UICollectionViewFlowLayout` to support reordering of cells. Similar to long press and pan on books in iBook.
http://lxcid.com/
MIT License
1.86k stars 328 forks source link

Problems with Supplementary Views #15

Open ToddManUtah opened 11 years ago

ToddManUtah commented 11 years ago

Before I start: AWESOME code!!!! This has saved me SO SO SO much time.

Here is my problem: I've implemented you code by integrating it with my existing UICollectionView object: @property (nonatomic, strong) IBOutlet UICollectionView *myCollectionView;

In my viewDidLoad method I added this code: LXReorderableCollectionViewFlowLayout *layout = [[LXReorderableCollectionViewFlowLayout alloc] init]; [self.myCollectionView setCollectionViewLayout:layout]; [layout setUpGestureRecognizersOnCollectionView];

Everything is working great except for my section headers. I am trying to support multiple sections in my collection view. I have changed the delegate that allows for move to allow for moving the items between sections and it works great.

The problem is that the section headers themselves don't show up. I see a space there but no text/graphics/buttons etc... I've tracked it down to the fact that my method:

isn't getting called. Do I need to do something special here? The section headers show up fine without integrating your code, but dissappear when I add the above code. I'm kinda of a newb and completely confused.

Is this a problem with your code or mine? (I'm SURE it's mine)

Thanks for any help.

ToddManUtah commented 11 years ago

I figured it out. The header reference size was zero. Once I added the line of code: layout.headerReferenceSize = CGSizeMake(0, 50);

everything started working. I'm not sure why this is needed. I didn't have to specify the size before. Either way, it's working now!

Thanks again for the great code.

ToddManUtah commented 11 years ago

I've found another problem along the same lines (problem with Supplementary Views).

The method "willMoveToIndexPath" isn't being called when I cross the border from one section to another section ONLY when there are no items in that section. If I drag an item from section "0" to section "1" and there are items in section "1", it works fine. If section "1" doesn't have items in it, the "willMoveToIndexPath" method is never called.

I using the collection view to show multiple albums of photos. Each section correlates to a different album. Dragging a photo from "Album 1" to "Album 2" works fine when each album already has items in it. But, I give the user the ability to create new albums (sections) and then drag items from other albums into it.

I've tracked the issue to "invalidateLayoutIfNecessary". When I cross the border from section "0" into section "1" the NSIndexPath theIndexPathOfSelectedItem goes to 0,0 (section, row). I would have expected it to go to 1,0. The problem seems to be with "indexPathForItemAtPoint". It isn't taking into consideration that the section might be empty and just defaults to 0, 0 when I cross the border into a different (and empty) section.

Like I said earlier, if there are items already in section "1" then the indexPath is set correctly.

Any ideas on what I'm doing wrong?

lxcid commented 11 years ago

From a rough guess, I think it might not be an easy problem to solve because it depends on the query of index path from collection view. I'll have check back the code and reply soon I hope.

Sent from my iPhone

On 9 Mar, 2013, at 5:51 AM, ToddManUtah notifications@github.com wrote:

I've found another problem along the same lines (problem with Supplementary Views).

The method "willMoveToIndexPath" isn't being called when I cross the border from one section to another section ONLY when there are no items in that section. If I drag an item from section "0" to section "1" and there are items in section "1", it works fine. If section "1" doesn't have items in it, the "willMoveToIndexPath" method is never called.

I using the collection view to show multiple albums of photos. Each section correlates to a different album. Dragging a photo from "Album 1" to "Album 2" works fine when each album already has items in it. But, I give the user the ability to create new albums (sections) and then drag items from other albums into it.

I've tracked the issue to "invalidateLayoutIfNecessary". When I cross the border from section "0" into section "1" the NSIndexPath theIndexPathOfSelectedItem goes to 0,0 (section, row). I would have expected it to go to 1,0.

Like I said earlier, if there are items already in section "1" then the indexPath is set correctly.

Any ideas on what I'm doing wrong?

— Reply to this email directly or view it on GitHub.

ToddManUtah commented 11 years ago

Thanks so much. I wanted to demo this Monday afternoon for someone and I'm all but done. Except for this one problem. And it's killing me.

Thanks SO much for the help.

This is the method I changed to account for multiple sections:

 - (void)collectionView:(UICollectionView *)theCollectionView layout:(UICollectionViewLayout *)theLayout itemAtIndexPath:(NSIndexPath *)theFromIndexPath willMoveToIndexPath:(NSIndexPath *)theToIndexPath
 {
NSLog(@"From: %d %d    To: %d %d", theFromIndexPath.section, theFromIndexPath.row, theToIndexPath.section, theToIndexPath.row);

MINAlbum *fromAlbum = self.pageVolume.albumsArray[theFromIndexPath.section];
MINAlbum *toAlbum = self.pageVolume.albumsArray[theToIndexPath.section];
id theFromItem = fromAlbum.albumItemsArray[theFromIndexPath.item];
[fromAlbum.albumItemsArray removeObjectAtIndex:theFromIndexPath.item];
[toAlbum.albumItemsArray insertObject:theFromItem atIndex:theToIndexPath.item];
[self.pageVolume writeToLocalDevice];
 }

I added this image to show you how I'm seeing the problem.

Screen Shot 2013-03-09 at 8 54 37 AM

On another point: I get a "some-times" crash when I only have two elements in the collection view. I traced this down to a bug with Apple. There is a work around. I changed the last part of your method "invalidateViewIfNecessary". The problem seems to happen after the second move (and not every time). I get an exception when calling:

 [self.collectionView performBatchUpdates:^{
        //[self.collectionView moveItemAtIndexPath:thePreviousSelectedIndexPath toIndexPath:theIndexPathOfSelectedItem];
        [self.collectionView deleteItemsAtIndexPaths:@[ thePreviousSelectedIndexPath ]];
        [self.collectionView insertItemsAtIndexPaths:@[ theIndexPathOfSelectedItem ]];
    } completion:^(BOOL finished) {
    }];

The fix according to what I've been able to find is to reloadData. This messes up the animation but not badly. I also only called this new call if I'm dragging into/out-of the zero based item. Here's the code that worked for me. Hope it helps:

 - (void)invalidateLayoutIfNecessary {
NSIndexPath *theIndexPathOfSelectedItem = [self.collectionView indexPathForItemAtPoint:self.currentView.center];
if ((![theIndexPathOfSelectedItem isEqual:self.selectedItemIndexPath]) &&(theIndexPathOfSelectedItem)) {
    NSIndexPath *thePreviousSelectedIndexPath = self.selectedItemIndexPath;

 ....
        // Proceed with the move
        [theDelegate collectionView:self.collectionView
                             layout:self
                    itemAtIndexPath:thePreviousSelectedIndexPath
                willMoveToIndexPath:theIndexPathOfSelectedItem];
    }

    // This seems to be a Apple bug - we get an exception when dealing with the first element in the collection view.  The fix is to just call "reloadData"
    if((thePreviousSelectedIndexPath.row == 0) || (theIndexPathOfSelectedItem.row == 0))
    {
        [self.collectionView reloadData];
    }
    else
    {
        [self.collectionView performBatchUpdates:^{
            //[self.collectionView moveItemAtIndexPath:thePreviousSelectedIndexPath toIndexPath:theIndexPathOfSelectedItem];
            [self.collectionView deleteItemsAtIndexPaths:@[ thePreviousSelectedIndexPath ]];
            [self.collectionView insertItemsAtIndexPaths:@[ theIndexPathOfSelectedItem ]];
        } completion:^(BOOL finished) {
        }];
    }

    /*
    [self.collectionView performBatchUpdates:^{
        //[self.collectionView moveItemAtIndexPath:thePreviousSelectedIndexPath toIndexPath:theIndexPathOfSelectedItem];
        [self.collectionView deleteItemsAtIndexPaths:@[ thePreviousSelectedIndexPath ]];
        [self.collectionView insertItemsAtIndexPaths:@[ theIndexPathOfSelectedItem ]];
    } completion:^(BOOL finished) {
    }];
     */
}

}

lxcid commented 11 years ago

I can't reply back by Monday afternoon though. I'm sorry so sorry about that. I had some personal errand to run for all of this weekend and part of next week. Is it possible you look for an alternative solution first?

The problem lies in that the very method that allows this project to happen is UICollectionView indexPathForItemAtPoint.

http://developer.apple.com/library/ios/documentation/UIKit/Reference/UICollectionView_class/Reference/Reference.html#//apple_ref/occ/instm/UICollectionView/indexPathForItemAtPoint:

This method will returns null if the point contains no item.

I don't think I can get around that limitation because there's no method (not that I know of) that allows you to query about the section the point is in.

A hack will be to adds a placeholder item at the end of each section or empty section. Be it being displayed or not. This will ensure there's always an item so that the query for index path at point will works.

I only foresee myself able to dive deep into this issue at ard Tuesday or Wednesday. After which I be away for most of the week again.

I hope my description here can help you in one way or another.

Sent from my iPhone

On 9 Mar, 2013, at 11:04 PM, ToddManUtah notifications@github.com wrote:

Thanks so much. I wanted to demo this Monday afternoon for someone and I'm all but done. Except for this one problem. And it's killing me.

Thanks SO much for the help.

— Reply to this email directly or view it on GitHub.

ToddManUtah commented 11 years ago

I had the same idea on creating a dummy placeholder item. I'll play around with that idea and see how far I can get.

Thanks for taking the time. I'm sure you'll figure out a WAY better solution than I will. Let me know if you have any ideas (don't worry about Monday).

lukescott commented 11 years ago

@ToddManUtah Try this fork, and if you still have the same problem, open an issue (and link to this one) and I'll take a look at it when I get a chance. The code is refactored from this one and it's a little easier to follow. You may be able to work out the problem there.

There is a pull request to here, but the code hasn't been reviewed yet. Eventually all of it should end up here.

jeffdav commented 10 years ago

As the code is currently written, in - (void)handleLongPressGesture:(UILongPressGestureRecognizer *)gestureRecognizer the case for UIGestureRecognizerStateBegan does not check the return value for indexPathForItemAtPoint. If it is an accessory view, it will be nil, but collectionView:willBeginDraggingItemAtIndexPath: is called anyway.

The case for UIGestureRecognizerStateEnded, however, does check for nil and does NOT send collectionView:willEndDraggingItemAtIndexPath:. This leads to unbalanced delegate callbacks, which can cause subtle state errors in the delegate.

It would be nice if there was (a) some way of noting its an accessory view other than sending a nil indexPath, or (b) always send balanced calls with nil so the delegate has a chance of doing the right thing.