GNOME Bugzilla – Bug 624893
Make StScrollBar respect padding in its allocation functions
Last modified: 2010-09-10 02:08:01 UTC
Update get_preferred_height(), get_preferred_width(), and allocate() functions for StScrollBar to respect padding. Once that's done, we want to add some padding (4-6px) on the left of the vertical scrollbar in the chat notification (i.e. between the chat messages and the scrollbar).
Created attachment 168439 [details] [review] [PATCH 1/4] change parent class of StScrollBar from StBin to StWidget 1. It doesn't have child 2. It work like StWidget without child
Created attachment 168440 [details] [review] [PATCH 2/4] add get_preferred_[width/height] to StScrollBar
Created attachment 168441 [details] [review] [3/4] remove scrollbar-[width/height]
Created attachment 168442 [details] [review] [4/4] property for StScrollBar.trough should be set directly
Comment on attachment 168439 [details] [review] [PATCH 1/4] change parent class of StScrollBar from StBin to StWidget >+ st_theme_node_get_content_box (theme_node, box, &childbox); >+ >+ scroll_bar_allocate_children (bar, &childbox, flags); scroll_bar_allocate_children() already calls st_theme_node_get_content_box(), so with this change, wouldn't it account for padding twice?
Comment on attachment 168440 [details] [review] [PATCH 2/4] add get_preferred_[width/height] to StScrollBar >+ #define ADJUST_WIDTH_FOR_ACTOR(name) \ >+ _st_actor_get_preferred_width (priv->name, for_height, TRUE, \ >+ &tmin_width_p, &tnatural_width_p); \ >+ if (min_width_p && tmin_width_p > *min_width_p) \ >+ *min_width_p = tmin_width_p; \ >+ if (natural_width_p && tnatural_width_p > *natural_width_p) \ >+ *natural_width_p = tnatural_width_p; >+ >+ ADJUST_WIDTH_FOR_ACTOR (bw_stepper); Hm... I don't love the macros, but if you want to keep them, I'd say definitely don't use the same macro name twice for two different things (the two ADJUST_WIDTH_FOR_ACTORs and two ADJUST_HEIGHT_FOR_ACTORs). It could be something like, ADJUST_WIDTH_IF_LARGER and ADD_TO_WIDTH, eg. Also, I think it would be clearer if you passed priv->bw_stepper rather than passing just "bw_stepper" and gluing it to the "priv->" inside the macro.
Comment on attachment 168441 [details] [review] [3/4] remove scrollbar-[width/height] >1. They useless "They're" >2. hard get it value from get_preferred_[width/height] in StScrollBar Er? Maybe "We can get its value from ..." ? >+ clutter_actor_get_preferred_width (CLUTTER_ACTOR (priv->vscroll), -1, would probably be better to pass a for_height value into get_scrollbar_width() for it to pass to clutter_actor_get_preferred_width(). In st_scroll_view_get_preferred_height() you'd pass -1 for the for_height, but in st_scroll_view_get_preferred_width() you'd have a real for_height value to pass. >+ if (min_size > 0) >+ result = min_size; > > return result; Nah, just get rid of the DEFAULT_SCROLLBAR_WIDTH stuff; if the scrollbar requests 0, then give it 0. (Likewise on all points for get_scrollbar_height, of course.)
Comment on attachment 168442 [details] [review] [4/4] property for StScrollBar.trough should be set directly is there any particular benefit to this?
Created attachment 168841 [details] [review] [1/4] change parent class of StScrollBar from StBin to StWidget > scroll_bar_allocate_children() already calls st_theme_node_get_content_box(), > so with this change, wouldn't it account for padding twice? fixed
Created attachment 168842 [details] [review] [2/4] add get_preferred_[width/height] to StScrollBar
Created attachment 168843 [details] [review] [3/4] remove scrollbar-[width/height] > Er? Maybe "We can get its value from ..." ? They are useless.
(In reply to comment #8) > (From update of attachment 168442 [details] [review]) > is there any particular benefit to this? yes. It need for StScrollBar with padding.
Comment on attachment 168442 [details] [review] [4/4] property for StScrollBar.trough should be set directly ok to commit, but explain it in the commit message. eg, "We were previously putting the decorations for the trough on the StScrollBar itself, which won't work if the scroll bar has padding."
Comment on attachment 168842 [details] [review] [2/4] add get_preferred_[width/height] to StScrollBar >+ min = st_theme_node_get_min_width (theme_node); >+ if (min > *min_width_p) >+ *min_width_p = min; >+ if (min > *natural_width_p) >+ *natural_width_p = min; >+ >+ st_theme_node_adjust_preferred_width (theme_node, min_width_p, natural_width_p); actually, you don't need to do that; st_theme_node_adjust_preferred_width() does it for you. (It doesn't it slightly differently, but I believe the way it does it is correct.)
Comment on attachment 168843 [details] [review] [3/4] remove scrollbar-[width/height] >1. They are useless >2. hard get it value from get_preferred_[width/height] in StScrollBar #2 there is still ungrammatical, and I'm not sure what it's supposed to mean. (I think "We can get its value from get_preferred_[width/height] in StScrollBar" ?) >+ max = st_theme_node_get_min_width (theme_node); Should be get_max_width() here of course, but as in the previous patch, you don't need to do this, because st_theme_node_adjust_preferred_width() does it for you. >+ sb_width = get_scrollbar_width (ST_SCROLL_VIEW (actor), avail_height); >+ sb_height = get_scrollbar_height (ST_SCROLL_VIEW (actor), avail_width); This is not quite right... if the scrollview is CLUTTER_REQUEST_HEIGHT_FOR_WIDTH, then you want to pass -1 to get_scrollbar_width() and sb_width to get_scrollbar_height, and if it's WIDTH_FOR_HEIGHT, you want to pass -1 to get_scrollbar_height and sb_height to get_scrollbar_width. (I think...)
Created attachment 169836 [details] [review] add get_preferred_[width/height] to StScrollBar
Created attachment 169837 [details] [review] remove scrollbar-[width/height]