Comments on GTK+ patches from Atk tarball

[Owen Taylor <otaylor redhat com>]

 * I don't see the point of GtkAccessible. As it stands now,
   the only thing it has now is a backpointer to the widget
   and no implementation. Is adding more stuff to this 
   planned?

   Basically, why have a publically visible gtk-namespaced AtkObject
   derivative? People using the accessible functionality will be
   referring to accessibles as AtkObject, I assume.

 * The fact that there is a public gtk_widget_get_accessible()
   doesn't strike me as right. I would expect simply people
   to use atk_object_ref_accessible (widget). [ There are
   some naming problems in Atk with the interfaces, etc,
   I'll comment on separately. ]

   And the pointer to the GtkAccessible in the GtkWidget structure
   strikes me as odd; after all, the way we are handling 
   accessibles is inherently treating them as flightweights.

   I suppose what you are trying to do here is provide something
   that conforms to the GTK+ conventions for naming and 
   referencing return objects - I think this is perhaps
   a false simplication; better to be conceptually simple
   than to merely look simple.

 * While GtkAccessibleFactory makes sense, it strikes 
   me that simply using a function pointer for this would
   be simpler / easier. Using a class for this is (IMO)
   overkill, and we certainly use function pointers througout
   GTK+ for similar things.

 * _gtk_weaktype_ref_accessible shouldn't have the leading
   underscore, since we don't use that for static functions,
   and by GTK+ convention, would be simply gtk_widget_ref_accesible.

 * The overhead of initializing every class in GTK+ is quite 
   large and we intentionally delay initialization until an
   instance of the class is created. So, I don't think
  
    gtk_widget_class_set_accessibility_factory()
 
   is a good idea; better to have a global factory.

Can I suggest something more like the following patch (not
tested to compile):

Index: gtk/gtkwidget.c
===================================================================
RCS file: /cvs/gnome/gtk+/gtk/gtkwidget.c,v
retrieving revision 1.199
diff -u -r1.199 gtkwidget.c
--- gtk/gtkwidget.c	2001/03/28 04:01:21	1.199
+++ gtk/gtkwidget.c	2001/03/28 23:05:23
@@ -196,6 +196,10 @@
 static GtkWidgetAuxInfo* gtk_widget_aux_info_new     (void);
 static void		 gtk_widget_aux_info_destroy (GtkWidgetAuxInfo *aux_info);
 
+static GtkAccessible* gtk_widget_real_ref_accessible       (GtkWidget      *widget);
+static void           gtk_widget_accessible_interface_init (AtkObjectIface *iface);
+static AtkObject *    gtk_widget_ref_accessible            (GObject        *obj);
+
 static gpointer         parent_class = NULL;
 static guint            widget_signals[LAST_SIGNAL] = { 0 };
 
@@ -247,7 +251,18 @@
 	(GtkClassInitFunc) NULL,
       };
       
+      static const GInterfaceInfo accessibilityInterfaceInfo =
+      {
+        (GInterfaceInitFunc) gtk_widget_accessible_interface_init,
+        (GInterfaceFinalizeFunc) NULL,
+        NULL /* interface data */
+      };
+
       widget_type = gtk_type_unique (GTK_TYPE_OBJECT, &widget_info);
+
+      g_type_add_interface_static (widget_type, ATK_TYPE_OBJECT_IFACE,
+                                   &accessibilityInterfaceInfo) ;
+
     }
   
   return widget_type;
@@ -344,6 +359,9 @@
   klass->drag_drop = NULL;
   klass->drag_data_received = NULL;
 
+  /* Accessibility support */
+  klass->ref_accessible = gtk_widget_real_ref_accessible;
+
   klass->no_expose_event = NULL;
 
   quark_property_parser = g_quark_from_static_string ("gtk-rc-property-parser");
