nicklockwood / iCarousel

A simple, highly customisable, data-driven 3D carousel for iOS and Mac OS
http://www.charcoaldesign.co.uk/source/cocoa#icarousel
Other
12k stars 2.58k forks source link

Index 0 always called in viewForItemAtIndex: reusingView: #590

Closed jnpetersen closed 9 years ago

jnpetersen commented 9 years ago

I'm using my iCarousel a little different where I have a tableview that when a cell is selected, that index of the tableviewcell sets the currentIndex for the carousel. This all works fine, until it goes to display the carousel and calls the viewForItemAtIndex: reusingView:.

Everytime this method is called, the first index it loads is always 0, which I believe is causing an issue when displaying my feed items. For example, lets say I select the 12th item in my tableview, it will load the following:

0, 10, 11, 12, 13, 14

When it does this, it makes one of the views index 0's object and messes everything up.

jnpetersen commented 9 years ago

Here is the example, I'm clicking on the very last one, and in the second image, its showing the content from index 0 instead of index 14.

ios simulator screen shot mar 30 2015 8 40 34 pm

ios simulator screen shot mar 30 2015 8 40 37 pm

jnpetersen commented 9 years ago

Any help would be greatly appreciated.

nicklockwood commented 9 years ago

The order in which iCarousel calls viewForItemAtIndex is unrelated to the currently selected index. I doubt that's related to your problem.

jnpetersen commented 9 years ago

Ok, well I've noticed that when it loads the carousel, its only loading 5 and repeating those, it does not load all of the views in the array i'm using. Any suggestions?

nicklockwood commented 9 years ago

I'm guessing that you're checking if reusingView is nil, and only setting the view if it isn't? Views are recycled like tableView cells, so the same view will be used for multiple indexes.

Even if reusingView is non-nil, you'll still need to configure it with the correct data for the current index (look more closely at how the example code works).

jnpetersen commented 9 years ago

Ok, I will take a look, if it works, I'll be back to close the issue.

jnpetersen commented 9 years ago

This is what comes in my output from when I click on the last item in my table. As you can see where it says "Current Index Selected" it shows 0 first, then the other 3 that are truely visible. The second line is the title of the article, which is newer than the one in the image above, but is still showing as the last item when it should be 6.

2015-04-15 12:13:37.170 NewsReader[9379:1275049] Current Index Selected: 0
2015-04-15 12:13:37.170 NewsReader[9379:1275049] Immigrant Students Fight to Retain the Tuition Benefits That Have Eased Their Paths to College
2015-04-15 12:13:37.172 NewsReader[9379:1275049] Current Index Selected: 5
2015-04-15 12:13:37.172 NewsReader[9379:1275049] (null)
2015-04-15 12:13:37.173 NewsReader[9379:1275049] Current Index Selected: 6
2015-04-15 12:13:37.173 NewsReader[9379:1275049] Corinthian Colleges Fined for Bogus Job-Placement Claims
2015-04-15 12:13:37.175 NewsReader[9379:1275049] Current Index Selected: 4
2015-04-15 12:13:37.175 NewsReader[9379:1275049] Debunking myths about college tuition
jnpetersen commented 9 years ago

Actually... sorry, its showing the last one, but then it shows 0 for 5 as it shows null, but this is a direct pull from my array.

