Closed
Bug 120485
Opened 23 years ago
Closed 22 years ago
Can't delete \NoSelect folders on IMAP server cascadely
Categories
(MailNews Core :: Networking: IMAP, defect)
MailNews Core
Networking: IMAP
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: Henry.Jia, Assigned: Henry.Jia)
Details
Attachments
(2 files, 3 obsolete files)
7.37 KB,
patch
|
mscott
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
shaver
:
approval+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011207 BuildID: 2002011510 Mozilla can't delete folders on IMAP server cascadely. From the communication and the code, I'm sure that mozilla didn't delete these folders successfully. Test Enviroment: mozilla nightly build 2002011510, mozilla 0.9.7 on solaris & win2k Reproducible: Always Steps to Reproduce: 1. Clear the default settings 'Edit'->'Mail & Newsgroup Account Settings...'->'Server Settings'->'Advanced...'->'Server supports folders that contain sub-folders and messages' for IMAP server that supports folders that can contain either sub-folders or messages 2. Create folders that can only contain sub-folders cascadely 3. Delete the toppest parent folder 4. Mozilla shows it's OK. But in fact, when you refresh the folder list of the mail window, you'll see the deleted folders. Actual Results: The folders can't be deleted cascadely. Expected Results: The folders should be deleted cascadely. And from the code, mozilla tries to do this.
mscott, I'm trying to solve this bug, can this bug assigned to me.
Comment 2•23 years ago
|
||
reassigning.
Assignee: mscott → Henry.Jia
Status: UNCONFIRMED → NEW
Ever confirmed: true
I test this patch on solaris & win2k professional. I test IMAP 4 and IMAP 4 Rev 1 servers(including UW server).
I made a stupid mistake last time. Now, I give the correct patch.
Comment 5•22 years ago
|
||
mscott@netcape.com: Would you review the patch for Henry please? Thanks.
Comment 6•22 years ago
|
||
I'm sorry I haven't done this yet. I started Friday and then accidentally killed the window I was typing in =). I'll finish this on Monday for sure.
Comment 7•22 years ago
|
||
Comment on attachment 68508 [details] [diff] [review] new suggested fix Thanks for the patch Henry. A couple comments, most of them are very minor nits. 1) we tend to not use the notation where you put a 'b' before booleans, a 'p' before pointers, etc. In the case of method arguments like bDeleteMyself we do encourage using an 'a' for argument. i.e. DeleteSubFolders(const char * aMailboxName, aDeleteSelf); instead of using myself, how about self: aDeleteSelf. myself makes it sound like we are deleting a person =). 2) small typo in your comment for DeleteSubFolders on: "dicide". the last part of the comment should be "and provide feedback...." 3) Can you move the comment for DeleteSubFolders above the method instead of below it? 4) instead of using PL_strlen, people are trying to convert existing code to just strlen. This is a recent change you wouldn't know about with out me telling you. For more details: Bug #124536 5) I'd get rid of variables like iLen and pSelectedMailboxDir and bMyselfInTheSubFolderList and just rename them to things like length, selectedMailboxDir, etc. 6) PL_strcpy can now be just strcpy 7) I'd express equalties with zero as: !PL_strcmp(currentName, selectedMailbox) 8) instead of bMyselfInTheSubfolderList how about folderInSubfolderList instead of bMyselfDelted how about folderDeleted 9) Instead of using PL_strcmp, use nsCRT::strcmp Thanks for the patch!
Comment 8•22 years ago
|
||
In addition to Scott's comments, I have some comments as well: + PR_FREEIF(pSelectedMailboxDir); This can just be PR_Free(pSelectedMailboxDir) since PR_Free checks for null - PR_FREEIF checks for null, and also sets pSelectedMailboxDir to null, which I don't think you need, so it's just extra code. Or, you could use an nsCAutoString, which will get cleaned up automatically. Also, the variable named "b" - that's not a useful name for a variable. In fact, can't you just pass in PR_FALSE directly and get rid of the variable completely?
according to the bug 124536 sccott's comments 9 may changed to 9) Instead of using PL_strcmp, use strcmp directly.
Assignee | ||
Comment 10•22 years ago
|
||
Thanks, mscott, bienvenu, margaret and jerry. I've revised the patch according to your suggestions. Bienvenu, since the argument 'aDeleteSelf' in the function DeleteSubFolders is not a general parameter, it's a reference parameter and wants to return something, so we can't pass PR_FALSE directly to the function.
Updated•22 years ago
|
Comment 11•22 years ago
|
||
Comment on attachment 71624 [details] [diff] [review] the revised patch one small nit, should we check for an allocation failure: selectedMailboxDir = (char *)PR_MALLOC(length+2); r=mscott You still need to get an sr= on this. I'd suggest bienvenu.
Attachment #71624 -
Flags: review+
Comment 12•22 years ago
|
||
Comment on attachment 71624 [details] [diff] [review] the revised patch sr=bienvenu
Attachment #71624 -
Flags: superreview+
Assignee | ||
Comment 13•22 years ago
|
||
Should I give a new patch according to mscott's comment 2002-03-05 or the patch that has passed r= and sr= will be checked in? Another thing is that I want to obsolete the patch(attachment 67368 [details] [diff] [review]) and the patch(attachment 68508 [details] [diff] [review]), but I don't have the right. Anyone can obsolete them to make it clear? How can I get this right? Thanks.
Updated•22 years ago
|
Attachment #67368 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #68508 -
Attachment is obsolete: true
Comment 14•22 years ago
|
||
just add a check for a failed memory allocation. No need to post a new patch. Your good to go. Looks like someone already obsoleted those 2 patches for you.
Comment 15•22 years ago
|
||
Unfortunately, Henry does not have checkin privileges yet. In order for someone to check in for him, I think it is better for him to add a new patch with the check. With that, I guess he will get automatic approval based on mscott@netscape.com's comment. It would be nice if someone can check it in for him after he has provided the patch. I am afraid that I may not have the time to do so until next week. Thanks, Margaret
Assignee | ||
Comment 16•22 years ago
|
||
According to Margaret & mscott's comments, I give this new patch. Just add a malloc check. Can anyone reviewed it and checked in? Thanks very much.
Assignee | ||
Comment 17•22 years ago
|
||
Make the check more strict. Compare to attachment #71624 [details] [diff] [review], two checks are added. 1. use if( selectedMailboxDir ) // only do the intelligent test if there is enough memory { strcpy(selectedMailboxDir, selectedMailbox); selectedMailboxDir[length] = onlineDirSeparator; selectedMailboxDir[length+1] = '\0'; PRInt32 i; for( i=0; i<numberToDelete && !folderInSubfolderList; i++ ) { char *currentName = (char *) m_deletableChildren->ElementAt(i); if( !strcmp(currentName, selectedMailbox) || !strcmp(currentName, selectedMailboxDir) ) folderInSubfolderList = PR_TRUE; } } instead of strcpy(selectedMailboxDir, selectedMailbox); selectedMailboxDir[length] = onlineDirSeparator; selectedMailboxDir[length+1] = '\0'; PRInt32 i; for( i=0; i<numberToDelete && !folderInSubfolderList; i++ ) { char *currentName = (char *) m_deletableChildren->ElementAt(i); if( !strcmp(currentName, selectedMailbox) || !strcmp(currentName, selectedMailboxDir) ) folderInSubfolderList = PR_TRUE; } 2. use if( selectedMailboxDir && !strcmp(selectedMailboxDir, longestName) ) // just myself instead of if( !strcmp(selectedMailboxDir, longestName) ) // just myself This should be final patch. Checkin wanted.
Attachment #75908 -
Attachment is obsolete: true
Comment 18•22 years ago
|
||
Comment on attachment 77400 [details] [diff] [review] the patch for checkin r=bienvenu on last patch
Attachment #77400 -
Flags: review+
Comment 19•22 years ago
|
||
Nominating (but I don't know if this will be accepted - it's a bit late)
Keywords: nsbeta1
Assignee | ||
Comment 20•22 years ago
|
||
Not too late, the patch is still fresh. The related files on trunk haven't been changed these days.
Comment 21•22 years ago
|
||
Comment on attachment 77400 [details] [diff] [review] the patch for checkin I thought I already reviewed this back in February. It sure looks familiar. Sorry if you were waiting long. sr=mscott
Attachment #77400 -
Flags: superreview+
Assignee | ||
Comment 22•22 years ago
|
||
Thanks, mscott and bienvenu. a= and checkin wanted
Comment 23•22 years ago
|
||
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 77400 [details] [diff] [review] the patch for checkin a=shaver for 1.0 branch.
Attachment #77400 -
Flags: approval+
Updated•22 years ago
|
Target Milestone: --- → mozilla1.0.1
Comment 26•22 years ago
|
||
please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+ keyword and add the fixed1.0.1 keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment 27•22 years ago
|
||
checked in to 1.0 branch.
Comment 28•22 years ago
|
||
if this is really in the 1.0 branch we should change the mozilla1.0.1+ keyword into a fixed1.0.0 keyword so QA will know to verify this on the branch.
Keywords: mozilla1.0.1+ → fixed1.0.0
Comment 29•22 years ago
|
||
Verified on all the platforms that we can delete \NoSelect folders cascadely: -------------------------------------------------------------------------------- 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 53 list "" "28_test1/*" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST (\NoSelect) "/" 28_test1/ 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST (\NoSelect) "/" 28_test1/28_test2 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST (\NoSelect) "/" 28_test1/28_test2/28_test3 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 53 OK LIST completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 54 list "" "28_test1/28_test2/28_test3/*" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST (\NoSelect) "/" 28_test1/28_test2/28_test3/ 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 54 OK LIST completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 55 delete "28_test1/28_test2/28_test3/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 55 OK DELETE completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 56 unsubscribe "28_test1/28_test2/28_test3/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 56 OK UNSUBSCRIBE completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 57 list "" "28_test1/28_test2/*" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST (\NoSelect) "/" 28_test1/28_test2/ 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 57 OK LIST completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 58 delete "28_test1/28_test2/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 58 OK DELETE completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 59 unsubscribe "28_test1/28_test2/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 59 OK UNSUBSCRIBE completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 60 delete "28_test1/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 60 OK DELETE completed 1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 61 unsubscribe "28_test1/" 1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 61 OK UNSUBSCRIBE completed -------------------------------------------------------------------------------- Verified on Windows 2000 06-27-08-1.0 branch build Verified on Linux 06-27-06-1.0 branch build Verified on Mac 06-27-05-1.0 branch build Changing keywords from "fixed1.0.0" to "verified1.0.1".
Keywords: fixed1.0.0 → verified1.0.1
Updated•22 years ago
|
Summary: Can't delete folders on IMAP server cascadely → Can't delete \NoSelect folders on IMAP server cascadely
Updated•20 years ago
|
Product: MailNews → Core
Updated•15 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•