428705 - Migrate notifications prefpane

Status: RESOLVED FIXED

Description

[Bruno 'Aqualon' Escherl]

This bug is for the migration of MailNews' notification prefpane.

Comment 1

[Bruno 'Aqualon' Escherl]

Created attachment 315307 [details] [diff] [review]
Migration of the prefpane v1

This patch doesn't include the necessary change to mailPrefsOverlay.xul, cause that file is bitrotted by the patch in bug 427365 anyway.

Comment 2

[Bruno 'Aqualon' Escherl]

Created attachment 315308 [details] [diff] [review]
diff -w version of patch v1

Comment 3

[[e-mail]]

What's the deal with all the var/let changes?

Comment 4

[[e-mail]]

You also forgot to include the overlay changes...

Comment 5

[Bruno 'Aqualon' Escherl]

Created attachment 315359 [details] [diff] [review]
patch for mailPrefsOverlay.xul

Comment 6

[Bruno 'Aqualon' Escherl]

Created attachment 315618 [details] [diff] [review]
Migration of the prefpane v2

Patch with var/let changes, where they make sense.

Comment 7

[Bruno 'Aqualon' Escherl]

Created attachment 315619 [details] [diff] [review]
diff -w version of patch v2

Comment 8

[[e-mail]]

Comment on attachment 315618 [details] [diff] [review]
Migration of the prefpane v2

