Re: imagemmagick patch 5

[joakim@verona.se]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The Imagemagick patch allows Emacs to use the Imagemagick libraries to
>> load images. This support can be used in parallell with the existing
>> image loading support. There is support for asking your imagemagick
>> installation which image types it supports, and registering them in
>> Emacs selectively.
>
> Thanks.  Looks pretty good.  Feel free to install it in the `pending'
> branch, but please see the comments below first, and don't forget to add
> a good NEWS entry when you install this change.

Ok. I will need to read up on bzr before committing. 

Could you possibly also comment on the interface I chose for scaling and
rotation? If they are ok, I will document them in the NEWS entry. I
assume the "index" spec is ok since that is also used for gifs already.

Also please comment if the interface to select which image types
imagemagick gets to handle is ok.

>
>> +HAVE_IMAGEMAGICK=no
>> +if test "${HAVE_X11}" = "yes" ; then
>
> Do I understand it right that this X11-only restriction could be lifted
> at some point in the future?

Yes. Theres nothing completely X specific in the patch I think, just the
same type of bitmap manipulations that goes on for the other image
libraries.

>> diff --git a/src/dbusbind.c b/src/dbusbind.c
>> index 7c0be49..7a47730 100644
>> --- a/src/dbusbind.c
>> +++ b/src/dbusbind.c
>> @@ -773,6 +773,7 @@ xd_add_watch (watch, data)
>>        if (fd == -1)
>>  	return FALSE;
>>  
>> +
>>        /* Add the file descriptor to input_wait_mask.  */
>>        add_keyboard_wait_descriptor (fd);
>>      }
>
> Please drop this gratuitous change.

Hmm, I have no idea how this crept in. Odd.


>> +/***********************************************************************
>> +				 imagemagick
>> + ***********************************************************************/
>> +#if defined (HAVE_IMAGEMAGICK)
>> +Lisp_Object Vimagemagick_render_type;
>> +/* Function prototypes.  */
>> +
>> +static int imagemagick_image_p P_ ((Lisp_Object object));
>> +static int imagemagick_load P_ ((struct frame *f, struct image *img));
>> +
>> +static int imagemagick_load_image P_ ((struct frame *, struct image *,
>> +                                       unsigned char *, unsigned int, unsigned char *));
>
> Please don't use the P_ macro in new code: just use prototypes.

Ok. Like this?

static int imagemagick_image_p (Lisp_Object object);
static int imagemagick_load (struct frame *f, struct image *img);

static int imagemagick_load_image (struct frame *, struct image *,
                                       unsigned char *, unsigned int, unsigned char *);


>> +//#include "/home/joakim/current/unison/data/ImageMagick-6.5.4-7/magick/xwindow-private.h"
>
> This should be removed as well.

Ok.

>> +    //try if magicexportimage is any faster than pixelpushing
> [...]
>> +    //oddly, the below code doesnt seem to work:
>
> We currently only use /*...*/ comments, so I'd rather we don't introduce
> // comments for now.
>

Sorry, I keep forgetting this, it seems to be in my muscle memory. 

>         Stefan
>
-- 
Joakim Verona