Palm Sync needs this fix so that users do not have to restart mozilla before doing a hotsync. The problem is that in the addrbook code global var 'dir_ServerList' is used to hold the list of addressbooks and it is not the same pointer (when seen/used) in both the conduit and mozilla programs (since both programs link in addrbook dll/lib). So we need to make accessing of 'dir_ServerList' in nsDirPrefs.cpp a service (or something equivalent).

this is going to be some work to fix. the palm code *statically* links in the addressbook code, which is going to lead to problems like this (and worse) mozilla\mailnews\extensions\palmsync\src\Makefile.in(72):EXTRA_DSO_LIBS += addrbook_s we don't want to do that. you're going to end up with two .dlls with the same symbols, which is why you have this problem. you are going to want to stop doing that, and fix the palmsync code to not use any internals of the addrbook XPCOM component. doing a quick search for DIR_* in mozilla\mailnews\extensions\palmsync, I see three usages (let's hope there isn't more) a) DIR_GetDirectories() [should be exposed] b) DIR_AddNewAddressBook() [partially exposed, but you can't create MAPI abs with it] c) DIR_SavePrefsForOneServer() [not exposed, but needed for modify, and I think a patch lives in another bug] we either have to dynamically link against the addrbook .dll (not xpcom, I better find out if this is ok), or expose the right things to do these three methods. I'll talk to cavin more about this in person.

some of the work (if we go the xpcom route) has been done by srilatha in another bug.

Also this has a problem that user sees, if Mozilla Address Book Window is open and user does a Palm Sync, the user has to close Mozilla AB and open again to see the changes done by sync. This was not considered critical or a blocker for the first release that we did in Mozilla 1.2 and also doing the change to DirServers is not straight forward since it effects AB majorly, several AB features would be effected. DirServers has lead to issues with other features like LDAP Sync, etc. Currently the design of DirServers is not really object oriented I guess it was moved from 4x and uses global vars, etc. But I feel it needs to be solved at some point.

the problem isn't necessarily with DIR_*, the problem is that everything should be going through the addressbook XPCOM component. but the problem is not all the DIR_* functionality has been exposed, so it left people no choice. when the consumer was from inside the addressbook component it not a big deal, since you could just use the DIR_* methods from C++. but since DIR_* isn't exposed (or scriptable) it lead to some problems. See bug #17230 (modify/rename) and bug #124059 (adding) There are two partical patches (from srilatha) to address these problems, but they need some work. if we were to finish those two bugs (they are nsbeta1+), I think we could then finish up this bug by going through XPCOM, and we wouldn't need to use the DIR_* methods at all.

note, those patch got worked into another patch, by shuehan (see bug #124007) but I think we have to go low risk here, and just start out with these two bugs. we'd like to fix #124007, but that patch needs a lot of work before it's ready. I'll talk this over with cavin

Mail triage team: nsbeta1+/adt1

Replace references to DIR_* with addrbook xpcom apis. It also includes patch from 190233 (dealing with string manipulation) due to the fact that 190233 has not been checked in.

Comment on attachment 115664 [details] [diff] [review] Proposed patch, v1 some initial comments 1) since you are chaning modifyAddressBook(), nsAbPalmHotSync::ModifyAB() will need to be changed 2) also, where are you getting this enum from? MAPIDirectory if it is coming from from nsDirPrefs.h, you don't want to include that file anymore. you shouldn't have "#include "nsDirPrefs.h" in the palmsync code 3) now that you don't depend on DIR_*, no need to do this: mozilla\mailnews\extensions\palmsync\src\Makefile.in EXTRA_DSO_LIBS += addrbook_s 4) + PRUint32 fileNameLen = strlen(fileName.get()); do fileName.Length() 5) + (dirType == PABDirectory)) remember, you can't do PABDirectory anymore either.

Incorporated inital comments. Still need to link in addrbook_s in order to make palmsync.dll.

Comment on attachment 115664 [details] [diff] [review] Proposed patch, v1 clearing request

Comment on attachment 116117 [details] [diff] [review] Proposed patch, v2 1) + nsresult rv = NS_OK; + nsCOMPtr<nsIRDFService> rdfService = do_GetService (NS_RDF_CONTRACTID "/rdf-service;1", &rv); + if(NS_FAILED(rv)) return E_FAIL; no need to initialize rv there. 2) can you add a comment about what directories you are skipping here? + if (((fileName.Length() > kABFileName_PreviousSuffixLen) && + strcmp(fileName.get() + fileName.Length() - kABFileName_PreviousSuffixLen, kABFileName_PreviousSuffix) == 0) && + (dirType == kPABDirectory)) continue; 3) if the description is non US-ASCII, won't this code cause problems? + nsCAutoString abName = NS_LossyConvertUCS2toASCII(description); 4) a, shouldn't this be a cast to (const char *) b, remove the extra ; - nsresult rv = palmHotSync.DeleteAB(aCategoryId, aABName, (char*)aABUrl);; + nsresult rv = palmHotSync.DeleteAB(aCategoryId, (char*)aABUrl);; 5) + nsresult rv = palmHotSync.RenameAB(aCategoryId, (char*)aABUrl);; shouldn't this be a cast to (const char *) 6) + // TODO: may need to skip mailing list?? but maybe not since there's no mailing list on the top level. no mailing lists at the top level. 7) can you add a comment about what you are skipping here? + if (((fileName.Length() > kABFileName_PreviousSuffixLen) && + strcmp(fileName.get() + fileName.Length() - kABFileName_PreviousSuffixLen, kABFileName_PreviousSuffix) == 0) && + (dirType == kPABDirectory)) continue; 8) + if(!mFileName.Length()) return NS_ERROR_FILE_INVALID_PATH; how about: if (mFileName.IsEmpty()) 9) +nsresult nsAbPalmHotSync::UpdateABInfo(PRUint32 aModTime, PRInt32 aCategoryId) +{ + // Fill in perperty info and call ModifyAB(). typo, perperty -> property

Comment on attachment 116229 [details] [diff] [review] Incorporated comments. r/sr=sspitzer but I don't like that we are casting away constness ++ nsAbPalmHotSync palmHotSync(aIsUnicode, aABName, (char*)aABName, aCategoryId); can you fix nsAbPalmHotSync::nsAbPalmHotSync() to take a const char *, so that we don't have to do that? once you fix that, r/sr=sspitzer

Fix checked in with. Issue in comment #13 was also addressed.

Trunk buid 2003-03-14: WinXP Verified Fixed, the basics are working. I'll perform more scenarios with the MozAB open and log separate bugs if issues arise.