166603 - IMAP: Drafts/Sent/etc not recognized as special folders, when using namespaces.

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[Aleksey Nogin]

I have in my prefs.js:

user_pref("mail.server.server3.namespace.other_users", "");
user_pref("mail.server.server3.namespace.personal",
"\"INBOX.Sys.\",\"INBOX.Work\",\"INBOX.Folders.\"");
user_pref("mail.server.server3.namespace.public", "");
user_pref("mail.server.server3.override_namespaces", false);

and

user_pref("mail.identity.id2.draft_folder",
"imap://[e-mail]/Drafts");
user_pref("mail.identity.id2.drafts_folder_picker_mode", "0");

With this setup Mozilla (1.1 on Red Hat Linux 7.3) correctly saves messages to
INBOX.Sys.Drafts when I do "save as draft". However when I go to the Drafts
folder, the message is displayed same as if it was an ordinary folder and there
is no way to continue editing the draft (other then "Edit as new")!

Comment 1

[Karen Huang]

Which build are you using? Can you provide the build info here?
What kind of the IMAP Server are you connecting with? 

Comment 2

[Aleksey Nogin]

I am using 1.1 release (using RPMs from ftp.mozilla.org) on Red Hat Linux 7.3.
The server is Courier IMAP.

Comment 3

[Aleksey Nogin]

Same with Sent - the sent messages will be filed there normally, but when
viewing the folder, it would show the sender instead of showing the recipient
(as it supposed to do in Sent).

P.S. Not sure if this is an IMAP issue, an FE issue, or both...

Comment 4

[philip zhao]

I reproduced this bug in my fastmail account (Cyrus server) too.  And also I
can't save massage in mydraft when IMAP server directory is set INBOX.

Comment 5

[Aleksey Nogin]

Yeah, still there in BuildID 2002103002 (trunk).

Comment 6

[philip zhao]

This may have something to do with the server.Need server prepare a pair of
Drafts and Sent and Templates folders for every personal namespace? If server
don't support default Drafts and Sent ... folders initially, mozilla can be
changed into create those folders when it first discoverfolders. Just like
1.CreateDefaultMailboxes,2.SetFlagsOnDefaultMailboxes.( And the key of the
solution will be flags' setting and using.) But it is expensive.

Comment 7

[Aleksey Nogin]

Re: comment #6

> This may have something to do with the server

Regardless of server, Mozilla is clearly confused - it knows to _save_ messages
into those folders, but for some reason faile to realize they are special when
_displaying_ them.

Comment 8

[Henry Jia]

This is the client issue.

Comment 9

[philip zhao]

Does someone have tracked if the flags is set correctly for Drafts folder?Do
Aleksey Nogin mean even if the flags is set correctly, we still can't see the
'edit draft...' button or other errors?Thnks:)

Comment 10

[Aleksey Nogin]

How do I check the flags? My guess is they are *not* set correctly - at least in
the folders pane the "normal folder" icons are used instead of the special ones.

Comment 11

[philip zhao]

If the MSG_FOLDER_FLAG_DRAFTS or such other flags(which means that this is a
special folder) is set correctly, I find the "Edit draft..." button and other
special functions works well. I'm finding if there is something wrong with the
setting of flags or recognize these special folder. Thks Aleksey.

Comment 12

[Henry Jia]

The reason should be the flags are *NOT* set correctly because of the namespace.

Comment 13

[philip zhao]

If drafts (or other special ones) is parrelled with 'INBOX' or subfolder of
'INBOX',flags is set. If they are in more deeper levels, for
example,'INBOX.Sys.Drafts',flags is not set. A question is ,need mozilla keep a
pair of Drafts and Templates and Sent folders for each personal namespace?

Comment 14

[Aleksey Nogin]

> A question is ,need mozilla keep a
> pair of Drafts and Templates and Sent folders for each personal namespace?

Why "each"??? Right now Mozilla (IMHO correctly) keeps Drafts, Trash and Sent in
the *first* personal namespace, but does not set any flags. 

In any case, if all available namespaces are subspaces of INBOX. (as in my
case), then Mozilla does not have amny other choice but to be able to use one of
those (and set the correct flags), which is what this bug is about.

Comment 15

[philip zhao]

Attachment: add a patch.

Comment 16

[Aleksey Nogin]

Comment on attachment 105540 [details] [diff] [review]
add a patch.

>+    if(name->Equals(NS_LITERAL_STRING(onlineNameWithoutNamespace)))

This fails to compile:

