The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Account Manager
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 3 bugs, {polish, ux-error-prevention})

Trunk
Thunderbird 15.0
polish, ux-error-prevention
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.18 KB, patch
mconley
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ 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"
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 624154 [details] [diff] [review]
patch
Attachment #624154 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 3

5 years ago
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 4

5 years ago
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+

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 624195 [details] [diff] [review]
patch v2
Attachment #624154 - Attachment is obsolete: true
Attachment #624195 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Blocks: 755518

Comment 8

5 years ago
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 */
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 755885
(Assignee)

Updated

5 years ago
Blocks: 755898

Updated

5 years ago
No longer blocks: 755898
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 12

5 years ago
(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?
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Created attachment 626180 [details] [diff] [review]
patch v3
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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/f4d49b713463
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
(Assignee)

Comment 19

5 years ago
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.