Using (commercial) build 2001043004, I am unable to import my Communicator 4.x mail files after I migrate my Communicator profile over to Mozilla. I only see import options for Outlook, Outlook Express and Eudora files.

Also, my mail files on my local machine are not automatically migrated to Mozilla when profile migration takes place.

Adding esther, jenm, and roberts to cc: list.

reassigning to racham. Lynn, how many profiles did you have in 4.x?

forgot to reassign.

I had 3 in 4.x, but only used one profile (my main one) when I moved to Mozilla. My other profiles don't have mail set up anyway.

In another test, instead of using my existing 4.x profile from the start, I used a default Mozilla profile. Then later I went into Profile Manager and switched to my old, main 4.x profile to see if everything would be transferred to Mozilla. Once I was in the browser, I went to Mail and still saw that my local mail files were not accessible. I also couldn't import them, although I saw options for Eudora and Outlook.

The reason I asked is that when there's one profile we migrate automatically. When there's more than one you have to specifically say you want to migrate. When you first ran 6.0, did you choose your old 4.x profile and then get a dialog asking you if you wanted to migrate it? Did your bookmarks get migrated?

I was asked if I wanted to migrate and I did. I also saw my bookmarks were migrated. My Mail setup was also migrated except for the fact that I couldn't access the mail that I had stored on my local machine instead of IMAP.

reproduced on build 2001050904 for Mozilla Not able to reproduce on Netscape 6.5 - build 2001050804, LocalMail Folders are getting migrated this is duplicate of bug 73768 *** This bug has been marked as a duplicate of 73768 ***

I'm wondering if we can keep this open as an import issue, even though two issues were originally addressed in this bug. I think we should give users the option of importing Communicator/Navigator mail after creating a new profile as well.

Mail/News may be able to tell if there is already an RFE for import- otherwise I can reopen

I'll reopen. I don't actually know if there's a bug for this. If there is then we can mark it a dup of that one. marking nsbeta1- and moving to future milestone.

Dup of bug 63389?

Yep, quite definately a dupe. *** This bug has been marked as a duplicate of 63389 ***

Changed summary to reflect issue and reopening. I'm using IMAP and have locally stored files. I can migrate my profile, but these files are not migrated. The other bug is about the import problem.

Lynn, There are two other bugs on importing profiles, bug 22689 and bug 87145. Are your issues addressed by either or both of these?

I'd say that bug 87145 might solve the problem, but perhaps the mail profile migration is really the important issue. What do you think? What I meant this bug to be about was this: my mail files that I filter to be stored on my local machine instead of IMAP are not automatically migrated to Mozilla when profile migration takes place

