emacs-devel
[Top][All Lists]
Advanced

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

Re: background colors for images


From: Gerd Moellmann
Subject: Re: background colors for images
Date: 24 Oct 2001 18:44:11 +0200
User-agent: Gnus/5.09 (Gnus v5.9.0) Emacs/21.1.50

Miles Bader <address@hidden> writes:

> Index: src/dispextern.h
> ===================================================================
> RCS file: /cvs/emacs/src/dispextern.h,v
> retrieving revision 1.113
> diff -u -r1.113 dispextern.h
> --- src/dispextern.h  2001/10/24 09:10:57     1.113
> +++ src/dispextern.h  2001/10/24 14:55:04
> @@ -1985,6 +1985,20 @@
>    unsigned long *colors;
>    int ncolors;
>  
> +  /* A single `background color' for this image, for the use of anyone that
> +     cares about such a thing.  Only valid if the `background_valid' field
> +     is true.  This should generally be accessed by calling the accessor
> +     macro `IMAGE_BACKGROUND', which will heuristically calculate a value
> +     if necessary.  */
> +  unsigned long background;
> +  /* True if this image has a `transparent' background -- that is, is
> +     uses an image mask.  The accessor macro for this is
> +     `IMAGE_BACKGROUND_TRANSPARENT'.  */
> +  unsigned background_transparent : 1;
> +  /* True if the `background' and `background_transparent' fields are
> +     valid, respectively. */
> +  unsigned background_valid : 1, background_transparent_valid : 1;
> +

I shouldn't be saying this in my situation, but anyway :-): Could you
please leave a blank line in front of the comments?  I'm personally
convinced that common coding conventions are a good thing to avoid
code ownership problems, and when I talked initially with Richard
about what coding conventions Emacs uses (should use it probably more
correct), be said that he'd like to have the comments formatted as

  |  /* lkajsldkjasldj */
  |  code code code

and 

  |/* ljsalkdjlaskjdlaskjdl  */
  |
  |code 

when the comment is a toplevel comment, i.e. starts in column 0.

I know that's probably nowhere documented, but maybe it would be good
idea to write something down for the future...

[...]

> +
> +/* Returns the background color of IMG, calculating one heuristically if
> +   necessary.  If non-zero, XIMG is an existing XImage object to use for
> +   the heuristic.  */

That would be a case where there would be blank line after the comment.

> +#define IMAGE_BACKGROUND(img, f, ximg)                                       
>       \
> +   ((img)->background_valid                                                \
> +    ? (img)->background                                                      
>       \
> +    : image_background (img, f, ximg))
> +
> +/* Returns true if IMG has a `transparent' background, using heuristics
> +   to decide if necessary.  If non-zero, MASK is an existing XImage
> +   object to use for the heuristic.  */
> +#define IMAGE_BACKGROUND_TRANSPARENT(img, f, mask)                         \
> +   ((img)->background_transparent_valid                                      
>       \
> +    ? (img)->background_transparent                                        \
> +    : image_background_transparent (img, f, mask))
>  

[...]

> +/* Return the `background' field of IMG.  If IMG doesn't have one yet,
> +   it is guessed heuristically.  If non-zero, XIMG is an existing XImage
> +   object to use for the heuristic.  */
> +unsigned long
> +image_background (img, f, ximg)
> +     struct image *img;
> +     struct frame *f;
> +     XImage *ximg;
> +{
> +  if (! img->background_valid)
> +    /* IMG doesn't have a background yet, try to guess a reasonable value.  
> */

Another comment thing would be to write these comments in front of the
if.

> +    {
> +      int free_ximg = !ximg;
> +
> +      if (! ximg)
> +     ximg = XGetImage (FRAME_X_DISPLAY (f), img->pixmap,
> +                       0, 0, img->width, img->height, ~0, ZPixmap);

(I don't know if that's easy to do, but it might be worth trying to
avoid XGetImage in the image code where possible because that function
is pretty slow (pixmaps are stored on the server and getting an image
might mean transfering them to the client.)  Maybe one can remember
the four corner colors when the pixmaps are built.  Or maybe one
should use the XIE extension [present in XFree] which contains some
handy features like compressed image transfer.  [This does not have
something to do with this code, I just thought I mention this blurb].)

> +
> +      img->background = four_corners_best (ximg, img->width, img->height);

[...]

The rest of the code looks fine, but I'd anyway check that the color
reference counts are right.  Please see the code with in #if
DEBUG_X_COLORS in xfaces.c.



reply via email to

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