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

[Michael Meeks <michael ximian com>]

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