Created attachment 226840 [details] [review]
Patch to hide workspaces view

The basic idea is to listen to the settings from d-conf and if dynamic workspaces is enabled and the count is set to 1 we hide the switcher.

(we use the assign field to indicate who is responsible for reviewing a bug, which obviously must not be the patch author)

Review of attachment 226840 [details] [review]:

Mostly style issues.

Oh, and the format of the commit message is wrong - the subject is way too long (rule of thumb: git log in a 80-char terminal should not wrap the line), confusing ("hide the workspace view? why would you do that?") and misses a body (not necessarily required, but in this case gory details like the exact condition should go there).

::: js/ui/workspacesView.js
@@ +544,3 @@
         this._swipeScrollEndId = 0;
+
+        this._isHidden = false;

this._isHidden sounds interchangeable with this.actor.visible - this._shouldHide/this._shouldBeHidden would be better, though I'd just get rid of the property altogether (see below).

@@ +546,3 @@
+        this._isHidden = false;
+        this._settings = new Gio.Settings({ schema: OVERRIDE_SCHEMA });
+        this._wm_settings = new Gio.Settings({ schema: WM_SCHEMA });

Style nit - _wmSettings instead of _wm_settings

@@ +547,3 @@
+        this._settings = new Gio.Settings({ schema: OVERRIDE_SCHEMA });
+        this._wm_settings = new Gio.Settings({ schema: WM_SCHEMA });
+        this._allowDynamicWorkspacesId =

It doesn't make sense to store the handler id if you are not going to disconnect the signals.

@@ +559,3 @@
+        if (!this._settings.get_boolean('dynamic-workspaces') &&
+            this._wm_settings.get_int('num-workspaces') == 1) {
+            this._thumbnailsBox.actor.hide();

More concise as

this._thumbnailsBox.actor.visible = this._settings.get_boolean('dynamic-workspaces') || this._wmSettings.get_int('num-workspaces') > 1;

@@ +590,3 @@
+            this._thumbnailsBox.hide();
+        else
+            this._thumbnailsBox.show();

Why not just call _onWorkspacesSettingsChanged() instead of using isHidden?
(I'd call the method _updateVisibility() or something, but that's more of a personal preference)

Created attachment 226843 [details] [review]
Patch to hide workspaces switcher

(Interjecting with a style suggestion; you didn't do anything wrong here, Seif)

Can we please stop doing things like WM_SCHEMA? They add no meaning to the code and are clearly a holdover from the C #define days.

It's especially exhausting when we have "changed::" + SOME_WEIRD_KEY, which is a poor translation of the C static string concatenation behavior.

Review of attachment 226843 [details] [review]:

Sorry it took me a while before re-reviewing this, I found a couple of issues while testing your patch (bug 686487).

I still don't like the commit message - "Add support" suggests that this is some user option, not automated behavior. Something like "workspacesView: Don't show workspace switcher when it doesn't make sense" makes more sense to me. Also the body should mention the reasoning, not only a description of what the patch does (the code itself already shows that).

::: js/ui/workspacesView.js
@@ +549,3 @@
+            Lang.bind(this, this._updateVisibility));
+        this._wmSettings.connect('changed::num-workspaces',
+            Lang.bind(this, this._updateVisibility));

Actually - we already connect to global.screen::notify::n-workspaces, so we could just reuse that; you'll need to call updateVisibility() in _workspacesChanged() then.

@@ +550,3 @@
+        this._wmSettings.connect('changed::num-workspaces',
+            Lang.bind(this, this._updateVisibility));
+        this._updateVisibility();

Calling this in show() and on updates is enough, you don't need the call here.

@@ +553,3 @@
+    },
+
+    _updateVisibility: function() {

OK, this was my suggestion, but: this suggests that it updates the visibility of the workspaces display, not just the switcher. Please rename to _updateSwitcherVisibility() or _updateWorkspaceThumbnailsVisibility() or something. Sorry about that.

@@ +556,3 @@
+        this._thumbnailsBox.actor.visible =
+            this._settings.get_boolean('dynamic-workspaces') ||
+                this._wmSettings.get_int('num-workspaces') > 1;

This would be global.screen.n_workspaces now.

Created attachment 226857 [details] [review]
Don't show workspaces switcher when it doesn't make sense

Review of attachment 226857 [details] [review]:

Good to commit after fixing this:

::: js/ui/workspacesView.js
@@ +23,2 @@
 const OVERRIDE_SCHEMA = 'org.gnome.shell.overrides';
+const WM_SCHEMA = 'org.gnome.desktop.wm.preferences'

Left-over constant from previous iteration.

Ok done... and pushed