Created attachment 168231 [details] [review]
[MessageTray] remove Source IDs

The tray itself does not actually need them, and to make status icon
sources work correctly the NotificationDaemon will need to be tracking
its sources by two separate IDs, so the existing system won't work.

Also remove MessageTray.removeSourceByApp(), which is
NotificationDaemon-specific, and implement the functionality in
notificationDaemon.js instead.

Created attachment 168232 [details] [review]
[MessageTray] remove Notification IDs

Notification daemon notifications have IDs, but other kinds don't
necessarily, and the generic code doesn't need to bother with it.

Created attachment 168233 [details] [review]
[MessageTray] use Params.parse in Notification

Created attachment 168234 [details] [review]
[MessageTray] Clean up source-vs-notification icon handling

A Source needs exactly one summary icon (which in the case of a
trayicon-based source won't even be just an image), but possibly many
notification icons, which may vary for successive notifications
(particularly in the case of NotificationDaemon notifications). So
differentiate these cases in the API.

Review of attachment 168231 [details] [review]:

The change seems fine, except it needs to be fixed up per comments below.

::: js/ui/messageTray.js
@@ +828,2 @@
     contains: function(source) {
+        return this._summaryItems.indexOf(source) != -1;

This and a similar check in removeSource() don't work because this._summaryItems contains SummaryItems and not Sources. Since it probably makes sense to keep Source unaware of its representation as a SummaryItem, we can just search through this._summaryItems "manually" when checking whether the message tray contains a given Source.

::: js/ui/notificationDaemon.js
@@ +169,3 @@
+            this._sources[appName] = source;
+
+            source.connect('clicked', Lang.bind(this,

This is a duplicate connection to 'clicked' - you just need to remove one of them.

@@ +279,3 @@
+            let source = this._sources[id];
+            if (source.app == tracker.focus_app)
+                source.destroy();

In theory, we should be able to return here. Unless the same app were to be sending notifications with two different appNames. It's up to you if you think a return statement is right here.

::: js/ui/windowAttentionHandler.js
@@ +60,3 @@
         let tracker = Shell.WindowTracker.get_default();
         let app = tracker.get_window_app(window);
+        let app_id = app.get_id();

appId

Created attachment 168430 [details] [review]
Look through this._summaryItems to find a SummaryItem for a given Source

Here is a patch that you can squash with the one for removing source ids
to do the source lookups correctly. It applies on top of the one for
removing source ids. I used it to test other patches in this bug.

Review of attachment 168231 [details] [review]:

::: js/ui/messageTray.js
@@ +902,3 @@
                 }
             }
+            for (i = 0; i < this._summaryItems.length; i++) {

'let' is missing here

Review of attachment 168232 [details] [review]:

Looks good.

Review of attachment 168233 [details] [review]:

Do you have specific examples for when we'll need to have a body for the notification that will not be used as a banner and when we'll need to have a banner for the notification that we will not want to move to the body when the notification is expanded? If not, maybe we should just get rid of all these options until we actually need them.

We actually don't have an implementation for wrapping the banner if it doesn't fit now (which would be similar to what we do for the title in bug 623970 ), so setting bannerBody to false won't work as is anyway.

Also, the current code will just ignore params.body when params.bannerBody is true, which should at least be addressed in a comment.

::: js/ui/notificationDaemon.js
@@ +226,3 @@
             notification.connect('action-invoked', Lang.bind(this, this._actionInvoked, source, id));
         } else {
+            notification.update(summary, body, { clear: true });

need to preserve bannerBody: true

::: js/ui/windowAttentionHandler.js
@@ +76,2 @@
         window.connect('notify::title', Lang.bind(this, function(win) {
+                                                            notification.update(this._getTitle(app, win), this._getBanner(app, win));

need to preserve bannerBody: true

Review of attachment 168233 [details] [review]:

::: js/ui/messageTray.js
@@ +154,3 @@
         if (this._icon)
             this._icon.destroy();
         if (this._scrollArea && (this._bannerBody || clear)) {

it should be params.clear here and in the next check over; also this._bannerBody might be undefined at this point

Review of attachment 168234 [details] [review]:

I think it is wrong that getSummaryIcon() in Source might return a new or existing actor depending on implementation (telepathyClient/windowAttentionHandler vs. notificationDaemon). Keeping this._iconBin in notificationDaemon.Source means that Source is aware of how its representation is drawn and that there can be only one representation at a time.

I think it will be more correct to keep createIcon() in notificationDaemon.Source that creates a new icon each time it is called. We can store the created icon objects in an array similarly to how we do that in telepathyClient and update all of them in notify().

::: js/ui/messageTray.js
@@ +171,3 @@
         this._bannerBody = params.bannerBody;
 
+        this._icon = params.icon || this.source.createNotificationIcon();

The function can just be called createIcon() because when we are asking the Source for an icon inside the Notification, we are acknowledging that we don't have a notification specific icon and we are going to use the same one as used for the source. If the implementations of how we create an icon for the source and an icon for notification ever change, e.g. we'll need them of a different size or style, it will make sense to have two functions - createIcon() and createIconForNotification() .

@@ +570,2 @@
 Source.prototype = {
+    ICON_SIZE: 24,

We should keep ICON_SIZE as a MessageTray constant. We use it when creating icons for notifications too.

::: js/ui/notificationDaemon.js
@@ +142,3 @@
     },
 
+    _iconForNotificationData: function(icon, hints, size) {

This can be a static function.

@@ +257,3 @@
             notification.connect('action-invoked', Lang.bind(this, this._actionInvoked, source, id));
         } else {
+            notification.update(summary, body, { icon: iconData,

it should be iconActor

@@ +269,3 @@
 
+        let sourceIconActor = this._iconForNotificationData(icon, hints, source.ICON_SIZE);
+        source.notify(sourceIconActor, notification);

So this means that the icon for source will be updated if the notification associated with this source has a new icon. Is this the desired behavior or should we keep the original icon? Ideally, the notification icons for the same source should not change.

@@ +344,3 @@
     __proto__:  MessageTray.Source.prototype,
 
+    _init: function(title) {

The signature of the function above should be updated too.

(In reply to comment #11)
> I think it is wrong that getSummaryIcon() in Source might return a new or
> existing actor depending on implementation
> (telepathyClient/windowAttentionHandler vs. notificationDaemon).

Oops. That's a bug introduced in a late rewrite. getSummaryIcon() is supposed to return the same ClutterActor every time it is called. (Although in fact, it only gets called once for each Source currently.)

I had changed it at one point so that instead of getSummaryIcon(), the source just had a ".icon" property. But it seemed like that should be initialized by the MessageTray.Source constructor, which meant each subclass had to pass an icon to its "super" _init method, which meant I ended up with things like:

    _init: function(stuff) {
        let myIcon = someComplicatedCall(toCreate, theIcon,
                                         maybeWithLotsOfArguments);
        MessageTray.Source.prototype._init.call(stuff, myIcon);
        ...

which I didn't really like. A simpler possibility would be to just have each _init method do "this.icon = blah" itself before returning rather than forcing them to pass the icon to the parent constructor.

> Keeping
> this._iconBin in notificationDaemon.Source means that Source is aware of how
> its representation is drawn and that there can be only one representation at a
> time.

Yes. That is an explicitly-added new constraint, which I guess I should have documented in the code, not just the commit message. This is because with trayicon sources, the Source icon is actually the embedded X window, and we can't just make another one of those if the message tray asks for it. So we clarify that the message tray *can't* ask for a second source icon; there is always exactly one.

> +        let sourceIconActor = this._iconForNotificationData(icon, hints,
> source.ICON_SIZE);
> +        source.notify(sourceIconActor, notification);
> 
> So this means that the icon for source will be updated if the notification
> associated with this source has a new icon. Is this the desired behavior or
> should we keep the original icon? Ideally, the notification icons for the same
> source should not change.

It's the existing behavior, and this patch just preserves it (while also making it easier to change behavior later if we want).

(In reply to comment #12)
> (In reply to comment #11)
> > I think it is wrong that getSummaryIcon() in Source might return a new or
> > existing actor depending on implementation
> > (telepathyClient/windowAttentionHandler vs. notificationDaemon).
> 
> Oops. That's a bug introduced in a late rewrite. getSummaryIcon() is supposed
> to return the same ClutterActor every time it is called. (Although in fact, it
> only gets called once for each Source currently.)
> 
> I had changed it at one point so that instead of getSummaryIcon(), the source
> just had a ".icon" property. But it seemed like that should be initialized by
> the MessageTray.Source constructor, which meant each subclass had to pass an
> icon to its "super" _init method, which meant I ended up with things like:
> 
>     _init: function(stuff) {
>         let myIcon = someComplicatedCall(toCreate, theIcon,
>                                          maybeWithLotsOfArguments);
>         MessageTray.Source.prototype._init.call(stuff, myIcon);
>         ...
> 
> which I didn't really like. A simpler possibility would be to just have each
> _init method do "this.icon = blah" itself before returning rather than forcing
> them to pass the icon to the parent constructor.
>

Yeah, it'd be fine for each init method to initialize the icon to whatever and then for the subclasses to set it when they have the right info.
 
> > Keeping
> > this._iconBin in notificationDaemon.Source means that Source is aware of how
> > its representation is drawn and that there can be only one representation at a
> > time.
> 
> Yes. That is an explicitly-added new constraint, which I guess I should have
> documented in the code, not just the commit message. This is because with
> trayicon sources, the Source icon is actually the embedded X window, and we
> can't just make another one of those if the message tray asks for it. So we
> clarify that the message tray *can't* ask for a second source icon; there is
> always exactly one.

Ok, so it does need a comment in the code. Maybe also say something about the fact that each caller is responsible for ensuring that the returned icon is only added to the stage once.

> 
> > +        let sourceIconActor = this._iconForNotificationData(icon, hints,
> > source.ICON_SIZE);
> > +        source.notify(sourceIconActor, notification);
> > 
> > So this means that the icon for source will be updated if the notification
> > associated with this source has a new icon. Is this the desired behavior or
> > should we keep the original icon? Ideally, the notification icons for the same
> > source should not change.
> 
> It's the existing behavior, and this patch just preserves it (while also making
> it easier to change behavior later if we want).

Makes sense.

(In reply to comment #9)
> Do you have specific examples for when we'll need to have a body for the
> notification that will not be used as a banner and when we'll need to have a
> banner for the notification that we will not want to move to the body when the
> notification is expanded?

We do it for telepathy notifications; we don't want the MessageTray code to automatically append the banner to the body, because we're already doing it ourselves in a more complicated way than it would do.

It could probably be simplified somehow though. (Later.)

> +            notification.update(summary, body, { clear: true });
> 
> need to preserve bannerBody: true

I changed notification.update to preserve the old value of bannerBody if a new value wasn't passed in.

Created attachment 168576 [details] [review]
[MessageTray] remove Source IDs

addressed all comments and squashed your patch

Created attachment 168577 [details] [review]
[MessageTray] remove Notification IDs

Created attachment 168578 [details] [review]
[MessageTray] use Params.parse in Notification

Created attachment 168579 [details] [review]
[MessageTray] Clean up source-vs-notification icon handling

kept getSummaryIcon, but added _setSummaryIcon for subclasses to use
to set it, and moved the "caching" into the base class.

I wanted to have getSummaryIcon() default to calling
createNotificationIcon() if you hadn't called _setSummaryIcon() yet, but
this makes a race condition between the summary calling getSummaryIcon()
and the subclass calling _setSummaryIcon() the first time...

Review of attachment 168576 [details] [review]:

Looks good!

Review of attachment 168430 [details] [review]:

This was just a test patch.

Review of attachment 168577 [details] [review]:

Looks good!

Review of attachment 168578 [details] [review]:

Could you add a comment that it is expected that params.body will be ignored when params.bannerBody is true OR move out
if (params.body)
    this.addBody(params.body);
into its separate "if" statement.

Ok to commit with the above change.

(In reply to comment #14)
> (In reply to comment #9)
> > Do you have specific examples for when we'll need to have a body for the
> > notification that will not be used as a banner and when we'll need to have a
> > banner for the notification that we will not want to move to the body when the
> > notification is expanded?
> 
> We do it for telepathy notifications; we don't want the MessageTray code to
> automatically append the banner to the body, because we're already doing it
> ourselves in a more complicated way than it would do.

But we are also not passing in the banner for the telepathy notifications.

Review of attachment 168579 [details] [review]:

I really like how icon-setting is done in this patch!

You still need to add a comment to getSummaryIcon() explaining that it's an explicit constraint that Source can only have one icon actor associated with it and that it's callers' responsibility to ensure that each icon is only added to the stage once.

Good to commit, once you address the comments below the way you see fit :).

::: js/ui/messageTray.js
@@ +571,2 @@
 Source.prototype = {
+    ICON_SIZE: 24,

Why not keep ICON_SIZE as a MessageTray constant, since we use it to create icons for notifications too, as in the notificationDaemon.js ?

@@ +575,3 @@
         this.title = title;
+        this._iconBin = new St.Bin({ width: ICON_SIZE,
+                                     height: ICON_SIZE });

It needs to be this.ICON_SIZE if we are keeping ICON_SIZE in Source.

::: js/ui/notificationDaemon.js
@@ +142,3 @@
     },
 
+    _iconForNotificationData: function(icon, hints, size) {

Do you think it would make sense to move this function outside of the NotificationDaemon class?

@@ +252,3 @@
             notification.connect('action-invoked', Lang.bind(this, this._actionInvoked, source, id));
         } else {
+            notification.update(summary, body, { bannerBody: true,

You don't need to be passing in bannerBody: true to update() any more.

(In reply to comment #22)
> Could you add a comment that it is expected that params.body will be ignored
> when params.bannerBody is true

It already implies that:

+// If @params contains a 'body' parameter, that text will be displayed
+// in the body area (as with addBody()). Alternatively, if it includes
+// a 'bannerBody' parameter with the value %true, then @banner will

"Alternatively"

(In reply to comment #23)
> Why not keep ICON_SIZE as a MessageTray constant, since we use it to create
> icons for notifications too, as in the notificationDaemon.js ?

Putting it into Source makes it clearer (IMHO) that it's going to be visible to other modules.

(Maybe the real fix here is that we should be underscore-prefixing private constants, so you can tell which ones aren't private.)

> +                                     height: ICON_SIZE });
> 
> It needs to be this.ICON_SIZE if we are keeping ICON_SIZE in Source.

yeah, I had that in my tree already, I'd just forgotten to commit it

> +    _iconForNotificationData: function(icon, hints, size) {
> 
> Do you think it would make sense to move this function outside of the
> NotificationDaemon class?

*shrug*

Attachment 168576 [details] pushed as d3031b4 - [MessageTray] remove Source IDs
Attachment 168577 [details] pushed as 8f6a7f3 - [MessageTray] remove Notification IDs
Attachment 168578 [details] pushed as 1951812 - [MessageTray] use Params.parse in Notification
Attachment 168579 [details] pushed as 6f57f07 - [MessageTray] Clean up source-vs-notification icon handling