Re: [evolution-patches] Patch for bug 45826, please review

[Yiming Cao <maxx cao sun com>]

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.

-- 

Yiming Cao <maxx cao sun com>