[5] Post {
        ID = 2436;
        title = Clinton Criticizes For-Profits, Talks College Costs;
        status = publish;
        type = newsarticle;
jnpetersen commented 9 years ago

I moved my logging around, and noticed it only loads 0, 6 and 4, no 5, and 5 is coming up with 0's data.

nicklockwood commented 9 years ago

I think it would help if you posted your viewForItemAtIndex method.

jnpetersen commented 9 years ago

Just a heads up, there are multiple carousels. If you have any questions, please let me know.

- (UIView *)carousel:(iCarousel *)carousel viewForItemAtIndex:(NSInteger)index reusingView:(UIView *)view
{
    UILabel *label = nil;
    UILabel *label2 = nil;
    UILabel *titleLabel = nil;
    UIImageView *logoView = nil;
    UILabel *dateLabel = nil;
    UILabel *textLabel = nil;
    UIScrollView *scrollView = nil;
    Post *object = nil;
//    NSLog(@"%@", self.array);

    if (view == nil)
    {
        if (carousel == self.feedPicker) {
//            NSLog(@"# %ld", index);
//            object =  self.array[index];
            view = [[UIView alloc] initWithFrame:CGRectMake(0, 50, 250, 400)];
            view.backgroundColor = [UIColor whiteColor];

            titleLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, 30, 225, 22)];
            titleLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:19];
            titleLabel.numberOfLines = 2;
            [titleLabel setLineBreakMode:NSLineBreakByWordWrapping];
            [view addSubview:titleLabel];

            //    cell.avatarImage.image = [image resizedImageByMagick:@"50x50#"];

            logoView = [[UIImageView alloc] initWithFrame:CGRectMake(180, -25, 50, 50)];
            logoView.backgroundColor = [UIColor orangeColor];
            [view addSubview:logoView];

            dateLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, titleLabel.frame.size.height+25, 225, 22)];
            dateLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:10];

            [view addSubview:dateLabel];

            scrollView = [[UIScrollView alloc] initWithFrame:CGRectMake(15, dateLabel.frame.origin.y+25, 225, 240)];

            [view addSubview:scrollView];

            textLabel = [[UILabel alloc] initWithFrame:CGRectMake(0, 0, 225, 200)];
            textLabel.font = [UIFont fontWithName:@"Roboto-Light" size:12];
            textLabel.numberOfLines = 0;

            [scrollView addSubview:textLabel];

//            NSLog(@"%@", object.acf.news_article_summary);
//            [view addSubview:textLabel];

            CGSize maxSize = CGSizeMake(textLabel.frame.size.width, CGFLOAT_MAX);
            CGSize labelSize = [textLabel sizeThatFits:maxSize];
            scrollView.contentSize = labelSize;

            UIButton *linkButton = [[UIButton alloc] initWithFrame:CGRectMake(15, view.bounds.size.height-50, 100, 35)];
            linkButton.backgroundColor = [UIColor orangeColor];
            [linkButton setTitle:@"FULL TEXT" forState:UIControlStateNormal];
            linkButton.titleLabel.font = [UIFont fontWithName:@"Roboto-Light" size:14];
            linkButton.tag = index;
            [linkButton addTarget:self action:@selector(loadArticle:) forControlEvents:UIControlEventTouchUpInside];
            [view addSubview:linkButton];
        } else {
            view = [[UIView alloc] initWithFrame:[self sizeForCarousel:carousel]];
            view.clipsToBounds = YES;
            view.backgroundColor = [UIColor clearColor];
            view.contentMode = UIViewContentModeCenter;
            label = [[UILabel alloc] initWithFrame:(self.monthYearPicker == carousel ? view.bounds : view.bounds)];
            label.backgroundColor = [UIColor clearColor];
            label.textColor = [UIColor whiteColor];
            label.textAlignment = NSTextAlignmentCenter;
            label.numberOfLines = 1;

            [view addSubview:label];

            if (carousel == self.monthYearPicker) {
                label2 = [[UILabel alloc] initWithFrame:CGRectMake(0, 35, view.bounds.size.width, view.bounds.size.height/2)];
                label2.backgroundColor = [UIColor clearColor];
                label2.textColor = [self colorFromHexString:@"#9fabdc"];
                label2.textAlignment = NSTextAlignmentCenter;
                label2.numberOfLines = 1;
                [view addSubview:label2];
                label2.font = [UIFont fontWithName:[self fontForCarousel:carousel] size:[self fontSizeForCarousel:carousel]-4];

            }

    //        if (carousel.currentItemIndex == index && carousel == self.dayPicker) {
            label.font = [UIFont fontWithName:[self fontForCarousel:carousel] size:[self fontSizeForCarousel:carousel]];
    //        } else if (carousel.currentItemIndex != index && carousel == self.dayPicker) {
    //        label.font = [UIFont fontWithName:[self fontForCarousel:carousel] size:[self fontSizeForCarousel:carousel]-20];
    //        }
        }

        if (!object) {
            object =  self.array[index];
        }
        NSLog(@"Current Index Selected: %ld", index);
        NSLog(@"%@", object.title);
        titleLabel.text = [[object.title stringByStrippingHTML] xmlSimpleUnescapeString];
        [titleLabel sizeToFit];

        NSURL *imageURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@", object.acf.news_source_logo]];

        [logoView sd_setImageWithURL:imageURL completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, NSURL *imageURL) {
            logoView.image = [image resizedImageByMagick:@"50x50#"];
            CALayer *layer = [logoView layer];

            [layer setMasksToBounds:YES];
            [layer setCornerRadius:25];
        }];

        NSDateFormatter *f = [[NSDateFormatter alloc] init];

        [f setDateFormat:@"yyyy-MM-dd'T'HH:mm:ssz"];

        NSDate *testDate = [f dateFromString:object.date];
        [f setDateFormat:@"MMMM dd, yyyy"];
        //            [self setSubtitleForCell:cell item:[f stringFromDate:testDate]];

        CGRect dateFrame = dateLabel.frame;

        dateFrame.origin.y = titleLabel.frame.size.height+25;
        [dateLabel setFrame:dateFrame];
        dateLabel.text = [f stringFromDate:testDate];

        CGRect textFrame = scrollView.frame;

        textFrame.origin.y = dateLabel.frame.origin.y+25;
        [scrollView setFrame:textFrame];

        textLabel.text = [[object.acf.news_article_summary stringByStrippingHTML] xmlSimpleUnescapeString];
        [textLabel sizeToFit];

    } else {
        if ([[view subviews] count] > 1) {
            label = [[view subviews] objectAtIndex:0];
            label2 = [[view subviews] objectAtIndex:1];
        } else {
            label = [[view subviews] lastObject];
        }
    }

    if (carousel == monthYearPicker) {
        NSArray *parts = [[self.pickerViewArray objectAtIndex:index] componentsSeparatedByString:@" "];
        label.text = [parts objectAtIndex:0];
        label2.text = [parts objectAtIndex:1];
    } else if (carousel == dayPicker) {
        label.text = [self.pickerDayViewArray objectAtIndex:index];
    }

    return view;
}
jnpetersen commented 9 years ago

