GNOME Bugzilla – Bug 611740
Size negotation in StScrollview/MxScrollview
Last modified: 2010-03-11 20:10:37 UTC
Size negotiation for StScrollView/MxScrollView Height-for-width allocation =========================== Clutter's layout model has a two pass size request policy to support height-for-width and width-for-height (though width-for-height is pretty indifferently supported in practice.) So, the question is how does this model fair when we insert a scrolling container with automatic scrollbars into the mix? To be concrete, let's consider the example of: StScrollView StBoxLayout ClutterText with wrapping on (a height-for-width actor) If we have only an automatic horizontal scrollbar, and no vertical scrollbar, then we cleanly preserve the height-for-width behavior - the width determines whether we have a horizontal scrollbar, which is added onto the minimum and natural height. But if we add an automatic vertical scrollbar, then things get more complicated. The natural height depends on the width, but the minimum width potentially depends on the height - if we have a scrollbar, then we need to reserve space for that horizontally. The simplest thing to do here - and the only way to stick within the the straightforward 2-pass model - is to simply reserve the space for the vertical scrollbar unconditionally in the minimum width. What about the natural width? It turns out that we need to also add the vertical scrollbar to the natural width as well - for example, if we took the above situation and turned off wrapping and turned on horizontal ellipsization, then we don't want to get the situation where when the StScrollView gets its natural size horizontally that results in every line having the last few characters ellipsized. The appropriate behavior when the child is width-for-height rather than height-for-width can be derived by swapping the vertical and horizontal scrollbars in the above analysis. Different minimum sizes ======================= For a scrolled area we essentially have *three* different relevant sizes. Considering horizontal scrolling: - There is the absolute minimum width of the child - this is determined by the non-scrolled part of the child. (E.g., for a StBoxLayout, the CSS borders are outside the scrolled area and only the content area scrolls.) - There is the minimum width at which the child requires a scrollbar. This is would be the minimum width of the child when no scrollbar was involved. - There is the natural width of the child. In get_preferred_width(), we just need to know the absolute minimum width of the child and the natural width of the child. But when we get to get_preferred_height(), we need to know whether we are going to have to reserve space for the horizontal scrollbar or not - so we need to be able to compare the 'for_width' passed into get_preferred_height() to the minimum unscrolled size. A couple of approaches here: 1) We require the child scrollable, when it gets get_preferred_height() for a given for_width, to update its horizontal adjustment based on on the given for_width. Then by looking at the adjustment, we can figure out whether a horizontal scrollbar is needed and report the right preferred heights. 2) We ignore the absolute minimum size of the child and assume it will always be zero. The justification for this is that you really don't want to get down to anywhere near the absolute minimum size anyways - really tiny scrollbars controlling a really tiny scrolled area don't work. It's up to the application author to make sure that this case doesn't happen - either by packing the ScrollView appropriately or by explicitly setting a minimum size on it. Then the scrollable child reports the minimum scrolled size as the minimum size, and the natural size as the natural size. The second approach here has the considerable advantage that the adjustments of the scrollable completely drop out of the request and allocation process - StScrollView doesn't need to worry about them at all - it just needs to hook them up to the scrollbars. Dealing with the allocation =========================== By making the simplifications of always reserving space for the vertical scrollbar and ignoring the possibility of an absolute minimum size, we've managed to fit the scrolled view into the Clutter allocation model. The size of the ScrollView will be determined in a reasonable manner. But once we have that size - the allocate() method of the ScrollView needs to go ahead and determine what scrollbars are visible, so that we can provide the correct allocation to the child. This is tricky because the simplification of just reserving space for the vertical scrollbar no longer works - it's going to look funny if there is a gap to the right of the child. So, we now get back to the full situation where the width and height are interdependent in a tricky fashion. The fully accurate approach is add in scrollbars in a multistep process: Step 1: h1 = child_min_height(allocated_width) vscrollbar_visible = h1 < allocated_height hscrollbar_visible = child_min_width > allocated_width - [vscrollbar_width if visible] vscrollbar_visible = h1 < allocated_height - [hscrollbar_height if visible] If Step 1 set vscrollbar_visible to true, then do step 2: h2 = child_min_height(allocated_width - vscrollbar_width) hscrollbar_visible = child_min_width > allocated_width - vscrollbar_width Step 3: Allocate the child based on the determined values of hscrollbar_visible, vscrollbar_visible. The problem with this method is that we are potentially asking the child to lay out at widths of: allocated_width - vscrollbar_width [get_preferred_height()] allocated_width [allocate(), step 1] allocated_width - vscrollbar_width [allocate(), step 2] allocated_width - vscrollbar_width [allocate(), step 3] The effect of this is actually worse than it might look at first glance - because of the caching in ClutterActor, repeated calls to size request the child at the *same* width are optimized out. So, the effect isn't going from 1 layout to 2 layouts, it will frequently be from 0 layouts to 2 layouts. The thing that can help here is to add our own caching - since queue_relayout is virtualized, we should be able to know when the cache needs to be discarded because the child has changed. A different approach is to simply approximate the computation: Step 1: h1 = child_min_height(allocated_width - vscrollbar_width) vscrollbar_visible = h1 < allocated_height hscrollbar_visible = child_min_width > allocated_width - [vscrollbar_width if visible] vscrollbar_visible = h1 < allocated_height - [hscrollbar_height if visible] Step 2: Allocate the child based on the determined values of hscrollbar_visible, vscrollbar_visible. Then we've simplified the layout widths to: allocated_width - vscrollbar_width [get_preferred_height()] allocated_width - vscrollbar_width [allocate(), step 1] If we need a vscrollbar: allocated_width - vscrollbar_width [allocate(), step 2] else allocated_width [allocate(), step 3] So, in the case where we need the scrollbar, we've managed to always lay out the child at the same width, preserving caching. And the case where the child is big enough to need a scrollbar is the case where laying out the child is extra-expensive. Still, even here there would be an advantage of adding caching. The disadvantage is that we will sometimes add a vertical scrollbar when we don't need one - this won't be apparent for static layouts (since once we've added the vertical scrollbar we do need it!), but can be a bit weird if the user is interactively resizing and watching very closely. Hiding and showing the scrollbars ================================= In the above, it should be noted that we don't make a final determination about whether the scrollbars are visible or not until we get to allocate(). Calling clutter_actor_hide() and clutter_actor_show() on them at that point is going to cause Clutter to call queue_relayout() and that will produce warnings. If we add an idle, then they won't be hidden/shown until the next drawn frame. The best thing to do is to simply leave the the scrollbars visible in clutter terms, but only a) reserve space b) propagate pick and paint to them if we actually want to show them.
Created attachment 155378 [details] [review] Fixes for resizing This implements pretty much everything described above other than caching for efficiency and width-for-height layout. The test case here (it's pretty neat, give it a try!) probably should replace scrolling.js - one thing that scrolling.js tests that this doesn't is a scrolled box with CSS borders, but that could be added as another togglable option, or maybe a border could just always be added to the scrolled area. One thing not noted in the commit message is that with this patch you can clutter_actor_hide() the scrollbars and they become invisible scrollbars. I'm not really sure why you would do this rather than just setting adjustments on the StBoxLayout and using it directly, but the feature has been asked for and it wasn't much code to do. Suggested review strategy: - Read this bug report - Try out the interactive test case to get a sense for how things work - Read the docs I added to St.Scrollable - Review the changes to St.BoxLayout - Read/skim through St.ScrollView with the patch applied - the size negotation code in get_preferred_width/height/ allocate and the code that hooks up the adjustments is probably easiest to read from scratch rather than as changes. I wouldn't worry too much about figuring out all the details of size negotiation - they seem to work pretty well from the interactive test and if there are subtle bugs, we'll fix them as necessary.
*** Bug 611951 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > - Try out the interactive test case to get a sense for > how things work 3 bugs: * the minimum height/width lines show the minimum height/width of the StScrollView, not the minimum height/width of the FlowedBoxes, which is what you want to see * the natural height is sometimes the height of the "text", and sometimes 1 line more. (As you shrink the scrollview horizontally, the natural height gains a line right before the word-wrapping changes, and then loses it again right after the new wrap.) This may be a bug in the St code as opposed to the demo? It appears that FlowedBoxes._getPreferredHeight is called with a given for_width, then called again with a different for_width, and allocated the second width with the height corresponding to the first width * it crashes on exit and 1 not-actual-bug but still confusing: * it looks like there's no "natural width" line, but this is just because it's offscreen. > - Read the docs I added to St.Scrollable + * In response to get_preferred_width(), the scrollable should report + * the minimum width at which horizontal scrolling is needed for the + * preferred width, and natural width of the actor when not + * horizontally scrolled as the natural width. Very hard to parse if you don't already know what it means. How about: In response to get_preferred_width(), the scrollable should report a %min_width that is the smallest width the actor can have without needing to scroll horizontally, and a %natural_width that is its natural width when not scrolled. + * is a long line without any spaces.) As for width, get_preferred_height() + * should return the minimum size at which no scrolling is needed for the + * minimum height, and the natural size of the actor when not vertically scrolled + * as the natural height. As with get_preferred_width(), get_preferred_height() should return a %min_height that is the smallest height it can have (at the given %for_width) without needing to scroll vertically, and a %natural_height that is its natural height when not scrolled.
(In reply to comment #3) > (In reply to comment #1) > > - Try out the interactive test case to get a sense for > > how things work > > 3 bugs: > > * the minimum height/width lines show the minimum height/width of > the StScrollView, not the minimum height/width of the FlowedBoxes, > which is what you want to see Well, the sizing illustrator container illustrates the height of the child. In this case, what I was interested in debugging was the behavior of the ScrollView, so I wanted to see that. (If you don't have the ScrollView, then you see the size of the FlowedBoxes.) I guess SizingIllustrator could have a non-interactive mode where it doesn't have the drag handle and then we could pack: SizingIllustrator ScrollView SizingIllustrator FlowedBoxes the trouble is going be knowing what lines go with what actor. > * the natural height is sometimes the height of the "text", and > sometimes 1 line more. (As you shrink the scrollview horizontally, > the natural height gains a line right before the word-wrapping > changes, and then loses it again right after the new wrap.) This > may be a bug in the St code as opposed to the demo? It appears that > FlowedBoxes._getPreferredHeight is called with a given for_width, > then called again with a different for_width, and allocated the > second width with the height corresponding to the first width I think this is working as I intended it. During size negotation, we always reserve space for an automatic vertical scrollbar, but if we get allocated a size where we don't need a vertical scrollbar, we wrap the contents to that wider width. Otherwise an automatic vertical scrollbar is like an ALWAYS scrollbar that we just don't draw at times. It looks a bit funny here, but really, the natural height of the scrolled view is unlikely to be an interesting quantity in most cases. > * it crashes on exit Hmm, doesn't happen for me. Wonder if that is a Clutter 1.0 vs. Clutter 1.2 difference, or if its GC difference. If you can get a backtrace with 'run-test.sh -g' might be useful. > and 1 not-actual-bug but still confusing: > > * it looks like there's no "natural width" line, but this is > just because it's offscreen. Not sure if extra code to handle this makes sense, or how that would work. Guess we could check if the line is off the stage and show a label. > > - Read the docs I added to St.Scrollable > > + * In response to get_preferred_width(), the scrollable should report > + * the minimum width at which horizontal scrolling is needed for the > + * preferred width, and natural width of the actor when not > + * horizontally scrolled as the natural width. > > Very hard to parse if you don't already know what it means. How about: > > In response to get_preferred_width(), the scrollable should report > a %min_width that is the smallest width the actor can have without > needing to scroll horizontally, and a %natural_width that is its > natural width when not scrolled. > > + * is a long line without any spaces.) As for width, get_preferred_height() > + * should return the minimum size at which no scrolling is needed for the > + * minimum height, and the natural size of the actor when not vertically > scrolled > + * as the natural height. > > As with get_preferred_width(), get_preferred_height() should return > a %min_height that is the smallest height it can have (at the given > %for_width) without needing to scroll vertically, and a %natural_height > that is its natural height when not scrolled. Good suggestions.
(In reply to comment #4) > > * the minimum height/width lines show the minimum height/width of > > the StScrollView, not the minimum height/width of the FlowedBoxes, > > which is what you want to see > > Well, the sizing illustrator container illustrates the height of the child. In > this case, what I was interested in debugging was the behavior of the > ScrollView, so I wanted to see that. (If you don't have the ScrollView, then > you see the size of the FlowedBoxes.) OK... it did not occur to me at all that the current behavior might be intentional or potentially useful for anything. The red line on the left never changes and doesn't seem to correspond to anything (except a scrollbar that you don't even see most of the time). The red line on the top also doesn't seem to correspond to anything, and mysteriously changes position when you cross an invisible line that seems much more relevant than either of the visible red lines. Maybe have the scrollview's red minimum size lines be drawn min_width/min_height away from the right/bottom (making it clearer that they represent the size of the scrollbars), and then have another (green? purple?) line representing the FlowedBoxes's min_width. > > * it crashes on exit > > Hmm, doesn't happen for me. it's the "JavaScript objects were leaked." crash. maybe because of the known clutter 1.0 leaks?
Comment on attachment 155378 [details] [review] Fixes for resizing > diff --git a/src/st/st-box-layout.c b/src/st/st-box-layout.c >+ if (priv->hadjustment) >+ { >+ /* If we're scrolled, the parent calls us with the width that >+ * we'll actually get, which can be smaller than the minimum >+ * width that we give our contents. >+ */ >+ gfloat min_width; >+ >+ get_content_preferred_width (self, -1, &min_width, NULL); >+ for_width = MAX (for_width, min_width); >+ } This seems a little inconsistent with the stuff in st-private, where we expect the parent to do the work of making sure the request and allocation are consistent. In particular, _st_actor_get_preferred_height() could check ST_IS_SCROLLABLE(actor) and make this adjustment itself before calling clutter_actor_get_preferred_height(). > g_object_set (G_OBJECT (priv->vadjustment), > "lower", 0.0, >- "upper", natural_height, >+ "upper", min_height, The StScrollable docs should probably mention how to set the adjustment bounds/increments correctly as well. (I would expect upper to be "min_height - avail_height" here, but I guess StScrollBar and StScrollView are set up to assume that you don't do it that way.) >+ if (avail_width < min_width) >+ { >+ avail_width = min_width; >+ content_box.x2 = content_box.x1 + avail_width; >+ } meaning that in the "if (priv->is_vertical)" just below this, the "MAX (avail_height, min_height)" and "MAX (avail_width, min_width)" can now be just "avail_height" and "avail_width". > diff --git a/src/st/st-scroll-view.c b/src/st/st-scroll-view.c >+ GtkPolicyType hscrollbar_policy; why did we end up using GtkPolicyType here? It's not hard to create a new enum... >+update_shadow_visibility (StScrollView *scroll) >+ priv->top_shadow_visible = fabs (value - lower) > 0.1; >+ priv->bottom_shadow_visible = fabs (upper - value - page_size) > 0.1; The fabs() calls are unnecessary and wrong. In particular, if you turn on shadows in the test program, you'll see the bottom shadow appear when the scrollview height is a bit *larger* than the natural height of the FlowedBoxes (and then it disappears when the scrollview is exactly the natural height of the flowedboxes, and then it reappears again when it's smaller). The "0.1" also seems broken. It ought to be some fraction or multiple of page_size or page_increment, since adjustments don't have any inherent scale where "0.1" would be meaningful. >@@ -347,32 +439,82 @@ st_scroll_view_get_preferred_height (ClutterActor *actor, ... >+ /* Should theoretically use the min height of the hscrollbar, vscrollbar, not h >+ * that that we don't need any scrollbars, see if that works, and if that that >+ /* Pass one, try without a vertical scrollbar */ >+ clutter_actor_get_preferred_height (priv->child, MAX (avail_width, child_min_width), >+ &child_min_height, NULL); The MAX(avail_width, child_min_width) here is redundant with the part of st_box_layout_get_preferred_height() I was complaining about before >+ else >+ { >+ hscrollbar_visible = FALSE; >+ vscrollbar_visible = FALSE; >+ } (That's the no child case.) I'd expect it to be hscrollbar_visible = priv->hscrollbar_policy != GTK_POLICY_NEVER; vscrollbar_visible = priv->vscrollbar_policy != GTK_POLICY_NEVER; > /*Shadows*/ can you fix the spacing in the comment while you're there? >- child_box.y2 = MAX (child_box.y1, content_box.y1 + clutter_actor_get_height (priv->top_shadow)); >+ child_box.y2 = content_box.y1 + clutter_actor_get_height (priv->top_shadow); This change seems wrong, and inconsistent with the allocation of bottom_shadow. OTOH, with the bottom shadow, the use of clutter_actor_get_height() causes problems if you are forced to allocate it less than the CSS-specified height in one round, and then later make the scrollview taller; clutter_actor_get_height() will return the shorter height from the previous allocation rather than the original CSS height (though it eventually snaps back to full size. You can see this in the test program.). And that doesn't even get into the whole "assuming that the application will provide appropriate CSS to assign a default height" thing. >+disconnect_hadjustment (StScrollView *scroll) what's the benefit of splitting this and disconnect_vadjustment() out of child_[hv]adjustment_notify_cb? >+ st_scroll_bar_set_adjustment (ST_SCROLL_BAR(priv->hscroll), priv->hadjustment); space after BAR > /* Force scroll step if neede. */ "needed" >+ g_object_set (priv->hadjustment, > "step-increment", priv->column_size, StAdjustment really needs getters/setters... >+ st_scrollable_get_adjustments (ST_SCROLLABLE(priv->child), NULL, &priv->vadjustment); >+ st_scroll_bar_set_adjustment (ST_SCROLL_BAR(priv->vscroll), priv->vadjustment); spaces > /* Force scroll step if neede. */ same typo >+ update_shadow_visibility (scroll); That needs to move outside the if statement; if you're displaying shadows, and remove the vadjustment, then the shadows need to go away. Then again, I don't think "notify::hadjustment"/vadjustment ever get emitted anyway... Probably st-box-layout.c:scrollable_set_adjustments() needs to call g_object_notify()? >+ child_hadjustment_notify_cb (G_OBJECT (actor), NULL, self); >+ child_vadjustment_notify_cb (G_OBJECT (actor), NULL, self); If StBoxLayout was notifying these properties properly, then you could connect to the signals before calling parent_iface->add() and then the signal handlers would be invoked automatically. And then in st_scroll_view_remove(), why are you doing st_scrollable_set_adjustments() to NULL, when you didn't set them to non-NULL in add()? Wait, where DO the scrollables come from? (debug debug debug). Um... they get created when the scrollview calls st_scrollable_get_adjustments() on it, because the box layout assumes that if something is calling get_adjustments, then it's supposed to have adjustments? Um... and it fills in default values in those adjustments based on the stage size? How nice...
(In reply to comment #5) > (In reply to comment #4) > > > * the minimum height/width lines show the minimum height/width of > > > the StScrollView, not the minimum height/width of the FlowedBoxes, > > > which is what you want to see > > > > Well, the sizing illustrator container illustrates the height of the child. In > > this case, what I was interested in debugging was the behavior of the > > ScrollView, so I wanted to see that. (If you don't have the ScrollView, then > > you see the size of the FlowedBoxes.) > > OK... it did not occur to me at all that the current behavior might be > intentional or potentially useful for anything. The red line on the left never > changes and doesn't seem to correspond to anything (except a scrollbar that you > don't even see most of the time). The red line on the top also doesn't seem to > correspond to anything, and mysteriously changes position when you cross an > invisible line that seems much more relevant than either of the visible red > lines. Well, it's really an aid for someone debugging the widget that is being illustrated, and it was pretty useful for me to track down the behaviors for all the different policy combinations (e.g, NEVER/NEVER should be like the StScrollView isnt' there.) > Maybe have the scrollview's red minimum size lines be drawn > min_width/min_height away from the right/bottom (making it clearer that they > represent the size of the scrollbars), and then have another (green? purple?) > line representing the FlowedBoxes's min_width. I'll try fooling around a bit with two SizingIllustrators with different colors - I don't want to special case it to the point of doing something different from scrollbars, since I see SizingIllustrator as something useful for any time we are doing negotiation work in the future. > > > * it crashes on exit > > > > Hmm, doesn't happen for me. > > it's the "JavaScript objects were leaked." crash. maybe because of the known > clutter 1.0 leaks? OK, will ignore for now.
Created attachment 155766 [details] [review] Incremental fixes for scrollview work - Fix existing typos and spacing problems - Get preferred height, not current height, of shadows - Let shadows overflow don't clamp them when we have too little space - Remove a now-unecessary stray MAX() - Fix up scrollview visibility for the pathological case of no child - Disconnect from adjustments on remove() - Don't unset the adjustments on the child on remove(), since they already existed or were autocreated on add() (We should what we are doing and set the adjustments of the scrollbars on the child rather than setting the adjustments of the child, so we match GTK+'s scrolllable interface, but this at least makes it consistent instead of a weird mix.)
(In reply to comment #6) > (From update of attachment 155378 [details] [review]) > > diff --git a/src/st/st-box-layout.c b/src/st/st-box-layout.c > > >+ if (priv->hadjustment) > >+ { > >+ /* If we're scrolled, the parent calls us with the width that > >+ * we'll actually get, which can be smaller than the minimum > >+ * width that we give our contents. > >+ */ > >+ gfloat min_width; > >+ > >+ get_content_preferred_width (self, -1, &min_width, NULL); > >+ for_width = MAX (for_width, min_width); > >+ } > > This seems a little inconsistent with the stuff in st-private, where we expect > the parent to do the work of making sure the request and allocation are > consistent. In particular, _st_actor_get_preferred_height() could check > ST_IS_SCROLLABLE(actor) and make this adjustment itself before calling > clutter_actor_get_preferred_height(). I originally went with doing this adjustment in the parent (in fact, as you noted the code was still doing it in the parent), but then I thought about how GtkTextView works - if you have a GtkTextView it wraps the lines it can to the width that the GtkTextView is allocated, but the minimum (unscrolled) width can be longer than this and is determined by the longest line without any spaces. A child that wanted to behave like this would need know the actual width it is going to be allocated in its get_preferred_height() function so we can't hide it from the child. > > g_object_set (G_OBJECT (priv->vadjustment), > > "lower", 0.0, > >- "upper", natural_height, > >+ "upper", min_height, > > The StScrollable docs should probably mention how to set the adjustment > bounds/increments correctly as well. (I would expect upper to be "min_height - > avail_height" here, but I guess StScrollBar and StScrollView are set up to > assume that you don't do it that way.) I think we should just fix it (but as a separate patch) [ various fixes ] > >+ GtkPolicyType hscrollbar_policy; > > why did we end up using GtkPolicyType here? It's not hard to create a new > enum... Colin wanted to tie GTK+ and ST closer together rather than creating duplication. I didn't have a huge opinion and figured we could always add our own enum later. > >+update_shadow_visibility (StScrollView *scroll) > > >+ priv->top_shadow_visible = fabs (value - lower) > 0.1; > >+ priv->bottom_shadow_visible = fabs (upper - value - page_size) > 0.1; > > The fabs() calls are unnecessary and wrong. In particular, if you turn on > shadows in the test program, you'll see the bottom shadow appear when the > scrollview height is a bit *larger* than the natural height of the FlowedBoxes > (and then it disappears when the scrollview is exactly the natural height of > the flowedboxes, and then it reappears again when it's smaller). The point of the fabs calls is to handle the case where value is -0.00001 or something. Of course, we really should have strictly: lower <= value <= upper - page_size in all cases, whether or not the scrollable is visible, which would make the fabs() calls both unnecessary and harmless. I think this is part of my above "we should just fix it (but as part of a separate patch)" > The "0.1" also seems broken. It ought to be some fraction or multiple of > page_size or page_increment, since adjustments don't have any inherent scale > where "0.1" would be meaningful. I think the scale for a scrollable widget needs to be pixels (well, or clutter units of allocation, which could theoretically be scaled by a scale on a parent but usually aren't) > >- child_box.y2 = MAX (child_box.y1, content_box.y1 + clutter_actor_get_height (priv->top_shadow)); > >+ child_box.y2 = content_box.y1 + clutter_actor_get_height (priv->top_shadow); > > This change seems wrong, and inconsistent with the allocation of bottom_shadow. I'm not sure why it seems wrong - the MAX() seems entirely pointless. The inconsistency is there, yes. I think I meant to do both like the top actor - just let the shadows lap out rather than resizing them to squash them in. > OTOH, with the bottom shadow, the use of clutter_actor_get_height() causes > problems if you are forced to allocate it less than the CSS-specified height in > one round, and then later make the scrollview taller; Yeah, fixed to use get_preferred_height)( > And that doesn't even get > into the whole "assuming that the application will provide appropriate CSS to > assign a default height" thing. I think there is an assumption that if ST was used outside of gnome-shell, there would be a toolkit-default stylesheet. > >+disconnect_hadjustment (StScrollView *scroll) > > what's the benefit of splitting this and disconnect_vadjustment() out of > child_[hv]adjustment_notify_cb? Was meant to be called when removing the child. Added. > That needs to move outside the if statement; if you're displaying shadows, and > remove the vadjustment, then the shadows need to go away. > > Then again, I don't think "notify::hadjustment"/vadjustment ever get emitted > anyway... Probably st-box-layout.c:scrollable_set_adjustments() needs to call > g_object_notify()? There's a gigantic "taffy-pull" of problems when you start looking at how the adjustments work. It's just inescapably wrong to have StScrollable have both: get_adjustments() that auto-creates the children if they dont' exist set_adjustments() The combination doesn't make any sense. I think the right thing to do is match the scrollable "interface" on GTK+ and have a straightforward set_adjustments(), and have StScrollbar set the adjustments from the scrollbars on the child in add() and unset them in remove(), and never connect to notification. But since the code was more the other way around, I just left it that way and cleaned it up a bit in this patch.
Created attachment 155779 [details] [review] Fixes for setting up scrolling adjustments > bounds/increments correctly as well. (I would expect upper to be "min_height - > avail_height" here, but I guess StScrollBar and StScrollView are set up to > assume that you don't do it that way.) You fooled me for a little bit - but GtkAdjustment doesn't work this way either - 'lower <= value <= upper - page_size' for that as well. (Not that you couldn't do it the way you describe, but I don't think we want to deviate.) This patch has some small fixes and adds the documentation. ==== Fixes for setting up scrolling adjustments StScrollable: Document how to set adjustments StBoxLayout: Make sure that we always have upper >= lower + page_size, so that clamping works properly. Set the page_increment to be slightly less than the page_size so there is some overlap, as is customary. StScrollView: Remove unnecessary fabs() calls, rewrite expressions for additional clarity.
Created attachment 155807 [details] [review] Simplify handling of adjustments This is a radical simplification of the handling of adjustments; I'm pretty sure that the change to StScrollable/StBoxLayout is right, but am a little less positive that making the adjustment of the StScrollbar read-only makes sense. It is maximally simple and should work for almost any use case (except for linking scrollbars together which is sort of neat, but completely useless), but it might make more sense for StScrollview to own the adjustments and hook them up both to the scrollbars and to the scrollable. GTK+ works more like that. === The relationship between adjustments and scrollbars and scrollable widgets was much more complex than it needed to be. StScrollbar: Make a scrollbar own an adjustment, that can't be set. StScrollView: Simply get the adjustments from the scrollbars, set them on the child, remove unnecessary change notification. StBoxLayout: Remove auto-create of adjustments, just take the adjustments from the scrollbars and set them on the scrollable child. Notify for hadjustment/vadjustment properties. StScrollable: Document how adjustment setting works.
Comment on attachment 155766 [details] [review] Incremental fixes for scrollview work >- child_box.y2 = MAX (content_box.y1, content_box.y2 - sb_height); >+ child_box.y2 = content_box.y2; should be content_box.y2 - sb_height; the way it is now it draws the bottom shadow over the horizontal scrollbar. >+ disconnect_hadjustment(self); >+ disconnect_vadjustment(self); space before (
Comment on attachment 155779 [details] [review] Fixes for setting up scrolling adjustments >- priv->top_shadow_visible = fabs (value - lower) > 0.1; >- priv->bottom_shadow_visible = fabs (upper - value - page_size) > 0.1; >+ priv->top_shadow_visible = value > lower + 0.1; >+ priv->bottom_shadow_visible = value < upper - 0.1; You need to keep the "page_size" there. As it is now, if you turn on shadows in the test program, the bottom shadow is always visible, because even if you scroll all the way to the bottom, value is only (upper - page_size), which is less than (upper - 0.1). Also, it still needs to take vscrollbar_visible into account. value is always 0 when the scrollview doesn't scroll vertically, and 0 is less than (upper - 0.1), so the bottom shadow is now always visible when the scrollview is taller than its contents.
Comment on attachment 155807 [details] [review] Simplify handling of adjustments Did you intend for this patch to depend on bug 611944? > if (priv->adjustment) >- st_scroll_bar_set_adjustment (bar, NULL); >+ { >+ g_object_run_dispose (G_OBJECT (priv->adjustment)); >+ g_object_unref (priv->adjustment); >+ priv->adjustment = NULL; >+ } why do you have to run_dispose? (add a comment?)
(In reply to comment #14) > (From update of attachment 155807 [details] [review]) > Did you intend for this patch to depend on bug 611944? I don't think there is an inherent dependency, but yes, they probably do overlap/interact to some extent. > > if (priv->adjustment) > >- st_scroll_bar_set_adjustment (bar, NULL); > >+ { > >+ g_object_run_dispose (G_OBJECT (priv->adjustment)); > >+ g_object_unref (priv->adjustment); > >+ priv->adjustment = NULL; > >+ } > > why do you have to run_dispose? (add a comment?) Since the scrollbar owns the adjustment, it should be responsible for making sure that the adjustment gets disposed. Ideally every GObject that has signals gets disposed to prevent leakage of the object through loops via signal handlers and language bindings. I'm also counting on the adjustment being disposed, and thus disconnected, to avoid having to explicitly disconect in StScrollView. Since StAdjustment isn't a ClutterActor or something else with a destroy() method, we need to call g_object_run_dispose() directly. (In reply to comment #12) > (From update of attachment 155766 [details] [review]) > >- child_box.y2 = MAX (content_box.y1, content_box.y2 - sb_height); > >+ child_box.y2 = content_box.y2; > > should be content_box.y2 - sb_height; the way it is now it draws the bottom > shadow over the horizontal scrollbar. Good catch. > >+ disconnect_hadjustment(self); > >+ disconnect_vadjustment(self); > > space before ( Fixed (though removed in a subsequent patch) (In reply to comment #13) > (From update of attachment 155779 [details] [review]) > >- priv->top_shadow_visible = fabs (value - lower) > 0.1; > >- priv->bottom_shadow_visible = fabs (upper - value - page_size) > 0.1; > >+ priv->top_shadow_visible = value > lower + 0.1; > >+ priv->bottom_shadow_visible = value < upper - 0.1; > > You need to keep the "page_size" there. As it is now, if you turn on shadows in > the test program, the bottom shadow is always visible, because even if you > scroll all the way to the bottom, value is only (upper - page_size), which is > less than (upper - 0.1). Yeah, just a victim of changing the setting of upper and changing it back. > Also, it still needs to take vscrollbar_visible into account. value is always 0 > when the scrollview doesn't scroll vertically, and 0 is less than (upper - > 0.1), so the bottom shadow is now always visible when the scrollview is taller > than its contents. When the scrollview doesn't scroll vertically, then we must have upper == page_size, so value >= upper - page_size - 0.1 = - 0.1 So, with the other fix I think it is fine. (I think the scrollable child needs to to do this correctly and can't get lazy - otherwise the scrollbars will be wrong for the ALWAYS policy - I should clarify the scrollable docs though.)
Created attachment 155884 [details] [review] Simplify handling of adjustments The relationship between adjustments and scrollbars and scrollable widgets was much more complex than it needed to be. StScrollView: Have the scroll view own a pair of adjustments, set them on the child on add(), remove unnecessary change notification signal connections. StBoxLayout: Remove auto-create of adjustments, just take the adjustments from the scrollbars and set them on the scrollable child. Notify for hadjustment/vadjustment properties. StScrollBar: Notify adjustment property. StScrollable: Document how adjustment setting works.
The last patch switches the ownership from the scrollbar to the scrollview to be closer to GTK+ and a bit more flexible. If this looks OK, I think we're ready to land all of this (and the switch-over to Clutter 1.2)
Comment on attachment 155884 [details] [review] Simplify handling of adjustments yeah, that seems cleaner. the comment about run_dispose() now seems unnecessary. I guess I was confused by the gtk-docs on g_object_run_dispose(), and the fact that the method isn't widely used... but that's because gtk_object_destroy/clutter_actor_destroy are used instead.
Pushed (forgot to squash the "Incremental fixes" patch, oh well) Attachment 155766 [details] pushed as f6cbb14 - Incremental fixes for scrollview work Attachment 155779 [details] pushed as c83883f - Fixes for setting up scrolling adjustments Attachment 155884 [details] pushed as 8917354 - Simplify handling of adjustments