Refactor the API for the screen reader to notify listeners of its status

Remove atspi_text_notify_reading_position, and instead add a region_changed
signal. Also make the dbus API more like the API used for other events.

Per discussion at https://gitlab.gnome.org/GNOME/orca/issues/36

11 files changed, 178 insertions, 136 deletions

diff --git a/atspi/atspi-accessible-private.h b/atspi/atspi-accessible-private.h
index 496f7d79..b9b68b11 100644
--- a/atspi/atspi-accessible-private.h
+++ b/atspi/atspi-accessible-private.h
@@ -29,6 +29,7 @@
 G_BEGIN_DECLS
 
 #include "atspi-accessible.h"
+#include "atspimarshal.h"
 
 struct _AtspiAccessiblePrivate
 {
diff --git a/atspi/atspi-accessible.c b/atspi/atspi-accessible.c
index 2a0eda10..b2f26915 100644
--- a/atspi/atspi-accessible.c
+++ b/atspi/atspi-accessible.c
@@ -26,9 +26,63 @@
 #include "atspi-accessible-private.h"
 #include <string.h>
 
+enum {
+  REGION_CHANGED,
+  LAST_SIGNAL
+};
+
 static gboolean enable_caching = FALSE;
 static guint quark_locale;
 
+static guint atspi_accessible_signals[LAST_SIGNAL] = { 0, };
+
+static gboolean
+screen_reader_signal_watcher (GSignalInvocationHint *signal_hint,
+                              guint                  n_param_values,
+                              const GValue          *param_values,
+                              gpointer               data)
+{
+  GObject *object;
+  AtspiAccessible *accessible;
+  GSignalQuery signal_query;
+  const char *name;
+  DBusMessage *signal;
+  DBusMessageIter iter, iter_struct, iter_variant, iter_array;
+  dbus_int32_t detail1, detail2;
+  const char *detail = "";
+
+  object = g_value_get_object (param_values + 0);
+  g_return_val_if_fail (ATSPI_IS_ACCESSIBLE(object), FALSE);
+
+  g_signal_query (signal_hint->signal_id, &signal_query);
+  name = signal_query.signal_name;
+  detail1 = g_value_get_int (param_values + 1);
+  detail2 = g_value_get_int (param_values + 2);
+  accessible = ATSPI_ACCESSIBLE (object);
+
+  signal = dbus_message_new_signal (ATSPI_DBUS_PATH_SCREEN_READER,
+                                    ATSPI_DBUS_INTERFACE_EVENT_SCREEN_READER,
+                                    "RegionChanged");
+  dbus_message_iter_init_append (signal, &iter);
+  dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &detail);
+  dbus_message_iter_append_basic (&iter, DBUS_TYPE_INT32, &detail1);
+  dbus_message_iter_append_basic (&iter, DBUS_TYPE_INT32, &detail2);
+  dbus_message_iter_open_container (&iter, DBUS_TYPE_VARIANT, "(so)",
+                                    &iter_variant);
+  dbus_message_iter_open_container (&iter_variant, DBUS_TYPE_STRUCT, NULL,
+                                    &iter_struct);
+  dbus_message_iter_append_basic (&iter_struct, DBUS_TYPE_STRING, &accessible->parent.app->bus_name);
+  dbus_message_iter_append_basic (&iter_struct, DBUS_TYPE_OBJECT_PATH, &accessible->parent.path);
+  dbus_message_iter_close_container (&iter_variant, &iter_struct);
+  dbus_message_iter_close_container (&iter, &iter_variant);
+  dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY, "{sv}",
+                                    &iter_array);
+  dbus_message_iter_close_container (&iter, &iter_array);
+  dbus_connection_send (_atspi_bus (), signal, NULL);
+  dbus_message_unref (signal);
+  return TRUE; 
+}
+
 static void
 atspi_action_interface_init (AtspiAction *action)
 {
@@ -189,6 +243,8 @@ atspi_accessible_finalize (GObject *object)
 #endif
 
   G_OBJECT_CLASS (atspi_accessible_parent_class)->finalize (object);
+
+  /* TODO: remove emission hook */
 }
 
 static void
