GNOME Bugzilla – Bug 436533
Allow more space efficient scroll arrows placement
Last modified: 2008-10-08 02:17:27 UTC
On (physically) small screen the menu scroll arrows on both sides of the menu can take significant amount of (vertical) space which is away from the content. Especially when you want the scroll arrows to be big enough to be used with fingers on a touchscreen. Moving both scroll arrows to the same side of the menu would leave more space for the content. Following is a set of patches to refactor the various scroll arrows related size calculations to more localised functions and finally adds a style property which controls the location of the scroll arrows.
Created attachment 87679 [details] [review] [PATCH] Refactor arrow border size calculations The upper and lower border size is calculated independently and the upper/lower border value is used as appropriate. This enables later moving both scroll arrows to the same side of the menu with fewer modifications. * gtk/gtkmenu.c (get_arrows_border): New function to calculate the border sizes needed for the scroll arrows. (gtk_menu_realize, gtk_menu_size_allocate, gtk_menu_scroll_by, gtk_menu_position, gtk_menu_scroll_to, gtk_menu_scroll_item_visible, get_visible_size, get_menu_height, gtk_menu_real_move_scroll): Update callers. --- gtk/gtkmenu.c | 155 +++++++++++++++++++++++++++++---------------------------- 1 files changed, 78 insertions(+), 77 deletions(-)
Created attachment 87680 [details] [review] [PATCH] Refactor arrow visible area calculations * gtk/gtkmenu.c (get_arrows_visible_area): New function (gtk_menu_paint): Refactor arrow visible area calculations to a separate function --- gtk/gtkmenu.c | 93 +++++++++++++++++++++++++++++++++++++++----------------- 1 files changed, 65 insertions(+), 28 deletions(-)
Created attachment 87681 [details] [review] [PATCH] Refactor arrow sensitive area calculations * gtk/gtkmenu.c (get_arrows_sensitive_area): New function (gtk_menu_handle_scrolling): Refactor arrow sensitive area calculations to separate function --- gtk/gtkmenu.c | 67 +++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 42 insertions(+), 25 deletions(-)
Created attachment 87682 [details] [review] [PATCH] Move the scroll arrows to the bottom Having both scroll arrows on the same side of the menu leaves more space for the menu content, and with physically small screen and large menu items/scroll arrows the difference can be fairly noticeable. This adds a new "opposite-arrows" style property which controls whether the scroll arrows are on opposite sides (top and bottom) or both on the same side (bottom) I am not entirely happy with the property name as one could conceivably want to place both scroll arrows on the left side as well, to save even more vertical space (right edge would be awkward if there are submenus, top edge when used with fingers your hand would obscure the content.) * gtk/gtkmenu.c (gtk_menu_class_init): Add "opposite-arrows" style property (default=TRUE) to toggle scroll arrow location (get_arrows_border, get_arrows_visible_area, get_arrows_sensitive_area): Calculate the arrow positions when they're both at the bottom (get_double_arrows): !opposite-arrows implies double-arrows --- gtk/gtkmenu.c | 106 +++++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 84 insertions(+), 22 deletions(-)
Comment on attachment 87679 [details] [review] [PATCH] Refactor arrow border size calculations >diff --git a/gtk/gtkmenu.c b/gtk/gtkmenu.c >index 71de7d7..93cec35 100644 >--- a/gtk/gtkmenu.c >+++ b/gtk/gtkmenu.c >@@ -2007,6 +2007,21 @@ gtk_menu_style_set (GtkWidget *widget, > } > > static void >+get_arrows_border (GtkMenu *menu, GtkBorder *border) >+{ >+ guint scroll_arrow_height; >+ >+ gtk_widget_style_get (GTK_WIDGET (menu), >+ "scroll-arrow-vlength", &scroll_arrow_height, >+ NULL); >+ >+ border->top = menu->upper_arrow_visible ? scroll_arrow_height : 0; >+ border->bottom = menu->lower_arrow_visible ? scroll_arrow_height : 0; >+ >+ border->left = border->right = 0; >+} >+ >+static void > gtk_menu_realize (GtkWidget *widget) > { > GdkWindowAttr attributes; >@@ -2526,9 +2535,10 @@ gtk_menu_paint (GtkWidget *widget, > else if (event->window == menu->bin_window) > { > gint y = -border_y + menu->scroll_offset; >+ GtkBorder arrow_border; > >- if (menu->upper_arrow_visible && !menu->tearoff_active) >- y -= scroll_arrow_height; >+ get_arrows_border (menu, &arrow_border); >+ y -= arrow_border.top; what's with the !menu->tearoff_active condition here? > > gtk_paint_box (widget->style, > menu->bin_window, >@@ -4040,7 +4048,7 @@ gtk_menu_scroll_to (GtkMenu *menu, > guint vertical_padding; > guint horizontal_padding; > gboolean double_arrows; >- gint scroll_arrow_height; >+ GtkBorder arrow_border; > > widget = GTK_WIDGET (menu); > >@@ -4061,7 +4069,6 @@ gtk_menu_scroll_to (GtkMenu *menu, > gtk_widget_style_get (GTK_WIDGET (menu), > "vertical-padding", &vertical_padding, > "horizontal-padding", &horizontal_padding, >- "scroll-arrow-vlength", &scroll_arrow_height, > NULL); > > double_arrows = get_double_arrows (menu); >@@ -4088,11 +4095,13 @@ gtk_menu_scroll_to (GtkMenu *menu, > if (!menu->upper_arrow_visible || !menu->lower_arrow_visible) > gtk_widget_queue_draw (GTK_WIDGET (menu)); > >- view_height -= 2 * scroll_arrow_height; >- y += scroll_arrow_height; >- > menu->upper_arrow_visible = menu->lower_arrow_visible = TRUE; > >+ get_arrows_border (menu, &arrow_border); >+ y += arrow_border.top; >+ view_height -= arrow_border.top; >+ view_height -= arrow_border.bottom; >+ > if (offset <= 0) > priv->upper_arrow_state = GTK_STATE_INSENSITIVE; > else if (priv->upper_arrow_state == GTK_STATE_INSENSITIVE) >@@ -4149,8 +4158,8 @@ gtk_menu_scroll_to (GtkMenu *menu, > last_visible = menu->upper_arrow_visible; > menu->upper_arrow_visible = offset > 0; > >- if (menu->upper_arrow_visible) >- view_height -= scroll_arrow_height; >+ get_arrows_border (menu, &arrow_border); >+ view_height -= arrow_border.top; > > if ((last_visible != menu->upper_arrow_visible) && > !menu->upper_arrow_visible) >@@ -4168,8 +4177,9 @@ gtk_menu_scroll_to (GtkMenu *menu, > last_visible = menu->lower_arrow_visible; > menu->lower_arrow_visible = offset < menu_height - view_height; > >- if (menu->lower_arrow_visible) >- view_height -= scroll_arrow_height; >+ /* recheck the bottom border */ >+ get_arrows_border (menu, &arrow_border); >+ view_height -= arrow_border.bottom; this comment is useless, it says *what* the code does, which is redundant. please state *why* the code re-qeuries the border, shouldn't it be valid from the first query within this function? > > if ((last_visible != menu->lower_arrow_visible) && > !menu->lower_arrow_visible) >@@ -4184,8 +4194,7 @@ gtk_menu_scroll_to (GtkMenu *menu, > } > } > >- if (menu->upper_arrow_visible) >- y += scroll_arrow_height; >+ y += arrow_border.top; > } > > /* Scroll the menu: */ >@@ -4263,14 +4272,12 @@ gtk_menu_scroll_item_visible (GtkMenuShell *menu_shell, > { > guint vertical_padding; > gboolean double_arrows; >- gint scroll_arrow_height; > > y = menu->scroll_offset; > gdk_drawable_get_size (GTK_WIDGET (menu)->window, &width, &height); > > gtk_widget_style_get (GTK_WIDGET (menu), > "vertical-padding", &vertical_padding, >- "scroll-arrow-vlength", &scroll_arrow_height, > NULL); > > double_arrows = get_double_arrows (menu); >@@ -4286,23 +4293,22 @@ gtk_menu_scroll_item_visible (GtkMenuShell *menu_shell, > } > else > { >- arrow_height = 0; >- if (menu->upper_arrow_visible && !menu->tearoff_active) >- arrow_height += scroll_arrow_height; >- if (menu->lower_arrow_visible && !menu->tearoff_active) >- arrow_height += scroll_arrow_height; >+ GtkBorder arrow_border; >+ >+ get_arrows_border (menu, &arrow_border); >+ arrow_height = arrow_border.top + arrow_border.bottom; again, what's with the !menu->tearoff_active condition? get_arrows_border does not take care of that... > > if (child_offset + child_height > y + height - arrow_height) > { > arrow_height = 0; > if ((!last_child && !menu->tearoff_active) || double_arrows) >- arrow_height += scroll_arrow_height; >+ arrow_height += arrow_border.bottom; > > y = child_offset + child_height - height + arrow_height; > if (((y > 0) && !menu->tearoff_active) || double_arrows) > { > /* Need upper arrow */ >- arrow_height += scroll_arrow_height; >+ arrow_height += arrow_border.top; > y = child_offset + child_height - height + arrow_height; > } > /* Ignore the enter event we might get if the pointer is on the menu in general i like this patch because it tends to simplify the code and make it clearer which allocations are due to the top and which are due to the bottom arrow. however the above 3 or so issues need to be worked out before applying.
(In reply to comment #5) > > what's with the !menu->tearoff_active condition here? My mistake, somehow I missed those. Thanks for catching. Fixing. > >@@ -4168,8 +4177,9 @@ gtk_menu_scroll_to (GtkMenu *menu, > > last_visible = menu->lower_arrow_visible; > > menu->lower_arrow_visible = offset < menu_height - view_height; > > > >- if (menu->lower_arrow_visible) > >- view_height -= scroll_arrow_height; > >+ /* recheck the bottom border */ > >+ get_arrows_border (menu, &arrow_border); > >+ view_height -= arrow_border.bottom; > > this comment is useless, it says *what* the code does, which is redundant. > please state *why* the code re-qeuries the border, shouldn't it be valid > from the first query within this function? Right. It seemed useful when I wrote it :-) (Anyway, the reason is that updating *_arrow_visible affects the border size calculation.) Updating the patch.
Created attachment 87686 [details] [review] [PATCH] Refactor arrow border size calculations The upper and lower border size is calculated independently and the upper/lower border value is used as appropriate. This enables later moving both scroll arrows to the same side of the menu with fewer modifications. * gtk/gtkmenu.c (get_arrows_border): New function to calculate the border sizes needed for the scroll arrows. (gtk_menu_realize, gtk_menu_size_allocate, gtk_menu_scroll_by, gtk_menu_position, gtk_menu_scroll_to, gtk_menu_scroll_item_visible, get_visible_size, get_menu_height, gtk_menu_real_move_scroll): Update callers. --- gtk/gtkmenu.c | 163 ++++++++++++++++++++++++++++++--------------------------- 1 files changed, 86 insertions(+), 77 deletions(-)
Created attachment 87687 [details] [review] [PATCH] Refactor arrow visible area calculations * gtk/gtkmenu.c (get_arrows_visible_area): New function (gtk_menu_paint): Refactor arrow visible area calculations to a separate function --- gtk/gtkmenu.c | 94 ++++++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 67 insertions(+), 27 deletions(-)
Updated two patches (text conflicts) based on the comments. Attachment 87679 [details] and attachment 87680 [details] [review] are now obsolete but I can't edit them :-/
(In reply to comment #7) > Created an attachment (id=87686) [edit] > [PATCH] Refactor arrow border size calculations ok, this patch looks good and can go in now. allthough i think the comment wordings: + /* upper_arrow_visible affects the border, so check it now */ + /* lower_arrow_visible affects the bottom border, so recheck */ can still be improved, e.g.: /* arrow_visible flags may have changed, so requery the border */ or use "retrieve" or "get", etc. i'm saying that, because "check" or "recheck" is something i'd rather expect as an "if()" clause comment.
(In reply to comment #8) > Created an attachment (id=87687) [edit] > [PATCH] Refactor arrow visible area calculations > > > * gtk/gtkmenu.c (get_arrows_visible_area): New function > (gtk_menu_paint): Refactor arrow visible area calculations to a > separate function > --- > gtk/gtkmenu.c | 94 ++++++++++++++++++++++++++++++++++++++++---------------- > 1 files changed, 67 insertions(+), 27 deletions(-) this patch mostly also looks good except for a few glitches: - function arguments always go on their own line in gdK/gtk: +get_arrows_visible_area (GtkMenu *menu, + GdkRectangle *upper, GdkRectangle *lower, + gint *arrow_space) so lacks a newline after "upper," - gdk/gtk always puts spaces aroun doperators, so: + upper->width = width - 2*border_x; needs to read "2 * border_x". there are multiple cases of this - the code simplifies or unifies calculations in some places, which is good. however the calculation of: + border_x = CONTAINER (widget)->border_width + ...xthickness + hpad...; + border_y = CONTAINER (widget)->border_width + ...ythickness + vpad...; is duplicated now and may easily get out of sync. so this needs to be fixed. i think there are only 2 reasonable approaches to this: 1) calc border_x/border_y before get_arrows_visible_area() and pass them in as function arguments. 2) calc border_x/border_y inside get_arrows_visible_area() and pass them out alongside the other out-arguments. of these two, i suspect (1) to look cleaner.
(In reply to comment #12) > - the code simplifies or unifies calculations in some places, which is good. > however the calculation of: > + border_x = CONTAINER (widget)->border_width + ...xthickness + hpad...; > + border_y = CONTAINER (widget)->border_width + ...ythickness + vpad...; > is duplicated now and may easily get out of sync. so this needs to be fixed. > i think there are only 2 reasonable approaches to this: > 1) calc border_x/border_y before get_arrows_visible_area() and pass them in > as function arguments. > 2) calc border_x/border_y inside get_arrows_visible_area() and pass them out > alongside the other out-arguments. > of these two, i suspect (1) to look cleaner. Either way it looks bad. Either you have the calculations oddly split, or have tons of out-arguments. After playing with it some I chose to add one more GdkRectangle out-parameter to hold the border_{x,y},width,height values. This seems nice compromise, neatly separating the calculations from the rest without expanding the signature too much. Also fixed the other things you mentioned.
Created attachment 87871 [details] [review] [1/3] Refactor arrow visible area calculations * gtk/gtkmenu.c (get_arrows_visible_area): New function (gtk_menu_paint): Refactor arrow visible area calculations to a separate function --- gtk/gtkmenu.c | 86 ++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 55 insertions(+), 31 deletions(-)
Created attachment 87872 [details] [review] [2/3] Refactor arrow sensitive area calculations * gtk/gtkmenu.c (get_arrows_sensitive_area): New function (gtk_menu_handle_scrolling): Refactor arrow sensitive area calculations to separate function --- gtk/gtkmenu.c | 67 +++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 42 insertions(+), 25 deletions(-)
Created attachment 87873 [details] [review] [3/3] Move the scroll arrows to the bottom Having both scroll arrows on the same side of the menu leaves more space for the menu content, and with physically small screen and large menu items/scroll arrows the difference can be fairly noticeable. This adds a new "opposite-arrows" style property which controls whether the scroll arrows are on opposite sides (top and bottom) or both on the same side (bottom) I am not entirely happy with the property name as one could conceivably want to place both scroll arrows on the left side as well, to save even more vertical space (right edge would be awkward if there are submenus, top edge when used with fingers your hand would obscure the content.) * gtk/gtkmenu.c (gtk_menu_class_init): Add "opposite-arrows" style property (default=TRUE) to toggle scroll arrow location (get_arrows_border, get_arrows_visible_area, get_arrows_sensitive_area): Calculate the arrow positions when they're both at the bottom (get_double_arrows): !opposite-arrows implies double-arrows --- gtk/gtkmenu.c | 106 +++++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 84 insertions(+), 22 deletions(-)
(In reply to comment #14) > Created an attachment (id=87871) [edit] > [1/3] Refactor arrow visible area calculations > > > * gtk/gtkmenu.c (get_arrows_visible_area): New function > (gtk_menu_paint): Refactor arrow visible area calculations to a > separate function patch looks good, should get tested and can go in then. (In reply to comment #15) > Created an attachment (id=87872) [edit] > [2/3] Refactor arrow sensitive area calculations > > > * gtk/gtkmenu.c (get_arrows_sensitive_area): New function > (gtk_menu_handle_scrolling): Refactor arrow sensitive area calculations > to separate function this one also looks ok, except that it still lacks a newline after "upper,": static void +get_arrows_sensitive_area (GtkMenu *menu, + GdkRectangle *upper, GdkRectangle *lower) can go in once that's fixed. i think Bratsche will take care of that.
With patches #1 and #2 I get a compile error on line 2555 of gtkmenu.c: if (menu->upper_arrow_visible && !menu->tearoff_active) y -= scroll_arrow_height; It looks like it should be maybe: if (menu->upper_arrow_visible && !menu->tearoff_active) y -= lower.height; Can you confirm if this is correct?
I think you missed attachment 87686 [details] [review] to get that error.
Sorry, it was unclear to me which patches needed testing and which ones were obsolete so Tim marked them all (including that one) obsolete except the last three. So now I built with the patches from comments #14, #15, #16, and #19. Are those correct, or should I be using any others as well?
Created attachment 87883 [details] [review] [1/4] Refactor arrow border size calculations The upper and lower border size is calculated independently and the upper/lower border value is used as appropriate. This enables later moving both scroll arrows to the same side of the menu with fewer modifications. * gtk/gtkmenu.c (get_arrows_border): New function to calculate the border sizes needed for the scroll arrows. (gtk_menu_realize, gtk_menu_size_allocate, gtk_menu_scroll_by, gtk_menu_position, gtk_menu_scroll_to, gtk_menu_scroll_item_visible, get_visible_size, get_menu_height, gtk_menu_real_move_scroll): Update callers. --- gtk/gtkmenu.c | 163 ++++++++++++++++++++++++++++++--------------------------- 1 files changed, 86 insertions(+), 77 deletions(-)
Created attachment 87884 [details] [review] [2/4] Refactor arrow visible area calculations * gtk/gtkmenu.c (get_arrows_visible_area): New function (gtk_menu_paint): Refactor arrow visible area calculations to a separate function --- gtk/gtkmenu.c | 86 ++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 55 insertions(+), 31 deletions(-)
Created attachment 87885 [details] [review] [3/4] Refactor arrow sensitive area calculations * gtk/gtkmenu.c (get_arrows_sensitive_area): New function (gtk_menu_handle_scrolling): Refactor arrow sensitive area calculations to separate function --- gtk/gtkmenu.c | 68 ++++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 43 insertions(+), 25 deletions(-)
Created attachment 87886 [details] [review] [4/4] Move the scroll arrows to the bottom Having both scroll arrows on the same side of the menu leaves more space for the menu content, and with physically small screen and large menu items/scroll arrows the difference can be fairly noticeable. This adds a new "opposite-arrows" style property which controls whether the scroll arrows are on opposite sides (top and bottom) or both on the same side (bottom) I am not entirely happy with the property name as one could conceivably want to place both scroll arrows on the left side as well, to save even more vertical space (right edge would be awkward if there are submenus, top edge when used with fingers your hand would obscure the content.) * gtk/gtkmenu.c (gtk_menu_class_init): Add "opposite-arrows" style property (default=TRUE) to toggle scroll arrow location (get_arrows_border, get_arrows_visible_area, get_arrows_sensitive_area): Calculate the arrow positions when they're both at the bottom (get_double_arrows): !opposite-arrows implies double-arrows --- gtk/gtkmenu.c | 106 +++++++++++++++++++++++++++++++++++++++++++++------------ 1 files changed, 84 insertions(+), 22 deletions(-)
The previous four patches should now have all the mentioned issues resolved. All of them compile for me. (Need to apply one on top of another.)
Okay, committed the first three patches to trunk as Tim requested. I'm waiting for Tim's approval for #4. Also, should these be applied to gtk-2-10 trunk as well?
Created attachment 111309 [details] [review] Move scroll arrows to the bottom This is an updated diff of the fourth, last outstanding patch. It works quite well for me. Can this go in?
That patch looks very sane to me.
just to prevent problems down the road, do we need to make this a tri-state { opposite, both-on-top, both-on-bottom } ?
Actually I agree with Matthias, this solution was tailored for maemo, and I think we want to go tri-state for upstream. Is there any enum we could use for this or does it need a new one?
Created attachment 112227 [details] [review] Use an enum This patch implements 'arrow-placement', a GtkArrowPlacement, which also allows for arrows only at the top. Note that this contains a bug, which causes the menu items to disappear from the visible part of the popup when the mouse hovers items in certain ways, but apart from that seems to work fine. At this point I don't see what causes this, but I'm uploading it anyway.
I'm sure Mitch will be happy to investigate that, since he was the last one to touch scrolling menus...
Created attachment 117936 [details] [review] Updated patch Updates - applies on top of current trunk - reordered and renamed enum values to { BOTH, START, END } so they don't imply any orientation - remove #include "gtkenums.h" (and btw gtkalias.h must be always the last include) - fixed get_arrows_border() to not leave members of the returned GtkBorder uninitialized - fixed indentation of switch() blocks I think the changes to get_arrows_border() have fixed the misbehavior mentioned (disappearing menu items), but I never tried without my changes, so please test & review.
(In reply to comment #33) > Created an attachment (id=117936) [edit] > Updated patch > > Updates > > - applies on top of current trunk > - reordered and renamed enum values to { BOTH, START, END } so they don't > imply any orientation > - remove #include "gtkenums.h" (and btw gtkalias.h must be always the > last include) > - fixed get_arrows_border() to not leave members of the returned GtkBorder > uninitialized > - fixed indentation of switch() blocks > > I think the changes to get_arrows_border() have fixed the misbehavior > mentioned (disappearing menu items), but I never tried without my changes, > so please test & review. Just tried the patch with all orientations, it works perfectly now in all cases. Your get_arrows_border() fixes seem to have done it.
Please commit the latest patch then.
Bug 436533 – Allow more space efficient scroll arrows placement * gtk/gtkenums.h: Add GtkArrowPlacement * gtk/gtkmenu.c (gtk_menu_class_init), (get_arrows_border), (get_arrows_visible_area), (get_double_arrows), (get_arrows_sensitive_area): Implement GtkMenu::arrow-placement to allow scrolling arrows to be placed at the start, end or both Patch by Tommi Komulainen and myself