Closed
Bug 428705
Opened 14 years ago
Closed 14 years ago
Migrate notifications prefpane
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a1
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Whiteboard: H'n'C)
Attachments
(1 file, 8 obsolete files)
17.17 KB,
patch
|
bugzilla
:
review+
bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
This bug is for the migration of MailNews' notification prefpane.
Assignee | ||
Comment 1•14 years ago
|
||
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•14 years ago
|
||
Comment 3•14 years ago
|
||
What's the deal with all the var/let changes?
Comment 4•14 years ago
|
||
You also forgot to include the overlay changes...
Assignee | ||
Comment 5•14 years ago
|
||
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Whiteboard: H'n'C
Target Milestone: --- → seamonkey2.0alpha
Comment 8•14 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 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•14 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•14 years ago
|
Attachment #315619 -
Attachment is obsolete: true
Comment 11•14 years ago
|
||
Also see bug 266192
Assignee | ||
Comment 12•14 years ago
|
||
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)
Comment 13•14 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="¬ifications.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: ¬ifications.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•14 years ago
|
||
Also might be worth changing the locale file so it reads "Custom sound file" rather than "Custom .wav file"
Assignee | ||
Comment 15•14 years ago
|
||
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•14 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•14 years ago
|
||
$ 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•14 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•14 years ago
|
||
Patch with fixes for Neil's sr-comments, taking over r+/sr+
Attachment #333363 -
Flags: superreview+
Attachment #333363 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #332898 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 20•14 years ago
|
||
Pushed changeset 4a68f92ebbe0 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•