Re: [PATCH] Feedback request for x_set_mouse_color on NextSTEP/MacOS

[Jan D.]

Hello.

Joe Matarazzo skrev 2014-08-12 20:32:
> Hi, I would like feedback on a patch to enable changing the text and
> non-text mouse pointer colors on MacOS, via "set-mouse-color". I'm new
> to Emacs development and objective-C, so I expect I have a sub-optimal
> and potentially leaky implementation.
>
> I chose to only modify those pointers, as my implementation is a bit
> heavy handed and changes the entire cursor image to the requested
> color. On the I-beam (text) cursor this is ok, but the (nontext) arrow
> pointer loses its thin white outline.

That would have to be fixed.

> The other cursors (the modeline
> and resize pointers) don't suffer the visibility problems the
> text/nontext pointers do when using a dark frame background, and are
> left alone.
>
> Thanks,
> Joe
>
> --------------
>
> diff --git a/src/nsfns.m b/src/nsfns.m
> index ca8f492..266ae5b 100644
> --- a/src/nsfns.m
> +++ b/src/nsfns.m
> @@ -61,11 +61,15 @@ int fns_trace_num = 1;
>   #ifdef HAVE_NS
> +// Required for set-mouse-color
> +#import <QuartzCore/CoreImage.h>
> +
>   extern NSArray *ns_send_types, *ns_return_types, *ns_drag_types;
>   extern Lisp_Object Qforeground_color;
>   extern Lisp_Object Qbackground_color;
>   extern Lisp_Object Qcursor_color;
> +extern Lisp_Object Qmouse_color;
>   extern Lisp_Object Qinternal_border_width;
>   extern Lisp_Object Qvisibility;
>   extern Lisp_Object Qcursor_type;
> @@ -878,15 +882,90 @@ x_set_cursor_type (struct frame *f, Lisp_Object
> arg, Lisp_Object oldval)
>     set_frame_cursor_types (f, arg);
>   }
> +
> +#undef Z

What is Z?

> +static void
> +changeMouseColor(NSCursor **crs, NSColor *col)

Avoid in/out parameter (crs), let the function return a NSCursor instead.

static NSCursor *
changeMouseColor(NSCursor *crs, NSColor *col)
...

> +{
> +
> +  NSPoint hs    = [*crs hotSpot];
> +  NSImage *img  = [*crs image];
> +
> +  CGContextRef myContext = [[NSGraphicsContext currentContext]
> graphicsPort];
> +  CIContext *context     = [CIContext contextWithCGContext:myContext
> options:nil];
> +
> +  // Convert to CIImage input
> +  NSRect  rect;
> +  rect.origin = NSMakePoint(0.0, 0.0);
> +  rect.size   = [img size];
> +
> +  CGImageRef cgImage = [img CGImageForProposedRect:&rect
> +                                           context:[NSGraphicsContext
> currentContext]
> +                                             hints:nil];
> +  CIImage *inputImage = [CIImage imageWithCGImage:cgImage];
> +
> +
> +  // Set up the filter path
> +  CIFilter *constColor = [CIFilter
> filterWithName:@"CIConstantColorGenerator"];
> +  CIColor  *newColor   = [CIColor colorWithCGColor:[col CGColor]];
> +  [constColor setValue:newColor forKey: kCIInputColorKey];
> +
> +  // Needs crop to be usable
> +  constColor = [CIFilter filterWithName:@"CICrop" keysAndValues:
> +                           kCIInputImageKey, [constColor valueForKey:
> kCIOutputImageKey],
> +                         @"inputRectangle", [CIVector vectorWithX:0.0f
> Y:0.0f Z:2.0f W:2.0f],
> +                         nil];
> +
> +  // Apply a constant color map to change all colors in the image
> +  CIFilter *colorMap   = [CIFilter filterWithName:@"CIColorMap"];
> +  [colorMap setValue:inputImage forKey:kCIInputImageKey];
> +  [colorMap setValue:[constColor valueForKey: kCIOutputImageKey]
> +              forKey:kCIInputGradientImageKey];
> +
> +
> +  // Render the output image
> +  CIImage      *render = [colorMap valueForKey: kCIOutputImageKey];
> +  CGRect        extent = [render extent];
> +  CGImageRef  finalImg = [context createCGImage:render fromRect:extent];
> +
> +  NSImage *resultImage = [[NSImage alloc] initWithCGImage:finalImg
> size:extent.size];
> +
> +  if (resultImage)
> +    {
> +      *crs = [[NSCursor alloc] initWithImage:resultImage hotSpot:hs];

This never gets reelased.

> +      [resultImage release];
> +    }
> +  else {
> +    error("Could not change cursor");
> +  }
> +}
> +

This whole function is very OSX specific.  It would be preferred if 
there was a common GNUStep/OSX implementation.  At the very least we 
would need a GNUStep implementation also before bringing it in Emacs.

> +
> +
>   /* called to set mouse pointer color, but all other terms use it to
>      initialize pointer types (and don't set the color ;) */
>   static void
>   x_set_mouse_color (struct frame *f, Lisp_Object arg, Lisp_Object oldval)
>   {
> -  /* don't think we can do this on Nextstep */
> +  NSColor *col;
> +  if (ns_lisp_to_color (arg, &col))
> +    {
> +      store_frame_param (f, Qmouse_color, oldval);
> +      error ("Unknown color");
> +    }
> +
> +  // Only change text and non-text cursor colors. The pointy hand and
> resize
> +  // cursors should be okay (ie. easily visible) in their appropriate
> window
> +  // locations.
> +  if (FRAME_CURSOR(f, text))    changeMouseColor(&FRAME_CURSOR(f,
> text), col);
> +  if (FRAME_CURSOR(f, nontext)) changeMouseColor(&FRAME_CURSOR(f,
> nontext), col);
> +
> +  store_frame_param(f, Qmouse_color, arg);
>   }
> +
> +
>   #define Str(x) #x
>   #define Xstr(x) Str(x)
> diff --git a/src/nsterm.h b/src/nsterm.h
> index 00a0b54..7c26259 100644
> --- a/src/nsterm.h
> +++ b/src/nsterm.h
> @@ -736,9 +736,10 @@ struct x_output
>   #define FRAME_DEFAULT_FACE(f) FACE_FROM_ID (f, DEFAULT_FACE_ID)
> -#define FRAME_NS_VIEW(f) ((f)->output_data.ns->view)
> +#define FRAME_NS_VIEW(f)      ((f)->output_data.ns->view)

Do not make whitespace only changes.

>   #define FRAME_CURSOR_COLOR(f) ((f)->output_data.ns->cursor_color)
>   #define FRAME_POINTER_TYPE(f) ((f)->output_data.ns->current_pointer)
> +#define FRAME_CURSOR(f, t)    ((f)->output_data.ns->t##_cursor)
>   #define FRAME_FONT(f) ((f)->output_data.ns->font)

        Jan D.