Created attachment 127485 [details] [review]
Two dragging-related fixes

- Don't let the user grab a moving window or we'll get dueling tweens.
  - Update _overlappedMode each time _positionWindows is called

Created attachment 127486 [details] [review]
Do window dragging in the stage, not in the original workspace's actor

This will allow for more code-sharing between window and app dragging,
and also means we won't run into trouble if we start using clipping on
the workspaces later

Created attachment 127488 [details] [review]
Move window clone functionality into its own class

As vaguely discussed before. Doing this *after* the drag-in-the-stage patch
makes it simpler, which is why I did it that way.

Created attachment 127490 [details] [review]
Move window clone functionality into its own class

oops, fix a few remnants of earlier code

All the patches looks very reasonable to me.

I'm not sure WindowClone and 'clone' variables are the right naming long term, since the fact the actor is a clone is somewhat peripheral to the functionality. But it does make local variable naming easier than having to distinguish different types of window.

_makeDesktopRectangle() can be removed, right?

(In reply to comment #5)
> I'm not sure WindowClone and 'clone' variables are the right naming long term,
> since the fact the actor is a clone is somewhat peripheral to the
> functionality. But it does make local variable naming easier than having to
> distinguish different types of window.

Yeah, exactly. Maybe we could have a better name though. WindowRepresentation? MiniWindow?

Created attachment 127801 [details] [review]
patch to add app/document dragging

OK, this reorganizes the window-dragging code, and adds app/document dragging (and points out that we need startup notification :-)

To make something draggable, you create a DND.Draggable, passing it your "model" object; Draggable assumes that the model has an "actor" property, and possibly also a "getDragActor" method, to be used to supply a actor to drag if it's not supposed to drag the original actor around (eg, for the app/doc buttons).

To make something a drop source, you define a method "acceptDrop" on it, and call DND.makeDroppable() on it, which again assumes you have an "actor" property. Originally, the plan was to *just* do the "define acceptDrop" thing, but the problem is that the Draggable code has to scan through ClutterActors trying to find a drop target, but you're defining acceptDrop on the "model", not the actor. So DND.makeDroppable() just copies the function over from the model to the actor, which is lame and if you've got a better idea, let's hear it. (Oh, yes, the fact that for drag sources you create a new object and for drag targets you call a static method is also lame. Which is better?)

(And before I forget, you need the latest gir-repository, to get the right annotations for clutter_stage_get_actor_at_pos, or else it will crash after a while.)

The special WindowClone.addTween method (that does the animationCount handling) ends up requiring another special case here. In fact, there's a lot of code now that does nothing except make sure we're not showing the title when we're not supposed to. We need something cleverer here eventually.

(In reply to comment #7)

The DND refactoring seems to work out well here. It's great how little code gets added to the GenericDisplayItem. (As always, a ton of work in the setup, and then the actual feature is trivial....)

> To make something draggable, you create a DND.Draggable, passing it your
> "model" object; Draggable assumes that the model has an "actor" property, and
> possibly also a "getDragActor" method, to be used to supply a actor to drag if
> it's not supposed to drag the original actor around (eg, for the app/doc
> buttons).
>
> To make something a drop source, you define a method "acceptDrop" on it, and
> call DND.makeDroppable() on it, which again assumes you have an "actor"
> property. Originally, the plan was to *just* do the "define acceptDrop" thing,
> but the problem is that the Draggable code has to scan through ClutterActors
> trying to find a drop target, but you're defining acceptDrop on the "model",
> not the actor. So DND.makeDroppable() just copies the function over from the
> model to the actor, which is lame and if you've got a better idea, let's hear
> it. (Oh, yes, the fact that for drag sources you create a new object and for
> drag targets you call a static method is also lame. Which is better?)

The only other idea that comes to mind is to have a standard backpointer property that we always set on the actor whenever we have this model/actor pair, so we can generically look at actor._model). (is "delegate" a better term than model here? I don't expect models to be 1:1 with the view object and handle UI events. http://developer.apple.com/DOCUMENTATION/Cocoa/Conceptual/CocoaFundamentals/CommunicatingWithObjects/chapter_6_section_4.html) 

Don't know if that is better or not.

I actually would like 'DND.makeDraggable(model)' [could return an object to connect to] better than 'new DND.Draggable(model)' - having the latter connect to signal handlers and alter the behavior of model seems slightly like a weird magic side effect to me.
 
> The special WindowClone.addTween method (that does the animationCount handling)
> ends up requiring another special case here. In fact, there's a lot of code now
> that does nothing except make sure we're not showing the title when we're not
> supposed to. We need something cleverer here eventually.

Don't have a lot of good ideas there. Maybe we could a) only hide the title dragging the window b) somehow force clutter to pick and send enter/leaves when we stop animating to prevent weird cases where a title might get stuck on if it got shown during animation .... we actually know when we stop animating in our frame ticker.

Maybe aa block comment at the top of DND.js explaining the usage. Some details, in particular that you can't (reliably) connect to button-pres/release yourself any more and need to use 'clicked' are non-intuitive. 

(Would things be improveed by making DND.js connect to the ::event signal rather than button-press-event? Then you'd be more likely to get in reliably before other connections, rather than before/after depending on ordering. And there would be less chance of a ::button-release-event handler running first and returning true and blocking the drag.)

+    _onButtonPress : function (actor, event) {

Probably good to check event.button and only trigger drags on button 1.

+        if (!Draggable._dragThreshold) {
+            let settings = Gtk.Settings.get_default();
+            Draggable._dragThreshold = settings.gtk_dnd_drag_threshold;
+        }

Don't think caching here is worthwhile, and it's going to break the user changing the threshold in the UI without logging back in/out.

+            this.emit('drag', event.get_time());

GTK+ uses ::drag-begin, ::drag-end for this. "drop" is always something that happens on the destination side. I think ::drag-end is better for something that conceptually would happen even if we added "Escape" cancelling of a drag.

+                this._dragOffsetX = -this._dragActor.width / 2;
+                this._dragOffsetY = -this._dragActor.height / 2;

Probably better to return an offset between the source actor and the drag actor along with the actor from getDragActor - if I click towards the upper left of a icon, it' going to look better if that's where the cursor points when I'm dragging.

+                let [cloneStageX, cloneStageY] = actor.get_transformed_position();

"clone" looks like a leftover.

+        this.emit('drop', dropX, dropY, event.get_time());

I don't think a detailed 'drop' like this is useful unless you had a return value and the dragee could say "I handled it, do nothing more", without that a pure notification ::drag-end is probably better. Maybe after the end of snap back in the failed drop case, though it's sort of nice if it occurs before the drop in the non-failed drop case, so not sure how that would work... :-)

+                if (target._acceptDrop(this.model, actor,
+                                       (dropX + this._xOffset - targX) / target.scale_x,
+                                       (dropY + this._yOffset - targY) / target.scale_y,

I tried it both ways for window dragging and it seemed most intuitive to me when the "hot spot" for the drop was the cursor position, rather than the middle or other point on the window. This is also how GTK+ DND works.

+    _onSnapBackComplete : function (dragActor) {
+        if (this._dragOrigParent) {
+            dragActor.reparent(this._dragOrigParent);
+            dragActor.set_scale(this._dragOrigScale, this._dragOrigScale);
+            dragActor.set_position(this._dragOrigX, this._dragOrigY);
+        } else
+            dragActor.destroy();
+    }

isn't there a danger of the user (especially if trying to double click when moving the mouse) starts another drag on the actor as its snapping back, causing things to go haywire?

+        let icon = new Clutter.CloneTexture({ parent_texture: this._icon });
+        [icon.width, icon.height] = this._icon.get_transformed_size();

I wonder if the returned actor should just be set up in the coordinate system of the parent of the drag source and the DND code should worry about scaling it? (Might be nasty to implement)

-        this._group.add_actor(this._icon);
+        this.actor.add_actor(this._icon);

If you feel like some 'git add -p' work, this would be nicer as a separate commit. (I agree with the idea)

+    _onDestroy: function () {
+        Tweener.removeTweens(this.actor);
     },

Looks like something we're going to be adding all the heck over the place. I wonder if some sort of tweener hook allow us to RTTI identify types with ::destroy and connect to that automatically would make sense.

Does removeWindow/onWindowRemoved need an update for the destroy() => _onDestroy() change for WindowClone? (A little hard for me to follow the logic with multiple intersecting patches in this area.) What happens if a window is removed during a drag - will the DND code try to reparent the destroyed window back?

@@ -583,7 +508,7 @@ Workspace.prototype = {
             // the compositor finds out about them...
             Mainloop.idle_add(Lang.bind(this,
                                         function () {
-                                            if (metaWin.get_compositor_private())
+                                            if (this.actor && metaWin.get_compositor_private())
                                                 this._windowAdded(metaWorkspace, metaWin);
                                             return false;
                                         }));

Look like an unrelated, if needed, change.

(In reply to comment #8)
> The only other idea that comes to mind is to have a standard backpointer
> property that we always set on the actor whenever we have this model/actor
> pair, so we can generically look at actor._model).

Yeah, that could probably get used for other stuff too. Though do we "_"-prefix it or not? It would be considered public, which argues against the "_", but we don't want to conflict with a native property name, which argues for it...

> (is "delegate" a better term than model here?

Probably. ("model" came from bug 562584 comment 6.)

> I actually would like 'DND.makeDraggable(model)' [could return an object to
> connect to] better than 'new DND.Draggable(model)' - having the latter connect
> to signal handlers and alter the behavior of model seems slightly like a weird
> magic side effect to me.

OK. (I think I was originally thinking makeDraggable(), but changed it when there needed to be an object to connect to signals on.)

> > The special WindowClone.addTween method... ends up requiring another
> > special case here.
...
> +    _onDestroy: function () {
> +        Tweener.removeTweens(this.actor);
>      },
> 
> Looks like something we're going to be adding all the heck over the place. I
> wonder if some sort of tweener hook allow us to RTTI identify types with
> ::destroy and connect to that automatically would make sense.

We could make our own wrapper around Tweener that:

  (1) connects to ::destroy on the tweened object as above
  (2) tracks the tween count as with WindowClone and calls methods on
      the object's delegate to indicate when animation starts/stops
  (3) turns off 'reactive' on actors while they're tweening? actually,
      maybe we don't want to do this always, but we could have a
      nonreactive_while_animating special property

Or maybe the clutter 0.9 animation framework will save the day for us? Tweener is nice to use, but the code is pretty gross and the API (other than addTween) is often not quite right.

> (Would things be improved by making DND.js connect to the ::event signal
> rather than button-press-event?

yes...

> +        this.emit('drop', dropX, dropY, event.get_time());
> 
> I don't think a detailed 'drop' like this is useful unless you had a return
> value and the dragee could say "I handled it, do nothing more", without that a
> pure notification ::drag-end is probably better.

Yeah, this is to some extent a leftover from before adding makeDroppable/acceptDrop. (You'll notice that the WindowClone _onDropped doesn't use any of the args.) It really is "drag-end', like you say.

> isn't there a danger of the user (especially if trying to double click when
> moving the mouse) starts another drag on the actor as its snapping back,
> causing things to go haywire?

Nope:

+    _onButtonPress : function (actor, event) {
+        if (Tweener.getTweenCount(actor))
+            return;

> +        let icon = new Clutter.CloneTexture({ parent_texture: this._icon });
> +        [icon.width, icon.height] = this._icon.get_transformed_size();
> 
> I wonder if the returned actor should just be set up in the coordinate system
> of the parent of the drag source and the DND code should worry about scaling
> it? (Might be nasty to implement)

It's easy to implement, because we already do that for the non-getDragActor case. But it turns out this isn't actually about scaled-vs-non-scaled size; the icon's transformed_size is the same as it's size, it's just that that's *not* the same as the icon's natural size, which is what the clone defaults to. (We're blurrily scaling the icons up currently. Presumably a bug.)

> -        this._group.add_actor(this._icon);
> +        this.actor.add_actor(this._icon);
> 
> If you feel like some 'git add -p' work, this would be nicer as a separate
> commit. (I agree with the idea)

OK, will probably just commit that after splitting it out.

> Does removeWindow/onWindowRemoved need an update for the destroy() =>
> _onDestroy() change for WindowClone?

No, it was broken before. :-)

> What happens if a window is removed during a drag

No clue...

(In reply to comment #9)
> (In reply to comment #8)
> > The only other idea that comes to mind is to have a standard backpointer
> > property that we always set on the actor whenever we have this model/actor
> > pair, so we can generically look at actor._model).
> 
> Yeah, that could probably get used for other stuff too. Though do we "_"-prefix
> it or not? It would be considered public, which argues against the "_", but we
> don't want to conflict with a native property name, which argues for it...

I think we need the _ if we are going to stick it on arbitrary actors, anything else is a recipe for weirdness.

> > > The special WindowClone.addTween method... ends up requiring another
> > > special case here.
> ...
> > +    _onDestroy: function () {
> > +        Tweener.removeTweens(this.actor);
> >      },
> > 
> > Looks like something we're going to be adding all the heck over the place. I
> > wonder if some sort of tweener hook allow us to RTTI identify types with
> > ::destroy and connect to that automatically would make sense.
> 
> We could make our own wrapper around Tweener that:
> 
>   (1) connects to ::destroy on the tweened object as above
>   (2) tracks the tween count as with WindowClone and calls methods on
>       the object's delegate to indicate when animation starts/stops
>   (3) turns off 'reactive' on actors while they're tweening? actually,
>       maybe we don't want to do this always, but we could have a
>       nonreactive_while_animating special property
> 
> Or maybe the clutter 0.9 animation framework will save the day for us? Tweener
> is nice to use, but the code is pretty gross and the API (other than addTween)
> is often not quite right.

Maybe we need to investigate the animation API and see on our side and the clutter side what needs adapting ... I have the feeling that it won't work for us without our attention. It would be nice to think that Clutter comes with the right api and something foreign and language specific doesn't have to be added on.
 
> > isn't there a danger of the user (especially if trying to double click when
> > moving the mouse) starts another drag on the actor as its snapping back,
> > causing things to go haywire?
> 
> Nope:
> 
> +    _onButtonPress : function (actor, event) {
> +        if (Tweener.getTweenCount(actor))
> +            return;
> 
> > +        let icon = new Clutter.CloneTexture({ parent_texture: this._icon });
> > +        [icon.width, icon.height] = this._icon.get_transformed_size();

Hmm, guess that works. :-)

> > I wonder if the returned actor should just be set up in the coordinate system
> > of the parent of the drag source and the DND code should worry about scaling
> > it? (Might be nasty to implement)
> 
> It's easy to implement, because we already do that for the non-getDragActor
> case. 

I was thinking it was harder because the drag actor wasn't in the tree so you couldn't call getTransformedSize().

> But it turns out this isn't actually about scaled-vs-non-scaled size; the
> icon's transformed_size is the same as it's size, it's just that that's *not*
> the same as the icon's natural size, which is what the clone defaults to.
> (We're blurrily scaling the icons up currently. Presumably a bug.)

Ah. Looks like some icons we are scaling u, some down, some maybe not at all. Does need investigation.

> > What happens if a window is removed during a drag
> 
> No clue...

OK, was pointing it out as something to think about. The DND code may need a connection to ::destroy on the actor itself.

 

(In reply to comment #10)
> > Or maybe the clutter 0.9 animation framework will save the day for us? Tweener
> > is nice to use, but the code is pretty gross and the API (other than addTween)
> > is often not quite right.
> 
> Maybe we need to investigate the animation API and see on our side and the
> clutter side what needs adapting ... I have the feeling that it won't work for
> us without our attention. It would be nice to think that Clutter comes with the
> right api and something foreign and language specific doesn't have to be added
> on.

clutter_actor_animate() (http://www.clutter-project.org/docs/clutter/0.9/clutter-Implicit-Animations.html#clutter-actor-animate) is very similar to Tweener.addTween (except being ClutterActor-specific, of course). It does automatically remove animations when they are destroyed, but other than that, it's mostly less powerful than tweener:

    - it doesn't do any callbacks (although there's a "completed" signal
      you can connect to, and I guess you could get the effect of onUpdated
      via "notify::property")

    - it only animates GObject properties, and you can't create special
      properties, so we'd have to find another way to do workspace_relative
      and clipHeight (and nonreactive_while_animating)

    - it doesn't support anything like Tweener.getTweenCount, or
      Tweener.removeTweens.

So I guess probably we'll want to stick with Tweener, maybe with a wrapper.

> > > What happens if a window is removed during a drag
> > 
> > No clue...
> 
> OK, was pointing it out as something to think about. The DND code may need a
> connection to ::destroy on the actor itself.

Yeah, there was an implicit "I'll investigate" in there. :)

Created attachment 127952 [details] [review]
Create a Tweener wrapper that handles some things for us

As pondered in comment #9, this makes a wrapper around Tweener that
(a) tracks the total animation count and calls special callbacks as needed,
to remove the need for us doing that ourselves, and (b) automatically
disconnects tweens when their actor is destroyed.

Created attachment 127953 [details] [review]
Abstract out drag and drop support, and add app/doc DND

and here's the updated DND patch. comments to follow

(In reply to comment #8)
> The only other idea that comes to mind is to have a standard backpointer
> property that we always set on the actor whenever we have this model/actor
> pair, so we can generically look at actor._model). (is "delegate" a better term
> than model here?

We now set _delegate on various actors, and use that from both Draggable and the Tweener wrapper.

> I actually would like 'DND.makeDraggable(model)' [could return an object to
> connect to] better than 'new DND.Draggable(model)'

done

> (Would things be improveed by making DND.js connect to the ::event signal
> rather than button-press-event? Then you'd be more likely to get in reliably
> before other connections, rather than before/after depending on ordering. And
> there would be less chance of a ::button-release-event handler running first
> and returning true and blocking the drag.)

This is done now, and GenericDisplayItem and WindowClone are back to using button-release-event. While testing things though, I noticed that just triggering off button-release-event is a bit of a lose, because it fires even when the button wasn't pressed in the same actor. So you can press the button on the backdrop, move into a workspace, and release the button, and the workspace will get selected. ClutterActor doesn't have a "clicked" signal, so this is hard to fix cleanly (although we could have a "makeClickable"...)

> +    _onButtonPress : function (actor, event) {
> 
> Probably good to check event.button and only trigger drags on button 1.

Not currently possible from gjs. Noted with a FIXME.

> +            Draggable._dragThreshold = settings.gtk_dnd_drag_threshold;
> 
> Don't think caching here is worthwhile

reverted

> +            this.emit('drag', event.get_time());
> 
> GTK+ uses ::drag-begin, ::drag-end for this

changed

> +                this._dragOffsetX = -this._dragActor.width / 2;
> +                this._dragOffsetY = -this._dragActor.height / 2;
> 
> Probably better to return an offset between the source actor and the drag actor
> along with the actor from getDragActor - if I click towards the upper left of a
> icon, it' going to look better if that's where the cursor points when I'm
> dragging.

ah, I was always dragging from the textual part of the label when I was testing it. Now it drags from where you dragged from if you drag on the icon, but drags from the center of the icon if you drag from the text.

> +                let [cloneStageX, cloneStageY] =
> actor.get_transformed_position();
> 
> "clone" looks like a leftover.

fixed

> +        this.emit('drop', dropX, dropY, event.get_time());
> 
> I don't think a detailed 'drop' like this is useful

fixed

> +                if (target._acceptDrop(this.model, actor,
> +                                       (dropX + this._xOffset - targX) /
> target.scale_x,
> +                                       (dropY + this._yOffset - targY) /
> target.scale_y,
> 
> I tried it both ways for window dragging and it seemed most intuitive to me
> when the "hot spot" for the drop was the cursor position, rather than the
> middle or other point on the window. This is also how GTK+ DND works.

And also how it works here; the hit detection has already happened by this point. The coordinates being computed there are "the coordinates of the drag actor in the target coordinate system", in case acceptDrop() needs to do something with it.

> +        let icon = new Clutter.CloneTexture({ parent_texture: this._icon });
> +        [icon.width, icon.height] = this._icon.get_transformed_size();
> 
> I wonder if the returned actor should just be set up in the coordinate system
> of the parent of the drag source and the DND code should worry about scaling
> it? (Might be nasty to implement)

Fixing the dragActor icon alignment issue above ends up making it easier for both sides if getDragActor() works in stage coordinates (since the easiest way to figure out if the user dragged from inside or outside the icon is to call stage.get_actor_at_pos()).

> -        this._group.add_actor(this._icon);
> +        this.actor.add_actor(this._icon);
> 
> If you feel like some 'git add -p' work, this would be nicer as a separate
> commit. (I agree with the idea)

That part was committed this morning.

> What happens if a window is
> removed during a drag

It disappears, apparently harmlessly (although you still run into the problem mentioned above, that when you release the button, the workspace under the pointer will think that means you clicked on it).

> -                                            if
> (metaWin.get_compositor_private())
> +                                            if (this.actor &&
> metaWin.get_compositor_private())

> Look like an unrelated, if needed, change.

Also committed separately earlier today.

(In reply to comment #14)
> This is done now, and GenericDisplayItem and WindowClone are back to using
> button-release-event. While testing things though, I noticed that just
> triggering off button-release-event is a bit of a lose, because it fires even
> when the button wasn't pressed in the same actor. So you can press the button
> on the backdrop, move into a workspace, and release the button, and the
> workspace will get selected. ClutterActor doesn't have a "clicked" signal, so
> this is hard to fix cleanly (although we could have a "makeClickable"...)

Trying to do this generically won't totally work, because Clutter's "grab" semantics are much simpler than gtk's, and so we can't have both the drag code and the click code each grabbing the actor without things getting out of sync. And without grabbing, we can't distinguish "press, leave, enter, release" (which is a click) from "press, leave, release, press, enter, release" (which isn't).

Comment on attachment 127952 [details] [review]
Create a Tweener wrapper that handles some things for us

In some sense the ui.Tweener addition is deeply wrong. We are taking a small Javascript used only by us and friends and only with Clutter and wrapping it so it works right with Clutter.

But that being said, it looks like a definite win for making our code robust clean and understandable.

My only concern reading through it is it doesn't ook to me that state.count isupdated properly if you call state.removeTweens passing in properties to restrict the removal to only some tweens. Maybe it's good enough to just disallow that usage.

It's a little annoying that the typical pattern of removeTweens() / add new tweens causes an onAnimationComplete()/onAnimationStart() pair but not worth working around with extra API.

Comment on attachment 127953 [details] [review]
Abstract out drag and drop support, and add app/doc DND

Looks nice. There's some oddity in the delegates vs. signal thing going on for DND, but will do for now.

committed, with tweener changes discussed on irc