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 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 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); > > > >> + 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? >