I am able to migrate Local Mail and messages filtered to Local Mail (called Local Folders in N6. Migration only takes place once- after that you must simulate the process again, there is no way to import mail or other profile data (from Netscape 4.x) that has accumulated in your 4.x profile since the first migration. Is that what is taking place? Do you want to remigrate and have uptodate profile information in Mozilla?

*** Bug 108810 has been marked as a duplicate of this bug. ***

reassigning to srilatha. changing the summary. This bug is about adding the ability to import 4.x local mail folders into Mozilla.

Note: Importing from 4.x should include the address book.

We already can import 4.x address books. This will only work in the Netscape commercial build and won't be fixed in a mozilla build (of course, we can import with ldif or by csv or tab).

Grace, since this bug is now about importing specifically, do you want to assign qa to me?

done- thnx

moving to 1.0

Will importing Comm 4.x Local Folders work on all operating systems (Win, Mac, Linux)?

FYI, this is #2 on Decision One's Top 10 Problems list (and has been for a while).

I have been working on this. Yes, import folders will work on all platforms. I have this working in my windows and unix builds. Trying to get this work on Mac too. I will post a patch in the next couple of days.

The above patch works on Linux and windows. I will attach a separate patch with the mac project. I have tested this on all platforms.

Srilatha, Patches look good. I just have 2 questions. 1. Are you using nsFileSpec being forced by other modules or lack of interfaces in nsIFile ? 2. Will it not be possible to share any of migration code with with nsIProfile service ? thanks, bhuvan.

> 1. Are you using nsFileSpec being forced by other modules or lack of interfaces in nsIFile ? Yes. In nsComm4xImportMail.cpp, the code is implementing nsIImportMail interfaces which are using nsIFileSpec. In nsComm4xMail.cpp, some of the methods I need are not available in nsIFile and also the array that is created for the mailboxes needs to be nsIFileSpec since the nsIMailBoxDescriptor uses nsIFileSpec. I changed one occurence of nsIFileSpec in nsComm4xProfile.cpp. 2. Will it not be possible to share any of migration code with with nsIProfile service ? nope, since none of the methods in nsIProfile or nsIProfileInternal return a list of 4.x profiles. There is one method that returns a non-migrated profile list which does not include the 4.x migrated profiles. and also once a profile is migrated it is pointing to the 6.x profile dir. nsIProfile::GetProfileList is generated from the nsProfileAccess memeber mProfiles, which contains the lsit of non-migrated and migrated and new profiles but we still have the problem where we do not know which is a migrated profile and which is a new profile and also the directory for the migrated profile is pointing to the current one not 4.x. Ninoschka found one problem in the opt builds I created. The subfolders are not getting migrated. I will post a new patch with a fix for that.

Comment on attachment 73945 [details] [diff] [review] patch for mac builds r=bhuvan

Comment on attachment 73946 [details] [diff] [review] patch for the install packages r=bhuvan

Comment on attachment 74857 [details] [diff] [review] patch v3 r=bhuvan. Please do add a list of tests covered when you get chance.

I'm not sure I understand your design. When I got to import mail from 4.x, you want to show all the 4.x profiles (can you attach a screen shot?) And then when the users picks a profile, you then try to find the prefs.js for that profile, get the pref that points to the mail.directory (by reading and scanning the prefs.js file for that pref), and then copy the files over, skipping certain files. is that right? I think I'm seeing a lot of code copy and pasted from other parts of mozilla. Did you copy some of this code from the pref migration code and some from the profile code? I'd expect we'd have to do some copy-and-paste from to create a new import module (for comm4x), and some code to find the mail.directory pref, but that should be about it. For the other parts, we should be re-using the existing code, extending what's already there to be used by our import code. I didn't review all the code, I stopped once I saw the copy and paste of the migration code and the profile code. Here are just a few comments that I have, before I stopped: 1) is the 4.x pref for the mail dir a cstring, on all platforms? What about internationalized builds? + /** + * Searches the preferences file of the given profile for the pref mail.directory + * If the pref does not exist it, returns the default value. + * @param index Index of the profile in the profiles array + * @return The path to the mail directory for the profile at the fiven index. + * + */ + string getMailDir(in long index); +}; 2) I don't see where these were used. +%{C++ +#define NS_IMAPIREGISTRY_CONTRACTID "@mozilla.org/comm4xProfile;1" +#define NS_IMAPIREGISTRY_CLASSNAME "Communicator 4.x Profile" +%} 3) bad localization note (copy and paste error) +# Description of import module +# LOCALIZATION NOTE : "Communicator 4.x" below is the used for previous versions of Netscape Communicator +# Please translate using the brandname in respective languages for Netscape Communicator 4 releases. +## @name COMM4XMAILIMPORT_MAILBOX_SUCCESS +## @loc None +2002=Mailbox %S imported 4) is Comm4xMailDebugLog.h really necessary? this looks like copy-and-paste of what tonyr did for the other import modules. I guess if it was useful when you were working on it we don't have to remove it. but prlog would be been the way to go. 5) you really require these? why do we require msgcompose? + +REQUIRES = xpcom \ + string \ + intl \ + import \ + necko \ + mork \ + msgcompose \ + msgbase \ + editor \ + dom \ + mailnews \ + msgbaseutil \ + msgdb \ + msglocal \ + mimetype \ + unicharutil \ + uconv \ + $(NULL) 6) I think I understand why you need to link in msgbsutl, but why the other libraries? + +LLIBS=\ + $(DIST)\lib\xpcom.lib \ + $(DIST)\lib\msgbsutl.lib \ + $(DIST)\lib\unicharutil_s.lib \ + $(LIBNSPR) \ + $(NULL) + 7) On error, you set error.data to the same thing, and return false. You can rewrite this to be simpler, with out all the if / else nesting. { + if (selectedModuleName == gImportMsgsBundle.getString('Comm4xImportName')) + { + //open the profile dialog. + var comm4xprofile = Components.classes ["@mozilla.org/comm4xProfile;1"].createInstance(); + if(comm4xprofile != null) { + comm4xprofile = comm4xprofile.QueryInterface( Components.interfaces.nsIComm4xProfile); + if(comm4xprofile != null) { + var length = {value:0}; + var profileList = comm4xprofile.getProfileList(length); + if (profileList) + { + var promptService = Components.classes ["@mozilla.org/embedcomp/prompt-service;1"].getService (Components.interfaces.nsIPromptService); + if (promptService) { + var selected = {value:0}; + var clickedOk = false; + clickedOk = promptService.select(window, + gImportMsgsBundle.getString ('profileTitle'), + gImportMsgsBundle.getString ('profileText'), + length.value, profileList, selected); + if (clickedOk) { + var profileDir = comm4xprofile.getMailDir(selected.value); + mailInterface.SetData( "mailLocation", CreateNewFileSpecFromPath( profileDir)); + } + else { + error.data = gImportMsgsBundle.getString('ImportMailNotFound'); + return( false); + } + } // promptService + else { + error.data = gImportMsgsBundle.getString('ImportMailNotFound'); + return( false); + } + } // profileList + else { + error.data = gImportMsgsBundle.getString('ImportMailNotFound'); + return( false); + } + } // comm4xprofile + else { + error.data = gImportMsgsBundle.getString('ImportMailNotFound'); + return( false); + } + } // comm4xprofile + else { + error.data = gImportMsgsBundle.getString('ImportMailNotFound'); + return( false); + } + } + else { var filePicker = Components.classes ["@mozilla.org/filepicker;1"].createInstance(); if (filePicker != null) { filePicker = filePicker.QueryInterface( Components.interfaces.nsIFilePicker); @@ -676,6 +725,7 @@ else { error.data = gImportMsgsBundle.getString('ImportMailNotFound'); return( false); + } } }

removing jenm as cc

thanks for the screen shot. 1) is that the only new UI? if not, please provide screen shots. 2) does it work properly when there are more than n profiles (where n is the number of visible rows)? 3) does the UI and strings have buy off from jglick / robinf?

