GNOME Bugzilla – Bug 575008
Should play a sound when calling
Last modified: 2009-06-26 16:29:09 UTC
When you call someone, empathy should play a phone song while the call is ringing on the other side.
Is there a name for that sound in the fd.o sound naming spec?
As per http://0pointer.de/public/sound-naming-spec.html , "phone-incoming-call" is what you are looking for.
Wrong answer, phone-outgoing-calling.
Ah I read this bug all the way around.
It should also be noted that this sound is highly locale specific... I don't know how the sound spec deals with that.
I agree we should just play that sound. We already do for incoming sounds. Let's set the gnome-love keyword, it's an easy bug for beginners :) @Pierre-Luc: I supposes in the future you could use something like "gnome-sound-theme-fr".
Created attachment 136840 [details] [review] The phone-outgoing-calling sound seems not to be included by default on most installations. Branch containing the fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/play-sound-when-calling src/empathy-call-window.c | 65 +++++++++++++++++-------- src/empathy-event-manager.c | 110 +++++++++++++++++++++++++++++++------------ src/empathy-event-manager.h | 5 ++- 3 files changed, 127 insertions(+), 53 deletions(-)
I didn't read your patch totally, but as a first cleanup, I think we should move sound related functions to empathy-sound.c,h. I's bad to use empathy-ui-utils for such thing I think.
I'd reorder the CONNECTED and CONNECTING states to make them more logic. You could use a typdef enum CallState. empathy_call_window_set_state_connecting won't be called with incoming call too? I prefer to let Xavier review the event-manager part.
Created attachment 137012 [details] [review] Fixed and created a new sound module. libempathy-gtk/Makefile.am | 2 + libempathy-gtk/empathy-sound.c | 276 +++++++++++++++++++++++++++++++++++++ libempathy-gtk/empathy-sound.h | 57 ++++++++ libempathy-gtk/empathy-ui-utils.c | 139 +------------------ libempathy-gtk/empathy-ui-utils.h | 26 ---- po/POTFILES.in | 1 + src/empathy-call-window.c | 89 +++++++++---- src/empathy-call-window.h | 3 +- src/empathy-chat-window.c | 1 + src/empathy-event-manager.c | 135 ++++++++++-------- src/empathy-event-manager.h | 5 +- src/empathy-main-window.c | 11 +- src/empathy.c | 2 +- 13 files changed, 490 insertions(+), 257 deletions(-)
Created attachment 137024 [details] [review] Addressed problems pointed out by Xavier. See commit message for details. libempathy-gtk/Makefile.am | 2 + libempathy-gtk/empathy-sound.c | 306 +++++++++++++++++++++++++++++++++++++ libempathy-gtk/empathy-sound.h | 56 +++++++ libempathy-gtk/empathy-ui-utils.c | 139 +----------------- libempathy-gtk/empathy-ui-utils.h | 26 --- po/POTFILES.in | 1 + src/empathy-call-window.c | 89 ++++++++--- src/empathy-call-window.h | 3 +- src/empathy-chat-window.c | 1 + src/empathy-event-manager.c | 125 ++++++++-------- src/empathy-event-manager.h | 5 +- src/empathy-main-window.c | 11 +- src/empathy.c | 2 +- 13 files changed, 509 insertions(+), 257 deletions(-)
Thanks. Some additionnal comments: 1) You should reorder functions to not declare playing_finished_cb on top. 2) Remove timeout_before_replay from the doc of empathy_sound_stop. 3) You should add a destroy func for the value inserted in repeating_sounds hash table. That function will remove the source if any and free the struct. Like that in empathy_sound_stop you can simply call g_hash_table_remove(repeating_sounds, GINT_TO_POINTER (sound_id)) 4) I don't think you have to free the hash table if it gets empty, it's fine to leak such thing. Or you can add a g_atexit() if you want. 5) doc of empathy_sound_play_full says it's for empathy_sound_play 6) Why are you using g_hash_table_lookup_extended()? g_hash_table_lookup seems enough... 7) There is a typo in the name event_soure_id, it is 'source' no? What about renaming it to replay_timeout_id? 8) You should connect "destroy" signal on widget in empathy_sound_start_playing() to stop the sound if the widget is destroyed. 9) empathy_sound_start_playing should first check if it's already in the hash table and return if the sound is already being played in loop. 10) _start_playing() should check for the gconf key to avoid entering the loop if that sound is disabled. 11) if empathy_sound_stop is called when the sound is being played, playing_finished_cb() will be called and the sound will get played again. 12) if you call empathy_sound_play_full when a repeated sound is being played, event_source_id will be 0 and the sound being played will be cancelled. Which will remove the sound from hash table if you fix point 11. I think we should instead have empathy_sound_play_internal() private function that will be used by empathy_sound_play_full and from playing_timeout_cb. Like that in empathy_sound_play_full you can check if the sound is in hash table and return directly or call empathy_sound_play_internal if it's not found.
I've just updated my branch. I have a question however: > 4) I don't think you have to free the hash table if it gets empty, it's fine to > leak such thing. Or you can add a g_atexit() if you want. Why is it OK to leak such thing?
Created attachment 137247 [details] [review] fixed libempathy-gtk/Makefile.am | 2 + libempathy-gtk/empathy-sound.c | 353 +++++++++++++++++++++++++++++++++++++ libempathy-gtk/empathy-sound.h | 56 ++++++ libempathy-gtk/empathy-ui-utils.c | 139 +--------------- libempathy-gtk/empathy-ui-utils.h | 26 --- po/POTFILES.in | 1 + src/empathy-call-window.c | 89 +++++++--- src/empathy-call-window.h | 3 +- src/empathy-chat-window.c | 1 + src/empathy-event-manager.c | 125 +++++++------- src/empathy-event-manager.h | 5 +- src/empathy-main-window.c | 11 +- src/empathy.c | 2 +- 13 files changed, 556 insertions(+), 257 deletions(-)
(In reply to comment #13) > Why is it OK to leak such thing? Because it's a static global variable you only create at most once during the lifetime of the program. The OS will release the memory for you at exit, if it has been allocated, and you save the (small) overhead of creating and freeing it every time you want to play a repeating sound.
I simplified the patch and pushed. Thanks for your great help!