GNOME Bugzilla – Bug 640974
Save enter/leave events and deliver them after DND
Last modified: 2011-03-07 21:59:38 UTC
This fixes a problem I was having on the workspace-thumbnails branch where when the slideout opened on dnd, it would close at the end of DND even though we were still hovered. I think it's a pretty non-hacky hack. (though it would be nicer if Clutter itself did enter/leave events for grabs) Possible questions in my mind: * Should it occur before or after the drag-end signal. It's after here because that's easier and because it makes sense to me to deliver the events when everything is back to the non-dnd state. But in the case above I actually needed to Meta.later_add(BEFORE_REDRAW) because I didn't want to slide in on drag-end until these events were delivered. * Would it be better to send on the first leave event as soon as we get it, rather than waiting for the end of the drag? It probably looks good if a source prelight sticks around during the drag, but if a source tooltip triggers during the drag that might look funny? But I think it's OK like this unless people have strong opinions otherwise, can always be adjusted later.
Created attachment 179657 [details] [review] Save enter/leave events and deliver them after DND During a drag-and-drop, our pointer grab keeps enter/leave events from being delivered. That means that after the DND ends, whatever actor is under the pointer won't have received the enter event it should have, and any state or hover effect dependent on that won't work right. So, save the first-leave and last-enter and deliver them at the end of the drag to get things into a consistent state for the current pointer position.
Comment on attachment 179657 [details] [review] Save enter/leave events and deliver them after DND Hm... why does this work? If you're dragging an actor, then that actor should (most of the time) be the actor-under-pointer, and so you should never enter or leave any other actor (unless the drawing lags behind the pointer motion). Also, we already have another (much-less-thorough) hack for this: st_widget_sync_hover(). If that's not sufficient for this case, then it's probably not sufficient for the other cases where it's used either.
(In reply to comment #2) > (From update of attachment 179657 [details] [review]) > Hm... why does this work? If you're dragging an actor, then that actor should > (most of the time) be the actor-under-pointer, and so you should never enter or > leave any other actor (unless the drawing lags behind the pointer motion). Remember, we now use shell_util_set_hidden_from_pick() on the drag actor, so it's ignored when computing the actor under the pointer and sending enters and leave. > Also, we already have another (much-less-thorough) hack for this: > st_widget_sync_hover(). If that's not sufficient for this case, then it's > probably not sufficient for the other cases where it's used either. The one place we use sync_hover, we have an idea about what actor the hover is broken for. We don't really have that in the DND case. I think a solution based on sync_hover would actually end up looking a lot like this: - Remember the first actor that gets a leave and the last actor that gets an enter - For each of those, walk up through their parent actors, and for any StWidgets in that set, call st_widget_sync_hover() I don't know if that's better or worse than this - it's cleaner in once sense because it doesn't deliver "blast from the past" Clutter events. But it's also less general - works only for the implementation of hover in StWidget and not any other implementations.
(In reply to comment #3) > Remember, we now use shell_util_set_hidden_from_pick() on the drag actor Wacky. I hadn't seen that. So I was thinking sync_hover() was used in more places than it actually is. But anyway, the point was, it seems like anywhere where we grab the pointer and then release it later, we want to be syncing hover/belatedly delivering leave and enter events. Eg, if you open a panel menu, then move down to the bottom of the screen and release, right now, the message tray doesn't show up (until you leave and re-enter it). So, like, replace "Clutter.grab_pointer()" with "Main.grab_pointer()", that calls the clutter method but also sets up the leave/enter listener from this patch, and "Main.ungrab_pointer()" that does the ungrab and delayed delivery. Or not.
Comment on attachment 179657 [details] [review] Save enter/leave events and deliver them after DND anyway, there are no problems with this patch if you don't think we should worry about the larger issue
(In reply to comment #5) > (From update of attachment 179657 [details] [review]) > anyway, there are no problems with this patch if you don't think we should > worry about the larger issue Hmm, I guess the "big picture" here would be to wrap the Clutter pointer grab and ungrab APIs at the ST level and then use the grab-time and ungrab-time values of the pointer actor to compute the widgets that need st_widget_sync_focus after ungrab. (But the "freeze hovers and delay sync until ungrab" thing is sort of DND specific - it might *also* be the right behavior for menus, but is it always the right behavior?) Do you think that would be better? This is all working around Clutter level deficiency really.
Created attachment 179961 [details] [review] So, the general patch ran into a snag - there are no enter/leave events delivered when the pointer actor is reparented out from its parent as we do during DND. Detecting that case and using _dragOrigParent is possible, but we can't do that using event delivery since there's no way to set the source actor in a ClutterEvent via Javascript. So this version switches over to using st_widget_sync_hover(). With the _dragOrigParent hack it's pretty DND specific and hard to generalize. === Fix hover state after DND During a drag-and-drop, our pointer grab keeps enter/leave events from being delivered. That means that after the DND ends, whatever actor is under the pointer won't have received the enter event it should have, and any state or hover effect dependent on that won't work right. By remembering the first-leave and last-enter events we can figure out what widgets we need to call st_widget_sync_hover() on after the drag.
Comment on attachment 179961 [details] [review] So, the general patch ran into a snag - there are no enter/leave events >+ // Actor is an actor we might have entered or left during the drag; call "might"? >+ // If the actor was reparented from its original location and >+ // destroyed, then start syncing hover at the original parent >+ if (actor == this._dragActor && this._actorDestroyed) >+ actor = this._dragOrigParent; So, this relies on the fact that removing the actor from its parent will result in an enter-event on it, but you don't explain that anywhere. Also, it feels like this special case doesn't belong inside _syncHover. Also, why does it matter if the actor was destroyed? Since you no longer need the rest of the event structures, you could change firstLeaveEvent and lastEnterEvent to firstLeftActor and lastEnteredActor, and initialize firstLeftActor to dragOrigParent right away in the reparenting case, and then you don't need the special case here.
Hm. See also bug 642920, which would be fixed by having the clutter grab workaround discussed in comment 4 / comment 6
(In reply to comment #8) > (From update of attachment 179961 [details] [review]) > >+ // Actor is an actor we might have entered or left during the drag; call > > "might"? OK, yeah, extraneous. > >+ // If the actor was reparented from its original location and > >+ // destroyed, then start syncing hover at the original parent > >+ if (actor == this._dragActor && this._actorDestroyed) > >+ actor = this._dragOrigParent; > > So, this relies on the fact that removing the actor from its parent will result > in an enter-event on it, but you don't explain that anywhere. No, that's not what is going on, we leave dragActor when we do: Shell.util_set_hidden_from_pick(this._dragActor, true); on next motion. The reparent has nothing to do with it, and as far as I know clutter won't generate such a leave event, but just leave our hover states messed up. > Also, it feels like this special case doesn't belong inside _syncHover. To me, it makes sense - we want to "sync up the hover" state of some actor, and if it no longer exists, then we find the best alternative we have. > Also, why does it matter if the actor was destroyed? If the actor wasn't destroyed, but was instead reparented back to the dragOrigParent, then it needs it's hover state synced. > Since you no longer need the rest of the event structures, you could change > firstLeaveEvent and lastEnterEvent to firstLeftActor and lastEnteredActor, and > initialize firstLeftActor to dragOrigParent right away in the reparenting case, > and then you don't need the special case here. Hmm, in the reparenting case we can't always make firstLeftActor dragOrigParent - we can do the dragOrigParent substtution when the actor is actually destroyed, perhaps. I'll attach another variant of this patch, which seems to work as well, though I can't remember exactly everything I was testing before. See if you like it better. (Another approach would obviously be to try and sync up with the way you are doing things in bug 643687 using Clutter.InputDevice.get_pointer_actor() - but that doesn't prevent the need to deal with the special case of the drag actor being the pointer actor and then being destroyed... that would be just as necessary with that approach.)
Created attachment 182584 [details] [review] Fix hover state after DND Slight variant approach tracking actors rather than events
(In reply to comment #10) > > Also, it feels like this special case doesn't belong inside _syncHover. > > To me, it makes sense - we want to "sync up the hover" state of some actor, and > if it no longer exists, then we find the best alternative we have. Right, but without that special case, _syncHover could have been a static method, or even an StWidget method, so it just seemed a little gratuitous to have the special case inside it, rather than at the point where it's called.
Comment on attachment 182584 [details] [review] Fix hover state after DND I do like this one better, but feel free to commit whichever you prefer.
Attachment 182584 [details] pushed as 36287fc - Fix hover state after DND