it looks like nsIProfile is froze, but not nsIProfileInternal why can't we use (or extend) it: /** * The following values are used with getProfileListX * * LIST_ONLY_NEW - the list will contain only migrated profiles * LIST_ONLY_OLD - the list will contain only un-migrated profiles * LIST_ALL - the list will contain all profiles * */ const unsigned long LIST_ONLY_NEW = 1; const unsigned long LIST_ONLY_OLD = 2; const unsigned long LIST_ALL = 3; void getProfileListX(in unsigned long which, out unsigned long length, [retval, array, size_is(length)] out wstring profileNames); adding ccarlen, the master of profiles, to the list so that he can be part of this discussion. Once we've finished figuring out how we can re-use (instead of copying) the profile code, we should discuss how to do the same with the migration code.

> For the other parts, we should be re-using the existing code, extending what's > already there to be used by our import code. > I didn't review all the code, I stopped once I saw the copy and paste of the > migration code and the profile code. That was my reaction as well. You should be able to use the Profile Mgr to access the list of profiles. Problem is, once a profile has been migrated, its info is updated in the registry to reflect its new location and the original unmigrated location is forgotten. For some other reason, I wanted to retain that info. If that was kept and there was an accessor, you could use that. If there's anything else needed, let me know and it can be put on nsIProfileInternal. Two other things: (1) The new XML projects shouldn't begin with underscores - an underscore gets added when the project is imported into an .mcp file. Files beginning with leading underscores can be searched for as files which are generated during the build process. Also, in your changes to the build script, they don't have the underscore there so that wouldn't build. (2) nsIFileSpec is being used all over the place in this code. That iface has been deprecated for years and when nsIFile moves to utf-8 only and the native implementations are normailized to Unicode, nsIFileSpec will really have to go. It's bad enough that we have the number of consumers of that iface right now, but we certainly can't have new consumers being added while trying to get rid of the current ones.

> That was my reaction as well. You should be able to use the Profile Mgr to > access the list of profiles. Problem is, once a profile has been migrated, its > info is updated in the registry to reflect its new location and the original > unmigrated location is forgotten. For me to get the 4.x profile dir I need the unmigrated location. Also as I understand from the code the profile list is generated from the mozregistry once it is created and we do not read the 4.x registry file. This is base on the code in nsProfile.cpp MigrateProfileInfo() which call nsProfileAccess::Get4xProfileInfo(). Calling Get4xProfileInfo() updates the profile list that is already created. > For some other reason, I wanted to retain that > info. If that was kept and there was an accessor, you could use that. No, this info is not kept. And also once a profile is migrated we cannot distinguish if it is a migrated profile or a new profile > If there's anything else needed, let me know and it can be put on nsIProfileInternal. Sure.

Talked to Conrad over the phone One way to avoid copying the code is 1) Save original 4.x profile directory in the registry. 2) Add a method to nsIProfileInternal to get the original directory 3) Make sure when GetProfileListX gets called we do update the list with the 4.x registry. >(1) The new XML projects shouldn't begin with underscores Will change that >(2) nsIFileSpec is being used all over the place in this code. I need to use nsIFileSpec since i am implementing the existing interface which used nsIFileSpec

