Issue with Singleton pattern through GCD

Question

QUI am using GCD to provide my app with a singleton class for use as a messaging / alert system. The singleton contains a method called addText, which adds the message to an NSMutable array, then displays each message in the array to the user.

The header is as follows:

#import <UIKit/UIKit.h>

@interface Banner : UIView

+ (Banner*) sharedBanner;
- (id) initWithWindow:(UIWindow*)window;
- (void) addText:(NSString*)bannerText;
- (void) callNext;

@end

The implementation is as follows:

#import "Banner.h"

static Banner* sharedBanner=nil;
static dispatch_queue_t serialQueue;

@implementation Banner
{
    @private
    UILabel* bannerLabel;
    NSMutableArray* textStrings;
    BOOL triggered;
    int counter;

    NSTimer* timer;
    UIButton* bannerButton;

    CGRect defaultFrame;
}

+ (id)allocWithZone:(NSZone *)zone {
    static dispatch_once_t onceQueue;

    dispatch_once(&onceQueue, ^{
        serialQueue = dispatch_queue_create("MyQueue.BannerQueue", NULL);
        if(sharedBanner == nil)
        {
            sharedBanner = [super allocWithZone:zone];
        }
    });
    return sharedBanner;
}


+ (Banner*) sharedBanner
{
    static dispatch_once_t onceQueue;
    dispatch_once(&onceQueue, ^{
        sharedBanner = [[Banner alloc] init];
    });
    return sharedBanner;
}

- (id) initWithWindow:(UIWindow*)window
{
    UIView* __block obj;

    dispatch_sync(serialQueue, ^
    {
        obj = [super initWithFrame:CGRectMake(0, 20, window.bounds.size.width, 0)];

        if (obj)
        {
            [self setBackgroundColor:[UIColor clearColor]];
            defaultFrame = self.frame;      
            timer = nil;
            counter = 0;
            triggered = NO;
            bannerLabel = [[UILabel alloc] initWithFrame:obj.bounds];
            textStrings = [[NSMutableArray alloc] init];  
            [bannerLabel setNumberOfLines:0];
            [bannerLabel setLineBreakMode:UILineBreakModeWordWrap];
            [bannerLabel setTextAlignment:UITextAlignmentCenter];
            [bannerLabel setBackgroundColor:[UIColor blackColor]];
            [bannerLabel setTextColor:[UIColor whiteColor]];
            [bannerLabel setAlpha:0.55];
            [self addSubview:bannerLabel];

            bannerButton = [[UIButton alloc] initWithFrame:self.bounds];
            [bannerButton addTarget:self action:@selector(complete)    forControlEvents:UIControlEventTouchUpInside];
            [bannerButton setBackgroundColor:[UIColor clearColor]];
            [bannerButton setEnabled:NO];
            [self addSubview:bannerButton];
    [window addSubview:self];
            [window bringSubviewToFront:self];
    }
    });
    self=(Banner*)obj;
    return self;
}


- (void) trigger
{
triggered = YES;
if ([textStrings count] > 0)
{       
    [bannerLabel setText:[textStrings objectAtIndex:0]];
    [bannerLabel sizeToFit];
    [self setFrame:CGRectMake(0, self.window.bounds.size.height - bannerLabel.frame.size.height, self.window.bounds.size.width, bannerLabel.frame.size.height)];
    [bannerLabel setFrame:self.bounds];
    [bannerButton setFrame:self.bounds];

    [bannerButton setEnabled:YES];
    [UIView animateWithDuration:1
                     animations:^
     {
         [bannerLabel setAlpha:1];
     }
                     completion:^(BOOL finished)
     {
         if (finished)
         {
             timer = [NSTimer scheduledTimerWithTimeInterval:4
                                                       target:self
                                                     selector:@selector(fade)
                                                     userInfo:nil
                                                      repeats:NO];
         }
     }];
}
else
{
    triggered = NO;
}
}

- (void) fade
{
[bannerButton setEnabled:NO];
if (timer)
{
timer = nil;
}
    [UIView animateWithDuration:1
                 animations:^
     {
     [bannerLabel setAlpha:0];
     }
                 completion:^(BOOL finished)
     {
     [self setFrame:defaultFrame];
     [bannerLabel setFrame:defaultFrame];
     [bannerButton setFrame:defaultFrame];

     DebugLog(@"BANNER_textStrings: %@", [textStrings objectAtIndex:0]);
     [textStrings removeObjectAtIndex:0];
     [self trigger];
 }];    
}

- (void) addText:(NSString*)bannerText
{
    dispatch_sync(serialQueue, ^
    {
        [textStrings addObject:bannerText];

         if (!triggered)
        {
            [self trigger];
        }
    });
}

    - (void) callNext
{
    counter++;
    if (!triggered)
    {
    [self trigger];
    }   
}

@end

The problem is, multiple messages are being displayed simultaneously, and when they fade out, the app crashes at the "removeItemAtIndex:0" line in the fade method.

Can anyone shed any light on what i have done wrong?

Answer 1

How do you create an instance of this Singleton? +allocWithZone then -initWithWindow ? Then call +sharedBanner to get the shared instance? ..but +sharedBanner inits a new instance? .. Why have +allocWithZone ? Are you trying to only alloc one but call init or initWithWindow multiple times? You can only init an object once.. Despite the weird alloc / init stuff this is useless as a Singleton if you have to call one method to create it and one to get the shared instance. Did you mean to call -initWithWindow from +sharedBanner but then realized you would always have to call sharedBanner with a window, which defeats the point of.. but no that still wouldn't explain why you override +allocWithZone... I can't honestly work out how many instances you are creating, how many times you are initing those you do make. I'm certain it isn't thread safe.

I guess the use of the serialQueue in -initWithWindow fits into the awful Singleton somewhere but using a private queue means you don't know which thread it will run on and all the GUI stuff you do there has to be done on the main thread. Then you assign the result of [super initWithFrame] to obj but then call methods on self. Later obj is assigned to self. Is this deliberate?

-trigger does some unsafe stuff with GUI objects on the serial queue (except for when you call it from -callNext, which isn't thread safe) and then sets up a timer which i have no idea how you are getting to work without a runloop.

You have absolutely no need here for a Singleton or any of the GCD stuff. It is doing you great harm and even if you could get it working you are not doing anything here that can benefit from it. You are approaching something that is simple, fast and conventional and making it slower, complicated and buggy, and well, unconventional. Don't override allocWithZone, don't have a sharedBanner, don't have a serialQueue and don't dispatch anything, once, sync or async (in this code).

In your ViewController have an instance variable for a Banner. Create one vanilla instance of banner and send it -addText: messages when you want to show an alert. If you need to show an alert from somewhere without access to the view controller or window, like a model, you should rethink your design and pass error messages back up the call chain rather using a Singleton and adding a dependency to a UIView Subclass. Follow standard convention when writing -init methods. It's always the same and will become automatic. Use a nib for laying out and configuring your views. Don't have the view add itself to the window or bring the window to the front. These are jobs for your ViewController.

Do it all on the main thread and only interact with it from the main thread. If you need to use serialQueues for locking do it at a level before you interact with the GUI.