nsImapMailFolder.cpp:390: `LonlineNameWithoutNamespace' undeclared

Comment 17

[Henry Jia]

Comment on attachment 105540 [details] [diff] [review]
add a patch.

Hi, philip,

Thx for the patch anyway. But there are something needed to be payed attention
to.

1. try to build and test the code before you give out the patch

2. pay attention to the code style, refer to
http://www.mozilla.org/hacking/mozilla-style-guide.html, also please refer to
the existing code

3. As for the bug, Drafts/Sent/etc should not be recongnized by comparing the
literals, they can be any names. Also, nsMsgFolder::SetPrefFlag
(nsMsgAccountManager::SetSpecialFoldersForIdentities) should be better place
for the patch.

Henry

Comment 18

[philip zhao]

Thanks Aleksey and henry. I will be careful later.:-)

I also find that nsMsgFolder::SetPrefFlag where mozilla set flags when it
initially find out Draft and other special ones will be the best place to add patch.

But nsIMAPNamespace *nsImapMailFolder::GetNamespaceForFolder() is a protect
method of nsImapMailFolder and we can't use it in
mailnews/base/util/nsMsgFolder.cpp.

How can we get the folder's namespace in nsMsgFolder.cpp then? 

Because nsMsgFolder mainly deal with those actions which needn't know what kind
of a folder it is, if we add a protect method GetNamespaceForFolder here(using
it only to get namespace of this folder when server supports namespace), it will
seems a not small change to mozilla's whole code structure. Should we do that?
Thank you.

Comment 19

[Henry Jia]

try nsIImapIncomingServer::getUriWithNamespacePrefixIfNecessary

Comment 20

[philip zhao]

Attachment: a patch for test

1.Reason of this bug is SENTMAIL,DRAFTS,TEMPLATES flags are not set correctly
because of namespace.

2.In nsMsgFolder::SetPrefFlag(),mozilla find out special folders using 
  2.1 accountMgr->GetFirstIdentityForServer(server, getter_AddRefs(identity))
  2.2 identity->GetFccFolder(getter_Copies(folderUri));
  2.3 rdf->GetResource(folderUri, getter_AddRefs(res))
  If these three steps all succeed, flag will be set using folder->SetFlag.

3.I guess 2.1 will not fail, for no identity no all. And 2.2 will not fail
either because the fail of GetFccFolder will only demonstrate there are no
special folders initially supported by server(Is it correct?). So only 2.3 may
fail.

4.Thus I wrote this patch,which means if folderUri (a uri without namespace
prefix) is not enough to let mozilla find the folder's resource, we will give
it a complete uri folderUriWithNamespace, it may find the folder and set flags
then.

5.Pls test it when you have time,I have no this bug's environment.:-)

Comment 21

[Aleksey Nogin]

Comment on attachment 106183 [details] [diff] [review]
a patch for test

This patch does not fix the problem for me.

However:

1) I agree with the reasoning behind the patch: It is likely that the
"rdf->GetResource" is the one that fails. But note - this is *not* what your
patch is currently doing - you seem to have attached "else" to the *wrong
branch*:

>@@ -1540,6 +1543,22 @@
>       folder = do_QueryInterface(res, &rv);
>       if (NS_SUCCEEDED(rv))
>         rv = folder->SetFlag(MSG_FOLDER_FLAG_SENTMAIL);
>+      else
>+      {

e.g. your "else" would only become active if do_QueryInterface(res) fails,
*not* if "rdf->GetResource" fails!

Comment 22

[Aleksey Nogin]

BTW, your reasoning have suggested a workaround! If I choose the folder to use
by selecting 

Place a copy in -> "Sent" Folder on: -> Work

then I see this bug. But if instead I choose

Place a copy in -> Other: -> Sent on Work

(e.g. I manually point to the folder I want), then it works fine!

P.S. Would it be better to try to fix the GetFccFolder function itself, rather
than trying to fix the place(s) that use it?

Comment 23

[Henry Jia]

>2.In nsMsgFolder::SetPrefFlag(),mozilla find out special folders using 
>  2.1 accountMgr->GetFirstIdentityForServer(server, getter_AddRefs(identity))
>  2.2 identity->GetFccFolder(getter_Copies(folderUri));
>  2.3 rdf->GetResource(folderUri, getter_AddRefs(res))
>  If these three steps all succeed, flag will be set using folder->SetFlag.
>
>3.I guess 2.1 will not fail, for no identity no all. And 2.2 will not fail
>either because the fail of GetFccFolder will only demonstrate there are no
>special folders initially supported by server(Is it correct?). So only 2.3 may
>fail.
It's not correct that 2.3 will fail. 2.3 always succeed. You need just add the
namespace if needed.

Fixing GetFccFolder is a good suggestion.

Pay attention to the fcc_folder, etc may locate on another server with different
namespace.

You need to setup an enviroment for the work. You may refer to our internal web
site for some test enviroment.

Comment 24

[philip zhao]

Attachment: a new patch

Pls try this. Added namespace before folder name of Draft etc.

Comment 25

[Henry Jia]

Comment on attachment 106317 [details] [diff] [review]
a new patch

See comment 23

Have you tried to test the patch?

Comment 26

[philip zhao]

Yes.Henry.I have tested it.
The differentce of the new patch with the second patch is simple, only let the
code of else to run.(I only add a '!' before NS_SUCCEEDED(rv) of line a111)

a111>if (!NS_SUCCEEDED(rv))
a222>        rv = folder->SetFlag(MSG_FOLDER_FLAG_SENTMAIL);
a333>      else
a444>      {//code here will run

In my test account of courier server matchpoint,username mailer1 ,password
mailer1, whatever override_namespace is true or false, or if we have configured
personal namespace by hand, the code all work well. I can see these special
folders with special icons now.And when I save a massage as draft, the 'Edit
Draft...' button shows .It works well.

In the original code,the code of line a222 do set flag.But flag is not set on
correct folder.
Originally,set flag on "imap://mailer1@matchpoint/Sent"(no this folder acturlly)
Now,set flag on "imap://mailer1@matchpoint/INBOX/Sent".(though we may not see
Sent folder at the beginning,but once we ever use it,we will see it from that
time on.The same is with Drafts and Templates.)
So I think it is correct.

Of course,the code should have a clean. But that will be easy. So we may test it
first.

Comment 27

[Henry Jia]

The logic of your code is not right. 

You should check if the uri includes namespace first before trying to get the
related folder object.

Comment 28

[philip zhao]

Acturlly, in nsIImapIncomingServer::getUriWithNamespacePrefixIfNecessary,
mozilla has checked if the uri(we give to this method as a parameter) have
namespace. That means, if uri is "imap://mailer1@matchpoint/INBOX/Sent", doesn't
change, if uri is "imap://mailer1@matchpoint/Sent", insert 'INBOX/' into the uri.

Comment 29

[philip zhao]

While,I think whatever the uri is "imap://mailer1@matchpoint/Sent" or
"imap://mailer1@matchpoint/INBOX/Sent" ,we should both setflag to the folder
object. Because in some other conditions the folder is just
"imap://mailer1@matchpoint/Sent". The code should be changed to this. This is
what I wanted in patch 2.

Comment 30

[Aleksey Nogin]

> if uri is "imap://mailer1@matchpoint/Sent",
> insert 'INBOX/' into the uri.

Ah, I think you've nailed it - that sound exactly where the bug is coming from!
It should insert the first namespace here, not the "INBOX/".

Comment 31

[philip zhao]

You are right.I think it did just as you say.I mean that too.

Comment 32

[Aleksey Nogin]

Comment on attachment 106317 [details] [diff] [review]
a new patch

I am starting to think - is the "old" branch necessary at all? Would not it
work better if the code just *always* use GetUriWithNamespacePrefixIfNecessary
and never even tries running GetResource(folderUri)? This way the brunch before
the "else" would be totally unnecessary.

Sure, the code will be slightly less efficient in the simple (no namespaces)
case, but I am sure the difference will be negligible and it's more important
that the code will be simpler.

Comment 33

[Aleksey Nogin]

Comment on attachment 106317 [details] [diff] [review]
a new patch

Sorry, ignore what I just said - of course, we can not always use
GetUriWithNamespacePrefixIfNecessary since it only exists for IMAP.

Comment 34

[philip zhao]

Attachment: better than the 3rd one

This patch add a logic to check if the server is a imap server.

Henry,I'm not sure what you mean to "check if the uri includes namespace first
before trying to get the related folder object". Amazing...:-) I'd like to talk
with you about it.

Comment 35

[Henry Jia]

Comment on attachment 106491 [details] [diff] [review]
better than the 3rd one

Hi, Philip,

Thx for the hard work. This patch looks good. Just a few small tips.

1. you may just use
nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server);
instead of
rv = server->GetType(&serverType);
to check if the mail server is imap. This can make the code a little more
efficient. And, try not to use the hardcode, such as "imap".

2. pay attention to the code style, such as the indent character number.

3. try to make the line is less or equal to 80 characters.

4. get rid of 
    //fccfolder treatment end
since you just have only one such comment.

5. get rid of 
       // && NS_SUCCEEDED(rdf->GetResource(folderUri, getter_AddRefs(res))))

As for "check if the uri includes namespace first
before trying to get the related folder object", I mean you should check if the
server is imap server and check if the uri includes namespace before you
"folder = do_QueryInterface(res, &rv);". You have done so in your new patch.
:-)

Keep on going.

Henry

Comment 36

[philip zhao]

Attachment: a new patch

I changed the lines as your comment,henry. 

1.Using nsCOMPtr<nsIImapIncomingServer> imapServer = do_QueryInterface(server)
and if(imapServer) to check if the server is a imap server.

2.Bring some lines out for efficiency.

3.Other is corrected as your comment.

Comment 37

[philip zhao]

Attachment: patch with folder_picker_mode checks

Add check of fcc_folder_picker_mode (and also with Drafts,Templates folder's
picker_mode) in this patch.

Comment 38

[Aleksey Nogin]

Philip, when you update patches, can you mark the old ones as obsolete
(provided, of course, they are indeed obsolete)? Thanks!

Comment 39

[philip zhao]

I get it.
Thx! Nogin!

Comment 40

[Henry Jia]

Comment on attachment 106627 [details] [diff] [review]
patch with  folder_picker_mode checks

1. use 'nsXPIDLCString folderPickerMode;' instead of 'char *
folderPickerMode;', thus you don't need to take care of the memory release.

2. still the code style :-)

Comment 41

[philip zhao]

Attachment: updated new patch

Having changed it as your comment, henry. 3x!:-)

Comment 42

[Henry Jia]

Comment on attachment 106637 [details] [diff] [review]
updated new patch

1. use
folderPickerMode.Equals("0")
instead of
PL_strcmp(folderPickerMode.get(), "0")

2. still code style. pay attention to the intend char number. Thx.

Comment 43

[philip zhao]

Attachment: updated new patch

henry,pls see this one. 

1.using folderPickerMode.Equals("0")
2.when the parameter is more than 2, using a new line for each parameter in a
method.

:-)

Comment 44

[Henry Jia]

Comment on attachment 106747 [details] [diff] [review]
updated new patch

It's OK. Thx, Philip.

r=henry

Comment 45

[Aleksey Nogin]

Comment on attachment 106747 [details] [diff] [review]
updated new patch

This patch works for me, thanks!

Comment 46

[David :Bienvenu]

Comment on attachment 106747 [details] [diff] [review]
updated new patch

first of all, there's no way the imap code should be in nsMsgFolder.cpp. For
imap, this could be in nsImapIncomingServer::PossibleImapMailbox. Also, this
patch duplicates a lot of code that could be in a helper routine.

Comment 47

[philip zhao]

What's the meaning and useness of redirector_type(always three values found: ""
and "aol" and "netscape") in nsImapIncomingServer ?

Comment 48

[philip zhao]

If we still based on currently used namespace architechture, 2 advanced bugs are
sure to be found when we have fixed bug #166603 and bug #154778.

Bug #166603 means that special folders like Drafts Templates Sent are not set
flags correctly.

Bug #154778 means that we can't access folders on Courier server when we set
IMAP server directory to "INBOX".
       
Now supposed we have conquered this two bugs.
1. Special flags are set because in our patch we can find the folder correctly.
2. We can access the folders when we set IMAP server directory on Courier server.

This bug will be found soon:
When the folderPickerMode is '1' which means we use "others" in "Copies &
Folders" and we manully selected a folder to let it act as a special folder, and
if we set IMAP server setting into "INBOX", the flags is not there any more.

Acturely, they are not set correctly again.
We have set flags into folder "INBOX.aaa.bbb", but in the folder pane the folder
is "aaa.bbb". It has no flags set.

Comment 49

[philip zhao]

Hi, David, Can you see attachment 106747 [details] [diff] [review] for a more time? I still resist it. :-)

Comment 50

[Billy Newsom]

I just put a long description in a very related bug 123707.  The problem with
that old bug is that it is listed as enhancement instead of normal.  I would
hope that either this bug gets updated to include the whole Windows trunk, or
that old one gets a new lease on life with a normal bug status.

Bug 114533 seems to be similar.  Bug 174670 is another one.

I am reporting this problem on Windows 2000 and Windows 98.  Probably all
Windows clients are affected.  I'm also seeing the bug on Mozilla 1.2.1 and
1.3a.  I have Courier IMAP server.  Please see my fuller description in bug 123707.

Comment 51

[Henrik Gemal]

also on windows

Comment 52

[Peep Chaintreuil]

bug 168060 may be blocked/a result by/of this bug.

Comment 53

[Wayne Mery (:wsmwk)]

Does this still fail for you?

I have "" in namespace and special folders works, but perhaps "" is just ignored?

Comment 54

[Aleksey Nogin]

I am using the workaround from comment #22 and a fairly old version of TBird, so I would not know if this works in a recent version without the workaround...

Comment 55

[Wayne Mery (:wsmwk)]

last patch author, philip zhao, is gone - email address is dead.
no one has picked up the patch.

Comment 56

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 106747 [details] [diff] [review]
updated new patch

Patch has bitrotted.

nsMsgFolder.cpp is no longer present at mailnews/base/util/

See:

http://hg.mozilla.org/comm-central/file/25a5e9e27cc4/mailnews/base/util/