@@ -200,6 +256,33 @@ atspi_accessible_class_init (AtspiAccessibleClass *klass)
   object_class->finalize = atspi_accessible_finalize;
 
   quark_locale = g_quark_from_string ("accessible-locale");
+
+  /**
+   * AtspiAccessible::region-changed:
+   * @atspiaccessible: the object which received the signal
+   * @arg1: an integer specifying the current offset of the text being read,
+   *        if the object is textual.
+   * @arg2: an integer specifying the ending offset of the text being read,
+   *        if the object is textual.
+   *
+   * The signal "region-changed" is emitted by a screen reader to inndicate
+   * that it is now reading or tracking a new object, or, a new piece of
+   * text within an object. This allows a magnifier to gain the information
+   * needded to highlight the object that the screen reader is reading.
+   */
+  atspi_accessible_signals[REGION_CHANGED] =
+    g_signal_new ("region_changed",
+		  G_TYPE_FROM_CLASS (klass),
+		  G_SIGNAL_RUN_LAST,
+		  G_STRUCT_OFFSET (AtspiAccessibleClass, region_changed), 
+		  NULL, NULL,
+		  atspi_marshal_VOID__INT_INT,
+		  G_TYPE_NONE,
+		  2, G_TYPE_INT, G_TYPE_INT);
+
+  g_signal_add_emission_hook (atspi_accessible_signals[REGION_CHANGED], 0,
+                              screen_reader_signal_watcher, NULL,
+                              (GDestroyNotify) NULL);
 }
 
 /**
diff --git a/atspi/atspi-accessible.h b/atspi/atspi-accessible.h
index a6b687c3..a17cf5ee 100644
--- a/atspi/atspi-accessible.h
+++ b/atspi/atspi-accessible.h
@@ -64,6 +64,8 @@ typedef struct _AtspiAccessibleClass AtspiAccessibleClass;
 struct _AtspiAccessibleClass
 {
   AtspiObjectClass parent_class;
+
+  void (*region_changed) (AtspiAccessible *accessible, gint current_offset, gint last_offset);
 };
 
 GType atspi_accessible_get_type (void); 
diff --git a/atspi/atspi-constants.h b/atspi/atspi-constants.h
index 25f1f796..e7c07dc1 100644
--- a/atspi/atspi-constants.h
+++ b/atspi/atspi-constants.h
@@ -1471,7 +1471,7 @@ typedef enum {
 #define ATSPI_DBUS_INTERFACE_SOCKET "org.a11y.atspi.Socket"
 
 #define ATSPI_DBUS_PATH_SCREEN_READER "/org/a11y/atspi/screenreader"
-#define ATSPI_DBUS_INTERFACE_SCREEN_READER "org.a11y.atspi.ScreenReader"
+#define ATSPI_DBUS_INTERFACE_EVENT_SCREEN_READER "org.a11y.atspi.Event.ScreenReader"
 
 #ifdef __cplusplus
 }
diff --git a/atspi/atspi-event-listener.c b/atspi/atspi-event-listener.c
index a89464d7..9b95c3dd 100644
--- a/atspi/atspi-event-listener.c
+++ b/atspi/atspi-event-listener.c
@@ -383,31 +383,23 @@ convert_event_type_to_dbus (const char *eventType, char **categoryp, char **name
   {
     gchar *matchrule;
     (*matchrule_array) = g_ptr_array_new ();
-    if (!strcmp (eventType, "object:text-reading-position"))
+    matchrule = g_strdup_printf ("type='signal',interface='org.a11y.atspi.Event.%s'", category);
+    if (name && name [0])
     {
-      matchrule = g_strdup ("type='signal',interface='org.a11y.atspi.ScreenReader',member='ReadingPosition',sender='org.a11y.Atspi.ScreenReader'");
-      g_ptr_array_add (*matchrule_array, matchrule);
+             gchar *new_str = g_strconcat (matchrule, ",member='", name, "'", NULL);
+             g_free (matchrule);
+             matchrule = new_str;
     }
-    else
+    if (detail && detail [0])
     {
-    matchrule = g_strdup_printf ("type='signal',interface='org.a11y.atspi.Event.%s'", category);
-      if (name && name [0])
-      {
-        gchar *new_str = g_strconcat (matchrule, ",member='", name, "'", NULL);
-        g_free (matchrule);
-        matchrule = new_str;
-      }
-      if (detail && detail [0])
-      {
-        gchar *new_str = g_strconcat (matchrule, ",arg0='", detail, "'", NULL);
-        g_ptr_array_add (*matchrule_array, new_str);
-        new_str = g_strconcat (matchrule, ",arg0path='", detail, "/'", NULL);
-        g_ptr_array_add (*matchrule_array, new_str);
-        g_free (matchrule);
-      }
-      else
-        g_ptr_array_add (*matchrule_array, matchrule);
+      gchar *new_str = g_strconcat (matchrule, ",arg0='", detail, "'", NULL);
+      g_ptr_array_add (*matchrule_array, new_str);
+      new_str = g_strconcat (matchrule, ",arg0path='", detail, "/'", NULL);
+      g_ptr_array_add (*matchrule_array, new_str);
+      g_free (matchrule);
     }
+    else
+      g_ptr_array_add (*matchrule_array, matchrule);
   }
   if (categoryp) *categoryp = category;
   else g_free (category);
@@ -470,7 +462,6 @@ listener_entry_free (EventListenerEntry *e)
  *            object:text-selection-changed
  *            object:text-changed
  *            object:text-caret-moved
- *            object:text-reading-position
  *            object:row-inserted
  *            object:row-reordered
  *            object:row-deleted
@@ -480,6 +471,9 @@ listener_entry_free (EventListenerEntry *e)
  *            object:model-changed
  *            object:active-descendant-changed
  *
+ *  (screen reader events)
+*             screen-reader:region-changed
+ *
  *  (window events)
  *
  *            window:minimize
@@ -977,19 +971,8 @@ _atspi_dbus_handle_event (DBusConnection *bus, DBusMessage *message, void *data)
   dbus_int32_t detail1, detail2;
   char *p;
   GHashTable *cache = NULL;
-  gboolean is_reading_position = 0;
 
-  if (dbus_message_is_signal (message, ATSPI_DBUS_INTERFACE_SCREEN_READER,
-      "ReadingPosition"))
-  {
-    if (strcmp (signature, "(so)ii") != 0)
-    {
-      g_warning ("Got invalid signature '%s' for ReadingPosition signal", signature);
-      return DBUS_HANDLER_RESULT_HANDLED;
-    }
-    is_reading_position = TRUE;
-  }
-  else if (strcmp (signature, "siiv(so)") != 0 &&
+  if (strcmp (signature, "siiv(so)") != 0 &&
       strcmp (signature, "siiva{sv}") != 0)
   {
    g_warning ("Got invalid signature %s for signal %s from interface %s\n", signature, member, category);
@@ -1007,16 +990,9 @@ _atspi_dbus_handle_event (DBusConnection *bus, DBusMessage *message, void *data)
       return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
     }
     category++;
-    if (!strcmp (category, "ScreenReader"))
-      category = "Object"; /* hack -- assume this is ReadingPosition */
-  }
-  if (is_reading_position)
-    e.source = _atspi_dbus_return_accessible_from_iter (&iter);
-  else
-  {
-    dbus_message_iter_get_basic (&iter, &detail);
-    dbus_message_iter_next (&iter);
   }
