GNOME Bugzilla – Bug 648788
StScrollBar: use clutter_grab_pointer()
Last modified: 2013-02-15 00:22:32 UTC
StScrollBar was intercepting motion events by using captured-event on the stage, which required additional dirty tricks, which required additional hacks. Simplify it by just using clutter_grab_pointer() instead.
Created attachment 186758 [details] [review] StScrollBar: use clutter_grab_pointer()
poke
There was a reason we weren't grabbing the pointer before, no? Probably because it breaks if we already had a pointer grab? I guess this behavior was imported from Mx. For reference, the current Mx sources still use a capture handler: https://github.com/clutter-project/mx/blob/master/mx/mx-scroll-bar.c From some grepping around it looks unlikely we'd hit this situation, but... "which required additional dirty tricks, which required additional hacks. " Code cleanup can be good, but I'd like to know the underlying motivation. Does this fix a bug you were actually hitting? Or you were just hacking on the popup code and thought it was ugly?
(In reply to comment #3) > There was a reason we weren't grabbing the pointer before, no? > ... > I guess this behavior was imported from Mx. Hm... it appears that it originally used a pointer grab and was then changed: https://github.com/clutter-project/mx/commit/2475dd491cf9e0379c0847d268c13378f5b293e8#nbtk/nbtk-scroll-bar.c The commit message doesn't explain why though. > Code cleanup can be good, but I'd like to know the underlying motivation. Does > this fix a bug you were actually hitting? Or you were just hacking on the popup > code and thought it was ugly? Bug 643687 would have required porting PopupMenuManager's "passEvents" hack over to GrabHelper, and it made more sense to just make the hack unnecessary.
Emmanuele: any idea why MxScrollBar would have switched away from using clutter_grab_pointer()? (see comment above)
(In reply to comment #5) > Emmanuele: any idea why MxScrollBar would have switched away from using > clutter_grab_pointer()? (see comment above) because grabs are generally evil, and we tend to use the capture event as a "soft grab", since it allows other code to run, including stacking gestures and pointer operations. I honestly have no idea what kinds of hacks did you feel were necessary: the patch seems to just use the capture to disable the motion events while the pointer is pressed, and do other things with scrolling. can you describe what the "pass event" hack is? the whole point of using the capture event is to allow children to get events even though the parent is currently inside a pointer-related operation.
(In reply to comment #6) > I honestly have no idea what kinds of hacks did you feel were necessary The shell PopupMenu also uses captured-event, to block events outside the menu. (Eg, a click outside the menu should get eaten after dismissing the menu, and so therefore also, enter/leave events outside the menu should get eaten, because things outside the menu shouldn't prelight when the menu is up.) But StScrollBar calls clutter_set_motion_events_enabled(FALSE) and then watches for motion events on the stage, but our captured-event handler blocks those events, since the stage isn't inside the menu. So we had to add a hack to set a flag when scroll-start is emitted, and then just immediately return false from the captured-event handler if that flag is set. Changing StScrollBar to use clutter_grab_pointer() instead fixes the problem without needing any changes to PopupMenu, because when the scroll bar has the pointer grab, our captured-event handler doesn't get a chance to run. > the whole point of using the capture event is to > allow children to get events even though the parent is currently inside a > pointer-related operation. No, not here. StScrollBar's handle_capture_event_cb() always returns TRUE, so no one else gets any events. Which is the desired behavior, I think, which is why using a pointer grab makes sense.
(In reply to comment #7) > > the whole point of using the capture event is to > > allow children to get events even though the parent is currently inside a > > pointer-related operation. > > No, not here. StScrollBar's handle_capture_event_cb() always returns TRUE, so > no one else gets any events. Which is the desired behavior, I think, which is > why using a pointer grab makes sense. then yes: if you're preventing every other actor from receiving events then using a real grab makes more sense than using a capture. that is not always the case, and especially with touches and gestures you really don't want to have exclusive grabs in place.
(In reply to comment #8) > then yes: if you're preventing every other actor from receiving events then > using a real grab makes more sense than using a capture. ok, so, Colin? rereview?
Review of attachment 186758 [details] [review]: One very minor comment. ::: src/st/st-scroll-bar.c @@ +867,3 @@ priv->x_origin += clutter_actor_get_x (priv->trough); priv->y_origin += clutter_actor_get_y (priv->trough); A g_assert (!priv->grabbed) might be nice here.
Created attachment 200230 [details] [review] StScrollBar: use clutter_grab_pointer() StScrollBar was intercepting motion events by using captured-event on the stage, which required additional dirty tricks, which required additional hacks. Simplify it by just using clutter_grab_pointer() instead. I'm picking these patches up now. Sorry Dan.
Patch was committed as part of grab helper work in 3.6 message tray.