Created attachment 207483 [details] [review]
autoWorkspaces: fix creation of new workspaces with application launchers

A function is added which allows to keep an empty workspace around
for some time. This is used to prevent the immediate removal of the
new workspace created when dragging an application launcher between
two existing workspaces.

Review of attachment 207483 [details] [review]:

I don't like the arbitrary 5 second delay. We already have tracking of launching an application and a timeout for that in the startup notification code.

See ShellWindowTracker - looks like something like:

 tracker = Shell.WindowTracker.get_default();
 tracker.connect('startup-sequence-changed', function() {
     if (tracker.get_startup_sequences().length > 0)
         log("Have starting app");
     else
         log("No starting app");
 });

would work. (Entirely untested). You could also add shell_startup_sequence_get_workspace() to more specifically only avoid collecting a workspace where something is started.

I'm not sure offhand how the startup notification stuff works, so you might need to add a very brief for the startup sequence to load, but it's a lot better than timing out with a separate timeout before the application opens its first window.

Created attachment 207957 [details] [review]
autoWorkspaces: fix creation of new workspaces with application launchers

In the workspace-collecting code we add a check to avoid collecting a
workspace if any startup sequence is running there. Since the sequence
can take some time to load, an helper function is also added which keeps
the (empty) workspace around for a very short time, while waiting for the
sequence to start.

---

Thanks Owen for the useful hints! I included here your suggestions.

Unluckly, this patch makes more visible a preexistent issue with some
single-instance applications which *do* implement the startup
notification protocol, but do it badly when it comes to open more than
one window. For example, Nautilus and Epiphany simply open a new
window in the current workspace regardless of where the launcher is
dropped (for Nautilus see bug 642684). Gedit opens just a new tab.

A remarkable exception is gnome-terminal, which has some custom code to
correctly set the startup id for every new window.

What's worse with this patch is that while the application opens the
window in the current workspace, another workspace remains oddly empty
for ~10 secs.

Maybe the ability to create new workspaces with app launchers could be
temporarly removed for 3.4, while waiting for at least the core
applications to be fixed.

Review of attachment 207957 [details] [review]:

Generally looks good to me - few comments

::: js/ui/main.js
@@ +285,3 @@
+    let sequences = Shell.WindowTracker.get_default().get_startup_sequences();
+    for (i = 0; i < sequences.length; i++)
+        emptyWorkspaces[sequences[i].get_workspace()] = false;

Checking the code, it looks like sequence.get_workspace() can return -1 if the sequence doesn't have a workspace set on it. Also, we should at least guard against a workspace number here greater than the number of workspaces.

@@ +334,3 @@
 }
 
+function timeoutWorkspaceRemoval(workspace, duration) {

A better name would be:

 keepWorkspaceAlive()

@@ +336,3 @@
+function timeoutWorkspaceRemoval(workspace, duration) {
+    if (workspace._keepAlive)
+        Mainloop.source_remove(workspace._keepAliveId);

I think it's cleaner to just have one variable keepAliveId

 if (workspace._keepAliveId != null) {
     Mainloop.source_remove(workspace._keepAliveId);
     delete Mainloo._keepAliveId;      
 }

And avoid having the keepAlive boolean say whether the keepAliveId field is valid or stale.

::: js/ui/workspaceThumbnail.js
@@ +26,3 @@
 const WORKSPACE_CUT_SIZE = 10;
 
+const WORKSPACE_REMOVAL_TIMEOUT = 100;

Given the name changed I suggested, something likeL

 WORKSPACE_KEEP_ALIVE_TIME

would be better

Created attachment 209840 [details] [review]
autoWorkspaces: fix creation of new workspaces with application launchers

In the workspace-collecting code we add a check to avoid collecting a
workspace if any startup sequence is running there. Since the sequence
can take some time to load, an helper function is also added which keeps
the (empty) workspace around for a very short time, while waiting for the
sequence to start.

Review of attachment 209840 [details] [review]:

Looks good

Attachment 209840 [details] pushed as 3c6737f - autoWorkspaces: fix creation of new workspaces with application launchers