86991 - imported mail needs to be placed as subfolder to "Local Folders" instead of as new incoming servers (of type "none")

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P2)

Description

[esther]

Per mail triage meeting, the current listing of the imported Outlook mail and
folders is confusing (see bug 70540).  Since we can't fix bug (70644) which
would have avoided the confusion, we need identify the imported mail better. 

Overview: When importing "Mail", the name of the account assumes the name of the
original email application (i.e. Outlook Express)

Steps to reproduce:
1. Create a new profile
2. When Mail starts cancel the Account Wizard
3. Select File|Import
4. Select the Mail radio button, Next
5. Select Outlook Express, Next

Actual Results: The name of the account that appears in the folder pane is
"Outlook Express".

Expected Results: The name should be "Imported Mail from Outlook Express"
So the user does not think the Outlook Express account was imported with
settings.  All we imported were the folders with messages.

Comment 1

[esther]

adding nsbeta1 because fixing this bug alleviates the need to fix bugs 70644 and
70540 now.

Comment 2

[jglick]

I don't think this solution is going to fix the confusion for users. First, the 
name is too long and will be truncated.  Second, even though the first part of 
the name is "Imported Mail...", the confusion is not necessarily the name, but 
in the fact that we are showing Imported Local messages as an Account, which it 
isn't. It will look like an account, but won't behave, as users will expect, 
like the other accounts.

As I mentioned in 70644:
I would expect that these messages would be grouped UNDER our Local Folders. Not 
as a separate account.  An account wasn't created. Local messages from a 
different application were pulled in. For example:

Local Folders
   Unsent Messages
   Drafts
   Templates
   Sent
   Trash
   Outlook Express Mail (subfolders within this folder as nec)

Comment 3

[Jaime Rodriguez, Jr.]

Is this really a nsbeta1+, P2? If yes, then we need to try and targeted to a
milestone M1.0 or earlier to make the beta.

Comment 4

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

I'm ok with what jglick suggested on 
http://bugzilla.mozilla.org/show_bug.cgi?id=86991#c2

minor questions:

1)  what if I import multiple times, or if the folder already exists?

Do we want to append a unique number?

Local Folders
  Trash
  Drafts
  Outlook Express Mail 
  Outlook Express Mail 2
  Outlook Express Mail 3

or perhaps:

Local Folders
  Trash
  Drafts
  Outlook Express Mail  
  Outlook Express Mail (date and time)
  Outlook Express Mail (date and time)

The good thing about this is users could rename the folder pretty easily.

2) to clarify, we will not be treating the imported folders as special folders 
when we import mail.  The will just be generic folders.

please confirm (and clarify) and update the spec, so I know it's official.  

Comment 5

[jglick]

>Outlook Express Mail 
>Outlook Express Mail 2
>Outlook Express Mail 3

Appending a number is fine (and shorter!).
I will try and update the spec soon.

Comment 6

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

we'd want this same behavior for imported 4.x mail, right?

if so, srilatha might be the one to take this, as she's working on that, and 
implementing this will make her life easier.

over to srilatha.

Comment 7

[scottputterman]

Yeah, we'd want this for imported 4.x code since we won't be associating it with
an account.

Comment 8

[Srilatha Moturi]

Attachment: patch v1

Comment 9

[Navin Gupta]

Comment on attachment 70758 [details] [diff] [review]
patch v1

The folder creation part 
looks ok to me, one thing
though

there is an accessor 
GetRootMsgFolder on server.Use that directly to get localRootFolder

Comment 10

[Srilatha Moturi]

Attachment: patch v2

Now using GetRootMsgFolder.

Comment 11

[Navin Gupta]

Comment on attachment 70778 [details] [diff] [review]
patch v2

r=naving

Comment 12

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

+  if (m_pName) {
+    name.AssignWithConversion(m_pName);
+    name.Append(" mail");
+  }
+  else
+    name.Assign("Imported Mail");

