This is the mailnews part of bug 264675

So far: * 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 * Implements missing pref locking in pref-receipts

This patch: * Implements missing pref locking in pref-mailnews * Implements missing pref locking in pref-viewing_messages - color picker not being disabled correctly * 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 * Implements missing pref locking in pref-labels - color picker not being disabled correctly and make restore defaults not try and overwrite locked prefs * Implements missing pref locking in pref-receipts * Implements missing pref locking in pref-offline * Implements missing pref locking in pref-composing_messages - color picker not being disabled correctly

As per v0.1 patch but also: * Ensures sound.url is a URL

pref-notifications changes are now in patch on bug 266192

Changes since v0.2a * Removed use of global variables when not needed * Moved lockpref changes into if statement for pref-viewing_messages.xul

Changes since v0.2b * Colorpickers can be locked and use prefs now (since bug 260335 landed) so take advantage of that. * Removed unused JS from pref-mailnews.js

Comment on attachment 170314 [details] [diff] [review] mailnews lockpref and colorpicker fix up patch v0.2c > /* Function to restore pref values to application defaults */ > function restoreColorAndDescriptionToDefaults() > { > var prefColor; > var description; > var pickerColor; > var dataColor; > var labelDescription; Some of these are no longer used... > var prefService = Components.classes["@mozilla.org/preferences-service;1"] > .getService(Components.interfaces.nsIPrefService); > var prefs = prefService.getDefaultBranch(null); [In theory you could get branches for the descriptions and labels] >- for(var i = 1; i <= 5; i++) >- { >+ for (var i = 1; i <= 5; i++) { Changing brace style is bad form (unless there are mixed styles). >+ if (labelDescription.getAttribute("disabled") != "true") { Consider !labelDescription.disabled (also applies to the colour). >+ description = prefs.getComplexValue("mailnews.labels.description." + i, >+ Components.interfaces.nsIPrefLocalizedString).data; >+ labelDescription.value = description; Interesting that you didn't just write labelDescription.value = prefs.getComplexValue(...).data; like you did for the colour. > <colorpicker > class="small-margin" > type="button" >- id="labelColorPicker1" >- palettename="standard" >- onchange="setColorWell(this);"/> >- <data > id="label1Color" >- pref="true" >- preftype="color" >- prefstring="mailnews.labels.color.1" >- prefattribute="value" >- wsm_attributes="value"/> >+ palettename="standard" >+ prefstring="mailnews.labels.color.1"/> Swapping the id and palettename lines makes the diff smaller ;-) >+ notInToCcLabel.disabled = disableAll; >+ outsideDomainLabel.disabled = disableAll; >+ otherCasesLabel.disabled = disableAll; Strange; we don't normally disable labels, do we? > if (aCheckbox.checked) >- aField.removeAttribute("disabled"); >+ if (!parent.hPrefWindow.getPrefIsLocked(aField.getAttribute("prefstring"))) >+ aField.removeAttribute("disabled"); > else > aField.setAttribute("disabled", "true"); At last, a bug! The else now mismatches the if.

Changes since v0.2c * In pref-labels.js made suggested changes to unused variables, getDefaultBranch and setting of labelDescription.value * In pref-labels.xul reordered id and palettename as suggested * In pref-offline.xul amended enableField to only use two variables and got rid of if statements for how aField.disabled was set wrt disabling labels: a) Looks better if we do b) Matches behaviour of when a radiogroup is disabled.

Comment on attachment 170343 [details] [diff] [review] fix for colorpickers and mailnews lockprefs v0.2d >+ var aField = document.getElementById("offlineCompactFolderMin"); >+ aField.disabled = (!aCheckbox.checked || >+ parent.hPrefWindow.getPrefIsLocked(aField.getAttribute("prefstring"))); > > if (!aField.disabled && setFocus) > aField.focus(); Might be worth renaming that variable - only arguments are supposed to have a leading "a". (In reply to comment #8) >wrt disabling labels: >a) Looks better if we do But still different to when the three menulists are locked. >b) Matches behaviour of when a radiogroup is disabled. But menulists have labels that display the current value as well as labels that describe the menulist; radio buttons only have the latter. Still, I'm happy with leaving it at the status quo.

Changes since v0.2d * Changed aField to folderMin as per reviewer's request Carrying forward r+ and request sr

Comment on attachment 170393 [details] [diff] [review] mailnew colorpickers/lockprefs fix v0.2e (Checked in) Checked in