359255 - Importers: Import Filters from current Eudora

Status: RESOLVED FIXED

(Thunderbird :: Migration, defect)

Description

[Matt Dudziak]

Penelope needs the ability to to cleanly import the Filters from the current version of Eudora, and to support those filters working exactly as they did in Eudora.

Comment 1

[dg1950]

Yes, indeed! Every single one of my filters ends in "Skip Rest". Otherwise, 99% of the filtered messages would end up in my "Work" mailbox, because that's the last action on the last filter.

Comment 2

[ljbmailprotect-mozilla]

I agree.  Filters is one of the biggest reasons that I use Eudora and other than a Spam system I don't see that the current version of Thunderbird has anything like Eudora.  This is a must have feature.

Comment 3

[kasdfb]

The key feature for me is Eudora's "intersect address book" filter feature.  I allow ONLY those emails into my in-box.  All others go into some other box, so my important emails don't get buried in spam.

Comment 4

[larry]

redundant bug open at https://bugzilla.mozilla.org/show_bug.cgi?id=272952


confirmed to still be a hole on Thunderbird 2.0.0.0.

Comment 6

[Jeff Beckley]

Attachment: patch for Eudora filter import

Here's code that will import filters from Classic Eudora to Thunderbird.  Not all filters can be cleanly moved over since the feature set of filters in the two applications is different.  If that's true for an imported filter a warning is generated.

Comment 7

[Scott MacGregor]

Comment on attachment 283259 [details] [diff] [review]
patch for Eudora filter import

Jeff, any reason this:

+  nsresult rv = NS_OK; 
+
+  nsCOMPtr<nsIImportFilters> importFilters;
+  nsresult rv2 = aImportModule->GetImportInterface(NS_IMPORT_FILTERS_STR, getter_AddRefs(importFilters));
+
+  if (NS_SUCCEEDED(rv2))
+  {

can't just use rv?
i.e.

+  nsCOMPtr<nsIImportFilters> importFilters;
+  nsresult rv = aImportModule->GetImportInterface(NS_IMPORT_FILTERS_STR, getter_AddRefs(importFilters));

Also, any reason not to just migrate filters along with the settings/preferences option instead of having a separate option for filters? Is there a reason why we wouldn't want to migrate filters along with the rest of the mail account settings?

Comment 8

[Jeff Beckley]

(In reply to comment #7)
> can't just use rv?

No because if an importer for another mail app (e.g. Outlook) can't handle filters then we want to migration to continue on with error.  Not having the ability to import filters isn't an error, it just won't move the filters over.


> Also, any reason not to just migrate filters along with the
> settings/preferences option instead of having a separate option for filters? 

I see filters as separate data from settings, just like address books and mail are.  The UI to modify filters and settings are different, and the data itself is stored in different files.  The general path for a new user is to do a migration, in which case all data will get imported (settings, mail, address books, filters), but the Tools->Import interface is more of a one-off type of functionality.

Comment 9

[Jeff Beckley]

Bad sentence above.  Should end "then we want migration to continue on without error."

Comment 10

[Scott MacGregor]

cool, that makes sense Jeff.

Comment 11

[David :Bienvenu]

Wow, very cool.

Looks like there are a lot of tabs in the patch - can you fix those?

	if (!description || !_retval || !location)
+		return( NS_ERROR_NULL_POINTER);
+	
+	*description = nsnull;	
+	*_retval = PR_FALSE;	
+
+	nsresult	rv;
+	m_pLocation =  do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv);
+	NS_ENSURE_SUCCESS(rv, rv);
+
+	*description = nsEudoraStringBundle::GetStringByID( EUDORAIMPORT_NAME);
+
+#if defined(XP_WIN) || defined(XP_OS2)
+	*_retval = nsEudoraWin32::FindFiltersFile( getter_AddRefs(m_pLocation) );
+#endif
+#ifdef XP_MACOSX
+	*_retval = nsEudoraMac::FindFiltersFile( getter_AddRefs(m_pLocation) );
+#endif
+	
+  NS_IF_ADDREF(*location = m_pLocation);
+	return NS_OK;
+}
+
+NS_IMETHODIMP nsEudoraFilters::SetLocation(nsIFile *location)
+{
+	m_pLocation = location;
+
+	return( NS_OK);
+}
+

Some of the filter import error messages are pretty technical, e.g., can't import outgoing filter context, can't import the %S meta-header, etc. I don't know if they're meaningful for Eudora users who might encounter them.

+		case nsMsgFilterAction::MoveToFolder:
+		case nsMsgFilterAction::CopyToFolder:
+			rv = action->SetTargetFolderUri(nsCAutoString(targetFolderUri));
+			NS_ENSURE_SUCCESS(rv, rv);
+			break;

here, maybe we could put one NS_ENSURE_SUCCESS(rv, rv) after the whole switch, instead of in each case:

{
+    boolean  AutoLocate( out wstring description, out nsIFile location);
+
+    void     SetLocation( in nsIFile location);
+
+    /*
+        Import filters and put any problems in the error out parameter.
+    */
+    boolean  Import( out wstring error);
+};

the arg names should be aDescription, aLocation, aError...

There's also some K&R braces style in files where the prevailing braces style is

if (a)
{
}
else
{
}

and I prefer not using braces where they're not required, if both clauses are a single line, e.g.,

if (a)
...
else
...

Comment 12

[Jeff Beckley]

(In reply to comment #11)
> Looks like there are a lot of tabs in the patch - can you fix those?

> the arg names should be aDescription, aLocation, aError...
 
> There's also some K&R braces style in files where the prevailing braces style

> and I prefer not using braces where they're not required, if both clauses are a
> single line, e.g.,

Yeah, much of the style of that code was taken from the existing nsEudoraSettings.cpp file before the cleanup in the move to the trunk.  I'll fix all of that.


> Some of the filter import error messages are pretty technical, e.g., can't
> import outgoing filter context, can't import the %S meta-header, etc. I don't
> know if they're meaningful for Eudora users who might encounter them.

Yeah, I tended to be brief on these warnings because there will be one per line in the final output.  In testing against real-world filters I found that there are a lot of warnings generated (i.e. not a one-to-one match of Eudora's filter capabilities with Thunderbird's).  I'm open to wording suggestions that aren't overly verbose.

 
> here, maybe we could put one NS_ENSURE_SUCCESS(rv, rv) after the whole switch,
> instead of in each case:

Good suggestion.  Will do.

Comment 13

[Scott MacGregor]

Comment on attachment 283259 [details] [diff] [review]
patch for Eudora filter import

Jumping in with a couple other smaller nits to keep Jeff from having to make some round trips:

+    NS_PRECONDITION(aImport != nsnull, "null ptr");
+    if (! aImport)
+        return NS_ERROR_NULL_POINTER;
+
+    *aImport = new nsEudoraFilters();
+    if (! *aImport)
+        return NS_ERROR_OUT_OF_MEMORY;
+
+    NS_ADDREF(*aImport);
+    return NS_OK;

I'd write that as:

NS_ENSURE_ARG_POINTER(aImport);
*aImport = new nsEudoraFilters();
NS_IF_ADDREF(*aImport);
return *aImport ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

I'd replace these:

+    NS_PRECONDITION(description != nsnull, "null ptr");
+    NS_PRECONDITION(_retval != nsnull, "null ptr");
+    NS_PRECONDITION(location != nsnull, "null ptr");
+	if (!description || !_retval || !location)
+		return( NS_ERROR_NULL_POINTER);
with NS_ENSURE_ARG_POINTER macros for each argument.

ditto here:

+	NS_PRECONDITION(error != nsnull, "null ptr");
+	NS_PRECONDITION( _retval != nsnull, "null ptr");

It seems unlikely that the following call would ever fail:

+		m_pLocation =  do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv);
+		if (NS_SUCCEEDED(rv)) {

So we could do NS_ENSURE_SUCCESS(rv, rv) and avoid the nested if clause. I think that pattern shows up a couple times in nsEudoraFilters::Import.

I think the uses of nsCRT::strcmp can  be replaced with strcmp.

In nsEudoraFilters::Init, can NS_ENSURE_SUCCESS be used to reduce the nested if clauses?

Same for AddCustomHeader.

Can you make the interface methods defined in nsIImportFilters start with a lower case letter? i.e. AutoLocate -> autoLocate, etc. That's the IDL standard, although we are inconsistent in following that pattern. We're trying to clean that up. I wonder if it makes sense for the wstring's in the method arguments to be AString's.


all these are just minor pattern suggestions. Our crappy import code is a hard place to be hacking on :)

