Last Comment Bug 378807 - Automatically delete settings are not properly disabled in Junk Settings
: Automatically delete settings are not properly disabled in Junk Settings
Status: RESOLVED FIXED
: fixed-seamonkey1.1.3, fixed1.8.1.5
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Account Configuration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Ian Neal
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-25 16:04 PDT by Ian Neal
Modified: 2007-07-03 12:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
prefislocked and broadcast combo patch v0.1 (8.99 KB, patch)
2007-05-28 08:03 PDT, Ian Neal
mnyromyr: review+
Details | Diff | Splinter Review
Spin off function patch v0.2 (8.82 KB, patch)
2007-06-09 10:55 PDT, Ian Neal
iann_bugzilla: review+
Details | Diff | Splinter Review
PrefBranch patch (8.80 KB, patch)
2007-06-10 16:39 PDT, Ian Neal
iann_bugzilla: review+
neil: superreview+
Details | Diff | Splinter Review
No null check patch v0.4 (8.80 KB, patch)
2007-06-16 14:43 PDT, Ian Neal
iann_bugzilla: review+
iann_bugzilla: superreview+
Details | Diff | Splinter Review
No check on gPrefBranch patch v0.5 (Checked into trunk) (8.81 KB, patch)
2007-06-17 13:49 PDT, Ian Neal
iann_bugzilla: review+
iann_bugzilla: superreview+
kairo: approval‑seamonkey1.1.3-
Details | Diff | Splinter Review
Branch version of patch 0.5 (checked into 1.8 branch) (3.63 KB, patch)
2007-06-27 14:38 PDT, Ian Neal
iann_bugzilla: review+
iann_bugzilla: superreview+
mscott: approval1.8.1.5+
kairo: approval‑seamonkey1.1.3+
Details | Diff | Splinter Review

Description Ian Neal 2007-04-25 16:04:45 PDT
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 neil@parkwaycc.co.uk 2007-04-26 08:15:00 PDT
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.
Comment 2 Ian Neal 2007-05-28 08:03:00 PDT
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
Comment 3 Karsten Düsterloh 2007-06-09 08:26:28 PDT
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.
Comment 4 Ian Neal 2007-06-09 10:55:48 PDT
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=
Comment 5 neil@parkwaycc.co.uk 2007-06-09 16:02:55 PDT
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.
Comment 6 Ian Neal 2007-06-10 16:39:18 PDT
Created attachment 267912 [details] [diff] [review]
PrefBranch patch

Changes since v0.2:
* Switched to using a pref branch as suggested.
Comment 7 neil@parkwaycc.co.uk 2007-06-11 16:41:50 PDT
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).
Comment 8 neil@parkwaycc.co.uk 2007-06-15 03:12:39 PDT
(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.
Comment 9 Ian Neal 2007-06-16 14:43:57 PDT
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
Comment 10 neil@parkwaycc.co.uk 2007-06-16 15:31:56 PDT
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?
Comment 11 Ian Neal 2007-06-17 13:49:15 PDT
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
Comment 12 Ian Neal 2007-06-26 15:45:47 PDT
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
Comment 13 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2007-06-26 15:51:21 PDT
You should check whether the tree is closed (which it is) before checking in.
Comment 14 Ian Neal 2007-06-26 16:06:19 PDT
Comment on attachment 268702 [details] [diff] [review]
No check on gPrefBranch patch v0.5 (Checked into trunk)

Requesting approvals for branch
Comment 15 Ian Neal 2007-06-26 16:18:15 PDT
(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 Robert Kaiser 2007-06-27 04:58:33 PDT
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.
Comment 17 Ian Neal 2007-06-27 14:38:14 PDT
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.
Comment 18 Robert Kaiser 2007-06-28 05:06:18 PDT
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
Comment 19 Scott MacGregor 2007-06-29 15:56:09 PDT
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.
Comment 20 Ian Neal 2007-07-03 12:05:06 PDT
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

Note You need to log in before you can comment on or make changes to this bug.