[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: imagemmagick patch 5

From: Stefan Monnier
Subject: Re: imagemmagick patch 5
Date: Fri, 05 Mar 2010 17:30:24 -0500
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.91 (gnu/linux)

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

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

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


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);
+  DEFVAR_LISP ("imagemagick-render-type", &Vimagemagick_render_type,
+    doc: /*   */);
 init_image ()
 /* arch-tag: 123c2a5e-14a8-4c53-ab95-af47d7db49b9
    (do not change this comment) */

reply via email to

[Prev in Thread] Current Thread [Next in Thread]