GNOME Bugzilla – Bug 542979
freedesktop sound theme support
Last modified: 2008-07-28 20:03:12 UTC
The sound theme tab in the sound capplet needs to be changed in accordance with the new freedesktop sound theme specification and the sound naming specification (http://lists.freedesktop.org/archives/xdg/2008-February/009242.html). In addition to that, sound events should be played using libcanberra instead of the current gstreamer thing that doesn't actually represent what esound is doing.
Created attachment 114609 [details] [review] First patch Handles setting the theme, whether to enable event sounds, and input feedback sounds. I still need to implement the customisation of themes, as well as the builtin bell support.
Created attachment 114610 [details] with no themes installed
Created attachment 114611 [details] with a theme installed that overrides details from the system-wide default
Created attachment 114733 [details] [review] patch #2 Still need to implement the all important theme saving, as well as the drop-down selection in the second column.
Created attachment 115031 [details] [review] patch #4 - Does nice popups in the second column, and sets filenames etc. as expected - Disables input feedback sound rows when disabled (still needs to look more "disabled" but I'll leave that to another bug, marked as gnome-love) - Supports audible bell and visual bell settings, removes the 3rd tab Only thing left to do is actually save the customised theme to the disk. Lennart?
Created attachment 115033 [details] updated screenie
Created attachment 115110 [details] [review] patch #5 This is the final version I'll be making. The code in most of the file helpers (sound-theme-file-utils.[ch] and get_file_type()) is utter crap, I know it but I'm sick of this code :) It behaves as expected when reloading with a customised theme. Please test thouroughly.
Huh, I forgot to implement previews... Whoop!
Created attachment 115184 [details] [review] Patch #6, now with lazy previews
Created attachment 115187 [details] [review] Patch #7, with cleanups Cleanups of a few portions of the code, added some comments, fixed a leak. IMO, this is ready for testing and commit. If Jens is happy with it, I can ask for the necessary approval from the release-team/i18n teams.
Created attachment 115212 [details] [review] Patch #8, applies cleanly against current SVN trunk
+PKG_CHECK_MODULES(CANBERRA, libcanberra-gtk gio-2.0, have_canberra=yes, AC_MSG_RESULT([no])) s/AC_MSG_RESULT([no])/have_canberra=no/ Otherwise you'll just print "no" twice. + //FIXME to remove + gtk_notebook_set_current_page (GTK_NOTEBOOK(WID("prefs_widget")), 1); ... - <property name="pulse_step">0.05</property> + <property name="pulse_step">0.0500000007451</property> Erm? In general, the glade part seems to have a number of chunks which are just swapping lines around. Please try to cut those out. +theme_changed_cb (GConfClient *client, + guint cnxn_id, + GConfEntry *entry, + GladeXML *dialog) +{ + char *theme_name; + + theme_name = gconf_client_get_string (client, SOUND_THEME_KEY, NULL); + set_combox_for_theme_name (dialog, theme_name); Leaking theme_name here. Also, please turn g_message into g_debug if you don't want to remove those calls, yet. One question regarding the UI: What does "Play system beep sound" mean? I'm still puzzling over that one. Otherwise, go ahead. We can iron out the kinks once it's in.
Btw, the appearance capplet has code to recursiely delete files/directories in theme-util.c. We should move that to common/ it seems.
Moved the directory removal helper functions to common/, and fixed the few problems mentioned above. As mentioned by Matthias in: https://bugzilla.redhat.com/show_bug.cgi?id=456689 I also: - added a real preview button - made which sounds are disabled much clearer - fixed warnings created by metacity for the visual bell setting I also filed a few follow-up bugs (bug 545055 and bug 545056) 2008-07-28 Bastien Nocera <hadess@hadess.net> * Makefile.am: * configure.in: Remove libsounds and esound usage, check for libcanberra instead (Closes: #542979) 2008-07-28 Bastien Nocera <hadess@hadess.net> * theme-util.c: * theme-util.h: Remove the directory deletion helpers, and move them to the common sub-directory 2008-07-28 Bastien Nocera <hadess@hadess.net> * Makefile.am: * capplet-util.c (directory_delete_recursive), (capplet_file_delete_recursive): * capplet-util.h: Move directory deletion helper function from the appearance capplet into a common directory 2008-07-28 Bastien Nocera <hadess@hadess.net> * Makefile.am: * sound-properties-capplet.c (create_dialog), (setup_dialog), (get_legacy_settings): * sound-properties.glade: * sound-theme-definition.h: * sound-theme-file-utils.[ch]: * sound-theme.[ch]: Remove separate bell settings tab, remove libsounds dependency, add freedesktop sound theme support through libcanberra (Closes: #542979)
This looks great! I have just a few recommended minor HIG fixes. 1. Category headings should use title capitalization, so "Sound theme" should read "Sound Theme" and "System sounds" should read "System Sounds". 2. There is a "P" mnemonic conflict. See "_Play system sounds" and "_Play system beep sound". 3. There is no mnemonic for "Play sounds when buttons are pressed".
This looks great indeed, can we get it in trunk and just start fixing bugs there, so it gets as much testing as possible, or is there anything else missing?
NEW bugs. This bug is closed because the code is committed to trunk.
Yeah sorry, just realised now, so discard my comment :-)
(In reply to comment #15) > 1. Category headings should use title capitalization, so "Sound theme" should > read "Sound Theme" and "System sounds" should read "System Sounds". > 2. There is a "P" mnemonic conflict. See "_Play system sounds" and "_Play > system beep sound". > 3. There is no mnemonic for "Play sounds when buttons are pressed". Feel up to fixing these yourself?