GNOME Bugzilla – Bug 630847
Show Rhythmbox as a message tray source when it is running
Last modified: 2011-03-22 13:16:50 UTC
We should show Rhythmbox as a message tray source as soon as it starts and ensure that the summary notification for it has all the options currently available in the Rhythmbox tray icon menu. This will allow us to get rid of the Rhythmbox tray icon.
Ideally only when it is running "in the background" it doesn't make too much sense for it to be in the foreground and in the message tray.
I guess the idea would be that rhythmbox would create a notification (if it didn't already have one) when hidden, with no track details, just the actions? Bug 623200 adds pause and previous actions (I'll probably polish up and commit that patch some time soon). If this is going to show up when not playing, we'll need to switch between play and pause actions. The other actions in the tray icon menu are: show the window, enable/disable notifications, quit. Rhythmbox plugins can theoretically add things to the tray icon menu, but I think we'll have to ignore that for now. Do we need an action to show the main window, or is there a more direct way of doing that? The enable/disable notification item doesn't make any sense in the shell, as far as I can tell. Not sure about quitting. I don't think I'd like to have that presented the same way as simple playback controls.
I think we need to figure out the way for Rhythmbox to create a message tray source on start up that has a summary notification associated with it that will have no track details, but just the actions. I don't think we necessarily need to show a new notification like that. It will either be redundant when it is shown at the same time as Rhythmbox starts up and is in focus or random when it is shown when Rhythmbox is automatically launched on start-up. Perhaps we can achieve getting the source for Rhythmbox in the message tray by Rhythmbox sending a notification with some special "no-show" hint. Just clicking on the notification shows the main window. We'll have open/remove/quit in the source right click menu described in bug 617224. We have to figure out how remove option can be undone, if it in fact should be present for the Rhythmbox source. Switching the status to "busy" suppresses the non-urgent notifications, but we could also have a "hush" option in the right click menu which will hush notifications for an individual source. It would be great if you polish and commit the patch in bug 623200. It relates to the bug 621009 and the bug 624584 in GNOME Shell. The first one has a patch for having a background image in notifications that we need to review and land. The second one proposes a way for making the icon action buttons explicitly opt-in and the Rhythmbox patch would need to use that way.
OK, so it sounds like I only need to deal with play vs pause and creating the message tray source early. Bug 623200 is done now. One slightly annoying thing there is that using the 'pause' button on a notification closes it, but since it doesn't result in a track change (unlike prev/next), we don't create a new notification to replace it. This leaves you with no way to start playback again. Should we create a new notification after pausing, or should there be a way (another hint?) to indicate that using the buttons shouldn't close it?
And note that after bug 631722 is fixed you will be able to detect if the notification server supports persistence and hide/not-use the RB status icon in response. See: http://git.gnome.org/browse/libnotify/tree/tests/test-persistence.c Let me know what you think of that approach. Thanks.
Once I start seriously working towards a GNOME 3 version of rhythmbox, I'm going to split the notification code out into a separate plugin, since it won't really have much to do with the status icon code, and probably disable the status icon plugin by default. I'm trying out the new hints and capabilities in notification-daemon to figure out the finer details, and I think that's getting pretty close to how I want it to work.
If I understand correctly the recommended way of implementing notifications in the Shell, you shouldn't split status icon and notification in two different plugins: you should have one plugin that provides either the former or the latter depending on whether the Shell is running or not. You should just check whether the running notification daemon supports persistence, and if it doesn't, fallback to a status icon. If you implement this using two plugins, you won't be able to change your behavior automatically. And messing with plugins to tweak this when you want to switch between GNOME 3 and GNOME "classic" would be silly...
Between gnome-shell, the notification daemon redesign (it supports persistence too now), and ubuntu's sound menu and notify-osd, I don't see the status icon fitting in anywhere anyway. If someone's going to the trouble of running rhythmbox 3 (or whatever) against an old notification daemon, they can enable a plugin too.
(In reply to comment #7) > latter depending on whether the Shell is running or not. You should just check > whether the running notification daemon supports persistence, and if it > doesn't, fallback to a status icon. (ftr, the right way to decide whether to do gnome-3-ish or gnome-2-ish behavior is to check for org.gnome.Shell on the bus)
Commit 7115c1f makes rhythmbox create a notification on startup when running in the shell. What else needs to be done here?
Ping. Jon, is the rhythmbox behaviour the one you are expecting?
For Shell: * The keynav focus style on buttons is really wrong. * don't show notifications when an app that sends them is focused * don't show a banner notification when source's controls are used For Rhythmbox: * use "Rhythmbox" rather than "Music Player" everywhere * don't add "(Paused)" in notification title We should think about how to include artwork at some point but it isn't a blocker.
Rhythmbox parts are done, commits 006d2ff and 1a182ed.
Marina: another thing that is kinda ugly is that when I click play or pause in the notifications focus blinks back to the app for a moment.
Created attachment 181964 [details] [review] MessageTray: keep a resident notification focused if its action is invoked Processing 'clicked' for St.Button returns the focus to the stage, so FocusGrabber::_focusActorChanged() is called. This resulted in us ungrabbing the focus, unlocking the message tray, and hiding it. However, we should grab the focus back if the notification that was interacted with is resident, so that we can keep it showing. Jon, this fixes showing the banner and focus blinking back to the app.
Comment on attachment 181964 [details] [review] MessageTray: keep a resident notification focused if its action is invoked I'm 75% through a rewrite/merger of PopupMenuManager and FocusGrabber to make it possible for summary area keynav to work. I'll make sure this bug gets fixed in the final patch.
The patches on bug 643687 should also fix the blinking
[Removing GNOME3.0 target as decided in release-team meeting on March 03, 2011. "nice-to-have" categorisation for GNOME3.0]
Created attachment 184000 [details] [review] NotificationDaemon: don't notify when application is focused We ignore non-resident notifications that are being sent by an application while its window is focused because this should not happen.We update the resident notifications because the user should be able to access them later from the summary,but don't display them to the user.
Comment on attachment 184000 [details] [review] NotificationDaemon: don't notify when application is focused I know that this makes sense in the context of Rhythmbox, but does it make sense in general? Eg, if I am composing mail in Evolution, but the main evolution window is not visible, I might still want to see "new mail" notifications. It seems like this is something that could be better handled by the app itself. (Although, I guess, in the case of a resident notification, it needs to update the notification so that it will show the correct song if the user looks at it later...) Anyway, the code looks like it does what it says, and renaming NotificationDaemon.Source.notify() to something else is probably a good idea since it's not quite the same as the MessageTray.Source method that it's overriding.
Created attachment 184023 [details] [review] NotificationDaemon: don't notify for resident notifications when application is focused The applications have to have a way of keeping resident notifications updated without unnecessarily notifying the user with the information the user is already seeing in the application window.
Comment on attachment 184023 [details] [review] NotificationDaemon: don't notify for resident notifications when application is focused >+ if (tracker.focus_app && tracker.focus_app == this.app && notification.resident) it's marginally more efficient to do: if (notification.resident && this.app && tracker.focus_app == this.app) because JS properties (notification.resident, this.app) can be retrieved faster than GObject properties (tracker.focus_app). Good too commit with that change.
Comment on attachment 184023 [details] [review] NotificationDaemon: don't notify for resident notifications when application is focused Attachment 184023 [details] got pushed - anything left on this bug?
no, the rest is going to be dealt with as part of bug 643687