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

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[:aceman]

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.

Comment 1

[:aceman]

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").

Comment 2

[:aceman]

Attachment: patch

Comment 3

[Ian Neal]

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.

Comment 4

[:aceman]

(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.

Comment 5

[:aceman]

Attachment: patch v2

Comment 6

[Ian Neal]

Comment on attachment 627463 [details] [diff] [review]
patch v2

What about am-offline.js and am-newsblog.js/xul?

Comment 7

[:aceman]

Attachment: patch v3

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.

Comment 8

[Ian Neal]

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

Comment 9

[:aceman]

Attachment: patch v4

Thanks, done.

Comment 10

[Mike Conley (: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.

Comment 11

[:aceman]

Attachment: patch v5

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).

Comment 12

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/comm-central/rev/88ce9f09dda7