368347 - Importer (Win Eudora, Mail) does not import message STATUS (read, unread, etc..)

Status: RESOLVED FIXED

(Thunderbird :: Migration, defect)

Description

[Matt Dudziak]

The problem is that the importers for the Eudora mail store do not import message status, so all messages are imported as unread.

To reproduce:

- In Eudora, mark some messages unread, some read, some replied, some forwarded, some redirected.

- Import Eudora mail into Penelope
- Note messages in Penelope are all marked unread.

Comment 1

[Geoffrey C. Wenger]

Attachment: Patch that fixes bug. Note patch was not created with CVS and this is my first attempt at a multiple file patch.

Improvements to importing mail from Windows Eudora. Basic message attributes and status is now maintained (read/unread, priority, label).

Comment 2

[Scott MacGregor]

Comment on attachment 260466 [details] [diff] [review]
 Patch that fixes bug. Note patch was not created with CVS and this is my first attempt at a multiple file patch.

In nsEudoraImport.cpp, you could probably do: 

rv = pTagService->GetTagForKey(PromiseFlatCString(defaultEudoraLabels[i].key), eudoraTag);

instead of declaring nsCString eudroaKey; and copying the string: eudoraKey = defaultEudoraLabels[i].key;

ditto for eudoraColor...

Some of the changes in nsEudoraMailbox.cpp are probably going to conflict with the work David has been doing to remove nsIFileSpec from mailnews on the trunk. See Bug #33451. You and David might want to coordinate here or one of you is going to have a nasty conflict.

+	{
+		tocEntry.m_bManuallyJunked = PR_FALSE;
 	}
I don't think you need the braces around this statement.

Comment 3

[Scott MacGregor]

Comment on attachment 260466 [details] [diff] [review]
 Patch that fixes bug. Note patch was not created with CVS and this is my first attempt at a multiple file patch.

I don't have any new comments other than the ones I already added. I think you're going to have a CVS conflict with David's file spec changes though when you get this patch ready to check in.

Comment 4

[David :Bienvenu]

yes, I've been resigned for a while to a fair amount of grief resolving conflicts with Geoffrey's recent patches. As soon as I land the file spec import stuff, I'll try to get his patches applied to the trunk, which I'll have to do by hand, I suspect.

Comment 5

[Geoffrey C. Wenger]

(In reply to comment #2)
> (From update of attachment 260466 [details] [diff] [review])
> In nsEudoraImport.cpp, you could probably do: 
> 
> rv = pTagService->GetTagForKey(PromiseFlatCString(defaultEudoraLabels[i].key),
> eudoraTag);
> 
> instead of declaring nsCString eudroaKey; and copying the string: eudoraKey =
> defaultEudoraLabels[i].key;
> 
> ditto for eudoraColor...

Using PromiseFlatCString in the manner that you describe resulted in compilation errors of the type:
"none of the 3 overloads can convert parameter 1 from type 'char *'"
If I'm understanding PromiseFlatCString I don't think it does what I needed to do anyway. I have char *'s, but I needed to pass something compatible with const nsACString & - hence the use of temporary nsCString variables. There may be some better way to do this conversion, but PromiseFlatCString doesn't seem to be it.


> Some of the changes in nsEudoraMailbox.cpp are probably going to conflict with
> the work David has been doing to remove nsIFileSpec from mailnews on the trunk.
> See Bug #33451. You and David might want to coordinate here or one of you is
> going to have a nasty conflict.

As David responded we're aware of the issues here. It's an unfortunate side effect of the fact that we're using a branch for current Penelope development.


> +       {
> +               tocEntry.m_bManuallyJunked = PR_FALSE;
>         }
> I don't think you need the braces around this statement.

True, the braces aren't needed to compile, but they make me happy :-). I'm ok with no braces for if/else with single lines - but if I needed to use braces for either the if or else statement, I prefer to use braces for both even if not strictly necessary.

Geoff

Comment 6

[David :Bienvenu]

You don't want to use PromiseFlatCString going forward, since it's going away on the trunk :-)

