XXX todo, persist the last sound directory and pass it in XXX todo filter by .wav XXX todo see if we can create a nsIURL from the native file path Also make sure we respect pref locking

This patch * Implements missing pref locking in pref-notifications * Persists the last sound directory and passes it in to the filepicker in pref-notifications * Filters by .wav / .wave in pref-notifications * Ensures sound.url is a URL

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

Neil, do you have time to review this?

Comment on attachment 163485 [details] [diff] [review] Provisional patch v0.1 >+ <stringbundle id="bundle_prefs" src="chrome://messenger/locale/prefs.properties"/> > <script type="application/x-javascript" src="chrome://messenger/content/pref-notifications.js"/> Nit: Stringbundles belong after scripts. >+const nsIFileHandler = Components.interfaces.nsIFileProtocolHandler; This confused me, please keep the name the same! >+var ioService; >+var gMailnewsSound; >+var gPlaySoundType; >+var gMailnewsSoundLocked; >+var gPlaySoundTypeLocked; >+var gSound = null; gSound but no g for the IO service? >+ // Make sure sound.url setting is actually a URL Please use URI fixup as per pref-downloads.js >+ var soundURL = gMailnewsSound.value; >+ var initialDir = null; >+ if (soundURL !="") { >+ var fileHandler = ioService.getProtocolHandler("file") >+ .QueryInterface(nsIFileHandler); >+ initialDir = fileHandler.getFileFromURLSpec(soundURL) >+ .QueryInterface(nsILocalFile); >+ } >+ fp.init(window, prefsBundle.getString("choosesound"), nsIFilePicker.modeOpen); >+ if (initialDir) >+ fp.displayDirectory = initialDir; Why not init it first, then you can set the initialDir directly. (Although maybe you should try/catch in case you didn't get a file URL). >+# preferences stuff This comment is meaningless. >+WAVFiles=Sounds (*.wav) Apparently GTK2 filepickers typically don't display the wildcard. And IIRC the XUL filepicker that we use on GTK1 automatically suffixes the wildcard. That the Windows filepicker does not is a windows widget bug that I'd eventually like to see fixed.

The .getString's don't seem to work, but no error shows up in the console.

Comment on attachment 243004 [details] [diff] [review] Updated patch >- if (soundURL.indexOf("file://") == -1) { >- // XXX todo see if we can create a nsIURL from the native file path >- // otherwise, play a system sound >- gSound.playSystemSound(soundURL); >- } >- else { >- var ioService = Components.classes["@mozilla.org/network/io-service;1"] >- .getService(Components.interfaces.nsIIOService); >- var url = ioService.newURI(soundURL, null, null); >- gSound.play(url) >+ gSound.play(gIOService.newURI(gMailnewsSound.value, null, null)); > } You forgot to delete this line.

Thanks Neil for spotting that extra brace. There was also another bug (I removed the references to soundURL, but then used it in the try block). Asking r? from Ian, since he was the first author.

Comment on attachment 243097 [details] [diff] [review] This should be it. >Index: mailnews/base/prefs/resources/content/pref-notifications.js >=================================================================== >@@ -1,5 +1,33 @@ >+const nsIFilePicker = Components.interfaces.nsIFilePicker; >+const nsILocalFile = Components.interfaces.nsILocalFile; >+const nsIIOService = Components.interfaces.nsIIOService; >+const nsIFileProtocolHandler = Components.interfaces.nsIFileProtocolHandler; >+ >+var gIOService; >+var gMailnewsSound; >+var gPlaySoundType; >+var gMailnewsSoundLocked; >+var gPlaySoundTypeLocked; >+var gSound = null; >+ > function Startup() > { >+ gIOService = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(nsIIOService); Nit: Need one more space in the line above to line up. > var ret = fp.show(); > if (ret == nsIFilePicker.returnOK) { > var mailnewsSoundFileUrl = document.getElementById("mailnewsSoundFileUrl"); This line is no longer needed. > // convert the nsILocalFile into a nsIFile url >- mailnewsSoundFileUrl.value = fp.fileURL.spec; >+ gMailnewsSound.value = fp.fileURL.spec; > } You could probably loose the brackets too. I know last time round Neil pointed out there was no way of setting the url back to being empty. Not sure if that is still an issue.

Ian, Giacomo, Are you still working on this ?

This will probably be closed off when it gets migrated to new prefs window.

This got fixed when it was migrated to the new prefs window.