Closed Bug 266192 Opened 20 years ago Closed 14 years ago

Implement todos from pref-notifications

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: iannbugzilla, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
Status: NEW → ASSIGNED
Attachment #163485 - Flags: review?(neil.parkwaycc.co.uk)
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
Product: Browser → Seamonkey
*** 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.
Attachment #163485 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #243004 - Attachment is obsolete: true
Attachment #243097 - Flags: review?(iann_bugzilla)
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.
Attachment #243097 - Flags: review?(iann_bugzilla) → review+
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.
Assignee: iann_bugzilla → nobody
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Component: MailNews: Account Configuration → Preferences
QA Contact: preferences
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: