GNOME Bugzilla – Bug 619203
DND regressions
Last modified: 2010-05-27 10:13:50 UTC
When introducing DND in linear view, the dragging of window previews to the dash (to fav the app) regressed: - in linear view, it is not working at all - in grid view, the preview is swallowed by the dash The dragging of previews to the dash might not be the most useful feature, but when we start zooming out on other drag actions as well (see bug 618055), any drop on the dash stops working (e.g. icons from the app menu) - the first patch fixes those more useful cases as well.
Created attachment 161555 [details] [review] [linearView] Fix dnd to dash when zoomed out While zoomed out, the workspaces view's drop target spans the entire monitor to implement reactive screen edges. When a drop event is not handled by the view, an attempt is made to pass it on down the stack, but it doesn't work properly. Fix it by iterating the target's parents as well.
Created attachment 161556 [details] [review] [dnd] Restore original opacity when cancelling a drag It is possible to use a different scale and opacity during drag operations - while size and position are restored after cancelling the drag, the opacity is not. Instead of doing so manually in response to the 'drag-end' signal, handle it automagically as well.
Created attachment 161557 [details] [review] [dnd] Optionally restore the drag actor Currently, the drag and drop code assumes that on a successful drop the target will either consume the drag actor or that it is otherwise OK to destroy the actor. As the drag behavior for window preview was changed, dropping a preview on the dash now results in the preview being swallowed - to fix, add an option to restore the actor in case of a successful drop as well.
Review of attachment 161555 [details] [review]: This is pretty horrible to contemplate :-) - without digging into the scariness too much, I'm positive there was a better solution than just raising an actor over the whole stage - dnd.js should be the piece of code that is coordinating what happens where during a drag. It can't be right that our most important drag-and-drop code path raises a big actor over everything during the drag and takes dnd.js out of the picture. So, maybe smaller actors should have been raised just where special handling was needed, maybe dnd.js needed some API enhancements. That being said, this looks like a reasonable fix, with one detail I don't like. ::: js/ui/workspacesView.js @@ +1145,3 @@ + return target._delegate.acceptDrop(source, dropActor, + (x - targX) / target.scale_x, + (y - targY) / target.scale_y, Not entirely a new problem here, but this is a weird mix between caring about the target scale and not caring about the target scale - target.scale_x is only the immediate scale on the actor itself, so this only works properly if just leaf nodes are scaled. I think it's probably best to use actor.transform_stage_point(x, y) here. (For our 2D case, the implementation is hugely overcomplicated, but we're only doing this for few actors on release, and target.get_transformed_position() is already not cheap.)
Review of attachment 161556 [details] [review]: Doesn't look necessary to me: dragOrigOpacity is already saved a little further down in the function where you added it the tween that onSnapbackComplete is the complete function for already restores the opacity to this._dragOrigOpacity (The reason that setting the position in the complete is necessary is because of the reparent - it *looks* like it is in the right position at the end of the tween, but once we reparent it it needs a different x/y)
Review of attachment 161557 [details] [review]: Looks like this depends on the patch I rejected, so needs redoing for that. Would it look better if we tweened the actor in from zero opacity to avoid a blink at the old location? Would this be better if it was a hook rather than a boolean - allow the source to do whatever it wanted with the actor after a successful drop? (For the latter, I can't think of anything it would make sense to do other than to just fade the actor back in, and if fading back in one usage makes sense it probably always makes sense, so a hook is likely unnecessary complexity.)
(In reply to comment #4) > Review of attachment 161555 [details] [review]: > > This is pretty horrible to contemplate :-) - without digging into the scariness > too much, I'm positive there was a better solution than just raising an actor > over the whole stage I agree. Note that my comment about the reactive screen edges is my interpretation of the code, there may be more to it. If not, adding a 'screen-edge-reached' or something to dnd.js may be a better solution (and possibly dnd should be moved to St anyway - it appears MX has added dnd, looking into it is on my todo list) > That being said, this looks like a reasonable fix, with one detail I don't > like. [...] > I think it's probably best to use actor.transform_stage_point(x, y) here. Not sure if I understand you correctly - do you mean this: diff --git a/js/ui/workspacesView.js b/js/ui/workspacesView.js index 241390d..f66196c 100644 --- a/js/ui/workspacesView.js +++ b/js/ui/workspacesView.js @@ -1180,11 +1180,9 @@ SingleView.prototype = { target._delegate != this && target._delegate.acceptDrop) { - let [targX, targY] = target.get_transformed_position(); + let [targX, targY] = target.transform_stage_point(x, y); return target._delegate.acceptDrop(source, dropActor, - (x - targX) / target.scale_x, - (y - targY) / target.scale_y, - time); + targX, targY, time); } target = target.get_parent(); } If so, should the same change be applied in dnd as well (mostly identical code in _updateDragPosition and _dragActorDropped)?
(In reply to comment #5) > Doesn't look necessary to me: > > dragOrigOpacity is already saved a little further down in the function where > you added it Duh. > the tween that onSnapbackComplete is the complete function for already > restores the opacity to this._dragOrigOpacity So the patch fixed a bug in the other patch which did not call onSnapbackComplete when reverting the actor ...
Created attachment 161741 [details] [review] [dnd] Optionally restore the drag actor (In reply to comment #6) > Would it look better if we tweened the actor in from zero opacity to avoid a > blink at the old location? Yes. > Would this be better if it was a hook rather than a boolean - allow the source > to do whatever it wanted with the actor after a successful drop? > > (For the latter, I can't think of anything it would make sense to do other than > to just fade the actor back in, and if fading back in one usage makes sense it > probably always makes sense, so a hook is likely unnecessary complexity.) Zooming the window in may be an alternative. A hook might make it possible to have the revert animation in parallel with the desktop zoom (but I think the same should apply for the snap back, which a hook wouldn't fix ...)
Review of attachment 161741 [details] [review]: Looks basically good to commit, except one chunk of cut-and-pasted code ::: js/ui/dnd.js @@ +393,3 @@ + let snapBackY = this._snapBackY; + if (this._dragActorSource && this._dragActorSource.visible) { + [snapBackX, snapBackY] = this._dragActorSource.get_transformed_position(); Hmm, I'm not sure that the whole concept of restore makes sense in the case where dragActorSource is set. But reusing this code with both cases is fine *if* you move it out to a helper function - like '[targetX, targetY] = this._getRestoreLocation()' or something. Cutting and pasting the logic, not so OK :-) @@ +398,3 @@ + this._dragActor.set_position(snapBackX, snapBackY); + this._dragActor.set_scale(this._snapBackScale, this._snapBackScale); + this._dragActor.opacity = 0; Could use a comment like 'fade the actor back in at its original location' here - it's not too hard to figure what it's doing, but takes a little reading to do so.
Created attachment 161980 [details] [review] [dnd] Optionally restore the drag actor OK. What about the questions raised in comment 7?
(In reply to comment #7) > > That being said, this looks like a reasonable fix, with one detail I don't > > like. > [...] > > I think it's probably best to use actor.transform_stage_point(x, y) here. > > Not sure if I understand you correctly - do you mean this: > > diff --git a/js/ui/workspacesView.js b/js/ui/workspacesView.js > index 241390d..f66196c 100644 > --- a/js/ui/workspacesView.js > +++ b/js/ui/workspacesView.js > @@ -1180,11 +1180,9 @@ SingleView.prototype = { > target._delegate != this && > target._delegate.acceptDrop) { > > - let [targX, targY] = target.get_transformed_position(); > + let [targX, targY] = target.transform_stage_point(x, y); > return target._delegate.acceptDrop(source, dropActor, > - (x - targX) / > target.scale_x, > - (y - targY) / > target.scale_y, > - time); > + targX, targY, time); > } > target = target.get_parent(); > } > > If so, should the same change be applied in dnd as well (mostly identical code > in _updateDragPosition and _dragActorDropped)? Yes and yes.
Review of attachment 161980 [details] [review]: Looks good
Created attachment 161997 [details] [review] [linearView] Fix dnd to dash when zoomed out (In reply to comment #13) > Yes and yes. Yes.
Comment on attachment 161980 [details] [review] [dnd] Optionally restore the drag actor Attachment 161980 [details] pushed as 3e2a9a5 - [dnd] Optionally restore the drag actor
Created attachment 162026 [details] [review] [linearView] Fix dnd to dash when zoomed out Mmmh, transform_stage_point() actually returns a triplet ...
Review of attachment 162026 [details] [review]: Looks good (wondered for a bit whether you should check r, but if the pointer is over an actor, than none of its parents can have a singular transform - can be transformed into a line - since then the actor itself would have a singular transform and the pointer couldn't be in it.)
Attachment 162026 [details] pushed as b3794f1 - [linearView] Fix dnd to dash when zoomed out