After this, we should mark "text-changed" as deprecated.

Created attachment 177284 [details] [review]
proposed patch

This would be the implementation. 

I'm not sure about using text-insert vs. text-inserted, etc...

Created attachment 179872 [details] [review]
Updated patch

Fixed patch. We don't want to break the ABI.

Created attachment 179874 [details] [review]
Patch for at-spi2 supporting these signals

As at-spi event for text-changed does include the inserted/removed text we could just handle these new atk signals and translate to the current at-spi event like in this example patch fot at-spi2

Created attachment 179895 [details] [review]
Updated patch for at-spi2 support for insert/remove

Updated patch to send at-spi event with "insert" or "delete" detail and keeping any other detail coming from atk (like "system" in firefox")

Review of attachment 179895 [details] [review]:

Two questions from Firefox developers. 

1.
819	// Add the insert and keep any detail coming from atk
820	minor = g_quark_to_string (signal_hint->detail);
821	if (minor)
822	minor = g_strconcat ("insert:", minor, NULL);
823	else
824	minor = g_strdup ("insert");
Is it possible to put ':' into the detail of the event, then the code would become:
minor = g_quark_to_string (signal_hint->detail);
minor = g_strconcat ("insert", minor, NULL);
No matter if minor is NULL.

2.
Who is responsible for freeing the text in param_values[3]? The application or bridge?

(In reply to comment #6)

> Is it possible to put ':' into the detail of the event, then the code would
> become:
> minor = g_quark_to_string (signal_hint->detail);
> minor = g_strconcat ("insert", minor, NULL);
> No matter if minor is NULL.

Yes
 
> 2.
> Who is responsible for freeing the text in param_values[3]? The application or
> bridge?

probably glib :). We are in a signal callback, so if this is internally copied by glib during signal emission/processing, it should be freed internally too. The original string used by the application on g_signal_emit should be free on the application side as normal.

(In reply to comment #6)

> Is it possible to put ':' into the detail of the event, then the code would
> become:

I think signal_pasrse_name() doesn't send ":" for detail, so the checking is necessary.

Comment on attachment 179895 [details] [review]
Updated patch for at-spi2 support for insert/remove

Thanks for the patch.  It looks good; just a couple of comment-related nits:

- I'd prefer /*...*/-style comments to match the rest of the code.

- I think that the comments for the new functions need fixing--then functions generate AT-SPI text-changed events, not text-insert.

Review of attachment 179872 [details] [review]:

I don't have build env on my hand, if you can confirm it can be built OK, then I am on with the patch. Feel free to commit the atk patch.

Comment on attachment 179872 [details] [review]
Updated patch

I've just committed per discussion with fer on IRC.




>diff --git a/atk/atkmarshal.list b/atk/atkmarshal.list
>index 649049b..0763ae8 100644
>--- a/atk/atkmarshal.list
>+++ b/atk/atkmarshal.list
>@@ -23,4 +23,6 @@
> #   BOOL	deprecated alias for BOOLEAN
> 
> VOID:INT,INT
>+VOID:INT,INT,STRING
>+VOID:INT,INT,INT,STRING
> VOID:STRING,BOOLEAN
>diff --git a/atk/atktext.c b/atk/atktext.c
>index 91b351c..bcf1121 100755
>--- a/atk/atktext.c
>+++ b/atk/atktext.c
>@@ -30,6 +30,9 @@ enum {
>   TEXT_CARET_MOVED,
>   TEXT_SELECTION_CHANGED,
>   TEXT_ATTRIBUTES_CHANGED,
>+  TEXT_INSERT,
>+  TEXT_REMOVE,
>+  TEXT_UPDATE,
>   LAST_SIGNAL
> };
> 
>@@ -168,7 +171,37 @@ atk_text_base_init (AtkTextIface *class)
> 		      atk_marshal_VOID__INT_INT,
> 		      G_TYPE_NONE,
> 		      2, G_TYPE_INT, G_TYPE_INT);
>-      
>+
>+      atk_text_signals[TEXT_INSERT] =
>+	g_signal_new ("text_insert",
>+		      ATK_TYPE_TEXT,
>+		      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
>+		      0,
>+		      (GSignalAccumulator) NULL, NULL,
>+		      atk_marshal_VOID__INT_INT_STRING,
>+		      G_TYPE_NONE,
>+		      3, G_TYPE_INT, G_TYPE_INT, G_TYPE_STRING);
>+
>+      atk_text_signals[TEXT_REMOVE] =
>+	g_signal_new ("text_remove",
>+		      ATK_TYPE_TEXT,
>+		      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
>+		      0,
>+		      (GSignalAccumulator) NULL, NULL,
>+		      atk_marshal_VOID__INT_INT_STRING,
>+		      G_TYPE_NONE,
>+		      3, G_TYPE_INT, G_TYPE_INT, G_TYPE_STRING);
>+
>+      atk_text_signals[TEXT_UPDATE] =
>+	g_signal_new ("text_update",
>+		      ATK_TYPE_TEXT,
>+		      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
>+		      0,
>+		      (GSignalAccumulator) NULL, NULL,
>+		      atk_marshal_VOID__INT_INT_INT_STRING,
>+		      G_TYPE_NONE,
>+		      4, G_TYPE_INT, G_TYPE_INT, G_TYPE_INT, G_TYPE_STRING);
>+
>       atk_text_signals[TEXT_CARET_MOVED] =
> 	g_signal_new ("text_caret_moved",
> 		      ATK_TYPE_TEXT,

Comment on attachment 179895 [details] [review]
Updated patch for at-spi2 support for insert/remove

Committed with minor comment tweaks.




>diff --git a/atk-adaptor/event.c b/atk-adaptor/event.c
>index cc1877a..07aeb97 100644
>--- a/atk-adaptor/event.c
>+++ b/atk-adaptor/event.c
>@@ -755,6 +755,8 @@ link_selected_event_listener (GSignalInvocationHint * signal_hint,
> /* 
>  * Handles the ATK signal 'Gtk:AtkText:text-changed' and
>  * converts it to the AT-SPI signal - 'object:text-changed'
>+ * This signal is deprecated by Gtk:AtkText:text-insert
>+ * and Gtk:AtkText:text-remove
>  *
>  */
> static gboolean
>@@ -788,6 +790,104 @@ text_changed_event_listener (GSignalInvocationHint * signal_hint,
>   return TRUE;
> }
> 
>+/* 
>+ * Handles the ATK signal 'Gtk:AtkText:text-insert' and
>+ * converts it to the AT-SPI signal - 'object:text-insert'
>+ *
>+ */
>+static gboolean
>+text_insert_event_listener (GSignalInvocationHint * signal_hint,
>+                            guint n_param_values,
>+                            const GValue * param_values, gpointer data)
>+{
>+  AtkObject *accessible;
>+  guint text_changed_signal_id;
>+  GSignalQuery signal_query;
>+  const gchar *name;
>+  gchar *minor, *text;
>+  gint detail1, detail2;
>+
>+  accessible = ATK_OBJECT (g_value_get_object (&param_values[0]));
>+  /* Get signal name for 'Gtk:AtkText:text-changed' so
>+   * we convert is to the AT-SPI signal - 'object:text-changed'
>+   */
>+  text_changed_signal_id = g_signal_lookup ("text-changed", G_OBJECT_TYPE (accessible));
>+  g_signal_query (text_changed_signal_id, &signal_query);
>+  name = signal_query.signal_name;
>+
>+
>+  // Add the insert and keep any detail coming from atk
>+  minor = g_quark_to_string (signal_hint->detail);
>+  if (minor)
>+    minor = g_strconcat ("insert:", minor, NULL);
>+  else
>+    minor = g_strdup ("insert");
>+
>+  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 (G_VALUE_TYPE (&param_values[3]) == G_TYPE_STRING)
>+    text = g_value_get_string (&param_values[3]);
>+
>+  emit_event (accessible, ITF_EVENT_OBJECT, name, minor, detail1, detail2,
>+              DBUS_TYPE_STRING_AS_STRING, text, append_basic);
>+  g_free (minor);
>+  return TRUE;
>+}
>+
>+/* 
>+ * Handles the ATK signal 'Gtk:AtkText:text-remove' and
>+ * converts it to the AT-SPI signal - 'object:text-insert'
>+ *
>+ */
>+static gboolean
>+text_remove_event_listener (GSignalInvocationHint * signal_hint,
>+                            guint n_param_values,
>+                            const GValue * param_values, gpointer data)
>+{
>+  AtkObject *accessible;
>+  guint text_changed_signal_id;
>+  GSignalQuery signal_query;
>+  const gchar *name;
>+  gchar *minor, *text;
>+  gint detail1, detail2;
>+
>+  accessible = ATK_OBJECT (g_value_get_object (&param_values[0]));
>+  /* Get signal name for 'Gtk:AtkText:text-changed' so
>+   * we convert is to the AT-SPI signal - 'object:text-changed'
>+   */
>+  text_changed_signal_id = g_signal_lookup ("text-changed", G_OBJECT_TYPE (accessible));
>+  g_signal_query (text_changed_signal_id, &signal_query);
>+  name = signal_query.signal_name;
>+
>+  minor = g_quark_to_string (signal_hint->detail);
>+
>+  // Add the delete and keep any detail coming from atk
>+  minor = g_quark_to_string (signal_hint->detail);
>+  if (minor)
>+    minor = g_strconcat ("delete:", minor, NULL);
>+  else
>+    minor = g_strdup ("delete");
>+
>+  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 (G_VALUE_TYPE (&param_values[3]) == G_TYPE_STRING)
>+    text = g_value_get_string (&param_values[3]);
>+
>+  emit_event (accessible, ITF_EVENT_OBJECT, name, minor, detail1, detail2,
>+              DBUS_TYPE_STRING_AS_STRING, text, append_basic);
>+  g_free (minor);
>+  return TRUE;
>+}
>+
>+
> /*---------------------------------------------------------------------------*/
> 
> /* 
>@@ -998,6 +1098,10 @@ spi_atk_register_event_listeners (void)
>                        "Gtk:AtkText:text-selection-changed");
>   add_signal_listener (text_changed_event_listener,
>                        "Gtk:AtkText:text-changed");
>+  add_signal_listener (text_insert_event_listener,
>+                       "Gtk:AtkText:text-insert");
>+  add_signal_listener (text_remove_event_listener,
>+                       "Gtk:AtkText:text-remove");
>   add_signal_listener (link_selected_event_listener,
>                        "Gtk:AtkHypertext:link-selected");
>   add_signal_listener (generic_event_listener,