Last Comment Bug 476426 - Thunderbird needs a "NO" option for the initial "Use Thunderbird as the Default Client for" dialog
: Thunderbird needs a "NO" option for the initial "Use Thunderbird as the Defau...
Status: RESOLVED FIXED
: ux-control
Product: Thunderbird
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
:
Mentors:
: 495936 599221 (view as bug list)
Depends on: 841533 1036592
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-02 01:37 PST by IU
Modified: 2014-10-30 08:00 PDT (History)
12 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (11.39 KB, patch)
2012-09-26 11:05 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (12.41 KB, patch)
2012-10-02 14:50 PDT, :aceman
mconley: review-
Details | Diff | Splinter Review
patch v3 (12.65 KB, patch)
2012-10-03 14:07 PDT, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v4 (14.18 KB, patch)
2012-10-10 12:23 PDT, :aceman
mconley: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v5 (14.23 KB, patch)
2012-10-16 14:10 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description IU 2009-02-02 01:37:50 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090201 Lightning/1.0pre

The "Default Client" nag that appears for new Shredder installations badly needs a "NO" option, as none of the options presently available on that dialog allow a user to permanently dismiss the dialog and allow a different default messaging client to exist.  Unchecking "Always perform this check when starting Shredder" and clicking cancel DOES NOT work.

The only other way to dismiss that dialog for good is to "Tools --> Options... --> General" and unchecking "Always check to see if Shredder is the default mail client on startup."  A user should not have to search around and come this far when the dialog should have taken care of it.


Reproducible: Always

Steps to Reproduce:
1. Download the .zip build of Shredder and extract to a different location
2. Create a shortcut to launch a profile with the newly extracted Shredder
3. Launch Shredder via the shortcut.  You will now get the "Default Client" nag.
4. Uncheck "Always perform this check when starting Shredder" and click Cancel
5. Close Shredder.
6. Launch Shredder via the shortcut.  Notice you still get the nag.
7. Now go to "Tools --> Options... --> General" and uncheck "Always check to see if Shredder is the default mail client on startup."  Click OK.
8. Launch Shredder via the shortcut.  Notice the nag is finally gone.
Comment 1 Wayne Mery (:wsmwk, NI for questions) 2009-02-02 02:58:14 PST
I've recently been thinking about this pain, and agree with the premise.

I think FF now has a checkbox on an install panel.  But I'm not sure if it kills the prompt forever.
Comment 2 Bryan Clark (DevTools PM) [:clarkbw] 2009-02-02 12:01:00 PST
Agreed, I've seen this several times.  You'd think checking the [x] don't bother me with this question would be enough.  Likely we should just change the dialog away from an OK/Cancel choice to action verbs buttons and then we can use the secondary button to honor the checkbox choice.
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2009-02-02 12:15:29 PST
confirming. I'll let someone else decide if this is the right component. (it occurred to me non-windows systems don't have the installer)
Comment 4 Mark Banner (:standard8, limited time in Dec) 2009-02-03 02:11:19 PST
Sounds like this is wanted, though will the work in bug 443358 mean this is not necessary? Assigning to Bryan for further comment.
Comment 5 Bryan Clark (DevTools PM) [:clarkbw] 2009-02-09 12:35:18 PST
I think bug 443358 is a little orthogonal to this issue.  If you don't check make default during install you'll see this nag dialog.