sounds like you and ccarlen are on the right track. I'd much rather see us modify or extend the existing profile interfaces (and implementation) than duplicate the code in mozilla/mailnews/import. the other half of my comments were for copying the mail files. instead of doing what you did, look into doing this: 1) extend nsIPrefMigration.idl, add [noscript] copyMailFiles(in nsIFileSpec aSrcDir, in nsIFileSpec aDestDir); 2) for the impl, do this: NS_IMETHODIMP nsPrefMigration::CopyMailFiles(nsIFileSpec *aSrcDir, nsIFileSpec *aDestDir) { NS_ENSURE_ARG_POINTER(aSrcDir); NS_ENSURE_ARG_POINTER(aDestDir); return DoTheCopy(aSrcDir, aDestDir, PR_TRUE); } nsPrefMigration.cpp contains all the [platform specific] code for skipping 4.x summary files, etc. in your import code, once you have the src and dest dirs, do: nsCOMPtr <nsIPrefMigration> prefMigrator = do_CreateInstance ("@mozilla.org/profile/migration;1", &rv); NS_ENSURE_SUCCESS(rv,rv); rv = prefMigrator->CopyMailFiles(src, dest); NS_ENSURE_SUCCESS(rv,rv);

yes, nsIFileSpec is deprecated. alternatively, we could use nsIFile, and then in CopyMailFiles() convert from nsIFile to nsIFileSpec.

I was being over zealous. Please ignore my comment in http://bugzilla.mozilla.org/show_bug.cgi?id=78585#c48 srilatha explained to me how the import code works, and what we need to do to implement the nsIImportMail interface. Her code for that looks fine. Right now, I only have a few questions, like: 1) nsShouldIgnoreFile() looks copied from nsLocalMailFolder.cpp perhaps, we should move this into baseutil so it can be shared? 2) in ImportMailbox(nsIFileSpec *pSrc, nsIFileSpec *pDst), we have code to copy the file. why can't we use copyToDir() to copy pSrc to the parent of pDst?

This patch is just for the mozilla/profile changes. with the previous design and few revisions of it the existing profile information might get changes/affected because we will be updating the registry and also the profiles list and there is no distinction between the newly aded ones and existing ones. After discussing with Conrad and going over the pros and cons of the design we decided to change it and in my opinion this is the cleanest way of doing it by not affecting the current profile information and functionality. This is how it is implemented 1) Added a new flag(isImportType) to the profilestruct 2) when called from import module looks at the profiles of type importtype(isImportType flag set) and in all other cases looks at the profile which are not of import type(isImportType flag not set) 3)Added a new const LIST_FOR_IMPORT. When getProfileListX get called with this type only the profile with isImportFlag set will be returned. 4)The functionality for any of the existing APIs has not changed. Since all the existing methods look at the profiles for which the isImportType flag is not set. Thanks to conrad for helping me with this. I will post a patch for mailnews code shortly.

From #c40 > 1) is the 4.x pref for the mail dir a cstring, on all platforms? What about > internationalized builds? I dont this the 4.x pref is a cstring. That is why I am comvertign it from ucs2 to utf8 and then to a cstring. > + string getMailDir(in long index); I have this as a string because the return value is passed to CreateNewFileSpec (which is setting nsIFileSpec.nativePath) in importDialog.js which takes a string. > 2) done > 3) done > 4) Yes the debug log was useful for me. > 5) done > 6) removed unicharutil_s.lib. but need xpcom.lib and LIBNSPR (for nsCRT.h) >7) done From comment #c43 > 1) is that the only new UI? if not, please provide screen shots. That's(the profile lists dialog) the only new UI > 2) does it work properly when there are more than n profiles (where n is the > number of visible rows)? Ninoschka tried it with 26 profiles and it works fine. > 3) does the UI and strings have buy off from jglick / robinf? working on it. From comment #c50 > 1) nsShouldIgnoreFile() looks copied from nsLocalMailFolder.cpp > perhaps, we should move this into baseutil so it can be shared? we can do that. Also I see that there is one definition of nsShouldIgnoreFile() in nsImapMailFolder.cpp which is different from the one in nsLocalMailFolder.cpp. Trying to figure out what the right place for this would be. >2) in ImportMailbox(nsIFileSpec *pSrc, nsIFileSpec *pDst), we have code to copy >the file. why can't we use copyToDir() to copy pSrc to the parent of pDst? I did that because teh the pDst file was already created and copyToDir will fail in that case. When I initially wrote this code we were creating a new account for imported mail and then the Destination filename is not the same as source filename and copyToDir wouldn't work in that case. Now that we create a new folder for imported mail we will not run into that situation so I changed the code such that I am deleting the destination file and then calling copyToDir.

+2003=Bad parameter passed to import mailbox. Suggested wording: An internal error occurred. Importing the mailbox %S failed. Try importing the mailbox again. +2004=Error importing mailbox %S, all messages may not be imported from this mailbox. Suggested wording: An error occurred while importing mailbox %S. Messages were not imported. Make more disk space available and try again. Communicator 4.x profiles diaog box: Suggest changing "Choose a profile" text to: "Choose the profile that contains the Local Mail you want to import" It would be great if you could suppress display of this dialog in cases where there's only one 4.x profile to import from.