Any ideas? I just noticed that when I'm running it, the 2nd to the last object in my array isn't being read, and instead index 0 is being put in place @nicklockwood.

nicklockwood commented 9 years ago

Yes, it's basically what I said: When you set

linkButton.tag = index;

Or look up

self.array[index];

You're doing that inside the if (view == nil) check, so you only set it when the view is first created. Like UITableView, iCarousel only allocates enough views to fill one screen's worth, then reuses them over and over again. It does not create a new view for each item in the carousel.

Due to view recycling, the next time that view is used, it probably won't have the same index, but you never update it again, so it will always appear with the details that were set for whatever the index was when you first created it.

Any view setup that is index-specific must be done outside of the if (view == nil) check. That's why the example code does that stuff with tags to look up the existing view label instead of setting it inside the if statement,

jnpetersen commented 9 years ago

@nicklockwood: Ok, I understand what your saying... but i'm still getting the same issue with this code.

- (UIView *)carousel:(iCarousel *)carousel viewForItemAtIndex:(NSInteger)index reusingView:(UIView *)view
{
    UILabel *label = nil;
    UILabel *label2 = nil;
    UILabel *titleLabel = nil;
    UIImageView *logoView = nil;
    UILabel *dateLabel = nil;
    UILabel *textLabel = nil;
    UIScrollView *scrollView = nil;
    Post *object = nil;
    UIButton *linkButton = nil;
//    NSLog(@"%@", self.array);

    NSLog(@"Current Index Selected: %ld / View: %@", index, view);

    if (view == nil)
    {
        if (carousel == self.feedPicker) {
            view = [[UIView alloc] initWithFrame:CGRectMake(0, 50, 250, 400)];
            view.backgroundColor = [UIColor whiteColor];

            titleLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, 30, 225, 22)];
            titleLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:19];
            titleLabel.numberOfLines = 2;
            [titleLabel setLineBreakMode:NSLineBreakByWordWrapping];
            [view addSubview:titleLabel];

            logoView = [[UIImageView alloc] initWithFrame:CGRectMake(180, -25, 50, 50)];
            logoView.backgroundColor = [UIColor orangeColor];
            [view addSubview:logoView];

            dateLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, titleLabel.frame.size.height+25, 225, 22)];
            dateLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:10];

            [view addSubview:dateLabel];

            scrollView = [[UIScrollView alloc] initWithFrame:CGRectMake(15, dateLabel.frame.origin.y+25, 225, 240)];

            [view addSubview:scrollView];

            textLabel = [[UILabel alloc] initWithFrame:CGRectMake(0, 0, 225, 200)];
            textLabel.font = [UIFont fontWithName:@"Roboto-Light" size:12];
            textLabel.numberOfLines = 0;

            [scrollView addSubview:textLabel];

