Last Comment Bug 755454 - Make the "Check for new articles every X minutes" field in RSS account settings numeric and disabled when not needed
: Make the "Check for new articles every X minutes" field in RSS account settin...
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: 532391
Blocks: 728681 755885 755898 755518
  Show dependency treegraph
 
Reported: 2012-05-15 12:38 PDT by :aceman
Modified: 2012-05-24 00:18 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.66 KB, patch)
2012-05-15 12:51 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v2 (5.19 KB, patch)
2012-05-15 14:32 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v3 (5.18 KB, patch)
2012-05-22 14:19 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review

Description :aceman 2012-05-15 12:38:27 PDT
+++ This bug was initially created as a clone of Bug #728681 +++

There are several pairs of fields of the pattern: checkbox and numeric field.
The numeric field is only enabled when the checkbox is enabled.
Examples of such pair is the "check for new messages every X minutes" or "ask me before downloading X messages". Some of the fields have some checks whether the entered value is correct and within sane boundaries. Some of the fields do not have such check.
The purpose of this bug is to build a shared set of checks and add the checks where they are missing.

This bug implements point a) from bug 728681 (and also the movement of onCheckItem into amUtils.js).
a) RSS "Check for new articles every X minutes"
Comment 1 :aceman 2012-05-15 12:40:00 PDT
Alta88, can I assume that a setting of "0 minutes" does not make much sense? So I would set a minimum of 1 (minute) on the field.
Comment 2 :aceman 2012-05-15 12:51:54 PDT
Created attachment 624154 [details] [diff] [review]
patch
Comment 3 :aceman 2012-05-15 12:53:08 PDT
I think this does not need ui-review as the same widgets are used throughout the Account manager.
Comment 4 Ian Neal 2012-05-15 14:00:31 PDT
Comment on attachment 624154 [details] [diff] [review]
patch

>+++ b/mailnews/base/prefs/content/am-server.js
>@@ -210,27 +210,16 @@ function secureSelect(aLoading)
> 
> function setupMailOnServerUI()
> { 
>    var checked = document.getElementById("pop3.leaveMessagesOnServer").checked;
>    var locked = getAccountValueIsLocked(document.getElementById("pop3.leaveMessagesOnServer"));
>    document.getElementById("pop3.deleteMailLeftOnServer").disabled = locked || !checked ;
>    setupAgeMsgOnServerUI();
> }
This function could also make use of onCheckItem function.
am-identity-edit.js also has places that could make use of the function.
Both these could be done in a followup bug.

>+++ b/mailnews/base/prefs/content/amUtils.js
+function onCheckItem(changeElementId, checkElementId)

Just wondering if this function would be better moved to am-prefs.js rather than amUtils.js, on balance I think it should be.

r=me with those points addressed.

