123596 - should we have a pref "default_mail_client"

Status: VERIFIED FIXED

(MailNews Core :: Simple MAPI, defect, P2)

Description

[Rajiv Dayal]

Since the MAPI setting for default mail application is stored in Windows
registry, do we need a separate pref "mailnews.default_mail_client" for
represting whether Mozilla is the default mail app ? A problem this poses is
that if some other messaging app is made the default mail client the MAPI
settings will be changed in the Windows registry but the Mozilla's pref
"mailnews.default_mail_client" will still be unchanged.

Comment 1

[Rajiv Dayal]

The fix below removes the unnecessary and problematic MAPI pref
"mailnews.default_mail_client" and rather just uses the Windows regsitry
settings for setting / un-setting itself as the default mail client. 

This solves the stated problem in bug summary above and also related problems like: 
a) different settings seen for different profiles although there is only one
system wide setting for MAPI, b) mozilla setting itself as default mail
application without asking the user which may overwrite another messaging app's
settings and mapi Dll.

Comment 2

[Rajiv Dayal]

Attachment: patch to fix problems related to MAPI settings

Hi JF and Scott,
Can you please r and sr the patch respectively.
thanks, 
- Rajiv.

Comment 3

[Jean-Francois Ducarroz]

1)
+                if (prefBranch.getBoolPref("defaultMailClient"))
+                    mapiRegistry.isDefaultMailClient = true ;
+                else
+                    mapiRegistry.isDefaultMailClient = false ;


could be:
   mapiRegistry.isDefaultMailClient = prefBranch.getBoolPref("defaultMailClient");

Also, you could define "defaultMailClient" as a constante instead of harcoding
it twice in the same file!


2)
+        if (parent.mapiPref.isDefaultMailClient)
+            mapiRegistry.isDefaultMailClient = true;
         else
-            mapiRegistry.unsetDefaultMailClient();
+            mapiRegistry.isDefaultMailClient = false;

again, could be:
   mapiRegistry.isDefaultMailClient = parent.mapiPref.isDefaultMailClient;

Appart that, looks good. R=ducarroz

Comment 4

[Rajiv Dayal]

Attachment: updated patch with changes as per comments

changed pref-mailnewsOverlay.js as per comment # 3. Also changed
accountUtils.js for the same.

thanks for your review, JF.

Comment 5

[Scott MacGregor]

Comment on attachment 68912 [details] [diff] [review]
updated patch with changes as per comments

I couldn't tell who calls the method Startup that you added to pref-mailnews.js
.

That method looks like it contains only mapi related behavior anyway. Can't we
put it in the mapi prefs panel overlay and then when that overlay gets loaded
it calls Startup? That way, the base pref JS doesn't depend on MAPI and you
don't need to include all the special try/catch clauses checking for the
existence of a mapi function.

Comment 6

[Rajiv Dayal]

If i define the onload property for pref-mailnewsOverlay it does not seem to 
call the function refered by it. Can I have an onload property for an overlay ?
I will need to look more into XUL here.

Comment 7

[Scott MacGregor]

Sorry I wasn't clear about what I meant.

In your mapi overlay code, add a line of JS that looks like the following:

addEventListener("load", YourMapiOnLoadMethod, true);

now when the overlay gets loaded and it fires an on load event, your method will
get called. 

Comment 8

[Rajiv Dayal]

Attachment: new local load event handler for the mapi pref overlay, no need for Startup in base

Cool! I didnt know about this, thanks Scott for enlightening me.
Please find the update patch here.

Comment 9

[Scott MacGregor]

Comment on attachment 69081 [details] [diff] [review]
new local load event handler for the mapi pref overlay, no need for Startup in base

sr=mscott

Comment 10

[Rajiv Dayal]

Thank you Scott for the review.
Fix checked in the trunk.

Trix can you please verfiy all the bugs blocked by this bug. Most of them 
should be fixed by this fix. thanks, - Rajiv.

Comment 11

[Michael Wardle]

I just filed a few bugs on 0.9.8 that could be affected by this change.
- bug 127762
- bug 127764
- bug 127766
- bug 127767

I'll have to download 0.9.9 when it's out and see how these are affected.

Have the user-visible (GUI-configurable) prefs changed due to these changes?

Comment 12

[Matthias Versen [:Matti]]

*** Bug 127764 has been marked as a duplicate of this bug. ***

Comment 13

[Matthias Versen [:Matti]]

*** Bug 127767 has been marked as a duplicate of this bug. ***

Comment 14

[Trix Supremo]

verified on mozilla & netscape trunk builds 2002022703

Comment 15

[R.K.Aa.]

*** Bug 133905 has been marked as a duplicate of this bug. ***