Re: gdk_drawable_get_image() patch

[Owen Taylor <otaylor redhat com>]

Ron Steinke <rsteinke w-link net> writes:

> 	This patch fixes gdk_drawable_get_image() for bitmaps,
> mostly by copying in old code. It also adds checks to make
> sure that the depth of an image matches that of its colormap.
> This fixes bug 57604. Okay to commit?

Various comments below. Could you also get together with Matthias 
Clasen and come up with a single patch for _gdk_x11_get_image() ...
I don't think it works to have two people rewriting the same
100 lines of code.

 (See http://bugzilla.gnome.org/show_bug.cgi?id=57608)

> Index: gtk+/gdk/gdkdraw.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/gdkdraw.c,v
> retrieving revision 1.26
> diff -u -r1.26 gdkdraw.c
> --- gtk+/gdk/gdkdraw.c	2001/06/29 01:58:58	1.26
> +++ gtk+/gdk/gdkdraw.c	2001/07/19 21:22:45
> @@ -198,6 +198,8 @@
>                             GdkColormap *cmap)
>  {
>    g_return_if_fail (GDK_IS_DRAWABLE (drawable));
> +  g_return_if_fail (cmap == NULL || gdk_drawable_get_depth (drawable)
> +                    == cmap->visual->depth);
>  
>    GDK_DRAWABLE_GET_CLASS (drawable)->set_colormap (drawable, cmap);
>  }

This part is fine.

> Index: gtk+/gdk/x11/gdkimage-x11.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/x11/gdkimage-x11.c,v
> retrieving revision 1.35
> diff -u -r1.35 gdkimage-x11.c
> --- gtk+/gdk/x11/gdkimage-x11.c	2001/06/29 01:59:00	1.35
> +++ gtk+/gdk/x11/gdkimage-x11.c	2001/07/19 21:22:45
> @@ -389,16 +389,16 @@
>  
>    visual = gdk_drawable_get_visual (drawable);
>  
> -  if (visual == NULL)
> +  if (visual == NULL && gdk_drawable_get_depth (drawable) != 1)
>      {
>        g_warning ("To get the image from a drawable, the drawable "
> -                 "must have a visual and colormap; calling "
> -                 "gtk_drawable_set_colormap() on a drawable "
> +                 "must have a visual and colormap (unless it's a bitmap); "
> +                 "calling gtk_drawable_set_colormap() on a drawable "
>                   "created without a colormap should solve this problem");
>  
>        return NULL;
>      }
>    impl = GDK_DRAWABLE_IMPL_X11 (drawable);
>    
>    have_grab = FALSE;
> @@ -436,23 +436,65 @@
>          }
>      }
>  
> -  image = gdk_image_new (GDK_IMAGE_FASTEST,
> -                         visual,
> -                         width, height);
> +  if (visual)
> +    {
> +      image = gdk_image_new (GDK_IMAGE_FASTEST,
> +                             visual,
> +                             width, height);
>  
> -  if (image == NULL)
> -    return NULL;
> +      if (image == NULL)
> +        {
> +          if (have_grab)
> +            gdk_x11_ungrab_server ();
> +          return NULL;
> +        }
>  
> -  private = PRIVATE_DATA (image);
> +      private = PRIVATE_DATA (image);
>    
> -  gdk_error_trap_push ();
> +      gdk_error_trap_push ();
>  
> -  ximage = XGetSubImage (impl->xdisplay,
> -                         impl->xid,
> -                         x, y, width, height,
> -                         AllPlanes, ZPixmap,
> -                         private->ximage,
> -                         x, y);
> +      ximage = XGetSubImage (impl->xdisplay,
> +                             impl->xid,
> +                             x, y, width, height,
> +                             AllPlanes, ZPixmap,
> +                             private->ximage,
> +                             x, y);
> +    }
> +  else /* It's a bitmap */
> +    {
> +      gdk_error_trap_push ();
> +
> +      ximage = XGetImage (impl->xdisplay,
> +                          impl->xid,
> +                          x, y, width, height,
> +                          AllPlanes, ZPixmap);
> +
> +      if (!ximage)
> +        {
> +          if (have_grab)
> +            gdk_x11_ungrab_server ();
> +          gdk_error_trap_pop ();
> +          return NULL;
> +        }
> +
> +      image = g_object_new (gdk_image_get_type (), NULL);
> +      private = PRIVATE_DATA (image);
> +
> +      private->xdisplay = gdk_display;
> +      private->ximage = ximage;
> +
> +      image->type = GDK_IMAGE_NORMAL;
> +      image->visual = NULL;
> +      image->width = width;
> +      image->height = height;
> +      image->depth = private->ximage->depth;
> +
> +      image->mem = private->ximage->data;
> +      image->bpl = private->ximage->bytes_per_line;
> +      image->bits_per_pixel = private->ximage->bits_per_pixel;
> +      image->bpp = (private->ximage->bits_per_pixel + 7) / 8;
> +      image->byte_order = private->ximage->byte_order; 
> +    }