It shouldn't be hard to change the buttons into these:
( Don't Make Default ) ( Make Default )

That removes the "Cancel" expectations from the dialog so we can use the "Don't" action to actually set the nag pref
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2009-02-09 13:28:54 PST
I like it
Comment 7 Bryan Clark (DevTools PM) [:clarkbw] 2009-08-07 10:29:25 PDT
resetting this so anyone can take it over.

This is mostly a matter of getting setup and then going into these files and making the right changes to the button actions and text.

http://mxr.mozilla.org/comm-central/find?text=&kind=text&string=systemIntegrationDialog
Comment 8 IU 2010-12-13 16:43:30 PST
Any reason why this hasn't been fixed yet?  I'm using Shredder for testing, in a corporate environment standardized to Outlook, and this bug is very annoying, as every launch of Shredder is nag nag nag.
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2010-12-14 01:35:05 PST
"Uncheck everything and click OK" is a workaround for now.
Comment 10 IU 2010-12-16 22:11:42 PST
(In reply to comment #9)
> "Uncheck everything and click OK" is a workaround for now.

Thanks.  That worked.  Still a more meaningful UI would be nice.
Comment 11 :aceman 2012-09-26 03:46:06 PDT
Which OS is this?

But yes, I can look if the pref behind "Always perform this check when starting Shredder" is stored properly.
Comment 12 :aceman 2012-09-26 03:57:55 PDT
I tried this on Windows XP. I think the dialog technically behaves as designed. Clicking Cancel does not remember any changes you have made in it, including unchecking the "always perform check". That is the usual dialog semantics.

If you need to preserve that unchecking you must click OK but that also saves the checks alongside the Email, News, Feeds rows.
(For some reason The I have Feeds checked and can't uncheck it, the checkbox is disabled).

So this would need UI decision how to reword and restructure the dialog.
Actually the new button of "No" that the reporter requested would make sense, it could behave as "OK", just completely ignore the checks for Email, News, Feeds. It would store "always perform check" as the user set it (would also allow it being unchecked).

1. if "always perform check" is unchecked, "NO" would behave equivalent to "Cancel".
2. if "always perform check" is checked, "NO" would behave equivalent to "OK" but not setting TB as default for anything.

"Cancel" could actually be removed and the "OK" and "NO" relabeled to something better, like "Set default" and "No integration".

Bwinton?
Comment 13 Siddharth Agarwal [:sid0] (inactive) 2012-09-26 04:14:23 PDT
Yeah I agree, that dialog's in need of a complete overhaul.
Comment 14 :aceman 2012-09-26 04:41:11 PDT
My proposal is to just change the meaning of the Cancel button and relabel it appropriately. But if there are any other enhancements proposed I can try to implement them.
Comment 15 :aceman 2012-09-26 05:00:39 PDT
Does this tag actually so anything?
<?xml-stylesheet href="chrome://global/skin/"?>
Comment 16 :aceman 2012-09-26 06:51:02 PDT
So I have a nice patch that overhauls the dialog internally without visible changes, except the button labels :)
So suppose we have two buttons now:
1. "set as default" = stores everything checked in the dialog
2. "no integration" = stores only the state of the "always check" checkbox.

The is one open question: what to do with the "search integration" checkbox that shows up on supported systems? Should it be saved on "no integration"-click or not? Or actually forced to unchecked as we said "no integration"?
Comment 17 Blake Winton (:bwinton) (:☕️) 2012-09-26 06:55:50 PDT
In the interests of simplification, I wonder if we need the checkboxes at all, or if we can just say "Set Thunderbird as your default mail and news application? / Yes / No", and avoid the question entirely…
Comment 18 :aceman 2012-09-26 06:59:39 PDT
There are already 3 options (4 with search integration) which I don't think can be answered by 1 answer at once.

And also it seems we can't remove existing integration, that is why I see my Feeds checked and disabled so I can't remove it.

Therefore "no integration" would be a bit misleading too.
Maybe "skip integration" would be better.
Comment 19 :aceman 2012-09-26 11:05:37 PDT
Created attachment 665058 [details] [diff] [review]
WIP patch

What about this? (modulo the string entities:))
Comment 20 :aceman 2012-09-27 01:40:44 PDT
*** Bug 495936 has been marked as a duplicate of this bug. ***
Comment 21 :aceman 2012-09-27 01:45:30 PDT
*** Bug 599221 has been marked as a duplicate of this bug. ***
Comment 22 Blake Winton (:bwinton) (:☕️) 2012-10-01 10:03:30 PDT
Comment on attachment 665058 [details] [diff] [review]
WIP patch

Yeah, this seems better.  ui-r=me.

As a localization note, I think the buttonlabelaccept and buttonlabelcancel should be entities, like title is.

Thanks,
Blake.
Comment 23 :aceman 2012-10-02 14:50:33 PDT
Created attachment 667186 [details] [diff] [review]
patch v2

Ok, this adds the entities.
Comment 24 Mike Conley (:mconley) 2012-10-03 13:09:34 PDT
Comment on attachment 667186 [details] [diff] [review]
patch v2

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

Just a few minor points, nothing too major. On the whole, this is a big improvement to what we currently have. :)  Thanks!

