I sent a similar proposal to the mailing list, which could complement this:

"In gnome 2.x I can drag an element (selected text, files...) to another window even if it's minimised or hidden, by first dragging it to its task list entry (which will bring the window to the top).

I have found that there is no way to do something similar in GS, and honestly, it kind of disrupts my work. Therefore, I would like to be able to drag those elements to the activities window to get an overview of all windows while still holding the selected items, and be able to drop them in any given window.

Maybe hold the cursor over a window for a couple seconds to close the overlay mode, in case you need to drop the elements in a specific part of the window (e.g. you want to drag a video to totem's sidebar menu to queue it, you want to drag text into a certain place in a document/form, or a file into a certain directory in a file manager)."

Ok, a lot of different things can happen, and all would be different features (different bugs for later?) but all would stem from point #1:

1- Overview launches when an item selected by the mouse is dragged to the hot corner. This could include files, selected text, images from the web, urls...
** maybe also launch overview on a prolonged hold over the activities button? 

2- Dropping a file on open workspace or '+' button launches the file with its preferred/default application in said workspace. Linear view would behave as mocked-up in bug 607821.

3- Hold the selected item over a window from any desktop to call this window to attention so the user can drop it there: images into gimp; selected text to gedit/email/text field/development app; video file to a playlist, etcetera. Linear view would again behave as mocked-up in bug 607821.

4- Drag a window into the overview to re-locate it on another workspace or in a new one by dropping over '+'. The window would become miniaturized as per mccann's mockups from bug 607821 again (zaspire's patch https://bugzilla.gnome.org/attachment.cgi?id=154734 kind of does this already)

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

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

Created attachment 165607 [details] [review]
[GdkWindowCache] Don't ignore the CompositeOverlayWindow's children

The CompositeOverlayWindow is invisible to XQueryTree which results into
not only the COW itself but all of it's children being ignored in
gdk_window_cache_new, which in some cases are reasonable drag & drop targets
(like gnome-shell's stage window).

Created attachment 165608 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover a over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

----

The attached gtk patch is needed for interacting with gtk apps.

Created attachment 165612 [details] [review]
Implement cross overview drag & drop

Rebase (last patch was against a locally patched shell).

Created attachment 165617 [details] [review]
Implement cross overview drag & drop

*) Add new file to Makefile.am

Created attachment 165619 [details] [review]
Implement cross overview drag & drop

*) Some cleanups

Created attachment 165621 [details] [review]
Implement cross overview drag & drop

Attach the correct one, sorry for the spam.

Created attachment 165627 [details] [review]
Implement cross overview drag & drop

The last cleanup contained a stupid copy & paste typo that broke draging to leftworkspaces, fix that.

Created attachment 165640 [details] [review]
Implement cross overview drag & drop

*) Don't hide the drag icon while being in the overview
*) Some more random cleanups and whitespace fixes

Created attachment 165641 [details] [review]
Implement cross overview drag & drop

*) Pixel align drag icon

Created attachment 165682 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover a over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

----

Yet another bugfix.

Created attachment 165690 [details] [review]
Implement cross overview drag & drop

*) Fix typo in the commit message
*) Highlight workspaces on hover (to be consistent with the default overview dnd)

Created attachment 165696 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

Created attachment 165720 [details] [review]
Implement cross overview drag & drop

*) Use MetaRectangle rather than GdkRectangle

Created attachment 166405 [details] [review]
Implement cross overview drag & drop

Rebase to master.

Created attachment 166406 [details] [review]
Implement cross overview drag & drop

Fix bug in previous rebase

Review of attachment 165607 [details] [review]:

OK, so I don't buy the "composite overlay window as a second root window" thing - just add the COW itself to the window cache and make sure that the corresponding gnome-shell code is setting XdndAware on the COW not on the stage window.

Other than that, I think this patch makes sense.

::: gdk/x11/gdkdnd-x11.c
@@ +588,3 @@
   g_free (children);
 
+  if (gdk_screen_is_composited (screen))

You need to comment what's going on with this check - and explain that you are discounting the case of a compositing manager without a composite overlay window. I was able to guess what was going on, but it is a bit subtle.

