Created attachment 181467 [details] [review]
Factor out hot corner handling into a class

This is needed so that we can have several instances, one per
monitor.

Created attachment 181468 [details] [review]
No need to Check for grabbed menus on hot corner enter/click

We never get enter events anyway due to the menu code, and if
the user clicks on a non-menu the menu is removed and ungrabbed
before the target actor gets the event anyway.

Created attachment 181469 [details] [review]
Remove not used reference from HotCorner to panel

Created attachment 181470 [details] [review]
Move HotCorner to chrome instead of in the panel

This prepares for there being multiple hot corners, one per monitor.

Created attachment 181471 [details] [review]
Put a hot corner on each monitor

Created attachment 181472 [details] [review]
Remove unused tracking of visibility in Chrome

Created attachment 181473 [details] [review]
Track fullscreen status per-monitor in Chrome

This is required since we can have chrome on multiple monitors (due to
per-monitor hot-corners).

Created attachment 181474 [details] [review]
Consider struts top/botton or left/right based on primary monitor size

Right now we require a strut to be as wide as the full screen to
create a TOP strut. This means that in a multi-monitor scenario (at
least if the primary monitor is leftmost) we will make the panel strut
be Meta.Side.LEFT due to the random side picking for corner objects.

This changes the width/height comparison to the primary monitor rather
than the screen to get this right. Also adds some docs about how
struts work in a multi-monitor situation.

Review of attachment 181467 [details] [review]:

::: js/ui/panel.js
@@ +507,3 @@
+        // hot corner and has not left both the hot corner and a surrounding
+        // guard area (the "environs"). This avoids triggering the hot corner
+        // multiple times due to an accidental jitter.

Comment got divorced from the variable that it was referring to

@@ +556,3 @@
+    },
+
+    _allocate: function (parentWidth, direction, flags) {

underscore prefix means class-private not module/file-private.

I don't like having part of the allocate() method of the parent coontainer here - we'd generally like have a JS class as corresponding to an actor - not a mishmash of a couple of actors and part of the implementation of the parent container.

The cleanest thing to do would be if you can change thing so this._environs is a container for this._corner and gets renamed to this.actor. For our purposes can just assume St.Widget.get_default_direction() won't change and statically position the._corner with the environs at a fixed location using that knowledge. Then the parent would be responsible for positioning hotCorner.actor in the appropriate corner of the screen.

@@ +660,3 @@
+    // of the hot corner being triggered. This check avoids opening and closing the overview if the user both triggered the hot corner
+    // and clicked the Activities button.
+    _maybeToggleOverviewOnClick: function() {

* No _ prefix, since it's not class private
* I'd rather if the overview.toggle() was in the caller - so maybe something like:

 if (!this._hotCorner.recentlyActivated())
   Main.overview.toggle();
 this._hotCorner.resetActivationTime(); // always activate on a second click

I don't think logic for handling clicks on the Activities button should be in the hot corner class.

Review of attachment 181468 [details] [review]:

Looks OK - menu handling has changed a lot since that check was added.

Review of attachment 181469 [details] [review]:

Fine

Re _allocate: The first patch mainly splits out the current code thats in
Panel, later commits starts changing the behaviour, including making it not be
allocated as a child of the panel.

Such mini-commits is the style i'm used to, but maybe thats not ideal for
reviewing like this?

Anyway, the containment and actor rename change makes sense too, I'll have more
feedback tomorrow.

Review of attachment 181470 [details] [review]:

OK

::: js/ui/panel.js
@@ +555,3 @@
     },
 
+    position: function(parentBox, direction) {

Don't pass in the direction, just use:

 let rtl = St.Widget.get_default_direction().get_direction() == St.TextDirection.RTL;

@@ +560,3 @@
+            x += parentBox.width - this._environs.width;
+        } else {
+        }

Find this empty else weird rather than informative

@@ +771,3 @@
         this._leftBox.add(this.button);
 
+        this._hotCorner = new HotCorner();

I'd probably have moved tihs directly to main.js in this patch - rather than having main.js access a private variable, but an ok micro-commit split.

Review of attachment 181471 [details] [review]:

Good except for cosmetics

::: js/ui/main.js
@@ +391,3 @@
+        for (let i = 0; i < hotCorners.length; i++) {
+            hotCorners[i].destroy();
+        }

We generally avoid unnecessary {}

@@ +409,3 @@
+        if (is_primary) {
+            panel.setHotCorner(corner);
+        }

More unnecessary {} (saw some in the previous patch too)

::: js/ui/panel.js
@@ +847,3 @@
     },
 
