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 737486 - window-list: option to show the window list on all monitors
window-list: option to show the window list on all monitors
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: extensions
3.14.x
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-09-27 15:46 UTC by Sylvain Pasche
Modified: 2016-12-03 17:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Disconnect drag and drop handlers (1.44 KB, patch)
2014-11-01 21:09 UTC, Sylvain Pasche
accepted-commit_now Details | Review
_pointerInTray was renamed to _pointerInNotification (bug 695800) (914 bytes, patch)
2014-11-01 21:09 UTC, Sylvain Pasche
accepted-commit_now Details | Review
window-list: Option to show the window list on all monitors (26.63 KB, patch)
2014-11-01 21:09 UTC, Sylvain Pasche
needs-work Details | Review
window-list: Disconnect drag and drop handlers (1.45 KB, patch)
2014-11-10 21:10 UTC, Sylvain Pasche
committed Details | Review
window-list: _pointerInTray was renamed to _pointerInNotification in bug 695800 (928 bytes, patch)
2014-11-10 21:13 UTC, Sylvain Pasche
committed Details | Review
window-list: Refactoring to use an Extension object (4.55 KB, patch)
2014-11-10 21:14 UTC, Sylvain Pasche
committed Details | Review
window-list: Move messageTray patching to the WindowList class (4.72 KB, patch)
2014-11-10 21:16 UTC, Sylvain Pasche
committed Details | Review
window-list: Refactor {Window,App}Button shared code into BaseButton (9.55 KB, patch)
2014-11-10 21:17 UTC, Sylvain Pasche
reviewed Details | Review
window-list: Option to show the window list on all monitors (18.37 KB, patch)
2014-11-10 21:18 UTC, Sylvain Pasche
needs-work Details | Review
window-list: Refactor {Window,App}Button shared code into BaseButton (15.53 KB, patch)
2014-11-16 15:25 UTC, Sylvain Pasche
committed Details | Review
window-list: Option to show the window list on all monitors (18.77 KB, patch)
2014-11-16 15:29 UTC, Sylvain Pasche
committed Details | Review
window-list preferences window with new checkbox (18.36 KB, image/png)
2014-12-01 18:08 UTC, Sylvain Pasche
  Details
window-list: Do not hardcode overrides schema (1.84 KB, patch)
2014-12-19 14:09 UTC, Florian Müllner
committed Details | Review

Description Sylvain Pasche 2014-09-27 15:46:59 UTC
When you have several monitors, it would be quite useful to show the taskbar on all monitors. The taskbar on each monitor would then show only the tasks on that monitor.

Windows 8 implements that feature. Here's the option dialog: http://goo.gl/geZ9xQ
Comment 1 Florian Müllner 2014-09-27 16:30:57 UTC
The window list extension is part of the classic session, which is why we are cautious about adding options; that said, this sounds reasonable to me, we should probably do this.
Comment 2 Sylvain Pasche 2014-11-01 21:09:08 UTC
Created attachment 289810 [details] [review]
Disconnect drag and drop handlers
Comment 3 Sylvain Pasche 2014-11-01 21:09:13 UTC
Created attachment 289811 [details] [review]
_pointerInTray was renamed to _pointerInNotification (bug 695800)
Comment 4 Sylvain Pasche 2014-11-01 21:09:18 UTC
Created attachment 289812 [details] [review]
window-list: Option to show the window list on all monitors
Comment 5 Sylvain Pasche 2014-11-01 21:16:54 UTC
So here's a patch that should be ready to review. I also attached two patches about small issues I found while working on this feature.

Hopefully things should still work the same as before when the multi-monitor option is not checked (the default setting).
Comment 6 Florian Müllner 2014-11-01 23:01:25 UTC
Review of attachment 289810 [details] [review]:

OK
Comment 7 Florian Müllner 2014-11-01 23:01:45 UTC
Review of attachment 289811 [details] [review]:

Grrr, yes
Comment 8 Florian Müllner 2014-11-01 23:01:52 UTC
Review of attachment 289812 [details] [review]:

Please split out the refactoring (BaseButton, Extension).

