Re: Comments on GTK+ patches from Atk tarball

[Bill Haneman <Bill Haneman sun com>]

Hi:

Here are some detailed comments on Owen's proposed alternate patch for 
GtkWidget, to support Atk.  Some of this may be moot or obvious based on 
what I posted previously about apparent misunderstandings about usage, 
but I thought I should respond specifically.  

My comments are in square brackets.

Here goes:



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

[ OK so far... ]

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

[Yep :-) ]

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

[ We use "get_accessible" and not "ref_accessible" since this method 
returns a persistant instance and not a flyweight.
We also need an efficient means of persisting the GtkAccessible inside 
the widget instance, thus the widget->accessible member.]

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

[No, this needs to be in its own .h and .c files.]

+static GSList *accessible_factories = NULL;

[There are not multiple factories per widget class, there is never a 
list of factories.  A single factory is associated with a widget class, 
via either inheritance or explicit setting of the 
widgetklass->accessible_factory .]

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

[Since we support two entry points to the API, both the general (via 
AtkAccessibleIface) and the Gtk-specific (via 
gtk_widget_get_accessible), we need both 
gtk_widget_real_get_accessible() and gtk_widget_real_ref_accessible().  
To indicate that the second function uses weak typing for its return 
value we called it gtk_widget_weaktype_ref_accessible().]


+/*
+ * 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;
+}

[Yes, though we do a check for non-null iface first.  Was that 
unnecessary ?]

+static AtkObject*
+gtk_widget_ref_accessible (GObject *obj)
+{
+  return GTK_WIDGET_GET_CLASS (obj)->ref_acesssible (GTK_WIDGET (obj));
+}

[In our implementation this is a special wrapper around get_accessible, 
which downcasts to AtkObject* and increments the reference count.  We 
have a similar, but non-static, function 
gtk_widget_get_accessible(GtkWidget *widget) which returns the 
persistent accessible instance.]

+/**
+ * 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;
 
 
[Again, I think our differences here have to do with a misunderstanding 
about how GtkAccessibleFactories work.] 
 
     }
 }
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);
+

[ We need to do some degree of reflection on the factory associated with 
a widget type, so storing just a func pointer is probably not 
sufficient.]

 /* 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);
+

[ We don't need a virtual function for the weakly-typed-flyweight 
version, which is only used via AtkAccessibleIface.  Instead we define
 GtkAccessible* (* get_accessible) (GtkWidget    *widget); 
which returns a persistant instance. ]

   /* 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

------
Bill Haneman x19279
Gnome Accessibility / Batik SVG Toolkit
Sun Microsystems Ireland