Created attachment 201478 [details] [review]
notificationDaemon: Group based on app name, not PID

We still need to keep the PID so we can find the app from the AppSystem, though


--

This is the only sane way to do this sort of thing. Of course, this means
that we need to make g-s-d show a separate app name for each source. I have
no idea if it does this already.

(In reply to comment #1)
> Created an attachment (id=201478) [details] [review]
> notificationDaemon: Group based on app name, not PID
> 
> We still need to keep the PID so we can find the app from the AppSystem, though
> 
> 
> --
> 
> This is the only sane way to do this sort of thing. Of course, this means
> that we need to make g-s-d show a separate app name for each source. I have
> no idea if it does this already.

The answer is "yes".

Review of attachment 201478 [details] [review]:

This approach breaks combining the tray icon and notifications from the same application under the same source. Attaching an alternative patch that uses the app name, but also uses the pid.

Created attachment 203043 [details] [review]
notificationDaemon: group sources based on a combination of pid and title

That way different system notifications, such as the ones about battery power
and the ones about software updates, are shown with separate message tray
sources.

This patch was tested with both Software Updates and Power notifications, as
well as with a tray icon and notifications for XChat.

Review of attachment 203043 [details] [review]:

::: js/ui/notificationDaemon.js
@@ +153,3 @@
     },
 
+    _lookupSource: function(title, pid, trayIcon) {

I don't like this. Just make source._pid public, and do:

    for (let sourceId in this._sources) {
        let source = this._sources[sourceId];
        if (source.title == title && source.pid == pid)
             return source;
    }

In fact, with this, we're no longer matching on a hash ID, so I'd just make this._sources into a list.

Created attachment 203928 [details] [review]
notificationDaemon: group sources based on a combination of pid and title

That way different system notifications, such as the ones about battery power
and the ones about software updates, are shown with separate message tray
sources.

Review of attachment 203928 [details] [review]:

Looks good.

this change breaks skype tray icon. 

now it treats skype as a notification instead of an actual application in the tray.

it doesn't have an icon, left click doesn't do anything, right click opens the open/remove dialog and the animation when sliding the cursor left/right contains 3 skype texts.

http://pkgbuild.com/~ioni/shell-20120118-1.webm

I can confirm that this change breaks many tray icons, e.g. pidgin, opera, dropbox...

The icons of some applications reappear when restarting gnome-shell while these programs are running, but this isn't true for all applications. Also I can confirm that clicking on existing (or non existing) icons doesn't have any effect anymore although it should for several applications.

Reverting this change fixes the problem.

Can people who are reporting problems here clarify whether they are testing with GNOME Shell 3.2.2 or the development branch?

I made a mistake in merging into the 3.2 branch which is probably responsible for the problems described, but want to make sure there aren't other problems. (In fact, I found another problem with master where it would crash after a legacy tray icon was removed.)

Will attaches patches for the mismerge and for the crash.

i'm sorry for not mention at the beginning. 

is gnome-shell 3.2.2

Created attachment 205649 [details] [review]
notificationDaemon: fix problem with cherry-pick to gnome-3-2

When backporting 7e654ab3ca
(notificationDaemon: group sources based on a combination of pid and title)
to old-style classes, I screwed up passing arguments to the Source
constructor.

Created attachment 205650 [details] [review]
notificationDaemon: fix order of arguments to _lookupSource()

The order of arguments passed to lookupSource() was wrong, causing
problems when tray icons were removed.

i just check your patches and it fixes my original problems with skype.

It will be nice to have another tester.

I can confirm that the patch in comment 13 applied to gnome-shell-3.2.2 solves the problem.

Thank you for this quick fix.

Applied patches #12 + #13 to gnome-shell 3.2.2; works well so far.

Review of attachment 205650 [details] [review]:

It should be _lookupSource() in the patch description.

Comment on attachment 205650 [details] [review]
notificationDaemon: fix order of arguments to _lookupSource()

Patches reviewed by Marina, pushing.

Attachment 205649 [details] pushed as 7ff63a2 - notificationDaemon: fix problem with cherry-pick to gnome-3-2