Migrate notifications prefpane

RESOLVED FIXED in seamonkey2.0a1

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: bugzilla, Assigned: bugzilla)

Tracking

Trunk
seamonkey2.0a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: H'n'C)

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

11 years ago
This bug is for the migration of MailNews' notification prefpane.
(Assignee)

Comment 1

11 years ago
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.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Attachment #315307 - Flags: superreview?(neil)
Attachment #315307 - Flags: review?(mnyromyr)
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

11 years ago
You also forgot to include the overlay changes...
(Assignee)

Comment 5

11 years ago
Created attachment 315359 [details] [diff] [review]
patch for mailPrefsOverlay.xul
(Assignee)

Comment 6

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

Patch with var/let changes, where they make sense.
Attachment #315307 - Attachment is obsolete: true
Attachment #315308 - Attachment is obsolete: true
Attachment #315359 - Attachment is obsolete: true
Attachment #315618 - Flags: superreview?(neil)
Attachment #315618 - Flags: review?(mnyromyr)
Attachment #315307 - Flags: superreview?(neil)
Attachment #315307 - Flags: review?(mnyromyr)
(Assignee)

Comment 7

11 years ago
Created attachment 315619 [details] [diff] [review]
diff -w version of patch v2
(Assignee)

Updated

11 years ago
Whiteboard: H'n'C
Target Milestone: --- → seamonkey2.0alpha

Comment 8

11 years ago
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

11 years ago
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.
(Assignee)

Comment 10

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

Removing review request until I have an updated version of that patch.
Attachment #315618 - Attachment is obsolete: true
Attachment #315618 - Flags: superreview?(neil)
Attachment #315618 - Flags: review?(mnyromyr)
(Assignee)

Updated

11 years ago
Attachment #315619 - Attachment is obsolete: true

Comment 11

11 years ago
Also see bug 266192
(Assignee)

Comment 12

10 years ago
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.
Attachment #331735 - Flags: superreview?(neil)
Attachment #331735 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

10 years ago
Depends on: 448107

Comment 13

10 years ago
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
Attachment #331735 - Flags: review?(iann_bugzilla) → review-

Comment 14

10 years ago
Also might be worth changing the locale file so it reads "Custom sound file" rather than "Custom .wav file"
(Assignee)

Updated

10 years ago
Depends on: 448106
(Assignee)

Comment 15

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

Patch addresses IanN's review comments.
Attachment #331735 - Attachment is obsolete: true
Attachment #332219 - Flags: superreview?(neil)
Attachment #332219 - Flags: review?(iann_bugzilla)
Attachment #331735 - Flags: superreview?(neil)

Comment 16

10 years ago
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
Attachment #332219 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 17

10 years ago
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+.
Attachment #332219 - Attachment is obsolete: true
Attachment #332898 - Flags: superreview?(neil)
Attachment #332898 - Flags: review+
Attachment #332219 - Flags: superreview?(neil)

Comment 18

10 years ago
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 ;
Attachment #332898 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 19

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

Patch with fixes for Neil's sr-comments, taking over r+/sr+
Attachment #333363 - Flags: superreview+
Attachment #333363 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #332898 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 20

10 years ago
Pushed changeset 4a68f92ebbe0 to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.