Created attachment 210987 [details] [review]
workspacesDisplay: Connect to 'notify::n-workspaces' early

Currently we only connect to the 'notify::n-workspaces' signal the
first time the overview is shown, which means we will miss any
changes to the workspace layout in the meanwhile.
In particular, the decision of whether the workspace switcher should be
shown is taken before the dynamic workspace handling takes over, and is
thus based entirely on the value of the num-workspaces user preference
rather than the actual number of workspaces.
Just connect the signal in _init() (with the nice side-effect to make it
explicit that the signal handler won't ever be disconnected).

Review of attachment 210987 [details] [review]:

I'm not sure I like this patch. The only effect of connecting to the signal in _init is to make sure that _updateAlwaysZoom is called, but we could do that before in show(), right?

(In reply to comment #2)
> I'm not sure I like this patch. The only effect of connecting to the signal in
> _init is to make sure that _updateAlwaysZoom is called, but we could do that
> before in show(), right?

If you mean something like

  if (this._nWorkspacesChangedId == 0)
      this._updateAlwaysZoom();

then yes, that's what my original patch did. I don't think it's particularly obvious to use the signal handler id like that, but it works.

Just doing it unconditionally would work as well of course, but every call except for the very first would be unnecessary, which just doesn't feel right :)

(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure I like this patch. The only effect of connecting to the signal in
> > _init is to make sure that _updateAlwaysZoom is called, but we could do that
> > before in show(), right?
> 
> If you mean something like
> 
>   if (this._nWorkspacesChangedId == 0)
>       this._updateAlwaysZoom();
> 
> then yes, that's what my original patch did. I don't think it's particularly
> obvious to use the signal handler id like that, but it works.

Yeah, I didn't like it either.

> Just doing it unconditionally would work as well of course, but every call
> except for the very first would be unnecessary, which just doesn't feel right
> :)

I think that's the cleaner approach.

Alternate suggestion:

    _workspacesChanged: function() {
        // We need to keep track of _alwaysZoomOut even before
        // workspaces exist, in order to make sure that our first
        // trip to the overview is handled correctly.
        this._updateAlwaysZoom();

        if (this._workspaces == null)
            return;

        ...
    }

Created attachment 211019 [details] [review]
workspacesDisplay: Connect to 'notify::n-workspaces' early

(In reply to comment #4)
> Alternate suggestion:
> 
>     _workspacesChanged: function() {
>         // We need to keep track of _alwaysZoomOut even before
>         // workspaces exist, in order to make sure that our first
>         // trip to the overview is handled correctly.
>         this._updateAlwaysZoom();

That looks best to me - it gets rid of the oldNumWorkspaces == newNumWorkspaces optimization, but the whole test seems rather pointless anyway.

I left out the comment though, as I think it's misleading (we need to always keep track of _alwaysZoomOut, even when we bail out early because the overview is hidden - it just happens that we already did that correctly after the overview had been shown once)

Review of attachment 211019 [details] [review]:

Yes.

Attachment 211019 [details] pushed as 76005f5 - workspacesDisplay: Connect to 'notify::n-workspaces' early