I was trying to report something (I think) related in Bug 613188 - resize windows in grid view.

Both resizing and snapping back to the original position before animating are annoying in grid view.

(In reply to comment #0)
> (There are also other bugs when dragging windows in the linear view even
> currently that may be related, like labels being left over when dragging a
> window off a workspace than back ...)

Should be fix by patch https://bugzilla.gnome.org/show_bug.cgi?id=610191#c26

Created attachment 156802 [details] [review]
[DND] Use Params for DND.makeDraggable()

Replace boolean 'manualMode' with a params object for
improved readability and future expansion.

Created attachment 156803 [details] [review]
[DND] Allow setting drag actor max size and opacity

Add parameters to DND.makeDraggable:

 dragActorMaxSize: Maximum size for actor drag icon
 dragActorOpacity: Opacity of actor during drag

Created attachment 156804 [details] [review]
[workspaces] Don't use a clone for the window drag actor

Using a clone for the drag actor causes the animation into
the final position not to work as expected; instead use
the newly added DND parameters to change the original
drag actors size and opacity. (This reverts the change in
011db9f).

Since we are using the original actor, we have to be careful
not to change it during the drag, so don't actually position
dragged window actors in Workspace.positionWindows().

There are various misbehaviors in the linear view with this patch set. I'd like to get the fixes in 610191 in before trying to track that down, since they may well be the same problems or related.

The dnd.js patch feels a bit messy to me, but I don't see significant cleanups that are possible, and I wanted to get these filed and get some feedback; let me know if there are parts of it which are especially confusing or look especially messy.

Comment on attachment 156803 [details] [review]
[DND] Allow setting drag actor max size and opacity

>+        params = Params.parse(params, { manualMode: false,
>+                                        dragActorMaxSize: null,
>+                                        dragActorOpacity: null });

I think "undefined" is more correct than "null" here. (Also, we might want to think about making dragActorOpacity have a default value?)

>+        this._snapBackScale = this.actor.scale_x;

every (new) "this.actor" in your patch should actually be "this._dragActor".

>+                Tweener.addTween(this.actor,
>+                                 { scale_x: scale * origScale,
>+                                   scale_y: scale * origScale,
>+                                   time: SNAP_BACK_ANIMATION_TIME,

maybe make a new SOMETHING_ANIMATION_TIME const? or rename that one

>+                                   transition: "easeOutQuad",
>+                                   onUpdate: function() {
>+                                       let currentScale = this.actor.scale_x / origScale;
>+                                       this._dragOffsetX = currentScale * origDragOffsetX;
>+                                       this._dragOffsetY = currentScale * origDragOffsetY;
>+                                       this.actor.set_position(this._dragX + this._dragOffsetX,
>+                                                               this._dragY + this._dragOffsetY);
>+                                   },

first, it's not obvious on first glance why you use onUpdate rather than just tweening x and y directly, so that should get a comment.

second, it's a little weird how this interacts with the set_position call in _updateDragPosition... (or rather, it's weird how it's carefully set up to magically *not* interact with that). Maybe the whole thing would be cleaner if we just adjusted _dragActor's anchor_point in startDrag?

>+ * If %dragActorMaxSize is present in @params, the drag actor will
>+ * be scaled down to be no larger than that size in pixels.
>+ *
>+ * If %dragActorOpacity is present in @params, the drag actor will
>+ * will be set to have that opacity during the drag.

Currently, the scale and opacity changes are reverted if the drag fails, but not if it succeeds. I think that's kinda weird, but if we're keeping that, it should be documented.

Comment on attachment 156804 [details] [review]
[workspaces] Don't use a clone for the window drag actor

This looks necessary, but given the 610191 problems it's hard to say for sure that it's sufficient. :)

(In reply to comment #7)
> (From update of attachment 156803 [details] [review])
> >+        params = Params.parse(params, { manualMode: false,
> >+                                        dragActorMaxSize: null,
> >+                                        dragActorOpacity: null });
> 
> I think "undefined" is more correct than "null" here. (Also, we might want to
> think about making dragActorOpacity have a default value?)

Hmm, my tendency is to treat 'undefined' as simply what you get if you read some property that you shoudn't have read, and something that intentionally has no value should be 'null'. But that's just wishful thinking that Javascript would warn when you make typos that reference undefined values. I'm happy to change it here if undefined seems more appropriate.

> >+        this._snapBackScale = this.actor.scale_x;
> 
> every (new) "this.actor" in your patch should actually be "this._dragActor".

Oops. Should have followed through and tested with something other than window DND.

> >+                Tweener.addTween(this.actor,
> >+                                 { scale_x: scale * origScale,
> >+                                   scale_y: scale * origScale,
> >+                                   time: SNAP_BACK_ANIMATION_TIME,
> 
> maybe make a new SOMETHING_ANIMATION_TIME const? or rename that one

Just was lazy when I was getting it to work and forgot to go back and add a new time later.

> >+                                   transition: "easeOutQuad",
> >+                                   onUpdate: function() {
> >+                                       let currentScale = this.actor.scale_x / origScale;
> >+                                       this._dragOffsetX = currentScale * origDragOffsetX;
> >+                                       this._dragOffsetY = currentScale * origDragOffsetY;
> >+                                       this.actor.set_position(this._dragX + this._dragOffsetX,
> >+                                                               this._dragY + this._dragOffsetY);
> >+                                   },
> 
> first, it's not obvious on first glance why you use onUpdate rather than just
> tweening x and y directly, so that should get a comment.
> 
> second, it's a little weird how this interacts with the set_position call in
> _updateDragPosition... (or rather, it's weird how it's carefully set up to
> magically *not* interact with that). Maybe the whole thing would be cleaner if
> we just adjusted _dragActor's anchor_point in startDrag?

I think the first and second are really the same:

                // The position of the actor changes as we scale
                // around the drag position, but we can't just tween
                // to the final position because that tween would
                // fight with updates as the user continues dragging
                // the mouse; instead we do the position computations in
                // an onUpdate() function.

Yes, it would seem cleaner to move the scale center and anchor to the drag position on drag start, then we could just tween the size and everything would work. I didn't do that because:

 - I didn't think of moving the anchor point as well as the scale center,
   and just moving the scale center wasn't working out as I naively
   expected.

 - I don't really understand the interaction between anchor point and
   scale center, and Emmanuele has been somewhat dismissive about the
   anchor point API in the past.

I think I'd rather try the simplification as a separate patch and see if it does simplify rather than folding it in here.

> >+ * If %dragActorMaxSize is present in @params, the drag actor will
> >+ * be scaled down to be no larger than that size in pixels.
> >+ *
> >+ * If %dragActorOpacity is present in @params, the drag actor will
> >+ * will be set to have that opacity during the drag.
> 
> Currently, the scale and opacity changes are reverted if the drag fails, but
> not if it succeeds. I think that's kinda weird, but if we're keeping that, it
> should be documented.

The "source actor is drag actor" code is somewhat specific to the window dragging case, really.. but in general the problem with reverting on accepted drops:

 - We can't revert the changes before acceptDrop because that will
   prevent the dropee from animating the actor into its new position,
   because it won't know the parameters to animate from.

 - We can't revert the changes after acceptDrop because that might fight
   with whatever acceptDrop did to the actor.

Added the comment:

+ * Note that when the drag actor is the source actor and the drop
+ * succeeds, the actor scale and opacity aren't reset; if the drop
+ * target wants to reuse the actor, it's up to the drop target to
+ * reset these values.
  */

But if you have better ideas about how we could actually reset them and avoid such a comment, I'd rather do that the comment weirdness.

 - We can't revert the changes

(In reply to comment #9)
> Hmm, my tendency is to treat 'undefined' as simply what you get if you read
> some property that you shoudn't have read, and something that intentionally has
> no value should be 'null'.

ok, maybe we just need a style guideline...

>    Emmanuele has been somewhat dismissive about the
>    anchor point API in the past.

How far in the past? In 0.8 it was pretty broken, but it got better in 1.0.

> I think I'd rather try the simplification as a separate patch and see if it
> does simplify rather than folding it in here.

sure

> But if you have better ideas about how we could actually reset them and avoid
> such a comment, I'd rather do that the comment weirdness.

Well... instead of just reverting the changes abruptly, you could add a tween before calling acceptDrop, and then acceptDrop could call removeTweens (or overwrite them with its own tweens) if it didn't like the default. But that's still pretty awkward.

As far as I can tell, this is a straight-up improvement with no regressions and as the bug 610191 patches are applied the remaining obvious issues with dragging in the linear view will be fixed. Pushed.

Attachment 156802 [details] pushed as dfb110c - [DND] Use Params for DND.makeDraggable()
Attachment 156803 [details] pushed as 94472ba - [DND] Allow setting drag actor max size and opacity
Attachment 156804 [details] pushed as 2a74044 - [workspaces] Don't use a clone for the window drag actor

*** Bug 613188 has been marked as a duplicate of this bug. ***