182405 - Palm address book conduit installs with nonsense title

Status: VERIFIED FIXED

(MailNews Core Graveyard :: Palm Sync, defect)

Description

[James Shiell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20021126

After installing the conduit the name appears in the Custom dialog of Palm
Desktop as a string of nonsense characters. This is mentioned in bug 155417 as well.

Reproducible: Always

Steps to Reproduce:
1. Install Palm conduit.
2. Open custom dialog in Palm Desktop.


Actual Results:  
Address book name is nonsensical characters.

Expected Results:  
Expected something similar to "Mozilla Address Book" or "Address Book"

Comment 1

[Ben Liblit]

I'm seeing more or less the same thing here.  AB conduit shows up in Palm
HotSync's "Custom" dialog as ",": a single comma character with no surrounding
quotes.  Shows up the same way in the HotSync progress dialog when synchonizing.

Quitting and restarting the HotSync manager has no effect.

Comment 2

[Pat Patterson]

This appears to be due to the lack of a 'Name' string in the
HKEY_CURRENT_USER\Software\U.S. Robotics\Pilot Desktop\ApplicationX registry key.

Find the currect key (the one with Conduit as C:\Program
Files\mozilla.org\Mozilla\mozABConduit.dll or whatever), and add a 'Name' string
with value 'Mozilla AddressBook'. Conduit now has that title in the list.

Comment 3

[Rajiv Dayal]

Thanks Pet for your findings. Will try and fix this for the 1.3 beta version in
the Mozilla palm sync install code.

Comment 4

[George]

Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.2.1) Gecko/20021130
Using Palm Desktop 4.1.

I can also confirm the problem, and that Pat's registry fix works.

I still can't use the "Change" button to alter the sync options for the Mozilla
Addressbook, which I originally assumed was related, but perhaps not.

Comment 5

[Cavin Song]

Taking.

Comment 6

[Cavin Song]

Attachment: Proposed patch, v1

Set title to "Mozilla Address Book" (but it may be changed later).

Comment 7

[Cavin Song]

Attachment: Screen shot of the new Custom lists.

Comment 8

[Cavin Song]

Ccing Robin for title suggestion.

Comment 9

[robinf]

"Mozilla Address Book" looks OK.

Comment 10

[Rajiv Dayal]

For (id=112449):

I think it will be a good idea to keep it in a separate resource string table so
that it can be localized and changed for Netscape releases too. You will have to
any way create a resource string table if you plan to display log messages.

Comment 11

[Thomas Lostaunau]

The patch will help, but when the conduit starts logging I believe it will be
garbled again.  The problem is that GetConduitName() in MozABConduitGenCond.cpp
is not filling in the buffer properly.  It should be doing a strncpy or strcpy
into the buffer.

Comment 12

[Cavin Song]

From the C/C++ Sync Suite Reference manual, it looks like 'Title' is the one 
being used if it's set:
"
HotSync Manager version 3.0.1 and later attempt to retrieve the name of your 
conduit in this sequence:

1. Read the name entry written with the CondCfg utility or the Conduit Manager 
API during conduit installation. (This refers to the 'Title' setting the patch 
is doing).
2. Else call GetConduitInfo
3. Else call GetConduitName.
"

Comment 13

[Cavin Song]

Attachment: Proposed patch, v2

Use 'brand short name' to construct display messages and conduit title.

Comment 14

[Samir Gehani]

Mail triage team: nsbeta1+/adt1

Comment 15

[Sean Su]

Comment on attachment 113624 [details] [diff] [review]
Proposed patch, v2

sorry for the delay.
r=ssu

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 113624 [details] [diff] [review]
Proposed patch, v2

1)

general issue question for ssu / cavin, forgive my ignorance:

does this change make the palm sync installer use XPCOM where it wasn't before?

or is this code run inside the mozilla process?  Is there anything to worry
about?	

(like having multiple processes accesing the component registry, or is that
safe now?)

2)

+  nsCOMPtr<nsIStringBundleService> stringService =
do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
+  if (NS_FAILED(rv) || !stringService)
+    return FALSE;

checking rv is enough. it won't succeeded if do_GetService() fails to get the
service.

3)

+  nsCOMPtr<nsIStringBundle> stringBundle;
+  rv = stringService->CreateBundle("chrome://global/locale/brand.properties",
getter_AddRefs(stringBundle));
+  if (NS_FAILED(rv) || !stringBundle)
+    return FALSE;

checking rv is enough.	it won't succeeded if CreateBundle() fails to create a
bundle.


4) a spin off issue: 

Something seems wrong with the sentence:

"Failed to register the Mozilla's Palm Sync Support Dll"

I think you might want:

"Failed to register the Mozilla Palm Sync Support Proxy Dll"
or
"Failed to register Mozilla's Palm Sync Support Proxy Dll"

(I suggest the first, but ask robinf)

Also, I'm not sure if Dll, DLL, or dll is preferred.

sr=sspitzer, assuming issue #1 isn't a blocker.  (check with ssu)

