Re: GdkColormap -> GObject

[Tim Janik <timj gtk org>]

On 15 May 2000, Havoc Pennington wrote:

> 
> Hi,
> 
> Started with the simplest GDK type needing GObject-ification.
> Here's the patch; please comment, and I'll move on to the other types.

thanks, that efford is much apprechiated.

> Issues - 
> 
>   - no object args added, since that stuff seems to be the subject
>     of controversy still. This means that gdk_x_colormap_init()
>     doesn't really do what it should do, and gdk_colormap_new() 
>     or gdk_colormap_get_system() must be used to create colormaps.
>     Once this stuff settles, default construction can be added.
> 
>  - It might seem more intuitive to have a GdkColormap type, 
>    with GdkXColormap as a subclass; however in practice I think 
>    it works out better to avoid introducing a type in 
>    cross-platform code (i.e. only GdkXColormap really exists, 
>    and there's a fake GdkColormap type which is user-visible).

ok, lemme outline some basic type system issues here:
first, GObject and GType are still experimental code, that is,
the parameter stuff is most likely to change, and other stuff will
definitely change soon, e.g. the GTypeFundamentalInfo structure.
second, GType is currently _not_ thread safe, so don't use CVS
gtk with threaded applications (that's soon going to be solved though).
third, to use glib, you should:
#include <glib.h>
for gmodule, you'd
#include <gmodule.h>
for GType/GObject, you have to:
#include <glib-object.h>
(not gobject/gobject.h)
fourth, reference documentation is not yet out there, but under way ;)


then some notes on general object creation:

- we don't cast GObject[Class] derived types directly at all, so always
  use the GTK_WIDGET(), GDK_COLORMAP() etc. casts that are in place.
  for stable releases, these macros will evaluate to plain casts, while
  for development extra type and pointer checks are performed.

- all object derivatives have to implement the following functions/macros:
  g_<MyObject>_get_type ();
  G_TYPE_<MyObject>
  G_<MyObject>(object)
  G_<MyObject>_CLASS(class)
  G_IS_<MyObject>(object)
  G_IS_<MyObject>_CLASS(class)
  G_<MyObject>_GET_CLASS(object)
  where G_ has to be replaced by the corresponding prefix of the
  implementing entity, and <MyObject> has to be replaced with the derivatives
  name. to ease macro implementations, appropriate wrappers are provided by
  gtk/glib, as an example, here're the macros introduced by GtkLabel, using
  the Gtk provided wrappers:
#define GTK_TYPE_LABEL            (gtk_label_get_type ())
#define GTK_LABEL(obj)            (GTK_CHECK_CAST ((obj), GTK_TYPE_LABEL, GtkLabel))
#define GTK_LABEL_CLASS(klass)    (GTK_CHECK_CLASS_CAST ((klass), GTK_TYPE_LABEL, GtkLabelClass))
#define GTK_IS_LABEL(obj)         (GTK_CHECK_TYPE ((obj), GTK_TYPE_LABEL))
#define GTK_IS_LABEL_CLASS(klass) (GTK_CHECK_CLASS_TYPE ((klass), GTK_TYPE_LABEL))
#define GTK_LABEL_GET_CLASS(obj)  (GTK_CHECK_GET_CLASS ((obj), GTK_TYPE_LABEL, GtkLabelClass))
  and the ones for GdkXColormap, based on glib's native type system macros:
#define	GDK_TYPE_XCOLORMAP		(gdk_xcolormap_get_type ())
#define GDK_XCOLORMAP(object)		(G_TYPE_CHECK_INSTANCE_CAST ((object), GDK_TYPE_XCOLORMAP, GdkXColormap))
#define GDK_XCOLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_CAST ((klass), GDK_TYPE_XCOLORMAP, GdkXColormapClass))
#define GDK_IS_XCOLORMAP(object)	(G_TYPE_CHECK_INSTANCE_TYPE ((object), GDK_TYPE_XCOLORMAP))
#define GDK_IS_XCOLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_TYPE ((klass), GDK_TYPE_XCOLORMAP))
#define GDK_XCOLORMAP_GET_CLASS(obj)	(G_TYPE_INSTANCE_GET_CLASS ((obj), GDK_TYPE_XCOLORMAP, GdkXColormap))

- classes are reference counted by the type system internally already, that
  implies that parent classes are always alive while derived classes exist,
  so class_init() and method implementation code should actually look like this:
static void g_<MyObject>_finalize (GObject *gobject);
static gpointer parent_class = NULL;
static void
g_<MyObject>_class_init (G<MyObject>Class *class)
{
  GObjectClass *gobject_class = G_OBJECT_CLASS (class);
  
  parent_class = g_type_class_peek (G_TYPE_OBJECT);
  
  gobject_class->finalize = g_<MyObject>_finalize;
  
  /* ... */
}
static void
g_<MyObject>_finalize (GObject *gobject)
{
  G<MyObject> *mob = G_<MyObject> (gobject);
  
  /* ... */
  
  G_OBJECT_CLASS (parent_class)->finalize (gobejct);
}
  note, that static "parent_class" is of type gpointer, this forces
  class casts for method invokation which can catch lots of common errors,
  especially when parent types have been changed. class_init uses the class
  cast macros to cast "class" into a parent class pointer for the same reason.
  initialization of "parent_class" is done with g_type_class_peek(), since
  as stated before, the parent class is guarranteed to stay around for the
  lifetime of derived classes.

- the G_IS_<MyObject>() and G_IS_<MyObject>_CLASS() style macros handle NULL
  pointers fine, so assertment code like:
{  
  g_return_if_fail (label != NULL);
  g_return_if_fail (GTK_IS_LABEL (label));
}
  is simply redundant and should be reduced to:
{  
  g_return_if_fail (GTK_IS_LABEL (label));
}

>  
> Havoc
> 


> Index: gdk/gdkcolor.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/gdkcolor.c,v
> retrieving revision 1.22
> diff -u -r1.22 gdkcolor.c
> --- gdk/gdkcolor.c	2000/03/28 01:40:57	1.22
> +++ gdk/gdkcolor.c	2000/05/15 22:33:27
> @@ -32,37 +32,30 @@
>  GdkColormap*
>  gdk_colormap_ref (GdkColormap *cmap)
>  {
> -  GdkColormapPrivate *private = (GdkColormapPrivate *)cmap;
> -
>    g_return_val_if_fail (cmap != NULL, NULL);
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     you can skip this line

> +  g_return_val_if_fail (GDK_IS_COLORMAP (cmap), NULL);
> +  
> +  g_object_ref (G_OBJECT (cmap));
>  
> -  private->ref_count += 1;
>    return cmap;
>  }

>  void
> Index: gdk/gdkcolor.h
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/gdkcolor.h,v
> retrieving revision 1.2
> diff -u -r1.2 gdkcolor.h
> --- gdk/gdkcolor.h	2000/03/14 19:57:22	1.2
> +++ gdk/gdkcolor.h	2000/05/15 22:33:27
> @@ -2,6 +2,7 @@
>  #define __GDK_COLOR_H__
>  
>  #include <gdk/gdktypes.h>
> +#include <gobject/gobject.h>

#include <glib-object.h>

>  #ifdef __cplusplus
>  extern "C" {
> @@ -24,17 +25,34 @@
>  
>  /* The colormap type.
>   */
> +typedef struct _GdkColormapClass GdkColormapClass;
> +
> +#define	GDK_TYPE_COLORMAP		(gdk_colormap_get_type ())
> +#define GDK_COLORMAP(object)		(G_TYPE_CHECK_INSTANCE_CAST ((object), GDK_TYPE_COLORMAP, GdkColormap))
> +#define GDK_COLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_CAST ((klass), GDK_TYPE_COLORMAP, GdkColormapClass))
> +#define GDK_IS_COLORMAP(object)		(G_TYPE_CHECK_INSTANCE_TYPE ((object), GDK_TYPE_COLORMAP))
> +#define GDK_IS_COLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_TYPE ((klass), GDK_TYPE_COLORMAP))

GDK_COLORMAP_GET_CLASS() is missing

> Index: gdk/x11/gdkcolor-x11.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/x11/gdkcolor-x11.c,v
> retrieving revision 1.22
> diff -u -r1.22 gdkcolor-x11.c
> --- gdk/x11/gdkcolor-x11.c	2000/03/28 01:24:42	1.22
> +++ gdk/x11/gdkcolor-x11.c	2000/05/15 22:33:27
> @@ -38,31 +38,113 @@
>  static gint  gdk_colormap_cmp         (Colormap    *a,
>  				       Colormap    *b);
>  
> +static void gdk_x_colormap_class_init (GdkXColormapClass *klass);
> +static void gdk_x_colormap_init       (GdkXColormap      *cmap);
> +static void gdk_x_colormap_finalize   (GObject           *object);
> +
> +
>  static GHashTable *colormap_hash = NULL;
> +static GObjectClass * parent_class = NULL;
> +
> +GType
> +gdk_x_colormap_get_type ()
                           (void)
> +{
> +  static GType object_type = 0;
> +
> +  if (!object_type)
> +    {
> +      static const GTypeInfo object_info =
> +      {
> +	sizeof (GdkXColormapClass),
> +	(GBaseInitFunc) NULL,
> +	(GBaseFinalizeFunc) NULL,
> +	(GClassInitFunc) gdk_x_colormap_class_init,
> +	NULL,		/* class_finalize */
> +	NULL,		/* class_data */
> +	sizeof (GdkXColormap),
> +	0,			/* n_preallocs */
> +	(GInstanceInitFunc) gdk_x_colormap_init,
> +      };
> +      
> +      object_type = g_type_register_static (G_TYPE_OBJECT,
> +                                            "GdkColormap",
> +                                            &object_info);
> +    }
> +
> +  return object_type;
> +}

ok, in general, a function prefix_object_name_get_type (void) should
only introduce a type "PrefixObjectName", several automated tools make
use of this convention.
i'm not sure if your idea of presenting a GdkColormap class at the generic
Gdk level and moving the actuall implementation into the platform subdirs
is a good idea. while i understand your reasonings, i have a bad feeling that
this might bite back someday for some reason... ;)

> +static void
> +gdk_x_colormap_class_init (GdkXColormapClass *klass)
> +{
> +  GObjectClass * object_class;
> +
> +  object_class = (GObjectClass *) klass;

us G_OBJECT_CLASS() for casts.

> +
> +  object_class->finalize = gdk_x_colormap_finalize;
> +
> +  parent_class = g_type_class_ref (G_TYPE_OBJECT);

g_type_class_peek()

> +}
> +
> +static void
> +gdk_x_colormap_init (GdkXColormap *x_cmap)
> +{
> +  GdkColormap *cmap;
> +
> +  cmap = (GdkColormap *) x_cmap;

GDK_COLORMAP() for casting

> +  
> +  x_cmap->xdisplay = gdk_display;
> +}
> +
> +static void
> +gdk_x_colormap_finalize (GObject *object)
> +{
> +  GdkXColormap *x_cmap;
> +  GdkColormap * colormap;
> +  
> +  x_cmap = GDK_X_COLORMAP (object);
> +  colormap = (GdkColormap *) x_cmap;
> +  
> +  gdk_colormap_remove (colormap);
> +  XFreeColormap (x_cmap->xdisplay, x_cmap->xcolormap);
>  
> +  if (x_cmap->hash)
> +    g_hash_table_destroy (x_cmap->hash);
> +  
> +  g_free (x_cmap->info);
> +  g_free (colormap->colors);
> +  
> +  (* parent_class->finalize) (object);

better:
G_OBJECT_CLASS (parent_class)->finalize (object);
(i.e. disentangle implicit parent class knowledge
from the implementation)

> @@ -350,13 +415,13 @@
>  		  gulong        *pixels,
>  		  gint           npixels)
>  {
> -  GdkColormapPrivateX *private;
> +  GdkXColormap *private;
>    gint return_val;
>    gint i;
>  
>    g_return_val_if_fail (colormap != NULL, 0);
>  
> -  private = (GdkColormapPrivateX*) colormap;
> +  private = (GdkXColormap *) colormap;
>  
>    return_val = XAllocColorCells (private->xdisplay, private->xcolormap,
>  				 contiguous, planes, nplanes, pixels, npixels);

exemplary change (should be applied to all other functions as well):

                 gulong        *pixels,
                 gint           npixels)
  {
!   GdkXColormap *xcmap;
    gint return_val;
    gint i;

!   g_return_val_if_fail (GDK_IS_XCOLORMAP (colormap), 0);

!   xcmap = GDK_XCOLORMAP (colormap);

    return_val = XAllocColorCells (xcmap->xdisplay, xcmap->xcolormap,
                                   contiguous, planes, nplanes, pixels, npixels);






> Index: gdk/x11/gdkcolor-x11.h
> ===================================================================
> RCS file: gdkcolor-x11.h
> diff -N gdkcolor-x11.h
> --- /dev/null	Tue May  5 16:32:27 1998
> +++ gdkcolor-x11.h	Mon May 15 18:33:27 2000
> @@ -0,0 +1,70 @@
> +/* GDK - The GIMP Drawing Kit
> + * Copyright (C) 1995-1997 Peter Mattis, Spencer Kimball and Josh MacDonald
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public
> + * License along with this library; if not, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 02111-1307, USA.
> + */
> +
> +/*
> + * Modified by the GTK+ Team and others 1997-1999.  See the AUTHORS
> + * file for a list of people on the GTK+ Team.  See the ChangeLog
> + * files for a list of changes.  These files are distributed with
> + * GTK+ at ftp://ftp.gtk.org/pub/gtk/. 
> + */
> +
> +#ifndef __GDK_COLOR_X11_H__
> +#define __GDK_COLOR_X11_H__
> +
> +#include <gdk/gdkcolor.h>
> +#include <X11/Xlib.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +typedef struct _GdkXColormap GdkXColormap;
> +typedef struct _GdkXColormapClass GdkXColormapClass;
> +
> +#define	GDK_TYPE_X_COLORMAP		(gdk_x_colormap_get_type ())
> +#define GDK_X_COLORMAP(object)		(G_TYPE_CHECK_INSTANCE_CAST ((object), GDK_TYPE_X_COLORMAP, GdkXColormap))
> +#define GDK_X_COLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_CAST ((klass), GDK_TYPE_X_COLORMAP, GdkXColormapClass))
> +#define GDK_IS_X_COLORMAP(object)       (G_TYPE_CHECK_INSTANCE_TYPE ((object), GDK_TYPE_X_COLORMAP))
> +#define GDK_IS_X_COLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_TYPE ((klass), GDK_TYPE_X_COLORMAP))

_GET_CLASS() is missing. also, according to our current tools
for automated CapitalName -> UPPER_NAME conversions, this has to
be:

#define	GDK_TYPE_XCOLORMAP		(gdk_xcolormap_get_type ())
#define GDK_XCOLORMAP(object)		(G_TYPE_CHECK_INSTANCE_CAST ((object), GDK_TYPE_XCOLORMAP, GdkXColormap))
#define GDK_XCOLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_CAST ((klass), GDK_TYPE_XCOLORMAP, GdkXColormapClass))
#define GDK_IS_XCOLORMAP(object)	(G_TYPE_CHECK_INSTANCE_TYPE ((object), GDK_TYPE_XCOLORMAP))
#define GDK_IS_XCOLORMAP_CLASS(klass)	(G_TYPE_CHECK_CLASS_TYPE ((klass), GDK_TYPE_XCOLORMAP))
#define GDK_XCOLORMAP_GET_CLASS(obj)	(G_TYPE_INSTANCE_GET_CLASS ((obj), GDK_TYPE_XCOLORMAP, GdkXColormap))

(we'd convert GtkCList to GTK_C_LIST otherwise, so single uppercase letters are
concatenated with succeeding captialized words)

> Index: gdk/x11/gdkpixmap-x11.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gdk/x11/gdkpixmap-x11.c,v
> retrieving revision 1.31
> diff -u -r1.31 gdkpixmap-x11.c
> --- gdk/x11/gdkpixmap-x11.c	2000/03/28 01:24:43	1.31
> +++ gdk/x11/gdkpixmap-x11.c	2000/05/15 22:33:27

> @@ -152,8 +140,8 @@
>  #define GDK_IMAGE_XDISPLAY(image)     (((GdkImagePrivate*) image)->xdisplay)
>  #define GDK_IMAGE_XIMAGE(image)       (((GdkImagePrivate*) image)->ximage)
>  #define GDK_GC_XDISPLAY(gc)           (GDK_GC_XDATA(gc)->xdisplay)
> -#define GDK_COLORMAP_XDISPLAY(cmap)   (((GdkColormapPrivateX *)cmap)->xdisplay)
> -#define GDK_COLORMAP_XCOLORMAP(cmap)  (((GdkColormapPrivateX *)cmap)->xcolormap)
> +#define GDK_COLORMAP_XDISPLAY(cmap)   (((GdkXColormap *)cmap)->xdisplay)
> +#define GDK_COLORMAP_XCOLORMAP(cmap)  (((GdkXColormap *)cmap)->xcolormap)

see? the "x" is concatenated with "colormap", also, use the provided casting macros,
now that GdkXColormap is_a GObject.


ok, in general, i think this is a very good first attempt at GObject-izing
Gdk objects. there are the general issues of adhearing to the various naming/
object/type conventions, that we need to keep the basic system going, but
that's easily resolved and the conventions have to be clearly stated in some
place anyways.
i do think though, that it might be a better idea to go with your initial
suggestion of having a real GdkColormap object and really derive platform
specific classes thereof.
input on this issue from language binders would greatly be apprechiated
(whether either/both solutions pose issues for them or whether they are
fine either way...)

once again, thanks havoc for starting out here.

---
ciaoTJ