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 777504 - window-list: Hide workspace indicator when there's 1 workspace
window-list: Hide workspace indicator when there's 1 workspace
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: extensions
3.23.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
: 777496 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-01-19 17:53 UTC by Felipe Borges
Modified: 2017-01-26 20:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
window-list: Hide workspace indicator when there's 1 workspace (1.70 KB, patch)
2017-01-19 17:53 UTC, Felipe Borges
none Details | Review
window-list: Hide workspace indicator when there's 1 workspace (1.81 KB, patch)
2017-01-19 18:50 UTC, Felipe Borges
none Details | Review
window-list: Hide workspace indicator when there's 1 workspace (1.81 KB, patch)
2017-01-19 18:52 UTC, Felipe Borges
none Details | Review
window-list: Hide workspace indicator when there's 1 workspace (3.93 KB, patch)
2017-01-20 14:10 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2017-01-19 17:53:25 UTC
.
Comment 1 Felipe Borges 2017-01-19 17:53:48 UTC
Created attachment 343829 [details] [review]
window-list: Hide workspace indicator when there's 1 workspace

There's no need to show the workspace indicator at the right
corner of the window-list if there's just a single workspace
AND the workspace creation is static. This saves us a bit more
of space.
Comment 2 Florian Müllner 2017-01-19 18:36:45 UTC
Review of attachment 343829 [details] [review]:

This makes sense to me and is consistent with the workspace switcher in the overview (see https://git.gnome.org/browse/gnome-shell/tree/js/ui/workspaceThumbnail.js#n682).

However the code isn't limited to static workspaces as claimed by the commit message - I don't think it is useful to have the workspace switcher suddenly appear when the first window is created, in particular when your main rationale are space savings (which aren't really an issue with an empty window list).
Comment 3 Felipe Borges 2017-01-19 18:50:42 UTC
Created attachment 343831 [details] [review]
window-list: Hide workspace indicator when there's 1 workspace

There's no need to show the workspace indicator at the right
corner of the window-list if there's just a single workspace
AND the workspace creation is static. This saves us a bit more
of space.
Comment 4 Felipe Borges 2017-01-19 18:52:34 UTC
Created attachment 343832 [details] [review]
window-list: Hide workspace indicator when there's 1 workspace

There's no need to show the workspace indicator at the right
corner of the window-list if there's just a single workspace
AND the workspace creation is static. This saves us a bit more
of space.
Comment 5 Florian Müllner 2017-01-19 19:32:27 UTC
Review of attachment 343832 [details] [review]:

::: extensions/window-list/extension.js
@@ +940,2 @@
         this._workspaceIndicator.actor.visible =
+            (this._workspaceSettings.get_boolean('dynamic-workspaces') ||

I'm afraid you cannot simply reuse those settings - this._workspaceSettings is the GSettings object that is used for the 'workspaces-only-on-primary' setting, which isn't necessarily the same for the 'dynamic-workspaces' one. Namely, both org.gnome.shell.overrides and org.gnome.shell.extensions.classic-overrides override the 'workspaces-on-primary' setting, but only the former overrides the 'dynamic-workspaces' one as well. So in classic mode, you'll try to look up a non-existent setting here, which is a fatal error ...

@@ +940,3 @@
         this._workspaceIndicator.actor.visible =
+            (this._workspaceSettings.get_boolean('dynamic-workspaces') ||
+             global.screen.n_workspaces > numWorkspaces > 1) &&

This is always false because global.screen.n_workspaces == numWorkspaces ;-)

While at it, how about splitting this up for readability?

  let hasWorkspaces = this._workspaceSettings.get_boolean('dynamic-workspaces') ||
                      global.screen.n_workspaces > 1;
  let workspacesOnMonitor = this._monitor == Main.layoutManager.primaryMonitor ||
                            !this._workspaceSettings.get_boolean('workspaces-only-on-primary');
  this._workspaceIndicator.actor.visible = hasWorkspaces && workspacesOnMonitor;

@@ +1114,3 @@
         }
+
+        this._updateWorkspaceIndicatorVisibility();

It is likely that the number of workspaces changes when the 'dynamic-workspaces' setting is changed, but probably a good idea to call this explicitly in that case anyway (I can only think of the case where the setting is changed from the <alt>f2 dialog when no windows are open where it makes a difference, but still ...)
Comment 6 Felipe Borges 2017-01-20 14:10:47 UTC
Created attachment 343905 [details] [review]
window-list: Hide workspace indicator when there's 1 workspace

There's no need to show the workspace indicator at the right
corner of the window-list if there's just a single workspace
AND the workspace creation is static. This saves us a bit more
of space.
Comment 7 Florian Müllner 2017-01-20 14:44:13 UTC
Review of attachment 343905 [details] [review]:

I didn't test this (sorry), but the code LGTM.
Comment 8 Felipe Borges 2017-01-20 15:31:50 UTC
Attachment 343905 [details] pushed as 0f9ac60 - window-list: Hide workspace indicator when there's 1 workspace
Comment 9 Joe Wright 2017-01-26 20:55:09 UTC
*** Bug 777496 has been marked as a duplicate of this bug. ***