feel free to spin off the wording issue into a new bug.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 113624 [details] [diff] [review]
Proposed patch, v2

taking back the sr, as ssu / cavin are working out some issues.

Comment 18

[(not reading, please use [e-mail] instead)]

cavin and ssu are going to sort this out.

here's the conversation that ssu, cavin and I had concerning this bug:

sspitzer: ssu, is there anything to worry about with making the palm sync 
installer (is it out of process?) an xpcom app?
sspitzer: or was it already?
ssu: not as far as I'm aware of because this is not the native win32 installer 
(or other native installers).  the palm sync installer is run after mozilla is 
installed
ssu: so everything (xpcom files) should all be there
ssu: oh I just remembered something.  cavin, do you know if the palm sync 
installer sets a flag in the windows registry if it had been run?
sspitzer: ok, anything to worry about if more than one process is trying to use 
the component registry at the same time?  (mozilla + the palm sync installer)?  
or do they not run concurrently?
ssu: cause the mozilla uninstaller will need to know if the palmsync installer 
was run, so it can run the palmsync uninstaller
cavin: no, i don't think so
cavin: where would it set the flag?
ssu: they do not run concurrently. the palmsync installer is run by the user 
after installation is done
ssu: since palmsync requires xpcom now, it can't be run by the mozilla 
installer because xpcom registration would not have been done yet
ssu: not sure if this will be a requirement in the future (that the palmsync 
installer be run by the mozilla installer)
ssu: cavin: not sure where it should be set.  I'll swing by and chat with you 
on an appropriate place
cavin: ok
ssu: this can be a different bug from the patch you did
cavin: yeah let's make it a separate problem to deal with.
sspitzer: ok, so what does this mean for cavin's bug?  can we not use XPCOM yet?
cavin: i can try the mozilla uninstall scenario and see if palmsync uninstall 
will be run.
ssu:  we can right now because xpcom registration is done at the end of the 
mozilla installer
cavin: we need to get the brand name out of our product though.
sspitzer: cavin:  I think the other installer has files that need localizing
sspitzer: meaning, they don't have to live in mozilla
sspitzer: or maybe not, they just might be duplicated in the ns tree.  ssu, do 
you think using string bundles is the right way to fix this issue?
ssu: *thinking*
sspitzer: (like do we need a PalmSyncInstall.rc in the ns tree?)
ssu: well.. if we don't, then we'll need update the build process to do a 
relink of the rc file in the ns tree
sspitzer: I'm just nervous about xpcom-component registry side effects.
ssu: we might not need a full duplicate PalmSyncInstall.rc file.  I think it 
supports .h files.  we might be able to do soemthing with just header files
ssu: seth, okay, I'll go talk with cavin in person about build tricks with .rc 
files
ssu: I'll work with him on coming up with a patch that does not require xpcom.
cavin: i actually prefer this option.
ssu: it is simpler, but I was just worried about product name changes by 3rd 
party people too.  
cavin: you mean brand name?
ssu: but I guess they'll have to do update another file for branding purposes
ssu: yes
ssu: not sure if they'll want everything to be called 'Mozilla'

Comment 19

[Thomas Lostaunau]

The patch fixes some of the problems, but the Call to GetConduitName will never
return the correct information the way it is written.  Below is the docs for
GetConduitName.  The current impementation in MozABConduitGenCond.cpp is:
#define CONDUIT_NAME "MozABConduit"
ExportFunc long GetConduitName(char* pszName,WORD nLen)
{
    pszName = CONDUIT_NAME;    
    return 0;
}

I have changed my local version to do:
    strcpy(pszName,CONDUIT_NAME);
and without any changes to registry it displays correctly. 


From: http://www.palmos.com/dev/support/docs/recipes/recipe_conduitapi.html
2. Implement GetConduitName

The GetConduitName function is called by HotSync Manager to provide the name of
a conduit. You must implement this function to return a name buffer and the
status of the operation. The following code is a sample of the GetConduitName
function:

ExportFunc long GetConduitName(char* pszName,WORD nLen)
{
    long retval = -1;

    // IDS_CONDUIT_NAME is maintained in the project String Table
    // and contains the conduit name.

    if (::LoadString((HINSTANCE)hLangInstance, IDS_CONDUIT_NAME,
        pszName, nLen))
        retval = 0;

    return retval;
}

Comment 20

[Cavin Song]

Thomas, thanks for pointing it out. I'll include it in the patch as well.

Comment 21

[Cavin Song]

Attachment: Proposed patch, v3

Used .rc file, instead of XPCOM / string bundles for the brand name.

Comment 22

[Cavin Song]

Comment on attachment 114392 [details] [diff] [review]
Proposed patch, v3

Carry over r=ssu,sr=sspitzer from bugscape 22615.

Comment 23

[Cavin Song]

Fix checked in.

Comment 24

[Cavin Song]

Marking FIXED.

Comment 25

[Ninoschka Baca]

Trunk build 2003-03-14: WinXP
Verified Fixed.