re braces, I prefer not having them for if/else with single lines, but our style is to always use braces if either the if or else requires them, as you say.

Comment 7

[David :Bienvenu]

porting this to the trunk is next on my list of things to do...I saved the most fun for last, it seems :-)

Comment 8

[David :Bienvenu]

Comment on attachment 260466 [details] [diff] [review]
 Patch that fixes bug. Note patch was not created with CVS and this is my first attempt at a multiple file patch.

this patch, when applied to the 2.0 branch, didn't build on Windows. I'll attach a patch in a second that does build.

One problem was that mime needs to be added to the list of directories required (Makefile.in)

The other was that you can't initialize an nsString in a struct on Windows, it seems...So I switched the code to use a const char * and convert it to an nsString class before calling the tag service.

Comment 9

[David :Bienvenu]

Attachment: patch that compiles on windows

The other thing Windows/MSVC wasn't happy about was lines like this:

+       static const PRUint16 MSF_ALT_SIGNATURE         = 0x0001;

inside the class definition. I moved them out of the class.

Now I'll work on getting this compiling against the trunk.

Comment 10

[Geoffrey C. Wenger]

(In reply to comment #9)
> Created an attachment (id=264060) [details]
> patch that compiles on windows
> 
> The other thing Windows/MSVC wasn't happy about was lines like this:
> 
> +       static const PRUint16 MSF_ALT_SIGNATURE         = 0x0001;
> 
> inside the class definition. I moved them out of the class.
> 
> Now I'll work on getting this compiling against the trunk.

That surprises me, because that's a completely good C++ thing to do and MSVC has been fine with my use of it in the past (and was for this case too). But the compiler being used for the trunk is newer and I guess I'm not shocked that it whined about stuff that the older version didn't complain about for me.

I'm sorry that this bug fix is proving such a pain to re-work. Thanks much for your efforts here (and on my other bugs that you fixed for use in the trunk).

Comment 11

[David :Bienvenu]

I was actually compiling the patch with a 2.0 tree and msvc 6 - I was trying to see what the patch looked like applied, so that I could more easily port it to the trunk.

msvc6 thought all those const declarations were defining pure virtual functions, and wanted it to say = 0 instead of = 0x0001! It was a bit surprising to me, but moving it outside the class worked fine.

NP about the patch - this is a really nice feature to have. I should be able to get it finished tomorrow.

Comment 12

[David :Bienvenu]

Attachment: trunk port of patch

this is what I'm going to land on the trunk - vc8 was happy with all the things that vc6 complained about, so this was a pretty straight port of file spec to nsIFile, except for the Makefile.in change...

I tested this on the trunk and read status was maintained (yay!) but you might want to try a trunk build at some point to make sure I didn't mess anything up.

Comment 13

[David :Bienvenu]

fix checked in.

Comment 15

[Geoffrey C. Wenger]

Attachment: Trunk patch that fixes small bugs in the filespec modifications made to my original patch, which caused code not to work as originally intended.

Comment 16

[David :Bienvenu]

Comment on attachment 305089 [details] [diff] [review]
Trunk patch that fixes small bugs in the filespec modifications made to my original patch, which caused code not to work as originally intended.

Geoff, sorry I stepped on your changes!

Comment 17

[Geoffrey C. Wenger]

David, that's ok. My patches being made on the branch + not with CVS was not a good combo.

Since this passed review, if I'm understanding correctly I should now add "checkin-needed" because I don't have CVS commit access.

Comment 18

[David :Bienvenu]

yes, checkin-needed, that's right. Thx for being understanding.

Comment 19

[Reed Loden [:reed]]

Checking in mailnews/import/eudora/src/nsEudoraMailbox.cpp;
/cvsroot/mozilla/mailnews/import/eudora/src/nsEudoraMailbox.cpp,v  <--  nsEudoraMailbox.cpp
new revision: 1.36; previous revision: 1.35
done