Created attachment 232569 [details] [review]
messageTray: Add context menu with clear item

Add a context menu to the tray that clean's up the sources.

Review of attachment 232569 [details] [review]:

::: js/ui/messageTray.js
@@ +1582,3 @@
+    _buildRightClickMenu: function() {
+        let item;
+        let rightClickMenu = new St.BoxLayout({ name: 'summary-right-click-menu',

Yikes. We use a BoxLayout + BoxPointer combo right now for message tray items but that's because we have an existing BoxPointer. We really should be using a PopupMenu, and there's no reason you shouldn't be either.

Th context menu might be a good place to add a "Notification Settings" item too.
A menu with one item looks stupid anyway.

Created attachment 235615 [details] [review]
messageTray: Add context menu

Add a context menu to the tray that allows cleaning up sources
and open the notification settings panel.

(In reply to comment #2)
> Review of attachment 232569 [details] [review]:
> 
> ::: js/ui/messageTray.js
> @@ +1582,3 @@
> +    _buildRightClickMenu: function() {
> +        let item;
> +        let rightClickMenu = new St.BoxLayout({ name:
> 'summary-right-click-menu',
> 
> Yikes. We use a BoxLayout + BoxPointer combo right now for message tray items
> but that's because we have an existing BoxPointer. We really should be using a
> PopupMenu, and there's no reason you shouldn't be either.

Uses a PopupMenu now ... didn't change the api though my attempt ended up kind of messy so I went with the "dummy actor pattern" we use in the rest of the code.

Created attachment 235752 [details] [review]
messageTray: Add context menu

Add a context menu to the tray that allows cleaning up sources
and open the notification settings panel.


Added separator before the settings entry.

Review of attachment 235615 [details] [review]:

Mostly OK.

::: js/ui/messageTray.js
@@ +1480,3 @@
+
+        this.addAction(_("Notification Settings"), function () {
+            Util.spawn(['gnome-control-center', 'notifications']);

We should probably make a .desktop file for this.

@@ +1481,3 @@
+        this.addAction(_("Notification Settings"), function () {
+            Util.spawn(['gnome-control-center', 'notifications']);
+            tray.escapeTray();

_escapeTray is supposed to be an internal method to popping down the tray. I'd prefer to have a close() method that calls this._escapeTray.

Created attachment 235753 [details] [review]
messageTray: Add context menu

Add a context menu to the tray that allows cleaning up sources
and open the notification settings panel.

the control-center already installs gnome-notifications-panel.desktop

Created attachment 235754 [details] [review]
messageTray: Add context menu

Add a context menu to the tray that allows cleaning up sources
and open the notification settings panel.


* Add a public close() method instead of calling espaceTray
* Use the desktop file to launch the settings.

Review of attachment 235754 [details] [review]:

OK.

Attachment 235754 [details] pushed as 9133f6d - messageTray: Add context menu

May i ask/suggest, why not have a Context Menu on *any* click within a blank area of the Message Tray? 
Currently it seems to be limited to just right-click.

(In reply to comment #13)
> May i ask/suggest, why not have a Context Menu on *any* click within a blank
> area of the Message Tray? 
> Currently it seems to be limited to just right-click.

No this would be really annoying.

It could happen on long press, but I'm not sure that's the best idea.

I just had this thought thinking about how i would open the Context Menu on e.g. a Touchscreen, so just having it pop-up on any click/touch made sense to me.

Of course there's always the risk of accidental clicks, so yeah that could get pretty annoying if one tends to do that a lot.


Another thing i'd like to bring up is the Label of the clear action. I was wondering, if there is nothing to clear, should it even be visible? Or maybe it could be inactive with a Label like "Nothing to clear".

(In reply to comment #16)
> I just had this thought thinking about how i would open the Context Menu on
> e.g. a Touchscreen, so just having it pop-up on any click/touch made sense to
> me.
> 

We should do long press for that, feel free to file a new bug for that.

> 
> 
> Another thing i'd like to bring up is the Label of the clear action. I was
> wondering, if there is nothing to clear, should it even be visible? Or maybe it
> could be inactive with a Label like "Nothing to clear".

http://git.gnome.org/browse/gnome-shell/commit/?id=2138fc2349b27e5f943af236e71ece0512a7c513

*** Bug 651289 has been marked as a duplicate of this bug. ***