@@ -5406,5 +5424,105 @@
     {
       *path_p = g_strdup (rev_path);
       g_strreverse (*path_p);
+    }
+}
+
+/*** Accessibility support ***/
+
+typedef struct _GtkAccessibleFactory GtkAccessibleFactory;
+
+struct _GtkAccessibleFactory 
+{
+  GtkAccessibleFactoryFunc *func;
+  gpointer data;
+}
+
+static GSList *accessible_factories = NULL;
+
+
+static GtkAccessible *
+gtk_widget_real_ref_accessible (GtkWidget *widget)
+{
+  AtkObject *result = NULL;
+  GSList *tmp_list;
+
+  tmp_list = accessible_factories;
+  while (!result && tmp_list)
+    {
+      GtkAccessibleFactory *factory = tmp_list->data;
+      tmp_list = tmp_list->next;
+
+      result = factory->func (widget, data);
+    }
+
+  return result;
+}
+
+/*
+ * Initialize a AtkObjectIface instance's virtual pointers as
+ * appropriate to this implementor's class (GtkWidget).
+ */
+static void
+gtk_widget_accessible_interface_init (AtkObjectIface *iface)
+{
+  iface->ref_accessible = gtk_widget_ref_accessible;
+}
+
+static AtkObject*
+gtk_widget_ref_accessible (GObject *obj)
+{
+  return GTK_WIDGET_GET_CLASS (obj)->ref_acesssible (GTK_WIDGET (obj));
+}
+
+/**
+ * gtk_widget_add_accessible_factory:
+ * @func: a function to be called to retrieve the #AtkObject
+ * @data: callback data to pass to @func
+ * 
+ * Add a function to be called to provde a #AtkObject when the widget doesn't
+ * provide its own implementation of #GtkWidget::ref_accessible. When
+ * multiple factories have been added, they will be called in sequence
+ * from the latest added until one returns non-NULL.
+ **/
+void
+gtk_widget_add_accessible_factory (GtkAccessibleFactoryFunc *func,
+				   gpointer                  data)
+{
+  GtkAccessibleFactory *factory;
+
+  factory = g_new (GtkAccessibleFactory, 1);
+  
+  factory->func = func;
+  factory->data = data;
+
+  accessible_factories = g_slist_prepend (accessible_factories, factory);
+}
+
+/**
+ * gtk_widget_remove_accessible_factory:
+ * @func: a function previously passed to gtK_widget_add_accessible_factory()
+ * @data: the callback data passed along with @func
+ * 
+ * Remove a function previously added with gtk_widget_add_accessible_factory()
+ **/
+void
+gtk_widget_remove_accessible_factory (GtkAccessibleFactoryFunc *func,
+				      gpointer                  data)
+{
+  GSList *tmp_list;
+
+  tmp_list = accessible_factories;
+  while (tmp_list)
+    {
+      GtkAccessibleFactory *factory = tmp_list->data;
+
+      if (factory->func == func && factory->data == data)
+	{
+	  accessible_factories = g_slist_delete_link (accessible_factories, tmp_list);
+	  g_free (factory);
+	  return;
+	}
+      
+      tmp_list = tmp_list->next;
     }
 }
Index: gtk/gtkwidget.h
===================================================================
RCS file: /cvs/gnome/gtk+/gtk/gtkwidget.h,v
retrieving revision 1.102
diff -u -r1.102 gtkwidget.h
--- gtk/gtkwidget.h	2001/03/28 19:12:56	1.102
+++ gtk/gtkwidget.h	2001/03/28 23:05:23
@@ -135,6 +135,9 @@
 typedef void     (*GtkCallback)        (GtkWidget        *widget,
 					gpointer	  data);
 
+typedef AtkObject *(*GtkAccessibleFactoryFunc) (GtkWidget *widget,
+						gpointer   data);
+
 /* A requisition is a desired amount of space which a
  *  widget may request.
  */
@@ -378,6 +386,10 @@
 				    guint               info,
 				    guint               time);
   
+  /* accessibility support 
+   */
+  AtkObject* (* ref_accessible) (GtkWidget         *widget);
+
   /* Padding for future expandsion */
   GtkFunction pad1;
   GtkFunction pad2;
@@ -533,6 +545,13 @@
 					 GtkType	widget_type);
 GdkColormap* gtk_widget_get_colormap	(GtkWidget	*widget);
 GdkVisual*   gtk_widget_get_visual	(GtkWidget	*widget);
+
+
+/* Accessibility support */
+void gtk_widget_add_accessible_factory    (GtkAccessibleFactoryFunc *func,
+					   gpointer                  data);
+void gtk_widget_remove_accessible_factory (GtkAccessibleFactoryFunc *func,
+					   gpointer                  data);
 
 /* The following functions must not be called on an already
  * realized widget. Because it is possible that somebody