updated the patch based on Conrad's suggestions. Changed the wstring getOriginalProfilePath to nsILocalFile getOriginalProfileDir will post a new mailnews patch right away

updated the patch based on robin's suggestions >+2003=Bad parameter passed to import mailbox. > >Suggested wording: An internal error occurred. Importing the mailbox %S failed. > Try importing the mailbox again. After talking to robin changed the above to An internal error occurred. Importing failed. Try importing again. Changed this since we may not know the name of the mailbox.\ Also changed importDialog.js so that we do not bring up the profilelist dialog when there is only one profile. Changed nsComm4xprofile.cpp so that we free the *_retval before calling GetPath() in GetMailDir(). Thanks to Conrad for pointing this out. Also changed the call to GetOrginalProfileDir based on the changes in the profiles patch.

Comment on attachment 76055 [details] [diff] [review] updateed patch for profile code to get4x profilelist r=ccarlen - and for the portion of the mailnews patch which interacts with this code.

r=cavin if the following are addressed: 1. Typo 'fiven' in import/comm4x/public/nsIComm4xProfile.idl. 2. In nsShouldIgnoreFile(), should we even worry about ".msf" since 4.x doesn't use it anyway? I think this function should be tailored for 4.x only. 3. + nsCOMPtr<nsIStringBundleService> sBundleService = + do_GetService(kStringBundleServiceCID, &rv); should be: nsCOMPtr<nsIStringBundleService> sBundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); and you can remove 'kStringBundleServiceCID' declaration. We need to clean up other import modules later.

for http://bugzilla.mozilla.org/attachment.cgi?id=76055&action=view\ 1) + *originalDir = profileDir; + NS_IF_ADDREF(*originalDir); instead do: NS_IF_ADDREF(*originalDir = profileDir); 2) + if (fromImport) + profileItem->isImportType = PR_TRUE; + else + profileItem->isImportType = PR_FALSE; how about: profileItem->isImportType = fromImport; 3) + if (fromImport) + profileItem->isImportType = PR_TRUE; + else + profileItem->isImportType = PR_FALSE; + same thing as #2 4) + if (NS_FAILED(rv)) return rv; instead, do: if (NS_FAILED(rv)) return rv; this allows us to set a breakpoint on the return rv; address those 4 minor nits, and sr=sspitzer for the first patch.

