After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 542979 - freedesktop sound theme support
freedesktop sound theme support
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Sound
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-07-14 20:30 UTC by Jens Granseuer
Modified: 2008-07-28 20:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
First patch (27.98 KB, patch)
2008-07-15 17:12 UTC, Bastien Nocera
needs-work Details | Review
with no themes installed (16.56 KB, image/png)
2008-07-15 17:13 UTC, Bastien Nocera
  Details
with a theme installed that overrides details from the system-wide default (17.07 KB, image/png)
2008-07-15 17:13 UTC, Bastien Nocera
  Details
patch #2 (32.91 KB, patch)
2008-07-17 21:47 UTC, Bastien Nocera
needs-work Details | Review
patch #4 (54.29 KB, patch)
2008-07-22 17:18 UTC, Bastien Nocera
needs-work Details | Review
updated screenie (38.14 KB, image/jpeg)
2008-07-22 17:21 UTC, Bastien Nocera
  Details
patch #5 (68.42 KB, patch)
2008-07-23 17:50 UTC, Bastien Nocera
none Details | Review
Patch #6, now with lazy previews (69.30 KB, patch)
2008-07-24 17:16 UTC, Bastien Nocera
none Details | Review
Patch #7, with cleanups (68.89 KB, patch)
2008-07-24 17:44 UTC, Bastien Nocera
none Details | Review
Patch #8, applies cleanly against current SVN trunk (68.88 KB, patch)
2008-07-24 23:11 UTC, Bastien Nocera
none Details | Review

Description Jens Granseuer 2008-07-14 20:30:43 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.
Comment 1 Bastien Nocera 2008-07-15 17:12:19 UTC
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.
Comment 2 Bastien Nocera 2008-07-15 17:13:17 UTC
Created attachment 114610 [details]
with no themes installed
Comment 3 Bastien Nocera 2008-07-15 17:13:51 UTC
Created attachment 114611 [details]
with a theme installed that overrides details from the system-wide default
Comment 4 Bastien Nocera 2008-07-17 21:47:43 UTC
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.
Comment 5 Bastien Nocera 2008-07-22 17:18:41 UTC
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?
Comment 6 Bastien Nocera 2008-07-22 17:21:23 UTC
Created attachment 115033 [details]
updated screenie
Comment 7 Bastien Nocera 2008-07-23 17:50:38 UTC
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.
Comment 8 Bastien Nocera 2008-07-24 15:31:59 UTC
Huh, I forgot to implement previews... Whoop!
Comment 9 Bastien Nocera 2008-07-24 17:16:32 UTC
Created attachment 115184 [details] [review]
Patch #6, now with lazy previews
Comment 10 Bastien Nocera 2008-07-24 17:44:39 UTC
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.
Comment 11 Bastien Nocera 2008-07-24 23:11:10 UTC
Created attachment 115212 [details] [review]
Patch #8, applies cleanly against current SVN trunk
Comment 12 Jens Granseuer 2008-07-26 08:09:15 UTC
+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.
Comment 13 Jens Granseuer 2008-07-27 09:47:57 UTC
Btw, the appearance capplet has code to recursiely delete files/directories in theme-util.c. We should move that to common/ it seems.
Comment 14 Bastien Nocera 2008-07-27 23:41:06 UTC
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)
Comment 15 Dennis Cranston 2008-07-28 05:18:24 UTC
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".

Comment 16 Rodrigo Moya 2008-07-28 09:23:28 UTC
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?
Comment 17 Bastien Nocera 2008-07-28 10:15:27 UTC
NEW bugs. This bug is closed because the code is committed to trunk.
Comment 18 Rodrigo Moya 2008-07-28 10:22:29 UTC
Yeah sorry, just realised now, so discard my comment :-)
Comment 19 Jens Granseuer 2008-07-28 20:03:12 UTC
(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?