Re: [g-a-devel]Changes for at-poke

["Padraig O'Briain" <Padraig Obriain Sun COM>]

Michael,

Thanks for your comments.

I have reworked the patch.

There are still problems when poking more than one application but that is work 
for another day.

Padraig

> Subject: Re: [g-a-devel]Changes for at-poke
> To: "Padraig O'Briain" <Padraig Obriain sun com>
> Cc: accessibility mailing list <gnome-accessibility-devel gnome org>
> Content-Transfer-Encoding: 7bit
> Mime-Version: 1.0
> 
> Hi Padraig,
> 
> On Tue, 2002-09-17 at 13:29, Padraig O'Briain wrote:
> > 1) Close the poke window for an application when the application terminates 
(bug 
> >  93213).
> > 2) Terminate at-poke application when its main window is closed, whether or 
not 
> > poke window is open or poked application is running.
> > 3) Fix leaks so that at-poke does not report leaks of accessibles as it 
> > terminates.
> 
> 	This is great stuff;
> 
> > Comments requested.
> 
> 	Havn't read it in great detail; but I have a few:
> 
> > +		is_remove = (g_strstr_len (event->type, -1,  "remove") != NULL);
> 
> 	brackets around the != NULL ')' not necessary, and confuse me:-)
> 
> > +		i = 0;
> > +		if (is_remove) {
> > +			for (l = cl->roots; l; l = l->next) {
> > +				if (i == event->detail1) {
> > +					g_signal_emit (cl, signals [ROOT_DIED], 
0, l->data);
> > +					Accessible_unref (l->data);
> > +					g_slist_remove_link (cl->roots, l);
> > +				}
> > +			}
> >  		}
> 
> 	I'm slightly unsure as to why i = 0, and never gets incremented here;
> and gets compared to event->detail.
> 
> 	Also, you leak the 'l' list node, g_slist_remove_link will not free the
> GSList element, you want g_slist_delete_link;
> 
> 	Also, if you delete the current node in the list, then the subsequent
> 'l->next' will point to something duff [ by fluke in this code it will
> be NULL so you'll hit the end of list prematurely ].
> 
> 	The standard idiom here is:
> 
> 	for (l = cl->roots; l; l = next) {
> 		next = l->next;
> 		...
> 		cl->roots = g_slist_remove_link (cl->roots, l);
> 	}	
> 
> 	Note also the re-assingment to cl->roots, in case we remove the first
> item - quite crucial.
> 
> 	Anyway code like this:
> 
> > -		for (l = cl->roots; l; l = l->next)
> > -			Accessible_unref (l->data);
> 
> 	Was dangerous anyway, since we could re-enter and touch cl->roots via
> the unref I imagine so ...
> 
> > -extern void poke (Accessible *accessible);
> > +extern void poke (Accessible *accessible, int index);
> 
> 	Didn't look far enough into it to understand the 'int index' there -
> but it looks somewhat flakey to me :-) surely we're poking an Accessible
> at the end of the day, and the index in the parent is a very dodgy and
> transitory thing over children-changed sort of events ?
> 
> 	Thanks again anyway,
> 
> 	Regards,
> 
> 		Michael.
> 
> -- 
>  mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot
> 

? config.h.in
? icons/Makefile
? icons/Makefile.in
? src/child-listener.c.NEW
cvs server: Diffing .
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/at-poke/ChangeLog,v
retrieving revision 1.34
diff -u -p -r1.34 ChangeLog
--- ChangeLog	13 Sep 2002 07:09:01 -0000	1.34
+++ ChangeLog	18 Sep 2002 08:04:17 -0000
@@ -1,3 +1,52 @@
+2002-09-17  Padraig O'Briain   <padraig obriain sun com>
+
+	* accessible-tree-model.c
+	(model_node_destroy): Remove call to Accessible_unref() as it is
+	called in mnode_destroy().
+	(model_node_sync_children): Add call to Accessible_unref() to
+	correspond to the call to Accessible_ref() made by call to 
+	Accessible_getChildAtIndex() when the accedssible was added to the list.
+	(accessible_tree_model_unref_node): Add call to mnode_destroy() to
+	fix leak.
+	(accessible_tree_model_dispose): Call model_node_destroy() on root
+	node to avoid leaks.
+
+	* child-listener.[ch]
+	(child_listener_global_event): This function is the event handler for 
+	children-changed event. If it is for the desktop check whether it is a
+	remove and if so, emit a "root_died" signal and remove the application
+	from the list. Use get_accessible_at_index() to determine which
+	application to remove.
+	(child_listener_create) New function which stores the GtkListStore
+	containing the list of application.
+
+	* main.c
+	(validate_up_down_linkage): Call Accessible_unref() on return from
+	Accessible_getParent() and Accessible_getChildAt_index() to avoid
+	leaking accessible objects.
+	(get_accessible_at_index): New function which returns the accessible
+	at the specified index in the GtkListStore.
+	(list_store_clear): New function which calls Accessible_unref() for
+	Accessible objects in the list store and calls gtk_list_store_clear().
+	(populate_app_list): Call list_store_clear() instead of 
+	gtk_list_store_clear().
+	(window_destroyed): New function which is defined as signal handler for
+	destroy signal on window for application.
+	(create_app_list): Free data structures on exit and 
+	call gtk_widget_destroy() for window, if it still exists.
+
+	* poke.c
+	Add poker_died variable to Poker structure. It is used to determine
+	whether the poke window or the main window was closed.
+	(root_died_cb): Set poker_died to TRUE.
+	(window_destroyed): New function which is defined as signal handler for
+	destroy signal on the poke window.
+	(poke): Add index argument to poke(). Use index argument in call to
+	child_listener_add_root().
+	Call gtk_widget_destroy() for window, if it still exists.
+	Call gtk_main_quit() if poker_died is not set so poke can close
+	if main window is closed.
+
 2002-09-13  Padraig O'Briain   <padraig obriain sun com>
 
 	* src/poke.c 