some initial comments on the mailnews patch, I haven't finished reviewing: 1) +# Error Message +# LOCALIZATION NOTE : Do not translate the word "%S" below. +## @name COMM4XMAILIMPORT_MAILBOX_BADPARAM +## @loc None +2003=An internal error occurred. Importing failed. Try importing again. that localization note is bogus, there is no %S 2) please get robinf / jglick to review all new strings that show up in the UI. 3) + char * pName; + nsXPIDLCString dirName; + nsAutoString currentFolderNameStr; + PRBool isDirectory; + nsAutoString ext; + + while (exists && NS_SUCCEEDED(rv)) { + rv = dir->GetCurrentSpec(getter_AddRefs(entry)); + if (NS_SUCCEEDED(rv)) { + rv = entry->GetLeafName(&pName); + nsMsgGetNativePathString((const char *) pName, currentFolderNameStr); + isFile = PR_FALSE; + entry->IsFile(&isFile); + if (isFile) { + if (!nsShouldIgnoreFile(currentFolderNameStr)) { + rv = FoundMailbox(entry, &currentFolderNameStr, pArray, pImport); + if (NS_FAILED(rv)) + return rv; + entry->GetNativePath(getter_Copies(dirName)); + dirName.Append(".sbd"); + rv = entry->SetNativePath(dirName.get()); + if (NS_FAILED(rv)) + return rv; + exists = PR_FALSE; + entry->Exists(&exists); + isDirectory = PR_FALSE; + entry->IsDirectory(&isDirectory); + if (exists && isDirectory) { + rv = ScanMailDir (entry, pArray, pImport); + if (NS_FAILED(rv)) + return rv; + } + + } + } + PR_FREEIF(pName); there are code paths where pName will leak. instead, use a nsXPIDLCString, do + rv = entry->GetLeafName(getter_Copies(pName)); + nsMsgGetNativePathString(pName.get(), currentFolderNameStr); and remove the PR_FREEIF(pName); 4) + nsIFileSpec *pSpec = nsnull; + desc->GetFileSpec(&pSpec); + if (pSpec) { + pSpec->FromFileSpec(mailFile); + NS_RELEASE(pSpec); + } + rv = desc->QueryInterface(kISupportsIID, (void **) &pInterface); + pArray->AppendElement(pInterface); + pInterface->Release(); use comptrs, something like: nsCOMPtr <nsIFileSpec> pSpec; desc->GetFileSpec(getter_AddRefs(pSpec)); if (pSpec) pSpec->FromFileSpec(mailFile); nsCOMPtr <nsISupports> pInterface = do_QueryInterface(desc); if (pInterface) pArray->AppendElement(pInterface); 5) + nsIImportMail * pMail = nsnull; + nsIImportGeneric *pGeneric = nsnull; + rv = ImportComm4xMailImpl::Create(&pMail); + if (NS_SUCCEEDED(rv)) { + nsCOMPtr<nsIImportService> impSvc(do_GetService (NS_IMPORTSERVICE_CONTRACTID, &rv)); + if (NS_SUCCEEDED(rv)) { + rv = impSvc->CreateNewGenericMail(&pGeneric); + if (NS_SUCCEEDED(rv)) { + pGeneric->SetData("mailInterface", pMail); + nsString name; + nsComm4xMailStringBundle::GetStringByID (COMM4XMAILIMPORT_NAME, name); + pGeneric->SetData("name", (nsISupports *) name.get()); + rv = pGeneric->QueryInterface(kISupportsIID, (void **) ppInterface); + } + } + } + NS_IF_RELEASE(pMail); + NS_IF_RELEASE(pGeneric); again, use comptrs for pMail and pGeneric 6) looks like there is some tabs in import/resources/content/importDialog.js 7) +%{C++ +#define NS_ICOMM4XPROFILE_CONTRACTID "@mozilla.org/comm4xProfile;1" +#define NS_ICOMM4XPROFILE_CLASSNAME "Communicator 4.x Profile" +%} +#define NS_ICOMM4XPROFILE_CONTRACTID "@mozilla.org/comm4xProfile;1" +#define NS_ICOMM4XPROFILE_CLASSNAME "Communicator 4.x Profile" this is defined twice 8) + rv = GetPrefValue(resolvedLocation, PREF_NAME, PREF_END, _retval); use an XPIDLCString, to avoid the call to ::Free() 9) rv = mailLocation->Append("Mail"); does that work for unix? I thought it was "mail" on UNIX in 4.x 10) + nsString name; + PRUnichar * pName; + if (NS_SUCCEEDED(pSource->GetDisplayName(&pName))) { + name = pName; + nsCRT::free(pName); + } do this instead: + nsString name; + PRUnichar * pName; + if (NS_SUCCEEDED(pSource->GetDisplayName(&pName))) { + name.Adopt(pName); + }

> I thought it was "mail" on UNIX in 4.x or was it nsmail?

