GNOME.org
 

Re: [g-a-devel]at-spi patch (bugfixes and extended events)

On Wed, 2002-11-20 at 15:15, Michael Meeks wrote:
> Hi Bill,
> 
> On Mon, 2002-11-18 at 19:25, Bill Haneman wrote:
> > I attach a patch for at-spi to add support for extended events
> 
> 	Overall the patch looks good; but (as you might expect ;-) I have a
> number of comments / questions:

Hi Michael:

Thanks for your comments.  I attach a patch that incorporates your
suggestions, I have a couple of comments below but in short I think the
patch covers all the issues you raise.

I will apply this patch later today; of course we can refine it further
if that seems warranted.

Best regards,

Bill

...

> > +/**
> > + * AccessibleEvent_getContextObject:
> > 
> 
> 	This really disturbs me; and follows the existing (bad) convention of
> exposing a struct which is essentially:
> 
> typedef struct {
> 	Object magic_1_I_guess_we_need_an_object;
> 	long   magic_2_we_might_need_a_long;
> 	double magic_3_someone_might_need_a_double;
> 	int    magic_4_an_int_for_good_measure;
> 	string magic_5_a_string;
> 	string magic_6_another_string_I_happened_to_need;
> } Event;

Well, we are not creating such a struct, we are using CORBA_any which is
designed for this and plenty flexible.  However I agree that there are
drawbacks to trying for a "generic" API here, the more specific methods
would be better.  The attached patch uses them, i.e.

AccessibleTextChangedEvent_getChangeString (AccessibleEvent *e),

etc. as you suggest.

>	Oh - and there's a leak / memory corruption issue there; you have to
> garentee the lifetime of 'a' over the event emission, otherwise you pass
> off an object handle that may well be pointing at invalid memory 

In the current model we pass memory that is valid for the emission,
however the Accessible being pointed to (for cases where that's what is
obtained) may not be in a pingable state by the time the event is
received.

Again, thanks for your comments.

-Bill


Index: atk-bridge/bridge.c
===================================================================
RCS file: /cvs/gnome/at-spi/atk-bridge/bridge.c,v
retrieving revision 1.50
diff -u -r1.50 bridge.c
--- atk-bridge/bridge.c	20 Nov 2002 17:14:28 -0000	1.50
+++ atk-bridge/bridge.c	21 Nov 2002 15:32:37 -0000
@@ -40,7 +40,8 @@
 
 #define DBG(a,b) if(_dbg>=(a))b
 
-static int _dbg;
+static int _dbg = 0;
+static char *spi_nil_string = "";
 
 static CORBA_Environment ev;
 static Accessibility_Registry registry = NULL;
@@ -73,25 +74,6 @@
    static guint atk_signal_column_deleted;
 */
 
-#define ATK_BRIDGE_RESERVED_CONTEXT_SIZE 16
-
-typedef enum {
-  ATK_BRIDGE_CONTEXT_TYPE_NONE = 0,
-  ATK_BRIDGE_CONTEXT_TYPE_STRING,
-  ATK_BRIDGE_CONTEXT_TYPE_OBJECT
-} AtkBridgeEventContextType;
-
-typedef union {
-  gchar       *string;
-  AtkObject   *object;
-  gpointer    *foo;
-} AtkBridgeEventContextData;
-
-typedef struct {
-  AtkBridgeEventContextType _type;
-  AtkBridgeEventContextData _data;
-} AtkBridgeEventContext;
-
 static Accessibility_Registry spi_atk_bridge_get_registry (void);
 static void     spi_atk_bridge_do_registration         (void);
 static void     spi_atk_bridge_toplevel_added          (AtkObject             *object,
@@ -151,13 +133,16 @@
 static int
 atk_bridge_init (gint *argc, gchar **argv[])
 {
+  const char *debug_env_string = g_getenv ("AT_SPI_DEBUG");
+
   if (atk_bridge_initialized)
     {
       return 0;
     }
   atk_bridge_initialized = TRUE;
 
-  _dbg = (int) g_ascii_strtod (g_getenv ("AT_SPI_DEBUG"), NULL);
+  if (debug_env_string)
+    _dbg = (int) g_ascii_strtod (debug_env_string, NULL);
 
   if (!bonobo_init (argc, argv ? *argv : NULL))
     {
@@ -183,7 +168,7 @@
       spi_atk_bridge_do_registration ();
     }
  
-  atk_bridge_init_event_type_consts ();
+  spi_atk_bridge_init_event_type_consts ();
 
   return 0;
 }
@@ -434,34 +419,6 @@
 }
 
 static void
