Closed Bug 353906 Opened 18 years ago Closed 18 years ago

Modernize mail's registry code to implement toolkit's nsIShellService

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird2.0

People

(Reporter: mscott, Assigned: mscott)

References

Details

(Keywords: fixed1.8.1)

Attachments

(7 files, 2 obsolete files)

In order to easier support the new vista registry code, we need to modernize the mail registry code which is currently lumped together with the mapi code even though they are two different things.

This bug will track the re-writing our default app registration code as a shell service in mail\components\shell.
Flags: blocking-thunderbird2+
Attached patch work in progress (obsolete) — Splinter Review
saving off some of my work...
> +pref("mailnews.display.headers", "subject,from,sender,relplyto,date,to,cc,bcc,newsgroups,followup-to,user-agent,content-base");

"relplyto"?
Attached patch another work in progress update (obsolete) — Splinter Review
add back end support for registering feed and news urls. Still need to add new pref UI for making tb the default client for news and feeds. The mac and gnome integration classes need updated too now.
Here's what the new default client dialog looks like.
Attachment #239748 - Attachment is obsolete: true
Attachment #240165 - Attachment is obsolete: true
Attached patch a patchSplinter Review
Ok, I think this latest patch is ready for code review. I've tested it on Mac and Windows. Still need to find a linux box for the gnome integration changes.

Here's what's going on:

1) We have a new default client dialog (see the screen shot). This dialog only appears when opening a 3-pane window, with at least one account, where the user has asked us to check at startup and we aren't the default for news, mail and rss. By default, we pre-select Mail in the default client dialog list and leave News and RSS unchecked. Items we are already registered as the default for are greyed out.

2) We no longer group MAPI support and default app registry settings together. They are too different things. nsIMapiRegistry.idl is no longer used. Our registry shell APIs are now in mail\components\shell\nsIShellService. nsMapiRegistryUtils and nsMapiRegistry are now obsolete. I added comments to the top of all of the obsolete mapi files to hopefully avoid confusion as they are still part of the seamonkey build.

3) The Options UI now looks like Firefox. A simple checkbox for checking at startup followed by button which launches the default client dialog if we have app types we aren't the default for. Otherwise the button launches an alert saying Thunderbird is already the default.

4) We've dropped support for restoring backup registry keys (like Fx) as this is incorrect Windows behavior and there are no promises that the app you are restoring too even exists.

5) Instead of calling ::SendMessageTimeout after changing the default mail or news application (which can 5-7 seconds), we now call SHChangeNotify (Bug 354467).

6) In order to make it easy for the windows shell service implementation to register the MAPI server, I've added a registerServer method to nsIMapiSupport which is our MAPI service class. This interface method just calls into an existing internal mapi server registration class.

7) I added a command line handler for setting Thunderbird as the default mail app via the command line like Firefox. We no longer need to build the old files for setting and unsetting from the command line from xpfe\components\winhooks.
Attachment #240565 - Flags: superreview?(bienvenu)
Comment on attachment 240565 [details] [diff] [review]
a patch

+  DWORD len = sizeof buf;

hmm, I'd not seen that usage before...


Wow, that's a lot of work!
Attachment #240565 - Flags: superreview?(bienvenu) → superreview+
Blocks: 354895
Blocks: 354896
Scott, I think you forgot to add mail/components/shell/public/Makefile to MAKEFILES_thunderbird in allmakefiles.sh - it causes make chrome to be broken in at least one case I've seen.
So I've been playing around with this.  I currently don't seem to be getting any MAPI support -- I can't 'send to > mail recipient', Word 2000's Send To menu doesn't even *have* mail recipient listed.  The Set Program Access and Defaults dialog (Control Panel / Add/Remove Programs) doesn't list Thunderbird.
The registration for mailto and .eml files appears to be working just fine.

There's been a long-standing hack to get around this problem with earlier versions by selecting another program (e.g. Outlook Express) as the default mail client, then telling TB to set itself.  I tried this, and still have no MAPI support, so this hack is now broken.  Is there any way for the user to set TB as the MAPI handler?  Did the patch's Vista-focus break this for downrev Windows?

