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...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-29 12:21 PST by Sebastian Hengst [:aryx][:archaeopteryx]
Modified: 2013-02-26 05:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
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 Sebastian Hengst [:aryx][:archaeopteryx] 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 :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 Blake Winton (:bwinton) (:☕️) 2013-02-03 12:09:03 PST
What about "Cancel" if we're launched from preferences?
Comment 3 :aceman 2013-02-03 14:23:53 PST
Created attachment 709531 [details] [diff] [review]
patch

No problem.
Comment 4 Sebastian Hengst [:aryx][:archaeopteryx] 2013-02-04 02:47:27 PST
Comment on attachment 709531 [details] [diff] [review]
patch

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

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

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["@mozilla.org/mail/shell-service;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.

Thanks,
Blake.
Comment 6 :aceman 2013-02-04 07:40:45 PST
Comment on attachment 709531 [details] [diff] [review]
patch

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 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 :aceman 2013-02-14 13:38:44 PST
They were renamed as they were not clear. See bug 476426.
Comment 9 Mike Conley (:mconley) - (needinfo me!) 2013-02-25 18:35:33 PST
Comment on attachment 709531 [details] [diff] [review]
patch

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 Ryan VanderMeulen [:RyanVM] 2013-02-26 05:07:40 PST
https://hg.mozilla.org/comm-central/rev/acf1d80ce58d

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