-spi_atk_bridge_event_context_init (CORBA_any *any, 
-				   AtkBridgeEventContext *ctx)
-{
-  SpiAccessible *accessible;
-  if (ctx) 
-    {
-      switch (ctx->_type) 
-	{
-	  /* FIXME	
-	    case ATK_BRIDGE_CONTEXT_TYPE_OBJECT:
-		accessible = spi_accessible_new (ctx->_data.object);    
-		spi_init_any_object (any, BONOBO_OBJREF (accessible));
-		break;
-	  */
-	case ATK_BRIDGE_CONTEXT_TYPE_STRING:
-	  spi_init_any_string (any, &ctx->_data.string);
-	  break;
-	default:
-	  spi_init_any_nil (any); 
-	} 
-    }
-  else
-    {
-      spi_init_any_nil (any); 
-    }
-} 
-
-static void
 spi_atk_bridge_focus_tracker (AtkObject *object)
 {
   SpiAccessible *source;
@@ -482,72 +439,17 @@
   CORBA_exception_free (&ev);
 }
 
-static
-AtkBridgeEventContext *
-spi_atk_bridge_event_context_create (GObject *gobject, 
-				     long detail1, 
-				     long detail2, 
-				     GSignalQuery *signal_query)
-{
-  AtkBridgeEventContext *ctx = g_new0 (AtkBridgeEventContext, 1);
-  /* don't free the context, it's on the stack */
-
-  /* if it's a window event, then marshal the window name */
-
-  /*
-  if (signal_query->signal_id == atk_signal_child_changed) 
-    {  
-      ctx->_type = ATK_BRIDGE_CONTEXT_TYPE_OBJECT;
-      ctx->_data.object = atk_object_ref_accessible_child (ATK_OBJECT (gobject),
-							   (gint) detail1);
-    }
-  else */ if (signal_query && (signal_query->signal_id == atk_signal_text_changed))
-    {
-      ctx->_type = ATK_BRIDGE_CONTEXT_TYPE_STRING;
-      ctx->_data.string = atk_text_get_text (ATK_TEXT (gobject),
-					     (gint) detail1,
-					     (gint) detail1+detail2);
-    }
-  else
-    {
-      ctx->_type = ATK_BRIDGE_CONTEXT_TYPE_NONE;
-    }
-  return ctx;
-}
-
-static void
-spi_atk_bridge_event_context_free (AtkBridgeEventContext *ctx)
-{
-  if (ctx->_type == ATK_BRIDGE_CONTEXT_TYPE_OBJECT)
-    g_object_unref (ctx->_data.object);
-  g_free (ctx);
-}
-
-
-static void
-spi_atk_bridge_any_init ()
-{
-  AtkBridgeEventContext *ctx;
-  /* build some event context data, depending on the type */
-  ctx = spi_atk_bridge_event_context_create (gobject, 
-					     detail1, detail2, 
-					     signal_query);
-  spi_atk_bridge_event_context_init (&e.any_data, ctx); 
-  spi_atk_bridge_event_context_free (ctx);
-}
-
 static void
