GNOME.org
 

Re: [Evolution-hackers] Re: [evolution-patches] patch for bug 48466

Hi, Not Zed

I am very sorry I have to ask you for more details.
I wonder if you can spare some to explain your meaning to me.
You said that the right way to do this is to destroy this widget, and the destroy action will cause a response signal to the widget, 
and suggest me to handle that case in the response_cb.
But I think this destroy action will not emit the "response" signal. And some tutorials and API refs also said that. Anyway, I make some experiment as you said, but find no response signal received. 

I think only destroying this widget will must get us leaked memory.
The attachment is the patch written as you said, I think it's not reasonable enough. 

Maybe there is something you haven't told me clear, or I misunderstand you?
Please give me some specific answer.

Thanks.
Charles Zhang

Charles Zhang wrote:


Thank you Not, but I'm not sure about what you said.
If a GtkDialog receives the "delete_event" signal, it will emit "response" with a response ID of GTK_RESPONSE_DELETE_EVENT. However, destroying a dialog does not emit the "response" signal; so We should be careful relying on "response" when using the GTK_DIALOG_DESTROY_WITH_PARENT flag. Fortunately, we doesn't use the GTK_DIALOG_DESTROY_WITH_PARENT flag in the glade at all. But anyway, if we use gtk_widget_destroy, and doesn't connect to any destroy signal, how could we catch the chance to free this Dialog_Data?
There must be something I've no idea about, so I gdb it, but to find that no response signal can be emitted from anywhere if I just call destroy. 
Perhaps I have to interrupt you again, I'm sorry.

Best Regards
Charles Zhang

Not Zed wrote:


Ok, but as i said, please use destroy.  It should be destroyed, thats
all there is to it.  It will be converted into a DESTROY dialog response
anyway, and you need to handle that case anyway, so use destroy.

On Tue, 2003-09-16 at 22:32 +0800, Charles Zhang wrote:


Hello Not

Thank you for your work, I have been to the gnome bugzilla and seen
#122356 as you told me, it is just as we have discussed. Thank you.

Following your advice, I studied the related parts of the code.
Now I still think we should force a "response" signal to the dialog,
but not simply widget_destroy it.
Because this dialog (the property dialog of attachment) is not created
with nothing else, in fact the dialog is a member of the struct Dialog_Data. 
Each time we popped up a dialog, we create a Dialog_Data to manage it,
and it is also in charge of some related handler.

In the ideal case, we click the ok or cancel or just close the dialog,
that will invoke the close_cb or ok_cb. What these funcion
do is to unref  the attachment, destroy the property dialog as one of
Dialog_Data's MEMBER, and free itself(Dialog_Data) finally.
Another case is that we close this compose window directly. If there
is a opened property dialog when we close the compose
window, the function can be invoked also because we have
gtk_signal_connect_while_alive (parent, "destroy", close_cb.....,
dialog).
Here parent is the compose window, close_cb is the handler, dialog is
the property dialog, when the dialog is alive, the compose window's
destroy signal can cause the close_cb function activity.

Then we come to the focus. When we remove an attachment with its
property dialog opened, if we just call widget_destroy,
the dialog can be destroyed but the Dialog_Data is missed by us. And
because we use this form of connect_while_alive, if we close this compose window, this Dialog_Data will not be freed either. 
Thus we leaked it.

So I think we can force a "response" signal to the dialog, and in its
signal handler "response_cb()" at line 382, it's the Dialog_Data's
turn
to destroy its member(the dialog) and free itself at last.

Are you with me? I have attached another patch, please review it if.

Best Regards
Charles Zhang


Not Zed wrote:



I've followed this up further.

- gnome bugzilla bug #122356, including a patch to fix it
- you can not do a g_list_reverse on the list returned by
get_selection!  It is static/internal data which must remain in the
correct order.  You will have to copy the list.
- so having said all that:
 - put the duplicate entry checking stuff in the original first-loop,
with a comment referencing the above bug
 - add a destroy call in remove_attachment if the editor is up

leave everything else as is.


On Mon, 2003-09-15 at 11:23 -0500, Not Zed wrote:


On Mon, 2003-09-15 at 22:32 +0800, Charles Zhang wrote:

Thank you, Not Zed.
I think it's necessary to do so.

First, in the existing code structure, it often do remove an item
more than once.
Because in the result of function gnome_iconlist_get_selection(),
the return GList contains the index of the selected item.
But in this index list, the item index information is duplicated if
you pressed 'CTRL' while selecting item (from unselected state) in
the iconlist widget.
For example, you insert three attachments, select the first one,
then multi-select the second one(PRESS CTRL WHILE DOING IT).
Thus you get the all three selected, and if you right click one of
the two selected icons and use the "remove" submenu, then these
three all disappear because what the function
gnome_iconlist_get_selection() return is 011, but not 01.
I don't know if I have showed it clearly enough. My libgnomeui's
version is 2.2.2-6.


Ok, well i confirmed this.  But this is definetly a bug in gnome icon
list, which should be fixed/relayed upstream.

Could you create a bug in gnome bugzilla about it, and reference the
bug in the code, saying exactly why the code works that way?

If you keep control-clicking an icon and leave it in the selected
state when you do a remove, the number gets added again and again.




Secondly, I think the existing code is less efficient, because it
makes another list, this is not necessary.
Why it makes this list, I'll coming to that in the third reason.



The third, in the existing code, the author uses two loop parts to
complete the remove action, he cannot do this remove action with one
loop because what the function gnome_iconlist_get_selection() return
is the index of the selected icon. So if he removes the item, the
other icon's index is not the order it has been. Thus he convert the
index to the pointer of the real object in the first loop part, and
remove them in the second loop part. If we use this list_reverse()
function, we don't have to consider the order at all.


