Closed Bug 378807 Opened 14 years ago Closed 14 years ago

Automatically delete settings are not properly disabled in Junk Settings

Categories

(SeaMonkey :: MailNews: Account Configuration, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

Details

(Keywords: fixed-seamonkey1.1.3, fixed1.8.1.5)

Attachments

(2 files, 4 obsolete files)

In both SM and TB the "Automatically delete junk mail older than" checkbox and its associated interval textbox do not properly observe the broadcaster "broadcaster_moveMode" when switching between Junk Settings panels in different accounts.
Steps to reproduce
1. Have at least two accounts
2. Have "Move new junk messages to:" unchecked on both accounts
3. Switch between Junk Settings on the two accounts

Expected result
1. The whole of the line starting with the checkbox and then "Automatically delete junk mail older than" is completely disabled/greyed out

Actual result
1. Only the final label "days" is disabled/greyed out

The reason I think this might be happening is:
AccountManager.js checks the locked status of each element's prefstring on the panel and sets whether it is disabled or not depending on that status and because the broadcaster's attributes have not changed the observing elements do not change.

Solutions:
1) Have AccountManager.js also check if the element is observing a broadcaster and take that into account when setting whether the element is disabled or not.
or
2) After AccountManager.js has done its work, toggle the broadcaster's attribute(s) back and forth.
Worse, if the prefs are locked, then toggling the checkbox will still enable the fields, so I guess the only cure is to make updateMoveTargetMode disable each element individually based on its locked state and the aEnable parameter.
This patch:
* Removes the observing of the broadcast from the two elements it does not work for and updates them individually
* Adds missing accesskeys
Assignee: mail → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #266391 - Flags: review?(mnyromyr)
Comment on attachment 266391 [details] [diff] [review]
prefislocked and broadcast combo patch v0.1

>Index: mailnews/base/prefs/resources/content/am-junk.js
>===================================================================
>+var gPrefInt = null;

This var name is pretty meaningless. Better choose something like gPrefService etc.

>+  if (!aEnable || (gIncomingServer &&
>+      gPrefInt.prefIsLocked("mail.server." + gIncomingServer.key + ".purgeSpam")))
>+    document.getElementById("server.purgeSpam").setAttribute("disabled", "true");
>+  else
>+    document.getElementById("server.purgeSpam").removeAttribute("disabled");
>+
>+  if (!aEnable || (gIncomingServer &&
>+      gPrefInt.prefIsLocked("mail.server." + gIncomingServer.key + ".purgeSpamInterval")))
>+    document.getElementById("server.purgeSpamInterval").setAttribute("disabled", "true");
>+  else
>+    document.getElementById("server.purgeSpamInterval").removeAttribute("disabled");

It'd look cleaner if you spin off a function here and call it twice, with the different suffix.


r=me with that.
Attachment #266391 - Flags: review?(mnyromyr) → review+
Attached patch Spin off function patch v0.2 (obsolete) — Splinter Review
Changes from v0.1:
* Used gPrefService instead of gPrefInt as suggested
* Spun repeated code off into a separate function updatePurgeSpam as suggested

Carrying forward r= and requesting sr=
Attachment #266391 - Attachment is obsolete: true
Attachment #267805 - Flags: superreview?(neil)
Attachment #267805 - Flags: review+
Comment on attachment 267805 [details] [diff] [review]
Spin off function patch v0.2