Also please add a proper commit message - subject-only commits are okay-ish for the other two patches, but here I'd like a bit more verbosity

::: extensions/window-list/extension.js
@@ +54,3 @@
     let [x, y,] = global.get_pointer();
     let actor = global.stage.get_actor_at_pos(Clutter.PickMode.REACTIVE, x, y);
+    if (extension.contains(actor))

extension.contains() reads odd, but I don't really have a good suggestion here - maybe someWindowListContains? Or make windowLists public and use extension.windowLists.some(function(wl) { return wl.actor.contains(actor); }) directly?

@@ +187,3 @@
 
+const BaseButton = new Lang.Class({
+    Name: 'BaseButton',

Are you sure there is not existing functionality that can be shared here?

@@ +190,3 @@
+    Abstract: true,
+
+    _init: function(isMultiMonitor, monitorIndex) {

isMultiMonitor is not a great name - it just suggests something like "Main.layoutManager.monitors.length > 1". How about perMonitor?

@@ +197,3 @@
+            this._windowEnteredMonitorId =
+                global.screen.connect('window-entered-monitor',
+                                      Lang.bind(this, this._windowEnteredOrLeftMonitor));

I don't like this. At the very least, it needs a big fat comment that subclasses *MUST* implement this method or stuff will fall apart

@@ +206,3 @@
+    _isWindowOnSameMonitor: function(window) {
+        if (!this._isMultiMonitor)
+            return true;

This is misleading. I'd just move this function inline in isWindowVisible:

  window.located_on_workspace() &&
  (!this._isMultiMonitor || window.get_monitor() == this._monitorIndex);

@@ +210,3 @@
+    },
+
+    _isWindowVisible: function(activeWorkspace, window) {

The workspace parameter is odd - if this is expected to always be the active workspace (as is the case here), use global.screen.get_active_workspace() inside the function.

@@ +218,3 @@
+    _onDestroy: function() {
+        if (this._isMultiMonitor) {
+            global.screen.disconnect(this._windowEnteredMonitorId);

The canonical style would be

if (this._windowEnteredMonitorId)
  global.screen.disconnect(...);
this._windowEnteredMonitorId = 0;

but OK

@@ +494,3 @@
     _updateVisibility: function() {
         let workspace = global.screen.get_active_workspace();
+        if (!this._isMultiMonitor) {

Style nit - no braces for one-line blocks

@@ +913,3 @@
     },
 
+    setMultiMonitor: function(multiMonitor) {

Could use a setter ("set multiMonitor(multiMonitor)", use like a normal property, e.g. "list.multiMonitor = true;")

@@ +922,3 @@
+
+    _initIndicatorsBox: function() {
+    },

I don't like this - the parent class should not know anything about what a subclass does. Please override _onDestroy/_getMaxWindowListWidth in PrimaryWindowList instead.

@@ +971,3 @@
+            }));
+        }
+        let nWindows = windows.length;

This is already an issue, but the additional filtering make this more likely: if nWindows == 0, the preferred ungrouped width is -spacing.
Maybe change this code to use something like:

let children = this._windowList.get_children().filter(function(c) { return c.visible; });
let nChildren = children.length;
if (nChildren == 0)
  return this._windowList.get_preferred_width(-1);

[...]

return nChildren * childWidth + (nChildren - 1) * spacing;

@@ +1247,3 @@
+
+    _init: function(monitor, isMultiMonitor) {
+        this.parent(monitor, isMultiMonitor);

This is a bit silly - "PrimaryWindowList" will (by definition) *always* use Main.layoutManager.primaryMonitor.
I wonder if it would actually be easier to not subclass windowList at all and just test for (this._monitor.index == Main.layoutManager.primaryIndex) to decide whether to add the indicatorBox - I doubt it would make the code in WindowList much harder, however not having to differentiate between primary and secondary window lists in Extension should make the code quite a bit easier.

@@ +1279,3 @@
+});
+
+const Extension = new Lang.Class({

If you add an Extension object, it should have enable/disable methods and be returned from init:

function init() {
  return new Extension();
}

@@ +1314,3 @@
+                                                            this._isMultiMonitor);
+        else
+            this._primaryWindowList.setMultiMonitor(this._isMultiMonitor);

No. If the function has been called in response to LayoutManager::monitors-changed rather than GSettings::changed, primaryWindowList._monitor may no longer be the primary monitor.

@@ +1351,3 @@
+
+        Main.messageTray._notificationWidget.hide();
+        Main.messageTray._notificationWidget.reparent(this._bottomActor || this._primaryWindowList.actor);

Looking at this, it doesn't really make sense to handle the messageTray/notifications if they are not on the same monitor as the window list. Probably out of scope of this bug, but would be nice to split out and handle properly ...

::: extensions/window-list/org.gnome.shell.extensions.window-list.gschema.xml.in
@@ +16,3 @@
       </_description>
     </key>
+    <key name="multi-monitor" type="b">

show-on-all-monitors maybe?
Comment 9 Sylvain Pasche 2014-11-10 21:10:59 UTC
Created attachment 290381 [details] [review]
window-list: Disconnect drag and drop handlers

Hi Florian, thanks for the review.

This patch is already reviewed, but I just updated the comment to be consistent with the others.
Comment 10 Sylvain Pasche 2014-11-10 21:13:21 UTC
Created attachment 290382 [details] [review]
window-list: _pointerInTray was renamed to _pointerInNotification in bug 695800

Updated comment for consistency (already reviewed).
Comment 11 Sylvain Pasche 2014-11-10 21:14:56 UTC
Created attachment 290383 [details] [review]
window-list: Refactoring to use an Extension object

> @@ +1279,3 @@
> +});
> +
> +const Extension = new Lang.Class({
> 
> If you add an Extension object, it should have enable/disable methods and be
> returned from init:
> 
> function init() {
>   return new Extension();
> }
> 

done.
Comment 12 Sylvain Pasche 2014-11-10 21:16:30 UTC
Created attachment 290384 [details] [review]
window-list: Move messageTray patching to the WindowList class

> @@ +1351,3 @@
> +
> +        Main.messageTray._notificationWidget.hide();
> +        Main.messageTray._notificationWidget.reparent(this._bottomActor ||
> this._primaryWindowList.actor);
> 
> Looking at this, it doesn't really make sense to handle the
> messageTray/notifications if they are not on the same monitor as the window
> list. Probably out of scope of this bug, but would be nice to split out and
> handle properly ...
> 
> :::

I moved the patching logic to WindowList to be handled only on the bottom monitor, which should makes things cleaner.
Comment 13 Sylvain Pasche 2014-11-10 21:17:25 UTC
Created attachment 290385 [details] [review]
window-list: Refactor {Window,App}Button shared code into BaseButton

> @@ +187,3 @@
> 
> +const BaseButton = new Lang.Class({
> +    Name: 'BaseButton',
> 
> Are you sure there is not existing functionality that can be shared here?
> 

I hadn't checked for other refactorings, but indeed I could share some more things.

> @@ +210,3 @@
> +    },
> +
> +    _isWindowVisible: function(activeWorkspace, window) {
> 
> The workspace parameter is odd - if this is expected to always be the active
> workspace (as is the case here), use global.screen.get_active_workspace()
> inside the function.
> 

fixed.
Comment 14 Sylvain Pasche 2014-11-10 21:18:40 UTC
Created attachment 290386 [details] [review]
window-list: Option to show the window list on all monitors

(In reply to comment #8)
> Review of attachment 289812 [details] [review]:
> 
> Please split out the refactoring (BaseButton, Extension).
> 
> Also please add a proper commit message - subject-only commits are okay-ish for
> the other two patches, but here I'd like a bit more verbosity
> 
> ::: extensions/window-list/extension.js
> @@ +54,3 @@
>      let [x, y,] = global.get_pointer();
>      let actor = global.stage.get_actor_at_pos(Clutter.PickMode.REACTIVE, x,
> y);
> +    if (extension.contains(actor))
> 
> extension.contains() reads odd, but I don't really have a good suggestion here
> - maybe someWindowListContains? Or make windowLists public and use
> extension.windowLists.some(function(wl) { return wl.actor.contains(actor); })
> directly?
> 

I went with someWindowListContains().

> @@ +190,3 @@
> +    Abstract: true,
> +
> +    _init: function(isMultiMonitor, monitorIndex) {
> 
> isMultiMonitor is not a great name - it just suggests something like
> "Main.layoutManager.monitors.length > 1". How about perMonitor?
> 

Yeah, sounds better.

> @@ +197,3 @@
> +            this._windowEnteredMonitorId =
> +                global.screen.connect('window-entered-monitor',
> +                                      Lang.bind(this,
> this._windowEnteredOrLeftMonitor));
> 
> I don't like this. At the very least, it needs a big fat comment that
> subclasses *MUST* implement this method or stuff will fall apart
> 

Added an empty base method with a comment.

> @@ +206,3 @@
> +    _isWindowOnSameMonitor: function(window) {
> +        if (!this._isMultiMonitor)
> +            return true;
> 
> This is misleading. I'd just move this function inline in isWindowVisible:
> 
>   window.located_on_workspace() &&
>   (!this._isMultiMonitor || window.get_monitor() == this._monitorIndex);
> 

done.

> @@ +218,3 @@
> +    _onDestroy: function() {
> +        if (this._isMultiMonitor) {
> +            global.screen.disconnect(this._windowEnteredMonitorId);
> 
> The canonical style would be
> 
> if (this._windowEnteredMonitorId)
>   global.screen.disconnect(...);
> this._windowEnteredMonitorId = 0;
> 
> but OK
> 
> @@ +494,3 @@
>      _updateVisibility: function() {
>          let workspace = global.screen.get_active_workspace();
> +        if (!this._isMultiMonitor) {
> 
> Style nit - no braces for one-line blocks
> 

ok.

> @@ +971,3 @@
> +            }));
> +        }
> +        let nWindows = windows.length;
> 
> This is already an issue, but the additional filtering make this more likely:
> if nWindows == 0, the preferred ungrouped width is -spacing.
> Maybe change this code to use something like:
> 
> let children = this._windowList.get_children().filter(function(c) { return
> c.visible; });
> let nChildren = children.length;
> if (nChildren == 0)
>   return this._windowList.get_preferred_width(-1);
> 
> [...]
> 
> return nChildren * childWidth + (nChildren - 1) * spacing;
> 

done.

> @@ +922,3 @@
> +
> +    _initIndicatorsBox: function() {
> +    },
> 
> I don't like this - the parent class should not know anything about what a
> subclass does. Please override _onDestroy/_getMaxWindowListWidth in
> PrimaryWindowList instead.
> @@ +1247,3 @@
> +
> +    _init: function(monitor, isMultiMonitor) {
> +        this.parent(monitor, isMultiMonitor);
> 
> This is a bit silly - "PrimaryWindowList" will (by definition) *always* use
> Main.layoutManager.primaryMonitor.
> I wonder if it would actually be easier to not subclass windowList at all and
> just test for (this._monitor.index == Main.layoutManager.primaryIndex) to
> decide whether to add the indicatorBox - I doubt it would make the code in
> WindowList much harder, however not having to differentiate between primary and
> secondary window lists in Extension should make the code quite a bit easier.

That's true, I went with a _isOnPrimaryMonitor property.

> @@ +913,3 @@
>      },
> 
> +    setMultiMonitor: function(multiMonitor) {
> 
> Could use a setter ("set multiMonitor(multiMonitor)", use like a normal
> property, e.g. "list.multiMonitor = true;")
> 
> @@ +1314,3 @@
> +                                                           
> this._isMultiMonitor);
> +        else
> +            this._primaryWindowList.setMultiMonitor(this._isMultiMonitor);
> 
> No. If the function has been called in response to
> LayoutManager::monitors-changed rather than GSettings::changed,
> primaryWindowList._monitor may no longer be the primary monitor.
> 

I did things a bit differently: on pref or monitors-changed event, I just rebuild all the window lists, which makes things simpler (because that situation should be rather rare, it shouldn't be too bad I guess).

> extensions/window-list/org.gnome.shell.extensions.window-list.gschema.xml.in
> @@ +16,3 @@
>        </_description>
>      </key>
> +    <key name="multi-monitor" type="b">
> 
> show-on-all-monitors maybe?

Yeah, that should be clearer.
Comment 15 Florian Müllner 2014-11-13 11:44:42 UTC
Review of attachment 290381 [details] [review]:

Grr, should have included this in 3.14.2 ...
Comment 16 Florian Müllner 2014-11-13 11:44:48 UTC
Review of attachment 290382 [details] [review]:

Dito.
Comment 17 Florian Müllner 2014-11-13 11:44:49 UTC
Review of attachment 290382 [details] [review]:

Dito.
Comment 18 Florian Müllner 2014-11-13 11:44:58 UTC
Review of attachment 290383 [details] [review]:

LGTM
Comment 19 Florian Müllner 2014-11-13 11:45:09 UTC
Review of attachment 290384 [details] [review]:

::: extensions/window-list/extension.js
@@ +842,3 @@
             }));
+
+        this._isOnBottomMonitor = Main.layoutManager.primaryIndex == Main.layoutManager.bottomIndex;

Would be nice to handle monitor changes here, but for now recreating the window lists altogether as done in a later patch should be good enough.
Comment 20 Florian Müllner 2014-11-13 11:45:19 UTC
Review of attachment 290385 [details] [review]:

Looks mostly good to me.

::: extensions/window-list/extension.js
@@ +189,3 @@
+const BaseButton = new Lang.Class({
+    Name: 'BaseButton',
+    Abstract: true,

There's quite a bit in the constructor that should be shared as well - namely creating this.actor (just leave out setting StButton:child to the subclasses) and connecting the ::allocation-changed, ::clicked, ::popup-menu and ::destroy signals. The latter fixes some ugliness in a later patch, where you define a destroy handler that subclasses are expected to chain up to, but set up the handlers in the subclasses.
Setting up the context menu can also be shared with some minor modifications.

@@ +203,3 @@
+
+    _updateStyle: function() {
+        if (this._isFocused())

I'd like some implementation of _isFocused in the base class, either "return false;" or "throw new Exception('Not implemented');"
Comment 21 Florian Müllner 2014-11-13 11:45:30 UTC
Review of attachment 290386 [details] [review]:

::: extensions/window-list/extension.js
@@ +847,2 @@
+            this._workspaceIndicator = new WorkspaceIndicator();
+            indicatorsBox.add(this._workspaceIndicator.container, {expand: false, y_fill: true});

Thinking about it, the workspace indicator does make some sense on secondary monitors if workspaces-only-on-primary is false. How about always adding the indicators box, but hide components as necessary (workspaces if not on primary monitor and workspaces-only-on-primary is true, trayButton if not on bottom monitor)

@@ +990,3 @@
+            }));
+        }
+        let nWindows = windows.length;

No. As mentioned in the previous review, nWindows can be 0 and you end up requesting -spacing. But this shouldn't be necessary anyway, no? If perMonitor is set, children that represent windows on other monitors are hidden, so you can kill of all that code and use "nChildren * childWidth + (nChildren - 1) * spacing" directly.

@@ +997,2 @@
     _getMaxWindowListWidth: function() {
+        var maxWindowListWidth = this.actor.width;

s/var/let/ (or if you follow the suggestion regarding workspaces/trayButton above, the function could be left as-is.

@@ +1292,3 @@
+        this._showOnAllMonitorsChangedId =
+            this._settings.connect('changed::show-on-all-monitors',
+                                   Lang.bind(this, this._buildWindowLists));

The brief flashing on the primary monitor when toggling the setting (due to the struct changes) is a bit ugly, but fine to leave that as a possible future enhancement.
Comment 22 Sylvain Pasche 2014-11-16 15:24:32 UTC
(In reply to comment #19)
> Review of attachment 290384 [details] [review]:
> 
> ::: extensions/window-list/extension.js
> @@ +842,3 @@
>              }));
> +
> +        this._isOnBottomMonitor = Main.layoutManager.primaryIndex ==
> Main.layoutManager.bottomIndex;
> 
> Would be nice to handle monitor changes here, but for now recreating the window
> lists altogether as done in a later patch should be good enough.

Oops, yes that case is not handled.
Comment 23 Sylvain Pasche 2014-11-16 15:25:09 UTC
Created attachment 290798 [details] [review]
window-list: Refactor {Window,App}Button shared code into BaseButton

(In reply to comment #20)
> Review of attachment 290385 [details] [review]:
> 
> Looks mostly good to me.
> 
> ::: extensions/window-list/extension.js
> @@ +189,3 @@
> +const BaseButton = new Lang.Class({
> +    Name: 'BaseButton',
> +    Abstract: true,
> 
> There's quite a bit in the constructor that should be shared as well - namely
> creating this.actor (just leave out setting StButton:child to the subclasses)
> and connecting the ::allocation-changed, ::clicked, ::popup-menu and ::destroy
> signals. The latter fixes some ugliness in a later patch, where you define a
> destroy handler that subclasses are expected to chain up to, but set up the
> handlers in the subclasses.
> Setting up the context menu can also be shared with some minor modifications.
> 

Done. I didn't find much to share for the context menu stuff, because AppButton does a few tricks with the context menu that is dynamically switched between AppContextMenu and WindowContextMenu.

> @@ +203,3 @@
> +
> +    _updateStyle: function() {
> +        if (this._isFocused())
> 
> I'd like some implementation of _isFocused in the base class, either "return
> false;" or "throw new Exception('Not implemented');"

I added implementations that raise an error for all methods that should be overridden.
Comment 24 Sylvain Pasche 2014-11-16 15:29:08 UTC
Created attachment 290800 [details] [review]
window-list: Option to show the window list on all monitors

(In reply to comment #21)
> Review of attachment 290386 [details] [review]:
> 
> ::: extensions/window-list/extension.js
> @@ +847,2 @@
> +            this._workspaceIndicator = new WorkspaceIndicator();
> +            indicatorsBox.add(this._workspaceIndicator.container, {expand:
> false, y_fill: true});
> 
> Thinking about it, the workspace indicator does make some sense on secondary
> monitors if workspaces-only-on-primary is false. How about always adding the
> indicators box, but hide components as necessary (workspaces if not on primary
> monitor and workspaces-only-on-primary is true, trayButton if not on bottom
> monitor)

Yeah, that sounds good. I'm using workspaces-only-on-primary set to false and didn't think of showing it on secondary monitors at first, but I realize it provides a quick way to switch workspace with the mouse and see on which workspace you are on.

> @@ +990,3 @@
> +            }));
> +        }
> +        let nWindows = windows.length;
> 
> No. As mentioned in the previous review, nWindows can be 0 and you end up
> requesting -spacing. But this shouldn't be necessary anyway, no? If perMonitor
> is set, children that represent windows on other monitors are hidden, so you
> can kill of all that code and use "nChildren * childWidth + (nChildren - 1) *
> spacing" directly.

Ok, now I understand what you meant. Only relying on the number of visible children may not do what's expected here:
- _getPreferredUngroupedWindowListWidth is called before the child is added to the _windowList, so we have one less child than expected when a window is added.
- when going from grouped to ungrouped, children are AppButtons so it doesn't compute what the width would be if there was only WindowButtons.

And I realize it brings nothing to filter the number of children according to their visibility, as we're only interested to know if it's empty or get a child for its preferred width. So I kept the old logic and added the empty window count check.

> @@ +997,2 @@
>      _getMaxWindowListWidth: function() {
> +        var maxWindowListWidth = this.actor.width;
> 
> s/var/let/ (or if you follow the suggestion regarding workspaces/trayButton
> above, the function could be left as-is.

ok.

> @@ +1292,3 @@
> +        this._showOnAllMonitorsChangedId =
> +            this._settings.connect('changed::show-on-all-monitors',
> +                                   Lang.bind(this, this._buildWindowLists));
> 
> The brief flashing on the primary monitor when toggling the setting (due to the
> struct changes) is a bit ugly, but fine to leave that as a possible future
> enhancement.

Ok, I'll open a follow-up bug.


I found a small issue during testing: with show-on-allmonitors active and auto grouping-mode, moving a window between monitors doesn't trigger a potential grouping mode switch. Are you ok if I handle it in a follow up bug?
Comment 25 Florian Müllner 2014-11-26 19:02:48 UTC
Ugh, sorry - I thought this had landed already; I'll push the a-c-n commits to clean up a bit, then review the remaining patches.

Attachment 290381 [details] pushed as 4a1f495 - window-list: Disconnect drag and drop handlers
Attachment 290382 [details] pushed as 57bfb94 - window-list: _pointerInTray was renamed to _pointerInNotification in bug 695800
Attachment 290383 [details] pushed as 5fc6644 - window-list: Refactoring to use an Extension object
Attachment 290384 [details] pushed as 5c2d13e - window-list: Move messageTray patching to the WindowList class
Comment 26 Florian Müllner 2014-11-26 19:54:05 UTC
Review of attachment 290798 [details] [review]:

OK, LGTM now
Comment 27 Florian Müllner 2014-11-26 19:54:17 UTC
Review of attachment 290800 [details] [review]:

(In reply to comment #24)
> Ok, now I understand what you meant. Only relying on the number of visible
> children may not do what's expected here:
> - _getPreferredUngroupedWindowListWidth is called before the child is added to
> the _windowList, so we have one less child than expected when a window is
> added.
> - when going from grouped to ungrouped, children are AppButtons so it doesn't
> compute what the width would be if there was only WindowButtons.

Ah, indeed - sorry for the confusion then. Looks mostly good to me now, except for some minor comments below:

::: extensions/window-list/extension.js
@@ +849,3 @@
         indicatorsBox.add(this._workspaceIndicator.container, { expand: false, y_fill: true });
 
+        this._settings = new Gio.Settings({ schema_id: 'org.gnome.shell.overrides' });

this._settings should be the primary settings object, e.g. the one that is owned by the module itself. this._workspacesSettings?

@@ +947,3 @@
         this._dndWindow = null;
 
+        this._extensionSettings = Convenience.getSettings();

Please keep this as this._settings, see above.

::: extensions/window-list/prefs.js
@@ +73,3 @@
         }
+
+        let check = new Gtk.CheckButton({ label: _("Show the window list on all monitors"),

I'd like some quick design feedback on the string - to me, mentioning the window list in the window list preferences feels like unnecessary duplication and I'd go with a simple "Show on all monitors". But maybe it's just me :-)
Comment 28 Sylvain Pasche 2014-12-01 18:08:17 UTC
Created attachment 291943 [details]
window-list preferences window with new checkbox

Thanks for the review, I'll update the patch after the UI review.

UI review:

The attached screenshot from the latest patch shows the new option on the bottom. See end of comment 27 about Florian's suggestion to use a shorter "Show on all monitors" label instead (which I agree might be better).
Comment 29 Florian Müllner 2014-12-19 14:08:46 UTC
(In reply to comment #28)
> I'll update the patch after the UI review.

Apparently neither of us managed to get that, so I hope you don't mind that I'll just push those patches (including the proposed changes), so they get included in 3.15.3.
Comment 30 Florian Müllner 2014-12-19 14:09:25 UTC
Created attachment 293058 [details] [review]
window-list: Do not hardcode overrides schema

Classic mode uses a different overrides schema, so make sure we use the
correct setting instead of hardcoding the usual org.gnome.shell.overrides
schema.
Comment 31 Florian Müllner 2014-12-19 14:19:33 UTC
Attachment 290798 [details] pushed as e80b790 - window-list: Refactor {Window,App}Button shared code into BaseButton
Attachment 290800 [details] pushed as a3f352d - window-list: Option to show the window list on all monitors
Attachment 293058 [details] pushed as 8b59c03 - window-list: Do not hardcode overrides schema
Comment 32 catscrash 2016-12-03 17:27:18 UTC
Hi,

at the moment it's only possible to show the task list on the primary monitor, or on all of them. It would be great if I could choose on which monitors it should show. I would e.g. like to say, don't show on 1, but show on 2 and 3.