[evolution-patches] Re: GtkWidget events vs virtual methods?? [was Patch for bug 45826, please review]

[Not Zed <notzed ximian com>]

Hmm, good point.  I've bounced this to evolution-hackers.

I have no idea, does anybody who understands Gtk+ out there have any
idea on this?

I never understood why signals had class pointers in the first place.

 Michael


On Thu, 2003-07-10 at 17:37, Yiming Cao wrote:
> I agree with you. The signal handler should not be overrided, logically.
> The reason I did it like that is that the original author used the
> "button_press_event" that way, so to keep consistency, mmm...
> 
> I'd like to change it, but should I touch the original codes?
> 
> On Thu, 2003-07-10 at 09:07, Not Zed wrote: 
> > Ok, most of it looks fine now.
> > 
> > One bit i'm not too sure about/think might need changing is the use of
> > the popup_menu member of the widget class - its actually a signal
> > handler, not a virtual method.
> > 
> > I dont know what overriding the method like that does, but i dont think
> > it is correct (?).  It should probably just be signal_connect'd on each
> > instance.
> > 
> > On Mon, 2003-07-07 at 14:59, Yiming Cao wrote:
> > > Thanks a lot for the comments. Here's the new patch, together with
> > > Changelog, please review.
> > > 
> > > On Mon, 2003-07-07 at 10:54, Not Zed wrote:
> > > > A few comments, (also, remember a changelog).
> > > > 
> > > > 
> > > > +static void
> > > > +popup_menu_placement_callback(GtkMenu *menu, gint *x, gint *y, gboolean
> > > > *push_in, gpointer user_data)
> > > > +{
> > > > +       EMsgComposerAttachmentBar *bar;
> > > > +       GnomeIconList *icon_list;
> > > > +       GList *selection;
> > > > +       GnomeCanvasPixbuf *image;
> > > > +       GList *first_item;
> > > > +
> > > > +       g_return_if_fail (user_data != NULL);
> > > > 
> > > > This one isn't needed, as you know what you're calling it with.
> > > > 
> > > > +       bar = E_MSG_COMPOSER_ATTACHMENT_BAR (user_data);
> > > > +       icon_list = GNOME_ICON_LIST (user_data);
> > > > +
> > > > +       selection = gnome_icon_list_get_selection (icon_list);
> > > > +       g_return_if_fail (selection != NULL);
> > > > 
> > > > Since this is probably a possible normal-case and not a coding error,
> > > > this should just be a return.
> > > > 
> > > > +       first_item = g_list_first (selection);
> > > > +       g_return_if_fail (first_item != NULL);
> > > > 
> > > > All of this is completely redundant, first_item will be set to
> > > > selection, so first_item doesn't need to exist.
> > > > 
> > > > +       gdk_window_get_origin (((GtkWidget*) bar)->window, x, y);
> > > > 
> > > > This should probably come first, so x and y are always set to something.
> > > > 
> > > > +       image = gnome_icon_list_get_icon_pixbuf_item (icon_list,
> > > > (gint)first_item->data);
> > > > +       g_return_if_fail (image != NULL);
> > > > 
> > > > If its possible to have a null icon, this should just be a return, or
> > > > probably an if.
> > > > 
> > > > +       /* Put menu to the center of icon */
> > > > +       *x += (gint)(image->item.x1 + image->item.x2)/2;
> > > > +       *y += (gint)(image->item.y1 + image->item.y2)/2;
> > > > +
> > > > +}
> > > > 
> > > > On Fri, 2003-07-04 at 21:17, Yiming Cao wrote:
> > > > > Hi,
> > > > > 
> > > > > This patch fix bug 45862, "shift+f10 should activate popup menu when
> > > > > focus on attachment". Now in the composer, if you press S-F10 when the
> > > > > attachments are focused, popup menu will be available. The menu comes at
> > > > > the center point of the first focused attachment.