Request to apply patches resulting from this thread: http://foo-projects.org/pipermail/xfce4-dev/2007-September/023569.html
Created attachment 1372 Add functions to XfcePanelPlugin API to position menus/popups Patch to libxfce4panel adding: xfce_panel_plugin_menu_popup() xfce_panel_plugin_popup_direction() xfce_panel_plugin_position_popup() It doesn't differentiate between internal and external plugins, since I don't know if there's a reason to. I didn't know if the resulting docs look or read right. I didn't know where in the file to put these functions, so I put them at the end.
Created attachment 1373 Utilize the new XfcePanelPlugin functions for the launcher menu, arrow
Created attachment 1374 Utilize the new XfcePanelPlugin functions for the windowlist menu, arrow
To clarify: Comment #2 refers to the launcher patch. Comment #3 refers to the windowlist patch. -Diego
I also forgot any copyright info. The original menu positioning code was taken from the notes plugin (Mike 2007), and I think the rest is mine (2007).
Excellent! I have just committed the patches (rev. 26115) with only minor modifications (see reply to the ML). PS Unfortunately I also made some other cosmetic changes to windowlist.c, so it's a bit hard to see what I changed there. I should not have done that, but I really didn't want to change it back. Sorry.
Reopened for further discussion of the API. Some of the concerns raised by Nick: - 'Ugly API', for want of a better qualification, when combining arrow type (popup direction) with popup positioning. Separating them might be better. - Shouldn't this simply be handled by a GtkMenuPositionFunc instead of an entire xfce_panel_plugin_menu_popup()?
Alright, let's discuss. What are the things we want to make easier for the user: - calculate the position of a popup menu, be it a real #GtkMenu or a home-made #GtkWidget. - get the arrow type to use for such a menu, possibly also with an initial estimate when the menu has not yet been created. - it would be really nice to call xfce_panel_plugin_register_menu() on behalf of the user, because people tend to forget this. I have some reservations about providing a simple GtkMenuPositionFunc: - calling *_register_menu() from a positioning function doesn't feel right - why would you not want to call xfce_panel_plugin_popup_menu(), but still be happy with just a simple positioning function? I have a feeling people will either want a simple all-in-one solution, or they want to do more, requiring a custom positioning function anyway. So, with this in mind how about this alternative API? /* all-in-one convenience function * - menu_arrow_type may be NULL * - this function calls ..._register_menu() * - menu is positioned relative to attach widget (see gtk_menu_attach_to_widget()), or plugin widget. */ void xfce_panel_plugin_popup_menu( XfcePanelPlugin *plugin, GtkMenu *menu, gint button, guint32 time, GtkArrowType *menu_arrow_type); /* positioning function * - menu widget is positioned relative to attach_widget * - intended to be called from custom GtkMenuPositionFunc */ void xfce_panel_plugin_position_menu_widget ( XfcePanelPlugin *plugin, GtkWidget *menu_widget, GtkWidget *attach_widget, gint *x, gint *y); /* get arrow type for menu popup widget * if menu_widget is NULL return the most likely arrow type */ GtkArrowType xfce_panel_plugin_arrow_type_for_menu ( XfcePanelPlugin *plugin, GtkWidget *menu_widget, GtkWidget *attach_widget);
The positioning function, xfce_panel_plugin_position_menu_widget, should probably be xfce_panel_plugin_position_popup_widget. This would make sense to me, since you (Jasper) said a goal was to "calculate the position of a popup menu, be it a real #GtkMenu or a home-made #GtkWidget." Other than that small change, I think this proposed external API is an improvement. As a side note, I think the name "xfce_panel_plugin_popup_menu" is already used by the panel internally.
I vote for only 2 new functions. When the panel is not floating, we know exactly where it is positioned (side of the screen), so then we also know the side with 'free' space (left/right/top/bottom). For floating panels we can do the same when we compare the plugin position relative to the center of the screen. No magic here, and we're always right from the moment the plugin is realized. So lets grab that function from the launcher and put it in libxfce4panel ^_^. So, we now know the free position relative to the plugin (without arbitrary stuff), then we need to position the menu. Well this is even easier, since gtk helps with large menus. Inside the new GtkMenuPositionFunc we grap the direction from the GtkArrowType function above, we calculate the position of the top/left corner and voila we're done. We can even set the menu screen and register the menu inside this function, since there is nothing wrong with that IMHO. My respond on the patches: Always using the monitor geometry to calculate the menu position is useless since the XFCE_SCREEN_POSITION already does this for you with fixed panels. The g_object_set_data and pointer conversions are not needed and unreadable. The attach widget stuff is not needed when you send the XfcePanelPlugin as user_data (there is ALWAYS a realized plugin and that's what you need). void (GtkMenuPositionFunc) xfce_panel_plugin_position_menu (GtkMenu *menu, gint *x, gint *y, gboolean *push_in, XfcePanelPlugin *plugin); { /* usage: * gtk_menu_popup (menu, NULL, NULL, xfce_panel_plugin_position_menu, * panel_plugin, button, activate_time); * * See as: gtk_status_icon_position_menu */ /* steps: * - set screen and register */ * - get arrow direction from xfce_panel_plugin_arrow_type * - get widget size request * - position menu based on arrow type * - check if the menu fits on screen */ } GtkArrowType xfce_panel_plugin_arrow_type (XfcePanelPlugin *plugin) { /* code from launcher_plugin_calculate_arrow_type */ } For not menu widgets: IMHO not _really_ interesting atm, but they are able to use the GtkMenuPositionFunc too with some small changes inside the function. So, long enough comment for now....
Small note: I think it's not a good idea to position to menu relative to the attached widget. As example, take the launcher plugin: it feels more logical when the menu popups above the icon and arrow instead of the arrow and the icon of another launcher next to it (not to mention the popup timeout on the icon button). The popup menu is mostly used for 1 button situations anyway, so it only makes things easier to drop the attached widget stuff.
(In reply to comment #11) > Small note: I think it's not a good idea to position to menu relative to the > attached widget. As example, take the launcher plugin: it feels more logical > when the menu popups above the icon and arrow instead of the arrow and the icon > of another launcher next to it (not to mention the popup timeout on the icon > button). The popup menu is mostly used for 1 button situations anyway, so it > only makes things easier to drop the attached widget stuff. > Yes, I agree. I reached the same conclusion myself after considering your suggestions. So, I like your proposal, since it's a lot simpler and simplicity is good. It seems to work fine, so I can commit it if you don't see any problems. Diego, do you see any problems with Nicks proposal? I made the position function take a GtkWidget as first argument so it can me used to position arbitrary widgets as well as regular menus. That should make most people happy. Updated API: GtkArrowType xfce_panel_plugin_arrow_type_for_menu (XfcePanelPlugin *plugin); void xfce_panel_plugin_menu_position_func (GtkWidget *menu, gint *x, gint *y, gboolean *push_in, XfcePanelPlugin *plugin);
(In reply to comment #12) > I made the position function take a GtkWidget as first argument so it can me > used to position arbitrary widgets as well as regular menus. That should make > most people happy. The downside of this is acompile warning (argument 4 defintion of a wrong type, something like that). We could add a macro to fix this: For gtk_menu_popup usage: #define xfce_panel_plugin_position_menu(menu,x,y,push_in,panel_plugin) ((GtkMenuPositionFunc) xfce_panel_plugin_position_widget (GTK_MENU (menu), x, y, push_in, panel_plugin) Then we could rename the real function to xfce_panel_plugin_position_widget, and inside the function: if (G_LIKELY (GTK_IS_MENU (widget)) { xfce_panel_plugin_register_menu (panel_plugin, GTK_MENU (widget); gtk_menu_set_screen (GTK_MENU (widget); } That should do the trick to make it portable. > Updated API: > > GtkArrowType > xfce_panel_plugin_arrow_type_for_menu (XfcePanelPlugin *plugin); Maybe the _for_menu is confusing since it will be used for arrow button types too? > void > xfce_panel_plugin_menu_position_func (GtkWidget *menu, > gint *x, > gint *y, > gboolean *push_in, > XfcePanelPlugin *plugin); Normaly the _func part is only added when you set a function (gtk_tree_view_set_row_separator_func), not when the function is... uh... a function.
Mm after some thoughts, the #define is not a good idea. Better to something like this: void xfce_panel_plugin_position_widget (GtkWidget *widget, gint *x, gint *y, XfcePanelPlugin *plugin) { g_return_if_fail (GTK_IS_WIDGET (widget)); g_return_if_fail (XFCE_PANEL_IS_PLUGIN (plugin)); g_return_if_fail (GTK_WIDGET_REALIZED (plugin)); /* note: we don't need the push_in here do we? */ /* get arrow type, set x and y */ } void xfce_panel_plugin_position_menu (GtkMenu *menu, gint *x, gint *y, gboolean *push_in, XfcePanelPlugin *plugin) { g_return_if_fail (GTK_IS_MENU (menu)); g_return_if_fail (XFCE_PANEL_IS_PLUGIN (plugin)); g_return_if_fail (GTK_WIDGET_REALIZED (plugin)); /* register with the panel */ xfce_panel_plugin_register_menu (plugin, menu); /* popup on plugin screen */ gtk_menu_set_screen (menu, gtk_widget_get_screen (GTK_WIDGET (plugin))); /* get popup coordinates */ xfce_panel_plugin_position_widget (GTK_WIDGET (menu), x, y, plugin); /* push inside screen */ *push_in = TRUE; }
(In reply to comment #13) > (In reply to comment #12) > > I made the position function take a GtkWidget as first argument so it can me > > used to position arbitrary widgets as well as regular menus. That should make > > most people happy. > > The downside of this is acompile warning (argument 4 defintion of a wrong type, > something like that). Ok, I'll add a separate function to calculate the position, taking a widget as argument. > > Updated API: > > > > GtkArrowType > > xfce_panel_plugin_arrow_type_for_menu (XfcePanelPlugin *plugin); > > Maybe the _for_menu is confusing since it will be used for arrow button types > too? Yes, maybe ..._arrow_type is sufficient. I wanted to indicate that it is meant to be used for widgets that pop up a menu. ..._arrow_type_for_widgets_that_popup_a_menu() is definitely too long though ;-) > > Normaly the _func part is only added when you set a function > (gtk_tree_view_set_row_separator_func), not when the function is... uh... a > function. Hehe, yeah. I added it to indicate that is should not be called directly, but used as a callback instead. I don't think we have (m)any other functions in our API that do that.
One more thing. If I don't keep the menu inside the monitor myself, gtk will keep the menu window inside the screen, but it also creates ugly scroll buttons and the contents are not moved inside the window. Is that a gtk bug? It is important for vertical panels to do this properly. Other than that I'm ready to commit my changes if there are no further objections.
I'm not sure I agree with comment #11. What about a plugin like this: [ ........ very wide text entry field .............. ] [Button!] If Button! opened a menu, it would open far away from Button! (to the left). For this reason, I think xfce_panel_plugin_position_* should *optionally* use an attach_widget instead of the XfcePanelPlugin container.
(In reply to comment #17) > I'm not sure I agree with comment #11. > > What about a plugin like this: > > [ ........ very wide text entry field .............. ] [Button!] > > If Button! opened a menu, it would open far away from Button! (to the left). > For this reason, I think xfce_panel_plugin_position_* should *optionally* use > an attach_widget instead of the XfcePanelPlugin container. > If this is a real concern -- are there any plugins that could benefit from this now? -- I think I would prefer to add an extra argument to the custom xfce_panel_plugin_position_widget() function. This means that for special cases you need a custom GtkMenuPositionFunc, but the 'normal' case keeps its implementation simple (xfce_panel_plugin_menu_position_func()). Would that be a reasonable compromise?
(In reply to comment #18) > (In reply to comment #17) > > I'm not sure I agree with comment #11. > > > > What about a plugin like this: > > > > [ ........ very wide text entry field .............. ] [Button!] > > > > If Button! opened a menu, it would open far away from Button! (to the left). > > For this reason, I think xfce_panel_plugin_position_* should *optionally* use > > an attach_widget instead of the XfcePanelPlugin container. > > > > If this is a real concern -- are there any plugins that could benefit from this > now? -- I think I would prefer to add an extra argument to the custom > xfce_panel_plugin_position_widget() function. > > This means that for special cases you need a custom GtkMenuPositionFunc, but > the 'normal' case keeps its implementation simple > (xfce_panel_plugin_menu_position_func()). > > Would that be a reasonable compromise? > Hmm, actually, it is not so difficult to also check if there is an explicitly set attach widget... Damn, I'm so quick to change my mind these days...
Re: Comment #18, I agree that is reasonable. Re: Comment #19, I agree that any extra complications are probably minimal and easy to maintain. So, my vote would be to try to check if there's an explicitly set attach widget. If that proves to be in any way complicated, fall back to Comment #18's proposal.
Maybe as better explanation, here are the API docs as I have them now. Nick, what do you think? /** * xfce_panel_plugin_arrow_type: * @plugin : an #XfcePanelPlugin * * Determine the #GtkArrowType for a widget that opens a menu and uses * xfce_panel_plugin_menu_position_func() to position the menu. * * Returns: The #GtkArrowType to use. **/ /** * xfce_panel_plugin_position_widget: * @plugin : an #XfcePanelPlugin * @menu_widget : a #GtkWidget that will be used as popup menu * @attach_widget : a #GtkWidget relative to which the menu should be positioned * @x : return location for the x coordinate * @y : return location for the y coordinate * * The menu widget is positioned relative to @attach_widget. This function is * intended for custom menu widgets. * * For a regular #GtkMenu you should use xfce_panel_plugin_menu_position_func() * instead (as callback argument to gtk_menu_popup()). * * See also: xfce_panel_plugin_menu_position_func(). **/ /** * xfce_panel_plugin_menu_position_func: * @menu : a #GtkMenu * @x : return location for the x coordinate * @y : return location for the y coordinate * @push_in : keep inside the screen (see #GtkMenuPositionFunc) * @panel_plugin : a pointer to an #XfcePanelPlugin * * Function to be used as #GtkMenuPositionFunc in a call to gtk_menu_popup(). * As data argument it needs an #XfcePanelPlugin. * * The menu is normally positioned relative to the plugin. If you want the * menu to be positioned relative to another widget, you can use * gtk_menu_attach_to_widget() to explicitly set a 'parent' widget. * * As a convenience, xfce_panel_plugin_menu_position_func() calls * xfce_panel_plugin_register_menu() for the menu. * * <example> * void * myplugin_popup_menu (XfcePanelPlugin *plugin, GtkMenu *menu, GdkEventButton *ev) * { * gtk_menu_popup (menu, NULL, NULL, * xfce_panel_plugin_menu_position_func, plugin, * ev->button, ev->time ); * } * </example> * * For a custom widget that will be used as a popup menu, use * xfce_panel_plugin_position_widget() instead. * * See also: gtk_menu_popup(). **/
We I think Gtk does the right thing with the scroll buttons. We already popup the menu on the side with the most available space, offscreen widgets are useless and there is nothing more annoying then menus under your cursor on button press (because release could trigger the menu item). I agree with the optional attach widget stuff. And I still think xfce_panel_plugin_position_menu is a better name ;). So except for the name thingy, go for it!
Committed with revision 26118. Nick, feel free to adjust more to your liking. Diego, let me know (or reopen the bug) if there is something that doesn't work for you. Thanks guys, for the excellent cooperation!
Created attachment 1377 windowlist at_pointer fix
Created attachment 1378 Attach widget documentation error, allow NULL
Reopening for at_pointer fix and at least some discussion on attachment 1378 .
Attachment 1378 looks good to me, except for the weird indentation ;). 1377 should be fine too.
Diego, I committed your two patches with revision 26124. For 1378 I made a few minor changes: Don't change plugin to panel_plugin in xfce_panel_plugin_position_widget(), use _panel_return_if_fail (that should already have been done, although I can't remember why we made our own version exactly ;-) and finally I remove the NULL-check on attach_widget in xfce_panel_plugin_position_menu, since that check is now in _position_widget already. Thanks again!
The _panel_return_* functions are disabled on non-debug code, to speed things up a bit.