Rest of the patch looks good to me. Nice stuff Jeff!

Comment 14

[James]

It would also be useful if Eudora could export Filters from itself and then Import them to another copy.  (I tried coping the filters file, but it loose the folder definitions and they all break).

Comment 15

[Jeff Beckley]

Attachment: Improved patch

Here's an improved patch with David and Scott's suggestions.

Comment 16

[Jeff Beckley]

Attachment: Get{Native}Target implementations for OS X

Forgot this piece.  It implements the Get{Native}Target functions under Mac OS X.  While not necessary for the filter import in general, it will help to find the filters file if Mac aliases are being used.

Comment 17

[David :Bienvenu]

Comment on attachment 284387 [details] [diff] [review]
Get{Native}Target implementations for OS X

Not sure who can review this, but it's not me (I'm not a module owner for xpcom). Trying Josh, to see if he can review it, or if not, who should review it.

Comment 18

[Josh Aas]

Comment on attachment 284387 [details] [diff] [review]
Get{Native}Target implementations for OS X

Pointing review at Mark Mentovai, I'm too overloaded right now with 1.9 beta stuff to get to this any time soon. He's probably a better reviewer for this than I am anyway.

Comment 19

[Mark Mentovai]

Comment on attachment 284387 [details] [diff] [review]
Get{Native}Target implementations for OS X

I'd add a blank line after the two early returns in this patch.  Looks good.

Comment 20

[David :Bienvenu]

Comment on attachment 284320 [details] [diff] [review]
Improved patch

This is an impressive patch, in size and quality. So now I'm really getting nitty, for the most part :-)

+  rv = SaveFilters();
+
+  return NS_SUCCEEDED(rv)? PR_TRUE : PR_FALSE;


the last line can just be return NS_SUCCEEDED(rv);

+        if (NS_SUCCEEDED(rv) && serverType.IsEmpty() == PR_FALSE)

last clause can just be !serverType.IsEmpty());