Well that is not true.  You are considering order, list-reverse is a
re-ordering method, and only works because you are making assumptions
about the order of the data returned from the icon list, and how it
relates to the selection.  You are making big unpublished assumptions
about how removing an item will affect the selection indices.

It is the original code that isn't considering order at all.




The fourth,  because the  gnome_iconlist_get_selection() function
return the duplicated information of the selected item' index (maybe
it has been updated or fixed in libgnomeui's new version, but we
should have all versions work.), we should check the item index in
the loop part to see if it has been removed.


Again this only works assuming the assumption about the order of
indices is correct.  Which is ok, only if clearly marked, and
indicating it's a workaround.




Finally, we can easily get the dialog struct, but I cannot get it's
wrapper struct and free it. Then I have to use this method to get us
free the DialogData struct defined in line
283(e-msg-composer-attachment.c).


Huh?  there's nothing wrong there, except use destroy instead of
forcing a close signal on the dialogue, which isn't the way it should
be done.

Anyway, having said that:
- it's an icon list bug with specific behaviour
- submit a bug (and patch if possible) for the icon list
- make it very clear in the code that this is a workaround for a bug,
via a comment
- don't remove the existing 'this is a mess' comment - the same
applies to the new code, if it still applied to the code you removed,
since they're both doing the same messy stuff.
- use widget_destroy rather than forcing a response signal, infact
,move that logic into remove_attachment() anyway.

Z













Index: composer/e-msg-composer-attachment-bar.c
===================================================================
RCS file: /cvs/gnome/evolution/composer/e-msg-composer-attachment-bar.c,v
retrieving revision 1.67.4.4
diff -u -p -r1.67.4.4 e-msg-composer-attachment-bar.c
--- composer/e-msg-composer-attachment-bar.c	30 Jul 2003 12:59:06 -0000	1.67.4.4
+++ composer/e-msg-composer-attachment-bar.c	19 Sep 2003 11:55:32 -0000
@@ -192,9 +192,16 @@ static void
 remove_attachment (EMsgComposerAttachmentBar *bar,
 		   EMsgComposerAttachment *attachment)
 {
+	g_return_if_fail (E_IS_MSG_COMPOSER_ATTACHMENT_BAR (bar));
+	g_return_if_fail (g_list_find (bar->priv->attachments, attachment) != NULL);
+
 	bar->priv->attachments = g_list_remove (bar->priv->attachments,
 						attachment);
 	bar->priv->num_attachments--;
+	if (attachment->editor_gui != NULL) {
+		GtkWidget *dialog = glade_xml_get_widget (attachment->editor_gui, "dialog");
+		gtk_widget_destroy (dialog);
+	}
 	
 	g_object_unref(attachment);
 	
@@ -356,8 +363,15 @@ remove_selected (EMsgComposerAttachmentB
 	p = gnome_icon_list_get_selection (icon_list);
 	for ( ; p != NULL; p = p->next) {
 		num = GPOINTER_TO_INT (p->data);
-		attachment = E_MSG_COMPOSER_ATTACHMENT (g_list_nth (bar->priv->attachments, num)->data);
-		attachment_list = g_list_prepend (attachment_list, attachment);
+		attachment = E_MSG_COMPOSER_ATTACHMENT (g_list_nth_data (bar->priv->attachments, num));
+
+		/* We need to check if there are duplicated index in the return value of 
+		   gnome_icon_list_get_selection() because of gnome bugzilla bug #122356.
+		   FIXME in the future. */
+
+		if (g_list_find (attachment_list, attachment) == NULL) {
+			attachment_list = g_list_prepend (attachment_list, attachment);
+		}
 	}
 	
 	for (p = attachment_list; p != NULL; p = p->next)
Index: composer/e-msg-composer-attachment.c
===================================================================
RCS file: /cvs/gnome/evolution/composer/e-msg-composer-attachment.c,v
retrieving revision 1.46.4.1
diff -u -p -r1.46.4.1 e-msg-composer-attachment.c
--- composer/e-msg-composer-attachment.c	29 Aug 2003 05:57:25 -0000	1.46.4.1
+++ composer/e-msg-composer-attachment.c	19 Sep 2003 11:55:34 -0000
@@ -448,6 +448,7 @@ e_msg_composer_attachment_edit (EMsgComp
 				      disposition && !g_ascii_strcasecmp (disposition, "inline"));
 	
 	connect_widget (editor_gui, "dialog", "response", (GCallback)response_cb, dialog_data);
+
 #warning "signal connect while alive"	
 	/* make sure that when the composer gets hidden/closed that our windows also close */
 	parent = gtk_widget_get_toplevel (parent);
Index: composer/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/composer/ChangeLog,v
retrieving revision 1.544.2.13
diff -u -p -r1.544.2.13 ChangeLog
--- composer/ChangeLog	29 Aug 2003 05:57:25 -0000	1.544.2.13
+++ composer/ChangeLog	19 Sep 2003 11:55:34 -0000
@@ -10,6 +10,15 @@
 	and composer icon name to get the path of composer icon.
 	[#47781]
 
+2003-09-13  Charles Zhang  <charles zhang sun com>
+
+	* e-msg-composer-attachment.c (e_msg_composer_attachment_edit): 
+	Connect destroy signal to attachment properties dialog.
+	* e-msg-composer-attachment-bar.c (remove_attachment): Add some
+	assertion. Destroy properties dialog while removeing an attachment.
+	* e-msg-composer-attachment-bar.c (remove_selected): Fix a re-
+	remove-attachment bug. [#48466]
+
 2003-08-19  Jeffrey Stedfast  <fejj ximian com>
 
 	* Original patch from David Woodhouse, but modified a bit by me.
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]