GNOME Bugzilla – Bug 300136
new feature: login success/failure sounds
Last modified: 2005-04-20 18:08:52 UTC
Distribution/Version: Gentoo I wanted X to play an "access granted/denied" wav for whenever a user attempted to login via gdm. I went ahead and made a patch that does this... This patch will add the option of allowing root to specify sound events for after both a successful login or a failed login. None of this code is new as it's just duplicated from the "SoundOnLogin" options. This patch primarily updates the daemon slave code as well as the gui configurator. The glade file that contains the layout xml for the configurator is also updated. I don't see how there could be any security problems as this code shouldn't have any code that wasn't already in gdm.
Created attachment 45114 [details] [review] Patch to add a success/failure login sound.
Andrew, this patch looks good and adds a neat feature to GDM2. I'd be happy to apply it, but you didn't include any docs for the new gdm.conf keys in the docs/C/gdm.xml file. Please update the patch so it includes docs. Also, I tried applying this patch to CVS head and it does not apply cleanly. Could you update your patch so it applies against gdm2 CVS head?
Created attachment 45421 [details] [review] Path to gdm2's cvs HEAD
Attached is a new patch that can be applied to gdm2's cvs HEAD branch. Along with your requested documentation addition and updates, I have also corrected the following bugs: 1) In the documentation under section SoundOnLoginFile where it says: "SoundOnLogin=/path/to/sound.wav" I changed this to the proper "SoundOnLoginFile=/path/to/sound.wav" 2) In gdmsetup I fixed the "No Sound" button so it will save the buttons action to the conf file instead not writing the change because of erroring on "string != NULL".
Okay, I've applied the patch. I noticed this TODO. Can you explain what is left to be done here in gdmcommon.c at the function gdm_common_login_sound? +/* TODO: make for each sound */
Oops, that was more of a question for myself and I forgot to remove it. I missed it when I went through all of my TODO notes. Sorry about that.
So, should I just delete that TODO comment?
yes please do.
Done. Thanks
Attaching Mike McLoughlin's patch along with a new bug fix... read below. # This is a combined patch for Mark McLoughlin's fixes to my patch # as well as fixing the following bug that i didn't bother putting into # bugzilla. When a sound option under gdmsetup's accessibility section is # changed, this option won't take effect until gdm is completely restarted. # For some reason the: #" setup_sensitivity_positive_toggle ("acc_sound_SOUNDKEY", #" "acc_sound_SOUNDKEY_file_box"); # lines cause this option not to reload the configuration file. I don't really # understand this function. So I've removed these lines for now. # Is this a bug with the setup_sensitivity_positive_toggle function? # I don't know but leaving it in is causing the sound not to update.
Created attachment 45466 [details] [review] Combined Mike McLoughlin's patch with my bug fix.
I already committed Mark's patch, so could you rebuild your patch from CVS head?
Created attachment 45494 [details] [review] Patch to fix this sound option bug. # When a sound option under gdmsetup's accessibility section is # changed, this option won't take effect until gdm is completely restarted. # For some reason the: #" setup_sensitivity_positive_toggle ("acc_sound_SOUNDKEY", #" "acc_sound_SOUNDKEY_file_box"); # lines cause this option not to reload the configuration file. I don't really # understand this function. So I've removed these lines for now. # Is this a bug with the setup_sensitivity_positive_toggle function? # I don't know but leaving it in is causing the sound not to update. # # # to patch: # place patch in parent directory of gdm2 directory and then run # patch -p0 < this_patch_file # # Andrew Case & Mike McLoughlin # 2005.04.20 I actually had this patch done first, but the last time I checked out the CVS head Mike's wasn't patched yet so I combined them. This one is just my patch for the above problem.
Fixed in CVS head.