Last Comment Bug 758618 - Account provionner: disable search button if no providers are listed
: Account provionner: disable search button if no providers are listed
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: 13 Branch
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on:
Blocks: AccountProvisioner
  Show dependency treegraph
 
Reported: 2012-05-25 07:41 PDT by Jb Piacentino
Modified: 2012-05-29 12:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Patch v1 (3.84 KB, patch)
2012-05-25 13:35 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: review+
bwinton: ui‑review+
Details | Diff | Review
Patch v2 (3.62 KB, patch)
2012-05-28 08:04 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Review

Description Jb Piacentino 2012-05-25 07:41:07 PDT
In languages where there are no default search providers, users can press 'Search' and get an error. 

The 'Search' button should be disabled if 
- no search providers are selected 
- or none are displayed
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-05-25 13:35:13 PDT
Created attachment 627341 [details] [diff] [review]
Patch v1

This patch disables the search button if there are no providers listed that support the users language.

The search button is re-enabled once a provider is checked.
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-05-25 13:40:47 PDT
I should point out that this patch relies on the patches in bug 757823, bug 758237, and bug 758707 in that order.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-05-25 13:44:00 PDT
I've applied all of those, but this one is giving me an error patching mail/test/mozmill/newmailaccount/test-newmailaccount.js
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-05-25 13:45:09 PDT
Ah, yeah, I see that. Hang on...
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-05-25 13:47:06 PDT
Ok, I had updated the patch for bug 758707 to include the test for making sure that the fields were re-enabled. Make sure you're using that patch, and not v1.  Should apply fine on top of that.
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-05-25 14:03:51 PDT
Comment on attachment 627341 [details] [diff] [review]
Patch v1

Looks good.  ui-r=me.

>+++ b/mail/components/newmailaccount/content/accountProvisioner.js
>@@ -607,16 +611,18 @@ var EmailAccountProvisioner = {
>       if (supportsSomeUserLang) {
>+        hasSupportForUserLang = true;
>@@ -629,16 +635,22 @@ var EmailAccountProvisioner = {
>+    // If we didn't load any providers that support the user's language,
>+    // disable the search button. We'll only re-enable once the user has
>+    // selected one of the other providers.
>+    if (!hasSupportForUserLang)
>+      EmailAccountProvisioner.searchButtonEnabled(false);

Instead of hoisting this variable up out of the loop, what about having EmailAccountProvisioner.searchButtonEnabled set to false by default, and then setting it to true if we get into the supportsSomeUserLang case?

Other than that, I like it.  r=me with that fixed.

Thanks,
Blake.
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-05-28 08:04:28 PDT
Created attachment 627700 [details] [diff] [review]
Patch v2

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6)
> Comment on attachment 627341 [details] [diff] [review]
> Patch v1
> 
> Looks good.  ui-r=me.
> 

Thanks!

> >+++ b/mail/components/newmailaccount/content/accountProvisioner.js
> >@@ -607,16 +611,18 @@ var EmailAccountProvisioner = {
> >       if (supportsSomeUserLang) {
> >+        hasSupportForUserLang = true;
> >@@ -629,16 +635,22 @@ var EmailAccountProvisioner = {
> >+    // If we didn't load any providers that support the user's language,
> >+    // disable the search button. We'll only re-enable once the user has
> >+    // selected one of the other providers.
> >+    if (!hasSupportForUserLang)
> >+      EmailAccountProvisioner.searchButtonEnabled(false);
> 
> Instead of hoisting this variable up out of the loop, what about having
> EmailAccountProvisioner.searchButtonEnabled set to false by default, and
> then setting it to true if we get into the supportsSomeUserLang case?
> 

So the call EmailAccountProvisioner.beOnline() that's fired just before the function exits calls EmailAccountProvisioner.searchEnabled(true), which enables all of the search fields - so setting EmailAccountProvisioner.searchButtonEnabled(false) will be overridden when the function exits, regardless of whether or not a supported language exists.

I considered refactoring the dialog's search enabling and disabling logic to be more granular for this sort of thing, but there are quite a few cases to consider, and I think the potential for it to expose us to new bugs is higher than this patch is.

Instead, I've had the provider-loading function stash a boolean into EmailAccountProvisioner for whether or not at least one provider supports the user's language. That's potentially a step in the right direction for that refactoring, which we should perhaps do in a separate bug (and not target for beta).
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-05-28 08:31:10 PDT
Comment on attachment 627700 [details] [diff] [review]
Patch v2

Okay, that seems reasonable.

Thanks,
Blake.
Comment 9 Mike Conley (:mconley) - (needinfo me!) 2012-05-28 09:54:07 PDT
comm-central: https://hg.mozilla.org/comm-central/rev/d8e6f2b61e01
Comment 10 Mike Conley (:mconley) - (needinfo me!) 2012-05-28 14:01:40 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/6ee7d7b3c428
Comment 11 Mike Conley (:mconley) - (needinfo me!) 2012-05-28 14:08:04 PDT
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/ff96b286f877

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