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

VERIFIED FIXED in mozilla1.0

Status

P2
major
VERIFIED FIXED
18 years ago
14 years ago

People

(Reporter: esther, Assigned: srilatha)

Tracking

Trunk
mozilla1.0
All
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT2])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
adding nsbeta1 because fixing this bug alleviates the need to fix bugs 70644 and
70540 now.
QA Contact: esther → nbaca
(Reporter)

Updated

18 years ago
Keywords: nsbeta1
Whiteboard: nsbeata1+
(Reporter)

Updated

18 years ago
Whiteboard: nsbeata1+ → [nsbeta1+]

Comment 2

18 years ago
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)

Updated

17 years ago
Blocks: 104166

Updated

17 years ago
Keywords: nsbeta1+
Whiteboard: [nsbeta1+]

Updated

17 years ago
Priority: -- → P2

Comment 3

17 years ago
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.
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.  

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0

Comment 5

17 years ago
>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.

Updated

17 years ago
Blocks: 126322
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.
Assignee: sspitzer → srilatha
Status: ASSIGNED → NEW

Comment 7

17 years ago
Yeah, we'd want this for imported 4.x code since we won't be associating it with
an account.
(Assignee)

Comment 8

17 years ago
Created attachment 70758 [details] [diff] [review]
patch v1

Comment 9

17 years ago
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
(Assignee)

Comment 10

17 years ago
Created attachment 70778 [details] [diff] [review]
patch v2

Now using GetRootMsgFolder.
Attachment #70758 - Attachment is obsolete: true

Comment 11

17 years ago
Comment on attachment 70778 [details] [diff] [review]
patch v2

r=naving
Attachment #70778 - Flags: review+
+  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".
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?
Summary: Imported Outlook and Outlook Express mail folders needs to be labeled as Imported to avoid confusion. → imported mail needs to be placed as subfolder to "Local Folders" instead of as new incoming servers (of type "none")
(Assignee)

Comment 14

17 years ago
Created attachment 72817 [details] [diff] [review]
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

17 years ago
>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?
(Assignee)

Comment 16

17 years ago
>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

17 years ago
Showing text on the last screen of the Import Wizard is fine too. :-)
(Assignee)

Comment 18

17 years ago
Tested with Eudora on Mac and windows. I will file a separate bug for the 
message in the Import wizard
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.
Attachment #70778 - Attachment is obsolete: true
I should add that the existing import code doesn't look i18n friendly either.
(Assignee)

Comment 21

17 years ago
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. 

Updated

17 years ago
Whiteboard: [ADT2]
(Assignee)

Comment 22

17 years ago
Created attachment 73779 [details] [diff] [review]
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

17 years ago
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.
(Assignee)

Comment 24

17 years ago
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.
(Assignee)

Updated

17 years ago
Attachment #72817 - Attachment is obsolete: true

Comment 25

17 years ago
>I think we dont need to convert any of these.
You are right, they are already Unicode.
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.
(Assignee)

Comment 27

17 years ago
Created attachment 73989 [details] [diff] [review]
using xpidlstring

Made changes based on Seth's comments.
Attachment #73779 - Attachment is obsolete: true
Comment on attachment 73989 [details] [diff] [review]
using xpidlstring

sr=sspitzer
Attachment #73989 - Flags: superreview+

Comment 29

17 years ago
Comment on attachment 73989 [details] [diff] [review]
using xpidlstring

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73989 - Flags: approval+
(Assignee)

Comment 30

17 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 31

17 years ago
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.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.