There is also am-offline.xul/js but that is a whole new ball game.
Comment 5 alta88 2012-05-15 14:07:11 PDT
(In reply to :aceman from comment #1)
> Alta88, can I assume that a setting of "0 minutes" does not make much sense?
> So I would set a minimum of 1 (minute) on the field.

the number picker in other account types will auto uncheck the box if 0 is reached via button.  however, it seems buggy that 0 can be entered manually and saved with the box checked, i'm not sure how the generic biff code handles that..

imo the feed picker should follow the behavior of biff minutes pickers in other account types, perhaps all should have a min value of 1.
Comment 6 :aceman 2012-05-15 14:17:14 PDT
Alta88, thanks, the biff minutes in other accounts are already changed in bug 532391 to not allow 0 so this one is actually updating RSS to be in line.

IanN, thanks, I am just going to file the bug for migrating setupMailOnServerUI() and setupAgeMsgOnServerUI() to onCheckItem(). I can move it to am-prefs.js as you wish.

I am going through the various number fields in bug 728681.
Yes, am-offline is hard.
Comment 7 :aceman 2012-05-15 14:32:41 PDT
Created attachment 624195 [details] [diff] [review]
patch v2
Comment 8 Magnus Melin 2012-05-15 22:40:22 PDT
Comment on attachment 624195 [details] [diff] [review]
patch v2

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

::: mailnews/base/prefs/content/am-prefs.js
@@ +126,5 @@
> + *                         if the checkElement is checked/unchecked
> + * @param checkElementId  the master element which changed .checked state
> + *
> + * See bug 728681 for the pattern on how this is used.
> + **/

Nit: the end of the comment is just a single */
Comment 9 :aceman 2012-05-16 13:28:34 PDT
Actually I think I see how am-offline.js::onCheckItem() works, it just uses gLockedPref array. I think I'll touch am-offline.js::disableIfLocked() function too as in https://bugzilla.mozilla.org/show_bug.cgi?id=754579#c12 so maybe it can be merged too. I will file the bug straight away.
Comment 10 Mike Conley (:mconley) - (needinfo me!) 2012-05-22 13:35:17 PDT
Comment on attachment 624195 [details] [diff] [review]
patch v2

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

::: mailnews/base/prefs/content/am-prefs.js
@@ +129,5 @@
> + * See bug 728681 for the pattern on how this is used.
> + **/
> +function onCheckItem(changeElementId, checkElementId)
> +{
> +  var element = document.getElementById(changeElementId);

We prefer let over var.

::: mailnews/base/prefs/content/am-server.js
@@ +213,5 @@
>          socketType == Ci.nsMsgSocketType.SSL ||
>          socketType == Ci.nsMsgSocketType.alwaysSTARTTLS ?
>          "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
>  }
>  

Can I assume that ALL callers of this function now fall through to the implementation in am-prefs.js?

::: mailnews/extensions/newsblog/content/am-newsblog.xul
@@ +101,3 @@
>                   size="3"
> +                 min="1"
> +                 increment="10"

Why are you changing the increment?
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2012-05-22 13:36:12 PDT
(In reply to Mike Conley (:mconley) from comment #10)
> Comment on attachment 624195 [details] [diff] [review]
> patch v2
> 
> Review of attachment 624195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/am-newsblog.xul
> @@ +101,3 @@
> >                   size="3"
> > +                 min="1"
> > +                 increment="10"
> 
> Why are you changing the increment?

Or, more specifically, why are you changing the increment to 10?
Comment 12 :aceman 2012-05-22 13:46:04 PDT
(In reply to Mike Conley (:mconley) from comment #10)
> ::: mailnews/base/prefs/content/am-server.js
> @@ +213,5 @@
> >          socketType == Ci.nsMsgSocketType.SSL ||
> >          socketType == Ci.nsMsgSocketType.alwaysSTARTTLS ?
> >          "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
> >  }
> >  
> 
> Can I assume that ALL callers of this function now fall through to the
> implementation in am-prefs.js?
What do you mean?

> ::: mailnews/extensions/newsblog/content/am-newsblog.xul
> @@ +101,3 @@
> >                   size="3"
> > +                 min="1"
> > +                 increment="10"
> 
> Why are you changing the increment?
It appeared to me that 10 would be a good increment here. I have no problem with any value you propose.
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2012-05-22 13:47:37 PDT
(In reply to :aceman from comment #12)
> (In reply to Mike Conley (:mconley) from comment #10)
> > ::: mailnews/base/prefs/content/am-server.js
> > @@ +213,5 @@
> > >          socketType == Ci.nsMsgSocketType.SSL ||
> > >          socketType == Ci.nsMsgSocketType.alwaysSTARTTLS ?
> > >          "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
> > >  }
> > >  
> > 
> > Can I assume that ALL callers of this function now fall through to the
> > implementation in am-prefs.js?
> What do you mean?

You've moved a function to another file. Can I assume that you've checked to ensure that all callers of that function import that other file?
Comment 14 :aceman 2012-05-22 13:59:45 PDT
Callers of onCheckItem() from inside am-server.js import am-prefs.js. I couldn't see any file that imports am-server.js so that it could expect to use its onCheckItem(). I am planning to play with that function more and convert more callers to the one in am-prefs.js now. See the dependent bugs.

What about the increment value?
Comment 15 Mike Conley (:mconley) - (needinfo me!) 2012-05-22 14:01:23 PDT
Since we're talking about minutes, I think using an increment of "1" makes sense.
Comment 16 :aceman 2012-05-22 14:19:33 PDT
Created attachment 626180 [details] [diff] [review]
patch v3
Comment 17 Mike Conley (:mconley) - (needinfo me!) 2012-05-22 14:20:56 PDT
Comment on attachment 626180 [details] [diff] [review]
patch v3

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

Let's roll with this. Thanks aceman - great work.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-05-22 16:26:46 PDT
https://hg.mozilla.org/comm-central/rev/f4d49b713463
Comment 19 :aceman 2012-05-24 00:18:32 PDT
Alta88, could you please verify the fix in a recent nightly?

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