Re: imagemmagick patch 5

[joakim]

address@hidden writes:

> address@hidden writes:
>
>> Stefan Monnier <address@hidden> 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. 
>
> I've done a local bzr branch now, called "imagemagick". I would now like
> to publish this branch.
>
> http://www.emacswiki.org/emacs/BzrForEmacsDevsmentions ways of publishing a 
> branch to launchpad etc, but I cant find
> how to publish it on savannah. Could some kind soul enlighten me?


I tried this, which seemingly works:

bzr push sftp://address@hidden/srv/bzr/emacs/imagemagick



>
>
>
>> 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