//            NSLog(@"%@", object.acf.news_article_summary);
//            [view addSubview:textLabel];

            CGSize maxSize = CGSizeMake(textLabel.frame.size.width, CGFLOAT_MAX);
            CGSize labelSize = [textLabel sizeThatFits:maxSize];
            scrollView.contentSize = labelSize;

            linkButton = [[UIButton alloc] initWithFrame:CGRectMake(15, view.bounds.size.height-50, 100, 35)];
            linkButton.backgroundColor = [UIColor orangeColor];
            [linkButton setTitle:@"FULL TEXT" forState:UIControlStateNormal];
            linkButton.titleLabel.font = [UIFont fontWithName:@"Roboto-Light" size:14];
            [linkButton addTarget:self action:@selector(loadArticle:) forControlEvents:UIControlEventTouchUpInside];
            [view addSubview:linkButton];
        } else {
            view = [[UIView alloc] initWithFrame:[self sizeForCarousel:carousel]];
            view.clipsToBounds = YES;
            view.backgroundColor = [UIColor clearColor];
            view.contentMode = UIViewContentModeCenter;
            label = [[UILabel alloc] initWithFrame:(self.monthYearPicker == carousel ? view.bounds : view.bounds)];
            label.backgroundColor = [UIColor clearColor];
            label.textColor = [UIColor whiteColor];
            label.textAlignment = NSTextAlignmentCenter;
            label.numberOfLines = 1;
            label.font = [UIFont fontWithName:[self fontForCarousel:carousel] size:[self fontSizeForCarousel:carousel]];

            [view addSubview:label];

            if (carousel == self.monthYearPicker) {
                label2 = [[UILabel alloc] initWithFrame:CGRectMake(0, 35, view.bounds.size.width, view.bounds.size.height/2)];
                label2.backgroundColor = [UIColor clearColor];
                label2.textColor = [self colorFromHexString:@"#9fabdc"];
                label2.textAlignment = NSTextAlignmentCenter;
                label2.numberOfLines = 1;
                [view addSubview:label2];
                label2.font = [UIFont fontWithName:[self fontForCarousel:carousel] size:[self fontSizeForCarousel:carousel]-4];
            }
        }
    } else {
        if ([[view subviews] count] > 1) {
            label = [[view subviews] objectAtIndex:0];
            label2 = [[view subviews] objectAtIndex:1];
        } else {
            label = [[view subviews] lastObject];
        }
    }

    if (carousel == monthYearPicker) {
        NSArray *parts = [[self.pickerViewArray objectAtIndex:index] componentsSeparatedByString:@" "];
        label.text = [parts objectAtIndex:0];
        label2.text = [parts objectAtIndex:1];
    } else if (carousel == dayPicker) {
        label.text = [self.pickerDayViewArray objectAtIndex:index];
    } else {
        if (!object) {
            object =  self.array[index];
        }
        NSLog(@"Current Index Selected: %ld", index);
        NSLog(@"%@", object.title);
        titleLabel.text = [[object.title stringByStrippingHTML] xmlSimpleUnescapeString];
        [titleLabel sizeToFit];

        NSURL *imageURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@", object.acf.news_source_logo]];

        [logoView sd_setImageWithURL:imageURL completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, NSURL *imageURL) {
            logoView.image = [image resizedImageByMagick:@"50x50#"];
            CALayer *layer = [logoView layer];

            [layer setMasksToBounds:YES];
            [layer setCornerRadius:25];
        }];

        NSDateFormatter *f = [[NSDateFormatter alloc] init];

        [f setDateFormat:@"yyyy-MM-dd'T'HH:mm:ssz"];

        NSDate *testDate = [f dateFromString:object.date];
        [f setDateFormat:@"MMMM dd, yyyy"];
        //            [self setSubtitleForCell:cell item:[f stringFromDate:testDate]];

        CGRect dateFrame = dateLabel.frame;
        linkButton.tag = index;
        dateFrame.origin.y = titleLabel.frame.size.height+25;
        [dateLabel setFrame:dateFrame];
        dateLabel.text = [f stringFromDate:testDate];

        CGRect textFrame = scrollView.frame;

        textFrame.origin.y = dateLabel.frame.origin.y+25;
        [scrollView setFrame:textFrame];

        textLabel.text = [[object.acf.news_article_summary stringByStrippingHTML] xmlSimpleUnescapeString];
        [textLabel sizeToFit];

    }

    return view;
}

