Automatically delete settings are not properly disabled in Junk Settings

RESOLVED FIXED

Status

SeaMonkey
MailNews: Account Configuration
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Ian Neal, Assigned: Ian Neal)

Tracking

({fixed-seamonkey1.1.3, fixed1.8.1.5})

Trunk
fixed-seamonkey1.1.3, fixed1.8.1.5

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

10 years ago
Created attachment 266391 [details] [diff] [review]
prefislocked and broadcast combo patch v0.1

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 3

10 years ago
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+
(Assignee)

Comment 4

10 years ago
Created attachment 267805 [details] [diff] [review]
Spin off function patch v0.2

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 5

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

Updated

10 years ago
Attachment #267805 - Flags: superreview?(neil)
(Assignee)

Comment 6

10 years ago
Created attachment 267912 [details] [diff] [review]
PrefBranch patch

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 7

10 years ago
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+

Comment 8

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

Comment 9

10 years ago
Created attachment 268628 [details] [diff] [review]
No null check patch v0.4

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 10

10 years ago
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?
(Assignee)

Comment 11

10 years ago
Created attachment 268702 [details] [diff] [review]
No check on gPrefBranch patch v0.5 (Checked into trunk)

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+
(Assignee)

Comment 12

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

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You should check whether the tree is closed (which it is) before checking in.
(Assignee)

Comment 14

10 years ago
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?
(Assignee)

Comment 15

10 years ago
(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 16

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

Updated

10 years ago
Attachment #268702 - Flags: approval-thunderbird2?
(Assignee)

Comment 17

10 years ago
Created attachment 270065 [details] [diff] [review]
Branch version of patch 0.5 (checked into 1.8 branch)

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 18

10 years ago
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+
(Assignee)

Updated

10 years ago
Attachment #270065 - Flags: approval1.8.1.5?

Comment 19

10 years ago
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?
(Assignee)

Comment 20

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

Updated

10 years ago
Keywords: fixed-seamonkey1.1.3, fixed1.8.1.5
You need to log in before you can comment on or make changes to this bug.