GNOME Bugzilla – Bug 51042
flipping menu arrows
Last modified: 2011-02-04 16:16:03 UTC
This patch does: * Makes submenu indicator arrows point in the direction that the submenu will pop up (left or right), rather than always right. * Makes the submenu offset (the vertical offset of each sucessive submenu) a #define'd quantity, instead of magic in the middle. Someone should make this be user-customizable via .gtkrc etc. * Removes hard-coding of submenus cascade right, and #define's it. This should also be user settable (i18n issues? tfel-ot-thgir languages probably should default to cascade left). * Minor code clean up (moved an assignment that sometimes gets immediately by an 'if ...' into the 'else').
Created attachment 275 [details] [review] gzipped patch
Created attachment 960 [details] [review] Un-gziped version of above patch.
*** Bug 25713 has been marked as a duplicate of this bug. ***
*** Bug 90464 has been marked as a duplicate of this bug. ***
Adding Nils Barth to the CC: as the author of this patch.
* For the flipping the menu indicators, there is the basic usability question of "should they flip". [ The flipping here is - if the submenu was going to pop up to the left, then +------------+ |Item >| +------------+ +------------+ |Item <| +------------+ I think none of Windows, Qt, Mozilla, do this, so there isn't much precedent. Still, it might be a cool touch; though there are definitely implementation challenges. * The implementation of this feature has in the patch has the problem that it doesn't track changes in the arrow direction it just checks every time an expose event occurs. The easiest way of reproducing this is to take a torn off menu with submenus and move it around the screen ... you can get half the submenu arrow drawn one way and half drawn the other way. This particular reproduction mechanism isn't that hard to fix.. you watch for configure events on the tearoff toplevel, and when you get a configure event, you do: foreach menu_item in the menu: _gtk_menu_item_check_toplevel_reposition (menu_item) However, it's much harder to catch if the requisition of the child menu changes while the menu is up... that could also cause the direction of arrows to change. The chance of this happening enough may be low enough to be not worth worrying about. * The performance of this patch should be somewhat better than it was when the patch was first submitted since we cache widget requisitions; still I have some concerns about having to compute the requisitions of the submenus as well as the menu itself when initially showing a menu. * The submenu_offset issue is should probably be a separate bug ... if we don't decide to simply hard-code the value to zero, it should be a style property. (Red Hat cross-reference http://bugzilla.redhat.com/bugzilla/show_bu.cgi?id=66529. Garrett actually wants a horizontal offset as well as getting rid of the vertical offset.) * The issue of cascading left for RTL languages is part of bug 76219. Note that the way that things flip there is different from here since the arrow goes on the opposite side of the menu item as well as pointing the other way. * Couple of comments on one chunk of the patch: + /* The temporary requisition var is so that gtk_widget_size_request + * doesn't whine; otherwise we'd just have + * &(GTK_WIDGET (menu_item->submenu)->requisition) above. + */ + GTK_WIDGET (menu_item->submenu)->requisition.width = requisition.width; + GTK_WIDGET (menu_item->submenu)->requisition.height = requisition.height; + gtk_menu_item_position_menu (GTK_MENU (menu_item->submenu), + &dummy_a, &dummy_b, menu_item); - "just so it doens't whine is a bit deceptive" - passing in &widget->requisition is illegal because it doesn't properly take into account gtk_widget_set_usize(). You can pass in NULL if you don't want to get the requisition yourself. - Poking into the fields like that is both unnecessary and illegal. GTK+ will take care of it for you. (It's in gtk/gtksizegroup.c/do_size_request() these days if you don't believe me.) - I'd probably make gtk_menu_item_position_menu() allow NULL for x/y/push_in rather than adding dummy variables. We are religious about allowing NULL for out variables for public GTK+ entry points, though we don't always do it for internal functions unless specifically needed.
Red Hat reference above should have been http://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=66259. I've filed bug 96557 for the submenu offset issue; please use this bug only for the issue of flipping the direction of the submenu arrow based on where the child will come up.
Moving various feature enhancements to the 2.4.0 milestone.
I can only address Owen's first question regarding whether the arrows should be fixed from a usability perspective: in the screenshot shown here, the backwardness of the arrows make us seem a bit amateurish, no? http://primates.ximian.com/~anna/goofy-arrows.png . I mean, obviously, I lack the skills to fix this bug. Therefore, I am amateurish compared to what is needed here. But, Owen, you aren't. :) I know there's a lot of pressure now to get 2.2 out-- for 2.4, this seems like quite a worthy problem. Also, the wrongly aligned arrows must be disconcerting in left to right languages, no?
right-to-left cascades for right-to-left languages are a somewhat different subject (bug 96637). In that case, you put the accelerator and arrow on the left of the text and the image on the right. +-------------------------+ | < Q-Alt TIUQ *| +-------------------------+ The case being discussed here is running out of space for a left-to-right menu cascade. I'd have trouble believing that the current behavior is "amateurish" when it is the behavior of Windows, MacOS, Mozilla, Qt. The problem with the case you show partly that there is a three level cascade, which is a bit pathological to begin with. (Bad UI design, really) For only a two level cascade, it's clearer why the second level pops up on the other side of the menu. I think switching to a right-to-left language style RTL cascade when the pop up point for a context menu is near the right edge of the screen would be weird. Maybe it might make sense for menus that are permanently fixed to the right side of the screen (foot menu on a panel on the right side of the screen.) If you did that, how much of the menu item would you reverse? Just turn the arrow around (as in this bug report)? Move the arrow to the other side but leave the image/accelerator in place? Move everything? For the minor case of simply running out of space in a left-to-right cascade, menu arrow flipping is closely related to bug 58377 ... which is the question of keynav. I think there are convincing reasons not to flip the keynav (for one thing, [Left] would be the keynav for both parent and child) so my instinct is the arrow direction probably shouldn't flip either.
*** Bug 104102 has been marked as a duplicate of this bug. ***
This should all be done, except for flipping the arrow when running out of space, which as Owen mentioned, is related to flipping the keynav (bug 58377). Since we decided to not flip the keynav, I guess this bug should also be closed. Reopen if you disagree.
*** Bug 131112 has been marked as a duplicate of this bug. ***
*** Bug 346476 has been marked as a duplicate of this bug. ***