+  dbus_message_iter_get_basic (&iter, &detail);
+  dbus_message_iter_next (&iter);
   dbus_message_iter_get_basic (&iter, &detail1);
   e.detail1 = detail1;
   dbus_message_iter_next (&iter);
@@ -1025,10 +1001,7 @@ _atspi_dbus_handle_event (DBusConnection *bus, DBusMessage *message, void *data)
   dbus_message_iter_next (&iter);
 
   converted_type = convert_name_from_dbus (category, FALSE);
-  if (is_reading_position)
-    name = g_strdup ("text-reading-position");
-  else
-    name = convert_name_from_dbus (member, FALSE);
+  name = convert_name_from_dbus (member, FALSE);
   detail = convert_name_from_dbus (detail, TRUE);
 
   if (strcasecmp  (category, name) != 0)
@@ -1051,58 +1024,64 @@ _atspi_dbus_handle_event (DBusConnection *bus, DBusMessage *message, void *data)
     converted_type = p;
   }
   e.type = converted_type;
-  if (!is_reading_position)
-    e.source = _atspi_ref_accessible (dbus_message_get_sender(message), dbus_message_get_path(message));
-  if (e.source == NULL)
+  if (strcmp (category, "ScreenReader") != 0)
   {
-    g_warning ("Got no valid source accessible for signal %s from interface %s\n", member, category);
-    g_free (converted_type);
-    g_free (name);
-    g_free (detail);
-    return DBUS_HANDLER_RESULT_HANDLED;
+    e.source = _atspi_ref_accessible (dbus_message_get_sender(message), dbus_message_get_path(message));
+    if (e.source == NULL)
+    {
+      g_warning ("Got no valid source accessible for signal %s from interface %s\n", member, category);
+      g_free (converted_type);
+      g_free (name);
+      g_free (detail);
+      return DBUS_HANDLER_RESULT_HANDLED;
+    }
   }
 
