265232 - Make sure we respect locked prefs throughout Mailnews UI

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Ian Neal]

This is the mailnews part of bug 264675

Comment 1

[Ian Neal]

Attachment: Work in progress 0.1

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

Comment 2

[Ian Neal]

Attachment: Patch v0.1 mailnews pref locking fixes

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

Comment 3

[Ian Neal]

Attachment: Revised patch v0.2 mailnews pref fixes

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

Comment 4

[Ian Neal]

Attachment: Reduced patch v0.2a with out pref-notifications part

pref-notifications changes are now in patch on bug 266192

Comment 5

[Ian Neal]

Attachment: mailnews lockpref patch v0.2b

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

Comment 6

[Ian Neal]

Attachment: mailnews lockpref and colorpicker fix up patch v0.2c

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 7

[[e-mail]]

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.

Comment 8

[Ian Neal]

Attachment: fix for colorpickers and mailnews lockprefs v0.2d

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 9

[[e-mail]]

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.

Comment 10

[Ian Neal]

Attachment: mailnew colorpickers/lockprefs fix v0.2e (Checked in)

Changes since v0.2d
* Changed aField to folderMin as per reviewer's request

Carrying forward r+ and request sr

Comment 11

[Ian Neal]

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

Checked in