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.
yes, I find this bug also.
each time I control-clicked the item from the unselected state, the
number get added once.
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
|
