Wouldn't this be part of a more general "sound notification" issue ?

Empathy had sound-related options in its preferences dialog but they were removed as they were not wired (see bug 479697).

Can I suggest using libcanberra? Cooking a patch is trivial, if there's no opposition I will attach one.

I guess there will be no problem but you should probably wait for release team approval of libcanberra as external dependency (see desktop-devel-list@).

Well yes, no rush to include it. Still the patch is *really* trivial. canberra turns out to be really easy to use. So as an argument in favour of it for approval I'll add a patch, or at least a proof of concept for incoming messages.

:)

I have a 'sound' branch on my git repository: http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/sound

I think you can add sounds for calls based on that code.

Created attachment 114780 [details] [review]
20080718_bgo_528929_canberra-empathy.diff

Adds libcanberra to configure.ac and proof of concept for messages and calls, but it's still very buggy due to my lack of knowledge of empathy's code.

Didn't see Xavier's branch yet.

Changing the title.

Xavier, I think it makes sense to keep empathy_sound_play() from your branch, and make the gconf checking there, I'll update the current patch. libcanberra does not require anything else thankfully.

Created attachment 114785 [details] [review]
20080718_bgo_528929_canberra-empathy.diff: v2

Updated to use Xavier's empathy-sound api.

Could you rather make a branch based on my branch so I can merge you patch into my branch? :D

Evil git. Ok i'll try later.

you don't have to use git, just attach a patch based on my branch instead of including the whole code. checkout my branch, make your changes and "git diff > my.path"

thanks :)

Created attachment 114865 [details] [review]
20080720_bgo_528929_canberra+empathy+git.diff: v3

Patch from git, using Xavier's sound branch as base.

Note: this is missing CA_PROP_EVENT_DESCRIPTION and /not sure/ if i18n in the other string props. Anyway, the base should be ok.

I pushed a git branch/tree/repo/whatever to gitorious: http://gitorious.org/projects/empathy-canberra/repos/mainline

I guess that's useful.

Created attachment 116108 [details] [review]
My version of the patch

Argh, didn't check if the bug had a patch before I got started. Anyway, here's another patch. This one does not have the empathy-sound structure so I guess the earlier one is better but it shows how to correctly handle outgoing messages and also plays sounds for going online/offline as per freedesktop sound spec (but it does not handle voip/phone sounds so you better merge these two ;])

Also to comment on the Diego's patch - we no longer need a preference to enable/disable certain sounds as it's now globally handled by gnome-control-center's sound preferences, we should just ask them to include the IM sounds in the g-c-c UI (as they are already part of the specification). A preference to mute sounds in DND could be useful though.

We don't actually plan on adding any new sounds, as those should cover most uses:
http://svn.gnome.org/viewvc/gnome-control-center/trunk/capplets/sound/sound-theme-definition.h?view=markup

You can install your own sounds in the freedesktop directory, as you would with icon themes. To talk over with Lennart is:
message-sent-instant being an action event
message-new-instant not being an action event

message-new-instant when action sounds are disabled should only trigger if the window of the dialogue isn't visible, whereas it should be rung out when receiving a message whenever a message is received otherwise.

Bastien:

What exactly do you mean by "when action sounds are disabled"? Is there some gconf key to check or is it something yet to be implemented? I'd be glad to come with a proper patch.

There's a GConf key:
/desktop/gnome/sound/input_feedback_sounds

Roughly:
if (input_feedback_sounds_enabled != FALSE) {
   generate_new_message_sound();
} else {
   if (dialog_window_is_visible() == FALSE)
     generate_new_message_sound();
}

Created attachment 116133 [details] [review]
Properly handle enabled/disabled input sounds, add voip event sounds

Here's another patch. It does not bring the whole empathy-sound.c from the sound branch in as it's a bit of an overkill for a single function call. Works out of the box and honors GNOME sound settings.

Bastien:

Could you please take a look at the patch too? I think I got your intentions right but a review will certainly do no harm :)