::: mail/base/content/systemIntegrationDialog.js
@@ +7,4 @@
>  
> +let gSystemIntegrationDialog = {
> +  _shellSvc: Components.classes["@mozilla.org/mail/shell-service;1"]
> +                               .getService(Components.interfaces.nsIShellService),

Nit - please align the period in .getService vertically with the period in .classes

@@ +44,2 @@
>      // read the raw pref value and not shellSvc.shouldCheckDefaultMail
> +    Components.utils.import("resource://gre/modules/Services.jsm", this);

Hm - I think you should be able to just import Services.jsm into this function scope, if you remove the "this" argument.

Then you can just use Services.prefs.getBoolPref as opposed to this.Services.prefs.getBoolPref, which just looks weird.

@@ +44,3 @@
>      // read the raw pref value and not shellSvc.shouldCheckDefaultMail
> +    Components.utils.import("resource://gre/modules/Services.jsm", this);
> +    this._startupCheckbox.checked = 

Nit - trailing whitespace

@@ +48,3 @@
>  
> +    // Search integration -- check whether we should show/disable integration
> +    let showSearchUI = false;

I don't think showSearchUI or disableSearchUI are really necessary. You can just use:

if (!SearchIntegration.osVersionTooLow) {
  this._searchCheckbox.hidden = false;
  if (SearchIntegration.osComponentNotRunning) {
    // ..etc
  }
}

@@ +71,4 @@
>      }
>    },
> +
> +  onAccept: function(aSetAsDefault)

Some documentation to describe what aSetAsDefault does would be good.

::: mail/base/content/systemIntegrationDialog.xul
@@ +38,2 @@
>          onload="gSystemIntegrationDialog.onLoad();"
> +        ondialogaccept="return gSystemIntegrationDialog.onAccept(true);"

Hm - I think onAccept is no longer the correct name for this function.

How about onDialogClosed instead?

::: mail/locales/en-US/chrome/messenger/systemIntegrationDialog.dtd
@@ +2,5 @@
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY systemIntegration.title  "System Integration">
> +<!ENTITY acceptIntegration.label  "Set as Default">

Are we sure on the capitalization here?  How about Default is capitalized, but integration is not?
Comment 25 :aceman 2012-10-03 14:07:16 PDT
Created attachment 667647 [details] [diff] [review]
patch v3
Comment 26 Mike Conley (:mconley) 2012-10-10 10:28:18 PDT
Comment on attachment 667647 [details] [diff] [review]
patch v3

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

This looks good - r=me with these last two nits fixed.

::: mail/base/content/systemIntegrationDialog.js
@@ +44,2 @@
>      // read the raw pref value and not shellSvc.shouldCheckDefaultMail
> +    Components.utils.import("resource://gre/modules/Services.jsm");

I'd rather this import occur at the top of the function.

@@ +50,1 @@
>      Components.utils.import("resource:///modules/SearchIntegration.js");

I'd rather this import occur at the top of the function.
Comment 27 :aceman 2012-10-10 12:23:30 PDT
Created attachment 670072 [details] [diff] [review]
patch v4

This corrects a problem introduced in bug 595723. If called from Preferences dialog, the user probably just wants to check what the current state is, so do not preset the "Mail" check box. Please try both ways and also on a system where SearchIntegration is available (Win7 maybe and Mac???) to see if it works as I changed the imports a bit. Thanks.
Comment 28 Blake Winton (:bwinton) (:☕️) 2012-10-11 08:42:17 PDT
Comment on attachment 670072 [details] [diff] [review]
patch v4

ui-r=me!

Thanks,
Blake.
Comment 29 Mike Conley (:mconley) 2012-10-16 11:45:53 PDT
Comment on attachment 670072 [details] [diff] [review]
patch v4

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

I like the look of this. Thanks aceman!

::: mail/base/content/systemIntegrationDialog.js
@@ +23,3 @@
>    {
> +    Components.utils.import("resource://gre/modules/Services.jsm", this);
> +    // This imports a this.Searchintegration object.

I think the comment should read:

Makes Services and SearchIntegration accessible via this.Services and this.SearchIntegration.
Comment 30 :aceman 2012-10-16 14:10:49 PDT
Created attachment 672017 [details] [diff] [review]
patch v5

Thanks.
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-10-19 17:18:03 PDT
https://hg.mozilla.org/comm-central/rev/4dae5d540baf

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