378723 - Palm Sync extension built in SeaMonkey says installing into Thunderbird AB

Status: RESOLVED FIXED

(MailNews Core Graveyard :: Palm Sync, defect)

Description

[Mark Banner (:standard8)]

The conduit installer installs the palm conduit into the registry and does the necessary that Thunderbird can't do. It gets run on first installation of the palm sync extension into a profile, or when the user double-clicks it in the folder.

Now suiterunner is capable of building Palmsync as a new-style toolkit extension, I've noticed that on startup the conduit installer says:

"You are about to add the Thunderbird Address Book conduit to your Palm Hot Sync installation, which may remove the Palm Desktop conduit.  Do you want to continue?"

Hence the "Thunderbird" isn't quite right.

I've traced the problem to its makefile, where there's an ifdef that I guess is mistakenly commented out (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpinstall/wizard/windows/palmsync/Makefile.in&rev=1.5&mark=72-74#72)

However, rather than fix that ifdef I think it would be really nice if we could find an alternative method whereby the same executable could be used for SeaMonkey and Thunderbird - this would problably mean we could then use the same palmsync xpi for equivalent versions of SeaMonkey and Thunderbird.

What I'd like to propose is:

- Add a command line switch to the installer which takes the brand short name from the extension.
- If the command line switch is specified, use that value in the messages.
- If the command line switch is not specified, default it to empty and therefore the message would be "You are about to add the Address Book conduit ...", I'm thinking that if the user runs it themselves, then they'd still be able to run it, and hopefully know which one they are adding it into...

Thoughts, comments? If not I'll come up with a patch in the next few days.

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch v1

This patch makes the installer executable take a command line switch (/n) to pass the name of the calling app. If its blank it just leaves the name blank and continues to run (hence double click on it will work) - the extension can therefore pass the app name (obtained via the nsIXULAppInfo).

I've also incorporated the potential change for installing using a short path name *if possible*. The documentation says its not always possible, so in that case we'll fall back to the long name. So this should help the situation in some cases (hopefully most).

Not requesting reviews yet as I'm still compiling to test the extension part and I'm hoping Wayne can test the short name installation for me (I sent him the exe which is quicker to build ;-) ).

Comment 2

[Mark Banner (:standard8)]

Attachment: Patch v1 (diff -w)

Comment 3

[Mark Banner (:standard8)]

Attachment: Patch v2 (checked in)

This patch builds on VC express. I've added an .rc change for some builds because it fails to build the conduit dll otherwise. I also corrected some mistakes in the v1 patch.

I've tested this with SeaMonkey and it appears to work fine.

diff -w coming up.

Comment 4

[Mark Banner (:standard8)]

Attachment: Patch v2 (diff -w)

Comment 5

[David :Bienvenu]

Comment on attachment 263024 [details] [diff] [review]
Patch v2 (checked in)

looks good, thx, Mark!

Comment 6

[Wayne Mery (:wsmwk)]

From the executable Mark sent me .. since the code does not call or rely on anything in the apps, I tested both SM and TB.  

with respect to this bug, is branding really worth the time and code maintanance?  It does not enhance functionality, and so many areas are lacking, so my first inclination when I saw mention of it is why not simply omit it?

Related is item 2 from bug 322628 comment 2 "We need to find a way to call PalmSyncInstall.exe /u when the user trys to disable or uninstall the extension." 

"disable" function in addons should also call palmsyncinstall /u

The short path name portion of the patch works great (bug 322628)

I encountered one issue.  registration fails 50% of the time with
  Could not find the address book palm sync conduit to install

I was running it from the command line

Comment 7

[Wayne Mery (:wsmwk)]

also ~50% of time on uninstall I get 
  Error in (un)installing the Address Book Palm Sync Conduit

even though the conduit was in fact correctly removed from HSM.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #6)
> with respect to this bug, is branding really worth the time and code
> maintanance?  It does not enhance functionality, and so many areas are lacking,
> so my first inclination when I saw mention of it is why not simply omit it?

It didn't take that long. The main thing it does mean is that when I change the path it builds into and hence the zip builds get updated for suiterunner so that palmsync will work straight away.. then I won't get loads of complaints it says Thunderbird and not SeaMonkey.

> Related is item 2 from bug 322628 comment 2 "We need to find a way to call
> PalmSyncInstall.exe /u when the user trys to disable or uninstall the
> extension." 
We can continue that in bug 322628 now, I only included the short path name change in this patch as it was convenient. 

> I encountered one issue.  registration fails 50% of the time with
>   Could not find the address book palm sync conduit to install
> 
> I was running it from the command line

Did you use any switches?

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to comment #8)
> Did you use any switches?

initial testing was with no switches. behavior is the same using /n 

note results step 3. exact steps, thunderbird is running:
1. palmsyncinstall.exe /u
2. palmsyncinstall.exe 
   message: could not find conduit
   conduit: not registered
3. palmsyncinstall.exe 
   message: success
   conduit: registered
4. palmsyncinstall.exe 
   message: could not find conduit
   conduit: changed from registered to UNregistered

same results if thunderbird NOT running:

with /u switch
1. palmsyncinstall.exe /u
   message: error uninstalling conduit
   conduit: unregistered fine

I get MSVCR80.dll not found running palmsyncinstall.exe in programs\suiterunner\extensions\[e-mail]\  
but not when running palmsyncinstall.exe /u

Comment 10

[Mark Banner (:standard8)]

Attachment: Fix short path name problem

This should fix Wayne's problem with the previous patch. The main problem was that InstallConduit is called recursively (max twice) - the second time it already has the dll name in the path.

When we were using long filenames we were fine - however now we have the possibility of it being "MOZABC~1.DLL" so we need to deal with that case and I've added a check for .DLL in the relevant place.

I have also moved the attempt to get the short path name down to after we've actually added the DLL name as this gives us a better chance of ending up with a shorter path name.

Comment 11

[David :Bienvenu]

Comment on attachment 263211 [details] [diff] [review]
Fix short path name problem

thx, Mark

Comment 12

[Mark Banner (:standard8)]

I've checked the second patch in, so I'm marking this as fixed. We'll deal with anything else in bug 322628 or new ones.