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

[Not Zed <notzed ximian com>]

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.