Created attachment 169782 [details] [review]
[NotificationDaemon] always resolve a notification's ShellApp first

Resolve a notification's ShellApp before creating its Source and
posting the notification, so that notification sources now always have
an app associated (assuming they came from an app as opposed to the
command line).

For Sources that have an associated app, use the app's icon for the
source, rather than tracking the notification icon.

This change also lets us get rid of appNameMap, since we can just use
shell_app_get_name() now.

Review of attachment 169782 [details] [review]:

Makes sense overall, however it creates a race condition where multiple Source objects could be created for the same application if we get more notifications from it before this._busProxy.GetConnectionUnixProcessIDRemote() returns. We will also ignore cancel requests and create new notifications for replace requests if we get them before the Source for the original notification is created.

I think this can be fixed by maintaining this._pendingNotifications that will map notification ids for which we've made Source requests to a simple object that will keep the data passed in with Notify() in its properties (.appName, .icon, .summary, etc.). We can then remove that entry if the notification gets cancelled and update it if the notification gets replaced (as well as not make a new call to this._busProxy.GetConnectionUnixProcessIDRemote() in that case).

We would also need to check if this._sources already contains appName inside the callback function before creating a new Source, but still call this._notifyForSource with the data from the corresponding this._pendingNotifications entry and remove that entry. That way we won't miss notifying on multiple notifications from the same application, will use the updated information for the notifications that were replaced, and will not notify on the notifications that were cancelled.

::: js/ui/notificationDaemon.js
@@ +171,3 @@
+        let id;
+
+        if (replacesId != 0 && this._currentNotifications[id])

this._currentNotifications[replacesId]

@@ +195,3 @@
+                function (pid, ex) {
+                    let app = Shell.WindowTracker.get_default().get_app_from_pid(pid);
+                    source = new Source(app ? app.get_name() : appName, app);

Why not use appNameMap since we still potentially use appName?

@@ +237,2 @@
         if (notification == null) {
+            notification = new MessageTray.Notification(source, summary, body, { icon: iconActor });

Eventually, we should use the app icon from the source, if we have it, for the notification too. This is ok for now though.

@@ +341,3 @@
+        this.app = app;
+        if (app)
+            this._setSummaryIcon(app.create_icon_texture (this.ICON_SIZE));

extra space

(In reply to comment #2)
> Makes sense overall, however it creates a race condition where multiple Source
> objects could be created for the same application if we get more notifications
> from it before this._busProxy.GetConnectionUnixProcessIDRemote() returns.

Doh. I thought about that at one point but then forgot.

> Why not use appNameMap since we still potentially use appName?

appName is only going to get used for things using "notify-send", etc, in which case there won't really be anything sane to map it to anyway.

More specifically, we don't need Empathy in appNameMap any more, so if we did keep it, it would be empty, so...

Created attachment 170263 [details] [review]
[NotificationDaemon] always resolve a notification's ShellApp first

Resolve a notification's ShellApp before creating its Source and
posting the notification, so that notification sources now always have
an app associated (assuming they came from an app as opposed to the
command line). Index sources by PID rather than by appName (so that,
eg, multiple calls to notify-send now show up as separate sources).

For Sources that have an associated app, use the app's icon for the
source, rather than tracking the notification icon.

This change also lets us get rid of appNameMap, since we can just use
shell_app_get_name() now.

Created attachment 170264 [details] [review]
[NotificationDaemon] always resolve a notification's ShellApp first

oops, lost a "_" in a last-second renaming in that last patch

Created attachment 170714 [details] [review]
[NotificationDaemon] always resolve a notification's ShellApp first

updated along wth the patches in bug 608869

Review of attachment 170714 [details] [review]:

Looks good. Good to commit with just the change of where this._notifications[id] gets assigned ndata.

I first tried it with Rhythmbox and for some reason it doesn't find the application for the Rhythmbox's process id with Shell.WindowTracker.get_default().get_app_from_pid(pid) . So it falls back to using the icons from the notifications. For some reason, Rhythmbox sends two notifications, with one replacing the other. The second notification doesn't have a valid icon, so the icon just disappears. However, this is not a regression, since we were using icons from notifications before as well and the icon was disappearing. We can research this later.

The XChat notifications work fine. It finds the application and the right icon by pid correctly.

::: js/ui/notificationDaemon.js
@@ +210,3 @@
+            replacesId = 0;
+            ndata.id = id = nextNotificationId++;
+            this._notifications[id] = ndata;

Don't we need to do this in either case? Otherwise, this._notifications[replacesId] still has the old ndata. We need to store the new ndata to be able to just return below when source is still null (we have a pending call to GetConnectionUnixProcessId) and replacesId is not null.

@@ +270,2 @@
         if (notification == null) {
+            notification = new MessageTray.Notification(source, summary, body, { icon: iconActor });

We could also only specify the icon here if there is no app associated with the source. We'd then need to implement createNotificationIcon() for Source in notificationDaemon.js . That way we'd be using source's icon in notification as well. I think this is what the design calls for. We can implement this later though.

Just noticed based on the Rhythmbox notifications outside of the Shell that the first notification has the Rhythmbox icon as an icon, which we display correctly. Then, the notification it is replaced with has album art as an icon, which we don't display correctly.

(In reply to comment #7)
> I first tried it with Rhythmbox and for some reason it doesn't find the
> application for the Rhythmbox's process id with
> Shell.WindowTracker.get_default().get_app_from_pid(pid)

This is bug 602941; when Rhythmbox is "minimized" to the tray, it has no windows, and therefore nothing for Shell.WindowTracker to track. It doesn't show up in Alt-Tab or the overview app well either.

Attachment 170714 [details] pushed as c9a7c9d - [NotificationDaemon] always resolve a notification's ShellApp first