I've moved out what you said, and its still not functioning as it should.

jnpetersen commented 9 years ago

@nicklockwood : Here is a link to a video I just took. You'll notice that if I scroll to the right and come back, the first 1 isn't the same as when I opened it. Also, you'll notice that there are reoccurring ones. Any help would be appreciated.

https://www.dropbox.com/s/bq7u70g09lob41u/2015-04-20_20-14-13.mp4?dl=0

jnpetersen commented 9 years ago

Index 5 and 9 seem to repeat 0 each time. I'm not sure what's going on. It's quite frustrating. I'm attaching another video that shows what indexes are in the carousel, so you can see it doesn't keep order either.

https://www.dropbox.com/s/4h1iaufbmqxqgfx/2015-04-20_22-07-39.mp4?dl=0

nicklockwood commented 9 years ago

There still seem to be some bugs in your code - you're only setting label and label2 each time the view is updated - all of these variables are only set the first time the view is created:

UILabel *titleLabel = nil;
UIImageView *logoView = nil;
UILabel *dateLabel = nil;
UILabel *textLabel = nil;
UIScrollView *scrollView = nil;

So on subsequent re-use (when view != nil), these variables will be nil and you're therefore not actually updating their values at all.

nicklockwood commented 9 years ago

Although it's not necessary to fix your problem, you might want to consider creating subclasses for each view type so that the logic easier to understand. For example:

@interface FeedPickerView : UIView

@property (nonatomic, strong) UILabel *titleLabel;
@property (nonatomic, strong) UIImageView *logoView;
@property (nonatomic, strong) UILabel *dateLabe;
@property (nonatomic, strong) UILabel *textLabel;
@property (nonatomic, strong) UIScrollView *scrollView;

- (void)configureWithPost:(Post)post index:(NSInteger)index;

@end

@implementation FeedPickerView

- (id)init
{
    if ((self = [super initWithFrame:CGRectMake(0, 50, 250, 400)])) {
        self.backgroundColor = [UIColor whiteColor];

        self.titleLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, 30, 225, 22)];
        self.titleLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:19];
        self.titleLabel.numberOfLines = 2;
        [self.titleLabel setLineBreakMode:NSLineBreakByWordWrapping];
        [self addSubview:self.titleLabel];

        self.logoView = [[UIImageView alloc] initWithFrame:CGRectMake(180, -25, 50, 50)];
        self.logoView.backgroundColor = [UIColor orangeColor];
        [self addSubview:self.logoView];

        self.dateLabel = [[UILabel alloc] initWithFrame:CGRectMake(15, titleLabel.frame.size.height+25, 225, 22)];
        self.dateLabel.font = [UIFont fontWithName:@"Roboto-Regular" size:10];

        [self addSubview:self.dateLabel];

        self.scrollView = [[UIScrollView alloc] initWithFrame:CGRectMake(15, dateLabel.frame.origin.y+25, 225, 240)];

        [self addSubview:self.scrollView];

        self.textLabel = [[UILabel alloc] initWithFrame:CGRectMake(0, 0, 225, 200)];
        self.textLabel.font = [UIFont fontWithName:@"Roboto-Light" size:12];
        self.textLabel.numberOfLines = 0;

        [self.scrollView addSubview:self.textLabel];

        CGSize maxSize = CGSizeMake(textLabel.frame.size.width, CGFLOAT_MAX);
        CGSize labelSize = [textLabel sizeThatFits:maxSize];
        self.scrollView.contentSize = labelSize;

        self.linkButton = [[UIButton alloc] initWithFrame:CGRectMake(15, view.bounds.size.height-50, 100, 35)];
        self.linkButton.backgroundColor = [UIColor orangeColor];
        [self.linkButton setTitle:@"FULL TEXT" forState:UIControlStateNormal];
        self.linkButton.titleLabel.font = [UIFont fontWithName:@"Roboto-Light" size:14];
        [self.linkButton addTarget:self action:@selector(loadArticle:) forControlEvents:UIControlEventTouchUpInside];
        [self addSubview:self.linkButton];
    }
    return self;
}

