GNOME Bugzilla – Bug 642831
Support sound in NotificationDaemon
Last modified: 2013-02-14 17:55:11 UTC
See patch.
Created attachment 181424 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra.
Not sure this is actually wanted
(In reply to comment #2) > Not sure this is actually wanted Well, consider that clients will play sounds anyway, if we don't expose the sound capability. At least with this we can enforce themeability (using libcanberra even for KDE/nonstandard apps) and you can get some control, like playing sound only when the notification is actually shown (not it this patch, though)
I kinda think that clients playing the sound is broken. A sound that is disconnected from the notification is pretty weird. Particularly when we have queued messages. You could have a sound played for a message that hasn't been seen yet. That stinks. As for the shell playing the sounds. I'm not sure how this fits in with the libcanberra auxiliary data story. And I'm not sure if it makes sense to allow any client to decide what to play. Do we know what is currently using sound hints? We need to be careful here because injudicious use of sound is in direct conflict with one our primary design goals - unobtrusive notifications.
(In reply to comment #4) > I kinda think that clients playing the sound is broken. A sound that is > disconnected from the notification is pretty weird. Particularly when we have > queued messages. You could have a sound played for a message that hasn't been > seen yet. That stinks. Actually, my patch (which focused on the Empathy case, so needed to handle this before the Notify call is turned into a Notification object) doesn't do that, but I could rewrite it. I'm not sure if sound should be played when the notification is shown, or instead when the event which originated the notification (for example, the phone call) actually happened. > > As for the shell playing the sounds. I'm not sure how this fits in with the > libcanberra auxiliary data story. And I'm not sure if it makes sense to allow > any client to decide what to play. Do we know what is currently using sound > hints? AFAIK, Evolution, PackageKit and Empathy are. (Empathy also allows to choose if sound is enabled) > > We need to be careful here because injudicious use of sound is in direct > conflict with one our primary design goals - unobtrusive notifications. Well, some notifications just need sound (calls for example), and in general, I think that all urgent notifications should have some kind of sound hint, or they risk to be too unobtrusive.
Backing up. Having the sound notification and the visual notification be disconnected and not use the same logic for when they are presented seems undesirable. Having the notification system present notifications (even sound ones) makes sense to me. This probably also implies supporting visual sound representations for the hearing impaired. We need to understand a bit more about the implications of not using the expressive canberra API directly from client apps. We may need to enhance the notification hints based on the above. We need to understand a bit more about what clients are using sounds for. Modulo the findings from the above I think it may be reasonable to only honor client provided sounds for urgent notifications. Which would be alarms, calls, etc.
Created attachment 183285 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra.
(In reply to comment #6) > Backing up. > > Having the sound notification and the visual notification be disconnected and > not use the same logic for when they are presented seems undesirable. I rewrote the patch, and now sound is played when the notification is first shown as a banner. > This probably also implies supporting visual sound representations for the > hearing impaired. Isn't the notification itself, being mostly visual, enough? We're not going to add any message in sound, more than "hey look down your screen, you have messages". > We need to understand a bit more about the implications of not using the > expressive canberra API directly from client apps. Part of the Canberra (and PulseAudio) API is for expressing metadata about the played file. Since most of the time it will be a standard bell, I don't think we need any of that. Part of the API deals with X11, events, windows and timestamps, as well as application ids, names and descriptions. libnotify already has relevant hints there. Useful could be canberra.cache-control, canberra.volume, canberra.xdg-theme.name and canberra.xdg-theme.output-profile, but I think they're only used in corner cases, and I don't want to clutter the API more than necessary. > We may need to enhance the notification hints based on the above. Maybe add more generally useful properties about the event that triggered the notification, like event-mouse-x, event-timestamp, event-window-x11 and the like. > We need to understand a bit more about what clients are using sounds for. To attract the attention of users, I suppose. > Modulo the findings from the above I think it may be reasonable to only honor > client provided sounds for urgent notifications. Which would be alarms, calls, > etc. Why only urgent notifications? (And would that be complaint with the specification?) If an application wants sound, it should get sounds, and if that annoys the user, you file a bug at the app (or stop using it).
(In reply to comment #8) > > We need to understand a bit more about what clients are using sounds for. > > To attract the attention of users, I suppose. We should compile a list of what sounds the shell will start emitting if we change this. It seems like some clients are playing sounds on their own as well. We'd need to track those down so two sounds aren't played. > > Modulo the findings from the above I think it may be reasonable to only honor > > client provided sounds for urgent notifications. Which would be alarms, calls, > > etc. > > Why only urgent notifications? (And would that be complaint with the > specification?) If an application wants sound, it should get sounds, and if > that annoys the user, you file a bug at the app (or stop using it). Applications aren't in charge. The spec allows application authors to represent what they would think should happen. It is our job to try to put the user first. I am not comfortable making these sort of changes without understanding more about the problem. I will try to look into this in detail after 3.0. We would also need to make the equivalent changes to the fallback mode.
No.
No -> maybe? Reopening for bug 685926.
Created attachment 228191 [details] [review] ShellGlobal: improve code to emit sound events Use libcanberra-gtk3 and improve the set of context properties to correctly associate the sounds with the shell.
Created attachment 228192 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra.
Anybody willing to review this ?
Review of attachment 228191 [details] [review]: ::: src/shell-global.c @@ +271,3 @@ + global->sound_context = ca_gtk_context_get (); + ca_context_change_props (global->sound_context, + CA_PROP_APPLICATION_NAME, "GNOME Shell", Can we keep using PACKAGE_NAME? Why do you need to set ICON_NAME and LANGUAGE? @@ +1613,3 @@ + ca_proplist_sets (props, CA_PROP_EVENT_ID, event_id); + ca_proplist_sets (props, CA_PROP_EVENT_DESCRIPTION, event_description); + const char *event_description, Why is this needed now? @@ +1634,3 @@ + + button = clutter_event_get_button (for_event); + { How does this information get used, if at all?
Review of attachment 228192 [details] [review]: ::: js/ui/messageTray.js @@ +883,3 @@ }, + notify: function() { I don't particularly like the name of this function, we use notify() in other places with different meanings than "play the sound". @@ +885,3 @@ + notify: function() { + if (!this.source.policy.enableSound) + return; Is this set anywhere? I can't find any references for enableSound in git master. ::: src/shell-global.h @@ +144,3 @@ + ClutterEvent *for_event, + const char *application_id, +void shell_global_play_sound_file_full (ShellGlobal *global, How about merging these two together and pass a GAppInfo directly?
(In reply to comment #15) > Review of attachment 228191 [details] [review]: > > ::: src/shell-global.c > @@ +271,3 @@ > + global->sound_context = ca_gtk_context_get (); > + ca_context_change_props (global->sound_context, > + CA_PROP_APPLICATION_NAME, "GNOME Shell", > > Can we keep using PACKAGE_NAME? Why do you need to set ICON_NAME and LANGUAGE? > > @@ +1613,3 @@ > + ca_proplist_sets (props, CA_PROP_EVENT_ID, event_id); > + ca_proplist_sets (props, CA_PROP_EVENT_DESCRIPTION, event_description); > + const char *event_description, > > Why is this needed now? Uhm, I just went through the list of properties canberra exports, and picked those that make sense. I think at least EVENT_DESCRIPTION is used by a11y, but I don't know for sure. > @@ +1634,3 @@ > + > + button = clutter_event_get_button (for_event); > + { > > How does this information get used, if at all? Docs say that the event coordinates may be used for surround effects (so the sound appears to be closer to the action that triggered it). I don't know if that's really implemented in PulseAudio. (In reply to comment #16) > Review of attachment 228192 [details] [review]: > > ::: js/ui/messageTray.js > @@ +883,3 @@ > }, > > + notify: function() { > > I don't particularly like the name of this function, we use notify() in other > places with different meanings than "play the sound". It is not "play the sound", it is, "make sure the user is aware of this Notification". It looks somehow natural to me for Source.notify() to call Notification.notify(). > @@ +885,3 @@ > + notify: function() { > + if (!this.source.policy.enableSound) > + return; > > Is this set anywhere? I can't find any references for enableSound in git > master. Look at the dependent bug. > ::: src/shell-global.h > @@ +144,3 @@ > + ClutterEvent *for_event, > + const char *application_id, > +void shell_global_play_sound_file_full (ShellGlobal *global, > > How about merging these two together and pass a GAppInfo directly? What if I don't have a GAppInfo (for example if application_name comes from org.freedesktop.Notifications)?
(In reply to comment #17) > Uhm, I just went through the list of properties canberra exports, and picked > those that make sense. > I think at least EVENT_DESCRIPTION is used by a11y, but I don't know for sure. > Docs say that the event coordinates may be used for surround effects (so the > sound appears to be closer to the action that triggered it). I don't know if > that's really implemented in PulseAudio. Okay. > It is not "play the sound", it is, "make sure the user is aware of this > Notification". It looks somehow natural to me for Source.notify() to call > Notification.notify(). But that's not what the function does, it plays a sound. Since the code that does the actual notification on screen is separate, it made sense to me to be explicit with the naming here. > > Is this set anywhere? I can't find any references for enableSound in git > > master. > > Look at the dependent bug. Ah okay. > What if I don't have a GAppInfo (for example if application_name comes from > org.freedesktop.Notifications)? You could create one with Gio.DesktopAppInfo.new() - but I don't see this ever happening in the code?
the panel has been merged now, would be nice to get this in
Created attachment 232128 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra. .notify() changed to .playSound(), which is clearer. I'm not chaning shell_global_play_theme_sound(), because it is possible to have a null application_id but a non null application_name, and that cannot be covered with a GAppInfo.
(In reply to comment #17) > Docs say that the event coordinates may be used for surround effects (so the > sound appears to be closer to the action that triggered it). I don't know if > that's really implemented in PulseAudio. It is.
Shall we get this in ?
ping again, would be nice to merge this, so the notification panel starts to make sense
Created attachment 233825 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra. Rebased to master.
Jasper, can you review this, please ?
I'll review this, but I'm curious about the design part of it. Has it passed the initial Jon review?
Review of attachment 228191 [details] [review]: One minor nit, otherwise fine. ::: src/shell-global.c @@ +1624,3 @@ + clutter_event_get_position (for_event, &point); + + ca_proplist_setf (props, CA_PROP_EVENT_MOUSE_X, "%i", (int)point.x); Use "%d", not "%i".
(In reply to comment #26) > I'll review this, but I'm curious about the design part of it. Has it passed > the initial Jon review? Jon put the 'sound' switch in his notification panel design, so I think we can be fairly confident that he expected sound in notifications to work.
Comment on attachment 228191 [details] [review] ShellGlobal: improve code to emit sound events Attachment 228191 [details] pushed as 427750d - ShellGlobal: improve code to emit sound events Please review the notification policy bug before I can push the other patch, thank you!
Review of attachment 233825 [details] [review]: ::: js/ui/messageTray.js @@ +1243,3 @@ + if (!this.isMuted && this.policy.showBanners) { + notification.playSound(); Just double-checked with Jon. If we have Show Banners turned off, we should play the sound as soon as it arrives in the shell.
how about it, then ?
ping ?
I'm not sure what ui-review is needed here. But since I haven't tried the patch I'll just make some general points. * We should ensure we never get duplicate sound events * We should ensure that the user's sound on/off setting is enforced/honored * We should try to provide enough capability for Clocks to use the notification system for alarms instead of doing something out of band. Are there any specific user experience questions?
Yes, one: should the sounds be played when the notification is generated, or when the notification is shown? Ie., what to do when the user is idle or interacting with other notifications?
(In reply to comment #34) > Yes, one: should the sounds be played when the notification is generated, or > when the notification is shown? Isn't the whole point of using a notification hint (instead of the app playing the sound itself) to get the latter behavior?
Well, the point is also having one central point to configure sound for notifications, and in general (think of calls) it makes sense to play the sound immediately. Although I agree that apps can just watch the GSettings themselves, and ignore the hint altogether in that case.
If a notification calls Update, we should not re-play the sound unless it changed. I'm not sure what notify-osd or notification-daemon do. If a notification arrives while another one is queued, or something is stopping banners (modal dialog, message tray), it should be delayed until the banner is viewed. If notification banners are turned off and sound is turned on, sound should happen when the Notify method call arrives.
Created attachment 236058 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra.
Created attachment 236059 [details] [review] NotificationDaemon: support sound in notifications The notifications spec has two hints for playing a sound, sound-file and sound-name. We can support them using the existing code that wraps libcanberra.
For the record, neither notify-osd nor notification-daemon support sound hints.
Review of attachment 236059 [details] [review]: Minor nit, otherwise OK. ::: js/ui/messageTray.js @@ +1284,3 @@ + if (!this.isMuted) { + // Play the sound now, if banners are enabled. if banners are *disabled*
Attachment 236059 [details] pushed as c30661c - NotificationDaemon: support sound in notifications