Closed Bug 755454 Opened 9 years ago Closed 9 years ago

Make the "Check for new articles every X minutes" field in RSS account settings numeric and disabled when not needed

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 15.0

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 3 open bugs)

Details

(Keywords: polish, ux-error-prevention)

Attachments

(1 file, 2 obsolete files)

+++ 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"
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #624154 - Flags: review?(iann_bugzilla)
I think this does not need ui-review as the same widgets are used throughout the Account manager.
Blocks: 728681
No longer depends on: 728681
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.
Attachment #624154 - Flags: review?(iann_bugzilla) → review+
(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.
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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #624154 - Attachment is obsolete: true
Attachment #624195 - Flags: review?(mconley)
Blocks: 755518
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 */
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.
Blocks: 755885
Blocks: 755898
No longer blocks: 755898
Blocks: 755898
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?
(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?
(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.
(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?
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?
Since we're talking about minutes, I think using an increment of "1" makes sense.
Attached patch patch v3Splinter Review
Attachment #624195 - Attachment is obsolete: true
Attachment #624195 - Flags: review?(mconley)
Attachment #626180 - Flags: review?(mconley)
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.
Attachment #626180 - Flags: review?(mconley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f4d49b713463
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
Alta88, could you please verify the fix in a recent nightly?
You need to log in before you can comment on or make changes to this bug.