-spi_atk_emit_eventv (GObject               *gobject,
-		     unsigned long          detail1,
-		     unsigned long          detail2,
-		     GSignalQuery          *signal_query,
-		     const char   *format, ...)
+spi_atk_emit_eventv (const GObject         *gobject,
+		     long                   detail1,
+		     long                   detail2,
+		     CORBA_any             *any,
+		     const char            *format, ...)
 {
   va_list             args;
   Accessibility_Event e;
   SpiAccessible      *source;
   AtkObject          *aobject;
-  AtkBridgeEventContext *ctx;
 #ifdef SPI_BRIDGE_DEBUG
   CORBA_string s;
 #endif
@@ -569,15 +471,18 @@
     {
       aobject = NULL;
       source  = NULL;
-      g_error ("received property-change event from non-AtkImplementor");
+      DBG (0, g_warning ("received property-change event from non-AtkImplementor"));
     }
 
-  if (source != NULL)
+  if (source) 
     {
       e.type = g_strdup_vprintf (format, args);
       e.source = BONOBO_OBJREF (source);
       e.detail1 = detail1;
       e.detail2 = detail2;
+      if (any) e.any_data = *any;
+      else spi_init_any_nil (&e.any_data);
+      
 #ifdef SPI_BRIDGE_DEBUG
       s = Accessibility_Accessible__get_name (BONOBO_OBJREF (source), &ev);
       g_warning ("Emitting event '%s' (%lu, %lu) on %s",
@@ -585,23 +490,20 @@
       CORBA_free (s);
 #endif
       CORBA_exception_init (&ev);
-      spi_atk_bridge_event_any_init (&e.any_data, gobject, 
-				     detail1, detail2, signal_query);
       Accessibility_Registry_notifyEvent (spi_atk_bridge_get_registry (), 
 					  &e, &ev);
-      /* I haven't freed any_data._value when it's a char*, does it leak ? */
 #ifdef SPI_BRIDGE_DEBUG
       if (ev._major != CORBA_NO_EXCEPTION)
-	      g_warning ("error emitting event %s, (%d) %s",
-			 e.type,
-			 ev._major,
-			 CORBA_exception_id(&ev));
+	g_warning ("error emitting event %s, (%d) %s",
+		   e.type,
+		   ev._major,
+		   CORBA_exception_id(&ev));
 #endif	      
       if (BONOBO_EX (&ev)) registry_died = TRUE;
       Accessibility_Accessible_unref (e.source, &ev);
-
+      
       CORBA_exception_free (&ev);
-
+      
       g_free (e.type);
     }
 
@@ -765,6 +667,57 @@
   return result;
 }
 
+
+static void
+spi_atk_signal_emit_event (const GObject *gobject, 
+			   const GSignalQuery *signal_query,
+			   long detail1, 
+			   long detail2,
+			   const gchar *name, 
+			   const gchar *detail)
+{
+  CORBA_any any;
+  CORBA_char *sp = NULL;
+  AtkObject *ao;
+
+  if (signal_query->signal_id == atk_signal_text_changed)
+    {
+      sp = atk_text_get_text (ATK_TEXT (gobject),
+			      detail1,
+			      detail1+detail2);
+      spi_init_any_string (&any, &sp);
+    }
+#ifdef EXTENDED_OBJECT_EVENTS_ARE_WORKING
+  else if ((signal_query->signal_id == atk_signal_child_changed) && gobject)
+    {
+      ao = atk_object_ref_accessible_child (ATK_OBJECT (gobject), 
+					    detail1);
+      if (ao) 
+	{
+	  spi_init_any_object (&any, ao); 
+	  atk_object_unref (ao);
+	}
+      else
+	{
+	  spi_init_any_nil (&any);
+	}
+    }
+#endif
+  else
+    {
+      spi_init_any_nil (&any);
+    }
+
+  if (detail)
+    spi_atk_emit_eventv (gobject, detail1, detail2, &any,
+			 "object:%s:%s", name, detail);
+  else
+    spi_atk_emit_eventv (gobject, detail1, detail2, &any,
+			 "object:%s", name);
+}
+
+
+
 static gboolean
 spi_atk_bridge_signal_listener (GSignalInvocationHint *signal_hint,
 				guint n_param_values,
@@ -775,7 +728,7 @@
   GSignalQuery signal_query;
   const gchar *name;
   const gchar *detail;
-  AtkBridgeEventContext *ctx = NULL;
+  CORBA_char *sp;
   
   gint detail1 = 0, detail2 = 0;
 #ifdef SPI_BRIDGE_DEBUG
@@ -795,35 +748,30 @@
   s = atk_object_get_name (ATK_OBJECT (g_value_get_object (param_values + 0)));
   fprintf (stderr, "Received signal %s:%s detail: %s from object %s (gail %s)\n",
 	   g_type_name (signal_query.itype), name, 
-                        detail ? detail : "<NULL>", s ? s : "<NULL>" , s2);
+	   detail ? detail : "<NULL>", s ? s : "<NULL>" , s2);
 #endif
-
+  
   gobject = g_value_get_object (param_values + 0);
   if (G_VALUE_TYPE (param_values + 1) == G_TYPE_INT)
     detail1 = g_value_get_int (param_values + 1);
   if (G_VALUE_TYPE (param_values + 2) == G_TYPE_INT)
     detail2 = g_value_get_int (param_values + 2);
-
-  if (detail)
-    spi_atk_emit_eventv (gobject, detail1, detail2, &signal_query,
-			 "object:%s:%s", name, detail);
-  else
-    spi_atk_emit_eventv (gobject, detail1, detail2, &signal_query,
-			 "object:%s", name);
-
+  
+  spi_atk_signal_emit_event (gobject, &signal_query,
+			     detail2, detail2,
+			     name, detail);
   return TRUE;
 }
 
 static gboolean
 spi_atk_bridge_window_event_listener (GSignalInvocationHint *signal_hint,
-				guint n_param_values,
-				const GValue *param_values,
-				gpointer data)
+				      guint n_param_values,
+				      const GValue *param_values,
+				      gpointer data)
 {
   GObject *gobject;
   GSignalQuery signal_query;
-  AtkBridgeEventContext ctx;
-
+  CORBA_any any;
   const gchar *name, *s;
 #ifdef SPI_BRIDGE_DEBUG
   const gchar *s2;
@@ -841,7 +789,11 @@
 #endif
   
   gobject = g_value_get_object (param_values + 0);
-  spi_atk_emit_eventv (gobject, 0, 0, &signal_query, "window:%s", name);
 
+  s = atk_object_get_name (ATK_OBJECT (gobject));
+  spi_init_any_string (&any, &s);
+  
+  spi_atk_emit_eventv (gobject, 0, 0, &any,
+		       "window:%s", name);
   return TRUE;
 }
