Closed
Bug 378807
Opened 17 years ago
Closed 17 years ago
Automatically delete settings are not properly disabled in Junk Settings
Categories
(SeaMonkey :: MailNews: Account Configuration, defect)
SeaMonkey
MailNews: Account Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
Details
(Keywords: fixed-seamonkey1.1.3, fixed1.8.1.5)
Attachments
(2 files, 4 obsolete files)
8.81 KB,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
superreview+
kairo
:
approval-seamonkey1.1.3-
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
superreview+
mscott
:
approval1.8.1.5+
kairo
:
approval-seamonkey1.1.3+
|
Details | Diff | Splinter Review |
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•17 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.
This patch: * Removes the observing of the broadcast from the two elements it does not work for and updates them individually * Adds missing accesskeys
Comment 3•17 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+
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•17 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.
Attachment #267805 -
Flags: superreview?(neil)
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•17 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•17 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.
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•17 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•17 years ago
|
||
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•17 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)
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You should check whether the tree is closed (which it is) before checking in.
Assignee | ||
Comment 14•17 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•17 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•17 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-
Attachment #268702 -
Flags: approval-thunderbird2?
Assignee | ||
Comment 17•17 years ago
|
||
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•17 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+
Attachment #270065 -
Flags: approval1.8.1.5?
Comment 19•17 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•17 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)
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.
Description
•