A couple more details.

The animation for showing and hiding the expanded view might work like the popup shown over the Download item on:
http://www.panic.com/coda/

Also, we should make an effort to animate between expanded popups when switching between sources.

Created attachment 163959 [details] [review]
Show source title on hover, notification on click in the message tray

This is part of the design update for the message tray.

Source now takes an extra argument called 'title'.

All expanded message tray items are same width, which is determined by
the width of the item with the longest title, up to MAX_SOURCE_TITLE_WIDTH.
This is done so that items don't move around too much when one is expanded
and another one is collapsed.

Moving between different message tray items and getting them to expand is
still kind of fidgety (e.g. it's easy to skip over an item), so I will try
adding some sort of timeouts in a follow-up patch.

Review of attachment 163959 [details] [review]:

This looks like a nice change to look forward to. Apart from the minor hover issues you already pointed out yourself, I also don't quite like the style of the MessageTrayItems - but hey, that's Jon's job anyway. The comments below are mostly stylistic - in fact, most are direct or indirect complaints about line length ;)

There are two (probably) tricky issues though, which I think should be fixed:
(1) The titles set in notificationDaemon are not suitable for display
(2) When clicking a second item while the summary notification is popped out, the notification is closed (just as if the corresponding item had been clicked) - I would expect the second item's notification to pop out and replace the first notification.

::: js/ui/messageTray.js
@@ +401,2 @@
 Signals.addSignalMethods(Source.prototype);
+function MessageTrayItem(source, minTitleWidth) {

Will this be used outside the summary view? If not, does SummaryItem make more sense here?

@@ +436,3 @@
+
+    getTitleNaturalWidth: function() {
+        let [sourceTitleBinMinWidth, sourceTitleBinNaturalWidth] = this._sourceTitleBin.get_preferred_width(-1);

I would suggest [minWidth, naturalWidth] - I find differences in the middle of variable names hard to parse, and the meaning is perfectly clear without the prefixes. Also, very long lines don't work well with splinter - e.g. with the line above, I only get the left part of the assignment, which means that I have to switch between editor and browser :(

@@ +448,3 @@
+        let [sourceIconMinWidth, sourceIconNaturalWidth] = this._sourceIcon.get_preferred_width(forHeight);
+        let [sourceTitleBinMinWidth, sourceTitleBinNaturalWidth] = this._sourceTitleBin.get_preferred_width(forHeight);
+        let minWidth = sourceIconNaturalWidth + (found && this._widthFraction > 0 ? spacing : 0) + this._widthFraction * Math.min(Math.max(sourceTitleBinNaturalWidth, this._minTitleWidth), MAX_SOURCE_TITLE_WIDTH);

First: this line needs some breaking up - I suggest a line break at each '+', which should make the expression much easier to understand (and show up in splinter - right now, about 2/3 are off-screen!)

Then I'm not sure why Math.max(naturalTitleWidth, this._minTitleWidth) is needed - if I understand correctly, _minTitleWidth is synchronized between the Items to always refer to the longest title width, so the max is always this._minTitleWidth, right?

@@ +477,3 @@
+
+        let titleBox = new Clutter.ActorBox();
+        if (width > sourceIconNaturalWidth + (found ? spacing : 0)) {

I'd suggest doing
  if (!found)
     spacing = 0;
above to make the following expressions a little bit more readable.

@@ +513,3 @@
+        // This replaces some text with the dots at the end of the animation, and then we replace the dots with
+        // the text before we begin the animation to collapse the title. These changes are not noticeable at the
+        // speed with which we do the animation, while annimating in the ellipsized mode does not look good.

This comment makes more sense above where the function is called - it's not about what the function does, but to explain why it is called at the end of the animation rather than the start.

Also typo: annimating

@@ +598,3 @@
             }));
+        this._longestMessageTrayItem;
+        this._messageTrayItems = {};

= null?

@@ +644,2 @@
         source.connect('notify', Lang.bind(this, this._onNotify));
+        messageTrayItem.actor.connect('notify::hover', Lang.bind(this,

This could be done just fine inside MessageTrayItem itself (this.actor.connect(...))

::: js/ui/notificationDaemon.js
@@ +158,3 @@
         if (source == null) {
         // been acknowledged.
+            source = new Source(this._sourceId(appName), appName, icon, hints);

appName doesn't usually contain a string we want to show users - e.g. evolution uses something like 'evolution-mail-notification'.
We probably want the 'Name' field from the corresponding desktop entry - not exactly sure how to do that though.

::: js/ui/telepathyClient.js
@@ -462,2 @@
         this._targetId = targetId;
-        this.name = this._targetId;

This is no longer necessary now?

(In reply to comment #3)
> Review of attachment 163959 [details] [review]:
> 
> This looks like a nice change to look forward to. Apart from the minor hover
> issues you already pointed out yourself, I also don't quite like the style of
> the MessageTrayItems - but hey, that's Jon's job anyway.

Thanks for the review! The mockup is not graphical, so there will likely be a style update.
 
> There are two (probably) tricky issues though, which I think should be fixed:
> (1) The titles set in notificationDaemon are not suitable for display

> ::: js/ui/notificationDaemon.js
> @@ +158,3 @@
>          if (source == null) {
>          // been acknowledged.
> +            source = new Source(this._sourceId(appName), appName, icon,
> hints);
> 
> appName doesn't usually contain a string we want to show users - e.g. evolution
> uses something like 'evolution-mail-notification'.
> We probably want the 'Name' field from the corresponding desktop entry - not
> exactly sure how to do that though.

It seems suitable in most cases and the notification spec encourages using a formal (if optional) name.

http://people.canonical.com/~agateau/notifications-1.1/spec/ar01s02.html
'This is the optional name of the application sending the notification. This should be the application's formal name, rather than some sort of ID. An example would be "FredApp E-Mail Client," rather than "fredapp-email-client." '

So we should be able to get the applications to change it.

We are also getting the application based on the process id of the application sender. However, right now it only works for the applications that have open windows (e.g. it won't work for Rhythmbox) and it is asynchronous. You can try to see if it would fix the 'evolution-mail-notification' title problem by printing out app.get_name() in Source::setApp() in notificationDaemon.js If it fixes that problem, it might be worthwhile to add a fix for that in a separate patch. It would mean having to work around the busProxy.GetConnectionUnixProcessIDRemote() call being asynchronous.


> (2) When clicking a second item while the summary notification is popped out,
> the notification is closed (just as if the corresponding item had been clicked)
> - I would expect the second item's notification to pop out and replace the
> first notification.

Good catch! I had this working, but apparently broke it with a last minute fix to unset this._clickedMessageTrayItem when we are hiding the notification, so that it doesn't reappear when the summary is shown again. We should only unset it if this._summaryState != State.SHOWN and the updated patch has that fix.

> 
> ::: js/ui/messageTray.js
> @@ +401,2 @@
>  Signals.addSignalMethods(Source.prototype);
> +function MessageTrayItem(source, minTitleWidth) {
> 
> Will this be used outside the summary view? If not, does SummaryItem make more
> sense here?

No, it won't be used outside the summary view. I changed the name to SummaryItem.

> 
> @@ +436,3 @@
> +
> +    getTitleNaturalWidth: function() {
> +        let [sourceTitleBinMinWidth, sourceTitleBinNaturalWidth] =
> this._sourceTitleBin.get_preferred_width(-1);
> 
> I would suggest [minWidth, naturalWidth] - I find differences in the middle of
> variable names hard to parse, and the meaning is perfectly clear without the
> prefixes. Also, very long lines don't work well with splinter - e.g. with the
> line above, I only get the left part of the assignment, which means that I have
> to switch between editor and browser :(

I also call get_preferred_width() for this._sourceTitleBin in _getPreferredWidth() just below, and there are two different calls to get_preferred_width() there, so I can't just call the variables minWidth and naturalWidth. I kept the original name for consistency and split the line.

> 
> @@ +448,3 @@
> +        let [sourceIconMinWidth, sourceIconNaturalWidth] =
> this._sourceIcon.get_preferred_width(forHeight);
> +        let [sourceTitleBinMinWidth, sourceTitleBinNaturalWidth] =
> this._sourceTitleBin.get_preferred_width(forHeight);
> +        let minWidth = sourceIconNaturalWidth + (found && this._widthFraction
> > 0 ? spacing : 0) + this._widthFraction *
> Math.min(Math.max(sourceTitleBinNaturalWidth, this._minTitleWidth),
> MAX_SOURCE_TITLE_WIDTH);
> 
> First: this line needs some breaking up - I suggest a line break at each '+',
> which should make the expression much easier to understand (and show up in
> splinter - right now, about 2/3 are off-screen!)

Done.

> 
> Then I'm not sure why Math.max(naturalTitleWidth, this._minTitleWidth) is
> needed - if I understand correctly, _minTitleWidth is synchronized between the
> Items to always refer to the longest title width, so the max is always
> this._minTitleWidth, right?


If the new title width is greater than the prior title widths, this._minTitleWidth only gets set to it when setMinTitleWidth() is called. I was surprised to discover that _getPreferredWidth() doesn't get called when we add the item to the stage with this._summary.insert_actor(summaryItem.actor, 0), but only gets called later, after the right this._minTitleWidth is set. So it seems that your suggestion would work now, but there still can be a scenario when Math.max(naturalTitleWidth, this._minTitleWidth) is needed there.
 
> @@ +644,2 @@
>          source.connect('notify', Lang.bind(this, this._onNotify));
> +        messageTrayItem.actor.connect('notify::hover', Lang.bind(this,
> 
> This could be done just fine inside MessageTrayItem itself
> (this.actor.connect(...))
> 

But we are connecting it to call a MessageTray function - this._onSummaryItemHoverChanged(summaryItem) . Seems to be more convenient to do that in the MessageTray code.

> 
> ::: js/ui/telepathyClient.js
> @@ -462,2 @@
>          this._targetId = targetId;
> -        this.name = this._targetId;
> 
> This is no longer necessary now?

No, we are using this.title instead.

Created attachment 164372 [details] [review]
Show source title on hover, notification on click in the message tray

This is part of the design update for the message tray.

Source now takes an extra argument called 'title'.

All expanded message tray items are same width, which is determined by
the width of the item with the longest title, up to MAX_SOURCE_TITLE_WIDTH.
This is done so that items don't move around too much when one is expanded
and another one is collapsed.

(In reply to comment #4)
> Thanks for the review! The mockup is not graphical, so there will likely be a
> style update.

Yeah, that's what I meant with "Jon's job" :)


> 'This is the optional name of the application sending the notification. This
> should be the application's formal name, rather than some sort of ID. An
> example would be "FredApp E-Mail Client," rather than "fredapp-email-client." '
> 
> So we should be able to get the applications to change it.


Sounds good. Meanwhile we could have a simple mapping for popular offenders, e.g.

appNameMap = {
  'fredapp-email-client': "FredApp E-Mail Client"
};
appName = appNameMap[appName] || appName;

Not sure if it's a smart thing to do though, as it gives authors less incentive to fix their apps.


> But we are connecting it to call a MessageTray function -
> this._onSummaryItemHoverChanged(summaryItem) . Seems to be more convenient to
> do that in the MessageTray code.

I meant that the MessageTray function only calls SummaryItem code, so the function itself could be implemented in SummaryItem - the question is whether we consider expand-on-hover a behavior of SummaryItem or a behavior of MessageTray (to be clear: I think both are perfectly valid answers).


> > ::: js/ui/telepathyClient.js
> > @@ -462,2 @@
> >          this._targetId = targetId;
> > -        this.name = this._targetId;
> > 
> > This is no longer necessary now?
> 
> No, we are using this.title instead.


What I meant is that this.name is not replaced with this.title here - the old code suggests that RequestAliasesRemote might not return an alias while the new code relies on the condition "aliases && aliases.length" to never be false.

(In reply to comment #6)
> (In reply to comment #4)

> Sounds good. Meanwhile we could have a simple mapping for popular offenders,
> e.g.
> 
> appNameMap = {
>   'fredapp-email-client': "FredApp E-Mail Client"
> };
> appName = appNameMap[appName] || appName;

Ok, I added that to the patch.

> 
> Not sure if it's a smart thing to do though, as it gives authors less incentive
> to fix their apps.

I don't think it will affect the likelihood of authors fixing their apps, as long as we communicate this to the authors.
 
> 
> > But we are connecting it to call a MessageTray function -
> > this._onSummaryItemHoverChanged(summaryItem) . Seems to be more convenient to
> > do that in the MessageTray code.
> 
> I meant that the MessageTray function only calls SummaryItem code, so the
> function itself could be implemented in SummaryItem - the question is whether
> we consider expand-on-hover a behavior of SummaryItem or a behavior of
> MessageTray (to be clear: I think both are perfectly valid answers).

I see. I think both ways would be fine, but since the 'clicked' callback is MessageTray specific, it makes sense to keep them in one place. Also, the SummaryItem can potentially expand on some other action.

> 
> 
> > > ::: js/ui/telepathyClient.js
> > > @@ -462,2 @@
> > >          this._targetId = targetId;
> > > -        this.name = this._targetId;
> > > 
> > > This is no longer necessary now?
> > 
> > No, we are using this.title instead.
> 
> 
> What I meant is that this.name is not replaced with this.title here - the old
> code suggests that RequestAliasesRemote might not return an alias while the new
> code relies on the condition "aliases && aliases.length" to never be false.

I see. We pass targetId to the Source constructor, so this.title gets set to targetId there and we don't need this line.

Comment on attachment 164372 [details] [review]
Show source title on hover, notification on click in the message tray

Committed with changes discussed in the comments.

After trying the implementation of this design update (from master/HEAD), for a while I was not able to kill notifications - clicking on their summary icon reopened the full notification instead, and clicking again closed it.

Only after a while I discovered that clicking on the icon inside the notification closed it (and that this worked also before it was sent to the summary area)

Therefore I propose to add an X button to all notifications (similar to the notification-daemon one), so that users can close theme rapidly.

(In reply to comment #9)
> Only after a while I discovered that clicking on the icon inside the
> notification closed it (and that this worked also before it was sent to the
> summary area)

This is related to bug 613932.

Created attachment 164569 [details] [review]
Grab focus in the summary notification

Because clicking on the summary item to have it display its notification
is a more deliberate action than hovering, we can now grab focus in that
notification. This makes chat notifications in the summary more convenient
to use because you don't need to click on the text entry there.

One other idea that came up during debugging of this patch is to remove
the cursor from the search entry box in the overview when something else,
other than the stage, has the focus. The examples are the run dialog, the
looking glass, and a chat notification. I'll implement that as a separate
patch.

(In reply to comment #11)
> Because clicking on the summary item to have it display its notification
> is a more deliberate action than hovering, we can now grab focus in that
> notification. This makes chat notifications in the summary more convenient
> to use because you don't need to click on the text entry there.

Is there anything else apart from setting focus on the text entry for chat notifications that you want to achieve? If not, then this seems like an overly complicated approach. Intercepting all events and manually passing hover/click events to some parts is extremely painful - I did it for the search entry and it still hurts! The difference is that the design called for find-as-you-type style behavior there - not setting the key-focus after clicking a button. So unless I'm overlooking something subtle here, why don't you add an empty Notification.activate() method in messageTray and call it when the summary item is clicked, then overwrite the method in telepathyClient to call set_key_focus() on the entry? Or is there a need for any notification to know what pointer events are happening outside itself?

(In reply to comment #12)
> (In reply to comment #11)
> > Because clicking on the summary item to have it display its notification
> > is a more deliberate action than hovering, we can now grab focus in that
> > notification. This makes chat notifications in the summary more convenient
> > to use because you don't need to click on the text entry there.
> 
> Is there anything else apart from setting focus on the text entry for chat
> notifications that you want to achieve? If not, then this seems like an overly
> complicated approach. Intercepting all events and manually passing hover/click
> events to some parts is extremely painful - I did it for the search entry and
> it still hurts! The difference is that the design called for find-as-you-type
> style behavior there - not setting the key-focus after clicking a button. So
> unless I'm overlooking something subtle here, why don't you add an empty
> Notification.activate() method in messageTray and call it when the summary item
> is clicked, then overwrite the method in telepathyClient to call
> set_key_focus() on the entry? Or is there a need for any notification to know
> what pointer events are happening outside itself?

Florian, notifications need to pop down when the user clicks outside of them (as we discussed on IRC).

Jon, it actually seems that it would make more sense to let the clicks or the hot corner activation to go through when the notification is shown. If the user just wanted to pop down the notification, they would have a way to do so just by clicking again on the summary item or by clicking Esc (which will be implemented shortly). It is rather annoying to have to click twice elsewhere or to move the mouse to the hot corner and wonder why is it not being activated. Would it be ok to implement it so that the clicks or hover over the hot corner pop down the notification AND go through?

Created attachment 165060 [details] [review]
Grab focus in the summary notification

Because clicking on the summary item to have it display its notification
is a more deliberate action than hovering, we can now grab focus in that
notification. This makes chat notifications in the summary more convenient
to use because you don't need to click on the text entry there.

Make sure summary notifications disappear when the run dialog or the overview
is shown.

Make sure the summary appears when we are showing a new notification and
switch to the overview at the same time.

shell-global changes in this patch were written by Dan and he will make
a separate patch with them. This patch can serve as a reference for him
for how to abstract away some usage of the new shell_global_focus_stage()
function he added. This patch will then be rebased.

After trying this out for a few weeks, I have to say it's rather frustrating. Part of the issue is that it creates a set of moving targets. Even when there is only a single icon it feels as if the target moves. Additionally the lower panel itself seems to present a small target, so when quickly trying to get to the appropriate notification (which keeps moving as I mouse over other notifications) I'll often find the panel suddenly no longer there. This is amplified in Empathy notifications where the sizes of the targets keep changing as well depending on who I am speaking to at the time (because all targets take the size of the person with the longest name), and when you click on the name, the chat box doesn't immediately get focus.

My proposal for how to alter this would be:

1) Mouse over an icon and it highlights.
2) An animation occurs to show the title in a box above the panel.
3) User clicks icon.
3) Box expands to show full "normal" notification.
4) Box is given focus.
5) User either clicks away from box to get rid of the panel and remove focus or
5a) User clicks panel icon to remain in the panel and choose another icon.

Created attachment 165449 [details] [review]
Grab focus in the summary notification

Because clicking on the summary item to have it display its notification
is a more deliberate action than hovering, we can now grab focus in that
notification. This makes chat notifications in the summary more convenient
to use because you don't need to click on the text entry there.

We pop down the notification when the user clicks anywhere outside of it,
triggers the overview, or the run dialog.

This patch also ensures that the summary appears when we are showing a new
notification and switch to the overview at the same time.

This patch depend on two patches in
https://bugzilla.gnome.org/show_bug.cgi?id=623429
that add SHELL_STAGE_INPUT_MODE_FOCUSED .

Here are some updates to the message tray behavior Jon and I discussed yesterday.

Here are the ones that are related to the Esc functionality I'm currently working on (bug https://bugzilla.gnome.org/show_bug.cgi?id=617231):

1) Take focus when new notifications are expanded by the user.
 - If a notification expanded automatically (i.e. because it was urgent), only take focus when the user hovers over it.
 - This means chat notifications will have focus as soon as you hover over them. Use same glow for the focused chat box as for the focused search box in the overview to make it more obvious.

2) Make Esc work for all focused notifications (both new ones and summary ones). Make Esc for the summary notification pop down the message tray even if you are still hovering over it.

3) Don't show the changed summary after the user is done with a notification that was explicitly expanded. This means we will never show a changed summary after the user its Esc for an expanded notification.

4) If a notification popped up under the pointer, only expand it if the user moves out of it and then moves back in. Use a longer timeout before popping it down in that case. (bug https://bugzilla.gnome.org/show_bug.cgi?id=617209 )

5) We should remove the new notification as soon as you hover away from it (not wait for the initial timeout).

Here are the ones that will likely end up as separate bugs:

1) Make the whole notification area clickable and have clicking on it take you to the application. The exceptions are text boxes and action buttons. There is already a bug for that with a patch: https://bugzilla.gnome.org/show_bug.cgi?id=613932

2) Chats should jump to the top as they are received. Not above urgent messages, but above Gwibber messages.

3) Once the notification is focused, enable navigating action buttons with left and right arrows, tab, and Enter.

4) Once the notification is focused, enable moving to the next new notification by hitting the down arrow. (The down arrow would act in the same way as Esc for the last notification that is currently queued.) We need some indication that there are more queued notifications.

5) Don't show the summary if it's content changes while the message tray is not in use (e.g. because the user focused on the window which caused the icon for that window to be removed from the message tray).

6) Use 'expires' or some other hint to make urgent notifications stay on the screen until the user acknowledges them (i.e. by selecting an action, clicking on the notification, hitting Esc or down arrow).

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

Created attachment 165587 [details] [review]
Grab focus in expanded notifications on hover

In addition to already grabbing focus in the summary notifications,
we also want to grab focus in the new notifications when the user
hovers over them and expands them.

The notifications that expand automatically will only get focus once
the user hovers over them. The notifications that don't expand will
never grab focus because they don't need it.

Make sure that we toggle the way we grab focus when switching between
the overview and the main mode. This is necessary because, unlike
summary notifications that pop down when the user moves between the two
modes, new notifications keep being shown as long as the user hovers
over them or until they time out.

Created attachment 165596 [details] [review]
Make Esc hide the tray when a notification has focus

It's convenient to have Esc working for expanded new notifications that
have focus and for summary notifications.

(In reply to comment #15)
> After trying this out for a few weeks, I have to say it's rather frustrating.
> Part of the issue is that it creates a set of moving targets. Even when there
> is only a single icon it feels as if the target moves. Additionally the lower
> panel itself seems to present a small target, so when quickly trying to get to
> the appropriate notification (which keeps moving as I mouse over other
> notifications) I'll often find the panel suddenly no longer there. This is
> amplified in Empathy notifications where the sizes of the targets keep changing
> as well depending on who I am speaking to at the time (because all targets take
> the size of the person with the longest name), and when you click on the name,
> the chat box doesn't immediately get focus.
> 
> My proposal for how to alter this would be:
> 
> 1) Mouse over an icon and it highlights.
> 2) An animation occurs to show the title in a box above the panel.
> 3) User clicks icon.
> 3) Box expands to show full "normal" notification.
> 4) Box is given focus.
> 5) User either clicks away from box to get rid of the panel and remove focus or
> 5a) User clicks panel icon to remain in the panel and choose another icon.

