Re: popup coordinates on keyboard shortcut; Was: Re: Come fix bugs! (Batteries included)

[Dave Camp <dave novell com>]

Thanks!  A few comments:

>                                                   GdkEventButton
> *event)
>  {
> +       GArray *selected_item_locations;
> +       gint count;
> +       GdkPoint point;
> +       GdkPoint window_origin;
> +       GdkPoint decoration_end;
> +       GdkPoint view_in_window;
> +       
> +       GtkWidget *toplevel;
> +       
>         g_assert (FM_IS_DIRECTORY_VIEW (view));
>  
>         /* Make the context menu items not flash as they update to
> proper disabled,
> @@ -6129,10 +6138,44 @@ fm_directory_view_pop_up_selection_conte
>          */
>         update_menus_if_pending (view);
>  
> +       point.x = EEL_DEFAULT_POPUP_MENU_DISPLACEMENT;
> +       point.y = EEL_DEFAULT_POPUP_MENU_DISPLACEMENT;
> +       
> +                       /* client -> root coordinates; there should be
> an easier way than that */
> +                       
> +                       toplevel = gtk_widget_get_toplevel (GTK_WIDGET
> (view));
> +                       if (toplevel) {
> +                               gdk_window_get_geometry (GTK_WIDGET
> (view)->window, &view_in_window.x, &view_in_window.y, 
> +                                       NULL, NULL, NULL);
> +                               gdk_window_get_root_origin
> (toplevel->window, &window_origin.x, &window_origin.y);
> +                               gdk_window_get_geometry
> (toplevel->window, &decoration_end.x, &decoration_end.y,
> +                                       NULL, NULL, NULL);
> +                                       
> +                               window_origin.x += decoration_end.x;
> +                               window_origin.y += decoration_end.y;
> +                               window_origin.x += view_in_window.x;
> +                               window_origin.y += view_in_window.y;

The easiest way I know of to get a widgets root origin is:

gdk_window_get_origin (GTK_WIDGET (widget)->window, &x, &y);
x += GTK_WIDGET (widget)->allocation.x;
y += GTK_WIDGET (widget)->allocation.y;

You probably want to constrain the popup to the visible portion if the
icon is out of the scroll view.

Also, this covers up the icon completely with the menu.  Maybe it should
be moved over a bit?

> +                                                                       
> +                               g_warning ("window origin %d %d",
> window_origin.x, window_origin.y);

Shouldn't have this in the final patch.

> +                               
> +                               point.x += window_origin.x;
> +                               point.y += window_origin.y;
> +                       } else {
> +                               /* hiss */
> +                       }

No need for this else clause - in this case should probably just pop up
at the mouse coordinates.
                    
> +               }
> +               g_array_free (selected_item_locations, TRUE);
> +       }
> +
>         eel_pop_up_context_menu (create_popup_menu 
>                                         (view,
> FM_DIRECTORY_VIEW_POPUP_PATH_SELECTION),
> -
> EEL_DEFAULT_POPUP_MENU_DISPLACEMENT,
> -
> EEL_DEFAULT_POPUP_MENU_DISPLACEMENT,
> +                                     point.x, point.y,
>                                       event);



> 
>  
> +static void
> +popup_menu_callback (GtkMenu *menu, gint *x, gint *y, gboolean
> *push_in, gpointer userdata)
> +{
> +       GdkPoint *point;
> +       
> +       point = (GdkPoint *)userdata;
> +       if (point) {
> +               if (point->x != EEL_DEFAULT_POPUP_MENU_DISPLACEMENT) {
> +                       *x = point->x;
> +               }
> +               
> +               if (point->y != EEL_DEFAULT_POPUP_MENU_DISPLACEMENT) {
> +                       *y = point->y;
> +               }
> +               
> +               /* FIXME push_in? whats that? */
> +               
> +               g_free (point);
> +       }
> +}
> +



I don't think the menu position func has once-and-only-once semantics,
so it isn't safe to free the position data there.  You should probably
save the point as object data:

g_object_set_data_full (G_OBJECT (menu), "menu-position", point,
(GDestroyNotify)g_free);

>  /**
>   * eel_pop_up_context_menu:
>   * 
> @@ -492,13 +513,15 @@ eel_pop_up_context_menu (GtkMenu       *m
>                               gint16          offset_y,
>                               GdkEventButton *event)
>  {
> -       GdkPoint offset;
> +       GdkPoint *offset;
>         int button;
>  
>         g_return_if_fail (GTK_IS_MENU (menu));
> +       
> +       offset = (GdkPoint *) g_new0 (GdkPoint, 1);
>  
> -       offset.x = offset_x;
> -       offset.y = offset_y;
> +       offset->x = offset_x;
> +       offset->y = offset_y;
>  
>         /* The event button needs to be 0 if we're popping up this
> menu from
>          * a button release, else a 2nd click outside the menu with
> any button
> @@ -517,8 +540,8 @@ eel_pop_up_context_menu (GtkMenu         *m
>         gtk_menu_popup (menu,                                   /*
> menu */
>                         NULL,                                   /*
> parent_menu_shell */
>                         NULL,                                   /*
> parent_menu_item */
> -                       NULL,
> -                       &offset,                                /*
> data */
> +                       (GtkMenuPositionFunc) popup_menu_callback,
> +                       offset,                         /* data */
>                         button,                                 /*
> button */
>                         event ? event->time : GDK_CURRENT_TIME); /*
> activate_time */