Closed
Bug 755454
Opened 13 years ago
Closed 13 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)
MailNews Core
Account Manager
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)
(deleted),
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
+++ 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.
Attachment #624154 -
Flags: review?(iann_bugzilla)
I think this does not need ui-review as the same widgets are used throughout the Account manager.
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.
Attachment #624154 -
Attachment is obsolete: true
Attachment #624195 -
Flags: review?(mconley)
Comment 8•13 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 */
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•13 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
@@ +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•13 years ago
|
||
(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•13 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.
Comment 13•13 years ago
|
||
(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•13 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?
Comment 15•13 years ago
|
||
Since we're talking about minutes, I think using an increment of "1" makes sense.
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #624195 -
Attachment is obsolete: true
Attachment #624195 -
Flags: review?(mconley)
Attachment #626180 -
Flags: review?(mconley)
Comment 17•13 years ago
|
||
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
Comment 18•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
Assignee | ||
Comment 19•13 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.
Description
•