cvs server: Diffing glade
cvs server: Diffing icons
cvs server: Diffing src
Index: src/accessible-tree-model.c
===================================================================
RCS file: /cvs/gnome/at-poke/src/accessible-tree-model.c,v
retrieving revision 1.19
diff -u -p -r1.19 accessible-tree-model.c
--- src/accessible-tree-model.c	30 Aug 2002 09:57:40 -0000	1.19
+++ src/accessible-tree-model.c	18 Sep 2002 08:04:17 -0000
@@ -144,7 +144,6 @@ model_node_destroy (GtkTreeModel *model,
 
 	make_iter (model, node, &iter);
 	path = gtk_tree_model_get_path (model, &iter);
- 	Accessible_unref (MODEL_NODE (node)->accessible);
 
 	mnode_destroy (ACCESSIBLE_TREE_MODEL (model), node->data);
 	g_node_destroy (node);
@@ -227,6 +226,7 @@ model_node_sync_children (GtkTreeModel *
 			GNode *child;
 
 			child = model_node_new (model, l->data);
+			Accessible_unref (l->data);
 
 			g_node_insert_before (node, lnode, child);
 			model_node_inserted (tree_model, child);
@@ -528,8 +528,9 @@ accessible_tree_model_unref_node (GtkTre
 		return;
 
 	mnode->ref--;
-	if (mnode->ref == 0)
-		/* FIXME: leak here ? */ ;
+	if (mnode->ref == 0) {
+		mnode_destroy (ACCESSIBLE_TREE_MODEL (tree_model), mnode);
+	}
 }
 
 gboolean
@@ -614,6 +615,8 @@ accessible_tree_model_dispose (GObject *
 	AccessibleTreeModel *model;
 
 	model = ACCESSIBLE_TREE_MODEL (object);
+	if (model->root)
+		model_node_destroy (GTK_TREE_MODEL (model), model->root);
 
 	if (model->event_listener != NULL) {
 		SPI_deregisterGlobalEventListenerAll (
Index: src/child-listener.c
===================================================================
RCS file: /cvs/gnome/at-poke/src/child-listener.c,v
retrieving revision 1.3
diff -u -p -r1.3 child-listener.c
--- src/child-listener.c	21 Mar 2002 21:46:59 -0000	1.3
+++ src/child-listener.c	18 Sep 2002 08:04:17 -0000
@@ -19,9 +19,12 @@ enum {
 
 static guint signals [LAST_SIGNAL];
 static GObjectClass *parent_class;
+static ChildListener *listener = NULL;
 
 GType child_listener_get_type (void);
 
+extern Accessible *get_accessible_at_index (GtkListStore *list_store, int i);
+
 GNOME_CLASS_BOILERPLATE (ChildListener,
 			 child_listener,
 			 GObject,
@@ -35,28 +38,24 @@ child_listener_global_event (const Acces
 
 	if (event->source == cl->desktop) {
 		GSList *l;
-		GSList *copy = NULL;
+		gboolean is_remove;
+		Accessible *accessible;
 
+		is_remove = g_strstr_len (event->type, -1,  "remove") != NULL;
+		if (is_remove)
+			accessible = get_accessible_at_index (cl->list_store, event->detail1);
 		g_signal_emit (
 			cl, signals [APP_LIST_CHANGED], 0);
 
-		for (l = cl->roots; l; l = l->next) {
-			copy = g_slist_prepend (copy, l->data);
-			Accessible_ref (l->data);
-		}
-
-		for (l = copy; l; l = l->next) {
-			char *txt;
-
-			if (!(txt = Accessible_getName (l->data)))
-				g_signal_emit (cl, signals [ROOT_DIED], 0, l->data);
-			else
-				SPI_freeString (txt);
+		if (is_remove) {
+			for (l = cl->roots; l; l = l->next) {
+				if (l->data == accessible) {
+					g_signal_emit (cl, signals [ROOT_DIED], 0, l->data);
+					Accessible_unref (l->data);
+					cl->roots = g_slist_delete_link (cl->roots, l);
+				}
+			}
 		}
-
-		for (l = cl->roots; l; l = l->next)
-			Accessible_unref (l->data);
-		g_slist_free (copy);
 	} else
 		g_signal_emit (cl, signals [CHILDREN_CHANGED], 0, event->source);
 }
@@ -132,14 +131,21 @@ child_listener_class_init (ChildListener
 }
 
 ChildListener *
-child_listener_get (void)
+child_listener_create (GtkListStore *list_store)
 {
-	static ChildListener *listener = NULL;
-
-	if (!listener)
+	if (!listener)  {
 		listener = g_object_new (
-			child_listener_get_type (), NULL);
+		child_listener_get_type (), NULL);
+		listener->list_store = list_store;
+	} else {
+		g_warning ("ChildListener already created");
+	}
+	return listener;
+}
 
+ChildListener *
+child_listener_get (void)
+{
 	return listener;
 }
 
Index: src/child-listener.h
===================================================================
RCS file: /cvs/gnome/at-poke/src/child-listener.h,v
retrieving revision 1.3
diff -u -p -r1.3 child-listener.h
--- src/child-listener.h	21 Mar 2002 21:46:59 -0000	1.3
+++ src/child-listener.h	18 Sep 2002 08:04:17 -0000
@@ -12,6 +12,7 @@
 
 #include <cspi/spi.h>
 #include <glib-object.h>
+#include <gtk/gtkliststore.h>
 
 typedef struct {
 	GObject parent;
@@ -19,6 +20,7 @@ typedef struct {
 	GSList *roots;
 	Accessible *desktop;
 	AccessibleEventListener *el;
+	GtkListStore *list_store;
 } ChildListener;
 
 typedef struct {
@@ -32,6 +34,7 @@ typedef struct {
 				  Accessible    *accessible);
 } ChildListenerClass;
 
+ChildListener *child_listener_create      (GtkListStore  *list_store);
 ChildListener *child_listener_get         (void);
 void           child_listener_add_root    (ChildListener *listener,
 					   Accessible    *root);
Index: src/main.c
===================================================================
RCS file: /cvs/gnome/at-poke/src/main.c,v
retrieving revision 1.13
diff -u -p -r1.13 main.c
--- src/main.c	12 Sep 2002 08:52:13 -0000	1.13
+++ src/main.c	18 Sep 2002 08:04:17 -0000
@@ -27,6 +27,7 @@
 #include "child-listener.h"
 
 extern void poke (Accessible *accessible);
+extern Accessible *get_accessible_at_index (GtkListStore *list_store, int i);
 
 #define APP_COL_NAME   0
 #define APP_COL_DESCR  1
@@ -115,6 +116,8 @@ validate_up_down_linkage (Accessible *ac
 	parent = Accessible_getParent (child);
 
 	g_assert (parent == accessible);
+	Accessible_unref (child);
+	Accessible_unref (parent);
 }
 
 static void
@@ -139,6 +142,43 @@ application_clicked (GtkButton *button,
 	}
 }
 
+Accessible *
+get_accessible_at_index (GtkListStore *list_store, int i)
+{
+	GtkTreeIter iter;
+	GtkTreeModel *model = GTK_TREE_MODEL (list_store);
+	Accessible *accessible;
+	gboolean ret;
+
+	ret = gtk_tree_model_iter_nth_child (model, &iter, NULL, i);
+	g_return_val_if_fail (ret, NULL);
+
+	gtk_tree_model_get (model, &iter,
+			    APP_COL_HANDLE, &accessible,
+			    -1);
+	return accessible;
+}
+
+static void
+list_store_clear (GtkListStore *list_store)
+{
+	GtkTreeIter iter;
+	GtkTreeModel *model = GTK_TREE_MODEL (list_store);
+	Accessible *accessible;
+	gboolean ret;
+
+	ret = gtk_tree_model_get_iter_first (model, &iter);
+
+	while (ret) {
+		gtk_tree_model_get (model, &iter,
+				    APP_COL_HANDLE, &accessible,
+				    -1);
+		Accessible_unref (accessible);
+		ret = gtk_tree_model_iter_next (model, &iter);
+	}
+	gtk_list_store_clear (list_store);
+}
+
 static void
 populate_app_list (AppWindow *app_window)
 {
@@ -150,7 +190,7 @@ populate_app_list (AppWindow *app_window
 
 	apps = Accessible_getChildCount (desktop);
 
-	gtk_list_store_clear (app_window->list_store);
+	list_store_clear (app_window->list_store);
 	for (i = 0; i < apps; i++) {
 		char *name, *descr;
 		Accessible *child;
@@ -190,6 +230,12 @@ app_list_changed_cb (ChildListener *list
 }
 
 static void
+window_destroyed (GtkObject *obj)
+{
+	gtk_main_quit ();
+}
+
+static void
 create_app_list (AppWindow *app_window)
 {
 	app_window->list_store = 
@@ -243,6 +289,7 @@ application_window (void)
 	GClosure  *closure;
 	GladeXML  *app_xml;
 	AppWindow *app_window = g_new0 (AppWindow, 1);
+	ChildListener *cl;
 
 	app_xml = get_glade_xml ();
 	app_window->app_xml = app_xml;
@@ -289,17 +336,26 @@ application_window (void)
 	g_object_watch_closure (
 		G_OBJECT (app_window->window),
 		closure);
+	cl = child_listener_create (app_window->list_store);
 	g_signal_connect_closure (
-		child_listener_get (),
+		cl,
 		"app_list_changed",
 		closure, FALSE);
 
+	g_signal_connect (app_window->window,
+			  "destroy",
+			  G_CALLBACK (window_destroyed),
+			  NULL);
+			  
 	gtk_main ();
 
-	gtk_widget_destroy (app_window->window);
+	if (GTK_IS_WIDGET (app_window->window))
+		gtk_widget_destroy (app_window->window);
 
+	list_store_clear (app_window->list_store);
 	g_object_unref (app_xml);
 	g_free (app_window);
+	g_object_unref (cl);
 }
 
 static gboolean
Index: src/poke.c
===================================================================
RCS file: /cvs/gnome/at-poke/src/poke.c,v
retrieving revision 1.24
diff -u -p -r1.24 poke.c
--- src/poke.c	13 Sep 2002 07:09:02 -0000	1.24
+++ src/poke.c	18 Sep 2002 08:04:17 -0000
@@ -87,6 +87,7 @@ typedef struct {
 	Accessible   *selected;
 
 	AccessibleCoordType ctype;
+	gboolean      poker_died;
 } Poker;
 
 enum {
@@ -102,6 +103,7 @@ enum {
 	RELATION_PATH
 };
 
+
 static void
 relation_row_activated_cb (GtkTreeView       *tree_view,
 			   GtkTreePath       *path,
@@ -1111,15 +1113,26 @@ init_accessible_tree (Poker *poker, Acce
 static void
 root_died_cb (ChildListener *listener, Accessible *root, Poker *poker)
 {
+	poker->poker_died = TRUE;
 	if (root == poker->root_node)
 		gtk_main_quit ();
 }
 
+static void
+window_destroyed (GtkObject *obj, Poker *poker)
+{
+	if (!poker->poker_died) {
+		poker->poker_died = TRUE;
+ 		gtk_main_quit ();
+	}
+}
+
 void
 poke (Accessible *accessible)
 {
 	Poker *poker;
 	GtkWidget *widget;
+	gboolean poker_died;
 
 	poker = g_new0 (Poker, 1);
 	
@@ -1382,6 +1395,12 @@ poke (Accessible *accessible)
 			"focus_out_event",
 			G_CALLBACK (poker_window_activate_cb),
 			poker);
+
+		g_signal_connect (
+			poker->window,
+			"destroy",
+			G_CALLBACK (window_destroyed),
+			poker);
 	}
 
 	init_accessible_tree (poker, accessible);
@@ -1391,8 +1410,12 @@ poke (Accessible *accessible)
 
 	child_listener_remove_root (child_listener_get (), poker->root_node);
 
-	gtk_widget_destroy (poker->window);
-	
+	if (GTK_IS_WIDGET (poker->window))
+		gtk_widget_destroy (poker->window);
+	g_object_unref (poker->tree_model);	
 	g_object_unref (poker->xml);
+	poker_died = poker->poker_died;
 	g_free (poker);
+	if (!poker_died)
+		gtk_main_quit();
 }