do those strings show up in the UI?  if so, we have to move them to a string 
bundle.

looks like we'll need two strings, one "%S mail" (which we'll need to use the 
formatter) and one for "Imported Mail".

Comment 13

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

updating summary to reflect what srilatha is going for.

srilatha, this change will affect import of all mail types:  OE, Outlook, 
Eudora and eventually 4.x local mail, right?  please make sure to test those, 
and to test mac. (we have at least import of eudora on mac.)

note to jglick:  are we worried about the discoverability of imported mail?  
perhaps in the import wizard, after import, we should tell the user where they 
can find there imported mail?

Comment 14

[Srilatha Moturi]

Attachment: patch v3. using String bundle for strings

Tested this patch on windows with OE and outlook. will test with Eudora on Mac
and windows

Comment 15

[jglick]

>we'd want this same behavior for imported 4.x mail, right?

Importing Messages only, yes. When importing a whole account, it should be at 
account level.

>to clarify, we will not be treating the imported folders as special folders 
>when we import mail.  The will just be generic folders.

Yup.

>note to jglick:  are we worried about the discoverability of imported mail?  
>perhaps in the import wizard, after import, we should tell the user where they 
>can find there imported mail?

what about expanding the Local Mail twistie and focusing the imported folder in 
3 pane on completion of import?

Comment 16

[Srilatha Moturi]

>what about expanding the Local Mail twistie and focusing the imported folder in 
>3 pane on completion of import?

If the user is importing Mail from any other window other than the Mail window 
this is not possible. Having a message with the location of the imported mail in 
the import wizard would be good.

Comment 17

[jglick]

Showing text on the last screen of the Import Wizard is fine too. :-)

Comment 18

[Srilatha Moturi]

Tested with Eudora on Mac and windows. I will file a separate bug for the 
message in the Import wizard

Comment 19

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

this code isn't i18n safe.  looking at it, if localizers set 

to anything other than US-ASCII, it won't work.

+DefaultFolderName=Imported Mail
+ModuleFolderName=%S Mail

cc nhotta and bienvenu for comments.

two minor issues, and the on big i18n issue:

1)

don't add this:
+static NS_DEFINE_CID(kStringBundleServiceCID, NS_STRINGBUNDLESERVICE_CID);

instead use this:
NS_STRINGBUNDLE_CONTRACTID

2)

instead of:

+  if (NS_FAILED(rv) || !bundleService) return PR_FALSE;
+  if (NS_FAILED(rv)) return PR_FALSE;

do this:

if (NS_FAILED(rv) || !bundleService) 
  return PR_FALSE;

if (NS_FAILED(rv)) 
  return PR_FALSE;


the reason for this convention (inspired by waterson) is so that you
can set a break point on the return.

3)

+  nsCString name;
+  if (m_pName) {
+    const PRUnichar *moduleName[] = { m_pName };
+    rv = bundle->FormatStringFromName(NS_LITERAL_STRING("ModuleFolderName").get(),
+                                      moduleName, 1,
+                                      getter_Copies(folderName));
+  }
+  else {
+    rv = bundle->GetStringFromName(NS_LITERAL_STRING("DefaultFolderName").get(),
+                                   getter_Copies(folderName));
+  }
+  if (NS_FAILED(rv)) {
+      IMPORT_LOG0( "*** Failed to get Folder Name!\n");
+      return PR_FALSE;
+  }
+  name.AssignWithConversion(folderName);

This conversion will fail in the property is non-US-ASCII, right?

later, I see you go back from char * to PRUnichar * to call createSubfolder().

The problem is you're building on non-i18n friendly code written along time ago.

These two methods of nsIMsgFolder

	boolean containsChildNamed(in string name);
	string generateUniqueSubfolderName(in string prefix,
                                     in nsIMsgFolder otherFolder);

and this method of nsIFolder
 nsISupports GetChildNamed(in string name);

