Last Comment Bug 755518 - make "For at most X days" field in server settings numeric (as in bug 728681)
: make "For at most X days" field in server settings numeric (as in bug 728681)
Status: RESOLVED FIXED
: polish, ux-error-prevention
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 15.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 755454
Blocks: 728681 755898
  Show dependency treegraph
 
Reported: 2012-05-15 14:40 PDT by :aceman
Modified: 2012-06-02 11:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (6.95 KB, patch)
2012-05-23 13:51 PDT, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v2 (9.55 KB, patch)
2012-05-26 03:57 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v3 (11.25 KB, patch)
2012-05-26 11:23 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v4 (11.25 KB, patch)
2012-05-27 05:20 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v5 (11.25 KB, patch)
2012-05-30 13:23 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-05-15 14:40:58 PDT
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 2012-05-15 14:55:36 PDT
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 2012-05-23 13:51:48 PDT
Created attachment 626589 [details] [diff] [review]
patch
Comment 3 Ian Neal 2012-05-25 10:25:13 PDT
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 2012-05-25 14:31:21 PDT
(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 2012-05-26 03:57:27 PDT
Created attachment 627463 [details] [diff] [review]
patch v2
Comment 6 Ian Neal 2012-05-26 11:04:39 PDT
Comment on attachment 627463 [details] [diff] [review]
patch v2

What about am-offline.js and am-newsblog.js/xul?
Comment 7 :aceman 2012-05-26 11:23:55 PDT
Created attachment 627500 [details] [diff] [review]
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 2012-05-27 04:57:09 PDT
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 2012-05-27 05:20:53 PDT
Created attachment 627555 [details] [diff] [review]
patch v4

Thanks, done.
Comment 10 Mike Conley (:mconley) 2012-05-30 13:11:20 PDT
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 2012-05-30 13:23:09 PDT
Created attachment 628436 [details] [diff] [review]
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) 2012-06-02 11:54:45 PDT
https://hg.mozilla.org/comm-central/rev/88ce9f09dda7

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