Nice - I like this: it's very intuitive.

-Mike.

Created attachment 133696 [details] [review]
adds drag-anything-onto-add-workspace-button functionality

This works just fine for me.  All I did was create a new class for the AddWorkspaceButton, and use that in place of the texture.  Then I just create a new workspace and delegate the drop function for that button to that newly created workspace.

A few caveats: I'm certain my Clutter-ignorance caused some wonkiness, there are a few bits in the logs like:
(metacity:24187): Clutter-WARNING **: Actor of type ClutterTexture is not inside a container


But it works perfectly over here, aside from warnings.

I just noticed a bug that this patch introduces.  The 'remove workspace' button is now missing.  I don't know why that happens.

OK.

> (metacity:24187): Clutter-WARNING **: Actor of type ClutterTexture is not
> inside a container

this is caused by calling this.actor.lower_bottom(); in the constructor for
AddWorkspaceButton before it has been added to any parent container. You
need to leave that call back in Workspaces.init. (The point of the call is to make sure that the button is the lowermost actor inside the Workspaces group, so that as workspaces slide on and off screen, they all pass *over* the button rather than *under* it.)

I don't like passing the "workspaces" object to AddWorkspaceButton and having AddWorkspaceButton poke around in its internals. (Instance variables whose names start with "_" are considered private.) It would be better to have AddWorkspaceButton emit a signal which Workspaces would then catch and do the right thing with. To do that, just add

    Signals.addSignalMethods(AddWorkspaceButton.prototype);

after the block declaring AddWorkspaceButton.prototype, and then from inside AddWorkspace, you can do "this.emit('signalname', argument, argument, etc);"
and Workspaces can catch this by doing addButton.connect('signalname', ...) as with other signals.

Reusing Workspace's acceptDrop is a good idea, although that then leaves the problem of what to do if the newly-created workspace doesn't actually want to accept the dragged item. (This wouldn't happen now because there are only two kinds of dragged item, and Workspace accepts both of them, but it could potentially happen in the future.)

Created attachment 133766 [details] [review]
Same functionality as previous patch, but done with Signals per danw

I believe I've cleaned up any of the particularly uncool things that the first patch did, aside from a few warnings still.

Newer patch: used signals per danw.  It replaces the old patch altogether.

It has a new warning:
(metacity:24017): Clutter-WARNING **: Actor of type ClutterGroup is not inside a container

It also retains the unfortunate bug whereby we lose the 'remove workspace' button.

Created attachment 133769 [details] [review]
Cleaned a bit of stuff up per owen

This replaces the previous two patches.

The latest patch still offers up a warning,
(metacity:24017): Clutter-WARNING **: Actor of type ClutterTexture is not inside a container

Also, there are still no remove workspace buttons.  It cleaned up a few things owen suggested, as well as fixed a regression I introduced whereby clicking the add workspace button didn't...add....a workspace.

Created attachment 133770 [details] [review]
sigh, fixed one more tiny warning regression

Replaces all previous patches, re-fixes the clutter texture warning...

Created attachment 133772 [details] [review]
replaces previous patches; 

sanderd pointed out an inconsistency in quote-use and a few newlines I needed to add for consistency, as well as a trailing semicolon I'd omitted.

Looking just about right. I got a bit of intermittent unreliability (workspace not added) when testing earlier, but can't reproduce it now, so I'll ignore that and say it is good to go in with the notes below:

 Signals.addSignalMethods(DesktopClone.prototype);
 
-
 function Workspace(workspaceNum) {
     this._init(workspaceNum);
 }

Please avoid unrelated whitespace change, even when they are correct.

-        buttonSize = addButtonSize;

This is why the removeButton went away. buttonSize is actually a global variable; without it being set the remove buttons were being created at a zero size. (Took me quite a while to track it down.) Adding a comment like 

  // Stash the button size in a global variable so we can us it to create 
  // matching button sizes for workspace remove buttons

Is probably a good idea.

I think the 'add-workspace-drop' signal should be called something like 'drop-received' - it's already an AddWorkspace button.

And finally, when you resubmit the patch, could you 'git commit' it with a nice commit log message, and then use 'git format-patch HEAD -1' to create the patch for attachment.

Josh, interested in doing another iteration based on Owen's comments? I really like the feature, it would *greatly* improve my activity workflow.

Michael,

I am interested, but I'm in the middle of launching a big project at work and just bought a house (AND started working out / stopped being such a slob), so I've become a bit busier than I'm used to.

If I haven't come through in a couple of weeks, I'm not against someone pinging me again to remind me, as the changes are minor.

I have an idea to open the application in a new workspace similar to how firefox opens a link in a new tab. When you middle click or click both mouse buttons on a link in firefox it opens that link in a new tab. In Gnome-Shell we can do the same for applications in new workspaces. 

(In reply to comment #14)
> I have an idea to open the application in a new workspace similar to how
> firefox opens a link in a new tab [...]

Nice idea for an additional way to do this. Still, the (+) should also be a drop target because not everyone has a middle mouse button (most touchpad users etc)

Well clicking both the left and right buttons works for opening the link in the new tab in firefox we could do the same and maybe for people with one button on the mouse ctrl+click (This was suggested on the mailing list)

Created attachment 140597 [details] [review]
Make dragging an item to the Add Workspace button launch it on a new workspace

This patch cleans up the previous patch from Josh and applies all the comments from Owen.

Seeing some unreliability:

 - Dropping windows on the (+) button works some of the time, but not most of the time. (Yes, I'm being careful to position the pointer over the plus)

 - Dropping documents on the (+) button launches the document but doesn't open a new workspace

Neither bug is immediately apparent to me as the cause. One problem I do see (but doesn't immediately explain the problem to me) is that the delegate acceptDrop function is supposed to return a boolean for whether the drop was handled or not. So, that has to be propagated back in some way. Since AFAIK the signals module doesn't have return values, that may mean that the drop-received signal need to be replaced with a callback function passed to the constructor.

(Reading the code I'd expect all drop to show a "snap back" since a function without a return statement returns 'undefined' which is false. but that doesn't happen.

Other than that, the patch looks good to me.

Created attachment 140606 [details] [review]
Make dragging an item to the Add Workspace button launch it on a new workspace

Use a callback function for accepting the drop instead of a signal so that we can return a flag indicating if the drop was accepted.

Dropping all items (apps, documents, windows) works reliably for me in Xephyr.

(In reply to comment #19)
> 
> Dropping all items (apps, documents, windows) works reliably for me in Xephyr.
> 

Works perfectly fine here, too. (with --replace)

Unreliability was bug 591643 - with that fixed it all works fine for me. This should be good to commit.

So what about the middle click functionality to open the application on the new workspace?

(In reply to comment #22)
> So what about the middle click functionality to open the application on the new
> workspace?

File that as a separate RFE if you are interested in it. (Should be relatively easy to do if you want to come up with a patch.)

I created a new feature request http://bugzilla.gnome.org/post_bug.cgi
I think it may already be implemented in the breadcrumbs branch though ill have a look.