bug#73384: [PATCH] Draw coloured stipples on NS

[Arash Esbati <arash@gnu.org>]

Many thanks for your comments Alan.  Since I'm only the messenger here,
I'm kindly asking Ben if he can incorporate your comments and post a new
patch.

Reg. your question:

> I take it this doesn't require the addition of any extra build flags
> to bring in CoreGraphics?

I don't think so, the patch just worked for me.

Best, Arash

Alan Third <alan@idiocy.org> writes:

>> +#ifdef NS_IMPL_COCOA
>> +/* Returns a cached CGImageMask of the stipple pattern */
>> +- (CGImageRef)stippleMask
>> +{
>> +  if (stippleMask == nil) {
>> +    CGDataProviderRef provider = CGDataProviderCreateWithData (NULL, [bmRep bitmapData],
>> +                                                             [self sizeInBytes], NULL);
>> +    CGImageRef mask = CGImageMaskCreate(
>> +                                        [self size].width,
>> +                                        [self size].height,
>> +                                        8, 8, [self size].width,
>> +                                        provider, NULL, 0);
>
> There's some weird formatting in this patch. Some of it looks like
> it's perhaps due to email, but other bits, like the above, just look
> wrong.
>
> Other things I've noticed include C++ comments, //, instead of C
> comments, /* */. Large blocks of code with no whitespace that is a bit
> hard to follow. It would be nicer if it was broken up into logical
> blocks.
>
>
>> +    r = NSMakeRect (s->x, s->y + box_line_width,
>> +        s->background_width,
>> +        s->height - 2 * box_line_width);
> <snip>
>> +    NSRectFill (r);
>> +    s->background_filled_p = 1;
>> +    CGImageRef mask = [dpyinfo->bitmaps[face->stipple - 1].img stippleMask];
>> +    CGRect bounds = CGRectMake (s->x, s->y + box_line_width,
>> +        s->background_width,
>> +        s->height - 2 * box_line_width);
>
> NSRect and CGRect are the same thing, so here "r" and "bounds" are
> identical. It might be worth just having one variable.
>
>> +	  else if (s->stippled_p) {
>
> Opening braces go on new lines.
>
> Really that's it, Just some polishing required and a proper commit
> message. Otherwise it looks OK to me.
>
> I take it this doesn't require the addition of any extra build flags
> to bring in CoreGraphics?