78585 - Import Local Folders from 4.x

Status: VERIFIED FIXED

(MailNews Core :: Profile Migration, defect, P1)

Description

[Lynn Webb]

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.

Comment 1

[Lynn Webb]

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

Comment 2

[Jaime Rodriguez, Jr.]

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

Comment 3

[scottputterman]

reassigning to racham.

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

Comment 4

[scottputterman]

forgot to reassign.

Comment 5

[Lynn Webb]

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.

Comment 6

[Lynn Webb]

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.

Comment 7

[scottputterman]

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?

Comment 8

[Lynn Webb]

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.

Comment 9

[Grace Bush]

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 ***

Comment 10

[Lynn Webb]

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.

Comment 11

[Grace Bush]

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

Comment 12

[scottputterman]

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.

Comment 13

[Chris Hiner]

Dup of bug 63389?

Comment 14

[Arnoud Berendsen]

Yep, quite definately a dupe.

*** This bug has been marked as a duplicate of 63389 ***

Comment 15

[Lynn Webb]

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.

Comment 16

[Grace Bush]

Lynn,

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

Comment 17

[Lynn Webb]

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

Comment 18

[Grace Bush]

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? 

Comment 19

[Matthew T (active 1999-2002)]

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

Comment 20

[scottputterman]

reassigning to srilatha.

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

Comment 21

[Ninoschka Baca]

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

Comment 22

[scottputterman]

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).

Comment 23

[Ninoschka Baca]

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

Comment 24

[Grace Bush]

done- thnx

Comment 25

[Srilatha Moturi]

moving to 1.0

Comment 26

[Ninoschka Baca]

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

Comment 27

[robinf]

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

Comment 28

[Srilatha Moturi]

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.

Comment 29

[Srilatha Moturi]

Attachment: patch v1

Comment 30

[Srilatha Moturi]

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.

Comment 31

[Srilatha Moturi]

Attachment: patch for mac builds

Comment 32

[Srilatha Moturi]

Attachment: patch for the install packages

Comment 33

[Srilatha Moturi]

Attachment: patch with minor string changes

Comment 34

[racham]

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.

Comment 35

[Srilatha Moturi]

> 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 36

[Srilatha Moturi]

Attachment: patch v3

Comment 37

[racham]

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

r=bhuvan

Comment 38

[racham]

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

r=bhuvan

Comment 39

[racham]

Comment on attachment 74857 [details] [diff] [review]
patch v3

r=bhuvan.

Please do add a list of tests covered when you get chance.

Comment 40

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

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);
+      }
       }
     }

Comment 41

[Jennifer Mulcaster]

removing jenm as cc

Comment 42

[Srilatha Moturi]

Attachment: screen shot with the profiles list

Comment 43

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

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?

Comment 44

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

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.

Comment 45

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 46

[Srilatha Moturi]

> 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.

Comment 47

[Srilatha Moturi]

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

Comment 48

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

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);

Comment 49

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

yes, nsIFileSpec is deprecated.  

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

Comment 50

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

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?

Comment 51

[Srilatha Moturi]

Attachment: patch for the profile code to get 4x profileslist

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.

Comment 52

[Srilatha Moturi]

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.

Comment 53

[Srilatha Moturi]

Attachment: patch for the mailnews part

Comment 54

[robinf]

+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.

Comment 55

[Srilatha Moturi]

Attachment: updateed patch for profile code to get4x profilelist

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

Comment 56

[Srilatha Moturi]

Attachment: updated patch for the mailnews part

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 57

[Conrad Carlen (not reading bugmail)]

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.

Comment 58

[Cavin Song]

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.

Comment 59

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

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.

Comment 60

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

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);
+    }

Comment 61

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

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

or was it nsmail?

Comment 62

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

more comments:

1)

+			if (errorValue == false) {

as in C++, 

do 

if (!errorValue)

2)

+      if (errorValue == true) {

as in C++, do

if (errorValue) {

Comment 63

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

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?

Comment 64

[Srilatha Moturi]

Attachment: updated profile patch

Comment 65

[Srilatha Moturi]

Attachment: updated mailnews patch

Comment 66

[Srilatha Moturi]

Attachment: updated profile patch

corrected a typo in the profile patch

Comment 67

[Cavin Song]

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);

Comment 68

[Srilatha Moturi]

Attachment: mailnews patch

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

Comment 69

[Cavin Song]

Comment on attachment 76311 [details] [diff] [review]
mailnews patch

r=cavin.

Comment 70

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

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

sr=sspitzer

please get ccarlen to do the r=.

Comment 71

[Rajiv Dayal]

Attachment: patch for mac builds

removed the underscores in the xml filenames

Comment 72

[Srilatha Moturi]

The above comments were from me not from Rajiv Dayal.

Comment 73

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

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.

Comment 74

[Ninoschka Baca]

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

Comment 75

[Srilatha Moturi]

Attachment: updated mailnews patch

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 76

[Srilatha Moturi]

Attachment: patch for pref-migrator changes

Comment 77

[Conrad Carlen (not reading bugmail)]

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

r=ccarlen

Comment 78

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

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.

Comment 79

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

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 80

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

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 81

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

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

sr=sspitzer

Comment 82

[Srilatha Moturi]

Attachment: updated mailnews patch

Comment 83

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

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

sr=sspitzer

Comment 84

[Cavin Song]

Comment on attachment 76356 [details] [diff] [review]
patch for pref-migrator changes

r=cavin.

Comment 85

[Cavin Song]

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

r=cavin.

Comment 86

[Srilatha Moturi]

Attachment: patch for mac builds

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 87

[Jean-Francois Ducarroz]

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

R=ducarroz

Comment 88

[Asa Dotzler [:asa]]

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 89

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 90

[Asa Dotzler [:asa]]

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 91

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 92

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 93

[jglick]

>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/

Comment 94

[Srilatha Moturi]

Fix checked in

Comment 95

[Ninoschka Baca]

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. 

Comment 96

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 97

[Srilatha Moturi]

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

Comment 98

[Ninoschka Baca]

Verified Fixed.