more comments: 1) + if (errorValue == false) { as in C++, do if (!errorValue) 2) + if (errorValue == true) { as in C++, do if (errorValue) {

1) + if (offset != -1) { with strings, do kNotFound instead of -1 2) about the string bundle code, I think what you have is too heavy weight (probably copied from the existing import code) I don't think you need nsComm4xMailStringBundle at all. the string bundle service caches string bundles. so all you really need is a helper function that gets the bundle. That helper function gets the bundle from the bundle service at the core, all you really need to do is one helper to get the string bundle and then get the string by id. see something like http://lxr.mozilla.org/mozilla/source/mailnews/extensions/mdn/src/nsMsgMdnGenera tor.cpp#131 If this is indeed copied from the other import code, can you log a bug for us to remember to clean it all up?

corrected a typo in the profile patch

Address 3 comments on "updated mailnews patch" (attachment 76159 [details] [diff] [review]) and r=cavin. 1. Need one extra tab for the return statements in function nsStringEndsWith(). 2. + nsIFileSpec * parent; + if (NS_FAILED (pDestination->GetParent(&parent))) I think you can use comptr like: nsCOMPtr<nsIFileSpec> parent; if (NS_FAILED (pDestination->GetParent(getter_AddRefs(parent)))) 3. + nsIFileSpec * inFile; + if (NS_FAILED(pSource->GetFileSpec(&inFile))) { I think you can make it: nsFileSpec inFile; dbPath->GetFileSpec(&inFile); then you can remove: + NS_RELEASE(inFile);

updated the patch based on Cavin's comments. Changed this + nsIFileSpec * inFile; + if (NS_FAILED(pSource->GetFileSpec(&inFile))) { to nsCOMPtr

Comment on attachment 76311 [details] [diff] [review] mailnews patch r=cavin.

Comment on attachment 76225 [details] [diff] [review] updated profile patch sr=sspitzer please get ccarlen to do the r=.

removed the underscores in the xml filenames

The above comments were from me not from Rajiv Dayal.

my apologies to srilatha and cavin. the original import code was never properly reviewed, and not well written, and they have been paying the price for it. 1) + PRInt32 endingLen = PL_strlen(ending); use strlen() instead 2) sorry for not catching this earlier. why do we need all this? + static nsresult Create(nsIImportMail** aImport); + nsCOMPtr <nsIImportGeneric> pGeneric; + rv = ImportComm4xMailImpl::Create(getter_AddRefs(pMail)); + +/////////////////////////////////////////////////////////////////////////////// // + +nsresult ImportComm4xMailImpl::Create(nsIImportMail** aImport) +{ + NS_PRECONDITION(aImport != nsnull, "null ptr"); + if (! aImport) + return NS_ERROR_NULL_POINTER; + + *aImport = new ImportComm4xMailImpl(); + if (! *aImport) + return NS_ERROR_OUT_OF_MEMORY; + + NS_ADDREF(*aImport); + nsresult rv = ((ImportComm4xMailImpl *) *aImport)->Initialize(); + return rv; +} + rv = ImportComm4xMailImpl::Create(getter_AddRefs(pMail)); This seems all hand-rolled and funky. Can we do something like this? nsCOMPtr <nsIImportMail> comm4xImporter = do_CreateInstance("contract id for this thing", &rv); NS_ENSURE_SUCCESS(rv,rv); nsCOMPtr <nsIImportGeneric> pGeneric = do_QueryInterface(comm4xImporter, &rv); NS_ENSURE_SUCCESS(rv,rv); In your factory, you'd have: NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(ImportComm4xMailImpl,Initialize) Then we don't need Create() at all. Or am I missing something? I see that this code is copied and pasted from the other import modules. If this type of approach works, we should do it this way and then log a bug to clean up the other import code. +NS_IMETHODIMP ImportComm4xMailImpl::GetDefaultLocation(nsIFileSpec **ppLoc, PRBool *found, PRBool *userVerify) +{ + NS_PRECONDITION(found != nsnull, "null ptr"); + NS_PRECONDITION(ppLoc != nsnull, "null ptr"); + NS_PRECONDITION(userVerify != nsnull, "null ptr"); + if (! found || !userVerify || !ppLoc) + return NS_ERROR_NULL_POINTER; use NS_ENSURE_ARG_POINTER() instead 3) +NS_IMETHODIMP ImportComm4xMailImpl::FindMailboxes(nsIFileSpec *pLoc, nsISupportsArray **ppArray) +{ + NS_PRECONDITION(pLoc != nsnull, "null ptr"); + NS_PRECONDITION(ppArray != nsnull, "null ptr"); + if (!pLoc || !ppArray) + return NS_ERROR_NULL_POINTER; use NS_ENSURE_ARG_POINTER() instead 4) +void ImportComm4xMailImpl::ReportStatus( PRInt32 errorNum, nsString& name, nsString *pStream) +{ + if (!pStream) return; + PRUnichar * statusStr; + const PRUnichar * fmtStr = name.get(); + nsresult rv = m_pBundleProxy->FormatStringFromID(errorNum, &fmtStr, 1, &statusStr); + if (NS_SUCCEEDED (rv)) { + pStream->Append (statusStr); + pStream->Append( PRUnichar(nsCRT::LF)); + } this looks like it leaks statusStr. 6) + rv = profileLocation->SetPersistentDescriptor((const char *) prefValue); do prefValue.get() 7) +#ifdef XP_UNIX + rv = profileLocation->Append("preferences.js"); +#elif defined (XP_MAC) + rv = profileLocation->Append("Netscape Preferences"); +#else + rv = profileLocation->Append("prefs.js"); +#endif Doesn't these strings live somewhere else already? I'm guessing at the mozilla/profile pref-migration code, probably one other place. If so, can't we find a way to only #define these three strings once, like: #ifdef XP_UNIX #define PREF_FILE_NAME_IN_4x "preferences.js" #elif defined (XP_MAC) #define PREF_FILE_NAME_IN_4x "Netscape Preferences" #else #define PREF_FILE_NAME_IN_4x "prefs.js" #endif and then change your code to be: rv = profileLocation->Append(PREF_FILE_NAME_IN_4x); I suggest the C++ block in nsIPrefMigration.idl and then include nsIPrefMigration.h.

I tested Srilatha's special build on my WinMe system and it's working as expected. All folders, subfolders and messages are appearing as expected. Messages included a variety such as plain, html, attachments (gif, jpg, doc, xls, html) - Imported from 4.x profile with an IMAP account - Imported from 4.x profile with a POP account

1) done 2) defined a new contractid for ImportCOmm4xMailImpl and moved some code from nsComm4xImport.cpp to .h file and got rid of Create. and it works! Will file a bug to cleanup other modules 3) done 4) changed statusStr to nsXPIDLString 6) done 7) done will post a new patch with the pref-migration code changes

