Closed
Bug 470725
Opened 16 years ago
Closed 15 years ago
fixes oexpress import settings and adds NNTP import
Categories
(MailNews Core :: Import, defect, P3)
MailNews Core
Import
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: philbaseless-firefox, Assigned: philbaseless-firefox)
References
Details
Attachments
(2 files, 22 obsolete files)
36.10 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
1.46 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081217 Shredder/3.0b1pre this includes the patch for <a href="show_bug.cgi?id=466753>466753</a> NNTP is not imported this includes fix for <a href="show_bug.cgi?id=468177">468177</a> patch is commented Reproducible: Always Steps to Reproduce: 1. 2. 3.
Component: Migration → Import
Product: Thunderbird → MailNews Core
Version: unspecified → Trunk
Updated•16 years ago
|
QA Contact: migration → import
Comment 4•16 years ago
|
||
(To get bug numbers auto linked, just prefix the bug number with "bug".)
Assignee: nobody → philbaseless-firefox
Updated•16 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
line 764 is removed. This adds a pref 'identity.*.IdentityName' that is not normally in the prefs when identities are manually added. and causes bad email notations for from: field see Bug 470587. Note this code is also in eudora and other import modules so someone might see if those work properly.
Attachment #354116 -
Attachment is obsolete: true
I tested this and it works great. I imported my oexpress accounts which have 2 mail pops with same smtp and same ISP. Each has throwaway addresses which use the pop and smtp and are imported nicely as identities. I have another ISP from work that it imports great as well as multiple email address from my website mail. Also I imported settings for newsgroups including GRC.com that requires authentications. The subscriptions aren't imported but those aren't settings in OE. Someone needs to check for leakage or tell me how to do that. Also I can straighten up the code if needed although it has little superfluous code
(In reply to comment #5) > line 764 is removed. This adds a pref 'identity.*.IdentityName' that is not correction: line 874
The wab or address book import opens wab.dll which in turn opens OE and resets the 'last user id' in registry so code has to go back to original of looking in reg for 'default user id'. Otherwise import of mail and settings conflict. We should have some sort of instruction how user can import multiple OE profiles (oe calls them identities but not the same as TB identity). I can advise if wanted
Attachment #354273 -
Attachment is obsolete: true
Comment 9•15 years ago
|
||
Phil: please see https://developer.mozilla.org/en/Getting_your_patch_in_the_tree. If you want to test leaks, please see https://developer.mozilla.org/en/Debugging_memory_leaks for more information.
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Comment 12•15 years ago
|
||
(In reply to comment #8) > Created an attachment (id=354372) [details] > fix a conflict with WAB import > > The wab or address book import opens wab.dll which in turn opens OE and resets > the 'last user id' in registry so code has to go back to original of looking in > reg for 'default user id'. Otherwise import of mail and settings conflict. > We should have some sort of instruction how user can import multiple OE > profiles (oe calls them identities but not the same as TB identity). I can > advise if wanted Well the obvious thing to do would be to hook the identities import into the profile migration code (which already allows different profile selection for SeaMonkey profiles at least), and then enable that code in the import option of mailnews. Unfortunately that's quite a bit of work to get right and I don't think we'll be ready by 3.0, so for now you probably just want to do the best you can.
Assignee | ||
Comment 13•15 years ago
|
||
It's not really a big item. Besides, the user may want to import all OE profiles mail into one TB profile. It would be easiest to have another box give the user a drop down list of OE profiles available to import. As a simple and workable default. We can just advise user on successfull completion "provile x has been imported. To import other profiles select is as default in OE and reimport"
Attachment #354372 -
Flags: review?(neil)
Updated•15 years ago
|
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Priority: -- → P3
Target Milestone: --- → Thunderbird 3.0b2
Comment 14•15 years ago
|
||
Comment on attachment 354372 [details] [diff] [review] fix a conflict with WAB import I'm afraid I'm finding this patch really hard to follow. It seems to contain a mix of whitespace reformatting, line wrapping and variable renaming as well as actual code. I'd prefer this split up in to separate patches; one for the reformatting, wrapping and renaming that you want to do, one to fix bugs in the existing code and one to add the new features. >+ static nsCString defMailName; >+ static PRInt32 checkNewMailTime; // = 30; >+ static PRBool checkNewMail; // = PR_FALSE; >+ static PRBool spa; Why are these static? (In particular static nsCString is always wrong). >+ if (::RegOpenKeyEx( HKEY_CURRENT_USER, "Identities", 0, KEY_QUERY_VALUE, &sKey) >+ == ERROR_SUCCESS) { We don't wrap operators on to the next line. >+ if(::RegOpenKeyEx( HKEY_CURRENT_USER, "Software\\Microsoft\\Outlook Express", Space after if >+ 0, KEY_QUERY_VALUE, &sKey) == ERROR_SUCCESS) When wrapping parameter lists, align with the first parameter. >+ if (*(PRInt32 *)pBytes != 0xffffffff){ 0xffffffff is an illegal constant. Use 0xffffffffU and PRUint32, or use -1. >- BYTE * pPort = nsOERegUtil::GetValueBytes( hKey, "IMAP Port"); >- if (pPort) >+ pBytesTemp = nsOERegUtil::GetValueBytes( hKey, "IMAP Use Sicily"); >+ if (pBytesTemp) Try not to insert new code in to the middle of a list of similar blocks.
Attachment #354372 -
Flags: review?(neil) → review-
Assignee | ||
Comment 15•15 years ago
|
||
will do. This is my first go at mozilla coding and I'm not that adept to start with and you are the unfortunate one chosen to review it. - re: wrapping--I wasn't sure about how firm the mozilla recommendations on line length be limited to 80.
Assignee | ||
Comment 16•15 years ago
|
||
Hopefully it is ok to forgo the formatting and whitespace changes, They were half-ass so I backed them out. This is the fix -to correctly import multiple identities with the same incoming server. -properly reuse smtp server or set correct settings for new smtp servers
Attachment #354372 -
Attachment is obsolete: true
Assignee | ||
Comment 17•15 years ago
|
||
backed out some formatting and minor correction that caused bad diff at line 462
Attachment #356389 -
Attachment is obsolete: true
Assignee | ||
Comment 18•15 years ago
|
||
attachment 356396 [details] [diff] [review] sec9 line 510 of existing is 595 of patch nsCOMPtr<nsIPop3IncomingServer> pop3Server = do_QueryInterface(in, &rv); and subsequent code has been placed earlier where there was a block of code with the same pop3server. If you want I can back out that change to see what I mean but eventually it should go back to the patched spot to eliminate that redundancy which is costing time and space.
Assignee | ||
Comment 19•15 years ago
|
||
line 845 missing case break: statement
Attachment #356396 -
Attachment is obsolete: true
Comment 21•15 years ago
|
||
phil, does your patch also pick up the case in bug 255102?
Assignee | ||
Comment 22•15 years ago
|
||
I can't tell, but the outlook module might have had the same deficiency. I don't have Outlook but I can take a look at the code and bug. The oe coders imported from the original registry installation but oe changed that to an 'identity' location in the reg. Outlook may have done the same.
Assignee | ||
Comment 23•15 years ago
|
||
Here's my notes on oe and sicily settings and SPA and secure authentication POP3 Secure Connection = 1 == ssl POP3 Use Sicily = <no key> or 0 = 1 use SPA SMTP Use Sicily = 0 outgoing doesn't require auth = 1 requires auth see SMTP User Name and SPA = 2 requires pop3 SPA and name = 3 requires auth see SMTP User Name no SPA 00 = SMTP Server does not require authentication (My Server Requires Authentication is not checked) 01 = logon using another account name and password + SPA 02 = use same settings as my incomming mail server 03 = logon using another account name and password
Assignee | ||
Comment 24•15 years ago
|
||
Attachment #356426 -
Attachment is obsolete: true
Assignee | ||
Comment 25•15 years ago
|
||
Adds NNTP import for oexpress
Assignee | ||
Comment 26•15 years ago
|
||
This fixes error in original file where the hKey may open and not get closed.
Attachment #356875 -
Attachment is obsolete: true
Attachment #357516 -
Flags: review+
Attachment #357516 -
Flags: review+ → review?(neil)
Updated•15 years ago
|
Attachment #357516 -
Flags: review?(neil) → review-
Comment 27•15 years ago
|
||
Comment on attachment 357516 [details] [diff] [review] closes handle and ensures closing of handle on error The patch looks mostly reasonable but I couldn't get it to apply... >+ nsCString userName; >+ // this only exists if NNTP server requires it or not anon login >+ if (pBytes = nsOERegUtil::GetValueBytes( hKey, "NNTP User Name")){ >+ userName = (char *)pBytes; >+ nsOERegUtil::FreeValueBytes(pBytes); >+ }else >+ userName = ""; Don't need this else block, nsCString defaults to the empty string. >+ // following is not xfered. Advise if needed. I assume these particular comments won't make the final patch ;-) >+ // "NNTP Prompt for Password"=dword:00000000 if exists also indicates username/password needed I think we have a setting for this, we call it pushAuth. Could be useful? >+ >+ Two is too many blank lines ;-) > void OESettings::SetIdentities( nsIMsgAccountManager *pMgr, nsIMsgAccount *pAcc, HKEY hKey, char *pIncomgUserName, PRBool spa ) > { > // need to get from smtpserver .smtpServer" >+ >+ // NNTP only needs name and address so do a check >+ PRBool isNNTP; >+ // OE requires NNTP Display Name for NG accts so use it for test Doesn't the caller already know whether or not they are NNTP? >+ if (isNNTP)id->SetReplyOnTop(0); >+ else id->SetReplyOnTop(1); Just use (aIsNNTP ? 0 : 1) >+ pBytes = nsOERegUtil::GetValueBytes( hKey, isNNTP? "NNTP Secure Connection" :"SMTP Secure Connection"); Nit: spaces both before and after both ? and : please
Assignee | ||
Comment 28•15 years ago
|
||
Comment on attachment 356462 [details] [diff] [review] change variables to file scope. correct a typo Neil attachment 356462 [details] [diff] [review] is the first part of changes to existing code that you wanted to see separate. That needs applied before the NNTP part I'll flag that for review and make the other changes to part 2.
Attachment #356462 -
Flags: review?(neil)
Assignee | ||
Comment 29•15 years ago
|
||
cleared & cleaned up some more
Attachment #356462 -
Attachment is obsolete: true
Attachment #357764 -
Flags: review?(neil)
Attachment #356462 -
Flags: review?(neil)
Assignee | ||
Comment 30•15 years ago
|
||
cleaned and cleared up more. format changes made to my code only left original to improve readability.
Attachment #357764 -
Attachment is obsolete: true
Attachment #357765 -
Flags: review?(neil)
Attachment #357764 -
Flags: review?(neil)
Attachment #357765 -
Attachment description: cosmetic → Part 1 of patch
Comment 31•15 years ago
|
||
Comment on attachment 357765 [details] [diff] [review] Part 1 of patch >+PRInt32 checkNewMailTime; // OE global setting use as default = 30 These should be static too (either class or file static). >+ // 'poll for messages' setting in OE is a global setting Actually it looks like a per-identity setting to me. >+ PRBool spa; // Secure Password Authentication == UseSecAuth Then you might as well name it useSecAuth, slightly more readable that way >+ pBytesTemp = nsOERegUtil::GetValueBytes(hKey, "POP3 Skip Account"); >+ if (pBytesTemp) >+ { >+ // OE and TB means opposite >+ pop3Server->SetDeferGetNewMail(*pBytesTemp == 1 ? PR_FALSE : PR_TRUE); POP3 Skip Account doesn't mean DeferGetNeilMail, does it? It's actually an override for checking for new mail. The IMAP equivalent is IMAP Polling. >+ pop3Server->SetLeaveMessagesOnServer(*pBytesTemp == 1 ? PR_TRUE : PR_FALSE); ? PR_TRUE : PR_FALSE is pointless, == already returns either PR_TRUE or PR_FALSE. >+#if 0 // setting this pref really messes up the reply box in composing > id->SetIdentityName(fullName); Really? I don't recognise the pref offhand, so what's its effect? >- id->SetEmail(nsCString(pEmail)); >+ if (pEmail) >+ id->SetEmail(nsCString(pEmail)); Our identities are required to have an email address. >+ nsCString userName; >+ userName = pIncomgUserName; > if (userName.IsEmpty()) { > nsCOMPtr <nsIMsgIncomingServer> incomingServer; > rv = pAcc->GetIncomingServer(getter_AddRefs(incomingServer)); > if (NS_SUCCEEDED(rv) && incomingServer) > rv = incomingServer->GetUsername(userName); > NS_ASSERTION(NS_SUCCEEDED(rv), "Unable to get UserName from incomingServer"); > } I don't think this is needed, since pIncomingUserName is always correct here. >+ PRInt32 useSicily; >+ if (pBytes = nsOERegUtil::GetValueBytes( hKey, "SMTP Use Sicily")){ >+ useSicily = *(PRInt32 *)pBytes; >+ nsOERegUtil::FreeValueBytes(pBytes); >+ } >+ else >+ useSicily = 0; Nit: Just use PRInt32 useSicily = 0; (or should that be PRUint32?) >+ smtpServer->SetTrySSL(3); // todo:make enum or #define We have one, it's called nsIMsgIncomingServer::useSSL >+ case 2 : // requires authentication No, this is use incoming server settings. >+ smtpServer->SetUseSecAuth(spa? PR_TRUE : PR_FALSE); spa should already be either PR_TRUE or PR_FALSE. >+ case 3 : >+ smtpServer->SetAuthMethod(1); >+ smtpServer->SetUseSecAuth(PR_TRUE); This is wrong, SetUseSecAuth(PR_FALSE) here.
Attachment #357765 -
Flags: review?(neil) → review-
Assignee | ||
Comment 32•15 years ago
|
||
(In reply to comment #31) > >+ case 2 : // requires authentication > No, this is use incoming server settings. In OE, which I think transfers to TB the same. Sicily 0 doesn't require authentication, 1 2 or 3 does. 2 means to use the incoming user name and incoming SPA. Please help if this is incorrect. The settings seem to xfer over proper as above. I'll double check before I post a corrected patch.
Assignee | ||
Comment 33•15 years ago
|
||
makes corrections that can be. Corrected formatting in changed code only. correct original errors regarding setusername and sethostserver redundancy. Should be final on part 1 except review changes. Will need to be merged with NNTP part 2 and that patch submitted for review.
Attachment #357765 -
Attachment is obsolete: true
Attachment #358627 -
Flags: review?(neil)
Assignee | ||
Comment 34•15 years ago
|
||
Globals need defined or file scope is easier if that's ok.
Attachment #358627 -
Attachment is obsolete: true
Attachment #358670 -
Flags: review?(neil)
Attachment #358627 -
Flags: review?(neil)
Assignee | ||
Comment 35•15 years ago
|
||
this is NNTP. Did a check and does it's job. Only need to fix formatting and white space after final review OK.
Attachment #357516 -
Attachment is obsolete: true
Attachment #358742 -
Flags: review?(neil)
Assignee | ||
Comment 36•15 years ago
|
||
I couldn't get bugzilla to diff to part1 so including here.
Attachment #358743 -
Flags: review?(neil)
Assignee | ||
Comment 37•15 years ago
|
||
diff to part1 without whitespace changes
Attachment #358743 -
Attachment is obsolete: true
Attachment #358744 -
Flags: review?(neil)
Attachment #358743 -
Flags: review?(neil)
Comment 38•15 years ago
|
||
(In reply to comment #32) > (In reply to comment #31) > > >+ case 2 : // requires authentication > > No, this is use incoming server settings. > > In OE, which I think transfers to TB the same. Sicily 0 doesn't require > authentication, 1 2 or 3 does. > 2 means to use the incoming user name and incoming SPA. > > Please help if this is incorrect. Your new patch has a better comment for this, thanks. I also asked about SetDeferGetNewMail and I'm still not convinced it means the same as "Check this account when synchronising", but whatever...
Comment 39•15 years ago
|
||
Comment on attachment 358670 [details] [diff] [review] part 1 of patch OK, this seems reasonable to me now.
Attachment #358670 -
Flags: review?(neil) → review+
Comment 40•15 years ago
|
||
Oh, and the #if 0 blocks and some of the comments should probably be removed.
Comment 41•15 years ago
|
||
Oh, and git-apply warned about 18 whitespace errors, apparently.
Comment 42•15 years ago
|
||
Comment on attachment 358742 [details] [diff] [review] includes NNTP I have some nits but I'll comment on those in attachment 358744 [details] [diff] [review].
Attachment #358742 -
Flags: review?(neil) → review+
Updated•15 years ago
|
Attachment #358744 -
Flags: review?(neil) → review+
Comment 43•15 years ago
|
||
Comment on attachment 358744 [details] [diff] [review] revised to exclude whitespace changes >+ nsresult rv = pMgr->FindServer(nsDependentCString(""), EmptyString() >+ nsDependentCString(pServerName), You can't use nsDependentCString because pServerName could be null. Since you use it twice you're better off assigning it to a local nsCString. >+ >+ Nit: double blank line >+ }else if(NS_SUCCEEDED( rv) && in){ // PATCH: use OE setting as multi ID to existing server Nit: spacing wrong >+#if 0 // useless code nowhere to be found on Earth >+#endif Better to just delete this. >+ if (isNNTP)id->SetReplyOnTop(0); >+ else id->SetReplyOnTop(1); id->SetReplyOnTop(isNNTP ? 0 : 1); >+ if (!isNNTP) // NNTP does not use SMTP in OE or TB Identities always have to have an SMTP server in case you want to reply to sender and group but I assume you'll get given a default server in this case?
Assignee | ||
Comment 44•15 years ago
|
||
(In reply to comment #43) > >+ nsDependentCString(pServerName), > You can't use nsDependentCString because pServerName could be null. Since you > use it twice you're better off assigning it to a local nsCString. Please recheck. This is in existing pop3 and imap server functions and those along with nntp function are only entered if pServerName is not null. It is only used in the FindServer and conditionally in the CreateServer. If it is necessary under NNTP then I should change the existing POP3 and IMAP code as well.
Assignee | ||
Comment 45•15 years ago
|
||
changes per review except have some whitespace cleanup as well as fixes to formatting that will be following.
Attachment #358670 -
Attachment is obsolete: true
Attachment #358742 -
Attachment is obsolete: true
Attachment #358744 -
Attachment is obsolete: true
Attachment #359457 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 46•15 years ago
|
||
changes per review except have some whitespace cleanup as well as fixes to formatting that will be following.
Attachment #359457 -
Attachment is obsolete: true
Attachment #359458 -
Flags: superreview?(bienvenu)
Attachment #359457 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 47•15 years ago
|
||
EmptyString() doesn't build. Changed back to -> nsDependentCString("")
Attachment #359458 -
Attachment is obsolete: true
Attachment #359459 -
Flags: superreview?(bienvenu)
Attachment #359458 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 48•15 years ago
|
||
same as attachment 359459 [details] [diff] [review] except fixes original file whitespace and formatting issues. Also changes if else formatting to style of original.
Attachment #359464 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 49•15 years ago
|
||
Attachment #359464 -
Attachment is obsolete: true
Attachment #359958 -
Flags: superreview?(bienvenu)
Attachment #359464 -
Flags: superreview?(bienvenu)
Assignee | ||
Comment 50•15 years ago
|
||
Attachment #359958 -
Attachment is obsolete: true
Attachment #359958 -
Flags: superreview?(bienvenu)
Attachment #359959 -
Flags: superreview?(bienvenu)
Comment 51•15 years ago
|
||
Comment on attachment 359459 [details] [diff] [review] review changes w/o whitespace fixes I believe this is obsoleted by a later patch.
Attachment #359459 -
Attachment is obsolete: true
Attachment #359459 -
Flags: superreview?(bienvenu)
Comment 52•15 years ago
|
||
Comment on attachment 359959 [details] [diff] [review] an error condition needs to close windows hKey carrying forward Neil's r, fix checked in, with some formatting improvements, thx very much for the patch, Phil.
Attachment #359959 -
Flags: superreview?(bienvenu)
Attachment #359959 -
Flags: superreview+
Attachment #359959 -
Flags: review+
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 53•15 years ago
|
||
For some reason strnlen wouldn't compile with VC7.1, so switching that too. (In reply to comment #47) > EmptyString() doesn't build. Changed back to -> nsDependentCString("") Sorry, I meant EmptyCString()...
Attachment #361489 -
Flags: superreview?(bienvenu)
Attachment #361489 -
Flags: review?(bienvenu)
Updated•15 years ago
|
Attachment #361489 -
Flags: superreview?(bienvenu)
Attachment #361489 -
Flags: superreview+
Attachment #361489 -
Flags: review?(bienvenu)
Attachment #361489 -
Flags: review+
Comment 54•15 years ago
|
||
Comment on attachment 361489 [details] [diff] [review] Tweaks Pushed changeset 9090876b3c1c to comm-central.
Assignee | ||
Comment 55•15 years ago
|
||
NNTP userName is not used in normal TB news account setup but when set password UI and maybe other parts of front end apparently mis-use it and create problems. Sometimes the password will show up as usernames for other newsgroup accounts. May be other bugs cropping up with it. No positive use for it that I see.
Attachment #362645 -
Flags: superreview?(bienvenu)
Updated•15 years ago
|
Attachment #362645 -
Attachment is obsolete: true
Attachment #362645 -
Flags: superreview?(bienvenu)
Comment 56•15 years ago
|
||
Comment on attachment 362645 [details] [diff] [review] NNTP userName should not be set Phil's going to include this in a larger patch to import newsgroups, if I understand correctly.
You need to log in
before you can comment on or make changes to this bug.
Description
•