If you are going to separate it into two code paths like this, I
think it is best two separate them by if (GDK_IS_WINDOW()) {}
rather than by whether we have the visual or not.

There are several reasons for this:

 - The code for pushing an error trap, possibly grabbing the server,
   clipping to the size of the image, etc, is all dealing with
   error conditions that only can occur for images.

 - XGetImage() is actually slightly more efficient than XGetSubImage
   (the implementation of XGetSubImage is to XGetImage to a temporary
   image, then copy into the destination image), so we'd be
   best off using XGetImage() where possible.

This also should mean that we never_need the warning... if we
have a window, we have a visual; if we don't have a window, we
don't need a visual.

> Index: gtk+/gdk/x11/gdkpixmap-x11.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/x11/gdkpixmap-x11.c,v
> retrieving revision 1.39
> diff -u -r1.39 gdkpixmap-x11.c
> --- gtk+/gdk/x11/gdkpixmap-x11.c	2000/10/25 22:34:07	1.39
> +++ gtk+/gdk/x11/gdkpixmap-x11.c	2001/07/19 21:22:46
> @@ -150,6 +150,7 @@
>    GdkDrawableImplX11 *draw_impl;
>    GdkPixmapImplX11 *pix_impl;
>    GdkColormap *cmap;
> +  gint window_depth;
>    
>    g_return_val_if_fail (window == NULL || GDK_IS_WINDOW (window), NULL);
>    g_return_val_if_fail ((window != NULL) || (depth != -1), NULL);
> @@ -161,8 +162,9 @@
>    if (GDK_WINDOW_DESTROYED (window))
>      return NULL;
>  
> +  window_depth = gdk_drawable_get_depth (GDK_DRAWABLE (window));
>    if (depth == -1)
> -    depth = gdk_drawable_get_depth (GDK_DRAWABLE (window));
> +    depth = window_depth;
>  
>    pixmap = g_object_new (gdk_pixmap_get_type (), NULL);
>    draw_impl = GDK_DRAWABLE_IMPL_X11 (GDK_PIXMAP_OBJECT (pixmap)->impl);
> @@ -179,11 +181,23 @@
>    pix_impl->height = height;
>    GDK_PIXMAP_OBJECT (pixmap)->depth = depth;
>  
> -  if (window)
> +  if (depth == window_depth)
> +    cmap = gdk_drawable_get_colormap (window);
> +  else
> +    cmap = NULL;
> +
> +  if (cmap == NULL)
> +    {
> +      GdkVisual *visual = gdk_visual_get_best_with_depth (depth);
> +      if (visual)
> +        {
> +          cmap = gdk_colormap_new (visual, FALSE);
> +        }
> +    }
> +
> +  if (cmap)
>      {
> -      cmap = gdk_drawable_get_colormap (window);
> -      if (cmap)
> -        gdk_drawable_set_colormap (pixmap, cmap);
> +      gdk_drawable_set_colormap (pixmap, cmap);
>      }
>    
>    gdk_xid_table_insert (&GDK_PIXMAP_XID (pixmap), pixmap);

I don't think trying to guess the visual here is a good idea. If they
supplied a depth and a window, and their depths don't, we should just
leave the colormap unset. The caller can always set it themselves
if they need to do that.

Regards,
                                        Owen