Re: imagemmagick patch 5

[Stefan Monnier]

> 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 don't have much experience with such features, so I can't really
usefully comment on them (in mpc.el I do scaling (externally, obviously)
for album-covers, and indeed, I do it by specifying the target size, so
it looks acceptable).

But even if it's not 100% resolved, it can be installed, since it's the
branch for Emacs-24, so there'll be time to adjust the API.

> 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.  There's nothing completely X specific in the patch I think, just
> the same type of bitmap manipulations that goes on for the other
> image libraries.

So is the test in configure.in necessary?

>> Please don't use the P_ macro in new code: just use prototypes.
> Ok.  Like this?
> static int imagemagick_image_p (Lisp_Object object);

Yes.  Similarly the function definition shouldn't use K&R style any more
(we didn't change all the existing code, but we try and do that for new
code).

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

It's no big problem.  Now that we accept and use prototypes, there
probably aren't any compiler that can compile Emacs but does not accept
// comments.


        Stefan


PS: A few more things:

+  /* furthermore :rotation. we need background color and angle for rotation */

Please capitalize your comments, and use double-spaces at the end of
sentences (including at the end of a comment, where we also want
a full-stop):

  /* Furthermore :rotation.  We need background color and angle for rotation.  
*/

+    if (FLOATP (value)){

Please put all "{" on their own line.

+      PixelWand* background = NewPixelWand();

Add a space before every open paren.

+      PixelSetColor (background, "#ffffff");/*TODO remove hardcode*/

Please use something like

       PixelSetColor (background, "#ffffff"); /* TODO remove hardcode.  */

I.e. notice the "full stop plus 2 spaces".

+      status=MagickRotateImage (image_wand, background,rotation);

Please add spaces around operators like "=".
Also, please drop the "add empty lines" part of the hunk below as well.

@@ -8521,12 +8991,20 @@ When an image has not been displayed this many seconds, 
remove it
 from the image cache.  Value must be an integer or nil with nil
 meaning don't clear the cache.  */);
   Vimage_cache_eviction_delay = make_number (30 * 60);
+
+#ifdef HAVE_IMAGEMAGICK  
+  DEFVAR_LISP ("imagemagick-render-type", &Vimagemagick_render_type,
+    doc: /*   */);
+#endif    
 }
 
 void
 init_image ()
 {
+
 }
 
+
+
 /* arch-tag: 123c2a5e-14a8-4c53-ab95-af47d7db49b9
    (do not change this comment) */