+  for (PRUint32 filterIndex=0; filterIndex < numFilters; filterIndex++)
+  {
+    nsCOMPtr<nsIMsgFilter> filter = do_QueryElementAt(m_pFilterArray, filterIndex, &rv);

can you put spaces around the '=', i.e., filterIndex = 0 ?

+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  if (NS_SUCCEEDED(rv))
+    m_addedAction = PR_TRUE;


Here, correct me if I'm wrong, but I don't think we need the if (NS_SUCCEEDED(rv)) line because we'll never get here unless we've succeeded earlier.

+  if (!*ppFolder)
+  {
+    *ppFolder = m_pMailboxesRoot;
+    NS_ADDREF(*ppFolder);
+  }

this can just be

if (!*ppFolder)
  NS_ADDREF(*ppFolder = m_pMailboxesRoot);


+      NS_RELEASE(*ppFolder);
+      rv = subFolder->QueryInterface(NS_GET_IID(nsIMsgFolder), (void **) ppFolder);
+      if (NS_SUCCEEDED(rv))
+      {
+        NS_ADDREF(*ppFolder);
+

QueryInterface already addrefs subFolder, so I don't see why you need to AddRef again.

I think a lot of the rest of the patch is whitespace cleanup, so a new -uw patch with the above issues addressed would be easier for me to review.

Comment 21

[Jeff Beckley]

Attachment: Latest patch (-w version coming afterwards)

Patch implementing David's latest suggestions.  A non-whitespace diff version of this will be attached next.

Comment 22

[Jeff Beckley]

Attachment: -w version of previous patch

Non-whitespace diff version of attachment 284570 [details] [diff] [review].

Comment 23

[Scott MacGregor]

Comment on attachment 284387 [details] [diff] [review]
Get{Native}Target implementations for OS X

this part looks good.

Comment 24

[David :Bienvenu]

Comment on attachment 284570 [details] [diff] [review]
Latest patch (-w version coming afterwards)

+  if (filtersInterface != null)
+    filtersInterface = filtersInterface.QueryInterface(Components.interfaces.nsIImportFilters);
+  if (filtersInterface == null) {
+    error.data = gImportMsgsBundle.getString('ImportFiltersBadModule');

we tend not to put the == null, or != null, and just use (!term) or (term)

There's got to be a stringapi way of doing this:
+    if (strnicmp(path.BeginReading(), pEudoraFilePath, pathLength) == 0 && path.Length() == pathLength)

perhaps:
StringBeginsWith(path, nsDependentCSubstringpEudoraFilePath, 0, pathLength), nsCaseInsensitiveCStringComparator())

hmm, neither are charming. Perhaps Scott has an idea...

Other than those minor things, this looks good to me.

Comment 25

[Scott MacGregor]

Comment on attachment 284570 [details] [diff] [review]
Latest patch (-w version coming afterwards)

Looks good Jeff. I don't have anything else to add after David's last set of comments.

Comment 26

[Jeff Beckley]

Attachment: New patch with David's nits

Here's a new patch with David's suggestion about null comparison.

Continuing r=bienvenu and sr=mscott.

Comment 27

[Damon Sicore (:damons)]

Comment on attachment 284387 [details] [diff] [review]
Get{Native}Target implementations for OS X

a1.9+=damons

Comment 28

[Reed Loden [:reed]]

Checking in xpcom/io/nsLocalFileOSX.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileOSX.cpp,v  <--  nsLocalFileOSX.cpp
new revision: 1.59; previous revision: 1.58
done

The huge patch is bitrotted in several places now. Can you attach a new patch and re-add the checkin-needed keyword? Thanks!

Comment 29

[Jeff Beckley]

Attachment: New up-to-date patch

New patch without the bit-rot.

Comment 30

[Reed Loden [:reed]]

Checking in mail/components/migration/content/migration.xul;
/cvsroot/mozilla/mail/components/migration/content/migration.xul,v  <--  migration.xul
new revision: 1.6; previous revision: 1.5
done
Checking in mail/components/migration/public/nsIMailProfileMigrator.idl;
/cvsroot/mozilla/mail/components/migration/public/nsIMailProfileMigrator.idl,v  <--  nsIMailProfileMigrator.idl
new revision: 1.3; previous revision: 1.2
done
Checking in mail/components/migration/src/nsEudoraProfileMigrator.cpp;
/cvsroot/mozilla/mail/components/migration/src/nsEudoraProfileMigrator.cpp,v  <--  nsEudoraProfileMigrator.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in mail/components/migration/src/nsProfileMigratorBase.cpp;
/cvsroot/mozilla/mail/components/migration/src/nsProfileMigratorBase.cpp,v  <--  nsProfileMigratorBase.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in mail/components/migration/src/nsProfileMigratorBase.h;
/cvsroot/mozilla/mail/components/migration/src/nsProfileMigratorBase.h,v  <--  nsProfileMigratorBase.h
new revision: 1.3; previous revision: 1.2
done
Checking in mail/locales/en-US/chrome/messenger/eudoraImportMsgs.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/eudoraImportMsgs.properties,v  <--  eudoraImportMsgs.properties
new revision: 1.6; previous revision: 1.5
done
Checking in mail/locales/en-US/chrome/messenger/importDialog.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/importDialog.dtd,v  <--  importDialog.dtd
new revision: 1.9; previous revision: 1.8
done
Checking in mail/locales/en-US/chrome/messenger/importMsgs.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/importMsgs.properties,v  <--  importMsgs.properties
new revision: 1.6; previous revision: 1.5
done
Checking in mail/locales/en-US/chrome/messenger/migration/migration.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/migration/migration.dtd,v  <--  migration.dtd
new revision: 1.5; previous revision: 1.4
done
Checking in mailnews/import/eudora/src/Makefile.in;
/cvsroot/mozilla/mailnews/import/eudora/src/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
RCS file: /cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraFilters.cpp,v
done
Checking in mailnews/import/eudora/src/nsEudoraFilters.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraFilters.cpp,v  <--  nsEudoraFilters.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraFilters.h,v
done
Checking in mailnews/import/eudora/src/nsEudoraFilters.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraFilters.h,v  <--  nsEudoraFilters.h
initial revision: 1.1
done
Checking in mailnews/import/eudora/src/nsEudoraImport.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraImport.cpp,v  <--  nsEudoraImport.cpp
new revision: 1.49; previous revision: 1.48
done
Checking in mailnews/import/eudora/src/nsEudoraImport.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraImport.h,v  <--  nsEudoraImport.h
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/import/eudora/src/nsEudoraMac.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraMac.cpp,v  <--  nsEudoraMac.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in mailnews/import/eudora/src/nsEudoraMac.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraMac.h,v  <--  nsEudoraMac.h
new revision: 1.9; previous revision: 1.8
done
Checking in mailnews/import/eudora/src/nsEudoraStringBundle.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraStringBundle.cpp,v  <--  nsEudoraStringBundle.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in mailnews/import/eudora/src/nsEudoraStringBundle.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraStringBundle.h,v  <--  nsEudoraStringBundle.h
new revision: 1.9; previous revision: 1.8
done
Checking in mailnews/import/eudora/src/nsEudoraWin32.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraWin32.cpp,v  <--  nsEudoraWin32.cpp
new revision: 1.62; previous revision: 1.61
done
Checking in mailnews/import/eudora/src/nsEudoraWin32.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraWin32.h,v  <--  nsEudoraWin32.h
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/import/public/Makefile.in;
/cvsroot/mozilla/mailnews/import/public/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/mailnews/import/public/nsIImportFilters.idl,v
done
Checking in mailnews/import/public/nsIImportFilters.idl;
/cvsroot/mozilla/mailnews/import/public/nsIImportFilters.idl,v  <--  nsIImportFilters.idl
initial revision: 1.1
done
Checking in mailnews/import/public/nsIImportModule.idl;
/cvsroot/mozilla/mailnews/import/public/nsIImportModule.idl,v  <--  nsIImportModule.idl
new revision: 1.7; previous revision: 1.6
done
Checking in mailnews/import/resources/content/import-test.html;
/cvsroot/mozilla/mailnews/import/resources/content/import-test.html,v  <--  import-test.html
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/import/resources/content/importDialog.js;
/cvsroot/mozilla/mailnews/import/resources/content/importDialog.js,v  <--  importDialog.js
new revision: 1.45; previous revision: 1.44
done
Checking in mailnews/import/resources/content/importDialog.xul;
/cvsroot/mozilla/mailnews/import/resources/content/importDialog.xul,v  <--  importDialog.xul
new revision: 1.29; previous revision: 1.28
done

Comment 31

[Håkan Waara]

I'm sorry if I'm late to this bug. I'm working on an Apple Mail.app importer, so I'm interested in this space. (see bug 420472).

Just wondering, why introduce Filters as its own importing type in the import/migration UI?  Why would anyone ever want to import filters but not the other settings? I think it would make more sense to merge into "Settings" in the UI, if nothing else than to avoid extra UI (of which mailnews has *plenty*).

Comment 32

[Jeff Beckley]

(In reply to comment #31)
> Just wondering, why introduce Filters as its own importing type in the
> import/migration UI?

I addressed this in comment #8.

Comment 33

[Rimas Kudelis [use needinfo to ping]]

Hm... Those new error messages in chrome/messenger/eudoraImportMsgs.properties are really hard to translate. Any suggestions/explanations? I've translated them word for word now, but I doubt they're understandable at that state. :(

Comment 34

[Jeff Beckley]

Are you referring to "action", "verb", and "meta-header"?  Those are parts of a filter.  "action" is what is done if the filter matches, and there can be multiple  actions for a filter.  "verb" is the how to match the header to the text, e.g. "contains", "is", "begins with".  "meta-header" is a Eudora filterism for things it could use as a shorthand to match more than one header, e.g. "Any Header" and "Any Recipient".  It also was used for non-header matching, like "Body" and "Junk Score".

Comment 35

[Rimas Kudelis [use needinfo to ping]]

Thank you, now it makes much more sense than it used to. How about "outgoing filter context"? Oh, and wouldn't "pseudo-header" seem more correct than "meta-header"?

BTW, Axel (the l10n coordinator) seems very dissatisfied with the l10n-related parts of this patch...

Comment 36

[Jeff Beckley]

(In reply to comment #35)
> How about "outgoing filter context"?

Classic Eudora supports specifying that filters can be run on messages as they are being sent out.  Thunderbird does not.


> Oh, and wouldn't "pseudo-header" seem more correct than "meta-header"?

Sure, that's a fine term.

Comment 37

[Axel Hecht [:Pike]]

Changes to the meaning of strings should change the name of the string. I.e., importDesc.label need to be changed, but don't change the .label.

The localization notes shouldn't focus on translating %S, that thing is very common.

Something like "%S will be replaced with foo" is more more to the point. And something like comment 34 would be well suited to be in the localized file as a comment, too.

Reopening to get these fixed.

Comment 38

[Jeff Beckley]

Attachment: l10n improvements

Here's an additional patch to address the localization issues that Axel and Rimas brought up.

Comment 39

[Rimas Kudelis [use needinfo to ping]]

From my POV, localization notes are just perfect now. Great job! :)

A few ideas:
* Perhaps importDescription.label could be merged with importDescription2.label to form a new importDescription1.label?
* I think one comment would be more than enough in importMsgs.properties. It could be like this: 
    # LOCALIZATION NOTE : In the following three lines, the %S will be replaced by the name of the import module.
  Oh, and once we're at this, I think the comment above should use a word "strings" instead of "string". This also applies to the paragraphs above, but these aren't our concern today, are they? ;)
* I'm not sure Axel will like the fact that in eudoraImportMsgs.properties, entities 2022 and 2024 have changed their meaning, but retained theid IDs. But that's for him to comment.

Comment 40

[Axel Hecht [:Pike]]

Comment on attachment 309207 [details] [diff] [review]
l10n improvements

Let's keep the notes per entry, it's what the file does, and some tools like to show only single entries, and don't grok "the next three".

As for 2022 and 2024, yeah, those should probably change. With that, r=me.

Comment 41

[Jeff Beckley]

Attachment: More l10n improvements

Here's a new version of the l10n patch which changes the resource IDs of the strings that changed.

Continuing r=Pike

Comment 42

[Reed Loden [:reed]]

Checking in mail/components/migration/content/migration.xul;
/cvsroot/mozilla/mail/components/migration/content/migration.xul,v  <--  migration.xul
new revision: 1.7; previous revision: 1.6
done
Checking in mail/locales/en-US/chrome/messenger/eudoraImportMsgs.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/eudoraImportMsgs.properties,v  <--  eudoraImportMsgs.properties
new revision: 1.7; previous revision: 1.6
done
Checking in mail/locales/en-US/chrome/messenger/importDialog.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/importDialog.dtd,v  <--  importDialog.dtd
new revision: 1.10; previous revision: 1.9
done
Checking in mail/locales/en-US/chrome/messenger/importMsgs.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/importMsgs.properties,v  <--  importMsgs.properties
new revision: 1.7; previous revision: 1.6
done
Checking in mail/locales/en-US/chrome/messenger/migration/migration.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/migration/migration.dtd,v  <--  migration.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in mailnews/import/eudora/src/nsEudoraStringBundle.h;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraStringBundle.h,v  <--  nsEudoraStringBundle.h
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/import/resources/content/importDialog.xul;
/cvsroot/mozilla/mailnews/import/resources/content/importDialog.xul,v  <--  importDialog.xul
new revision: 1.30; previous revision: 1.29
done

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 310377 [details] [diff] [review]
More l10n improvements

As bug 399312 previously did, this patch broke SeaMonkey (further).
Fixes should come from bug 421405...

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 308382 [details] [diff] [review]
New up-to-date patch

This patch already did broke SeaMonkey (further) too...

Comment 45

[Rimas Kudelis [use needinfo to ping]]

Reopening per IRC discussion with Jeff

The fact that Thunderbird imports filters is not reflected in Macs, so we either add importFromMac.label to migration.dtd, or change importFromUnix.label to mention filters too (even though there are no filter importers for Linux).

Also (from the same discussion), string 2025 in eudoraImportMsgs.properties is actually talking about a folder, so perhaps it should use that word instead of the word "mailbox" (it would become 2029 then).

Comment 46

[Jeff Beckley]

Attachment: Update migration label under non-Win OSes to mention filters

This small patch adds to the migration label that non-Windows OSes can import filters as well.

I didn't change the word in the one string from "mailbox" to "folder" as in looking through the rest of the strings it seems that "mailbox" is the term that's used.

Comment 47

[Rimas Kudelis [use needinfo to ping]]

I guess the entity should be named importFromUnix1.label instead of importFromNonWin.label.

Comment 48

[Jeff Beckley]

(In reply to comment #47)
> I guess the entity should be named importFromUnix1.label instead of
> importFromNonWin.label.

The way it gets used in the code, that entity gets used on all non-Windows OSes:

#ifdef XP_WIN
    <description>&importFromWin.label;</description>
#else
    <description>&importFromNonWin.label;</description>
#endif

It's just a coincidence that the non-Win platforms also happen to be Unix.

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 311080 [details] [diff] [review]
Update migration label under non-Win OSes to mention filters

>RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/migration/migration.dtd,v
> 
> <!ENTITY importFromWin.label            "Import Options, Account Settings, Address Book, Filters and other data from:">

Do we actually want to have Options/Win and Preferences/NonWin ?

>-<!ENTITY importFromUnix.label           "Import Preferences, Account Settings, Address Book and other data from:">
>+<!ENTITY importFromNonWin.label         "Import Preferences, Account Settings, Address Book, Filters, and other data from:">

Comment 50

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #49)
> Do we actually want to have Options/Win and Preferences/NonWin ?

This is the way these words are used all around.

Comment 51

[Serge Gautherie (:sgautherie)]

Comment on attachment 310377 [details] [diff] [review]
More l10n improvements

>RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/importDialog.dtd,v
>retrieving revision 1.9
> <!ENTITY importDialog.settingsTitle "Settings">
> 
>-<!ENTITY importDescription.label  "This wizard will import mail messages, address book entries, preferences, and/or filters from other mail programs and common address book formats into &brandShortName;.">

Other nit:
Description reads |preferences|, radio box reads |Settings| :-/

Comment 52

[Jeff Beckley]

Yes, there are some inconsistencies with Settings/Options/Preferences, and those need to be fixed.  But that is straying from this bug, which has already been reopened several times.  I propose that a new bug be created for the Settings et al issue, and this bug can get closed after review of the last patch.

Comment 53

[Axel Hecht [:Pike]]

Comment on attachment 311080 [details] [diff] [review]
Update migration label under non-Win OSes to mention filters

r=me

Comment 54

[Axel Hecht [:Pike]]

Comment on attachment 311080 [details] [diff] [review]
Update migration label under non-Win OSes to mention filters

r=me

Comment 55

[Serge Gautherie (:sgautherie)]

Other nits (+/- related to this very bug:
In bug 421405 comment 11, Neil suggested to change line 50 to include "filter"...
Looking at that file, I noticed that line 38 probably needs a s/outlook express/eudora/.

Comment 56

[Serge Gautherie (:sgautherie)]

Ah, forgot the URL:
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/locales/en-US/chrome/mailnews/eudoraImportMsgs.properties&rev=1.6&mark=38,50#35>

Comment 57

[Reed Loden [:reed]]

Checking in mail/components/migration/content/migration.xul;
/cvsroot/mozilla/mail/components/migration/content/migration.xul,v  <--  migration.xul
new revision: 1.8; previous revision: 1.7
done
Checking in mail/locales/en-US/chrome/messenger/migration/migration.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/migration/migration.dtd,v  <--  migration.dtd
new revision: 1.7; previous revision: 1.6
done