-  if (!is_reading_position)
+  dbus_message_iter_recurse (&iter, &iter_variant);
+  switch (dbus_message_iter_get_arg_type (&iter_variant))
   {
-    dbus_message_iter_recurse (&iter, &iter_variant);
-    switch (dbus_message_iter_get_arg_type (&iter_variant))
+    case DBUS_TYPE_STRUCT:
     {
-      case DBUS_TYPE_STRUCT:
+      AtspiRect rect;
+      if (demarshal_rect (&iter_variant, &rect))
+      {
+        g_value_init (&e.any_data, ATSPI_TYPE_RECT);
+        g_value_set_boxed (&e.any_data, &rect);
+      }
+      else
       {
-        AtspiRect rect;
-        if (demarshal_rect (&iter_variant, &rect))
+        AtspiAccessible *accessible;
+ 	accessible = _atspi_dbus_return_accessible_from_iter (&iter_variant);
+        if (!strcmp (category, "ScreenReader"))
         {
-          g_value_init (&e.any_data, ATSPI_TYPE_RECT);
-          g_value_set_boxed (&e.any_data, &rect);
+          e.source = accessible;
         }
         else
         {
-          AtspiAccessible *accessible;
-	  accessible = _atspi_dbus_return_accessible_from_iter (&iter_variant);
           g_value_init (&e.any_data, ATSPI_TYPE_ACCESSIBLE);
           g_value_set_instance (&e.any_data, accessible);
           if (accessible)
             g_object_unref (accessible);	/* value now owns it */
         }
-        break;
       }
-      case DBUS_TYPE_STRING:
-      {
-        dbus_message_iter_get_basic (&iter_variant, &p);
-        g_value_init (&e.any_data, G_TYPE_STRING);
-        g_value_set_string (&e.any_data, p);
-        break;
-      }
-    default:
       break;
     }
-
-    dbus_message_iter_next (&iter);
-    if (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY)
+    case DBUS_TYPE_STRING:
     {
-      /* new form -- parse properties sent with event */
-      cache = _atspi_dbus_update_cache_from_dict (e.source, &iter);
+      dbus_message_iter_get_basic (&iter_variant, &p);
+      g_value_init (&e.any_data, G_TYPE_STRING);
+      g_value_set_string (&e.any_data, p);
+      break;
     }
+  default:
+    break;
+  }
+
+  dbus_message_iter_next (&iter);
+  if (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY)
+  {
+    /* new form -- parse properties sent with event */
+    cache = _atspi_dbus_update_cache_from_dict (e.source, &iter);
   }
 
   if (!strncmp (e.type, "object:children-changed", 23))
diff --git a/atspi/atspi-misc.c b/atspi/atspi-misc.c
index f6809996..fddf6010 100644
--- a/atspi/atspi-misc.c
+++ b/atspi/atspi-misc.c
@@ -71,7 +71,6 @@ const char *atspi_interface_table = ATSPI_DBUS_INTERFACE_TABLE;
 const char *atspi_interface_table_cell = ATSPI_DBUS_INTERFACE_TABLE_CELL;
 const char *atspi_interface_text = ATSPI_DBUS_INTERFACE_TEXT;
 const char *atspi_interface_cache = ATSPI_DBUS_INTERFACE_CACHE;