Addendum: After more experimenting part of the issue with target finding would be mitigated if there was a minimum 1-2s delay before the panel retracted.

Review of attachment 165449 [details] [review]:

Looks good - the "also ensures that ..." part could be split out for clarity (even if it's a one-line change if i'm not mistaken) - up to you.

Review of attachment 165587 [details] [review]:

Looks good overall, comments below

::: js/ui/messageTray.js
@@ +505,3 @@
+    // and in the main view, we need to change how it is
+    // done when we move between the two.
+    _toggleFocusGrab: function() {

Can this be something like toggleFocusGrabMode or regrabFocus?
The current name sounds like:
if (grabbed)
  ungrab()
else
  grab()

@@ +748,3 @@
         this._summaryNotificationState = State.HIDDEN;
         this._summaryNotificationTimeoutId = 0;
+        this._overviewVisible = Main.overview.visible;

Is it possible that the message tray is constructed after entering the overview?

@@ +764,3 @@
                 this._overviewVisible = true;
                 this.unlock();
+                this._updateState();

Why is it necessary to call _updateState twice when the tray is locked? I would have expected something like:

if (this._locked)
  this.unlock();
else
  this._updateState();

@@ +770,3 @@
                 this._overviewVisible = false;
                 this.unlock();
+                this._updateState();

Dito.

::: js/ui/telepathyClient.js
@@ -583,2 @@
         this._responseEntry = new St.Entry({ style_class: 'chat-response' });
-        this._responseEntry.clutter_text.connect('key-focus-in', Lang.bind(this, this.grabFocus));

Can you squash this with the previous patch?

Review of attachment 165596 [details] [review]:

OK to commit after addressing the comment below:

::: js/ui/messageTray.js
@@ +459,3 @@
+            case Clutter.EventType.BUTTON_PRESS:
+                if (!this.actor.contains(source))
+                    this.ungrabFocus();

Missing break!

Review of attachment 165449 [details] [review]:

Sorry for the noise, but I found something else :(

::: js/ui/messageTray.js
@@ +475,3 @@
+
+        if (this._prevFocusedWindow) {
+            metaDisplay.set_input_focus_window(this._prevFocusedWindow, true, global.get_current_time());

I think you should pass 'false' to set_input_focus_window - otherwise the decoration ends up focused, but the actual window only receives key strokes if it has mouse focus.

Another comment:
Should we really remove the app-menu while a notification has focus? I guess the answer is yes if we want the menu to mean "the application which will receive input if I start typing", but is that what we want? The notification feels less disruptive if the app-menu is left in place (even if technically wrong).

Jon?

The second patch in bug 623429 changes that behavior (leaving the app menu unchanged when a notification is focused)

> @@ +748,3 @@
>          this._summaryNotificationState = State.HIDDEN;
>          this._summaryNotificationTimeoutId = 0;
> +        this._overviewVisible = Main.overview.visible;
> 
> Is it possible that the message tray is constructed after entering the
> overview?

Not right now, but I thought it'd be more correct to use this variable.
> 
> @@ +764,3 @@
>                  this._overviewVisible = true;
>                  this.unlock();
> +                this._updateState();
> 
> Why is it necessary to call _updateState twice when the tray is locked? I would
> have expected something like:
> 
> if (this._locked)
>   this.unlock();
> else
>   this._updateState();

I debated this one for a while before. I think it is a trade-off between shorter code and more pseudo-efficient code. I think we typically prefer shorter code. Because this.unlock() does the check for this._locked and because this._updateState() should not have an effect if called multiple times, I thought it was fine to just do what I did. I did change it to what you suggested because either way is really fine :).

> ::: js/ui/telepathyClient.js
> @@ -583,2 @@
>          this._responseEntry = new St.Entry({ style_class: 'chat-response' });
> -        this._responseEntry.clutter_text.connect('key-focus-in',
> Lang.bind(this, this.grabFocus));
> 
> Can you squash this with the previous patch?

No, because in the previous patch the new chat notification still needed to handle 'key-focus-in' when the user clicked in the chat text box, so this line was necessary. 

> ::: js/ui/messageTray.js
> @@ +475,3 @@
> +
> +        if (this._prevFocusedWindow) {
> +            metaDisplay.set_input_focus_window(this._prevFocusedWindow, true,
> global.get_current_time());
> 
> I think you should pass 'false' to set_input_focus_window - otherwise the
> decoration ends up focused, but the actual window only receives key strokes if
> it has mouse focus.

Wow! Thank you for noticing this!

> Should we really remove the app-menu while a notification has focus? I guess
> the answer is yes if we want the menu to mean "the application which will
> receive input if I start typing", but is that what we want? The notification
> feels less disruptive if the app-menu is left in place (even if technically
> wrong).
> 
> Jon?

Jon said that we should leave the app menu in place when the notification has focus when we worked on the message tray design last Thursday. Exactly for the reason that this way it is less disruptive and this way it tells you what your main activity is and what you'll switch back to if you hit Esc.

(In reply to comment #28)
> Not right now, but I thought it'd be more correct to use this variable.

Yes, it just raises the question why we mirror that variable if we could use Main.overview.visible all along ...


> I debated this one for a while before. I think it is a trade-off between
> shorter code and more pseudo-efficient code. I think we typically prefer
> shorter code. Because this.unlock() does the check for this._locked and because
> this._updateState() should not have an effect if called multiple times, I
> thought it was fine to just do what I did.

Oh, I wasn't concerned about efficiency (pseudo-efficient nails it pretty well) - I was just less confident that it is safe to call this._updateState() multiple times like that. You certainly know that code better than me, so that concern was unfounded.


> Jon said that we should leave the app menu in place when the notification has
> focus when we worked on the message tray design last Thursday. Exactly for the
> reason that this way it is less disruptive and this way it tells you what your
> main activity is and what you'll switch back to if you hit Esc.

Yeah, sorry for the noise - I somehow overlooked that Dan already had a patch for it ...

(In reply to comment #29)
> Yes, it just raises the question why we mirror that variable if we could use
> Main.overview.visible all along ...

We want the message tray to slide in/out along with the zoom in/out animation, which means messageTray._overviewVisible has to become true when the entering-overview animation starts, and become false when the leaving-overview animation starts. Main.overview.visible won't work becuase it doesn't become false until the leaving-overview animation *ends*.

(If the overview had a 4-way HIDDEN/SHOWING/SHOWN/HIDING state like the message tray does, then we wouldn't need _overviewVisible.)

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

Should focus really be lost on the tray if the user clicks/mouses over on activities while something is open? It looks a little strange. The should be more indication of what's happening as it feels somewhat jarring.

Additionally it looks a bit odd to have the tray hide then show again immediately in this example. If the user presses SUPER and leaves the mouse in the tray area this doesn't occur, and it looks much more natural.

Created attachment 180428 [details] [review]
This patch is the implement the right click menu as described in section 2 of the mockup in the first post of this bug report.

The menu alignment at present is aligned vertically (as opposed to it being mockup-ed as horizontal. The open and the 'quit' part of the menu has not been implemented yet. This is quite a huge patch so I think I should just post it up first.

This patch incoperates a patch posted by marinaz for bug 641810(attachment 180370 [details] [review]) for improved focus management.

Can you please first apply my updated patch from bug 641810, and then create a patch for the right click menu that applies on top of that. That way it will be much easier to review and continue working with once my patch is committed. It's generally fine to upload a patch and say that it applies on top of a patch in some other bug or another patch in the same bug.

Created attachment 180477 [details] [review]
Patch update to include the earlier commits by marinaz.

Created attachment 180487 [details] [review]
Sorry, I overlooked something. This is the final one marina. :)

The last patch still has my patch as part of it. Also bug 641810 now has a new patch, so perhaps you can rebase on top of it.

Created attachment 180677 [details] [review]
I noticed bug 641810 is committed.

Kinda tricky dealing around with multiple versions of 641810. the previous patch i posted here is based on the prev version of the 641810 patch.

So... updated to latest HEAD (today). Spent 4 hrs rebuilding gnome-shell *cries*.

Created attachment 180685 [details] [review]
Finally! This is completely clean and working! Updated to include the latest (was patches then,) commits by marinaz.

This patch includes a minor line rearrangement of the very long condition in updateState();

Review of attachment 180685 [details] [review]:

Overall this patch is pretty close, but it requires some refinement.

Suggested commit message:
"MessageTray: add right click menu for summary items

This provides straight forward controls for opening the corresponding application or removing the summary item."

You'll need to rebase this patch to apply to the latest on master.

Some general comments about functionality:
- Open needs to be implemented (comments on how to do that are inline)
- The menu should work in the same way as the menus in the top panel: no option should be selected from the start and up and down keys should work right away to move the selection (might have to resolve how key nav in the popup menu relates to the focus grabbing in the message tray)
- The menu should have top and bottom padding as the menus in the top panel
- Left clicking on the summary item when showing the menu should show the notification immediately (comments on how to do that are inline)

::: js/ui/messageTray.js
@@ +918,3 @@
+        item = new PopupMenu.PopupMenuItem(_("Open"));
+        item.connect('activate', Lang.bind(this, function() {
+                                                 }));

Just rename _notificationClicked() to open() in Source and all three subclasses, use it as a callback function for notification.connect('clicked', ...) , and call it here.

We usually just give a 4 spaces indentation to the first line of the function or we put "function() {" on the following line with 4 spaces indentation.

@@ +924,3 @@
+        item.connect('activate', Lang.bind(this, function() {
+                                                    source.destroy();
+                                                 }));

Fix up indentation.

@@ +995,3 @@
         this._clickedSummaryItem = null;
+        // This can hold 3 numbers. 1, 2 or 3 which signify the left, middle and right mouse button respectively.
+        this._clickedSummaryItemMouseButton = -1;

I don't think the comment is necessary here. It's unclear here how these values get assigned, but at the point when we do the assignment to event.get_button() anyone can look up the return values for get_button() .

@@ +1349,3 @@
 
+    _onSummaryItemClicked: function(summaryItem, event) {
+        if (!this._clickedSummaryItem || this._clickedSummaryItem != summaryItem) {

I think adding

|| this._clickedSummaryItemMouseButton != event.get_button()

will allow switching between right-click menu and the notification for the same summary item with a single click. It's possible that you will also need to change how we figure out 'wrongSummaryNotification' value.

@@ +1471,3 @@
+                                   !(this._notification && this._notification.urgency == Urgency.CRITICAL) &&
+                                   !this._pointerInTray && !this._locked) ||
+                                   this._notificationRemoved);

Please don't add unrelated line re-arrangements as part of the patch. It's ok to keep it as a long line, at least until we need to modify this line again :).

@@ +1522,2 @@
         if (this._summaryNotificationState == State.HIDDEN) {
             if (haveSummaryNotification && !summaryNotificationIsMainNotification && canShowSummaryNotification)

You should update names of these variables to reflect that it's ether the notification or the right click menu that we should show/hide. So perhaps
haveSummaryNotification -> haveClickedSummaryItem
canShowSummaryNotification -> canShowSummaryBoxPointer
wrongSummaryNotification -> wrongSummaryBoxPointer

@@ +1790,3 @@
         if (index != -1)
             this._notificationQueue.splice(index, 1);
+        if (this._clickedSummaryItemMouseButton == 1) {

You should put everything related to initializing this._summaryNotification inside this condition. You should also rename variables that could have either notification or right-click menu to be generic, e.g.
this._summaryNotificationBoxPointer -> this._summaryBoxPointer
this._summaryNotificationState -> this._summaryBoxPointerState

@@ +1843,3 @@
     },
 
+    _hideSummaryBoxPointer: function() {

You should also rename _hideSummaryNotificationCompleted() to _hideSummaryBoxPointerCompleted() and be sure to only do things related to this._summaryNotification if one is set.

Maybe I'm a bit late in this, but since we're adding a popup menu to notifications - would it make sense to let notifications override the items in it?
It would require exposing it in Notification instead of SummaryItem, and be an instance of a custom class (inheriting from PopupMenuBase), but it would be a clean entry point for extensions (I'm thinking of a dbusmenu extension, as a stop gap until KDE apps are ported to libnotify).
Also, it would allow to have more specific items for special notifications like chat (so one would have "Close chat" instead of "Remove" and "Open in Empathy" instead of just "Open").

Created attachment 182613 [details] [review]
Added right click menu for summary items

This enables users to open, remove or quit items by using this menu.

Review of attachment 182613 [details] [review]:

Suggested commit message:
"MessageTray: add right click menu for summary items

This provides straight forward controls for opening the corresponding
application or removing the summary item."

As we discussed on IRC, the right click menu also needs padding.

::: js/ui/messageTray.js
@@ +926,3 @@
+
+        item = new PopupMenu.PopupMenuItem(_("Open"));
+        item.connect('activate', Lang.bind(this, function() {

Adding source.open(); here works for persistent notifications that have associated apps, but I'm still looking into what we need to do here for notifications that don't have associated apps or are resident.

@@ +935,3 @@
+        }));
+
+        this.rightClickMenu.add(item.actor);

Sync up having or not having a blank line before add() with the previous add() .

To have the key-nav/hovering work, add here:
        let focusManager = St.FocusManager.get_for_stage(global.stage);
        focusManager.add_group(this.rightClickMenu);

And add 'key-focus-out' logic in PopupBaseMenuItem in popupMenu.js :

        if (params.reactive) {
            this.actor.connect('key-focus-in', Lang.bind(this, this._onKeyFocusIn));
            this.actor.connect('key-focus-out', Lang.bind(this, this._onKeyFocusOut));
        }

    _onKeyFocusOut: function (actor) {
        this.setActive(false);
    },

@@ +960,3 @@
     setEllipsization: function(mode) {
+        if (this._sourceTitle.clutter_text)
+            this._sourceTitle.clutter_text.ellipsize = mode;

This seems unrelated to the patch.

@@ +1003,2 @@
                                                                         { reactive: true,
                                                                           track_hover: true });

Need to adjust alignment.

@@ +1570,3 @@
+        let wrongSummaryBoxPointer = (haveClickedSummaryItem &&
+                                     this._summaryNotification != this._clickedSummaryItem.source.notification &&
+                                     this._summaryBoxPointer.bin.child != this._clickedSummaryItem.rightClickMenu);

This fixes switching between the notification and the right click menu for the same item:

        let wrongSummaryNotification = this._clickedSummaryItemMouseButton == 1 && this._summaryNotification != this._clickedSummaryItem.source.notification;
        let wrongSummaryRightClickMenu = this._clickedSummaryItemMouseButton == 3 && this._summaryBoxPointer.bin.child != this._clickedSummaryItem.rightClickMenu;
        let wrongSummaryBoxPointer = haveClickedSummaryItem && (wrongSummaryNotification || wrongSummaryRightClickMenu);

@@ +1846,1 @@
                                                                                Lang.bind(this, this._escapeTray));

Need to adjust alignment.

Created attachment 182649 [details] [review]
MessageTray: add right click menu for summary items

This provides straight forward controls for opening the corresponding
application or removing the summary item.

This patch requires Bug 644043 to be fixed.(ie, requires the patch from there to work properly).

Created attachment 182650 [details] [review]
MessageTray: add right click menu for summary items

This provides straight forward controls for opening the corresponding
application or removing the summary item.

Edited some indentation errors.

Created attachment 182652 [details] [review]
MessageTray: add right click menu for summary items

This provides straight forward controls for opening the corresponding
application or removing the summary item.

Fixed a variable name error.

Review of attachment 182652 [details] [review]:

Looks great! Thanks for all your work on this!

Will commit it once the patch for bug 644043 lands.

can this bug be closed?

I fully agree with https://bugzilla.gnome.org/show_bug.cgi?id=617224#c21

It is really hard to catch moving icons in messaging tray...

Here is link to discussion about this:
http://mail.gnome.org/archives/usability/2011-May/msg00000.html

More people also finds this animation annoying and not comfortable:
http://mail.gnome.org/archives/usability/2011-May/msg00005.html
http://mail.gnome.org/archives/usability/2011-May/msg00006.html

These individuals must not be using 3.0.x builds if they can't click on the text. Since before 3.0 the entire thing including the label has been click-able. We even developed a way of hinting this to the user through highlighting.

Note that the highlighting doesn't occur on legacy icons. This is the bug that should be fixed. The basic functionality is fine.

Don't know what version use these individuals, but I use 3.0.1-0ubuntu1~build1. Yes text is click-able, but still, if you move mouse cursor from left to right it is hard to catch jumping icons even if it is possible to click on text, an empty space appears in same place where I wanted to click. Of course clicking on empty space also works, bet this behavior is strange...

Other think that annoys me even more is when I move mouse cursor from right to left and if I move it too fast, mouse cursor goes for about 2 pixels above next icon to the left. Ok it's my mistake, but icon, that I wanted to click jumps to the right, so instead moving my mouse cursor 2 pixels back to correct my mistake, I'm forced to move it back through all instantly appeared next icon's label... Then again, when I reach icon that just run away from my mouse cursor, it moves to the left! Again!? Of course I can click on empty space, below my cursor, but it seems, that those icons just plays with me a game called „catch me if you can“...

I don't understand what purpose of these moving icons, but even now, when I know that I can click on text and empty space between text and icon it still annoys me and all the time it is hard to me, to catch those moving icons...

I tried bringing this up on the gnome-shell mailing list a while ago, but nobody seemed to be terribly helpful, so I'll re-iterate my thoughts here. I'm using Gnome Shell 3.0.2.

The problem with the notification bar is with the fact that the target area changes size as you mouse over each notification. To be more specific, when you mouse over an icon, it slides, revealing it's label, and then when the mouse leaves the icon, it shrinks. This is the root cause of the usability problem with the new notification bar, as the target area for each notification changes size in an inconsistent and unclear manner. This makes selecting notifications feel clumsy. It also wastes the user's time, which is never a good thing.

To list the problems briefly:

1. The changing target sizes create a large penalty for the user if the pointer overshoots the distance between notifications
2. The changing target sizes don't make it clear how far the user has to move the pointer to mouse over it after the bar is opened
3. The changing/small target sizes make it hard for the user to gauge where the notification is when the notification bar is collapsed.

Below, I will elaborate on what I just listed, in case what I said wasn't clear, as well as possible solutions. Feel free to skip the following explanation if what I said made sense.

Let's say we have four notifications, A, B, C, and D. They are positioned on the notification bar in that order, notification D being in the rightmost corner of the screen.

To demonstrate the first problem, I'll move the mouse to an arbitrary point that will trigger the notification bar to open. Let's say that my mouse is on notification C, and this causes notification C to expand. In this specific case, the goal is to get to notification B. If for one reason or the other I overshoot the distance, and my mouse travels to notification A instead, then my mouse has to travel quite the distance in order to correct my error (specifically, the width of notification A's label). If the notifications didn't change target sizes, the penalty for overshooting a distance would be significantly less.

To demonstrate the second problem, Lets say we just opened the notification bar, and we're about to select notification A. The problem is that our mouse is over notification C, causing it to expand. This makes us think that A is actually much closer than we think it will be upon opening it. So, we must wait for the animation to finish, and then move over to notification A. This wastes the user's time, and makes the interface feel more clumsy.

Lastly, the third problem. When the notification bar is closed, it's hard to gauge where a given notification will be. Even if the target areas were to remain static, the icons are too small to be able to quickly open them. In gnome 2, it was easy to select items from the window picker because they were wide. The same principles can be applied toward the notification bar in gnome 3; if we make the labels always shown, then it will be easier to gauge where they are when the bar is collapsed because it's wider, so the target area is bigger, and user error/overshooting can be taken into account.

</lengthy explanation>

For a solution, I propose that we keep the labels always shown, and remove the expanding animation. When short on space, we could truncate the labels (or make the font smaller) and make the target areas proportionally smaller (much like the window picker in gnome 2). This way, the position and target areas of each notification is consistent, clear, and easy to gauge while the dock is closed.

It may be a bit hard to pull off, but it might also be nice to add a fisheye-sort of animation to make the labels bigger, mainly when short on space. Care would need to be taken such that it doesn't affect usability (eg make the target areas shift toward the left of the screen), but it may improve readability of the labels when short on space. Food for thought.

Sorry for the length of the comment, but I figured it would save everyone's time if I was clear the first time. Please let me know what you think. I love Gnome 3 so far, and I want to help make it better for everyone. Thanks!

I mostly agree with Will, the resizing really makes it hard to hit the correct target when moving the pointer quickly. However, I would rather have the labels not show up in this bar at all.

Real shell-style notifications could just "open" and show their content. Legacy notifications tray icons could show a text label (=name) in a separate bubble on hover.

discussion of the "accordian" effect is in bug 650392

I'm closing this bug; the original design has been implemented, and having a bug whose summary is basically "do stuff to the message tray" is not useful. If there are specific issues remaining here, they should be filed as new bugs.