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

From: Ben Simms <[email-masked]>
Date: Sun, 5 Jan 2025 20:17:42 +0100

Hi all, I've revised my patch from Alan's feedback.
You can find it attached.

On Sat, 4 Jan 2025 at 09:49, Arash Esbati <[email-masked]> wrote:

> 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 <[email-masked]> 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?
>