- (void)configureWithPost:(Post)object index:(NSInteger)index
{
    self.titleLabel.text = [[object.title stringByStrippingHTML] xmlSimpleUnescapeString];
    [self.titleLabel sizeToFit];

    NSURL *imageURL = [NSURL URLWithString:[NSString stringWithFormat:@"%@", object.acf.news_source_logo]];

    [self.logoView sd_setImageWithURL:imageURL completed:^(UIImage *image, NSError *error, SDImageCacheType cacheType, NSURL *imageURL) {
        logoView.image = [image resizedImageByMagick:@"50x50#"];
        CALayer *layer = [logoView layer];

        [layer setMasksToBounds:YES];
        [layer setCornerRadius:25];
    }];

    NSDateFormatter *f = [[NSDateFormatter alloc] init];

    [f setDateFormat:@"yyyy-MM-dd'T'HH:mm:ssz"];

    NSDate *testDate = [f dateFromString:object.date];
    [f setDateFormat:@"MMMM dd, yyyy"];

    CGRect dateFrame = self.dateLabel.frame;
    linkButton.tag = index;
    self.dateFrame.origin.y = self.titleLabel.frame.size.height+25;
    [self.dateLabel setFrame:dateFrame];
    self.dateLabel.text = [f stringFromDate:testDate];

    CGRect textFrame = self.scrollView.frame;

    self.textFrame.origin.y = self.dateLabel.frame.origin.y+25;
    [self.scrollView setFrame:self.textFrame];

    self.textLabel.text = [[object.acf.news_article_summary stringByStrippingHTML] xmlSimpleUnescapeString];
    [self.textLabel sizeToFit];
}

@end
nicklockwood commented 9 years ago

Then your carousel logic would just be:

 - (UIView *)carousel:(iCarousel *)carousel viewForItemAtIndex:(NSInteger)index reusingView:(UIView *)view
 {
     Post *object = self.array[index];

     if (carousel == self.feedPicker) {
         FeedPickerView *feedPickerView = (FeedPickerView *)view;
         if (feedPickerView == nil) {
             feedPickerView = [[FeedPickerView alloc] init];
         }
         [feedPickerView configureWithPost:object index:index];
         return feedPickerView;
     } else {
         OtherKindOfView *otherKindOfView = (OtherKindOfView *)view;
         if (otherKindOfView == nil) {
             otherKindOfView = [[OtherKindOfView alloc] init];
         }
         [otherKindOfView configureWithPost:object index:index]; 
     }
 }

Which is a bit easier to read and less likely to go wrong. Just a suggestion.

jnpetersen commented 9 years ago

It was more refactored, but I started moving everything in so I could see if I was missing anything. Thanks for the suggestion, I like yours better than mine :)

jnpetersen commented 9 years ago

Woohoo!!! If I could give you a high five I would! This example ended up leading me on the right path and now it is working as expected! Thanks! @nicklockwood I really appreciate your help!

Mozahler commented 8 years ago

Thanks Nick for taking the time to explain this. I learned this when I started using your code ages ago, but had forgotten when I came back a year later. It's clear as day once again!