> function onPreInit(account, accountValues)
> {
>+  gIncomingServer = account.incomingServer;
You never use the server anyway, only its key, and then only to get the right branch of prefs, so you might as well get the pref branch here in the first place and use that in updatePurgeSpam instead.
Attachment #267805 - Flags: superreview?(neil)
Attached patch PrefBranch patch (obsolete) — Splinter Review
Changes since v0.2:
* Switched to using a pref branch as suggested.
Attachment #267805 - Attachment is obsolete: true
Attachment #267912 - Flags: superreview?(neil)
Attachment #267912 - Flags: review+
Comment on attachment 267912 [details] [diff] [review]
PrefBranch patch

OK, although I don't see how an account can not have an incoming server (at least not an account with a junk settings pane).
Attachment #267912 - Flags: superreview?(neil) → superreview+
(In reply to comment #7)
>(From update of attachment 267912 [details] [diff] [review])
>OK, although I don't see how an account can not have an incoming server (at
>least not an account with a junk settings pane).
Nowhere else in Account Manager do we seem to null-check the incoming server.
Attached patch No null check patch v0.4 (obsolete) — Splinter Review
Changes since v0.3:
* Removed null check on incomingServer

Carried forward r/sr
Attachment #267912 - Attachment is obsolete: true
Attachment #268628 - Flags: superreview+
Attachment #268628 - Flags: review+
Comment on attachment 268628 [details] [diff] [review]
No null check patch v0.4

>+  if (!aEnable || (gPrefBranch && gPrefBranch.prefIsLocked(aPref)))
You don't need to null-check gPrefBranch do you?
Changes from v0.4:
* Removed null check on gPrefBranch

Carried forward r/sr
Attachment #268628 - Attachment is obsolete: true
Attachment #268702 - Flags: superreview+
Attachment #268702 - Flags: review+
Comment on attachment 268702 [details] [diff] [review]
No check on gPrefBranch patch v0.5 (Checked into trunk)

Checking in (trunk)
mail/locales/en-US/chrome/messenger/am-junk.dtd;
new revision: 1.2; previous revision: 1.1
mailnews/base/prefs/resources/content/am-junk.js;
new revision: 1.12; previous revision: 1.11
mailnews/base/prefs/resources/content/am-junk.xul;
new revision: 1.6; previous revision: 1.5
suite/locales/en-US/chrome/mailnews/pref/am-junk.dtd;
new revision: 1.2; previous revision: 1.1
done
Attachment #268702 - Attachment description: No check on gPrefBranch patch v0.5 → No check on gPrefBranch patch v0.5 (Checked into trunk)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You should check whether the tree is closed (which it is) before checking in.
Comment on attachment 268702 [details] [diff] [review]
No check on gPrefBranch patch v0.5 (Checked into trunk)

Requesting approvals for branch
Attachment #268702 - Flags: approval-thunderbird2?
Attachment #268702 - Flags: approval-seamonkey1.1.3?
(In reply to comment #13)
> You should check whether the tree is closed (which it is) before checking in.
> 
At the time I started the checkin the tinderboxen page was still saying "The tree is open", I am happy to back out if necessary, though changes are all non-firefox ones so should not impact on pageload performance.
Comment on attachment 268702 [details] [diff] [review]
No check on gPrefBranch patch v0.5 (Checked into trunk)

Umm, we take take it in this form on branch, as you're changing locale files and we are in a locale freeze. Though from what I see this are accesskey addtions/changes only, so it should probably be easy to do a version that is not changing L10n and which can be taken on branch.
Attachment #268702 - Flags: approval-seamonkey1.1.3? → approval-seamonkey1.1.3-
Attachment #268702 - Flags: approval-thunderbird2?
This is a L10n safe patch for branch, carrying forward r/sr from trunk patch and requesting approval for SM/TB, not part of Firefox build so should not need 1.8.1.5 approval.
Attachment #270065 - Flags: superreview+
Attachment #270065 - Flags: review+
Attachment #270065 - Flags: approval-thunderbird2?
Attachment #270065 - Flags: approval-seamonkey1.1.3?
Comment on attachment 270065 [details] [diff] [review]
Branch version of patch 0.5 (checked into 1.8 branch)

This looks good to me for SeaMonkey 1.1.3
Attachment #270065 - Flags: approval-seamonkey1.1.3? → approval-seamonkey1.1.3+
Attachment #270065 - Flags: approval1.8.1.5?
Comment on attachment 270065 [details] [diff] [review]
Branch version of patch 0.5 (checked into 1.8 branch)

a=mscott for mailnews only change to 1.8.1.5.
Attachment #270065 - Flags: approval1.8.1.5?
Attachment #270065 - Flags: approval1.8.1.5+
Attachment #270065 - Flags: approval-thunderbird2?
Comment on attachment 270065 [details] [diff] [review]
Branch version of patch 0.5 (checked into 1.8 branch)

Checking in (1.8 branch)
am-junk.js;
new revision: 1.1.2.7; previous revision: 1.1.2.6
am-junk.xul;
new revision: 1.1.2.4; previous revision: 1.1.2.3
done
Attachment #270065 - Attachment description: Branch version of patch 0.5 → Branch version of patch 0.5 (checked into 1.8 branch)
You need to log in before you can comment on or make changes to this bug.