@@ +590,3 @@
+  if (gdk_screen_is_composited (screen))
+    {
+      cow = XCompositeGetOverlayWindow (gdk_x11_get_default_xdisplay (), GDK_WINDOW_XWINDOW (root_window));

GDK_SCREEN_XDISPLAY (screen)

Review of attachment 166406 [details] [review]:

Very neat that you got this working!

The code in xdndHandler.js needs some cleanup. Hopefully my comments will give you some ideas. Let me know if you have questions.

::: js/ui/overview.js
@@ +193,2 @@
         this.visible = false;
+        this.grab = true;

I don't think modelling this as a property that is changed externally is a good idea. (If it is changed while the overview is visible, for one thing, things will get all confused.)

My idea for an API would be to have both show/hide and showTemporary/hideTemporary where we are visible if shown || temporarilyShown but only grabbed if shown.

::: js/ui/workspace.js
@@ +112,3 @@
 
+        /* Used by xdndHandler to track windows */
+        this.actor.metaWindow = realWindow.meta_window;

Not needd - you can use this.actor._delegate.metaWindow

::: js/ui/xdndHandler.js
@@ +22,3 @@
+        this._x = 0;
+        this._y = 0;
+        this._switchTimeoutId = 0;

Think this would be better as 'hoverTimeoutId' since you use it for different sorts of hovers.

@@ +24,3 @@
+        this._switchTimeoutId = 0;
+        this._lastActiveWorkspace = null;
+        this._clone = null;

Calling this "clone" isn't much more descriptive than calling it "actor" or "foo" - should be dragIconClone or something like that.

@@ +29,3 @@
+        global.init_xdnd();
+        global.connect('xdnd-position-changed', Lang.bind(this, this._positionHandler));
+        global.connect('xdnd-leave', Lang.bind(this, this._onLeave));

Different naming conventions for the two handlers - (onBlah is my general preference for a naming convention.)

@@ +42,3 @@
+
+            Main.overview.hide();
+            this._lastActiveWorkspace.activate(global.get_current_time());

This is the multihead case where you can mouse out of the primary monitor? It definitely needs to be commented!

The use of global.get_currnet_time() points out that you need to propagate the timestamp from the Xdnd events into global.get_current_time()

@@ +53,3 @@
+
+        if (activeWorkspaceIndex > 0 && ((this._x <= primary.x && this._y > Panel.PANEL_HEIGHT) ||
+            target.name == 'left-workspaces-shadow')) {

What's the reason for duplicating the check here? Worry about actor positions changing? If you need to duplicate the check, then you need to move the check logic into a function - _isLeftWorkspaceHover() or something.

@@ +55,3 @@
+            target.name == 'left-workspaces-shadow')) {
+              global.screen.get_workspace_by_index(activeWorkspaceIndex - 1).activate(global.get_current_time());
+              this._positionHandler(null, ((this._x << 16) | this._y));

Do we really want to keep bumping over workspaces if the user never moves the mouse again? I'm not sure which way is right, but if we aren't sure we want the behavior, cleaner not to have the call to _positionHandler

@@ +87,3 @@
+        if (Main.overview.visible && this._x == primary.x && this._y == primary.y) {
+            Main.panel.rippleAnimation();
+            this._onLeave();

You can't call an onLeave handler when something that happens which is clearly not a leave

@@ +118,3 @@
+                let windows = global.get_windows();
+                let cursorWindow = windows[windows.length - 1];
+                /* FIXME: more reliable way? */

A comment! ;-)

@@ +136,3 @@
+                this._removeTimeout();
+                this._switchTimeoutId = Mainloop.timeout_add(WORKSPACE_SWITCH_TIMEOUT,
+                                                                Lang.bind(this, this._switchToLeftWorkspace));

This type of logic where you are continually removing timeouts every time you get a motion can make it hard for people with jittery hands or jittery mice to trigger the behavior. But I don't want to make this patch more complicated by adding thresholds, etc, so lets leave it that way for now..

@@ +139,3 @@
+                actor.hide();
+                this._workspaceActor = this._pickActor();
+                this._workspaceActor.opacity = 255;

I'm sure this horrible hack is doing something important, but I have no idea what.

@@ +161,3 @@
+                this._removeTimeout();
+                this._switchTimeoutId = Mainloop.timeout_add(HOT_CORNER_TIMEOUT,
+                                                                Lang.bind(this, this._hotCornerExit));

This doesn't seem to work for me - I can't get out of the overview once I get in.

@@ +166,3 @@
+        }
+
+        if (actor.name == 'panelActivities' || (this._x == primary.x && this._y == primary.y)) {

You have the this._x == primary.x .. check both here and above, so both will trigger? This is probably why leaving the overview isnt' working for me. Did you want all this code in an else ?

@@ +169,3 @@
+            Main.overview.grab = false;
+            if (this._x == primary.x && this._y == primary.y)
+                Main.panel.rippleAnimation();

You are continually adding new ripples as the pointer is in in the corner. Looks weird, is slow.

@@ +173,3 @@
+            Main.overview.beginItemDrag(null);
+        }
+        else if (actor.metaWindow) {

Is there a reason this isn't in the overview-active section?

@@ +175,3 @@
+        else if (actor.metaWindow) {
+                this._removeTimeout();
+                this._switchTimeoutId = Mainloop.timeout_add(WINDOW_SWITCH_TIMEOUT, Lang.bind(this, function () {

Why an inline function here when you have separate methods for other hovers?

::: src/shell-global.c
@@ +22,3 @@
 #include <dbus/dbus-glib.h>
 #include <gio/gio.h>
+#include <X11/extensions/Xcomposite.h>

You don't use this

@@ +38,3 @@
+#define XDND_STATUS_WANT_POSITION_SET(e,b) (e)->xclient.data.l[1] = ((e)->xclient.data.l[1] & ~0x2UL) | (((b) == 0) ? 0 : 0x2UL)
+#define XDND_STATUS_RECT_SET(e,x,y,w,h)		{(e)->xclient.data.l[2] = ((x) << 16) | ((y) & 0xFFFFUL); (e)->xclient.data.l[3] = ((w) << 16) | ((h) & 0xFFFFUL); }
+#define XDND_STATUS_ACTION(e)		((e)->xclient.data.l[4])

If we use this code, it would need to have the copyright and source information (according to IRC conversation, it comes from the Xdnd sample implementation), but IMO, it's too ugly to use. (And uses the GCC statement expression extension)

Just code out setting and reading the xclient fields directly. See gdkdnd-x11.c for how I did it there.

@@ +1196,3 @@
+      XDND_STATUS_WILL_ACCEPT_SET (&xevent, FALSE);
+      XDND_STATUS_WANT_POSITION_SET(&xevent, TRUE);
+      XDND_STATUS_RECT_SET(&xevent, primary->x, primary->y, primary->width, primary->height);

Since you set bit 2 in the flags, then this rectangle is ignored, so there is no reason to set l[2]/l[3] to anything but 0/0.

@@ +1203,3 @@
+      g_boxed_free (META_TYPE_RECTANGLE, primary);
+
+      g_signal_emit (G_OBJECT (global), shell_global_signals[XDND_POSITION_CHANGED], 0, (guint) xev->xclient.data.l[2]);

You should decode the x,y positions and pass them as two argument to the signal. This will mean changing how you define the signal and making an addition to shell-marshal.list. (See how we use _shell_marshal_* in shell-wm.c)

@@ +1291,3 @@
+ * @global: the #ShellGlobal
+ *
+ * Enables tracking of xdnd events

Xdnd when used as a name in text

@@ +1292,3 @@
+ *
+ * Enables tracking of xdnd events
+ *

Extra blank line

@@ +1295,3 @@
+ */
+
+void shell_global_init_xdnd (ShellGlobal *global) {

void
shell_global_init_xdnd (ShellGlobal *global)
{

@@ +1296,3 @@
+
+void shell_global_init_xdnd (ShellGlobal *global) {
+  long int xdnd_version = 3;

just long not 'long int'

@@ +1297,3 @@
+void shell_global_init_xdnd (ShellGlobal *global) {
+  long int xdnd_version = 3;
+  Display *dpy = gdk_x11_get_default_xdisplay ();

xdisplay not dpy

@@ +1300,3 @@
+  ClutterStage *stage = CLUTTER_STAGE(mutter_plugin_get_stage (global->plugin));
+
+  XChangeProperty (dpy, clutter_x11_get_stage_window (stage), gdk_x11_get_xatom_by_name ("XdndAware"), XA_ATOM,

Should set on the overlay window not the stage window, see comments on other patch

@@ +1301,3 @@
+
+  XChangeProperty (dpy, clutter_x11_get_stage_window (stage), gdk_x11_get_xatom_by_name ("XdndAware"), XA_ATOM,
+                    32, PropModeAppend, (const unsigned char *)&xdnd_version, 1);

Using PropModeAppend instead of PropModeReplace here is weird.

@@ +1303,3 @@
+                    32, PropModeAppend, (const unsigned char *)&xdnd_version, 1);
+
+  clutter_x11_add_filter (stage_event_filter, global);

Since you are setting XdndAware on the stage window, which isn't a clutter window, you'll need to add a GDK filter function, or better, I think, handle it out of gnome_shell_plugin_xevent_filter - the reason I think that's better is that the more independent places we have filtering the X event stream, the more confusing it gets how they all interact.

(In reply to comment #20)
> Review of attachment 165607 [details] [review]:
> 
> OK, so I don't buy the "composite overlay window as a second root window" thing
> - just add the COW itself to the window cache and make sure that the
> corresponding gnome-shell code is setting XdndAware on the COW not on the stage
> window.

OK

> Other than that, I think this patch makes sense.
> 
> ::: gdk/x11/gdkdnd-x11.c
> @@ +588,3 @@
>    g_free (children);
> 
> +  if (gdk_screen_is_composited (screen))
> 
> You need to comment what's going on with this check - and explain that you are
> discounting the case of a compositing manager without a composite overlay
> window. I was able to guess what was going on, but it is a bit subtle.

OK makes sense.

(In reply to comment #21)
> Review of attachment 166406 [details] [review]:
> 
> Very neat that you got this working!

;)

Thanks for the review.

> The code in xdndHandler.js needs some cleanup. Hopefully my comments will give
> you some ideas. Let me know if you have questions.

Questions are inline.

> ::: js/ui/overview.js
> @@ +193,2 @@
>          this.visible = false;
> +        this.grab = true;
> 
> I don't think modelling this as a property that is changed externally is a good
> idea. (If it is changed while the overview is visible, for one thing, things
> will get all confused.)
> 
> My idea for an API would be to have both show/hide and
> showTemporary/hideTemporary where we are visible if shown || temporarilyShown
> but only grabbed if shown.

OK this makes sense.

> ::: js/ui/workspace.js
> @@ +112,3 @@
> 
> +        /* Used by xdndHandler to track windows */
> +        this.actor.metaWindow = realWindow.meta_window;
> 
> Not needd - you can use this.actor._delegate.metaWindow

OK

> ::: js/ui/xdndHandler.js
> @@ +22,3 @@
> +        this._x = 0;
> +        this._y = 0;
> +        this._switchTimeoutId = 0;
> 
> Think this would be better as 'hoverTimeoutId' since you use it for different
> sorts of hovers.

OK

> @@ +24,3 @@
> +        this._switchTimeoutId = 0;
> +        this._lastActiveWorkspace = null;
> +        this._clone = null;
> 
> Calling this "clone" isn't much more descriptive than calling it "actor" or
> "foo" - should be dragIconClone or something like that.

Yeah clone itself does indeed mean nothing.

> @@ +29,3 @@
> +        global.init_xdnd();
> +        global.connect('xdnd-position-changed', Lang.bind(this,
> this._positionHandler));
> +        global.connect('xdnd-leave', Lang.bind(this, this._onLeave));
> 
> Different naming conventions for the two handlers - (onBlah is my general
> preference for a naming convention.)

OK

> @@ +42,3 @@
> +
> +            Main.overview.hide();
> +            this._lastActiveWorkspace.activate(global.get_current_time());
> 
> This is the multihead case where you can mouse out of the primary monitor? It
> definitely needs to be commented!

Not sure I get what you are saying this is to get back to the workspace the user started from in case he moved to another one and canceled the drag (i.e release the mouse button).

> The use of global.get_currnet_time() points out that you need to propagate the
> timestamp from the Xdnd events into global.get_current_time()

The question here is does it really matter?

> @@ +53,3 @@
> +
> +        if (activeWorkspaceIndex > 0 && ((this._x <= primary.x && this._y >
> Panel.PANEL_HEIGHT) ||
> +            target.name == 'left-workspaces-shadow')) {
> 
> What's the reason for duplicating the check here? Worry about actor positions
> changing? 

No but about the mouse moving away before the hover timeout.

>If you need to duplicate the check, then you need to move the check
> logic into a function - _isLeftWorkspaceHover() or something.

OK.

> @@ +55,3 @@
> +            target.name == 'left-workspaces-shadow')) {
> +              global.screen.get_workspace_by_index(activeWorkspaceIndex -
> 1).activate(global.get_current_time());
> +              this._positionHandler(null, ((this._x << 16) | this._y));
> 
> Do we really want to keep bumping over workspaces if the user never moves the
> mouse again? I'm not sure which way is right, but if we aren't sure we want the
> behavior, cleaner not to have the call to _positionHandler

Hmm ... lets say you are on workspace 3 and want to go to 1 you'd want to "bump over the workspaces" or you'll get annoyed.

> @@ +87,3 @@
> +        if (Main.overview.visible && this._x == primary.x && this._y ==
> primary.y) {
> +            Main.panel.rippleAnimation();
> +            this._onLeave();
> 
> You can't call an onLeave handler when something that happens which is clearly
> not a leave

OK, will strip the logic out into a separate function.

> @@ +118,3 @@
> +                let windows = global.get_windows();
> +                let cursorWindow = windows[windows.length - 1];
> +                /* FIXME: more reliable way? */
> 
> A comment! ;-)

Gah! How did I miss this ;)

But yeah I should add more of them.

> @@ +136,3 @@
> +                this._removeTimeout();
> +                this._switchTimeoutId =
> Mainloop.timeout_add(WORKSPACE_SWITCH_TIMEOUT,
> +                                                               
> Lang.bind(this, this._switchToLeftWorkspace));
> 
> This type of logic where you are continually removing timeouts every time you
> get a motion can make it hard for people with jittery hands or jittery mice to
> trigger the behavior. But I don't want to make this patch more complicated by
> adding thresholds, etc, so lets leave it that way for now..

OK, it seems to work fine for me using a touchpad though.

> @@ +139,3 @@
> +                actor.hide();
> +                this._workspaceActor = this._pickActor();
> +                this._workspaceActor.opacity = 255;
> 
> I'm sure this horrible hack is doing something important, but I have no idea
> what.

Ugh .. what this is trying to do is: Highlight the adjacent workspace when hovering over it. 
How it does it: When we got the workspace shadow under the mouse it is moved "out of sight" (i.e hidden) then we pick again to get the workspace actor to be able to change it's opacity.

Without this (horrible hack) it feels odd that the workspaces only highlight when dragging windows or items from the dash but not here.

> @@ +161,3 @@
> +                this._removeTimeout();
> +                this._switchTimeoutId =
> Mainloop.timeout_add(HOT_CORNER_TIMEOUT,
> +                                                               
> Lang.bind(this, this._hotCornerExit));
> 
> This doesn't seem to work for me - I can't get out of the overview once I get
> in.

It seems to work here. Going to the overview, back to the corner and wait a bit.
The "wait a bit" part does not feel right though. I have added the delay to avoid the case where you go to the workspace move the mouse a bit and find yourself out of the overview again.

> @@ +166,3 @@
> +        }
> +
> +        if (actor.name == 'panelActivities' || (this._x == primary.x &&
> this._y == primary.y)) {
> 
> You have the this._x == primary.x .. check both here and above, so both will
> trigger? This is probably why leaving the overview isnt' working for me. Did
> you want all this code in an else ?

Good catch! This makes the above timeout hack redundant with a nice side effect that the "wait a bit" part is gone.

> @@ +169,3 @@
> +            Main.overview.grab = false;
> +            if (this._x == primary.x && this._y == primary.y)
> +                Main.panel.rippleAnimation();
> 
> You are continually adding new ripples as the pointer is in in the corner.
> Looks weird, is slow.

Indeed will fix.

> @@ +173,3 @@
> +            Main.overview.beginItemDrag(null);
> +        }
> +        else if (actor.metaWindow) {
> 
> Is there a reason this isn't in the overview-active section?

None that I can recall and it looks odd to me now, will move to the other section.

> @@ +175,3 @@
> +        else if (actor.metaWindow) {
> +                this._removeTimeout();
> +                this._switchTimeoutId =
> Mainloop.timeout_add(WINDOW_SWITCH_TIMEOUT, Lang.bind(this, function () {
> 
> Why an inline function here when you have separate methods for other hovers?

History ;)
Before I added the clones it was a rather short function so I preferred to have it inline but now it makes sense to move it out.

> ::: src/shell-global.c
> @@ +22,3 @@
>  #include <dbus/dbus-glib.h>
>  #include <gio/gio.h>
> +#include <X11/extensions/Xcomposite.h>
> 
> You don't use this

Indeed but know that we want to use the COW I will.

> @@ +38,3 @@
> +#define XDND_STATUS_WANT_POSITION_SET(e,b) (e)->xclient.data.l[1] =
> ((e)->xclient.data.l[1] & ~0x2UL) | (((b) == 0) ? 0 : 0x2UL)
> +#define XDND_STATUS_RECT_SET(e,x,y,w,h)        {(e)->xclient.data.l[2] = ((x)
> << 16) | ((y) & 0xFFFFUL); (e)->xclient.data.l[3] = ((w) << 16) | ((h) &
> 0xFFFFUL); }
> +#define XDND_STATUS_ACTION(e)        ((e)->xclient.data.l[4])
> 
> If we use this code, it would need to have the copyright and source information
> (according to IRC conversation, it comes from the Xdnd sample implementation),
> but IMO, it's too ugly to use. (And uses the GCC statement expression
> extension)
> 
> Just code out setting and reading the xclient fields directly. See gdkdnd-x11.c
> for how I did it there.

OK, will look at that. 

> @@ +1196,3 @@
> +      XDND_STATUS_WILL_ACCEPT_SET (&xevent, FALSE);
> +      XDND_STATUS_WANT_POSITION_SET(&xevent, TRUE);
> +      XDND_STATUS_RECT_SET(&xevent, primary->x, primary->y, primary->width,
> primary->height);
> 
> Since you set bit 2 in the flags, then this rectangle is ignored, so there is
> no reason to set l[2]/l[3] to anything but 0/0.

As we agreed on IRC this line is nonsense will remove it.

> @@ +1203,3 @@
> +      g_boxed_free (META_TYPE_RECTANGLE, primary);
> +
> +      g_signal_emit (G_OBJECT (global),
> shell_global_signals[XDND_POSITION_CHANGED], 0, (guint)
> xev->xclient.data.l[2]);
> 
> You should decode the x,y positions and pass them as two argument to the
> signal. This will mean changing how you define the signal and making an
> addition to shell-marshal.list. (See how we use _shell_marshal_* in shell-wm.c)

OK

> @@ +1291,3 @@
> + * @global: the #ShellGlobal
> + *
> + * Enables tracking of xdnd events
> 
> Xdnd when used as a name in text

OK

> @@ +1292,3 @@
> + *
> + * Enables tracking of xdnd events
> + *
> 
> Extra blank line

OK

> @@ +1295,3 @@
> + */
> +
> +void shell_global_init_xdnd (ShellGlobal *global) {
> 
> void
> shell_global_init_xdnd (ShellGlobal *global)
> {
> 
> @@ +1296,3 @@
> +
> +void shell_global_init_xdnd (ShellGlobal *global) {
> +  long int xdnd_version = 3;
> 
> just long not 'long int'

OK

> @@ +1297,3 @@
> +void shell_global_init_xdnd (ShellGlobal *global) {
> +  long int xdnd_version = 3;
> +  Display *dpy = gdk_x11_get_default_xdisplay ();
> 
> xdisplay not dpy

OK

> @@ +1300,3 @@
> +  ClutterStage *stage = CLUTTER_STAGE(mutter_plugin_get_stage
> (global->plugin));
> +
> +  XChangeProperty (dpy, clutter_x11_get_stage_window (stage),
> gdk_x11_get_xatom_by_name ("XdndAware"), XA_ATOM,
> 
> Should set on the overlay window not the stage window, see comments on other
> patch

OK

> @@ +1301,3 @@
> +
> +  XChangeProperty (dpy, clutter_x11_get_stage_window (stage),
> gdk_x11_get_xatom_by_name ("XdndAware"), XA_ATOM,
> +                    32, PropModeAppend, (const unsigned char *)&xdnd_version,
> 1);
> 
> Using PropModeAppend instead of PropModeReplace here is weird.
> 
> @@ +1303,3 @@
> +                    32, PropModeAppend, (const unsigned char *)&xdnd_version,
> 1);
> +
> +  clutter_x11_add_filter (stage_event_filter, global);
> 
> Since you are setting XdndAware on the stage window, which isn't a clutter
> window, you'll need to add a GDK filter function,

I assume you mean s/stage/overlay/ otherwise this does not make sense.

> or better, I think, handle it
> out of gnome_shell_plugin_xevent_filter - the reason I think that's better is
> that the more independent places we have filtering the X event stream, the more
> confusing it gets how they all interact.

OK (which means we don't need xcomposite.h after all)

(In reply to comment #23)

> > @@ +42,3 @@
> > +
> > +            Main.overview.hide();
> > +            this._lastActiveWorkspace.activate(global.get_current_time());
> > 
> > This is the multihead case where you can mouse out of the primary monitor? It
> > definitely needs to be commented!
> 
> Not sure I get what you are saying this is to get back to the workspace the
> user started from in case he moved to another one and canceled the drag (i.e
> release the mouse button).

Ah, so the point is that you get a XdndLeave when the drag is cancelled. A comment above the onLeave handler along the lines of // Called when the user drags into another window and also when the drag is cancelled
would help someone reading the code.

> > The use of global.get_currnet_time() points out that you need to propagate the
> > timestamp from the Xdnd events into global.get_current_time()
> 
> The question here is does it really matter?

That question may be harder to answer than just propagating timestamps.

> > @@ +53,3 @@
> > +
> > +        if (activeWorkspaceIndex > 0 && ((this._x <= primary.x && this._y >
> > Panel.PANEL_HEIGHT) ||
> > +            target.name == 'left-workspaces-shadow')) {
> > 
> > What's the reason for duplicating the check here? Worry about actor positions
> > changing? 
> 
> No but about the mouse moving away before the hover timeout.

But if the mouse moves, won't you remove the timeout and re-add it?

> > @@ +55,3 @@
> > +            target.name == 'left-workspaces-shadow')) {
> > +              global.screen.get_workspace_by_index(activeWorkspaceIndex -
> > 1).activate(global.get_current_time());
> > +              this._positionHandler(null, ((this._x << 16) | this._y));
> > 
> > Do we really want to keep bumping over workspaces if the user never moves the
> > mouse again? I'm not sure which way is right, but if we aren't sure we want the
> > behavior, cleaner not to have the call to _positionHandler
> 
> Hmm ... lets say you are on workspace 3 and want to go to 1 you'd want to "bump
> over the workspaces" or you'll get annoyed.

Maybe it would be OK to have to move the mouse a bit have to bump over? Was the call to positionHandler something you added because it felt bad without it?
 
> > @@ +139,3 @@
> > +                actor.hide();
> > +                this._workspaceActor = this._pickActor();
> > +                this._workspaceActor.opacity = 255;
> > 
> > I'm sure this horrible hack is doing something important, but I have no idea
> > what.
> 
> Ugh .. what this is trying to do is: Highlight the adjacent workspace when
> hovering over it. 
> How it does it: When we got the workspace shadow under the mouse it is moved
> "out of sight" (i.e hidden) then we pick again to get the workspace actor to be
> able to change it's opacity.
> 
> Without this (horrible hack) it feels odd that the workspaces only highlight
> when dragging windows or items from the dash but not here.

Hmm, you should at least add API to workspacesView to set the hover state and not just change opacity directly. Not sure if there's annything better to do than to hide the shadow and to show it again- I may need to ask Florian or someone else who understands the workspacesView to look over this part of the changes once we have the Xdnd specifics settled.

> > Since you are setting XdndAware on the stage window, which isn't a clutter
> > window, you'll need to add a GDK filter function,
> 
> I assume you mean s/stage/overlay/ otherwise this does not make sense.

Yes, I meant the overlay window.

(In reply to comment #24)
> (In reply to comment #23)
> 
> > > @@ +42,3 @@
> > > +
> > > +            Main.overview.hide();
> > > +            this._lastActiveWorkspace.activate(global.get_current_time());
> > > 
> > > This is the multihead case where you can mouse out of the primary monitor? It
> > > definitely needs to be commented!
> > 
> > Not sure I get what you are saying this is to get back to the workspace the
> > user started from in case he moved to another one and canceled the drag (i.e
> > release the mouse button).
> 
> Ah, so the point is that you get a XdndLeave when the drag is cancelled. A
> comment above the onLeave handler along the lines of // Called when the user
> drags into another window and also when the drag is cancelled
> would help someone reading the code.

OK

> > > The use of global.get_currnet_time() points out that you need to propagate the
> > > timestamp from the Xdnd events into global.get_current_time()
> > 
> > The question here is does it really matter?
> 
> That question may be harder to answer than just propagating timestamps.

OK, fair enough.

> > > @@ +53,3 @@
> > > +
> > > +        if (activeWorkspaceIndex > 0 && ((this._x <= primary.x && this._y >
> > > Panel.PANEL_HEIGHT) ||
> > > +            target.name == 'left-workspaces-shadow')) {
> > > 
> > > What's the reason for duplicating the check here? Worry about actor positions
> > > changing? 
> > 
> > No but about the mouse moving away before the hover timeout.
> 
> But if the mouse moves, won't you remove the timeout and re-add it?

The point here is the pointer is over a workspace; the timeout starts; the user moves to a "dead spot" (one that does not trigger any timeouts) it will result into the workspaces being changed which isn't expected here. So the handler checks after the timeout whether the user is still hovering over the actor which triggered the timeout (i.e "are the conditions that triggered the timeout still met").

> > > @@ +55,3 @@
> > > +            target.name == 'left-workspaces-shadow')) {
> > > +              global.screen.get_workspace_by_index(activeWorkspaceIndex -
> > > 1).activate(global.get_current_time());
> > > +              this._positionHandler(null, ((this._x << 16) | this._y));
> > > 
> > > Do we really want to keep bumping over workspaces if the user never moves the
> > > mouse again? I'm not sure which way is right, but if we aren't sure we want the
> > > behavior, cleaner not to have the call to _positionHandler
> > 
> > Hmm ... lets say you are on workspace 3 and want to go to 1 you'd want to "bump
> > over the workspaces" or you'll get annoyed.
> 
> Maybe it would be OK to have to move the mouse a bit have to bump over? Was the
> call to positionHandler something you added because it felt bad without it?

I removed it now and it does not seem to be that bad, so I guess we can leave it out and see if someone complains.

> > > @@ +139,3 @@
> > > +                actor.hide();
> > > +                this._workspaceActor = this._pickActor();
> > > +                this._workspaceActor.opacity = 255;
> > > 
> > > I'm sure this horrible hack is doing something important, but I have no idea
> > > what.
> > 
> > Ugh .. what this is trying to do is: Highlight the adjacent workspace when
> > hovering over it. 
> > How it does it: When we got the workspace shadow under the mouse it is moved
> > "out of sight" (i.e hidden) then we pick again to get the workspace actor to be
> > able to change it's opacity.
> > 
> > Without this (horrible hack) it feels odd that the workspaces only highlight
> > when dragging windows or items from the dash but not here.
> 
> Hmm, you should at least add API to workspacesView to set the hover state and
> not just change opacity directly.

OK

> Not sure if there's annything better to do
> than to hide the shadow and to show it again- 

Note that this "hide and show again" isn't visible at least I cannot spot it with my eyes.

>I may need to ask Florian or
> someone else who understands the workspacesView to look over this part of the
> changes once we have the Xdnd specifics settled.

OK

> > > Since you are setting XdndAware on the stage window, which isn't a clutter
> > > window, you'll need to add a GDK filter function,
> > 
> > I assume you mean s/stage/overlay/ otherwise this does not make sense.
> 
> Yes, I meant the overlay window.

OK

(In reply to comment #25)
> > But if the mouse moves, won't you remove the timeout and re-add it?
> 
> The point here is the pointer is over a workspace; the timeout starts; the user
> moves to a "dead spot" (one that does not trigger any timeouts) it will result
> into the workspaces being changed which isn't expected here. So the handler
> checks after the timeout whether the user is still hovering over the actor
> which triggered the timeout (i.e "are the conditions that triggered the timeout
> still met").

seems like if you remove the existing timeout any time the pointer moves to or within an area that *does* trigger an action, you should definitely remove the timeout when the pointer moves to a "dead spot" as well.

(In reply to comment #20)
> Review of attachment 165607 [details] [review]:
> 
> OK, so I don't buy the "composite overlay window as a second root window" thing
> - just add the COW itself to the window cache and make sure that the
> corresponding gnome-shell code is setting XdndAware on the COW not on the stage
> window.

Well there is a small problem with that ... it simply does not work.

I have tried to figure out why but I failed gtk does find the window when the cursor is over it but for some reason never sends a xdnd event to it. 

I have placed lots of debug prints in gdkdnd-x11.c and it seems that xdnd_check_dest() does seem to return None for the cow and therefore gtk ignores it.

I verified with xprop that the XdndAware ATOM is set for the cow though.

So I am kind of out of idea on how to make this work or why it does not work.

Created attachment 169989 [details] [review]
[GdkWindowCache] Don't ignore the CompositeOverlayWindow

Add the composite overlay window to the cache, as this can be a reasonable Xdnd proxy as well.

This is only done when the screen is composited in order to avoid mapping
the COW. We assume that the CM is using the COW (which is true for pretty
much any CM currently in use).

-----

For the record the above problem has been solved after a IRC discussion with Owen. It turned out that the CompositeOverlayWindow doesn't has an "owner" like tthe root window and thus has to be used as XdndProxy.

Created attachment 170013 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

-----------------------------

Changes:
*) Use the COW as proxy for Xdnd events (requires the new gtk patch now)
*) Merge the xevent filter with the one in gnome_shell_plugin.c
*) Decode the position in C and pass the values to JS
*) Remove the XDND_* macros
*) Avoid continually adding new ripples
*) Fix hot corner exit
*) Remove timeouts in 'dead spots' to avoid code duplication
*) Cleanups
*) Add some comments

(In reply to comment #21)

> The use of global.get_currnet_time() points out that you need to propagate the
> timestamp from the Xdnd events into global.get_current_time()

ClientMessage events don't have any timestamps. Should I fake one?

> (In reply to comment #21)
> 
> > The use of global.get_currnet_time() points out that you need to propagate the
> > timestamp from the Xdnd events into global.get_current_time()
> 
> ClientMessage events don't have any timestamps. Should I fake one?

OK, this seems to be needed to avoid focus prevention weirdness, but I am not sure what is the sane way to do it.

Review of attachment 169989 [details] [review]:

Please commit both to GTK+ master and the gtk-2-22 branch (the development branch leading to GTK+-2.22)

Comment on attachment 169989 [details] [review]
[GdkWindowCache] Don't ignore the CompositeOverlayWindow

Pushed to gtk+ master and to the gtk-2-22 branch.

Created attachment 170179 [details] [review]
Implement cross overview drag & drop

*) Use the timestamp from the XdndPosition event when activating windows.

Review of attachment 170013 [details] [review]:

Already started a review of this version, so most comments here, then I'll look at the timestamp changes in the next version.

::: js/ui/Makefile.am
@@ +38,3 @@
 	workspacesView.js	\
 	workspaceSwitcherPopup.js \
+	workspace.js        \

Please preserve alignment of \ (workspaceSwitchPopup.js isn't aligned because that would have involved realigning everything)

::: js/ui/overview.js
@@ +556,3 @@
+    showTemporary: function () {
+        this._temporarilyShown = true;
+        this.show();

this should read more like:

 showTemporary: function() { 
      let wasShown = this._temporarilyShown || this._shown;
      this._temporarilyShown = true;
      if (!wasShown)
           this._doShow();
 }

 show: function() {
      let wasShown = this._temporarilyShown || this._shown;
      this._shown = true;
      if (!wasShown)
           this._doShow();
 }

And so forth

::: js/ui/xdndHandler.js
@@ +26,3 @@
+        this._workspaceActor = null;
+        this._firstVisit = false;
+        this._exitPending = false;

I don't think this is the best way to handle this. The logic, I think should be that the hot corner is disabled when overview.animationInProgress is true. Then if you only start a ripple animation when hiding or showing the overview, you'll avoid the constant-stream-of-ripples thing.

@@ +46,3 @@
+
+    _exit: function() {
+        if (Main.overview.visible) {

* Checking .visible here goes against the show/showTemporary split - I think I'd keep track locally in xdndHandler as to whether we've entered the overview and not exited it.
* _exit is too generic a name. _exitOverview() perhaps.
* - If the purpose of the function is to exit the overview, then it's cleaner to write it as:

   if (!this._inOverview())
       return;

   then to put the whole block in the if(). (If there was conceptually an "else" to the if, then I'd leave it as an if() {..} but since it's a "nothing to do" short-circuit, it's better as a return)

@@ +60,3 @@
+    },
+
+    _switchToLeftWorkspace: function() {

You should clear this._switchTimeoutId in these functions?

@@ +99,3 @@
+    _hotCornerExit: function () {
+        let primary = global.get_primary_monitor();
+        if (Main.overview.visible && this._x == primary.x && this._y == primary.y && !this._exitPending) {

There's duplication of these checks and the check sin the calling code. With the duplication this is somewhere between _exitOverviewOnHotCornerActivation() and _checkForHotCornerActivationExit() - you need to pick one or the other and not be inbetween.

@@ +121,3 @@
+        let primary = global.get_primary_monitor();
+
+        let actor = this._pickActor();

'actor' isn't very good name when we have other actors about like this._workspaceActor - something like 'pointerActor' would be better.

@@ +127,3 @@
+        if (!Main.overview.visible) {
+            this._lastActiveWorkspace = activeWorkspace;
+        }

No braces for one-line if

@@ +142,3 @@
+            if (this._cursorWindowClone) {
+                this._cursorWindowClone.x = Math.floor(this._x - this._cursorWindowClone.width / 2);
+                this._cursorWindowClone.y = Math.floor(this._y - this._cursorWindowClone.height / 2);

I couldn't reproduce a problem, but I don't see how this works out and gets the window positioned exactly correctly. The cursor clone should follow the position of the window that you are cloning, not the cursor.

@@ +147,3 @@
+            if (this._workspaceActor && this._workspaceActor.index == activeWorkspaceIndex) {
+                this._workspaceActor.opacity = 200;
+                this._workspaceActor = null;

This is definitely not right, and we absolutely needed to do something like, it needs to be commented at length. But as I said, Florian or Maxim should look at this.

@@ +152,3 @@
+            if (activeWorkspaceIndex > 0 &&
+                ((this._x <= primary.x && this._y > Panel.PANEL_HEIGHT) ||
+                actor.name == 'left-workspaces-shadow')) {

My memory of the behavior is that normally bumping completely into the left or right the monitor is an immediate switch not a hover-and-wait switch. Shouldn't we do the same here?

@@ +156,3 @@
+                this._removeTimeout();
+                this._switchTimeoutId = Mainloop.timeout_add(WORKSPACE_SWITCH_TIMEOUT,
+                                                                Lang.bind(this, this._switchToLeftWorkspace));

Alignment problems

@@ +163,3 @@
+                actor.show();
+            }
+            else if (activeWorkspaceIndex < global.screen.n_workspaces - 1 && (this._x >= (primary.x + primary.width)

I think you mean (primary.x + primary.width - 1) - on a 1024x768 screen the max reported mouse position will be 1023

@@ +186,3 @@
+
+            /* Allow leaving the overview using the hot corner*/
+            if (this._x == primary.x && this._y == primary.y && !this._firstVisit) {

I assume firstVisit here is about not immediately triggering out again as soon as you get into the overview. Name is poorly descriptive - I think I'd call it 'pointerOnHotCorner'

@@ +200,3 @@
+                Main.panel.rippleAnimation();
+            Main.overview.showTemporary();
+            Main.overview.beginItemDrag(null);

Need a comment about why you are calling this

::: src/gnome-shell-plugin.c
@@ +516,3 @@
 
+  /*
+   *  Handles XDND Position and Leave events.

I think you should be a bit more descriptive here about what the purpose of the code is - because otherwise someone reading the code is going to say "hey, where's the actual handling of drag and drop"?

@@ +534,3 @@
+        xevent.xclient.data.l[0] = xev->xclient.window;
+        xevent.xclient.data.l[1] |= 0;
+        xevent.xclient.data.l[1] |= 0x2;

|= 0 doesn't make much sense. I'd probably do this as

      /* flags: bit 0: will we accept the drop? bit 1: do we want more position messages */
      xevent.xclient.data.l[1] = 2;

@@ +537,3 @@
+        xevent.xclient.data.l[4] = gdk_x11_get_xatom_by_name ("XdndLeave");
+
+        XSendEvent (xdisplay, src, 0, 0, &xevent);

False not 0 for the first 0

@@ +539,3 @@
+        XSendEvent (xdisplay, src, 0, 0, &xevent);
+
+        g_signal_emit_by_name (G_OBJECT (shell_plugin->global), "xdnd-position-changed", (int)(xev->xclient.data.l[2] >> 16), (int)(xev->xclient.data.l[2] & 0xFFFF));

line break after "xdnd-position-changed"

::: src/shell-global.c
@@ +235,3 @@
   gobject_class->set_property = shell_global_set_property;
 
+  shell_global_signals[XDND_POSITION_CHANGED] =

As a helpful aid to whoever is trying to understand this code, I'd add:

 /* Emitted from gnome-shell-plugin.c during event handling */

(It would be a bit cleaner to add _shell_global_xdnd_leave _shell_global_xdnd_position_changed to shell-global-private.h as wrappers to emit the signals, but I think it's OK without that.)

@@ +1321,3 @@
+ * Enables tracking of Xdnd events
+ *
+ */

Blank line

@@ +1322,3 @@
+ *
+ */
+void shell_global_init_xdnd (ShellGlobal *global) {

{ on the wrong line

@@ +1335,3 @@
+
+  XChangeProperty (xdisplay, stage_win, gdk_x11_get_xatom_by_name ("XdndAware"), XA_ATOM,
+                    32, PropModeReplace, (const unsigned char *)&xdnd_version, 1);

Lines aren't properly lined up

@@ +1341,3 @@
+
+  XChangeProperty (xdisplay, stage_win, gdk_x11_get_xatom_by_name ("XdndProxy"), XA_WINDOW,
+                    32, PropModeReplace, (const unsigned char *)&stage_win, 1);

Probably good to comment this with something like:

 /* XdndProxy is additionally set on the proxy window as verification that the
    XdndProxy property on the target window isn't a left-over */

since it otherwise looks a bit puzzling.

Review of attachment 170179 [details] [review]:

Just commentary on timestamps

::: src/gnome-shell-plugin.c
@@ +540,3 @@
+        
+        /* Store the timestamp of the xdnd position event */
+        g_object_set (shell_plugin->global, "xdnd-timestamp", xev->xclient.data.l[3], NULL);

I think at this point you are better off with:

 if (_shell_global_check_xdnd_event (xev))
    return true;

::: src/shell-global.c
@@ +87,3 @@
   PROP_IMAGEDIR,
   PROP_USERDATADIR,
+  PROP_XDND_TIMESTAMP,

Better way to do this:

 global->xdnd_timestamp = <timestamp>;
 <emit signal>
 global_>xdnd_timestamp = 0;

In shell_global_get_current_time 

 if (global->xdnd_timestamp != 0)
   return global->xdnd_timestamp;

In xdndHandler.c, when adding a timeout that will cause a switch:

 this._switchTimestamp = global.get_current_time();

(In reply to comment #34)
> Review of attachment 170013 [details] [review]:
> 
> Already started a review of this version, so most comments here, then I'll look
> at the timestamp changes in the next version.
> 
> ::: js/ui/Makefile.am
> @@ +38,3 @@
>      workspacesView.js    \
>      workspaceSwitcherPopup.js \
> +    workspace.js        \
> 
> Please preserve alignment of \ (workspaceSwitchPopup.js isn't aligned because
> that would have involved realigning everything)

OK

> ::: js/ui/overview.js
> @@ +556,3 @@
> +    showTemporary: function () {
> +        this._temporarilyShown = true;
> +        this.show();
> 
> this should read more like:
> 
>  showTemporary: function() { 
>       let wasShown = this._temporarilyShown || this._shown;
>       this._temporarilyShown = true;
>       if (!wasShown)
>            this._doShow();
>  }
> 
>  show: function() {
>       let wasShown = this._temporarilyShown || this._shown;
>       this._shown = true;
>       if (!wasShown)
>            this._doShow();
>  }
> 
> And so forth

OK

> ::: js/ui/xdndHandler.js
> @@ +26,3 @@
> +        this._workspaceActor = null;
> +        this._firstVisit = false;
> +        this._exitPending = false;
> 
> I don't think this is the best way to handle this. The logic, I think should be
> that the hot corner is disabled when overview.animationInProgress is true. Then
> if you only start a ripple animation when hiding or showing the overview,
> you'll avoid the constant-stream-of-ripples thing.

OK

> @@ +46,3 @@
> +
> +    _exit: function() {
> +        if (Main.overview.visible) {
> 
> * Checking .visible here goes against the show/showTemporary split - I think
> I'd keep track locally in xdndHandler as to whether we've entered the overview
> and not exited it.
> * _exit is too generic a name. _exitOverview() perhaps.
> * - If the purpose of the function is to exit the overview, then it's cleaner
> to write it as:
> 
>    if (!this._inOverview())
>        return;
> 
>    then to put the whole block in the if(). (If there was conceptually an
> "else" to the if, then I'd leave it as an if() {..} but since it's a "nothing
> to do" short-circuit, it's better as a return)

OK

> @@ +60,3 @@
> +    },
> +
> +    _switchToLeftWorkspace: function() {
> 
> You should clear this._switchTimeoutId in these functions?

Hmm ... I don't see a reason why this should be needed. We return false when done to avoid being called again. And when moving to something else the timeout is removed when moving over a 'dead spot'.

> @@ +99,3 @@
> +    _hotCornerExit: function () {
> +        let primary = global.get_primary_monitor();
> +        if (Main.overview.visible && this._x == primary.x && this._y ==
> primary.y && !this._exitPending) {
> 
> There's duplication of these checks and the check sin the calling code. With
> the duplication this is somewhere between _exitOverviewOnHotCornerActivation()
> and _checkForHotCornerActivationExit() - you need to pick one or the other and
> not be inbetween.

The check shouldn't be needed, will just remove it.

> @@ +121,3 @@
> +        let primary = global.get_primary_monitor();
> +
> +        let actor = this._pickActor();
> 
> 'actor' isn't very good name when we have other actors about like
> this._workspaceActor - something like 'pointerActor' would be better.

OK

> @@ +127,3 @@
> +        if (!Main.overview.visible) {
> +            this._lastActiveWorkspace = activeWorkspace;
> +        }
> 
> No braces for one-line if

OK

> @@ +142,3 @@
> +            if (this._cursorWindowClone) {
> +                this._cursorWindowClone.x = Math.floor(this._x -
> this._cursorWindowClone.width / 2);
> +                this._cursorWindowClone.y = Math.floor(this._y -
> this._cursorWindowClone.height / 2);
> 
> I couldn't reproduce a problem, but I don't see how this works out and gets the
> window positioned exactly correctly. The cursor clone should follow the
> position of the window that you are cloning, not the cursor.

It should be pretty much the same as the other window follows the cursor as well, I can't recall a specific reason why I did it this way but I can try using the window position instead.

> @@ +147,3 @@
> +            if (this._workspaceActor && this._workspaceActor.index ==
> activeWorkspaceIndex) {
> +                this._workspaceActor.opacity = 200;
> +                this._workspaceActor = null;
> 
> This is definitely not right, and we absolutely needed to do something like, it
> needs to be commented at length. But as I said, Florian or Maxim should look at
> this.

Yeah as you said "look over this part of the changes once we have the Xdnd specifics settled."  so I didn't bother to make any changes here yet (other than fixing one bug).

> @@ +152,3 @@
> +            if (activeWorkspaceIndex > 0 &&
> +                ((this._x <= primary.x && this._y > Panel.PANEL_HEIGHT) ||
> +                actor.name == 'left-workspaces-shadow')) {
> 
> My memory of the behavior is that normally bumping completely into the left or
> right the monitor is an immediate switch not a hover-and-wait switch. Shouldn't
> we do the same here?

Good point, otherwise it feels inconsistent.

> @@ +156,3 @@
> +                this._removeTimeout();
> +                this._switchTimeoutId =
> Mainloop.timeout_add(WORKSPACE_SWITCH_TIMEOUT,
> +                                                               
> Lang.bind(this, this._switchToLeftWorkspace));
> 
> Alignment problems

OK

> @@ +163,3 @@
> +                actor.show();
> +            }
> +            else if (activeWorkspaceIndex < global.screen.n_workspaces - 1 &&
> (this._x >= (primary.x + primary.width)
> 
> I think you mean (primary.x + primary.width - 1) - on a 1024x768 screen the max
> reported mouse position will be 1023

OK

> @@ +186,3 @@
> +
> +            /* Allow leaving the overview using the hot corner*/
> +            if (this._x == primary.x && this._y == primary.y &&
> !this._firstVisit) {
> 
> I assume firstVisit here is about not immediately triggering out again as soon
> as you get into the overview.

Yeah

> Name is poorly descriptive - I think I'd call it
> 'pointerOnHotCorner'

OK
 
> @@ +200,3 @@
> +                Main.panel.rippleAnimation();
> +            Main.overview.showTemporary();
> +            Main.overview.beginItemDrag(null);
> 
> Need a comment about why you are calling this

OK

> ::: src/gnome-shell-plugin.c
> @@ +516,3 @@
> 
> +  /*
> +   *  Handles XDND Position and Leave events.
> 
> I think you should be a bit more descriptive here about what the purpose of the
> code is - because otherwise someone reading the code is going to say "hey,
> where's the actual handling of drag and drop"?

OK, makes sense.

> @@ +534,3 @@
> +        xevent.xclient.data.l[0] = xev->xclient.window;
> +        xevent.xclient.data.l[1] |= 0;
> +        xevent.xclient.data.l[1] |= 0x2;
> 
> |= 0 doesn't make much sense. I'd probably do this as
> 
>       /* flags: bit 0: will we accept the drop? bit 1: do we want more position
> messages */
>       xevent.xclient.data.l[1] = 2;

OK

> @@ +537,3 @@
> +        xevent.xclient.data.l[4] = gdk_x11_get_xatom_by_name ("XdndLeave");
> +
> +        XSendEvent (xdisplay, src, 0, 0, &xevent);
> 
> False not 0 for the first 0

OK

> @@ +539,3 @@
> +        XSendEvent (xdisplay, src, 0, 0, &xevent);
> +
> +        g_signal_emit_by_name (G_OBJECT (shell_plugin->global),
> "xdnd-position-changed", (int)(xev->xclient.data.l[2] >> 16),
> (int)(xev->xclient.data.l[2] & 0xFFFF));
> 
> line break after "xdnd-position-changed"

OK

> ::: src/shell-global.c
> @@ +235,3 @@
>    gobject_class->set_property = shell_global_set_property;
> 
> +  shell_global_signals[XDND_POSITION_CHANGED] =
> 
> As a helpful aid to whoever is trying to understand this code, I'd add:
> 
>  /* Emitted from gnome-shell-plugin.c during event handling */

OK

> (It would be a bit cleaner to add _shell_global_xdnd_leave
> _shell_global_xdnd_position_changed to shell-global-private.h as wrappers to
> emit the signals, but I think it's OK without that.)

Well as this signals are never emitted from within shell-global I don't think adding them would be that useful.

> @@ +1321,3 @@
> + * Enables tracking of Xdnd events
> + *
> + */
> 
> Blank line

OK

> @@ +1322,3 @@
> + *
> + */
> +void shell_global_init_xdnd (ShellGlobal *global) {
> 
> { on the wrong line

OK

> @@ +1335,3 @@
> +
> +  XChangeProperty (xdisplay, stage_win, gdk_x11_get_xatom_by_name
> ("XdndAware"), XA_ATOM,
> +                    32, PropModeReplace, (const unsigned char *)&xdnd_version,
> 1);
> 
> Lines aren't properly lined up

OK

> @@ +1341,3 @@
> +
> +  XChangeProperty (xdisplay, stage_win, gdk_x11_get_xatom_by_name
> ("XdndProxy"), XA_WINDOW,
> +                    32, PropModeReplace, (const unsigned char *)&stage_win,
> 1);
> 
> Probably good to comment this with something like:
> 
>  /* XdndProxy is additionally set on the proxy window as verification that the
>     XdndProxy property on the target window isn't a left-over */
> 
> since it otherwise looks a bit puzzling.

Yeah makes sense as even the wording in the spec confused me at first.

(In reply to comment #35)
> Review of attachment 170179 [details] [review]:
> 
> Just commentary on timestamps
> 
> ::: src/gnome-shell-plugin.c
> @@ +540,3 @@
> +        
> +        /* Store the timestamp of the xdnd position event */
> +        g_object_set (shell_plugin->global, "xdnd-timestamp",
> xev->xclient.data.l[3], NULL);
> 
> I think at this point you are better off with:
> 
>  if (_shell_global_check_xdnd_event (xev))
>     return true;

As this is in gnome-shell-plugin I am not sure a _shell_global_* name is appropriate. (But moving it to a separate function makes sense nevertheless) 

> ::: src/shell-global.c
> @@ +87,3 @@
>    PROP_IMAGEDIR,
>    PROP_USERDATADIR,
> +  PROP_XDND_TIMESTAMP,
> 
> Better way to do this:
> 
>  global->xdnd_timestamp = <timestamp>;
>  <emit signal>
>  global_>xdnd_timestamp = 0;

Hmm ... the code that actually do this is outside of shell-global, so accessing global like that might be a bit ugly.

Or did you mean moving the whole thing into shell-global and calling it from gnome-shell-plugin in the filter?

> In shell_global_get_current_time 
> 
>  if (global->xdnd_timestamp != 0)
>    return global->xdnd_timestamp;
>
> In xdndHandler.c, when adding a timeout that will cause a switch:
> 
>  this._switchTimestamp = global.get_current_time();

OK

(In reply to comment #37)
> (In reply to comment #35)
> > Review of attachment 170179 [details] [review] [details]:
> > 
> > Just commentary on timestamps
> > 
> > ::: src/gnome-shell-plugin.c
> > @@ +540,3 @@
> > +        
> > +        /* Store the timestamp of the xdnd position event */
> > +        g_object_set (shell_plugin->global, "xdnd-timestamp",
> > xev->xclient.data.l[3], NULL);
> > 
> > I think at this point you are better off with:
> > 
> >  if (_shell_global_check_xdnd_event (xev))
> >     return true;
> 
> As this is in gnome-shell-plugin I am not sure a _shell_global_* name is
> appropriate. (But moving it to a separate function makes sense nevertheless) 
> 
> > ::: src/shell-global.c
> > @@ +87,3 @@
> >    PROP_IMAGEDIR,
> >    PROP_USERDATADIR,
> > +  PROP_XDND_TIMESTAMP,
> > 
> > Better way to do this:
> > 
> >  global->xdnd_timestamp = <timestamp>;
> >  <emit signal>
> >  global_>xdnd_timestamp = 0;
> 
> Hmm ... the code that actually do this is outside of shell-global, so accessing
> global like that might be a bit ugly.
> 
> Or did you mean moving the whole thing into shell-global and calling it from
> gnome-shell-plugin in the filter?

Yeah, I meant move everything to shell-global.c, add the prototype for _shell_global_check_xdnd_event() into shell-global-private.h (returns gboolean, TRUE if it handled the event, otherwise FALSE), and just call that single function from the plugin's filter function.

(In reply to comment #36)
> > @@ +60,3 @@
> > +    },
> > +
> > +    _switchToLeftWorkspace: function() {
> > 
> > You should clear this._switchTimeoutId in these functions?
> 
> Hmm ... I don't see a reason why this should be needed. We return false when
> done to avoid being called again. And when moving to something else the timeout
> is removed when moving over a 'dead spot'.

If you don't set it to 0 here, won't you try to remove a non-existent timeout the next time the cursor moves? - we should avoid calling Mainloop.source_remove() on the ID of a source that has already been removed.

(In reply to comment #38)
>
> > Or did you mean moving the whole thing into shell-global and calling it from
> > gnome-shell-plugin in the filter?
> 
> Yeah, I meant move everything to shell-global.c, add the prototype for
> _shell_global_check_xdnd_event() into shell-global-private.h (returns gboolean,
> TRUE if it handled the event, otherwise FALSE), and just call that single
> function from the plugin's filter function.

OK

(In reply to comment #39)
> (In reply to comment #36)
> > > @@ +60,3 @@
> > > +    },
> > > +
> > > +    _switchToLeftWorkspace: function() {
> > > 
> > > You should clear this._switchTimeoutId in these functions?
> > 
> > Hmm ... I don't see a reason why this should be needed. We return false when
> > done to avoid being called again. And when moving to something else the timeout
> > is removed when moving over a 'dead spot'.
> 
> If you don't set it to 0 here, won't you try to remove a non-existent timeout
> the next time the cursor moves? - we should avoid calling
> Mainloop.source_remove() on the ID of a source that has already been removed.

Ah OK that makes sense.

Created attachment 170356 [details] [review]
Implement cross overview drag & drop

*) Implement fixes and cleanups from the review

Created attachment 170397 [details] [review]
Implement cross overview drag & drop

*) Rebase to HEAD
*) Minor fixes / cleanups

Created attachment 170435 [details] [review]
Implement cross overview drag & drop

*) Rebase to master and fix small style issues found while doing so

Created attachment 170493 [details] [review]
Add a facility to show the stage without grabbing

Hmm, the showTemporarilyy/hideTemporarily thing is sort of complex to get right
and reveals some holes in the current logic.

Here's my attempt at it (it's showTemporarily rather than showTemporary - I was
a bit inconsistent in my suggestions - I think 'temporarily' probably works
a bit better grammatically.)

I haven't actually tested the temporary parts of this, just the non-temporary
parts, so there might be some stupid bugs there. And definitely needs a second
pair of eyes looking at the logic.

Review of attachment 170435 [details] [review]:

The XDND stuff looks good to me now; going to reassign the bug to Florian, and you can work with him to figure out how to make the workspace dragging work right without poking into the internals as much.

::: js/ui/xdndHandler.js
@@ +213,3 @@
+                Main.panel.rippleAnimation();
+            Main.overview.showTemporary();
+            // Switch to the 'drag mode'

This is a little non-descriptive :-). // Switch the workspaces view to the zoomed out mode for dragging stuff between workspaces
or something like that.

Review of attachment 170493 [details] [review]:

This looks fine and works here (tested both Temporarily and "normal" versions).

::: js/ui/overview.js
@@ +530,3 @@
+                this._modal = false;
+            }
+            global.stage_input_mode = Shell.StageInputMode.FULLSCREEN;

You have to import Shell for this (it isn't anymore in the current version).

Created attachment 174428 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

-------------

Changes:
*) Now depends on Owen's patch "Add a facility to show the stage without grabbing"
*) Now tracks XdndEnter events too
*) Hooks into existing DND infrastructure (uses handleDragOver and dragMonitors)*) Overview DND stuff is no longer inside XdndHandler but spread all over
*) Remove code duplication (some events are handled by generic dragMonitor, like workspace switching in the overview)
*) NO support for Xdnd drops (ok, isn't really a change but worth mentioning anyway).

Created attachment 176270 [details] [review]
Add a facility to show the stage without grabbing

When the user is doing a drag-and-drop, we want to temporarily show the
stage to allow them to drag to a different window. But we're not "really"
in the overview, and getting a grab would conflict with the X client doing
the drag and drop.

So add a showTemporarily()/hideTemporarily() pair of methods that show
the overview without grabbing.

This adds a lot more possibilities for asynchronous race conditions, so
rework the code to be more robust against multiple calls to show*()
and hide*(). The interpretation is now that all calls to show*() and
hide*() affect the state, but if we have conflicting calls to show and
hide we wait until the current animation is finished before correcting
to the right visual state.

---

Rebased

Created attachment 176271 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

---
Rebased

Note there is a gtk bug that prevents this from working as intended:

Basically it will work once for each (gtk) app until restarted.

Something is broken in GdkWindowCache, I still haven't figured out what.
Always recreating the cache in drag_context_find_window_cache() (gdkdnd-x11.c) fixes it but this obviously isn't a proper fix I am still debugging this.

Created attachment 176373 [details] [review]
gdkdnd-x11: Don't remove shape events from the queue

It seems that we can end up with multiple filters and/or
drag contexts for some reason which causes dnd to not work
as the first filter eats all events and causes the other ones
to starve.

----

NOTE: I am still not sure why we are ending up with two contexts/filters.

Review of attachment 176271 [details] [review]:

Quite impressive that it's working :-)

Still, I think the details can use a bit of love, see comments below.

::: js/ui/overview.js
@@ +206,3 @@
+        this._windowSwitchTimeoutId = 0;
+        this._windowSwitchTimestamp = 0;
+        this._lastActvieWorkspaceIndex = global.screen.get_active_workspace_index();

s/Actvie/Active/

Could be initialized to -1?

@@ +213,3 @@
+    _onDragBegin : function() {
+        DND.addDragMonitor(this._dragMonitor);
+        // Remember the workspace we stared from

*started

@@ +218,3 @@
+
+    _onDragEnd : function(time) {
+        // In case the drag was cancled while in the overview

*canceled

@@ +230,3 @@
+
+    _onDragMotion : function(dragEvent) {
+        if (dragEvent.targetActor && dragEvent.targetActor._delegate

I would suggest adding another line break to stress that all conditions have equal priority. Also the operator should be placed at the end of the previous line (bug 612941).

@@ +235,3 @@
+                Mainloop.source_remove(this._windowSwitchTimeoutId);
+                this._windowSwitchTimeoutId = 0;
+                this._windowSwitchTimestamp = global.get_current_time();

I don't think there's much of a point resetting this._windowSwitchTimeoutId right before setting it to a new value.

The timestamp needs to be set unconditionally, right?

@@ +238,3 @@
+            }
+            this._windowSwitchTimeoutId = Mainloop.timeout_add(1250, Lang.bind(this, function() {
+                                                                                        Main.activateWindow(dragEvent.targetActor._delegate.metaWindow,

Magic values should use a constant.

@@ +244,3 @@
+        }
+        else if (this._windowSwitchTimeoutId != 0)
+            Mainloop.source_remove(this._windowSwitchTimeoutId);

Uhm - here you _do_ need to reset _windowSwitchTimeoutId?

Probably cleaner to do:

if (this._windowSwitchTimeoutId != 0) {
    Mainloop.source_remove(this._windowSwitchTimeoutId);
    this._windowSwitchTimeoutId = 0;
    this._windowSwitchTimestamp = 0; // not really necessary
}

if (dragEvent.targetActor &&
    dragEvent.targetActor._delegate &&
    dragEvent.targetActor._delegate.metaWindow) {
    this._windowSwitchTimestamp = global.get_current_time();
    this._windowSwitchTimeoutId = Mainloop.timeout_add(...);
}

::: js/ui/panel.js
@@ +751,3 @@
+        this.button._delegate = this.button;
+        this.button.handleDragOver = Lang.bind(this, function(source, actor, x, y, time) {
+                                                         if (source == Main.xdndHandler) {

This is pretty nasty in a standard terminal, so I'd suggest an indentation of:

   this.button.handleDragOver = Lang.bind(this,
       function(...) {
       });

@@ +796,3 @@
 
+        this._hotCorner._delegate = this._hotCorner;
+        this._hotCorner.handleDragOver = Lang.bind(this, function(source, actor, x, y, time) {

This works extremely unreliable for me - I'm thrown out of the overview right away if I leave the hot corner too fast or too slowly.

It's also inconsistent to allow toggling with the hot corner, but not the button.

@@ +804,3 @@
+                                                                        Main.overview.beginItemDrag(actor);
+                                                                    }
+                                                                    else {

No line break before else.

::: js/ui/workspacesView.js
@@ +660,3 @@
         // reactive monitor edges
         let leftEdge = primary.x;
+        let switchLeft = (dragEvent.x <= leftEdge && leftWorkspace) && !Main.overview.animationInProgress;

RTL locales have the activity button / hot corner on the right ;-)

Actually, I don't think _anything_ in the handler makes sense during the overview animation, so I'd do a

if (Main.overview.animationInProgress)
    return DND.DragMotionResult.CONTINUE;

at the beginning of the function.

::: js/ui/xdndHandler.js
@@ +32,3 @@
+        global.connect('xdnd-enter', Lang.bind(this, this._onEnter));
+        global.connect('xdnd-position-changed', Lang.bind(this, this._onPositionChange));
+        global.connect('xdnd-leave', Lang.bind(this, this._onLeave));

Hmmm, 'enter' and 'leave' sound a bit strange to me - might be nicer to be consistent with our JS code rather than X and use _dragBegin/_dragEnd.

@@ +49,3 @@
+        let actor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y);
+        if (this._cursorWindowClone)
+            this._cursorWindowClone.show();

Can't you set up the cursor clone with Shell.util_set_hidden_from_pick()? You could just do

let pickedActor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y);

in _onPositionChange() then ...

@@ +53,3 @@
+    },
+
+    _onPositionChange: function(obj, x, y) {

Function name should be '_onPositionChanged'.

@@ +65,3 @@
+            }
+
+            // We remove the clone once the real window shows up again

*show

@@ +66,3 @@
+
+            // We remove the clone once the real window shows up again
+            global.window_group.connect("notify::visible", Lang.bind(this, function () {

Again, the indentation is fiddly in a terminal (sorry for being anal).

I wonder if it would be clearer to:

 - connect to 'notify::visible' in _onEnter

 - create or destroy the clone in the handler

 - disconnect the signal in _onLeave

@@ +78,3 @@
+        if (this._cursorWindowClone) {
+            this._cursorWindowClone.x = this._cursorWindowClone.source.x;
+            this._cursorWindowClone.y = this._cursorWindowClone.source.y;

Could this be done automatically with a ClutterBindConstraint?

@@ +92,3 @@
+            let motionFunc = DND.dragMonitors[i].dragMotion;
+            if (motionFunc) {
+                motionFunc(dragEvent);

This is not consistent with the behavior in dnd.js - I guess you can't update the cursor, but I don't see a reason to ignore the return value (e.g. return if the function returns something different than DragMotionResult.CONTINUE).

If I'm overlooking something and you must ignore the return value, the block doesn't need braces.

@@ +102,3 @@
+                                                         x,
+                                                         y,
+                                                         global.get_current_time());

Same comment about not checking the return value.

(In reply to comment #52)
> Review of attachment 176271 [details] [review]:
> 
> Quite impressive that it's working :-)
> 
> Still, I think the details can use a bit of love, see comments below.
> 
> ::: js/ui/overview.js
> @@ +206,3 @@
> +        this._windowSwitchTimeoutId = 0;
> +        this._windowSwitchTimestamp = 0;
> +        this._lastActvieWorkspaceIndex =
> global.screen.get_active_workspace_index();
> 
> s/Actvie/Active/

OK.

> Could be initialized to -1?

Yes.
 
> @@ +213,3 @@
> +    _onDragBegin : function() {
> +        DND.addDragMonitor(this._dragMonitor);
> +        // Remember the workspace we stared from
> 
> *started

OK.

> @@ +218,3 @@
> +
> +    _onDragEnd : function(time) {
> +        // In case the drag was cancled while in the overview
> 
> *canceled

OK.

> @@ +230,3 @@
> +
> +    _onDragMotion : function(dragEvent) {
> +        if (dragEvent.targetActor && dragEvent.targetActor._delegate
> 
> I would suggest adding another line break to stress that all conditions have
> equal priority. Also the operator should be placed at the end of the previous
> line (bug 612941).

OK.

> @@ +235,3 @@
> +                Mainloop.source_remove(this._windowSwitchTimeoutId);
> +                this._windowSwitchTimeoutId = 0;
> +                this._windowSwitchTimestamp = global.get_current_time();
> 
> I don't think there's much of a point resetting this._windowSwitchTimeoutId
> right before setting it to a new value.

Yeah that line can be removed.

> The timestamp needs to be set unconditionally, right?

Indeed, good catch.

> @@ +238,3 @@
> +            }
> +            this._windowSwitchTimeoutId = Mainloop.timeout_add(1250,
> Lang.bind(this, function() {
> +                                                                              
>          Main.activateWindow(dragEvent.targetActor._delegate.metaWindow,
> 
> Magic values should use a constant.

OK, I though I already did that but apparently not.

> @@ +244,3 @@
> +        }
> +        else if (this._windowSwitchTimeoutId != 0)
> +            Mainloop.source_remove(this._windowSwitchTimeoutId);
> 
> Uhm - here you _do_ need to reset _windowSwitchTimeoutId?
> 
> Probably cleaner to do:
> 
> if (this._windowSwitchTimeoutId != 0) {
>     Mainloop.source_remove(this._windowSwitchTimeoutId);
>     this._windowSwitchTimeoutId = 0;
>     this._windowSwitchTimestamp = 0; // not really necessary
> }
> 
> if (dragEvent.targetActor &&
>     dragEvent.targetActor._delegate &&
>     dragEvent.targetActor._delegate.metaWindow) {
>     this._windowSwitchTimestamp = global.get_current_time();
>     this._windowSwitchTimeoutId = Mainloop.timeout_add(...);
> }

Yeah that indeed looks cleaner.

> ::: js/ui/panel.js
> @@ +751,3 @@
> +        this.button._delegate = this.button;
> +        this.button.handleDragOver = Lang.bind(this, function(source, actor,
> x, y, time) {
> +                                                         if (source ==
> Main.xdndHandler) {
> 
> This is pretty nasty in a standard terminal, so I'd suggest an indentation of:
> 
>    this.button.handleDragOver = Lang.bind(this,
>        function(...) {
>        });

OK

> @@ +796,3 @@
> 
> +        this._hotCorner._delegate = this._hotCorner;
> +        this._hotCorner.handleDragOver = Lang.bind(this, function(source,
> actor, x, y, time) {
> 
> This works extremely unreliable for me - I'm thrown out of the overview right
> away if I leave the hot corner too fast or too slowly.
> 
> It's also inconsistent to allow toggling with the hot corner, but not the
> button.

So you are suggesting to allow leaving using the button too? Or disallow going back this way at all (it doesn't really add that much value and if it is
confusing we can remove it) ?

> @@ +804,3 @@
> +                                                                       
> Main.overview.beginItemDrag(actor);
> +                                                                    }
> +                                                                    else {
> 
> No line break before else.

OK.

> ::: js/ui/workspacesView.js
> @@ +660,3 @@
>          // reactive monitor edges
>          let leftEdge = primary.x;
> +        let switchLeft = (dragEvent.x <= leftEdge && leftWorkspace) &&
> !Main.overview.animationInProgress;
> 
> RTL locales have the activity button / hot corner on the right ;-)

Oh, true.

> Actually, I don't think _anything_ in the handler makes sense during the
> overview animation, so I'd do a
> 
> if (Main.overview.animationInProgress)
>     return DND.DragMotionResult.CONTINUE;
> 
> at the beginning of the function.

That makes sense.

> ::: js/ui/xdndHandler.js
> @@ +32,3 @@
> +        global.connect('xdnd-enter', Lang.bind(this, this._onEnter));
> +        global.connect('xdnd-position-changed', Lang.bind(this,
> this._onPositionChange));
> +        global.connect('xdnd-leave', Lang.bind(this, this._onLeave));
> 
> Hmmm, 'enter' and 'leave' sound a bit strange to me - might be nicer to be
> consistent with our JS code rather than X and use _dragBegin/_dragEnd.

You mean "xdnd-drag-begin" etc. ?
I'd rather not mix the names as those are the low level operations, note that the ones emitted by xdndHandler.js are actually "drag-begin" and "drag-end".

> @@ +49,3 @@
> +        let actor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y);
> +        if (this._cursorWindowClone)
> +            this._cursorWindowClone.show();
> 
> Can't you set up the cursor clone with Shell.util_set_hidden_from_pick()? You
> could just do
> 
> let pickedActor = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y);

Yeah wanted to use that since it was added but forgot about it.

> in _onPositionChange() then ...
> 
> @@ +53,3 @@
> +    },
> +
> +    _onPositionChange: function(obj, x, y) {
> 
> Function name should be '_onPositionChanged'.

OK.

> @@ +65,3 @@
> +            }
> +
> +            // We remove the clone once the real window shows up again
> 
> *show

OK.

> @@ +66,3 @@
> +
> +            // We remove the clone once the real window shows up again
> +            global.window_group.connect("notify::visible", Lang.bind(this,
> function () {
> 
> Again, the indentation is fiddly in a terminal (sorry for being anal).
> 
> I wonder if it would be clearer to:
> 
>  - connect to 'notify::visible' in _onEnter
> 
>  - create or destroy the clone in the handler
> 
>  - disconnect the signal in _onLeave

OK.

> @@ +78,3 @@
> +        if (this._cursorWindowClone) {
> +            this._cursorWindowClone.x = this._cursorWindowClone.source.x;
> +            this._cursorWindowClone.y = this._cursorWindowClone.source.y;
> 
> Could this be done automatically with a ClutterBindConstraint?

Yes.

> @@ +92,3 @@
> +            let motionFunc = DND.dragMonitors[i].dragMotion;
> +            if (motionFunc) {
> +                motionFunc(dragEvent);
> 
> This is not consistent with the behavior in dnd.js - I guess you can't update
> the cursor, but I don't see a reason to ignore the return value (e.g. return if
> the function returns something different than DragMotionResult.CONTINUE).
> 
> If I'm overlooking something and you must ignore the return value, the block
> doesn't need braces.

Well I am ignoring the return value because we can't do much with it, neither cancel the drag nor set the cursor or anything else that makes sense.

> @@ +102,3 @@
> +                                                         x,
> +                                                         y,
> +                                                        
> global.get_current_time());
> 
> Same comment about not checking the return value.

See comment about why this is being done ;)

(In reply to comment #53)
> So you are suggesting to allow leaving using the button too? Or disallow going
> back this way at all (it doesn't really add that much value and if it is
> confusing we can remove it) ?

Up to you - removing it will probably fix the fiddlyness when triggering the overview, so I'd probably do that ;-)


> You mean "xdnd-drag-begin" etc. ?
> I'd rather not mix the names as those are the low level operations[...]

OK.


> > This is not consistent with the behavior in dnd.js - I guess you can't update
> > the cursor, but I don't see a reason to ignore the return value (e.g. return if
> > the function returns something different than DragMotionResult.CONTINUE).
> > 
> > If I'm overlooking something and you must ignore the return value, the block
> > doesn't need braces.
> 
> Well I am ignoring the return value because we can't do much with it, neither
> cancel the drag nor set the cursor or anything else that makes sense.

dnd.js does not cancel the drag, it stops running further drag monitors / handleDragOver functions *until the next time the motion event is triggered*

I don't see a reason why xdnd can't do that as well.

Created attachment 176655 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

--

Changes:
*) Fixed stuff mentioned in the review
*) Allow drops without moving after activating the window
(done by warping the pointer to the same spot).

Created attachment 176656 [details] [review]
Implement cross overview drag & drop

*) Fix typo / whitespace

Review of attachment 176656 [details] [review]:

Much better, but the handling of the return values of dragMonitor.motionFuncs still is not quite correct. The removal of the "toggle" behavior of the hot corner has improved the reliability when triggering the overview a lot - strangely, only in the case of being "too slow": if I leave the button while the overview transition is still ongoing, I'm kicked out of the overview. No obvious idea what's triggering this - would be cool if you manage to track it down, otherwise I'm OK with handling it in a new bug after landing this.

::: js/ui/panel.js
@@ +755,3 @@
+                    if(!Main.overview.visible && !Main.overview.animationInProgress) {
+                        this.rippleAnimation();
+                        Main.overview.showTemporarily();

Hmmm, I'm not sure about the ripple animation on hovering the button. On the one hand, it is quite similar to the hot corner (e.g. area triggering an even on hover), but on the other hand the animation is pretty much tied to the corner (e.g. it's a bit weird that when placing the pointer on the right edge of the button the top-left corner indicates activity)

Also, I sometimes manage to throw the pointer into the corner quick enough to not trigger the handler (compiling helps) - you probably don't want to remove the hotCorner.handleDragOver handler (the previous comment was about the hiding, not the entire handler).

::: js/ui/xdndHandler.js
@@ +58,3 @@
+    _onWindowGroupVisibilityChanged : function() {
+        if (!global.window_group.visible) {
+            if (!this._cursorWindowClone) {

You can save one level of indentation with:

    if (this._cursorWindowClone)
        return;

@@ +62,3 @@
+                let cursorWindow = windows[windows.length - 1];
+                // FIXME: more reliable way?
+                if (cursorWindow.is_override_redirect()) {

Dito.

@@ +78,3 @@
+            }
+        }
+        else if (this._cursorWindowClone) {

No line break before else.

I wonder if

} else {
    if (...)

is a bit cleaner (e.g. the test for _cursorWindowClone is not on the same logical level as the visibility test, but rather on the same one as the identical test in the if part)

@@ +104,3 @@
+                let result = motionFunc(dragEvent);
+                if (result != DND.DragMotionResult.CONTINUE)
+                    break;

Should be return - the point is that drag monitors can overwrite normal dnd handling (e.g. blocking handleDragOver functions).

@@ +116,3 @@
+                                                                      global.get_current_time());
+                    if (result != DND.DragMotionResult.CONTINUE)
+                        break;

The break here is OK, but I'd still use return for consistency.

Review of attachment 176656 [details] [review]:

Also, xdndHandler.js needs to be added to the build system.

Comment on attachment 176373 [details] [review]
gdkdnd-x11: Don't remove shape events from the queue

Moved to bug 637691

Review of attachment 176270 [details] [review]:

Marking reviewed to get it off the list - no further comments apart from the missing import.

(In reply to comment #57)
> Review of attachment 176656 [details] [review]:
> 
> Much better, but the handling of the return values of dragMonitor.motionFuncs
> still is not quite correct. The removal of the "toggle" behavior of the hot
> corner has improved the reliability when triggering the overview a lot -
> strangely, only in the case of being "too slow": if I leave the button while
> the overview transition is still ongoing, I'm kicked out of the overview. No
> obvious idea what's triggering this - would be cool if you manage to track it
> down, otherwise I'm OK with handling it in a new bug after landing this.

Hmmm ... sounds like you are moving it away before chrome.js updates the input shape.

> ::: js/ui/panel.js
> @@ +755,3 @@
> +                    if(!Main.overview.visible &&
> !Main.overview.animationInProgress) {
> +                        this.rippleAnimation();
> +                        Main.overview.showTemporarily();
> 
> Hmmm, I'm not sure about the ripple animation on hovering the button. On the
> one hand, it is quite similar to the hot corner (e.g. area triggering an even
> on hover), but on the other hand the animation is pretty much tied to the
> corner (e.g. it's a bit weird that when placing the pointer on the right edge
> of the button the top-left corner indicates activity)

The reason why I added is because you can't really go to the hotcorner unless you move over the button first which means no ripple for the hotcorner. We probably should use a (sub second) timeout here and remove the ripple from the button.

> Also, I sometimes manage to throw the pointer into the corner quick enough to
> not trigger the handler (compiling helps) - you probably don't want to remove
> the hotCorner.handleDragOver handler (the previous comment was about the
> hiding, not the entire handler).

Yeah makes sense.

> ::: js/ui/xdndHandler.js
> @@ +58,3 @@
> +    _onWindowGroupVisibilityChanged : function() {
> +        if (!global.window_group.visible) {
> +            if (!this._cursorWindowClone) {
> 
> You can save one level of indentation with:
> 
>     if (this._cursorWindowClone)
>         return;

OK

> @@ +62,3 @@
> +                let cursorWindow = windows[windows.length - 1];
> +                // FIXME: more reliable way?
> +                if (cursorWindow.is_override_redirect()) {
> 
> Dito.

OK

> @@ +78,3 @@
> +            }
> +        }
> +        else if (this._cursorWindowClone) {
> 
> No line break before else.
> 
> I wonder if
> 
> } else {
>     if (...)
> 
> is a bit cleaner (e.g. the test for _cursorWindowClone is not on the same
> logical level as the visibility test, but rather on the same one as the
> identical test in the if part)

OK

> @@ +104,3 @@
> +                let result = motionFunc(dragEvent);
> +                if (result != DND.DragMotionResult.CONTINUE)
> +                    break;
> 
> Should be return - the point is that drag monitors can overwrite normal dnd
> handling (e.g. blocking handleDragOver functions).

OK

> @@ +116,3 @@
> +                                                                     
> global.get_current_time());
> +                    if (result != DND.DragMotionResult.CONTINUE)
> +                        break;
> 
> The break here is OK, but I'd still use return for consistency.

OK

Created attachment 177570 [details] [review]
Add a facility to show the stage without grabbing

When the user is doing a drag-and-drop, we want to temporarily show the
stage to allow them to drag to a different window. But we're not "really"
in the overview, and getting a grab would conflict with the X client doing
the drag and drop.

So add a showTemporarily()/hideTemporarily() pair of methods that show
the overview without grabbing.

This adds a lot more possibilities for asynchronous race conditions, so
rework the code to be more robust against multiple calls to show*()
and hide*(). The interpretation is now that all calls to show*() and
hide*() affect the state, but if we have conflicting calls to show and
hide we wait until the current animation is finished before correcting
to the right visual state.

----

*) Rebased
*) Added missing import

Created attachment 177571 [details] [review]
Implement cross overview drag & drop

The gnome-panel allows the user to hover over a tasklist entry
while draging to activate a minimized or obscured window and drop onto it.

Implement a similar behaviour by allowing draging to the activities button or
the hotcorner (and thus opening the overview), which allows the user to
activate any window (even on different workspaces) as a drop target.

----

*) Rebased
*) Fixed ripple behaviour
*) Fixed issues noted in the review

Review of attachment 177571 [details] [review]:

Looks good.

::: js/ui/panel.js
@@ +849,3 @@
     },
 
+    _xdndShowOverview : function (actor) {

We are not very consistent on that, but the vast majority of functions don't use a space before the colon (1043 vs 185).

::: js/ui/xdndHandler.js
@@ +56,3 @@
+    },
+
+    _onWindowGroupVisibilityChanged : function() {

Another space on the left of the colon ...

Review of attachment 177570 [details] [review]:

Looks good.

::: js/ui/overview.js
@@ +286,3 @@
+    },
+
+    _animateVisible : function() {

See previous comment.

@@ +418,3 @@
+    //// Private methods ////
+
+    _syncInputMode : function() {

dto.

Attachment 177570 [details] pushed as 62507c9 - Add a facility to show the stage without grabbing
Attachment 177571 [details] pushed as ceedc7e - Implement cross overview drag & drop