Closed Bug 755518 Opened 8 years ago Closed 8 years ago

make "For at most X days" field in server settings numeric (as in bug 728681)

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 15.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 2 open bugs)

Details

(Keywords: polish, ux-error-prevention)

Attachments

(1 file, 4 obsolete files)

Currently it is possible to enter letters into the "For at most X days" field (which get converted to 0).
Also the functions setupMailOnServerUI() and setupAgeMsgOnServerUI() could be reduced to onCheckItem() as proposed in bug 728681.

Also, on the same pane, the server port field should not allow setting a port of 0.
Depends on: 755454
Converting setupAgeMsgOnServerUI() will need to extend onCheckItem() to allow several "master" elements that must all be checked to enable the slave element.
That will also be usefull on e.g. the Junk settings panel (for "delete junk older than X days").
Blocks: 755898
No longer blocks: 755898
Blocks: 755898
Attached patch patch (obsolete) — Splinter Review
Attachment #626589 - Flags: review?(iann_bugzilla)
Status: NEW → ASSIGNED
Comment on attachment 626589 [details] [diff] [review]
patch

>+++ b/mailnews/base/prefs/content/am-prefs.js
>+function onCheckItem(changeElementId, checkElementIds)
If you are changing this line you may as well make the arguments of the form aVariableName.
> {
>+  let elementToControl = document.getElementById(changeElementId);
> 
>+  let enabled = true;
>+  if (!(checkElementIds instanceof Array))
>+    checkElementIds = [ checkElementIds ];
My preference is you change all the callers so they pass an array (even it is an array of one).

>+++ b/mailnews/base/prefs/content/am-server.js
> function setupMailOnServerUI()
>+{
>+  onCheckItem('pop3.deleteMailLeftOnServer', 'pop3.leaveMessagesOnServer');
Nit: this file uses double quotes not single quotes, so please use double.
>+  setupAgeMsgOnServerUI();
> }
> 
> function setupAgeMsgOnServerUI()
>+{
>+  const leaveMsgsId = 'pop3.leaveMessagesOnServer';
>+  const deleteByAgeId = 'pop3.deleteByAgeFromServer';
>+  onCheckItem(deleteByAgeId, leaveMsgsId);
>+  onCheckItem('daysEnd',  leaveMsgsId);
>+  onCheckItem('pop3.numDaysToLeaveOnServer', [ leaveMsgsId, deleteByAgeId ]);
Nit: this file uses double quotes not single quotes, so please use double.
Nit: you don't need spaces between [ and the const.
My preference is not to use the constants here but just the actual value each time.

r- for the moment as I would like to see the new patch.
Attachment #626589 - Flags: review?(iann_bugzilla) → review-
(In reply to Ian Neal from comment #3)
> > function setupAgeMsgOnServerUI()
> >+{
> >+  const leaveMsgsId = 'pop3.leaveMessagesOnServer';
> >+  const deleteByAgeId = 'pop3.deleteByAgeFromServer';
> >+  onCheckItem(deleteByAgeId, leaveMsgsId);
> >+  onCheckItem('daysEnd',  leaveMsgsId);
> >+  onCheckItem('pop3.numDaysToLeaveOnServer', [ leaveMsgsId, deleteByAgeId ]);
> My preference is not to use the constants here but just the actual value
> each time.
Why? Isn't it cleaner and more obvious with constants? I have personally made a mistake in the IDs that only got visible when I converted it to constants.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #626589 - Attachment is obsolete: true
Attachment #627463 - Flags: review?(iann_bugzilla)
Comment on attachment 627463 [details] [diff] [review]
patch v2

What about am-offline.js and am-newsblog.js/xul?
Attachment #627463 - Flags: review?(iann_bugzilla)
Attached patch patch v3 (obsolete) — Splinter Review
Thanks for catching feeds. But what's up with am-offline? It has its own copy of onCheckItem and will be migrated in bug 755885.
Attachment #627463 - Attachment is obsolete: true
Attachment #627500 - Flags: review?(iann_bugzilla)
Comment on attachment 627500 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/prefs/content/am-server.js
> function setupAgeMsgOnServerUI()
>+{
>+  const leaveMsgsId = "pop3.leaveMessagesOnServer";
>+  const deleteByAgeId = "pop3.deleteByAgeFromServer";
>+  onCheckItem(deleteByAgeId, [leaveMsgsId]);
>+  onCheckItem("daysEnd",  [leaveMsgsId]);
Nit: extra space after ,
>+  onCheckItem("pop3.numDaysToLeaveOnServer", [leaveMsgsId, deleteByAgeId]);
> }
r=me with that fixed
Attachment #627500 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Thanks, done.
Attachment #627500 - Attachment is obsolete: true
Attachment #627555 - Flags: review?(mconley)
Comment on attachment 627555 [details] [diff] [review]
patch v4

Review of attachment 627555 [details] [diff] [review]:
-----------------------------------------------------------------

Just one super tiny nit. r=me with that fixed. Good work!

::: mailnews/base/prefs/content/am-server.js
@@ +222,5 @@
>  }
>  
>  function setupAgeMsgOnServerUI()
> +{
> +  const leaveMsgsId = "pop3.leaveMessagesOnServer";

const variables should begin with a k - see Variable Prefixes under https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Naming_and_formatting_code

@@ +226,5 @@
> +  const leaveMsgsId = "pop3.leaveMessagesOnServer";
> +  const deleteByAgeId = "pop3.deleteByAgeFromServer";
> +  onCheckItem(deleteByAgeId, [leaveMsgsId]);
> +  onCheckItem("daysEnd", [leaveMsgsId]);
> +  onCheckItem("pop3.numDaysToLeaveOnServer", [leaveMsgsId, deleteByAgeId]);

I like how you've axed a lot of the extraneous logic from this function, and put it into onCheckItem instead. That's good stuff.
Attachment #627555 - Flags: review?(mconley) → review+
Attached patch patch v5Splinter Review
Thanks!
The new code is probably a tiny bit slower but is not used much and is probably more readable. I hope to migrate more stuff to the onCheckItem and use the cache of locked prefs so it gets fast again (bug 755885).
Attachment #627555 - Attachment is obsolete: true
Attachment #628436 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/88ce9f09dda7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.