need to be fixed to use wstring.  Luckily, I think they are only used by the
import code.

Comment 20

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

I should add that the existing import code doesn't look i18n friendly either.

Comment 21

[Srilatha Moturi]

1) and 2) I will fix those in my next patch
I agree with Seth on 3). If we can change the string to wstring the i18N issue 
will be solved. 

Comment 22

[Srilatha Moturi]

Attachment: methods using wstring instead of string.

In this patch I changed the two methods of nsIMsgFolder
	boolean containsChildNamed(in string name); 
	string generateUniqueSubfolderName(in string prefix,
				     in nsIMsgFolder otherFolder);
to 
	boolean containsChildNamed(in wstring name); 
	string generateUniqueSubfolderName(in wstring prefix,
				     in nsIMsgFolder otherFolder);

and the method of nsIFolder
 nsISupports GetChildNamed(in string name);
to
 nsISupports GetChildNamed(in wstring name);

Comment 23

[nhottanscp]

Looks fine with the wstring changes.
One question, where does the folder name come from? Is that always taken from
the bundle? If anything coming from the actual file name then we need to convert
that to Unicode.

Comment 24

[Srilatha Moturi]

In ::CreateFolder
folderName either comes from the string bundle or an int appended to the sting
bundle.
In ImportMailThread lastName is being passed to these methods
This is how lastName is assigned a value. This is existing code
-------
pName = nsnull;
box->GetDisplayName( &pName);
if (pName) {
    lastName = pName; //pName is a wstring
    nsCRT::free( pName);
}
else
   lastName.Assign(NS_LITERAL_STRING("Unknown!"));
----
NS_IMETHOD
GetDisplayName( PRUnichar **pName) { *pName = ToNewUnicode(m_displayName);
return( NS_OK);}
----
I think we dont need to convert any of these.

Comment 25

[nhottanscp]

>I think we dont need to convert any of these.
You are right, they are already Unicode.

Comment 26

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

looks great.  sr=sspitzer

three minor issuse.  (two of these these aren't in your code, but it's in the
code you are touching.  can you see if we can fix these while you are in there?)

1)

+nsISupports GetChildNamed(in wstring name);

use interCaps, so this should be:

+nsISupports getChildNamed(in wstring name);

luckily, there don't appear to be any js calls to this, so you only have to
change the idl.

2)

+
				PRUnichar *pSubName = nsnull;
+
				curProxy->GenerateUniqueSubfolderName( lastName.get(), nsnull, &pSubName);
 
				if (pSubName) {
-
					strName.Assign(pSubName);
+
					lastName.Assign(pSubName);
 
					nsMemory::Free(pSubName);
 
				}

use an nsXPIDLString to simplify this code.

nsXPIDLCString subName
curProxy->GenerateUniqueSubfolderName( lastName.get(), nsnull,
getter_Copies(subName);
if (!subName.IsEmpty()) {
  strName.Assign(pSubName);
  lastName.Assign(pSubName);
}

3)

+        if (exists) {
+          PRUnichar * pName = nsnull;
+          localRootFolder->GenerateUniqueSubfolderName(folderName.get(),
nsnull, &pName);
+          if (pName) {
+            folderName.Assign(pName);
+            nsMemory::Free(pName);
+          }
+          else {

again, use an nsXPIDLString to simplify this code.

Comment 27

[Srilatha Moturi]

Attachment: using xpidlstring

Made changes based on Seth's comments.

Comment 28

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

Comment on attachment 73989 [details] [diff] [review]
using xpidlstring

sr=sspitzer

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 73989 [details] [diff] [review]
using xpidlstring

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

Comment 30

[Srilatha Moturi]

Fix checked in.

Comment 31

[Ninoschka Baca]

Trunk build 2002-03-22: WinMe, Mac 9.1
Verified Fixed. Checked Eudora, Outlook, Outlook Express.

Importing 4.x Folders is being checked in a separate bug.