After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 664202 - Creating workspaces at arbitrary positions only works for windows
Creating workspaces at arbitrary positions only works for windows
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Owen Taylor
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2011-11-16 13:57 UTC by Florian Müllner
Modified: 2012-03-16 16:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
autoWorkspaces: fix creation of new workspaces with application launchers (2.83 KB, patch)
2012-02-13 19:20 UTC, Stefano Facchini
needs-work Details | Review
autoWorkspaces: fix creation of new workspaces with application launchers (5.85 KB, patch)
2012-02-18 21:55 UTC, Stefano Facchini
needs-work Details | Review
autoWorkspaces: fix creation of new workspaces with application launchers (5.89 KB, patch)
2012-03-15 14:10 UTC, Stefano Facchini
committed Details | Review

Description Florian Müllner 2011-11-16 13:57:00 UTC
When dragging a launcher between two existing workspaces, a new workspace is created correctly, but before the application opens its first window, the workspace is auto-removed.
Comment 1 Stefano Facchini 2012-02-13 19:20:24 UTC
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.
Comment 2 Owen Taylor 2012-02-15 23:44:22 UTC
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.
Comment 3 Stefano Facchini 2012-02-18 21:55:09 UTC
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.
Comment 4 Owen Taylor 2012-03-14 18:36:31 UTC
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
Comment 5 Stefano Facchini 2012-03-15 14:10:39 UTC
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.
Comment 6 Owen Taylor 2012-03-16 13:06:19 UTC
Review of attachment 209840 [details] [review]:

Looks good
Comment 7 Florian Müllner 2012-03-16 16:20:24 UTC
Attachment 209840 [details] pushed as 3c6737f - autoWorkspaces: fix creation of new workspaces with application launchers