CODEWITHSUNDEEP
objective-ciphone
coure2011
coure2011

Reputation: 42444

Passing UIColor to a method and app crashing

see code, interface

@interface ColorPreview : UIView {
    UIColor *backgroundColor;
}
@property (retain) UIColor *backgroundColor;

-(void) reDrawPreviewWith:(UIColor *)bgColor;

@end

implementation

@implementation ColorPreview

@synthesize backgroundColor;
- (id)initWithFrame:(CGRect)frame {

    if ((self = [super initWithFrame:frame])) {
        // Initialization code
        backgroundColor = [[UIColor alloc] init];
    }
    return self;
}

- (void)drawRect:(CGRect)rect {
    ............
    //app crashes on this line
    CGContextSetFillColorWithColor(context, [backgroundColor CGColor]);

    NSLog(@"rect");
}

-(void) reDrawPreviewWith:(UIColor *)bgColor 
{
    backgroundColor = bgColor;
    [self setNeedsDisplay];
}

- (void)dealloc {
    [backgroundColor release];
    [super dealloc];
}

@end

calling my method like this

[preview reDrawPreviewWith:[UIColor colorWithRed:red green:green blue:blue alpha:1.0]];

Upvotes: 0

Views: 879

Answers (1)

danyowdee
danyowdee

Reputation: 4698

Gary has it almost right:
The reason that causes your crash is that you are not really setting the color you obtain in reDrawPreviewWithColor: as an argument -- as a matter of fact, your reDraw... will almost never work correct: it either keeps a reference to an object it doesn't own ie. crashes on autoreleased objects (what you are seeing) or it leaks.

So here's a fix:

-(void)reDrawPreviewWithColor:(UIColor *)newColor
{
  [self setBackgroundColor: newColor];  // or use the dot-syntax if you please ;-)
  [self setNeedsDisplay];
}

Or even better scrap reDraw... altogether and instead write your own setter for backgroundColor 'cause you should redraw after updating the color anyway:

-(void)setBackgroundColor:(UIColor *)newColor
{
    if (backgroundColor == newColor) 
      return; // if they are *identical*, there's nothing to do

    [newColor retain];
    @synchronize(self)
    {
        [backgroundColor release];
        backgroundColor = newColor;
        [self setNeedsDisplay];
    }
}

Oh and while we're at it:
Is there a good reason for backgroundColor being an atomic property? You may want to consider declaring it as

@property (nonatomic, retain) UIColor *backgroundColor;

This way you could scrap the @synchronize directive from the setter I proposed.

Something important as an aside:
Gary suggested to write self.backgroundColor = [[UIColor alloc] init] in initWithFrame:.

Please don't!

You own what you alloc, so if you're trying to own it twice Ur Doin' it Wrong!™ -- but you already had this right, so I assume you are aware of this.

Upvotes: 3

Related Questions