103908 - The default email selection dialog box continues to display even when the "Do not display..." is selected.

Status: VERIFIED FIXED

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

Description

[Trix Supremo]

In the default email app selection dialog box, if you check the checkbox "Do not
display this dialog again" and select the NO button, you are still prompted with
the same dialog box again when you relaunch the email client.

DIRECTIONS:
1. Make sure Outlook Express is the default email client.
2. Launch the browser and then the email client.
3. In the default email app selection dialog, check the checkbox "Do not display
this dialog again" and click the NO button.
4. Close the email client and the browser and then relaunch both.

ACTUAL:
The email selection dialog box reappears.

EXPECTED:
Selecting the option should have disabled the dialog box from reappearing.

Comment 1

[Scott MacGregor]

I've been seeing this behavior too. Nominating for the branch. currently I get
prompted every time I start up the app for the dialog. This was using a build
from yesterday though so maybe this is working today.

I particularly notice it after running a mozilla build where it asks me. Then go
back to a netscape build and it asks me again. 

Comment 2

[Jaime Rodriguez, Jr.]

Is this required for 094? If so, we should mark it nsbranch+

Comment 3

[Scott MacGregor]

I think the trick to seeing it is if I select No and I check the box for don't
ask me again. Then I see the dialog. If others also see this behavior then it
sounds like a good plus candidate to me since I shouldn't get asked every time I
start up my profile.

Comment 4

[Srilatha Moturi]

Marking this nsbranch+

Comment 5

[Srilatha Moturi]

Attachment: patch v1

Comment 6

[Srilatha Moturi]

ccing ssu and dveditz for reviews.

Comment 7

[Srilatha Moturi]

Attachment: patch with more context

Comment 8

[Srilatha Moturi]

Attachment: now checking mapi32.dll before we say we are the default mail client

Comment 9

[Srilatha Moturi]

Sean, the above patch has a huge context. let me know if you want one with 
shorter context

Comment 10

[Sean Su]

Comment on attachment 52924 [details] [diff] [review]
now checking mapi32.dll before we say we are the default mail client

r=ssu

Comment 11

[Jaime Rodriguez, Jr.]

tentative PDT+, pending testing and sr from dan.

Comment 12

[Tiantian Kong]

Dan: 

Hopefully you are back home by now after our phone conversation at around 9pm.

Please could you work with Srilatha, and try to get the patch in tonight?

Please repost your comments as soon as possible. Thanks.

Comment 13

[Daniel Veditz [:dveditz]]

Re-typed my review comments, and lost them again. Argh!

Comment 14

[Srilatha Moturi]

Attachment: Using GetSystemDirectory()

Comment 15

[Srilatha Moturi]

Attachment: preending the file name with \\

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 52924 [details] [diff] [review]
now checking mapi32.dll before we say we are the default mail client

> PRBool IsDefaultMailClient()
> {
>+    if (!isSmartDll() && !isMozDll()) return PR_FALSE;
>     nsCAutoString name(GetRegistryKey(HKEY_LOCAL_MACHINE, 
>                                       "Software\\Clients\\Mail", ""));

Put the return on its own line. Putting if's in a single line like this
obscures control flow making it harder to read, and makes it more
difficult to debug since debuggers are typically line-oriented.

An extra blank line after the early bailout wouldn't hurt, either.


>+static PRBool isMozDll() 
>+{ 
>+    nsAutoString mapiFilePath; 
>+    nsSpecialSystemDirectory sysDir(nsSpecialSystemDirectory::Win_SystemDirectory); 
>+    ((nsFileSpec*)&sysDir)->GetNativePathString(mapiFilePath); 
>+    mapiFilePath.AppendWithConversion("Mapi32.dll"); 

Mentioned in AIM, but "WithConversion" methods are a red flag. First they are
deprecated and you should use explicit conversions, in this case
NS_LITERAL_STRING("Mapi32.dll") which will have a performance benefit
too -- on some platforms it compiles away.

>+    HINSTANCE hInst; 
>+    GetMapiDllVersion *doesExist = nsnull;
>+    nsCAutoString filePath;
>+    filePath.AssignWithConversion(mapiFilePath.get()); 
>+    hInst = LoadLibrary(filePath.get()); 

Converting paths is especially bad, breaks i18n. nsSpecialSystemDirectory
is deprecated and so is nsFileSpec -- don't ever use them. In XP code use
the directory service (NS_OS_SYSTEM_DIR here) and nsIFile. And in nsIFile
you can get the cstring native path if you need it, and conversions would
be avoided here.

But this is win-specific code, and you go ahead and call the WinAPI 
LoadLibrary(), so just use GetSystemDirectory() and dispense with the
XP overhead.

This is the one substantive thing I'm objecting to, and you should fix
similar places that are already checked-in.

>+    doesExist = (GetMapiDllVersion *) GetProcAddress (hInst, "GetMapiDllVersion"); 
>+    FreeLibrary(hInst); 

Before this lands on the trunk let's change the name of this function.
GetMapiDllVersion is too generic to be a reliable indicator that this
belongs to us. Take the "smartDLL" GetOutlookVersion as an example and
do something like GetMozillaVersion, or open up the file version resource
and look for the product, creator, original name, or some such.

I imagine changing this now would impact a lot of places so I can live
with it. Changing it later is worse, though, because future programs may
have to look two places.

> PRBool getShowDialog() {
>     PRBool rv = PR_FALSE;
>     nsCString showDialog(GetRegistryKey(HKEY_LOCAL_MACHINE, 
>                                         "Software\\Mozilla\\Desktop", 
>                                         "showMapiDialog"));

Why is this a global registry setting? The "show dialog" setting should be
a per-profile pref. I wouldn't want my wife to be bothered by this dialog,
but I'd want to see if some other app changed my settings.


>     if (!showDialog.IsEmpty()) {
>         if (showDialog.Equals(NS_LITERAL_CSTRING("1")))
>             rv = PR_TRUE;
>     }
>     else
>         rv = PR_TRUE;
>     if (!rv) {

This logic confuses me (and do you need the NS_LITERAL_CSTRING there?).
maybe:

     // show dialog unless user suppressed it
     if (showDialog.IsEmpty() || showDialog.Equals("1"))
        rv = PR_TRUE;
     else {
         // even if suppressed, show dialog if someone else hijacked us
         ... rest of stuff....
     }

>     return rv;


> // This function returns the (fully-qualified) name of this executable.
> static nsCString thisApplication();
> 
> // returns the brandName of the current application
> static nsCString brandName();
> 
> // returns TRUE if the Mapi32.dll is smart dll.
> static PRBool isSmartDll();
> 
>+// returns TRUE if the Mapi32.dll is a Mozilla dll.
>+static PRBool isMozDll();
>+

prototypes for static functions are questionable in a header, but
downright wrong in an exported header. Move these into the C++ file
that contains these functions.

Comment 17

[Tiantian Kong]

Tiantian,
I have checked in all the fixes. Somehow bugzilla is down so Sean, Dan
and I could not update the bug. But I got r= from Sean and sr=from dan
over AIM.

Srilatha

Comment 18

[Srilatha Moturi]

Attachment: the patch that is checked into the tree

Comment 19

[Srilatha Moturi]

The last patch attachment 53112 [details] [diff] [review] is the one I got r and sr from Sean and Dan.
This is checked into 0.9.4 branch

Comment 20

[Tiantian Kong]

Fixed in 0.9.4 branch

Comment 21

[Trix Supremo]

verified on 2001-10-15-05-0.9.4