-const char *atspi_interface_screen_reader = ATSPI_DBUS_INTERFACE_SCREEN_READER;
 const char *atspi_interface_value = ATSPI_DBUS_INTERFACE_VALUE;
 
 static const char *interfaces[] =
@@ -741,11 +740,6 @@ process_deferred_message (BusDataClosure *closure)
   {
     _atspi_dbus_handle_event (closure->bus, closure->message, closure->data);
   }
-  if (dbus_message_is_signal (closure->message, atspi_interface_screen_reader,
-      "ReadingPosition"))
-  {
-    _atspi_dbus_handle_event (closure->bus, closure->message, closure->data);
-  }
   if (dbus_message_is_method_call (closure->message, atspi_interface_device_event_listener, "NotifyEvent"))
   {
     _atspi_dbus_handle_DeviceEvent (closure->bus,
@@ -843,11 +837,6 @@ atspi_dbus_filter (DBusConnection *bus, DBusMessage *message, void *data)
   {
     return defer_message (bus, message, data);
   }
-  if (dbus_message_is_signal (message, atspi_interface_screen_reader,
-      "ReadingPosition"))
-  {
-    return defer_message (bus, message, data);
-  }
   if (dbus_message_is_signal (message, "org.freedesktop.DBus", "NameOwnerChanged"))
   {
     defer_message (bus, message, data);
diff --git a/atspi/atspi-text.c b/atspi/atspi-text.c
index a84dbbb2..a360f56c 100644
--- a/atspi/atspi-text.c
+++ b/atspi/atspi-text.c
@@ -962,47 +962,6 @@ atspi_text_scroll_substring_to_point (AtspiText *obj,
   return retval;
 }
 
-/**
- * atspi_text_notify_reading_position:
- * @obj: the #AtspiText object being read.
- * @startOffset: the offset of the text currently being read.
- * @endOffset: the offset of the end of the text currently being read.
- *
- * Notifies interested listeners of the specific text that the screen
- * reader is currently reading. This allows a magnifier to synchronize with
- * the screen reader and highlight the text that is currently being read.
- */
-void
-atspi_text_notify_reading_position (AtspiText *obj,
-                                    gint startOffset,
-                                    gint endOffset)
-{
-  DBusMessage *signal;
-  AtspiAccessible *accessible;
-  DBusMessageIter iter, iter_struct;
-
-  g_return_if_fail (obj != NULL);
-
-  accessible = ATSPI_ACCESSIBLE(obj);
-
-  if (!_atspi_prepare_screen_reader_interface ())
-    return;
-
-  signal = dbus_message_new_signal (ATSPI_DBUS_PATH_SCREEN_READER,
-                                    ATSPI_DBUS_INTERFACE_SCREEN_READER,
-                                    "ReadingPosition");
-  dbus_message_iter_init_append (signal, &iter);
-  dbus_message_iter_open_container (&iter, DBUS_TYPE_STRUCT, NULL,
-                                    &iter_struct);
-  dbus_message_iter_append_basic (&iter_struct, DBUS_TYPE_STRING, &accessible->parent.app->bus_name);
-  dbus_message_iter_append_basic (&iter_struct, DBUS_TYPE_OBJECT_PATH, &accessible->parent.path);
-  dbus_message_iter_close_container (&iter, &iter_struct);
-  dbus_message_iter_append_basic (&iter, DBUS_TYPE_INT32, &startOffset);
-  dbus_message_iter_append_basic (&iter, DBUS_TYPE_INT32, &endOffset);
-  dbus_connection_send (_atspi_bus (), signal, NULL);
-  dbus_message_unref (signal);
-}
-
 static void
 atspi_text_base_init (AtspiText *klass)
 {
diff --git a/atspi/atspi-text.h b/atspi/atspi-text.h
index f789adb5..693c13a4 100644
--- a/atspi/atspi-text.h
+++ b/atspi/atspi-text.h
@@ -140,8 +140,6 @@ gboolean atspi_text_set_selection (AtspiText *obj, gint selection_num, gint star
 gboolean atspi_text_scroll_substring_to (AtspiText *obj, gint start_offset, gint end_offset, AtspiScrollType type, GError **error);
 
 gboolean atspi_text_scroll_substring_to_point (AtspiText *obj, gint start_offset, gint end_offset, AtspiCoordType coords, gint x, gint y, GError **error);
-
-void atspi_text_notify_reading_position (AtspiText *obj, gint startOffset, gint endOffset);
 G_END_DECLS
 
 #endif	/* _ATSPI_TEXT_H_ */
diff --git a/atspi/atspimarshal.list b/atspi/atspimarshal.list
new file mode 100644
index 00000000..2d9dd51c
--- /dev/null
+++ b/atspi/atspimarshal.list
@@ -0,0 +1,25 @@
+# see glib-genmarshal(1) for a detailed description of the file format,
+# possible parameter types are:
+#   VOID	indicates   no	 return	  type,	 or  no	 extra
+#		parameters. if VOID is used as	the  parameter
+#		list, no additional parameters may be present.
+#   BOOLEAN	for boolean types (gboolean)
+#   CHAR	for signed char types (gchar)
+#   UCHAR	for unsigned char types (guchar)
+#   INT		for signed integer types (gint)
+#   UINT	for unsigned integer types (guint)
+#   LONG	for signed long integer types (glong)
+#   ULONG	for unsigned long integer types (gulong)
+#   ENUM	for enumeration types (gint)
+#   FLAGS	for flag enumeration types (guint)
+#   FLOAT	for single-precision float types (gfloat)
+#   DOUBLE	for double-precision float types (gdouble)
+#   STRING	for string types (gchar*)
+#   PARAM	for GParamSpec or derived types	 (GParamSpec*)
+#   BOXED	for boxed (anonymous but reference counted) types (GBoxed*)
+#   POINTER	for anonymous pointer types (gpointer)
+#   OBJECT	for GObject or derived types (GObject*)
+#   NONE	deprecated alias for VOID
+#   BOOL	deprecated alias for BOOLEAN
+
+VOID:INT,INT
diff --git a/atspi/meson.build b/atspi/meson.build
index 2a6915d7..14c771b0 100644
--- a/atspi/meson.build
+++ b/atspi/meson.build
@@ -59,6 +59,13 @@ atspi_includedir = join_paths(get_option('prefix'), get_option('includedir'), 'a
 
 install_headers(atspi_headers, install_dir: atspi_includedir)
 
+# Marshallers
+atspi_marshals = gnome.genmarshal('atspimarshal',
+  sources: 'atspimarshal.list',
+  prefix: 'atspi_marshal',
+)
+atspi_marshal_h = atspi_marshals[1]
+
 atspi_enums = gnome.mkenums('atspi-enum-types',
                             sources: [ 'atspi-constants.h', 'atspi-types.h' ],
                             c_template: 'atspi-enum-types.c.template',
@@ -67,7 +74,7 @@ atspi_enums = gnome.mkenums('atspi-enum-types',
                             install_header: true)
 atspi_enum_h = atspi_enums[1]
 
-atspi = library('atspi', atspi_sources + atspi_enums,
+atspi = library('atspi', atspi_sources + atspi_enums + atspi_marshals,
                        version: soversion,
                        soversion: soversion.split('.')[0],
                        include_directories: [ root_inc, registryd_inc ],
diff --git a/doc/libatspi/libatspi-sections.txt b/doc/libatspi/libatspi-sections.txt
index af2ab719..f4a0eec1 100644
--- a/doc/libatspi/libatspi-sections.txt
+++ b/doc/libatspi/libatspi-sections.txt
@@ -32,7 +32,6 @@ atspi_text_get_text_attribute_value
 atspi_text_get_text_attributes
 atspi_text_scroll_substring_to
 atspi_text_scroll_substring_to_point
-atspi_text_notify_reading_position
 <SUBSECTION Standard>
 ATSPI_TEXT
 ATSPI_IS_TEXT