*Before* I tried the OE tweak, I ran TB 3a1-1004 in a new profile; it came up with the Check Default dialog, with E-Mail and Feeds checked and disabled and Newsgroups unchecked.  I clicked OK, exited, restarted: got the same dialog.

Then I manually changed HKLM\Software\Clients\Mail, restarted: same thing.  (This used to be the thing you'd change to get TB to prompt to be reset as the default client.)  Then I changed HKCU\Software\Clients\Mail and restarted: again, same dialog.

But after setting OE as the default, then restarting, the E-Mail checkbox was enabled.  Still, no change in MAPI behavior after resetting TB as the default.

Note that all testing here was under Windows 2000, running as Administrator.


One more thing: if TB believes it's the default client for all of the functions which the user has specified -- that is, all checkboxes are either unchecked or checked-and-disabled -- then why show the dialog at all?  Previous versions only put up a "make me default" prompt if TB had lost its expected default-client status.
(In reply to comment #10)
> So I've been playing around with this.  I currently don't seem to be getting
> any MAPI support -- I can't 'send to > mail recipient', Word 2000's Send To
> menu doesn't even *have* mail recipient listed.  The Set Program Access and
> Defaults dialog (Control Panel / Add/Remove Programs) doesn't list Thunderbird.

I'm using open office and Send / Document as E-Mail is properly invoking MAPI. I'm not sure why Word 2000 would be different. Set Program Access and Defaults also lists Thunderbird on my Win XP machine. 

> Did the patch's Vista-focus break this for downrev
> Windows?

Since this was developed on Win XP, i didn't have vista at the time the answer to your question about Vista focus is no. 

> Previous versions
> only put up a "make me default" prompt if TB had lost its expected
> default-client status.

We should only be showing the prompt if the boolean is true AND you aren't the default already for news, rss and feed:

http://lxr.mozilla.org/mozilla/source/mail/base/content/msgMail3PaneWindow.js#875

That's working for me but not for you it seems. We should try to figure out why.
(In reply to comment #11)
> (In reply to comment #10)
> > I currently don't seem to be getting any MAPI support [...]
> > The Set Program Access and Defaults dialog [...] doesn't list Thunderbird.
> 
> I'm using open office and Send / Document as E-Mail is properly invoking
> MAPI.  I'm not sure why Word 2000 would be different. Set Program Access and
> Defaults also lists Thunderbird on my Win XP machine. 

Did you test this on a fresh Windows installation?


> > Previous versions
> > only put up a "make me default" prompt if TB had lost its expected
> > default-client status.
> 
> We should only be showing the prompt if the boolean is true AND you aren't the
> default already for news, rss and feed:
> 
> That's working for me but not for you it seems. We should try to figure out
> why.

If I'm reading that right, it appears that if I want TB to check that it's the default for *any* client type, the prompt will show unless it's the default for *all* client types.  This means people who want to TB to be their default mail client, but not their default news client, and want the check to be sure that some other program hasn't made itself the default mail client, will always get the prompt.
I went back and ran 2b1-1004 as Admin; it asked if it should be made the default mail program and I said yes.  Immediately, using Send To > Mail Recipient from Explorer started to work.  I closed 2b1, ran 3a1 as non-Admin.
(The profile had already been flagged to Not Show Message Again about setting as default.)  Send To > Mail Recipient from Explorer opened a compose window under 3a1 (as expected).  However, Word 2000 still didn't show a Mail Recipient option in the Send To submenu; I'm not sure what's wrong there.  (The submenu actually doesn't look right at all: two items, one disabled, both drawn as if hovered-over.)

Closed 3a1, ran it again as Administrator.  It asked whether to be the default mail program, I said yes.  Immediately, Send To > Mail Recipient stopped working.


Also: running as Admin and telling to be the default for all three client types, then closing and restarting, does not present me with the prompt.  The logic on that needs to be fixed so that the prompt only shows when those 
client-types that the user has specified as defaults lose their default status.
See also bug 356063.
(In reply to comment #12)
> 
> Did you test this on a fresh Windows installation?

Mike, what steps are you doing to simulate a fresh install? I'm deleting Software\Clients\Mail\Mozilla Thunderbird and Software\Clients\News\Mozilla Thunderbird...
(In reply to comment #15)
> (In reply to comment #12)
> > 
> > Did you test this on a fresh Windows installation?
> 
> Mike, what steps are you doing to simulate a fresh install? I'm deleting
> Software\Clients\Mail\Mozilla Thunderbird and Software\Clients\News\Mozilla
> Thunderbird... 

I didn't simulate a fresh install, I actually had one.  However, I'm certain I had set TB as the default client in that install before the patch for this bug was checked in, so perhaps that's not a fruitful line of inquiry, particularly given the 2b1/3a1 intereaction described in comment 13.
Depends on: 356237
(In reply to comment #15)
> (In reply to comment #12)
> > 
> Mike, what steps are you doing to simulate a fresh install? I'm deleting
> Software\Clients\Mail\Mozilla Thunderbird and Software\Clients\News\Mozilla
> Thunderbird...
> 
I deleted all occurencies of Thunderbird from my registry. When I install a unbranded selfmad trunk build I see under "Software\Clients\Mail/News Client" the key value for DLLPath to be set to C:\PROGRA~1\MOZILL~3\THUNDE~1.EXE\mozMapi32.dll
The same I get wiht the current nightly (except that its under Software\Clients\Mozilla Thunderbird).
sendto does nothing unless I remove the THUNDER~1.EXE\ from the key.
From my 2.0a1 release install the DLLPath key is set to C:\PROGRA~1\MOZILL~2\mozMapi32.dll

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #12)
> > > 
> > Mike, what steps are you doing to simulate a fresh install? I'm deleting
> > Software\Clients\Mail\Mozilla Thunderbird and Software\Clients\News\Mozilla
> > Thunderbird...
> > 
> I deleted all occurencies of Thunderbird from my registry. When I install a
> unbranded selfmad trunk build I see under "Software\Clients\Mail/News Client"
> the key value for DLLPath to be set to
> C:\PROGRA~1\MOZILL~3\THUNDE~1.EXE\mozMapi32.dll
> The same I get wiht the current nightly (except that its under
> Software\Clients\Mozilla Thunderbird).
> sendto does nothing unless I remove the THUNDER~1.EXE\ from the key.
> From my 2.0a1 release install the DLLPath key is set to
> C:\PROGRA~1\MOZILL~2\mozMapi32.dll
> 

That would be a problem! Excellent catch. Patch coming up.
When setting thunderbird as the default app from the options dialog (the installer works), we were including the process name "thunderbird.exe" in the path to mozmapi32.dll for the DLLPath key.

We should strip off the process name and append mozMapi32.dll when setting this string in the registry from our shell service.
Attachment #242524 - Flags: superreview?(bienvenu)
Attachment #242524 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 242524 [details] [diff] [review]
[fixed on the trunk] fix mapi DLL key value

This patch is now on the trunk. Mike/Walter, can you guys help test again with tomorrow's build? Thanks for the great detective work.
Attachment #242524 - Attachment description: fix mapi DLL key value → [fixed on the trunk] fix mapi DLL key value
When I read comment 17, I immediately checked my registry and found that the path was correct (pointing to my 2b1 installation).  And then when I checked the operation, I found that MAPI was working.  Since I wasn't keeping close watch after my testing in comment 10, I don't know what I did differently, and I never thought to check the path when I was having the problem.
(In reply to comment #21)
> When I read comment 17, I immediately checked my registry and found that the
> path was correct (pointing to my 2b1 installation).  And then when I checked
> the operation, I found that MAPI was working.  Since I wasn't keeping close
> watch after my testing in comment 10, I don't know what I did differently, and
> I never thought to check the path when I was having the problem.
> 

any chance you used the installer at some point? That would have written out the correct value for the key. 
(In reply to comment #21)
> When I read comment 17, I immediately checked my registry and found that the
> path was correct (pointing to my 2b1 installation).  And then when I checked
> the operation, I found that MAPI was working.  Since I wasn't keeping close
> watch after my testing in comment 10, I don't know what I did differently, and
> I never thought to check the path when I was having the problem.
> 

I had again experimented (this afternoon before the checkin) a little bit with a selfmade build, not branded, I installed in programme\Mozilla Thunderbird30. In parallel I have 2.0a1 in Mozilla Thunderbird. Now, using the installer for the trunk build I found that it not only made a new key for Mail/News client (not branded) but overwrote the settings for official TB 2.0a1, Mozilla Thunderbird key with all its settings pointing to the new location. However, while the mapi32.dll path in the mail/news client had the wrong entry as seen before, the overwritten key from my parallel Mozilla Thunderbird was just altered by setting the new locations, i. e. it pointed to C:\PROGRA~1\MOZILL~2\mozMapi32.dll. Mapi then worked with trunk. Then I changed back the default mail client to the official build, the registry changed again back to TB2.0a1 settings for Thunderbird, not changing anything in mail/news client. Switching back to trunk build after installation did not alter any more the settings of TB2.0a1. So it seems to be depended on an already existing installation / configuration what in the end was changed.
Anyhow, I'm just building with the new patch and will come back
(In reply to comment #22)
> any chance you used the installer at some point? That would have written out
> the correct value for the key. 

I haven't used a TB installer since filing bug 289348; ZIP'd nightlies are much easier to deal with.
As far as I can see all keys seem to be written properly now either with installer or with zip, after setting tb the default mail client. sendto is not working here though, it works with tb2.0a1 but not with the latest nightly.
Yeah, in comment #23 I described the same as Mike did in bug289348
(In reply to comment #25)
> As far as I can see all keys seem to be written properly now either with
> installer or with zip, after setting tb the default mail client. sendto is not
> working here though, it works with tb2.0a1 but not with the latest nightly.
> Yeah, in comment #23 I described the same as Mike did in bug289348
> 

On my current setup, send to / Mail Recippients on a file on my desktop was not working with a trunk build as my default. I then deleted all the keys under Clients\Mail\Mozilla Thunderbird.

I ran with a trunk installer which put the keys back, and send to from a file on my desktop worked. I then removed all the keys again, restarted thunderbird and this time used the shell service to set it as the default app. This caused the keys to get written back out again. Send To Still worked.

I wonder if a bad key got written out on the trunk while iterating over several of these patches?

I don't think it's related but the installer writes out 8.3 for the actual file name, mozMapi32.dll in the DllPath key where the shell service writes out the full file name. Both use short paths to the DLL.
(In reply to comment #26)
After deleting every key (not just the client keys) or value referring to thunderbird in the registry, it works now with both the zip and the installer latest trunk (2006-10-18-06-trunk). Sorry, I obviously played around a little bit too much in the registry. But as tb2.0a1 and oe didn't show any similar problems, I came at first to a wrong conclusion.
If someone has time, it would be good to also get some extra testing using Thunderbird 2 to install the default set of keys. Then upgrading to thunderbird 3 (using a zip build and maybe again using an installer). Then using the shell service (the options UI) to set as the default again in thunderbird 3. Making sure that Send To works after each of these steps.
Currently we show the default app dialog at startup when you have the option checked and we aren't the default client for Mail or News or RSS. I'm starting to think we should only show the dialog if we aren't the default mail client...The dialog would still give the options for configuring news and RSS. Thoughts? 
(In reply to comment #29)
> Currently we show the default app dialog at startup when you have the option
> checked and we aren't the default client for Mail or News or RSS. I'm
> starting to think we should only show the dialog if we aren't the default
> mail client...The dialog would still give the options for configuring news
> and RSS.  Thoughts? 

This code, from the LXR URL you provided at comment 11, confuses me:

  if (accountManager.defaultAccount && shellService.shouldCheckDefaultClient 
        && !shellService.isDefaultClient(true, 
           nsIShellService.MAIL | nsIShellService.NEWS | nsIShellService.RSS))
     window.openDialog("chrome://messenger/content/defaultClientDialog.xul",
                       "DefaultClient",
                       "modal,centerscreen,chrome,resizable=no");

What is 'shouldCheckDefaultClient'?  Who sets it?  Most importantly, *which* client "should" be checked if it's true?  The logic for this decision, it seems to me, should be (pseudocode here)

   if ((shouldBeMailDefault && !isMailDefault) ||
       (shouldBeNewsDefault && !isNewsDefault) ||
       (shouldBeRSSDefault  && !isRSSDefault))

Do you have the capability to test the 'should' for each individual client?
(In reply to comment #30)
> 
> This code, from the LXR URL you provided at comment 11, confuses me:
> 
>   if (accountManager.defaultAccount && shellService.shouldCheckDefaultClient 
>         && !shellService.isDefaultClient(true, 
>            nsIShellService.MAIL | nsIShellService.NEWS | nsIShellService.RSS))
>      window.openDialog("chrome://messenger/content/defaultClientDialog.xul",
>                        "DefaultClient",
>                        "modal,centerscreen,chrome,resizable=no");
> 
> What is 'shouldCheckDefaultClient'?  Who sets it?  

shouldCheckDefaultClient behaves as follows
1) FALSE If we have already shown the user the default client dialog this session (think opening multiple 3-panes)
2) FALSE If the user has turned off the ability to check for default client status at startup ("mail.shell.checkDefaultClient"). 

in all other cases it returns true. 

> Most importantly, *which*
> client "should" be checked if it's true?  The logic for this decision, it seems
> to me, should be (pseudocode here)
> 
>    if ((shouldBeMailDefault && !isMailDefault) ||
>        (shouldBeNewsDefault && !isNewsDefault) ||
>        (shouldBeRSSDefault  && !isRSSDefault))
> 
> Do you have the capability to test the 'should' for each individual client?

We can't test if we should be the default. Only if we are (or aren't) the default. 

So the question is, do we nag if you choose not to use Thunderbird for RSS or news? I'm thinking Mail is the one we should always nag on, leaving the others alone. Currently we nag if any of the three are not the default and you ask us to check at startup. 
(In reply to comment #31)
> (In reply to comment #30)
> > Most importantly, *which*
> > client "should" be checked if it's true?  The logic for this decision, it
> > seems to me, should be (pseudocode here)
> > 
> >    if ((shouldBeMailDefault && !isMailDefault) ||
> >        (shouldBeNewsDefault && !isNewsDefault) ||
> >        (shouldBeRSSDefault  && !isRSSDefault))
> > 
> > Do you have the capability to test the 'should' for each individual client?
> 
> We can't test if we should be the default. Only if we are (or aren't) the
> default. 

I meant the 'should' setting to correspond to the user's setting of the checkboxes in the dialog.  Do those not persist?


> So the question is, do we nag if you choose not to use Thunderbird for RSS or
> news? I'm thinking Mail is the one we should always nag on, leaving the others
> alone. Currently we nag if any of the three are not the default and you ask us
> to check at startup. 

Nag for RSS or News *if* user has specified RSS or News defaultitude.
(In reply to comment #28)
> If someone has time, it would be good to also get some extra testing using
> Thunderbird 2 to install the default set of keys. Then upgrading to thunderbird
> 3 (using a zip build and maybe again using an installer). Then using the shell
> service (the options UI) to set as the default again in thunderbird 3. Making
> sure that Send To works after each of these steps.
> 
Sent to works here with all variations I tested (setting or not oe inbetween as the default mail handler), works with zip and installer (instantly). I experienced that you can use the trunk installer to install directly over 2.0a1. If you do it the other way round, tb2.0a1 fails to start.
When you use the zip it sets all keys except in protokol the URL:MailTo Protocol value remains unset (also in the news section). Those are set by the installer however.
The trunk installer leaves the HKCU\Software\Clients\Mail alone in contrast to the 2.0 installer. Personally I appreciate this, cause changing the default mail client then does not depend on the email settings of the start menu. 2.0 in this case is a little bit impertinent.

Comment on attachment 240565 [details] [diff] [review]
a patch

going to land this on the branch now that we've shaken it out on the trunk for the last couple of weeks.
Attachment #240565 - Flags: approval-thunderbird2+
Attachment #240770 - Flags: approval-thunderbird2+
Attachment #240992 - Flags: superreview+
Attachment #242524 - Flags: approval-thunderbird2+
Keywords: fixed1.8.1
for some reason the include file didn't get picked up when I merged to the branch...fix is checked in. Thanks for pointing this out Kairo.
Should this have a late-l10n keyword?
(In reply to comment #37)
> Should this have a late-l10n keyword?
> 

I'm not sure why it should. We aren't close to a Thunderbird string freeze yet, but am happy to hear thoughts to the contrary. 
c:/tinderbox/SeaMonkey-1.8/WINNT_5.1_Depend/mozilla/mailnews/mapi/mapihook/src/nsMapiRegistryUtils.cpp(1202) : error C2065: 'rv' : nichtdeklarierter Bezeichner

nsCOMPtr<nsIMapiSupport> mapiService (do_GetService(NS_IMAPISUPPORT_CONTRACTID, &rv));

|rv| should be |result|.  I just committed that.  Hopefully that will make it go green.
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061024 SeaMonkey/1.1b] (nightly) (W98SE)

When starting MailNews,

[
81 scott      1.42.22.1 #ifndef MOZ_THUNDERBIRD
]
regressed SeaMonkey:
[
Error: illegal character
Source File: chrome://messenger/content/accountUtils.js
Line: 81
Source Code:
#ifndef MOZ_THUNDERBIRD

Error: verifyAccounts is not defined
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 694
]

which gives a mostly empty window.
(In reply to comment #40)
> [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061024
> SeaMonkey/1.1b] (nightly) (W98SE)
> 
> When starting MailNews,
> 
> [
> 81 scott      1.42.22.1 #ifndef MOZ_THUNDERBIRD
> ]
> regressed SeaMonkey:
> [
> Error: illegal character
> Source File: chrome://messenger/content/accountUtils.js
> Line: 81
> Source Code:
> #ifndef MOZ_THUNDERBIRD
> 
> Error: verifyAccounts is not defined
> Source File: chrome://messenger/content/msgMail3PaneWindow.js
> Line: 694
> ]
> 
> which gives a mostly empty window.
> 

This was one sucky merge. Sorry guys. I didn't check in the change to mailnews\jar.mn. I just did that now.
The #ifdef wasn't even necessary, the component detection should work fine.
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20061024 SeaMonkey/1.1b] (tinderbox-builds, 2006102422) (W98SE)

Comment 40 regression V.Fixed on MOZILLA_1_8_BRANCH.
Attachment #243910 - Flags: superreview?(mscott)
Comment on attachment 243910 [details] [diff] [review]
unfork accountUtils.js

ah very nice Neil!
Attachment #243910 - Flags: superreview?(mscott)
Attachment #243910 - Flags: superreview+
Attachment #243910 - Flags: approval-thunderbird2+
No idea if it's just my fault for not providing it a good place to run (though it's a fairly stock Ubuntu install), but my Linux builds are throwing

Error: uncaught exception: [Exception... "ServiceManager::GetService returned failure code:"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: delayedOnLoadMessenger :: line 828"  data: no]

from this, which then stops the rest of delayedOnLoadMessenger from running, which doesn't make for a very pretty result (for example, it took me forever to figure out why MailToolboxCustomizeDone() wasn't running, or even being set on the toolbox).
Phil, see Bug 358339
Patch from comment #5 introduced SeaMonkey regression, see bug 361233.
Not sure why this isn't marked as fixed, it's been in the trunk and the branch for a while.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: