420472 - Implement Mail.app importer in mailnews core

Status: RESOLVED FIXED

(MailNews Core :: Import, enhancement)

Description

[Håkan Waara]

This bug is about implement support for Mail.app mail importing in mailnews core.

Hopefully this can be used by all mailnews core users such as Thunderbird, Eudora, Correo.

Comment 1

[Håkan Waara]

Attachment: WIP patch 1

This is a milestone WIP patch. It actually works to import all your (account-global) mailboxes from Mail with it! 

It doesn't handle accounts yet (or their mailboxes). It's also probably buggy, etc... Still work in progress.

Comment 2

[Håkan Waara]

Attachment: WIP patch v2

More WIP. I have some issues I'd like comments/ideas on to be able to move this forward. Please see below.

Open issues:

* Should I import accounts' inboxes/sent messages/drafts (etc) folders also when you choose to import "mail"? 

I'm thinking maybe choosing to import "mail" would only grab global mailboxes, and if you choose "everything" you'd get accounts (with their mailboxes) + global mailbxoes. Does anyone know what other Thunderbird mail importers do here?

* I'll try to roll in implementation of account migration in this patch as well, if it isn't too much work. 

* I get lots of nasty exceptions and thread-safety assertions. I wonder if that's only for me or for all mail importers. Anyone wanna test an OE import, or Netscape mail import and see if they get spew in the console?

* Right now the importer creates some weird folders, like for example Mail's ToDo folder and a mysterious folder called "Mail Mail".

Comment 3

[Håkan Waara]

Attachment: WIP patch v3

This is starting to look pretty good.

Changes:

* I've made it import *all* mailboxes (including your accounts' mailboxes), since this is how other seem to behave. The mail accounts' mailboxes are included in the dir structure like this:

- My global mailbox 1
- Mails from John
- [e-mail]
 -- INBOX
 -- Junk Mail
etc.

* This is actually getting good enough for review I think. I will have another look at squashing the bug that creates a "Mail Mail" folder, and other small things, and then see if I can convince someone to review it.

Comment 4

[Håkan Waara]

Attachment: First patch for review v1

I fixed the last bug about the imported folders not going into the "Apple Mail import" folder! Now this works fine, and I think it's time to get the review ball rolling...

So, to summarize:

* This implements importing of mailboxes from Mail.app
* Migration of flags / unread/read markers, etc is not yet implemented
* Nor is settings import or anything like that, but since that's another option in the import wizard, I think we can safely split that work from this bug

I might start work on migration of flags and other message bits, but meanwhile you can start the review, and at that point (should it come) use interdiff, because it's not gonna be a major change, I think.

If anyone feels like testing, feel free to. Comments / bug reports appreciated.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 311203 [details] [diff] [review]
First patch for review v1

Looks like I'm only going to get to do this in bits this week. So here's the first stage of comments.

Firstly, any chance you could do the l10n changes in suite/locales as well please? It should only take a couple of mins and it'll save us breaking SM. If not, please ensure Mnyromyr is cc'ed on this bug.

 # LOCALIZATION NOTE: Do not translate the word "%S" below.
-ModuleFolderName=%S Mail
+ModuleFolderName=%S Import

I think you need to change the name of the string here to ensure l10n pick it up.

+MODULE	= appleMail
+MODULE		= impAppleMail

all lowercase please.

+MOZILLA_INTERNAL_API = 1

not a requirement, but it'd be useful if mailnews/import/applemail/src/ compiles without this defined.

+CPPSRCS		= 	\
+			nsAppleMailImport.cpp			\
+			$(NULL)

you don't need to have as many tabs as this.

CPPSRCS = \
{tab}{tab}nsAppleMailImport.cpp \
{tab}{tab}$NULL

should do it (or something like that).

+include $(topsrcdir)/config/rules.mk
+
+

No need for the extra new lines at the end of files (several places).

-LOCAL_INCLUDES += -I$(srcdir)/../eudora/src
+LOCAL_INCLUDES += -I$(srcdir)/../eudora/src \
+                  -I$(srcdir)/../applemail/src

you should add a $(NULL) line when expanding it like this (several places)

-/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+/* -*- Mode: Javascript; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

nit: Whilst you're here, please make this tab-width: 2 and c-basic-offset: 2.

That's most of the build/structure items sorted. Code next time ;-)

Comment 6

[Håkan Waara]

Attachment: Patch v2 for review

Here's a patch of the mail importer, which is now 100% complete when it comes to mail importing.

Changes:

* The importer now migrates metadata such as whether a mail is read/unread/flagged, has been replied to etc.

* Fixed a bunch of bugs which boiled down to properly escaping the message data in a way such that it works in the mboxrd format (basically escaping "From " lines).

* Standard8's Makefile and whitespace nits addressed

* The component now compiles with MOZILLA_INTERNAL_API unset, however, it won't *link* to the rest of the import/ module if I unset it (because all other code is still using MOZILLA_INTERNAL_API). I think this is normal?

This stuff is good enough to be used now, so I'll await your review, and then spin off new bugs for importing of settings etc.

Comment 7

[Håkan Waara]

(In reply to comment #5)
> Firstly, any chance you could do the l10n changes in suite/locales as well
> please? It should only take a couple of mins and it'll save us breaking SM. If
> not, please ensure Mnyromyr is cc'ed on this bug.

I don't have a suite/ tree, so I would be thankful if someone (maybe Stefan, or Mnyromyr?) could help me with this. 

The changes are: 1 new .properties file and minor wording changes to the main import .properties file.

Comment 8

[Stefan [:stefanh]]

Attachment: add suite/locales

Here's the suite/locales part.

Comment 9

[Håkan Waara]

Thanks Stefan!

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 312490 [details] [diff] [review]
Patch v2 for review

>+nsAppleMailImportModule::nsAppleMailImportModule()
...
>+  nsCOMPtr <nsIStringBundleService> pBundleService;
>+  nsresult rv;
>+
>+  m_pBundle = nsnull;
>+
>+  pBundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && (pBundleService))
>+    pBundleService->CreateBundle(APPLEMAIL_MSGS_URL, getter_AddRefs(m_pBundle));
>+}

You don't need to initialise m_pBundle to null (nsCOMPtr will do that for you). Additionally, please do the do_GetService on the construction line of pBundleService.

>+NS_IMETHODIMP nsAppleMailImportModule::GetName(PRUnichar **name)
>+{
>+  NS_ENSURE_ARG_POINTER(name);
>+  nsresult rv = NS_ERROR_FAILURE;
>+  if (m_pBundle)
>+    rv = m_pBundle->GetStringFromID(APPLEMAILIMPORT_NAME, name);
>+  return rv;
>+}

I think I'd prefer the last 4 lines to be made into:

return m_pBundle ? m_pBundle->GetStringFromID(APPLEMAILIMPORT_NAME, name) : NS_ERROR_FAILURE;

with some appropriate wrapping. Same for GetDescription as well.

>+NS_IMETHODIMP nsAppleMailImportModule::GetImportInterface(const char *pImportType, nsISupports **ppInterface)
...
>+  nsresult    rv;

nit: No need for extra spaces between nsresult and rv;

>+  if (!strcmp(pImportType, "mail")) {
>+    nsCOMPtr <nsIImportMail> pMail = do_CreateInstance(NS_APPLEMAILIMPL_CONTRACTID, &rv);

nit: no space between nsCOMPtr and < please (various places throughout the patch).

>+    if (NS_SUCCEEDED(rv)) {
>+      nsCOMPtr <nsIImportGeneric> pGeneric;
>+      nsCOMPtr<nsIImportService> impSvc(do_GetService(NS_IMPORTSERVICE_CONTRACTID, &rv));
>+      if (NS_SUCCEEDED(rv)) {
>+        rv = impSvc->CreateNewGenericMail(getter_AddRefs(pGeneric));

You don't need to create pGeneric until after the second if statement.

>+nsAppleMailImportMail::nsAppleMailImportMail() 
>+: mBundleProxy(nsnull),
>+  mProgress(0),
>+  mCurDepth(0)

As I said before, nsCOMPtrs do their own initialisation.

>+nsresult nsAppleMailImportMail::Initialize()
>+{
>+  nsCOMPtr <nsIStringBundleService> pBundleService;
>+  nsresult rv;
>+  nsCOMPtr <nsIStringBundle>  pBundle;
>+
>+  pBundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);

Ditto from above with pBundleService.


>+NS_IMETHODIMP nsAppleMailImportMail::GetDefaultLocation(nsIFile **aLocation, PRBool *found, PRBool *userVerify)
>+{
>+  NS_ENSURE_ARG_POINTER(found);
>+  NS_ENSURE_ARG_POINTER(aLocation);
>+  NS_ENSURE_ARG_POINTER(userVerify);

Lets name parameters consistently. I'd prefer the aLocation style everywhere, so aFound and aUserVerify;

>+    nsresult rv = mailFolder->InitWithNativePath(NS_LITERAL_CSTRING("~/Library/Mail/"));

I think it might be useful to have constants/#define/named strings defined for these (and the others) at the start of the file.

>+// this is the method that initiates all searching for mailboxes.
>+// it will assume that it has a directory like ~/Library/Mail/
>+NS_IMETHODIMP nsAppleMailImportMail::FindMailboxes(nsIFile *aMailboxFile, nsISupportsArray **ppArray)

nit: aArray, or even better aResult.

>+  nsCOMPtr<nsIImportService> importService = do_GetService(NS_IMPORTSERVICE_CONTRACTID, &rv);

nit: please use form:

nsCOMPtr<nsIImportService> importService(do_GetService(NS_IMPORTSERVICE_CONTRACTID, &rv));

several other places as well.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  NS_NewISupportsArray(ppArray);
>+  if (!ppArray)
>+    return NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY

>+    
>+  mCurDepth = 0;
>+  
>+  // 1. look for accounts with mailboxes
>+  mCurDepth++;

Why not just initialise mCurDepth to 1?

>+  // 2. look for "global" mailboxes, that don't belong to any specific account. they are inside the
>+  //    root's Mailboxes/ folder
>+  nsCOMPtr<nsILocalFile> mailboxesDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>+  nsCOMPtr<nsILocalFile> loc = do_QueryInterface(aMailboxFile);

You've missed checking that the QI on aMailboxFile worked.

>+  if (NS_FAILED(rv))
>+    goto bail;

No. What you should be doing, is to create a temporary nsCOMPtr for the nsISupportsArray result and using that within this function, you can then abort at any time, and nsCOMPtr will clean up the memory for you.

At the end of the function you can do mytempcomptr.swap(*aResult); to pass the value back to the caller (assuming you renamed it as I suggested above.

>+void nsAppleMailImportMail::FindAccountMailDirs(nsIFile *aRoot, nsISupportsArray *aMailboxDescs, nsIImportService *aImportService)
...
>+    
>+    // get the next file entry
>+    nsCOMPtr<nsILocalFile> currentEntry;
>+    {
>+      nsCOMPtr<nsISupports> rawSupports;
>+      directoryEnumerator->GetNext(getter_AddRefs(rawSupports));
>+      if (!rawSupports)
>+        continue;
>+      currentEntry = do_QueryInterface(rawSupports);
>+      if (!currentEntry)
>+        continue;
>+    }

No need for the extra block indentation (various places)

>+      if (isAccountFolder) {
...
>+        nsCOMPtr<nsILocalFile> mailboxDescFile;
>+        rv = desc->GetFile(getter_AddRefs(mailboxDescFile));
>+        if (!mailboxDescFile)
>+          return;

I'm not sure you want to return here, what about other potential accounts?

>+        mailboxDescFile->InitWithFile(currentEntry);
>+
>+        // add this mailbox descriptor to the list
>+        nsCOMPtr<nsISupports> pInterface = do_QueryInterface(desc);
>+        if (pInterface)
>+          aMailboxDescs->AppendElement(pInterface);

You shouldn't need to QI here, just append desc directly (several places).

>+// adds the specified file as a mailboxdescriptor to the array
>+nsresult nsAppleMailImportMail::AddMboxDir(nsILocalFile *aFolder, nsISupportsArray *aMailboxDescs, nsIImportService *aImportService)
>+{
>+  nsAutoString folderName;
>+  aFolder->GetLeafName(folderName);
>+  

nit: There's unnecessary blank space on the last "+" line here, ditto in other places, and at the end of some lines, please fix.

>+NS_IMETHODIMP nsAppleMailImportMail::ImportMailbox(nsIImportMailboxDescriptor *aMailbox, nsIFile *aDestination, 
>+                                                   PRUnichar **aErrorLog, PRUnichar **aSuccessLog, PRBool *aFatalError)
>+{
>+  nsString success, error;
...
>+  // check for bad args
>+  if (!aMailbox || !aDestination || !aFatalError) {
>+    nsString errorString;
>+    mBundleProxy->GetStringFromID(APPLEMAILIMPORT_MAILBOX_BADPARAM, getter_Copies(errorString));
>+    error = errorString;

I don't see the need for the errorString temporary value here.

>+  nsString mailboxName;
>+  PRUnichar *pName;
>+  if (NS_SUCCEEDED(aMailbox->GetDisplayName(&pName))) {
>+      mailboxName.Adopt(pName);
>+  }

Drop pName, and use aMailbox->GetDisplayName(getter_Copies(mailboxName));

>+  nsCOMPtr<nsILocalFile> mboxFolder;
>+  nsresult rv = aMailbox->GetFile(getter_AddRefs(mboxFolder));
>+  if (NS_FAILED(rv) || ! mboxFolder) {

nit: No space between ! and mboxFolder.

>+  if (mailboxIdentifier != kAccountMailboxID) {
>+    // move to the .mbox's Messages folder
>+    nsCOMPtr<nsIFile> messagesFolder;
>+    mboxFolder->Clone(getter_AddRefs(messagesFolder));
>+    rv = messagesFolder->Append(NS_LITERAL_STRING("Messages"));
>+    if (NS_FAILED(rv)) {
>+      // even if there are no messages, it might still be a valid mailbox, or even
>+      // a parent for other mailboxes.
>+      goto done;

No again, goto is not allowed (in my book at least). I would suggest wrapping the following bits in a block and checking NS_SUCCEEDED for entry into that block.

>+void nsAppleMailImportMail::ReportStatus(PRInt32 errorNum, nsString &name, nsString *pStream)
>+{
>+  if (!pStream)
>+    return;
>+    
>+  nsString statusStr;
>+  const PRUnichar * fmtStr = name.get();
>+  nsresult rv = mBundleProxy->FormatStringFromID(errorNum, &fmtStr, 1, getter_Copies(statusStr));

Should be:

const PRUnichar *params[] = { name.get() };

then you don't need to pass the address of fmtStr.

>+  if (NS_SUCCEEDED(rv)) {
>+    pStream->Append(statusStr.get());

You shouldn't need the .get(), they are both nsString.

>+    pStream->Append(PRUnichar('\n'));

Use AppendLiteral('\n');

>+NS_IMETHODIMP nsAppleMailImportMail::TranslateFolderName(const nsAString & aFolderName, nsAString &_retval)

nit: no need for space between & and aFolderName.

>Index: mailnews/import/applemail/src/nsEmlxHelperUtils.mm

I'd like someone with a bit more mac experience to go through this file as well please (bienvenu?)

>+nsresult nsEmlxHelperUtils::ConvertToMboxRD(const char *aMessageBufferStart, const char *aMessageBufferEnd, nsCString **aOutBuffer)

aOutBuffer should be an nsCString & (passed by reference).

>+      buf->Append(NS_LITERAL_CSTRING(">"));

AppendLiteral

>+  // write the bogus "From " line which is a magic separator in the mbox format
>+  rv = aOut->Write(kBogusFromLine.get(), kBogusFromLine.Length(), &actualBytesWritten);
>+  if (NS_FAILED(rv))
>+    goto bail;

Again, I don't like the goto. In this case, you could wrap each of the following ifs in if (NS_SUCCEEDED(rv) && ...)

>+    char x_mozilla_status_header[50];
>+    PR_snprintf(x_mozilla_status_header, sizeof(x_mozilla_status_header), X_MOZILLA_STATUS_FORMAT MSG_LINEBREAK, x_mozilla_flags);

Is 50 guaranteed to be enough?

Comment 11

[Håkan Waara]

Thanks for the review, Mark. 

I've addressed all of your comments except for those below. As part of this, I went through all of the code and made lots of other general cleanup.

> >+    nsresult rv = mailFolder->InitWithNativePath(NS_LITERAL_CSTRING("~/Library/Mail/"));
> 
> I think it might be useful to have constants/#define/named strings defined for
> these (and the others) at the start of the file.

I've put some of the strings as #defines, where it makes things easier to understand (or for those I use multiple times).

With some strings (e.g., when I'm appending "Messages" to a path, to go into that subfolder) I find that the code doesn't become easier to read if you have to go to the head of the file to look at what the string is, so I'll keep such cases in place.
 
> >+void nsAppleMailImportMail::FindAccountMailDirs(nsIFile *aRoot, nsISupportsArray *aMailboxDescs, nsIImportService *aImportService)
> ...
> >+    
> >+    // get the next file entry
> >+    nsCOMPtr<nsILocalFile> currentEntry;
> >+    {
> >+      nsCOMPtr<nsISupports> rawSupports;
> >+      directoryEnumerator->GetNext(getter_AddRefs(rawSupports));
> >+      if (!rawSupports)
> >+        continue;
> >+      currentEntry = do_QueryInterface(rawSupports);
> >+      if (!currentEntry)
> >+        continue;
> >+    }
> 
> No need for the extra block indentation (various places)

I don't want to spew out those temporaries to the rest of the method, so that's why I wrap it inside a block.   Note that this block's purpose is just to get |currentEntry| - so I'm happy if the rawSupports can go away early.

> 
> >+// adds the specified file as a mailboxdescriptor to the array
> >+nsresult nsAppleMailImportMail::AddMboxDir(nsILocalFile *aFolder, nsISupportsArray *aMailboxDescs, nsIImportService *aImportService)
> >+{
> >+  nsAutoString folderName;
> >+  aFolder->GetLeafName(folderName);
> >+  
> 
> nit: There's unnecessary blank space on the last "+" line here, ditto in other
> places, and at the end of some lines, please fix.

I've deliberately put blank lines between the actions in this method, to let it breathe, and to make it more readable.

> 
> >+nsresult nsEmlxHelperUtils::ConvertToMboxRD(const char *aMessageBufferStart, const char *aMessageBufferEnd, nsCString **aOutBuffer)
> 
> aOutBuffer should be an nsCString & (passed by reference).

Only when any of the data is converted, will aOutBuffer be non-null. This is a pretty important optimization to avoid copying of all message data to heap, so aOutBuffer needs to be a pointer in order to allow null.

Comment 12

[Håkan Waara]

Attachment: Patch v3

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #11)
> > >+// adds the specified file as a mailboxdescriptor to the array
> > >+nsresult nsAppleMailImportMail::AddMboxDir(nsILocalFile *aFolder, nsISupportsArray *aMailboxDescs, nsIImportService *aImportService)
> > >+{
> > >+  nsAutoString folderName;
> > >+  aFolder->GetLeafName(folderName);
> > >+  
> > 
> > nit: There's unnecessary blank space on the last "+" line here, ditto in other
> > places, and at the end of some lines, please fix.
> 
> I've deliberately put blank lines between the actions in this method, to let it
> breathe, and to make it more readable.

I don't mind the blank lines, I prefer them to be there. What I was trying to say is that some of the blank lines aren't empty - they have whitespace (spaces) which doesn't need to be there, and also:

+  NS_ASSERTION(actualBytesWritten == (convertedData ? convertedData->Length() : numberOfBytesToRead), 
+               "Didn't write as many bytes as expected for .emlx file?");

the NS_ASSERTION line has an extra space at the end of it which doesn't need to be there.

> > >+nsresult nsEmlxHelperUtils::ConvertToMboxRD(const char *aMessageBufferStart, const char *aMessageBufferEnd, nsCString **aOutBuffer)
> > 
> > aOutBuffer should be an nsCString & (passed by reference).
> 
> Only when any of the data is converted, will aOutBuffer be non-null. This is a
> pretty important optimization to avoid copying of all message data to heap, so
> aOutBuffer needs to be a pointer in order to allow null.

I don't understand your argument. The current code does:

+  // convert message data to MBOX RD, if needed.
+  nsCString *convertedData = NULL;
+  rv = ConvertToMboxRD(startOfMessageData, (startOfMessageData + numberOfBytesToRead), &convertedData);
+  if (NS_FAILED(rv)) {
+    delete convertedData;
+    [pool release];
+    return rv;
+  }
+    
+  // write the actual message data.
+  if (!convertedData)
+    rv = aOut->Write(startOfMessageData, (PRUint32)numberOfBytesToRead, &actualBytesWritten);
+  else {
+    IMPORT_LOG1("Escaped From-lines in %s!", path.get());
+    rv = aOut->Write(convertedData->get(), convertedData->Length(), &actualBytesWritten);
+  }
+    
+  // we don't need converted data any longer
+  delete convertedData;

My suggestion would be:

// convert message data to MBOX RD, if needed.
nsCString convertedData;
rv = ConvertToMboxRD(startOfMessageData, (startOfMessageData + numberOfBytesToRead), convertedData);
if (NS_FAILED(rv)) {
  [pool release];
  return rv;
}
  
// write the actual message data.
if (convertedData.IsEmpty())
  rv = aOut->Write(startOfMessageData, (PRUint32)numberOfBytesToRead, &actualBytesWritten);
else {
  IMPORT_LOG1("Escaped From-lines in %s!", path.get());
  rv = aOut->Write(convertedData->get(), convertedData->Length(), &actualBytesWritten);
}

Thus you avoid the dynamic allocation of an nsCString (and needing to remember to delete it), but still only copy the data if you need to.

Comment 14

[Håkan Waara]

Attachment: Patch v4

This patch addresses Standard8's last review comments.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 314161 [details] [diff] [review]
Patch v4

Just a couple of nits:

+    ////////////////////////////////////////////////////////////////////////////////
+    // apple mail import components
+    ////////////////////////////////////////////////////////////////////////////////
+    #if defined(XP_MACOSX)
+        ,{ "Apple Mail Import Component", NS_APPLEMAILIMPORT_CID,
+        "@mozilla.org/import/import-applemail;1", nsAppleMailImportModuleConstructor, AppleMailRegister, nsnull },
+        { "Apple Mail Import Implementation", NS_APPLEMAILIMPL_CID,
+          NS_APPLEMAILIMPL_CONTRACTID, nsAppleMailImportMailConstructor},
+    #endif

Please indent the ifdef/endif and the lines the same as the rest of the file.

+nsresult nsAppleMailImportMail::Initialize()
...
+  rv = proxyObjectManager->GetProxyForObject(NS_PROXY_TO_MAIN_THREAD, 
+                                             NS_GET_IID(nsIStringBundle),
+                                             bundle, NS_PROXY_SYNC | NS_PROXY_ALWAYS, 
+                                             getter_AddRefs(mBundleProxy));
+
+  return rv;
+}

May as well just return the result straight away and not go through rv.

r=me with those fixed.

Comment 16

[Håkan Waara]

Thanks Mark, I've fixed your last two nits in my local hg tree.  Now awaiting bienvenu's super-review...

Comment 17

[David :Bienvenu]

Two things about nsEmlxHelperUtils::AddEmlxMessageToStream :

Are you writing out the x-mozilla-status2 header as well?
I think you should write out an x-mozilla-keywords header (or whatever that header is called) at the same time, to allow space for tags, like we do in the pop3 sink code (we add an x-mozilla-keywords header with 76 blanks (or so) at the end to allow space to write in keywords.

There are several places where you use braces {} where there's only a single line of code after the if (). My preference would be not to use braces in that situation, but I guess Mark didn't mind...

Comment 18

[Håkan Waara]

Attachment: Patch v5

bienvenu, here's a new patch addressing your comments.

Changes:

* Remove braces from one-line conditional clauses (thank god for regexps!)
* Add X-Mozilla-Keywords and X-Mozilla-Status2 headers
* In order to do the above, needed to fix a #define in nsMsgLocalFolderHdrs.h that unnecessarily depended on another #define in mime/public

Comment 19

[Håkan Waara]

Bienvenu, any idea when you'll get to this?  Thanks.

Comment 20

[David :Bienvenu]

Hakan - soon, probably tomorrow; I just need to get a mac build up and running with the patch to try it out.

Comment 21

[David :Bienvenu]

tried it out, and ran into a problem. This code is wrong:

+  // write the X-Mozilla-Status header according to which flags we've gathered above.
+  PRUint32 dummyRv;
+  if (x_mozilla_flags) {

if x_mozilla_flags is 0, you still want to write out the x-mozilla-status header; you just want it to be 0000.

I hope that's why unread messages were imported as read...

Comment 22

[Håkan Waara]

(In reply to comment #21)
> tried it out, and ran into a problem. This code is wrong:
> 
> +  // write the X-Mozilla-Status header according to which flags we've gathered
> above.
> +  PRUint32 dummyRv;
> +  if (x_mozilla_flags) {
> 
> if x_mozilla_flags is 0, you still want to write out the x-mozilla-status
> header; you just want it to be 0000.
> 
> I hope that's why unread messages were imported as read...
> 

I think I did that because x_mozilla_flags = 0 is effectively the same as not writing out a header, although I agree that we might as well do it anyway.

According to this page: http://www.eyrich-net.org/mozilla/X-Mozilla-Status.html?en a read message would have X-Mozilla-Status: 0x0001 so I don't *think* that that should be the problem in this case.

Can you try to comment out the if-statement out and see if it makes a difference for you? If not, it would be good if I can debug that emlx message somehow. Looking at the Message Source and seeing what the headers look like in Thunderbird might be interesting too.

I assume you're using OS X 10.4 or later?

Comment 23

[David :Bienvenu]

it's not the same as not writing out the header - the header is where we store the flags persistently, in case the .msf file gets rebuilt. If you don't write out the header, we don't have any place to persistently store the flags.

I think we changed the code to assume that messages with no mozilla-status flags were read, because of a long-since fixed bug in the original importers that left folks with folders with no x-mozilla-status flags, and they were having messages appear as unread when the .msf file got invalidated.

I'll try commenting out the if statement. I don't think there's anything special about the .emlx file. The message was simply unread. I'm using OS X 10.5 - just out of curiousity, if you import an unread local message from mail.app, does it show up as unread or read for you, in TB?

Comment 24

[Håkan Waara]

(In reply to comment #23)
> 
> I'll try commenting out the if statement. I don't think there's anything
> special about the .emlx file. The message was simply unread. I'm using OS X
> 10.5 - just out of curiousity, if you import an unread local message from
> mail.app, does it show up as unread or read for you, in TB?
> 

I just tried, and importing an unread message works (worked) fine here, both before and after commenting out that if clause. Maybe it's something to do with POP vs IMAP accounts? I only have a POP account. 

Can you retry with that change and see if it fixes the problem for you?  I'm getting eager to get this patch in... :-)

Comment 25

[David :Bienvenu]

Comment on attachment 317375 [details] [diff] [review]
Patch v5

Importing an unread  message w/o commenting the line works fine for me, except that the message is shown as read, and there is no x-mozilla-status line in the imported message.  You *must* have such a line. I'm surprised that doesn't make a difference for you. Anyway, thx for doing this, sr=bienvenu, if you remove the if (x_mozilla_flags) check, and braces.

Comment 26

[Håkan Waara]

Attachment: Final patch (as checked in)

Comment 27

[Håkan Waara]

All checked in. Except for a local mistake in suite's jar.mn file I had to do two one-liner bustage fixes in nsEmlxHelperUtils.mm:

* #include prprf.h to get PR_smprintf and other PR_* symbols. No idea why this worked for me, but not on the tboxes.

* Changed obj-c message integerValue to intValue. integerValue is a 10.5-only message while intValue is supported from 10.0 and later. There is no practical difference between the two in our case.

Finally fixed; yay!!