It's not only the notification position (which is relatively easy to change): we are hiding banners while the time+date drop-down is shown, but with banners on the right, other menus like the system one get in the way of banners just as much as the calendar drop-down.
I think this would be best addressed generically in gnome-shell proper, by setting the banner position according to the panel box (left, center, right) the dateMenu is in, and extend the hide-banner behavior to all menus which are in the same box.

Wouldn't placing notifications at the top right obscure the window close button?

(In reply to Jasper St. Pierre from comment #2)
> Wouldn't placing notifications at the top right obscure the window close
> button?

You could offset it from the screen edge? The clock isn't right in the corner, anyway.

Created attachment 301896 [details] [review]
Hide banners when any menu in box of time+date dropdown is open

Created attachment 301929 [details] [review]
Move notification banners below the time+date dropdown

Please provide better commit messages - a one-line subject which describes what the patch does is not enough, you should also outline the reason for the change[0]. You will likely find that the patches are in the wrong order then (the reason for moving banners is that there's a strong relationship with the calendar drop-down, which holds the message list; without that change, there is no reason for hiding banners when opening a menu in the same group)

[0] http://lists.cairographics.org/archives/cairo/2008-September/015092.html

Review of attachment 301896 [details] [review]:

::: js/ui/panel.js
@@ +980,3 @@
         let nChildren = box.get_n_children();
 
+        this._boxWithDateMenu = (elements.indexOf('dateMenu') != -1) ? box : this._boxWithDateMenu;

This does not handle the case of no dateMenu (e.g. by default the dateMenu is centered and hidden on the lock screen, however boxWithDateMenu will still point to the centerBox)

@@ +1014,3 @@
+            indicator.menu.connect('open-state-changed', Lang.bind(this, function(menu, isOpen) {
+                Main.messageTray.bannerBlocked = isOpen;
+            }));

The signal is never disconnected, so menus that *were* in the same group as the dateMenu will still block notification banners.

Review of attachment 301929 [details] [review]:

::: js/ui/messageTray.js
@@ +977,3 @@
+    // so that they appear below the dateMenu
+    updateAlignment: function(boxWithDateMenu) {
+        if (boxWithDateMenu == "panelLeft")

Ugh, no - actor names are used as element IDs in CSS and are useful for debugging, but don't treat them as public API. This should be something like setBannerAlignment(align), so you don't need any implementation details from panel.js or comments explaining what the function does.

Created attachment 302146 [details] [review]
messageTray: Move notification banners below the time+date dropdown to reinforce their relationship

Created attachment 302148 [details] [review]
messageTray: Move notification banners below the time+date dropdown to reinforce their relationship

Created attachment 302151 [details] [review]
panel: Hide banners when any menu in box of time+date dropdown is open

Created attachment 302152 [details] [review]
panel: Hide banners when any menu in box of time+date dropdown is open

https://bugzilla.gnome.org/show_bug.cgi?id=745910

panel: Hide banners when any menu in box of time+date dropdown is open

Review of attachment 302148 [details] [review]:

The commit message still needs improvement - I did not suggest to cramp everything into a super-long subject line (it should be limited to around 72 characters!), but to add some reasoning to the *body* of the commit message.

::: js/ui/messageTray.js
@@ +977,3 @@
     },
 
+    setBannerAlignment: function(align) {

Please pass in the alignment, not some string

::: js/ui/panel.js
@@ +980,3 @@
         let nChildren = box.get_n_children();
 
+        this._boxWithDateMenu = (elements.indexOf('dateMenu') != -1) ? box : this._boxWithDateMenu;

As noted in the previous review, this will not catch a removed dateMenu.

Review of attachment 302152 [details] [review]:

In addition to the error pointed out below, the commit message is missing a body

::: js/ui/panel.js
@@ +1013,3 @@
         let destroyId = indicator.connect('destroy', Lang.bind(this, function(emitter) {
+            if (blockBannerSignal)
+                indicator.menu.disconnect(blockBannerSignal);

This does not work for indicators that are hidden/moved rather than destroyed, namely *all* the built-in ones

(In reply to Florian Müllner from comment #13) 
> ::: js/ui/panel.js
> @@ +980,3 @@
>          let nChildren = box.get_n_children();
>  
> +        this._boxWithDateMenu = (elements.indexOf('dateMenu') != -1) ? box
> : this._boxWithDateMenu;
> 
> As noted in the previous review, this will not catch a removed dateMenu.

But if there's no dateMenu shouldn't the default alignment be Center?

(In reply to Meet from comment #15)
> But if there's no dateMenu shouldn't the default alignment be Center?

It should be *some* alignment, and 'center' looks like a reasonable pick.

(In reply to Florian Müllner from comment #16)
> (In reply to Meet from comment #15)
> > But if there's no dateMenu shouldn't the default alignment be Center?
> 
> It should be *some* alignment, and 'center' looks like a reasonable pick.

I guess you could make a point for the current behavior of "wherever the dateMenu used to be", but that would at least need a comment

(In reply to Florian Müllner from comment #14)
> Review of attachment 302152 [details] [review] [review]:
> 
> In addition to the error pointed out below, the commit message is missing a
> body
> 
> ::: js/ui/panel.js
> @@ +1013,3 @@
>          let destroyId = indicator.connect('destroy', Lang.bind(this,
> function(emitter) {
> +            if (blockBannerSignal)
> +                indicator.menu.disconnect(blockBannerSignal);
> 
> This does not work for indicators that are hidden/moved rather than
> destroyed, namely *all* the built-in ones

How are indicators moved? ( I cant find a function for that. If an indicator is hidden its state can never be changed so what is the purpose of disconnecting the signal?

(In reply to Meet from comment #18)
> > This does not work for indicators that are hidden/moved rather than
> > destroyed, namely *all* the built-in ones
> 
> How are indicators moved?

https://git.gnome.org/browse/gnome-shell/tree/js/ui/panel.js#n994


> If an indicator is hidden its state can never be changed so what is 
> the purpose of disconnecting the signal?

True, it just seems easier to remove the connection when the indicator is no longer in the same box as the dateMenu (including the "no box" case) instead of checking whether the indicator is still in the same box as the dateMenu the next time it is shown.

Some more thoughts on this:

Banners should be blocked when a menu is in the way, which is not necessarily the dateMenu box in the case where the dateMenu is missing. So you either need to handle that case when checking for the box, or lie and set _boxWithDateMenu to centerBox.
I think a nicer way than either option is to reflect the intention of the code, and set the banner alignment from position of the dateMenu (falling back to centering as you do now), then figure out whether a menu should block banners from that. Something like:

if (indicator._openChangedId > 0)
    indicator.menu.disconnect(indicator._openChangedId);
indicator._openChangedId = 0;

if (indicator.menu)
    indicator._openChangedId = indicator.menu.connect('open-state-changed',
        Lang.bind(this, function(menu, isOpen) {
            let boxes = { Clutter.ActorAlign.START: this._leftBox,
                          Clutter.ActorAlign.END: this._rightBox,
                          Clutter.ActorAlign.CENTER: this._centerBox };
            if (boxes[this._bannerAlignment] == box)
                Main.messageTray.bannerBlocked = isOpen;
        }));

Created attachment 302211 [details] [review]
Not working temp commit

It builds correctly but exits on execution with error :

(gnome-shell:23009): Gjs-CRITICAL **: JS ERROR: SyntaxError: missing : after property id @ resource:///org/gnome/shell/ui/panel.js:986
** Message: Execution of main.js threw exception: JS_EvaluateScript() failed

There is no missing ':'? Any help?

It's complaining about non-string property names in an object literal - this would work:

let alignments = {};
alignments[this._leftBox] = Clutter.ActorAlign.LEFT;
alignments[this._rightBox] = Clutter.ActorAlign.RIGHT;

I'd just use

if (panel.left.indexOf('dateMenu') != -1)
    this._bannerAlignment = Clutter.ActorAlign.LEFT;
else if (panel.right.indexOf('dateMenu') != -1)
    this._bannerAlignment = Clutter.ActorAlign.RIGHT;
else
    this._bannerAlignment = Clutter.ActorAlign.CENTER;
Main.messageTray.setBannerAlignment(this._bannerAlignment);

It's not really more code, and the fallback is in the right place ...

(Another option would be to use getters/setters in MessageTray, so that bannerAlignment appears as a public property:

get bannerAlignment() {
    return this._bannerBin.get_x_align();
},

set bannerAlignment(align) {
    this._bannerBin.set_x_align(align);
}

Then you would not have to keep a separate _bannerAlignment property in panel ...

Created attachment 302232 [details] [review]
Move notification banners below time+date dropdown

This enforces the relationship between the two.

Created attachment 302234 [details] [review]
Move notification banners below time+date dropdown

This reinforces the relationship between the two.

(In reply to Florian Müllner from comment #20)
> if (indicator.menu)
>     indicator._openChangedId = indicator.menu.connect('open-state-changed',
>         Lang.bind(this, function(menu, isOpen) {
>             let boxes = { Clutter.ActorAlign.START: this._leftBox,
>                           Clutter.ActorAlign.END: this._rightBox,
>                           Clutter.ActorAlign.CENTER: this._centerBox };
>             if (boxes[this._bannerAlignment] == box)
>                 Main.messageTray.bannerBlocked = isOpen;
>         }));

Wouldnt this not work due to the same reason as above?

(In reply to Florian Müllner from comment #20)

> if (indicator._openChangedId > 0)
>     indicator.menu.disconnect(indicator._openChangedId);
> indicator._openChangedId = 0;

What is indicator._openChangedId ?

Created attachment 302242 [details] [review]
Hide banners when notification banners and opened menus may have overlapped

(In reply to Meet from comment #27)
> (In reply to Florian Müllner from comment #20)
> 
> > if (indicator._openChangedId > 0)
> >     indicator.menu.disconnect(indicator._openChangedId);
> > indicator._openChangedId = 0;
> 
> What is indicator._openChangedId ?

I understood it.

Review of attachment 302234 [details] [review]:

Subject of the commit message should be prefixed, otherwise looks good to me - thanks!

Review of attachment 302242 [details] [review]:

Missing prefix in commit message, otherwise fine

Should I reword the commits with respective prefixes messageTray and panel? I did not add prefix as multiple components were being modified.

Created attachment 302326 [details] [review]
panel: Move notification banners below time+date dropdown

Reattaching patches with beefed-up commit messages - the changes in messageTray/dateMenu are only secondary to the ones in panel, so the latter is the right prefix.

Created attachment 302327 [details] [review]
panel: Block banners when opening menus that would overlap

Dto.

Attachment 302326 [details] pushed as 674325e - panel: Move notification banners below time+date dropdown
Attachment 302327 [details] pushed as 08690d6 - panel: Block banners when opening menus that would overlap

I pushed this to gnome-3-16 as well.

Ok. Read the new commit messages as well and learnt how to type commit messages.

Created attachment 302644 [details] [review]
panel: Set up 'open-state-changed' handler on menu changes

Commit 08690d658f42 generalized the banner-blocking behavior of the
dateMenu to all menus that would obscure the banner. However setting
up the 'open-state-changed' handler only when an indicator is added
does not work for indicators that change their entire menu (like the
app menu) - we currently end up with menus with no connected signal
handler, and throw an error when trying to disconnect an invalid
signal ID.
To address this, add a new PanelButton::menu-set signal and use that
to set up the 'open-state-changed' handler.

Review of attachment 302644 [details] [review]:

::: js/ui/panel.js
@@ +1060,3 @@
+
+        if (indicator.menu._openChangedId)
+            indicator.menu.disconnect(indicator.menu._openChangedId);

If indicator.menu gets replaced by a different instance we never execute this. Returning the previous menu instance as an argument to the menu-set signal would allow us do disconnect here

(In reply to Rui Matos from comment #39)
> Review of attachment 302644 [details] [review] [review]:
> If indicator.menu gets replaced by a different instance we never execute
> this. Returning the previous menu instance as an argument to the menu-set
> signal would allow us do disconnect here

If there was a previous menu instance, setMenu() destroys it before emitting ::menu-set. I think we could just as well not disconnect the signal at all, and use
  if (!indicator.menu || indicator.menu._openChangedId > 0)
      return;
to avoid multiple connections in case the menu did not change. Or just not care about multiple connections now that we figure out the panel box in the signal handler, dunno - we already end up with multiple connections to the ::destroy and ::menu-set signals ...

(In reply to Florian Müllner from comment #40)
> If there was a previous menu instance, setMenu() destroys it before emitting
> ::menu-set.

We could emit the signal before destroying the old instance but I don't think it's worth it.

> I think we could just as well not disconnect the signal at all,
> and use
>   if (!indicator.menu || indicator.menu._openChangedId > 0)
>       return;

Sounds good to me, feel free to push with that.

Attachment 302644 [details] pushed as 1092f55 - panel: Set up 'open-state-changed' handler on menu changes