Index: cspi/spi_event.c
===================================================================
RCS file: /cvs/gnome/at-spi/cspi/spi_event.c,v
retrieving revision 1.22
diff -u -r1.22 spi_event.c
--- cspi/spi_event.c	19 Nov 2002 20:04:17 -0000	1.22
+++ cspi/spi_event.c	21 Nov 2002 15:32:37 -0000
@@ -327,36 +327,18 @@
   cspi_device_listener_unref (listener);
 }
 
-/**
- * AccessibleEvent_getContextString:
- * @event: a pointer to the #AccessibleEvent being queried.
- *
- * Queries an #AccessibleEvent for a string whose meaning depends
- *         on the event's context, that is, the event type and details.
- *         Not all events have context strings, in which case this 
- *         query returns NULL.  
- *         Context strings may be returned by:
- *         object:text-changed events, 
- *         in which case they represent the text inserted or deleted;
- *         window: events, in which case they indicate the window title,
- *         or other events.
- *
- * Returns: the context string for the event, or %NULL if
- *          there is no context string for the event.
- **/
-char *
-AccessibleEvent_getContextString (const AccessibleEvent *e)
+static char *
+cspi_internal_event_get_text (const InternalEvent *e)
 {
-  InternalEvent *foo = (InternalEvent *) e;
   CORBA_any *any;
-  g_return_val_if_fail (foo, NULL);
-  g_return_val_if_fail (foo->data, NULL);
-  any = (CORBA_any *) foo->data;
+  g_return_val_if_fail (e, NULL);
+  g_return_val_if_fail (e->data, NULL);
+  any = (CORBA_any *) e->data;
   if (CORBA_TypeCode_equivalent (any->_type, TC_CORBA_string, NULL)) 
     {
       return * (char **) any->_value;
     } 
-  else 
+  else
     {
 #ifdef EVENT_CONTEXT_DEBUG
       fprintf (stderr, "requested string, TC is not TC_CORBA_string! (%u)\n",
@@ -366,35 +348,56 @@
     }
 }
 
+static char *
+cspi_internal_event_get_object (const InternalEvent *e)
+{
+  CORBA_any *any;
+  g_return_val_if_fail (e, NULL);
+  g_return_val_if_fail (e->data, NULL);
+  any = (CORBA_any *) e->data;
+  if (any->_type == TC_CORBA_Object) 
+    return cspi_object_add (* (CORBA_Object *) any->_value);
+  else 
+    return NULL;
+}
+
+
+/**
+ * AccessibleTextChangedEvent_getChangeString:
+ * @event: a pointer to the #AccessibleEvent being queried.
+ *
+ * Queries an #AccessibleEvent of type "object:text-changed", 
+ *         returning the text inserted or deleted.
+ *
+ * Returns: a UTF-8 text string indicating the text inserted,
+ *          deleted, or substituted by this event.
+ **/
+char *
+AccessibleTextChangedEvent_getChangeString (const AccessibleEvent *e)
+{
+  InternalEvent *foo = (InternalEvent *) e;
+  /* TODO: check the event type? expensive... */
+  return cspi_internal_event_get_text (e);
+}
+
 /**
- * AccessibleEvent_getContextObject:
+ * AccessibleChildChangedEvent_getChildAccessible:
  * @event: a pointer to the #AccessibleEvent being queried.
  *
- * Queries an #AccessibleEvent for an #Accessible whose meaning depends
- *         on the event's context, that is, the event type and details.
- *         Not all events have context strings, in which case this 
- *         query returns NULL.  
- *         Context #Accessible objects may be returned by, for instance:
- *         object:child-changed events, 
- *         in which case they represent the child added or deleted.
+ * Queries an #AccessibleEvent of type "object:children_changed"
+ *         to get a reference to the changed #Accessible.
  *         Note that context #Accessibles are not guaranteed to outlive
  *         event delivery, in which case this call may return %NULL
  *         even if the object existed at the time of dispatch.
  *
  * Returns: the context #Accessible for the event, or %NULL if
- *          there is no context #Accessible object for the event.
+ *          there is no longer a valid context #Accessible 
+ *          object for the event.
  **/
 Accessible *
-AccessibleEvent_getContextObject (const AccessibleEvent *e)
+AccessibleChildChangedEvent_getChildAccessible (const AccessibleEvent *e)
 {
   InternalEvent *foo = (InternalEvent *) e;
-  CORBA_any *any;
-  g_return_val_if_fail (foo, NULL);
-  g_return_val_if_fail (foo->data, NULL);
-  any = (CORBA_any *) foo->data;
-  if (any->_type == TC_CORBA_Object) 
-    return cspi_object_add (* (CORBA_Object *) any->_value);
-  else 
-    return NULL;
+  return cspi_internal_event_get_object (e);
 }
 
Index: registryd/registry.c
===================================================================
RCS file: /cvs/gnome/at-spi/registryd/registry.c,v
retrieving revision 1.54
diff -u -r1.54 registry.c
--- registryd/registry.c	20 Nov 2002 00:17:14 -0000	1.54
+++ registryd/registry.c	21 Nov 2002 15:32:39 -0000
@@ -41,7 +41,7 @@
 /* A pointer to our parent object class */
 static SpiListenerClass *spi_registry_parent_class;
 
-extern int _dbg = 0;
+int _dbg = 0;
 
 typedef enum {
   ETYPE_FOCUS,
@@ -69,10 +69,10 @@
 } SpiListenerStruct;
 
 static void
-spi_registry_set_debug (char *debug_flag_string)
+spi_registry_set_debug (const char *debug_flag_string)
 {
   if (debug_flag_string) 
-    _dbg = g_ascii_digit_value(debug_flag_string);
+    _dbg = (int) g_ascii_strtod (debug_flag_string, NULL);
 }
 
 SpiListenerStruct *
Index: test/event-listener-test.c
===================================================================
RCS file: /cvs/gnome/at-spi/test/event-listener-test.c,v
retrieving revision 1.13
diff -u -r1.13 event-listener-test.c
--- test/event-listener-test.c	19 Nov 2002 20:04:23 -0000	1.13
+++ test/event-listener-test.c	21 Nov 2002 15:32:39 -0000
@@ -275,7 +275,7 @@
   fprintf (stderr, "(detail) %s %s %d %d\n", event->type, s,
 	   event->detail1, event->detail2);
   if (s) SPI_freeString (s);
-  s = AccessibleEvent_getContextString (event);
+  s = AccessibleTextChangedEvent_getChangeString (event);
   fprintf (stderr, "context string %s\n", (s) ? s : "<nil>");
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]