Closed
Bug 147995
Opened 23 years ago
Closed 22 years ago
Difficulty finding trash folder when using an IMAP server directory
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: mkmail, Assigned: Henry.Jia)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
2.17 KB,
patch
|
Henry.Jia
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
Please note that this is not a user issue as the code is written in such a way
that the end user does not see this bug.
Code says more than a thousand words:
From function: nsImapProtocol::DiscoverMailboxSpec
if (serverTrashName)
{
if (!PL_strcasecmp(nsPrefix, "INBOX.")) // need to special-case
this because it should be case-in
sensitive
{
#ifdef DEBUG
NS_ASSERTION (PL_strlen(serverTrashName) > 6,
"Oops.. less that 6");
#endif
trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
(PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
!PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
!PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
}
else
{
trashExists = (PL_strcmp(serverTrashName,
adoptedBoxSpec->allocatedPathName) == 0);
}
The problem is the following. If using a Cyrus server (or a few others), the
personal namespace is "INBOX." and the serverTrashName is "INBOX/Trash" and
works properly. (I am not sure about the delimiter here, something I have to
check, but I think it is a / because the serverTrashName is in canonical namespace).
When using a server directory though and this server directory is "INBOX/", then
the serverTrashname (which is in canonical namespace) is not INBOX.Trash but
just simply "Trash".
As you can see in the above code, it will fail to find the Trash folder, because
the namespace *is* "INBOX." but the ServerTrashname simply "Trash".
Also, the above code makes a few assumptions it shouldn't.
It compares against INBOX.
* It assumes that INBOX is a personal namespace (which it doesn't have to be
* It assumes that . is a personal namespace delimiter
I am not sure if the check for "INBOX." should be there at all. If the server
name were properly set to a personal namespace, then we wouldn't have this problem.
This seems to me like a quick hack to get cyrus imap severs to somehow work at all.
The reason this still worked when having a server directory set to "INBOX." was
that evengthough the trash folder was not found, a new one is created later in
the code as "Trash". Add the server directory and this becomes "INBOX.Trash"
which is the TRash folder as we know it.
A quick hack to temporarily fix this is:
if (serverTrashName)
{
trashExists = (PL_strcmp(serverTrashName,
adoptedBoxSpec->allocatedPathName) == 0);
if ((!trashExists) && (!PL_strcasecmp(nsPrefix,
"INBOX."))) // need to special-case t
his because it should be case-insensitive
{
#ifdef DEBUG
NS_ASSERTION (PL_strlen(serverTrashName) > 6,
"Oops.. less that 6");
#endif
trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
(PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
!PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
!PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
}
But like I said, the check against "INBOX." shouldn't be there at all.
Assign this to me if you want. I am getting quite into the IMAP code.
Reporter | ||
Comment 4•23 years ago
|
||
I don't consider this a dupe of the other two bugs.
This is just a bad code issue.
Because the trash folder is being "found" (or rather recreated) later on. But it
shouldn't try to recreate an exisiting trash folder.
Updated•23 years ago
|
QA Contact: gayatri → meehansqa
This is a logical error. After mozilla changes the 'Trash' folder name to
canonical format, it will never get the name with 'INBOX.' prefixed. If the
'IMAP Server Directory' setting is not set, the changed name will be
'INBOX/Trash'. If the setting is set, the changed name will be 'Trash' that
trigger the ASSERTION.
Taking. :-)
Add mscott to CC list.
'INBOX' should be case insensitive.
Pls r=/sr=.
Attachment #94009 -
Attachment is obsolete: true
Comment 8•23 years ago
|
||
+ ns ? ns->GetDelimiter() : 0, &serverTrashName);
0 will never work, right?
David, you have the sharpest eye. :-)
Indeed, if code goes here, there is no need to check ns. It must not be nsnull.
We have checked above.
Here is the new patch. Pls r=/sr=. Thx.
Attachment #94013 -
Attachment is obsolete: true
Assignee | ||
Comment 10•23 years ago
|
||
Add navin & cavin to the CC list.
Pls r=. Thx.
Status: NEW → ASSIGNED
Comment 11•22 years ago
|
||
Comment on attachment 94288 [details] [diff] [review]
new patch for review
r=naving
Attachment #94288 -
Flags: review+
Comment 12•22 years ago
|
||
This should also be related to bug 24064
Comment 13•22 years ago
|
||
+ if (strlen(serverTrashName) >= 6 &&
+ !PL_strncasecmp(serverTrashName, "INBOX/", 6)) //
case-insensitive
can't this just be:
if (!PL_strncasecmp(serverTrashName, "INBOX/", 6)) ?
what is the purpose of this change? It looks superfluous to me. It seems to me
that the change that actually fixes this bug is that one line above, changing
INBOX. to INBOX/
- trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
- (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
- !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
- !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
+ trashExists = (strlen(adoptedBoxSpec->allocatedPathName) >= 6
&& /* "INBOX/" */
+ !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX/" */
+ !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
Assignee | ||
Comment 14•22 years ago
|
||
A clearer patch. The patch does mainly compare 'serverTrashName' (in canonical
format) with 'INBOX/'.
Just a little change, carrying navin's r=.
Attachment #94288 -
Attachment is obsolete: true
Attachment #96670 -
Flags: review+
Comment 15•22 years ago
|
||
that looks like the same patch to me, but it removes these two lines
trashExists = ((PL_strlen(serverTrashName) > 6 /* nsCRT::strlen("INBOX.") */) &&
- (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6
which are actually needed because of this line:
+ !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6);
Again, do you really need to change anything more than the one line?
Assignee | ||
Comment 16•22 years ago
|
||
The core of this patch is
- if (!PL_strcasecmp(nsPrefix, "INBOX.")) // need to special-case
this because it should be case-insensitive
+ if (!PL_strncasecmp(serverTrashName, "INBOX/", 6)) //
case-insensitive
After setting 'IMAP Server Directory', 'serverTrashName' may not include 'INBOX/'.
I removes these two lines is because they are not needed. If code goes there, we
have executed the following code and the condition is ok
if (!PL_strncasecmp(serverTrashName, "INBOX/", 6))
and
!PL_strncasecmp(adoptedBoxSpec->allocatedPathName, serverTrashName, 6)
which implies the removed lines.
Comment 17•22 years ago
|
||
Comment on attachment 96670 [details] [diff] [review]
new patch for review
ok, thx, I think I see.
Attachment #96670 -
Flags: superreview+
Assignee | ||
Comment 18•22 years ago
|
||
In trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Product: MailNews → Core
Updated•17 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•