unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Alan Third <alan@idiocy.org>
To: Arash Esbati <arash@gnu.org>
Cc: "Rudolf Adamkovič" <rudolf@adamkovic.org>,
	73384@debbugs.gnu.org, "Stefan Kangas" <stefankangas@gmail.com>,
	"JD Smith" <jdtsmith@gmail.com>,
	"Ben Simms" <bsimms.simms@gmail.com>
Subject: bug#73384: [PATCH] Draw coloured stipples on NS
Date: Wed, 18 Dec 2024 21:55:44 +0000	[thread overview]
Message-ID: <Z2NE4CVBQH4cKCq7@faroe.holly.idiocy.org> (raw)
In-Reply-To: <m2cyhyoi5l.fsf@macmutant.fritz.box>

> +#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?
-- 
Alan Third





      parent reply	other threads:[~2024-12-18 21:55 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-20  7:57 bug#73384: [PATCH] Draw coloured stipples on NS Ben Simms
2024-09-21 11:41 ` Stefan Kangas
2024-09-21 15:08   ` Arash Esbati
2024-09-28 18:30     ` JD Smith
2024-09-28 23:49       ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-09-29 17:51         ` JD Smith
2024-09-29 20:33         ` Stefan Kangas
2024-09-29 23:31           ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-09-30  0:13             ` Stefan Kangas
2024-09-30 11:24             ` Eli Zaretskii
2024-10-08  5:03               ` Arash Esbati
2024-10-14  1:29     ` Rudolf Adamkovič
2024-10-14 18:35       ` Ben Simms
2024-10-17 10:43         ` Arash Esbati
2024-10-19  8:34           ` Ben Simms
2024-10-21 18:23             ` Arash Esbati
     [not found]               ` <CALNBX0YYHHtHh-FMyBVNz5pE8EWeQ7v9cpdr6ifZyeQGf2Ecvg@mail.gmail.com>
2024-10-21 19:26                 ` Arash Esbati
2024-10-29 10:31                   ` Arash Esbati
2024-12-11  6:20                     ` Arash Esbati
2024-12-15 14:21                       ` Stefan Kangas
2024-12-18 21:55                       ` Alan Third [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Z2NE4CVBQH4cKCq7@faroe.holly.idiocy.org \
    --to=alan@idiocy.org \
    --cc=73384@debbugs.gnu.org \
    --cc=arash@gnu.org \
    --cc=bsimms.simms@gmail.com \
    --cc=jdtsmith@gmail.com \
    --cc=rudolf@adamkovic.org \
    --cc=stefankangas@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).