476426 - Thunderbird needs a "NO" option for the initial "Use Thunderbird as the Default Client for" dialog

Status: RESOLVED FIXED

(Thunderbird :: OS Integration, defect)

Description

[IU]

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)]

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]]

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)]

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)]

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]]

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)]

I like it

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

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]

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

[Rain [:rain1] (inactive)]

"Uncheck everything and click OK" is a workaround for now.

Comment 10

[IU]

(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]

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]

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

[Rain [:rain1] (inactive)]

Yeah I agree, that dialog's in need of a complete overhaul.

Comment 14

[:aceman]

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]

Does this tag actually so anything?
<?xml-stylesheet href="chrome://global/skin/"?>

Comment 16

[:aceman]

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) (:☕️)]

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]

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]

Attachment: WIP patch

What about this? (modulo the string entities:))

Comment 22

[Blake Winton (:bwinton) (:☕️)]

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]

Attachment: patch v2

Ok, this adds the entities.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

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]

Attachment: patch v3

Comment 26

[Mike Conley (:mconley) (:⚙️)]

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]

Attachment: 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) (:☕️)]

Comment on attachment 670072 [details] [diff] [review]
patch v4

ui-r=me!

Thanks,
Blake.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

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]

Attachment: patch v5

Thanks.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4dae5d540baf