GNOME Bugzilla – Bug 645744
fix boxpointer positioning/animations
Last modified: 2011-09-22 18:47:57 UTC
Currently, boxpointer has a bad hack to keep it from going offscreen, which has the side effect of breaking the slide-in/-out animations. Additionally, it doesn't act immediately; it draws the boxpointer partially-offscreen first, and then fixes it afterward. Additionally... the whole way boxpointer works, with the caller having to call setPosition(), is a little weird (especially the fact that the message tray has to call it repeatedly during animations). We should pass sourceActor and alignment at construct time (and kill off the unused "gap" parameter), and then just DTRT after that. This would require changes to the message tray code though, because it currently shares a single boxpointer among all the summaryitems, and just changes which one it passes to setPosition(). When doing the patch in bug 645647, I'd thought we could fix the positioning problem by using a ClutterConstraint, but Owen brought up some issues with that: > A clutter constraint is probably not a good idea because clutter constraints > are slightly non-functional - they only work correctly if the target actor is > allocated *after* the source actor *and* the target actor is always allocated > every time the source actor is allocated, both of which depend on packing and > the details of implementation of the actors in the hierarchy containing the two > actors. (Actually, I guess in both cases it would work fine for this, since > we're only interested in updating the source actor position when the source > actor is allocated, but it can't be used to constraint the box pointer to point > to the source actor.) > > My suggestion would be to do it the same way we do tooltips now - leave the box > pointer positioned at 0, 0, in it's parent but do: > > clutter_actor_set_anchor_point ((ClutterActor*) tooltip, -tooltip_x, > -tooltip_y); > > it's a hack, but works because the anchor position is a second layer of > positioning which is independent of the request/allocate cycle. However, anchor points are also broken (qv bug 635695 comment 11, bug 614047), so I'm not excited about that. We may need to start looking at finding some way to generically fix the problem of ClutterActors whose size/position depends on other ClutterActors (here, tooltips, PopupMenuItem, etc).
Created attachment 184285 [details] [review] boxpointer: unbreak the slide-in/slide-out animation further hacks on top of previous hack. I'm not actually recommending this patch, just providing it if other people feel more strongly about the slide-in/out animation than I do...
(In reply to comment #0) > > My suggestion would be to do it the same way we do tooltips now - leave the box > > pointer positioned at 0, 0, in it's parent but do: > > > > clutter_actor_set_anchor_point ((ClutterActor*) tooltip, -tooltip_x, > > -tooltip_y); > > > > it's a hack, but works because the anchor position is a second layer of > > positioning which is independent of the request/allocate cycle. > > However, anchor points are also broken (qv bug 635695 comment 11, bug 614047), > so I'm not excited about that. We may need to start looking at finding some way > to generically fix the problem of ClutterActors whose size/position depends on > other ClutterActors (here, tooltips, PopupMenuItem, etc). I'm definitely suspicious of gravity. Don't really know any problems with anchor points set statically as x/y positions other than the possibility of code not dealing with transformations properly. But transformations are pretty basic to clutter, so saying they just don't work is extreme.
Created attachment 184322 [details] [review] Add shell_util_get_transformed_allocation() Add a function that gets the current allocation of an actor transformed into stage coordinates. This avoids a misfeature of clutter_actor_get_transformed_size() where when a size request is queued (even if it won't eventually change the size), the returned value is the transformed size request rather than the last allocation.
Created attachment 184323 [details] [review] boxPointer: use shell_util_get_transformed_allocation() Using ClutterActor.get_transformed_size() can produce bugs if we happen to position the box pointer when the source actor has a relayout queued. Use our newly added reliable utility function instead.
Created attachment 184324 [details] [review] boxPointer: Use the anchor point to fix problems with allocations Instead of setting the x/y position of the box pointer, which results in a long change of workarounds for limitations of the Clutter layout system, set the anchor point instead, which takes the positioning out of the layout system. The position is computed as a combination of the position computed from the allocation and the box pointer's size, and an offset that we tween when animating showing and hiding the box pointer.
Attached my take at the problem. The first two patches are necessary because a positioning bug popped up when not continually reallocating the box pointer - if the source actor had a relayout queued when _reposition() was initially called, the value from get_transformed_size() would be wrong and the box pointer would end up in the wrong place. This happened with the user status menu where the relayout was queued during change the state of the menu to active.
I'd like to get my patches in here prior to 3.0.0 - while they are fairly large and intrusive, I think we need some fix for the box pointer slideouts - this was a behavior that Maxim spent quite a bit of time achieving, and I don't think it makes sense to lose it via accidental last-minute regression. Note that my patches also fix allocation cycle warnings from clutter if you mouse over other notification icons in the message tray while a right-click menu is up.
Comment on attachment 184322 [details] [review] Add shell_util_get_transformed_allocation() yes, this is basically a simple transformation of clutter_actor_get_transformed_size()
Comment on attachment 184323 [details] [review] boxPointer: use shell_util_get_transformed_allocation() looks right
Comment on attachment 184324 [details] [review] boxPointer: Use the anchor point to fix problems with allocations it all seems right
Pushed with release-team signoff. Attachment 184322 [details] pushed as 905c4bb - Add shell_util_get_transformed_allocation() Attachment 184323 [details] pushed as bc48bd5 - boxPointer: use shell_util_get_transformed_allocation() Attachment 184324 [details] pushed as 22c22e0 - boxPointer: Use the anchor point to fix problems with allocations
Probably shouldn't have closed this, since there was the component of not calling setPointer repeatedly during animations. (Fixing that may be a little hard since connecting to notify::allocation wasn't reliable last I remember.)
(In reply to comment #12) > Probably shouldn't have closed this, since there was the component of not > calling setPointer repeatedly during animations. (Fixing that may be a little > hard since connecting to notify::allocation wasn't reliable last I remember.) "not reliable" in the sense of "doesn't work at all". anyway, this is working well enough. no need to keep this bug open.