You're missing the application name (CA_PROP_APPLICATION_NAME, use g_get_application_name()) and event description (CA_PROP_EVENT_DESCRIPTION) in the calls to ca_gtk_play_for_widget().

Outgoing and incoming phone calls should always make sounds.

Rest looks alright apart from spurious tabs and C++-style comments.

Bastien:

I'll fix the comments and tabs but for the CA_* props it would be easier if you pointed me to a document about the intended usage. For example which of these are mandatory (from the code it seems only the envent id or file name) and what are apps supposed to put there. Like do I use "Empathyy" or "Instant Messager" for the APPLICATION_NAME and what to put in the EVENT_DESCRIPTION. Are these meant for ATK? Ar they presented somewhere in the GUI?

Use the g_get_application_name output for CA_PROP_APPLICATION_NAME, and event description is indeed for ATK, so needs to be translated. See:
http://bugzilla.gnome.org/show_bug.cgi?id=542693

Created attachment 116166 [details] [review]
Fixed patch

Bastien:

As per review - fixed the calls to include descriptions. Could not find spurious tabs - tried to retain the the original formatting which is quite different from file to file. Also fixed the C++ comment to use /**/.

There's your spurious tab:
 	DEBUG ("Start flashing");
+	
 	window->flash_timeout_id = g_timeout_add (FLASH_TIMEOUT,

Rest looks fine to me.

Created attachment 116173 [details] [review]
Fixed fixed patch

Actually it pays to compile the same tree you are modifying. You get to find syntax errors. Stupid me.

Awesome! I quickly looked at the patch and it seems to be what we need. I agree that libcanberra is cool enough to remove that EmpathySound stuff.

I have a question about the patch: You create a ctx var all the time be never use it. Is it needed?

Created attachment 116175 [details] [review]
Drop the spurious empty line

Same as previous but without the line Bastien mentioned.

Looking at the code of canberra, asking for gtk ctx ensures canberra is properly initialized.

I think you don't need the context. ca_gtk_play_for_widget is meant to save you that work.

I checked the source code, ca_gtk_play_for_widget() does

ret = ca_context_play_full(ca_gtk_context_get(), id, p, NULL, NULL);

So you can remove all those ca_gtk_context_get() from the patch ;)

Created attachment 116176 [details] [review]
Drop the ctx calls

Here ya go :)

Please note that you don't have to specificy CA_PROP_APPLICATION_NAME each time. libcanberra-gtk fills in the data from g_get_application_name() anyway the first time it is used. Otherwise the patch looks fine to me!

Ok so we can remove all CA_PROP_APPLICATION_NAME from the patch.

I don't think it's useful to have a sound when I send a message, only when receiving a message is useful.

I would like to revisit sound preferences too. Empathy inherit gossip's gconf keys for sound:

EMPATHY_PREFS_SOUNDS_FOR_MESSAGES
EMPATHY_PREFS_SOUNDS_WHEN_AWAY
EMPATHY_PREFS_SOUNDS_WHEN_BUSY

I think it's a good idea to be able to disable sound when busy/away, when I'm working on my computer I don't want to be disturbed with sound notifications. I don't think it's useful to disable message sounds explicitely. I think some sounds are not concerned by this option, for example I want phone-outgoing-calling to be played even if I'm busy.

Any thoughs about this?

Dropping CA_PROP_APPLICATION_NAME is easy, I can prepare a new patch.

Just a note:

EMPATHY_PREFS_SOUNDS_FOR_MESSAGES represents the global GNOME "input related sounds" preference and if enabled should result in playing sounds for outgoing messages too (it also enables sounds for button clicks, window maximization etc.).

I also think that in AWAY/BUSY incoming chat (as opposed to incoming message) should still be audible. AWAY/BUSY shouldn't affect sounds caused by user interaction (only those caused by incoming events).

I marked the patch as need-work because we should fix those config problems first. But the basics of the patch seems good.

