Closed Bug 354895 Opened 14 years ago Closed 13 years ago

Add Vista Registry Support to Thunderbird's Shell Service

Categories

(Thunderbird :: General, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird2.0

People

(Reporter: mscott, Assigned: mscott)

References

Details

(Keywords: fixed1.8.1.3)

Attachments

(4 files, 7 obsolete files)

the windows shell service for Thunderbird needs to set the correct Vista keys.

This is the Firefox equivalent of Bug 352424
Blocks: 354896
Attached patch fix (untested on vista) (obsolete) — Splinter Review
This patch checks and sets for all types on vista (instead of testing news, mail and rss individually). It will probably change as I learn more about how to set specific app types.
Blocks: 354132
Attached patch updated work in progress (obsolete) — Splinter Review
Properly register as the default on vista for mail and news (separately). 

Much of this patch is going to change as we have to move the registry key changes into a separate process on Vista so it can run with the right priveleges.
Attachment #240681 - Attachment is obsolete: true
Attached patch updated fix (obsolete) — Splinter Review
This should look pretty similar to the current Firefox Vista implementation except we have an app key for mail (Mozilla Thunderbird) and another one for News: Mozilla Thunderbird (News).

I also fixed a problem with my original shell service implementation where I was putting quotes around %1 for -compose %1, -mail %1 for the various protocols. this makes us consistent with the keys we used to write out.
Attachment #241876 - Flags: review?(robert.bugzilla)
Comment on attachment 241876 [details] [diff] [review]
updated fix

Are you sure about not quoting the params send to the cmdline? They are typically quoted in all apps. The rest looks fine but I am going to hold off until I review the other patch
Also, the change to quoting will make everyone have to reset Thunderbird as the default if it is default.
(In reply to comment #5)
> Also, the change to quoting will make everyone have to reset Thunderbird as the
> default if it is default.
> 

Hmm actually, without that change, they get prompted about it not being the default because 2.0 and 1.0 write out -compose %1 and -mail %1 for the mailto and nntp keys respectively and the comparision routine in the shell service for the default app compares both the path and everything that comes after the path (i.e. the value of the entire key). Maybe we should change that in the shell service...
Comment on attachment 241876 [details] [diff] [review]
updated fix

I'll put back the quotes make the routine which compares keys when determining default status smart enough to ignore the arguments in the key value.
Attachment #241876 - Attachment is obsolete: true
Attachment #241876 - Flags: review?(robert.bugzilla) → review-
Ok, I think you might like this patch more. Quotes %1 for all protocols.

In the test for default comparison routine, only compare the actual paths involved instead of including the arguments. This makes us compatible with clients set as the default in the same file location using 1.0/1.5 which are not quoting the %1 It's also how the old mapi registry code was comparing keys for default status.
Attachment #241364 - Attachment is obsolete: true
Attachment #241887 - Flags: review?(robert.bugzilla)
Flags: blocking-thunderbird2+
Why is this a Normal bug?
Vista is out and new users are not able to use Thunderbird correctly.
IMHO, this needs to be an Major Severity bug.
Comment on attachment 241887 [details] [diff] [review]
updated patch using quotes around %1

clearing the review request, this patch is now obsolete.

I've been working on a new one with much help from Seth and Robert.
Attachment #241887 - Attachment is obsolete: true
Attachment #241887 - Flags: review?(robert.bugzilla)
Attached patch saving off a work in progress (obsolete) — Splinter Review
trunk only, uses long & quoted path names.
Attached patch updated fix (obsolete) — Splinter Review
I think we are getting somewhere. This patch appears to be fully functional in my limited testing on Win XP (still) and Vista. 

The feel url registraton in vista isn't hooked up yet, I haven't figured out what to do there but Mail and News are working ok when thunderbird is set as the default application through the shell service.

This patch is probably ready for review but I want to test it a bit more first.
Attachment #255015 - Attachment is obsolete: true
Comment on attachment 255389 [details] [diff] [review]
updated fix

This patch is for the trunk only. It uses long paths which are quoted when included with command line arguments. The code still accepts a short path when reading in the keys and when comparing for default app status so we don't prompt the user to become the default again.
Attachment #255389 - Flags: review?(robert.bugzilla)
Comment on attachment 255389 [details] [diff] [review]
updated fix

Rob and I did a code review on this earlier today and found some problems. New patch coming up.
Attachment #255389 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 255389 [details] [diff] [review]
updated fix

oops.
Attachment #255389 - Flags: review+ → review-
Attached patch updatd fix (obsolete) — Splinter Review
Rob, this patch fixes the following issues we identified earlier today:

1) CLS_EML was getting written into Software\Classes\.eml instead of ThunderbirdEML
2) Use a short path for the mapi32 dll
3) The default icon for ThunderbirdEML was getting the wrong icon.
4) Removed some obsolete keys.
5) Remove the dependency on the code getting the uninstall.exe path from the mapi dll path.
Attachment #255389 - Attachment is obsolete: true
Attachment #255865 - Flags: review?(robert.bugzilla)
Rob, here's the same patch but without the -w and it also uses a long path for the DLLPath key.
Attachment #255865 - Attachment is obsolete: true
Attachment #256104 - Flags: review?(robert.bugzilla)
Attachment #255865 - Flags: review?(robert.bugzilla)
Comment on attachment 256104 [details] [diff] [review]
patch without -w, using long path for DLLPath