+    setHotCorner: function(corner) {

Would like a little bit of a comment indicating the significance of this - something like

 // While there can be multiple hotcorners (one per monitor), the hot corner
 // that is on top of the Activities button is special since it needs special
 // coordination with clicking on that button

Review of attachment 181472 [details] [review]:

Dan said that he though this was probably left-over cruft in the patch when initially committed

Review of attachment 181473 [details] [review]:

General idea looks fine, but reads somewhat messy to me - various comments below.

::: js/ui/chrome.js
@@ +217,3 @@
+        this._monitors = []
+        for (let i = 0; i < monitors.length; i++) {
+            // Create a plain js object so we can extend it nicely

Hmm, I guess it's a style question - there's really no problem with "expando" adding properties to a Meta.Rectangle - whether that's "nice" or not is harder to say. I guess I'd consider it nice enough and just go ahead and do it and skip the 4 lines of code.

@@ +218,3 @@
+        for (let i = 0; i < monitors.length; i++) {
+            // Create a plain js object so we can extend it nicely
+            let monitor = {x: monitors[i].x,

Missing space before 'x' if you stick with creating a pure JS object

@@ +228,3 @@
+                monitor.height == primary.height) {
+                this._primaryMonitor = monitor;
+            }

unnecessary {}

@@ +232,3 @@
+    },
+
+    _findMonitorAt: function(pos) {

Is the duck-typing of having this function take a 'pos' and calling it with a window with x/y properties intentional or accidental? Some people might consider it good weakly typed language practice, but I think it would be less confusing with (x, y) if you don't always want it to take a window. And if it always took a window, findMonitorForWindow() would make more sense.

@@ +234,3 @@
+    _findMonitorAt: function(pos) {
+        let x = Math.max(pos.x, 0);
+        let y = Math.max(pos.y, 0);

unclear why you max() - we still have the possibility that the position isn't on any monitor, right?

@@ +237,3 @@
+        for (let i = 0; i < this._monitors.length; i++) {
+            let monitor = this._monitors[i];
+            // We consider the window origin to define which monitor a window is on

Center would be more robust right? if the center is on a monitor or on the next monitor over by a pixel, it isn't very obvious to the user if we get it wrong. Is there some reason that this works better?

This comment seems misplaced in any case

@@ +241,3 @@
+                y >= monitor.y && y < monitor.y + monitor.height) {
+                return monitor;
+            }

and here

@@ +248,3 @@
+    _findMonitorForActor: function(actor) {
+        let [x, y] = actor.get_transformed_position();
+        return this._findMonitorAt({x: Math.round(x), y: Math.round(y)})

rounding is unclear why - I think it' sbecause if get_transformed_position() returned 1023.999 or something you want that to be on the monitor at position 1024 ... probably could use a comment, and maybe should be inside findMonitorAt?

@@ +272,3 @@
+        for (let i = 0; i < this._monitors.length; i++) {
+            this._monitors[i].inFullscreen = false;
+        }

{}

@@ +300,3 @@
+                let monitor = this._findMonitorAt(windows[i]);
+                if (Math.max(windows[i].x, 0) >= monitor.x && Math.max(windows[i].x, 0) < monitor.x + monitor.width &&
+                    Math.max(windows[i].y, 0) >= monitor.y && Math.max(windows[i].y, 0) < monitor.y + monitor.height) {

this looks like it should be some monitorIsAt() function - it's closely related to _findMonitorAt() so having inlined is weird

@@ +310,1 @@
                     windows[i].y <= primary.y &&

leftover primary? how does this not error?

@@ +321,3 @@
+        for (let i = 0; i < this._monitors.length; i++) {
+            wasInFullscreen[i] = this._monitors[i].inFullscreen;
+        }

More {}

Created attachment 181714 [details] [review]
Factor out hot corner handling into a class

This is needed so that we can have several instances, one per
monitor.

This also makes the hot corner environments a container which
contains the corner.

Created attachment 181715 [details] [review]
Move HotCorner to chrome instead of in the panel

This prepares for there being multiple hot corners, one per monitor.

Created attachment 181716 [details] [review]
Don't rely on HotCorner method to toggle overview mode in panel

Instead of using an internal HotCorner method we expose two new
public methods: recentlyActivated and resetActivationTime so that
the panel can check the state of the hot corner instead of relying
on its code.

Created attachment 181717 [details] [review]
Put a hot corner on each monitor

Created attachment 181718 [details] [review]
Track fullscreen status per-monitor in Chrome

This is required since we can have chrome on multiple monitors (due to
per-monitor hot-corners).

Windows are primary associated with the monitor that their center is on.

Created attachment 181719 [details] [review]
Consider struts top/botton or left/right based on primary monitor size

Right now we require a strut to be as wide as the full screen to
create a TOP strut. This means that in a multi-monitor scenario (at
least if the primary monitor is leftmost) we will make the panel strut
be Meta.Side.LEFT due to the random side picking for corner objects.

This changes the width/height comparison to the primary monitor rather
than the screen to get this right. Also adds some docs about how
struts work in a multi-monitor situation.

New series, slightly rearranged, fixes everything pointed out by current reviews.

Review of attachment 181716 [details] [review]:

::: js/ui/panel.js
@@ +798,1 @@
     _maybeToggleOverviewOnClick: function() {

Hmm, didn't actually notice that this function was used both internally and externally because we had the clicking-on-the-hot-corner issue to deal with. I probably would have just recommended removing the _ if I had - it's better to have a function with weird side effects than to duplicate some rather complex logic. Up to you if you want to go with this patch or revert it and just make the function non-private.

Review of attachment 181714 [details] [review]:

Looks good

Review of attachment 181715 [details] [review]:

Looks good

Review of attachment 181718 [details] [review]:

Looks fine except for one bug. OK to commit with that fixed.

::: js/ui/chrome.js
@@ +196,1 @@
                 this.actor.set_skip_paint(actorData.actor, true);

If we actually had actors on multiple monitors, then this wouldn't really work, but nothing really would - we are trying to get the illusion that the fullscreen window is above the chrome actors by hiding them when there is a fullscreen window. Treating actors and windows seems fine.

@@ +217,3 @@
+        this._monitors = monitors;
+        for (let i = 0; i < monitors.length; i++) {
+            let monitor = monitors[i].x;

Stray .x

@@ +303,3 @@
+                let monitor = this._findMonitorForWindow(window);
+                if (monitor)
+                    monitor.inFullscreen = true;

Hmm, this probably doesn't interact right with the newer net-wm spec ability to fullscreen across multiple monitors (_NET_WM_FULLSCREEN_MONITORS or something), but we'll wait until someone complains; or maybe file a bug about it.

Review of attachment 181717 [details] [review]:

One style fix, otherwise good to commit

::: js/ui/main.js
@@ +406,3 @@
+        let monitor = monitors[i];
+        let corner = hotCorners[i];
+        let is_primary = (monitor.x == primary.x &&

isPrimary

Review of attachment 181719 [details] [review]:

::: js/ui/chrome.js
@@ +382,3 @@
             // we don't create a strut for it at all.
             let side;
+            if (w >= this._primaryMonitor.width) {

I think this would be better to make into a "spans primary monitor horizontally" check.

Do you think it make sense to try to handle the case where the primary monitor isn't the full screen height by increasing the height of the strut by screen_height - monitor.height?

Review of attachment 181716 [details] [review]:

::: js/ui/panel.js
@@ +798,1 @@
     _maybeToggleOverviewOnClick: function() {

I'll drop it, i made this a separate patch at the end to make that easy to do.

Attachment 181714 [details] pushed as 196d104 - Factor out hot corner handling into a class
Attachment 181715 [details] pushed as 259c84e - Move HotCorner to chrome instead of in the panel
Attachment 181717 [details] pushed as 7cf311d - Put a hot corner on each monitor
Attachment 181718 [details] pushed as 885f668 - Track fullscreen status per-monitor in Chrome

Created attachment 181815 [details] [review]
Fix positioning of panel on startup

We need to do the initial relayout before we start up the startup
animation, and we the startup animation can't hardcode the position
of the panel to zero.

Created attachment 181816 [details] [review]
Consider struts top/botton or left/right based on primary monitor size

Right now we require a strut to be as wide as the full screen to
create a TOP strut. This means that in a multi-monitor scenario (at
least if the primary monitor is leftmost) we will make the panel strut
be Meta.Side.LEFT due to the random side picking for corner objects.

This changes the width/height comparison to the primary monitor rather
than the screen to get this right. Also adds some docs about how
struts work in a multi-monitor situation.

Created attachment 181817 [details] [review]
Ensure that all struts we set extends to their boundary

Mutter really expects this, as this is how app-specified struts
happen. For instance, if the primary display is beside a taller
screen and is not positioned at the top, then we extend the struts
for the panel with the size of the unused area above the primary
monitor.

Review of attachment 181815 [details] [review]:

Typo in the commit message 'and we the startup animation'. Otherwise looks fine. 

(clearly the animation won't work for vertically stacked monitors with the panel not at the top, but that's a tiny tip of the iceberg of problems we have in that scenario.)

Can you close bug 625924 after pushing this?

Review of attachment 181816 [details] [review]:

Looks good to me

Review of attachment 181817 [details] [review]:

Good (probably mutter should be doing this itself internally, but not really worth worrying about at the moment.)