1) Never play sound when I sent a message. Pidgin does that and it's totally stupid.

2) Add an option to disable sound when receiving a message if away/busy. New conversation should still be audible.

3) Possibly new conversation could have a different sound than new message on an existing conversation? With an option to have only sound for new conversation and not new message. Note that sound for new events can be added in EmpathyEventManager instead of in main_window_flash_start.

4) Drop CA_PROP_APPLICATION_NAME.

*** Bug 558609 has been marked as a duplicate of this bug. ***

Xavier, I am planning to work on this; some comments before start coding:

- do you think an option to globally disable sounds is needed? Personally, sounds are the first thing I turn off in any IM client.
- would that disable also incoming/outgoing phone call requests?
- I am asking because currently, when it comes to IM, gnome-control-center does not allow us to specify which sounds are to be played and which aren't.
- so, in the end, given that Empathy is the official GNOME IM client, isn't it better if we provide in the Empathy preferences a tab like the one which lives in gnome-control-center for the sound theme, where you can enable/disable sounds per-event, together with two options: Enable sounds and Disable when not available (I don't think it makes much sense to have two separate options for away/busy)

Let me know what you think. I've also CC-ed Matthew for some usability feedback.

(In reply to comment #37)
> Xavier, I am planning to work on this; some comments before start coding:
> 
> - do you think an option to globally disable sounds is needed? Personally,
> sounds are the first thing I turn off in any IM client.

yes, a global "disable sound" setting is fine.

> - would that disable also incoming/outgoing phone call requests?

IMO, sound for in/out call should never be disabled.

> - so, in the end, given that Empathy is the official GNOME IM client, isn't it
> better if we provide in the Empathy preferences a tab like the one which lives
> in gnome-control-center for the sound theme, where you can enable/disable
> sounds per-event, together with two options: Enable sounds and Disable when not
> available (I don't think it makes much sense to have two separate options for
> away/busy)

Hm, and what about patching directly gnome-control-center to provide IM sound? So for each event we can have default/custom/disabled. I agree that we need an option to disable sound when not available (no need to distinguish between away/ext-away/busy/etc...). Hidden could be considered as available or busy, I'm not sure...

(In reply to comment #38)

> Hm, and what about patching directly gnome-control-center to provide IM sound?
> So for each event we can have default/custom/disabled. I agree that we need an
> option to disable sound when not available (no need to distinguish between
> away/ext-away/busy/etc...). Hidden could be considered as available or busy,
> I'm not sure...

Yeah, I think disabling sounds also makes sense for the "Hidden" state.
After a chat with Bastien on IRC, I think this plan could serve all purposes:
- have a global "disable sound" option
- have a "disable sound when away" option
- provide a granular way of enabling/disabling these single sound events: new conversations, new messages, incoming file transfers, service login/logout (not sure about incoming/outgoing phone calls). IMHO it doesn't make sense to provide a way to also customize the sound effect but the user should at least be able to enable/disable it.

If that's fine for you, I will start implementing this ASAP.

Fine for me, we can do that now, and let the user change settings in g-c-c later if it makes sense.

I pushed a branch for review implementing these ideas here.

http://git.collabora.co.uk/?p=user/cosimoc/empathy.git;a=shortlog;h=refs/heads/canberra2

Forgot to mention that this is still missing the notifications for buddies logging in/out (though there's already the UI to configure them).
I think I should connect to |members_changed| on |list_iface| from inside empathy-main-window.c, but still have to find a way to skip the signal flood when first connecting to a crowded network. Any ideas?

There is no real solution for that, the problem is we get the contact list asyncronously so it is more complicate to know when we get the first batch of contacts.

I think we should add a method on EmpathyContactManager like:

gboolean
empathy_contact_manager_is_account_sending_first_batch_of_contact(manager, account);

Of course with a better name.

This has been merged in trunk for 2.25.4; closing as FIXED.