Last Comment Bug 825452 - system integration dialog says 'Skip integration' even when launched from preferences
: system integration dialog says 'Skip integration' even when launched from pre...
Product: Thunderbird
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2012-12-29 12:21 PST by Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout)
Modified: 2013-02-26 05:07 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (6.17 KB, patch)
2013-02-03 14:23 PST, :aceman
mconley: review+
bwinton: ui‑review+
aryx.bugmail: feedback+
Details | Diff | Splinter Review

Description User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2012-12-29 12:21:07 PST
The Integration dialog where the user can set Thunderbird as default client for mails, newsgroups etc. can open in two situations:
- automatically on startup when Thunderbird detects that it isn't the default client (and it should check for this)
- from the preferences panel 'Advanced' > 'General'

There is a 'Skip integration' button in the dialog which makes not really sense if launched from the preferences, so a conditional 'Cancel' / 'Skip integration' button would make sense (likely sent to the window as argument).
Comment 1 User image :aceman 2012-12-30 05:07:46 PST
Yeah, this is my work. I think we already know if we are called from preferences. I'd need some clever string proposal from bwinton.
Comment 2 User image Blake Winton (:bwinton) (:☕️) 2013-02-03 12:09:03 PST
What about "Cancel" if we're launched from preferences?
Comment 3 User image :aceman 2013-02-03 14:23:53 PST
Created attachment 709531 [details] [diff] [review]

No problem.
Comment 4 User image Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2013-02-04 02:47:27 PST
Comment on attachment 709531 [details] [diff] [review]

Review of attachment 709531 [details] [diff] [review]:

Thank you for the patch.
Comment 5 User image Blake Winton (:bwinton) (:☕️) 2013-02-04 07:18:45 PST
Comment on attachment 709531 [details] [diff] [review]

I like it, ui-r=me!

>+++ b/mail/components/preferences/advanced.js
>@@ -1,16 +1,17 @@
> var gAdvancedPane = {
>   mPane: null,
>   mInitialized: false,
>+  mShellServiceWorking: false,

Do we need to make this change as part of this patch?  It seems unrelated, and I worry a little about the case where we check for the shell service before it's initialized, and then set it to notWorking, and never re-check…

>@@ -83,22 +86,18 @@ var gAdvancedPane = {
>   checkDefaultNow: function (aAppType)
>   {
>-    var nsIShellService = Components.interfaces.nsIShellService;
>-    var shellSvc;
>-    try {
>-      shellSvc = Components.classes[";1"]
>-                           .getService(nsIShellService);
>-    } catch (ex) { return; }
>+    if (!this.mShellServiceWorking)
>+      return;

(In particular, this case.)

I'll let the code reviewer trace into it, and make the final decision here, but I wanted to express my concern.

Comment 6 User image :aceman 2013-02-04 07:40:45 PST
Comment on attachment 709531 [details] [diff] [review]

Yeah, that change is not needed, it is just an optimization (catching of exceptions can be costly).
I think when the preferences dialog is opened the shell-service should be already initialized (from app startup time?). So caching the state should be fine while in the dialog.
Comment 7 User image Kent James (:rkent) 2013-02-14 13:21:07 PST
I don't know if this is the same bug or not, but every time I create a new profile, I get a System Integration prompt. OK, but all I want to do is to properly set the default client values, and ask that the dialog be accepted.

But it is not clear to me what I am supposed to do. "Set as Default" seems wrong, since I am explictly telling it to NOT set Earlybird as the default client. "Skip Integration" sounds like the same thing as "Cancel".

I suppose what I should do is to say "Set as Default" that it is NOT the default, which really seems strange to me. I am tempted to check the code to see what this is supposed to do, but how does the average user make sense of this?

What happened to plain old "OK" and "Cancel"? That I understand.
Comment 8 User image :aceman 2013-02-14 13:38:44 PST
They were renamed as they were not clear. See bug 476426.
Comment 9 User image Mike Conley (:mconley) 2013-02-25 18:35:33 PST
Comment on attachment 709531 [details] [diff] [review]

Review of attachment 709531 [details] [diff] [review]:

It's true that the optimization in here is orthogonal to the problem that we're trying to solve, but I also think it helps to clear up what's going on. I like it.

Thanks Archaeopteryx!
Comment 10 User image Ryan VanderMeulen [:RyanVM] 2013-02-26 05:07:40 PST

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