Comment on attachment 76225 [details] [diff] [review] updated profile patch r=ccarlen

Comment on attachment 76356 [details] [diff] [review] patch for pref-migrator changes sr=sspitzer once you remove this comment: /* and the winner is: Windows */ It went with the other comment you removed: -/* who's going to win the file name battle? */ we don't need that any more.

the mailnews patch is looking good. I'm glad that you were able to get rid of create. don't forget to log a bug for the clean up of the other existing code. three remaining issues: 1) +static NS_DEFINE_CID(kComm4xMailImportCID, NS_COMM4XMAILIMPORT_CID); I don't think you need to define the second CID in nsComm4xMailImport.cpp 2) + if (!nsCRT::strcmp(pImportType, "mail")) { use strcmp() instead, for performance. 3) + nsXPIDLString name; + rv = m_pBundle->GetStringFromID( COMM4XMAILIMPORT_NAME, getter_Copies(name)); + pGeneric->SetData("name", (nsISupports *) name.get()); + rv = pGeneric->QueryInterface(kISupportsIID, (void **) ppInterface); whoa, casting a const char * to a nsISupports *? I see the existing import code doing that, but it's not right. I think you should create a nsISupportsString, and pass that in, but make sure who ever gets the "name" thing coming out can handle it, and isn't just casting back to a const char *. You might end up having to fix the other callers and the "getter" code that.

Comment on attachment 76324 [details] [diff] [review] patch for mac builds sr=sspitzer, but please get a mac guru to review it, like ducarroz.

Comment on attachment 73946 [details] [diff] [review] patch for the install packages sr=sspitzer

Comment on attachment 76377 [details] [diff] [review] updated mailnews patch sr=sspitzer

Comment on attachment 76356 [details] [diff] [review] patch for pref-migrator changes r=cavin.

Comment on attachment 76377 [details] [diff] [review] updated mailnews patch r=cavin.

updated the patcch by removing the password stuff. also removed some access paths that are not needed. thanks to Jean Francois for helping with this

Comment on attachment 76454 [details] [diff] [review] patch for mac builds R=ducarroz

Comment on attachment 73946 [details] [diff] [review] patch for the install packages a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 76225 [details] [diff] [review] updated profile patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 76356 [details] [diff] [review] patch for pref-migrator changes a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 76377 [details] [diff] [review] updated mailnews patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 76454 [details] [diff] [review] patch for mac builds a=asa (on behalf of drivers) for checkin to the 1.0 trunk

>Hi Jennifer, Robin, > >For importing mail from local folder from 4.x I added a new dialog. The new >dialog shows the user the list of 4.x profiles from which he/she can import >mail from. I am attaching a screen shot of the dialog. Can you please look at >it and let me know as soon as possibe if the UI and strings are ok. and also >these some more strings that will be added >2000=Communicator 4.x >2001=Import Local Mail from Communicator 4.x. >2002=Mailbox %S imported (%S will be replaced with whatever the mailbox name >is) >2003=Bad parameter passed to import mailbox. >2004=Error importing mailbox %S, all messages may not be imported from this >mailbox. (%S will be replaced with whatever the mailbox name is) Dialog you have is fine. I would just add a colon after "Choose a profile:". As for the text strings, its hard to know on some of them what to suggest without knowing the circumstances in which they appear, but from what I can see: 2000 and 2001 seem fine. 2003. We are importing local messages right? I would probably say that instead of 'mailbox' then. "Local messages were successfully imported from %S". 2003. "Bad parameter passed to import mailbox" I don't think users will understand this one. Try and tell them exactly what went wrong and what they should do to fix the problem. 2004. Do we know what caused the error or what the user can do to fix it? If not, maybe "An error occured while trying to import messages from %s. All messages may not have been imported." There is a spec if we have time for improvements in the future: http://www.mozilla.org/mailnews/specs/import/

Fix checked in

Trunk build 2002-04-01: WinMe, Mac 10.1.3, fixed. Trunk build 2002-04-01: Linux RH 7.1 After selecting File|Import, Mail radio button, Comm 4.x then it automatically retrieves Local Folders from the last profile run in 4.x. I never get a dialog showing all the 4.x profiles. Should this be possible on linux? Srilatha, I just want to double check with you regarding the text in the dialogs.

> I never get a dialog showing all the 4.x profiles. Should this be possible on linux? On linux with 4.x, there was only one profile per OS account so, no, it shouldn't be possible.

Conrad is right regarding the single profile on linux. you will never see the list of profiles > Srilatha, I just want to double check with you regarding the text in the > dialogs. any text in particular? let me know. ping me on AIM

Verified Fixed.