> function Startup()
> {
>-  PlaySoundCheck();
IMHO it would be better if PlaySoundCheck was fixed to work from startup...

>+  try
>+  {
>+    let nsIURIFixup = Components.interfaces.nsIURIFixup;
>+    let uriFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                             .getService(nsIURIFixup);
>+    soundURL = uriFixup.createFixupURI(soundURL, nsIURIFixup.FIXUP_FLAG_NONE).spec;
>   }
>+  catch (ex) {}
>+
>+  var ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                            .getService(Components.interfaces.nsIIOService);
>+  gSound.play(ioService.newURI(soundURL, null, null));
It's a nice idea to try urifixup but if fixup fails then what chance does the IO service have? Might as well play the fixed up uri directly.

Comment 9

[Ian Neal]

Comment on attachment 315618 [details] [diff] [review]
Migration of the prefpane v2

>Index: mailnews/base/prefs/resources/content/pref-notifications.js
>===================================================================
>+
> function Startup()
> {
>-  PlaySoundCheck();
See below.

>@@ -17,56 +57,49 @@ function PlaySoundCheck()
> {
>   var playSound = document.getElementById("newMailNotification").checked;
Perhaps look at the preference mail.biff.play_sound instead
>   var playSoundType = document.getElementById("newMailNotificationType");
>   playSoundType.disabled = !playSound;
> 
>   var disableCustomUI = !(playSound && playSoundType.value == 1);
Perhaps look at the preference mail.biff.play_sound.type instead.
>   var mailnewsSoundFileUrl = document.getElementById("mailnewsSoundFileUrl");
> 
>-  mailnewsSoundFileUrl.disabled = disableCustomUI
>+  mailnewsSoundFileUrl.disabled = disableCustomUI;
>   document.getElementById("preview").disabled = disableCustomUI || (mailnewsSoundFileUrl.value == "");
Perhaps look at the preference mail.biff.play_sound.url instead
>   document.getElementById("browse").disabled = disableCustomUI;
> }
> 
>-const nsIFilePicker = Components.interfaces.nsIFilePicker;
>-
> function Browse()
> {
>+  var nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var fp = Components.classes["@mozilla.org/filepicker;1"]
>-                       .createInstance(nsIFilePicker);
>+                     .createInstance(nsIFilePicker);
> 
>-  // XXX todo, persist the last sound directory and pass it in
I cannot see how you have resolved this "todo" - see how it is done in pref-download.js
>-  // XXX todo filter by .wav
>   fp.init(window, document.getElementById("browse").getAttribute("filepickertitle"), nsIFilePicker.modeOpen);
>-  fp.appendFilters(nsIFilePicker.filterAll);
>+  fp.appendFilter("*.wav", "*.wav");
You cannot just have filtered by *.wav - some platforms will probably allow other sound files and are not always limited by a 3 letter extension.

There may be other places where you should look at the pref rather than the UI elements status/value.

Comment 10

[Bruno 'Aqualon' Escherl]

Comment on attachment 315618 [details] [diff] [review]
Migration of the prefpane v2

Removing review request until I have an updated version of that patch.

Comment 11

[Ian Neal]

Also see bug 266192

Comment 12

[Bruno 'Aqualon' Escherl]

Created attachment 331735 [details] [diff] [review]
Migration of the prefpane v3

New version of the patch, incorporating some of the changes from bug 266192.

Note: the patch needs the change from bug 448107 to work.

Comment 13

[Ian Neal]

Comment on attachment 331735 [details] [diff] [review]
Migration of the prefpane v3

>--- a/mailnews/base/prefs/resources/content/mailPrefsOverlay.xul
>+++ b/mailnews/base/prefs/resources/content/mailPrefsOverlay.xul
>@@ -71,6 +71,11 @@
>                   prefpane="viewing_messages_pane"
>                   url="chrome://messenger/content/pref-viewing_messages.xul"
>                   helpTopic="mail_prefs_display"/>
>+        <treeitem id="notificationsItem"
>+                  label="&notifications.label;"
>+                  prefpane="notifications_pane"
>+                  url="chrome://messenger/content/pref-notifications.xul"
>+                  helpTopic="mail_prefs_notifications"/>
>         <treeitem id="tagsItem"
>                   label="&tags.label;"
>                   prefpane="tags_pane"
You are missing the (Migrated: &notifications.label;) part.

>--- a/mailnews/base/prefs/resources/content/pref-notifications.js
>+++ b/mailnews/base/prefs/resources/content/pref-notifications.js
>+
>+  gIOService = Components.classes["@mozilla.org/network/io-service;1"]
>+                         .getService(Components.interfaces.nsIIOService);
>+  PlaySoundCheck();
Call this function with document.getElementById("mail.biff.play_sound").value
> }
> 
> function PlaySoundCheck()
Have the function take the play_sound value as an argument (aPlaySound).
> {
>+  EnableElementById("newMailNotificationType", playSound, false);
>+  gSoundURL = document.getElementById("mail.biff.play_sound.url").value;
You should be grabbing the preference element not the value and in the Startup function. (gSoundURLPref perhaps as a name).
> 
>+  let playSound = document.getElementById("mail.biff.play_sound").value;
Not needed any more.
>+  let playSoundType = document.getElementById("mail.biff.play_sound.type").value;
Now call a new function with playSound && (playSoundType == 1) as an argument.

>+  EnableElementById("mailnewsSoundFileUrl", enableCustomUI, false);
and this would be in your new function.
>+
>+  if(!document.getElementById("mail.biff.play_sound.url").locked)
>+    document.getElementById("browse").disabled = !enableCustomUI;
>+
>+  document.getElementById("preview").disabled = !enableCustomUI ||
>+                                                (gSoundURL == "");
These lines should not be needed if using <observes>, see comments further down.
> }
> 
> function Browse()
> {
>+  var prefBundle = document.getElementById("bundle_prefutilities");
>+
>+  var nsIFilePicker = Components.interfaces.nsIFilePicker;
>   var fp = Components.classes["@mozilla.org/filepicker;1"]
>+                     .createInstance(nsIFilePicker);
> 
>+  fp.init(window, prefBundle.getString("choosesound"), nsIFilePicker.modeOpen);
> 
>+  try
>+  {
Shouldn't need the try/catch
>+    if (gSoundURL != "") {
if (gSoundURLPref.value)
Nit: { should be on the next line.
>+      let nsIFileHandler = Components.interfaces.nsIFileProtocolHandler;
>+      let nsILocalFile = Components.interfaces.nsILocalFile;
>+
>+      let fileHandler = gIOService.getProtocolHandler("file")
>+                                  .QueryInterface(nsIFileHandler);
>+      fp.displayDirectory = fileHandler.getFileFromURLSpec(gSoundURL).parent
>+                                       .QueryInterface(nsILocalFile);
You shouldn't need this QI.
>+    }
>+  } catch (ex) { }
>+
>+  fp.appendFilter(prefBundle.getString("SoundFiles"), "*.wav; *.wave");
Best to have all files as another option - fp.appendFilters(nsIFilePicker.filterAll);
>+
>+  if (fp.show() == nsIFilePicker.returnOK)
>+  {
>+    document.getElementById("mail.biff.play_sound.url").value = fp.fileURL.spec;
Shouldn't need this if using filefield
>+    gSoundURL = fp.fileURL.spec;
gSoundURLPref.value = fp.fileURL.spec;
>   }
> 
>+  document.getElementById("preview").disabled = (gSoundURL == "");
Shouldn't need this either.
> }
> 
> function PreviewSound()
> {
>+  if (!gSound)
>+    gSound = Components.classes["@mozilla.org/sound;1"]
>+                       .createInstance(Components.interfaces.nsISound);
> 
>+  try
>+  {
>+    let nsIURIFixup = Components.interfaces.nsIURIFixup;
>+    let uriFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>+                             .getService(nsIURIFixup);
>+    gSoundURL = uriFixup.createFixupURI(gSoundURL, nsIURIFixup.FIXUP_FLAG_NONE).spec;
>+  }
>+  catch (ex) {}
As migrating profile (see bottom of comments), should not need this as we will always have a URLspec.
var soundURL = gSoundURLPref.value
if (soundURL)
  gSound.play(gIOService.newURI(soundURL, null, null));
else
  gSound.play(whatever the default mail beep is)
> 
>+  gSound.play(gIOService.newURI(gSoundURL, null, null));
> }
>diff --git a/mailnews/base/prefs/resources/content/pref-notifications.xul b/mailnews/base/prefs/resources/content/pref-notifications.xul
>--- a/mailnews/base/prefs/resources/content/pref-notifications.xul
>+++ b/mailnews/base/prefs/resources/content/pref-notifications.xul
>+      <preference id="mail.biff.play_sound"
>+                  name="mail.biff.play_sound"
>+                  type="bool"
>+                  onchange="PlaySoundCheck();"/>
Pass this.value into this function.
>+      <preference id="mail.biff.play_sound.type"
>+                  name="mail.biff.play_sound.type"
>+                  type="int"
>+                  onchange="PlaySoundCheck();"/>
Pass this.value == 1 into a new function - see above
>+      <preference id="mail.biff.play_sound.url"
>+                  name="mail.biff.play_sound.url"
>+                  type="string"/>
>+    </preferences>

>+      <hbox align="center" class="indent" id="newMailNotificationSoundSelectBox">
Do we need this id?
>+        <radiogroup id="newMailNotificationType"
>+                    preference="mail.biff.play_sound.type"
>+                    orient="vertical"
>+                    aria-labelledby="newMailNotification">
>+          <radio id="system" class="iconic" value="0"
>+                 label="&systemsound.label;"
>+                 accesskey="&systemsound.accesskey;"/>
Nit: One attribute per line if line length >80
>+          <radio id="custom" class="iconic" value="1"
>+                 label="&customsound.label;"
>+                 accesskey="&customsound.accesskey;"/>
Nit: One attribute per line if line length >80
>+        </radiogroup>
>+      </hbox>
> 
>+      <hbox align="center" class="indent">
>+        <textbox id="mailnewsSoundFileUrl" flex="1" readonly="true"
>+                 preference="mail.biff.play_sound.url"
>+                 aria-labelledby="custom"/>
Change to <filefield> see bug 444582 or bug 441403 for ideas how to use it and use onsyncfrompreference attribute.
>+        <hbox align="center">
>+          <button id="browse" label="&browse.label;"
>+                  filepickertitle="&browse.title;"
>+                  accesskey="&browse.accesskey;"
>+                  oncommand="Browse();"
>+                  disabled="true"/>
Add <observes element="mailnewsSoundFileUrl" attribute="disabled"/> as a child to <button></button> and remove the disabled="true"
>+          <button id="preview" label="&preview.label;"
>+                  accesskey="&preview.accesskey;"
>+                  oncommand="PreviewSound();"
>+                  disabled="true"/>
As above.

The profile migration probably needs to be amended to make sure that the mail.biff_play_sound.url is a URLspec - see Profile migration patch for download preferences v0.1 on bug 444582 for the sort of thing - something like:
MAKEPREFTRANSFORM("mail.biff.play_sound.url", 0, String, File)

r- for the moment

Comment 14

[Ian Neal]

Also might be worth changing the locale file so it reads "Custom sound file" rather than "Custom .wav file"

Comment 15

[Bruno 'Aqualon' Escherl]

Created attachment 332219 [details] [diff] [review]
Migration of the prefpane v4

Patch addresses IanN's review comments.

Comment 16

[Ian Neal]

Comment on attachment 332219 [details] [diff] [review]
Migration of the prefpane v4

>+    gSound = Components.classes["@mozilla.org/sound;1"]
>+                       .createInstance(Components.interfaces.nsISound);
>+  var soundURL = gSoundUrlPref.value
>+  if (soundURL)
>+    gSound.play(gIOService.newURI(soundURL, null, null));
>+  else
>+    gSound.beep();
To get the default mail sound instead of system beep, this should be:
gSound.playSystemSound("_moz_mailbeep");

r=me with that change

Comment 17

[Bruno 'Aqualon' Escherl]

Created attachment 332898 [details] [diff] [review]
Migration of the prefpane v5

$ diff bug428705.v4.diff bug428705.v5.diff
186c186
< +    gSound.beep();
---
> +    gSound.playSystemSound("_moz_mailbeep");

Taking over r+.

Comment 18

[[e-mail]]

Comment on attachment 332898 [details] [diff] [review]
Migration of the prefpane v5

>-function Browse()
>+function SelectSound()
But you forgot to update the XUL to match...

>+  var soundURL = gSoundUrlPref.value
Nit: missing ;

Comment 19

[Bruno 'Aqualon' Escherl]

Created attachment 333363 [details] [diff] [review]
Migration of the prefpane v6

Patch with fixes for Neil's sr-comments, taking over r+/sr+

Comment 20

[[e-mail]]

Pushed changeset 4a68f92ebbe0 to comm-central.