>Index: shell/nsMailWinIntegration.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mail/components/shell/nsMailWinIntegration.cpp,v
>retrieving revision 1.3
>diff -u -r1.3 nsMailWinIntegration.cpp
>--- shell/nsMailWinIntegration.cpp	21 Nov 2006 01:16:57 -0000	1.3
>+++ shell/nsMailWinIntegration.cpp	22 Feb 2007 23:52:51 -0000
>...
>@@ -83,26 +84,41 @@
>...
>+// Sets the default mail registry keys for Windows versions prior to Vista.
>+// Try to open / create the key in HKLM and if that fails try to do the same
>+// in HKCU. Though this is not strictly the behavior I would expect it is the
>+// same behavior that Firefox and IE has when setting the default browser previous to Vista.
Hey Scott, have you checked what the behavior of Outlook and Outlook Express is? The behavior of IE is very aggressive and I would hate Thunderbird to duplicate the IE behavior if other mail apps aren't as aggressive.
Just checked Outlook 2003 and it is just as aggressive... I suspect OE is as well but it would be good to confirm.
Comment on attachment 256104 [details] [diff] [review]
patch without -w, using long path for DLLPath

Thanks Scott!
Attachment #256104 - Flags: review?(robert.bugzilla) → review+
Ok this is fixed on the trunk. I'll start porting it to the branch. Thanks for the review rob.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Use short paths in porting the shell service changes to the branch.
I think this is easier to look at than the entire diff. This shows just the changes between the trunk and the branch.
After talking to Rob, we're going to go with long paths on the branch. This made the branch merge much easier as the trunk changes applied cleanly here on the branch.

Rob, did you want to put another r stamp on this patch? It's identical to what you already looked at + a couple of white space changes I checked into the trunk after the initial trunk landing.
Attachment #256951 - Flags: review?(robert.bugzilla)
These patches have now landed on the branch and so far are working well using a tinderbox build for me. The changes will be available to branch testers on the nightly build channel tomorrow!

Thanks again Rob.
Keywords: fixed1.8.1.3
It doesn't work as it should. Now Thunderbird sees itself as default on Vista, even if it isn't.
(In reply to comment #26)
> It doesn't work as it should. Now Thunderbird sees itself as default on Vista,
> even if it isn't.
> 

I'm not able to reproduce that using a fresh vista vm install. Can you give more specific steps as to how you got this to happen? I'm able to set and unset it as the default through Set Default Programs.
(In reply to comment #27)
> Can you give more specific steps as to how you got this to happen?

Sure:
1. Open Windows Mail Client
2. When it asks you to be the default program, click Yes
3. Close Windows Mail
4. Open Default Programs to check, if Thunderbird isn't the default program for mail (it should read "This program has 0 of 3 default settings")
5. Open Thunderbird and check if it asks for default program (in my case it doesn't)
Piotr, do you have the option to always check to see if Thunderbird is the default mail client at startup turned on? (Tools / Options / General). I'm still struggling to reproduce the problems you are having. Thanks!
Yes, I do have this option turned on. What's weird, it seems that turning it off, restarting the program and turning it back on resolved the problem. Sorry for the trouble, now everything seems to work as it should.
Attachment #256951 - Flags: review?(robert.bugzilla)
You need to log in before you can comment on or make changes to this bug.