My idea was that we'd add the timeout only in cases where we think this is likely going on - might be as simple as when the last window on the workspace was a dialog window or splash screen.

(My guess is that most apps that put up an unsaved changse dialog will close that dialog *before* their main window. If that ends up being a problem we could add more heuristics about whether the window had been transient for another window, etc.)

Then you can probably make the timeout generous - 10 seconds or so.

One thing is that apps like firefox have occasional required restarts (modifying or updating extensions usually), and for example I always use it on its own workspace.

I agree, auto-closing is none sense. When I start gnome-shell from GNOME 2, all my gnome-terminal are moved to the same desktop and so most of my desktops are closed....

Created attachment 181022 [details] [review]
autoWorkspaces: Better handle apps that open a slashscreen or dialog at startup

Currently we remove a workspace when last window on that workspace closes,
which turned out to be too agressive as some apps open either a splashscreen
or a "I have crashed last time" dialog at startup.

Try to detect such apps and give them a chance to open their "real window(s)"
on the workspace.

Created attachment 181048 [details] [review]
windowManager: Add mechanism to block animations

Add an API to allow blocking animations in situations where
they aren't desireable.

Created attachment 181049 [details] [review]
autoWorkspaces: Merge empty workspaces with the always empty one

When closing a workspace due to the last window on that workspace
closing, switch to the overview and show the always empty workspace
rather then just going to the adjacent workspace.

Created attachment 181050 [details] [review]
shell_global: Use clutter_event_get_time rather then get_current_event_time

Using clutter_get_current_event_time can result into too old timestamps
in some situations like after a grab where the event has not been delivered
to clutter and hence being out of date.

Created attachment 181064 [details] [review]
shell_global: Use clutter_event_get_time rather then get_current_event_time

Using clutter_get_current_event_time can result into too old timestamps
in some situations like after a grab where the event has not been delivered
to clutter and hence being out of date.

---

Add NULL check

Created attachment 181204 [details] [review]
autoWorkspaces: Merge empty workspaces with the always empty one

When closing a workspace due to the last window on that workspace
closing, switch to the overview and show the always empty workspace
rather then just going to the adjacent workspace.

---

Handle the case where the user moves the window to another workspace.
i.e either in the overview using dnd or using ctrl-alt-shit-up/down

We do not want to switch to the empty workspace in that case.

Review of attachment 181064 [details] [review]:

Body of commit message reads a bit odd - edit:

 Using clutter_get_current_event_time can result into too old timestamps
 when there is no current Clutter or Mutter event, since
 clutter_get_current_event_time() returns the timestamp of the last event
 delivered to Clutter. This can result in, for example, grabs failing. Use
 the event time of the current event (if any) and CurrentTime otherwise.

::: src/shell-global.c
@@ +1655,3 @@
+   * We don't use clutter_get_current_event_time as it can give us a
+   * too old timestamp shortly after grabs (as the event never got
+   * passed to clutter).

This also is confusing. Grabs really don't have anything to do with it

 it will give us a too old timestamp if there is no current event

@@ +1657,3 @@
+   * passed to clutter).
+   */
+  if ((clutter_event = clutter_get_current_event ()) != NULL)

Two lines

Created attachment 181523 [details] [review]
shell_global: Use clutter_event_get_time rather then get_current_event_time

Using clutter_get_current_event_time can result into too old timestamps
when there is no current Clutter or Mutter event, since
clutter_get_current_event_time() returns the timestamp of the last event
delivered to Clutter. This can result in, for example, grabs failing. Use
the event time of the current event (if any) and CurrentTime otherwise.

Review of attachment 181523 [details] [review]:

Looks good

Comment on attachment 181523 [details] [review]
shell_global: Use clutter_event_get_time rather then get_current_event_time

Attachment 181523 [details] pushed as 8a22ea9 - shell_global: Use clutter_event_get_time rather then get_current_event_time

Review of attachment 181022 [details] [review]:

::: js/ui/main.js
@@ +253,3 @@
+    let windowCount = global.get_window_actors().filter(function (win) {
+                        return !win.get_meta_window().is_on_all_workspaces() && win.get_workspace() == workspace.index();
+                      }).length;

Array.forEach() and Array.some() have the same compat considerations - both ECMAscript 5 additions.

So, you can write:

 let windowsOnWorkspace = global.get_window_actors().some(function(actor) { 
     /* check for window on workspace */
 });

@@ +256,3 @@
+
+    if (windowCount > 0)
+        return;

See below for why I'm not really that psyched about optimizing calls to queueCheckWorkspaces, but if this check is needed anyways to detect the "wait" case, it's pretty transparently OK.

@@ +269,3 @@
+    }
+    else
+        _queueCheckWorkspaces();

Here's the absolutely minimal code approach:

function _windowRemoved(workspace, window) {
  workspace._lastRemovedWindow = window;
  _queueCheckWorkspaces();
  Mainloop.timeout_add(LAST_WINDOW_GRACE_TIME, function() { 
      if (workspace._lastRemovedWindow == window) {
          workspace._lastRemovedWindow = null;
          _queueCheckWorkspaces();
      }
  });
}

And then do the obvious thing in checkWorkspaces.

@@ +272,3 @@
+}
+
+function _windowAdded(workspace, window) {

order them added/removed - removed/added is oddball.

@@ +274,3 @@
+function _windowAdded(workspace, window) {
+    if (workspace.index() != global.screen.n_workspaces - 1)
+        return;

I don't like this optimization ... I can't come up with a scenario where it is wrong, but I'd like the "check workspaces" step just to be fine to call whenever: reasonably cheap and does no harm. (The queue handling is meant to make sure that no matter how much changes we just run through all the windows once.)

(In reply to comment #14)
> Review of attachment 181022 [details] [review]:
> 
> ::: js/ui/main.js
> @@ +253,3 @@
> +    let windowCount = global.get_window_actors().filter(function (win) {
> +                        return !win.get_meta_window().is_on_all_workspaces()
> && win.get_workspace() == workspace.index();
> +                      }).length;
> 
> Array.forEach() and Array.some() have the same compat considerations - both
> ECMAscript 5 additions.
> 
> So, you can write:
> 
>  let windowsOnWorkspace = global.get_window_actors().some(function(actor) { 
>      /* check for window on workspace */
>  });

OK, wasn't aware of Array.some() but sounds like handy for this kind of task.

> @@ +256,3 @@
> +
> +    if (windowCount > 0)
> +        return;
> 
> See below for why I'm not really that psyched about optimizing calls to
> queueCheckWorkspaces, but if this check is needed anyways to detect the "wait"
> case, it's pretty transparently OK.

This wasn't supposed to be an optimization, I can make it call _queueCheckWorkspaces in this case but it is pointless.

> @@ +269,3 @@
> +    }
> +    else
> +        _queueCheckWorkspaces();
> 
> Here's the absolutely minimal code approach:
> 
> function _windowRemoved(workspace, window) {
>   workspace._lastRemovedWindow = window;
>   _queueCheckWorkspaces();
>   Mainloop.timeout_add(LAST_WINDOW_GRACE_TIME, function() { 
>       if (workspace._lastRemovedWindow == window) {
>           workspace._lastRemovedWindow = null;
>           _queueCheckWorkspaces();
>       }
>   });
> }
> 
> And then do the obvious thing in checkWorkspaces.

OK.

> @@ +272,3 @@
> +}
> +
> +function _windowAdded(workspace, window) {
> 
> order them added/removed - removed/added is oddball.

OK, but when removing the optimization in _windowAdded it will be nothing but a wrapper around _queueCheckWorkspaces so we could as well call that directly.

> @@ +274,3 @@
> +function _windowAdded(workspace, window) {
> +    if (workspace.index() != global.screen.n_workspaces - 1)
> +        return;
> 
> I don't like this optimization ... I can't come up with a scenario where it is
> wrong, but I'd like the "check workspaces" step just to be fine to call
> whenever: reasonably cheap and does no harm. (The queue handling is meant to
> make sure that no matter how much changes we just run through all the windows
> once.)

I did it because it seemed like an obvious (micro) optimization (i.e when not on the last workspace there is nothing to do anyway) but yeah can just remove it.

Created attachment 181754 [details] [review]
autoWorkspaces: Better handle apps that open a slashscreen or dialog at startup

Currently we remove a workspace when last window on that workspace closes,
which turned out to be too agressive as some apps open either a splashscreen
or a "I have crashed last time" dialog at startup.

Try to detect such apps and give them a chance to open their "real window(s)"
on the workspace.

---

Removed optimization and went with the simple approuch you proposed.

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

Review of attachment 181754 [details] [review]:

I like this better. As you said, the optimization of checking on added windows whether they are on the last workspace seems pretty obvious, but it isn't *completely* obvious to me that there couldn't be some intermediate state with a empty workspace in the middle if we add the ability to reorder workspaces or something. Less code the better I think.

Just some minor stuff here.

::: js/ui/main.js
@@ +223,3 @@
 let _checkWorkspacesId = 0;
 
+const LAST_WINDOW_GRACE_TIME = 1000;

Can you add a comment here for what this is about?

@@ +230,3 @@
 
+    for (i = 0; i < _workspaces.length; i++) {
+        let window = _workspaces[i]._lastRemovedWindow;

let lastRemoved = ... - this variable isn't 'window' in the context of iterating through workspace

@@ +268,3 @@
 
+function _windowRemoved(workspace, window) {
+   workspace._lastRemovedWindow = window;

indentation wrong

Created attachment 181760 [details] [review]
autoWorkspaces: Better handle apps that open a slashscreen or dialog at startup

Currently we remove a workspace when last window on that workspace closes,
which turned out to be too agressive as some apps open either a splashscreen
or a "I have crashed last time" dialog at startup.

Try to detect such apps and give them a chance to open their "real window(s)"
on the workspace.

(In reply to comment #18)
> Review of attachment 181754 [details] [review]:
> 
> I like this better. As you said, the optimization of checking on added windows
> whether they are on the last workspace seems pretty obvious, but it isn't
> *completely* obvious to me that there couldn't be some intermediate state with
> a empty workspace in the middle if we add the ability to reorder workspaces or
> something. Less code the better I think.

Well I couldn't think of a case where it would be a problem but well we are talking about a non measurable optimization here, so I agree that we should stay on the safe side here (it even means less code).

Here is the way I use workspaces: I have an app open on each of 10 workspaces and switch between them using shortcut key combinations. These are apps that I regular use in my work. I do much switching between them by switching workspaces. With the auto-remove feature this is what happens: My browser on workspace #1 crashes, leaves empty the workspace it was on and the workspace is removed and all the open applications effectively move to a new workspace and the keyboard shortcuts become useless for switching between applications. The auto-removing of empty workspaces in this case is a hindrance to my work.

There must at least be an option for "fixed" workspaces that are manually removed. Or, only the last empty workspace is auto-removed.

There are certainly many different ways workspaces get used.

Auto-close and switching to another workspace can break confidentiality.
It may be some mails or files we don't wan't to show or view
but that will win the focus if the current application suddenly crashes or restart;
What do you think of an visible and short timeout which can be clicked to stay in the current workspace ?
Also, what about a button to "Lock" on the current workspace even if it becomes empty ?
With my own usage, there are a few workspace opened for mail, download or compilation and the last for my work's tasks.
I don't have tested the different patch, but I will try

Review of attachment 181760 [details] [review]:

Good to commit, except for a suggestion about the comment.

::: js/ui/main.js
@@ +224,3 @@
 
+/*
+ * When an app starts with a dialog window or slash screen before mapping

splash screen not slash screen.

I think this comment is a little deceptive, because it implies we are paying attention to apps *starting*, and currently we aren't. Suggested alternative:

 When the last window closed on a workspace is a dialog or splash
 screen, we assume that it might be an initial window shown before
 the main window of an application, and give the app a grace period
 where it can map another window before we remove the workspace.

I've tried the patch 181760
It's better for apps with splash screen
But what about application quick restarting, such as firefox in many case ?

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

Created attachment 182788 [details] [review]
autoWorkspaces: Merge empty workspaces with the always empty one

OK, I don't understand the reason for the Meta.later_add(), and it seems
to work fine without it in this patch. I also don't see any reason for
the special casing of "user moved window"

Based on a patch from Adel Gadllah <adel.gadllah@gmail.com>.

Review of attachment 181204 [details] [review]:

As noted in the comment on the version attached, don't really understand the reasoning for the complexity here - something simpler seems to work.

Review of attachment 181048 [details] [review]:

Would like to see this._animationBlockCount rather than a boolean, to reduce risk of different code portions stepping on each other

(In reply to comment #26)
> Created an attachment (id=182788) [details] [review]
> autoWorkspaces: Merge empty workspaces with the always empty one
> 
> OK, I don't understand the reason for the Meta.later_add(), and it seems
> to work fine without it in this patch.

Actually I don't know why I started off with the complicated approach without testing the simpler one first; the reason was to hide the fact that mutter switches to the adjacent workspace after remove (and we switch to the empty one directly after that) so I wanted to hide a possible artifact here (going to a workspace and back) by having BEFORE_REDRAW priority but it seems to be indeed not needed.

> I also don't see any reason for
> the special casing of "user moved window"

Because it is very weird otherwise lets say you do move a window from a workspace using ctrl-alt-shift-up (and this was the last window on that workspace) switching to the empty workspace in that case is not what the user wants, he clearly moved to the adjacent workspace on purpose and took the window with him, so a switch to the empty one is clearly not what the user expects here.

The other case would be that a user is rearranging windows in the overview dragging a window out of a workspace to place it elsewhere should not switch the active workspace (actually it should in case the user was on the workspace being removed which my patch didn't address), it is simply not what the user expects and is inconsistent with other workspaces where dragging windows around does not actually change workspaces.

(In reply to comment #28)
> Review of attachment 181048 [details] [review]:
> 
> Would like to see this._animationBlockCount rather than a boolean, to reduce
> risk of different code portions stepping on each other

OK, makes sense.

(In reply to comment #29)
> (In reply to comment #26)
> > Created an attachment (id=182788) [details] [review] [details] [review]
> > autoWorkspaces: Merge empty workspaces with the always empty one
> > 
> > OK, I don't understand the reason for the Meta.later_add(), and it seems
> > to work fine without it in this patch.
> 
> Actually I don't know why I started off with the complicated approach without
> testing the simpler one first; the reason was to hide the fact that mutter
> switches to the adjacent workspace after remove (and we switch to the empty one
> directly after that) so I wanted to hide a possible artifact here (going to a
> workspace and back) by having BEFORE_REDRAW priority but it seems to be indeed
> not needed.
> 
> > I also don't see any reason for
> > the special casing of "user moved window"
> 
> Because it is very weird otherwise lets say you do move a window from a
> workspace using ctrl-alt-shift-up (and this was the last window on that
> workspace) switching to the empty workspace in that case is not what the user
> wants, he clearly moved to the adjacent workspace on purpose and took the
> window with him, so a switch to the empty one is clearly not what the user
> expects here.
> 
> The other case would be that a user is rearranging windows in the overview
> dragging a window out of a workspace to place it elsewhere should not switch
> the active workspace (actually it should in case the user was on the workspace
> being removed which my patch didn't address), it is simply not what the user
> expects and is inconsistent with other workspaces where dragging windows around
> does not actually change workspaces.

Obviously, we shouldn't switch to the empty workspace in the control-alt-shift case, but I don't think that requires any special casing - by the time the workspace removing code is run the window has been moved and the active workspace switched, so we just need to check if we'll be removing the current workspace or not, as my patch does.

Created attachment 182801 [details] [review]
windowManager: Add mechanism to block animations

Add an API to allow blocking animations in situations where
they aren't desireable.

---

Use a counter rather then a boolean.

(In reply to comment #31)
> (In reply to comment #29)
> > (In reply to comment #26)
>
> Obviously, we shouldn't switch to the empty workspace in the control-alt-shift
> case, but I don't think that requires any special casing - by the time the
> workspace removing code is run the window has been moved and the active
> workspace switched, so we just need to check if we'll be removing the current
> workspace or not, as my patch does.

Indeed this makes sense and indeed works.

Review of attachment 182788 [details] [review]:

Looks good and is much simpler than my original patch.

Review of attachment 182801 [details] [review]:

Looks good. (Possibly should warn about mismatched block/unblock, but not really worth it.)

Attachment 182788 [details] pushed as e054dd7 - autoWorkspaces: Merge empty workspaces with the always empty one
Attachment 182801 [details] pushed as 76d788a - windowManager: Add mechanism to block animations