105385 - Cyrus IMAP: Server directory has / appended always

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Oliver Jones]

When using Cyrus all user created folders are subfolders of INBOX.  Folder names
are seperated by periods (.).  If I got into Mail/News Account Settings->Server
Settings->Advanced and change Mail Server Directory to "INBOX." like I can in
OutLook Express (to make all the folders appear to be top level rather than sub
folders of INBOX) Moz appends a / character to whatever I entered.

In short this causes all folder operations to fail.  It would not be a problem
if the / was not appended automatically.  By putting a / there, Moz is assuming
I'm using wu-imapd or some other IMAP server that seperates folders names with /.

"Assumption is the mother of all f*ckups."
Please fix this.

Platform: Moz 0.9.5 (2001101201)
Linux: RedHat 6.2
Cyrus imapd: 1.6.24

Comment 1

[Oliver Jones]

Corrected subject.  

Comment 2

[Henry Jia]

CCing to myself

Comment 3

[Henry Jia]

Attachment: patch for review

We should not only support the "IMAP Server Directory" ended with '/', but with
other delimiters, such as '.', etc. In the patch, I don't append '/' when
saving the preference. When using the directory, I append it with the delimiter
that is returned from the server in specific namespace if it is needed.

Comment 4

[Michael Klose]

I accidentally posted a patch to this bug under the wrong bug #27002.

It is attachment #85493 [details] [diff] [review].

For now I will copy the patch comments:

Created an attachment (id=85493)
Change to IMAP URL

The main aim of this patch is the following:

The server directory is left as it is and the delimiter for the server
directory is always a /.

To achieve this, the following is modified:

If we go under the assumption that the server directory delimiter is always a
/, then nsImapUrl::AllocateCanonicalPath contains a bug:

(original version, not patch)

      int len = PL_strlen(onlineDir);
      if (!PL_strncmp(onlineDir, currentPath, len)) 

The problem here was that onlinedir used the / as a delimiter and currentPath
uses whatever the server uses.

The patch converts the currentPath to a canonical form before the comparison is
done instead of at the end as in the original version.

If we continue the assumption that the server directory should use the / as a
delimiter, then nsImapUrl::AllocateServerPath to reconstruct the server path
from the canonical name also contains a problem: AddOnlineDirectoryIfNecessary
which adds the server directory in front of the folder name has to be executed
while the folder name is still in a canonical form. The patch just rearranges
the calls a little to ensure that the conversion into canonical form is done
after the AddOnlineDirectoryIfNecessary is called.

I think this is a good and proper fix. I have tested this and it works
properly.

It does though show the same symptoms of difficulty of finding a trash folder
as it did when we just forced the server subdirectory to be "INBOX." in the
prefs.js or user.js. The trash issue is a general problem when using a server
directory.

For this I will open a new bug now.

Comment 5

[Jeremy Howard]

Good work Michael!

I haven't seen any problem using Trash when setting the Server Directory to
'INBOX.'. In fact, I've *never* had a problem with setting the Server Directory
in any situation. Are you maybe using Henry Jia's bug 27002 patches? Could this
be the cause of that problem? You and Henry probably need to look at each
other's patches and make sure you're on the same wave-length since you're
tackling some of the same bits of code.

Comment 6

[Michael Klose]

Evaluation of the two patches:

Basically they follow two different concepts:

- Mine: leave the server directory in canonical "namespace"
- Henry's: leave the server directory in server "namespace"

What I like about mine is that the user does not have to know what the server
delimiter is.

Also, in your patch, you only check if the last character is a valid delimiter
or not. You do not call ConvertToCanonicalFormat and only look at the ending
characters. This is not good if there are multiple delimiters in the server
directory name and the user has entered some other delimiters. OK, if someone
uses a delimiter different from / in my version, then it also does not work.

I may be biased because I wrote my patch, but I think keeping the directory
delimiter constant is more user friendly and can also be better explained in the
help.

Comment 7

[Jeremy Howard]

Regarding the Trash issue--I now see that the fact that it worked was actually a
bug, as Michael explains in this thread:
http://www.fastcheck.org/phpbb2/viewtopic.php?t=129

Comment 8

[Michael Klose]

Jeremy, the trash issue is just a code one. The end result of the way it is is
that it works.

The reason is, when using a server directory, the trash folder is not found when
the personal name space at the same time is "INBOX.".

After not finding a trash folder (eventhough it is there), one is created in the
same place as it was, so it will be there.

Just when running in debug moder there are a lot of assertions.

Comment 9

[Michael Klose]

To comment #4:

I made a slight but important typing mistake:

original:

"The patch just rearranges
the calls a little to ensure that the conversion into canonical form is done
after the AddOnlineDirectoryIfNecessary is called."

what I meant:

"The patch just rearranges
the calls a little to ensure that the conversion into NON-canonical form is done
after the AddOnlineDirectoryIfNecessary is called."

Comment 10

[Michael Klose]

Attachment: Patch to keep using / as a server directory delimiter

This used to be attachment #85453 [details] which I posted under the wrong bug number.

Comment 11

[Michael Klose]

Trash issue:

Look at bug #147995

Comment 12

[Henry Jia]

Attachment: new patch which is more flexible

In my opinion, user may fell confusing if we tell them they should use the
delimiter '/' when the server says that the delimiter is different, such as
'.'. And, from the comments in the source code, I think this also is what the
original thought when they used '/' as the delimiter. They said at that time,
the only supported delimiter was '/'. So, in my first patch, I make the patch
support the server delimiter.
    But I also think your saying is reasonable, especially for the users who
don't know the delimiter of the server and the unix users who like the symbol
'/'. When we tell them the delimiter is '/', they'll willing to accept it. And
it's also a benifit for us when we write the help. Thanks, mklose.
    Now, in my new patch, I give the use the flexibility. They may either use
the delimiter of the server or use the specific delimiter '/', and they can
even omit the last delimiter.

Comment 13

[Michael Klose]

Henry, your latest patch rocks.

Let's put it out into the wild.

Now we only need someone offical to r and sr it.

Comment 14

[Tom Mraz]

Nominating to get some attention.

Comment 15

[Henry Jia]

Let me take care of this bug, add [e-mail] to the CC list.

Comment 16

[David :Bienvenu]

I think this code would be a lot cleaner if you used an nsCAutoString instead of
a char *. Then, you can use ReplaceChar, you don't have to worry about freeing
memory, you don't have to worry about null terminating, etc.

Comment 17

[Henry Jia]

Attachment: new patch using nsCAutoString instead of char*

According to David's suggestion, give the new patch using nsCAutoString instead
of char*.

Comment 18

[David :Bienvenu]

Comment on attachment 87916 [details] [diff] [review]
new patch using nsCAutoString instead of char*

r=bienvenu, looks good.

Comment 19

[Scott MacGregor]

Comment on attachment 87916 [details] [diff] [review]
new patch using nsCAutoString instead of char*

sr=mscott

Comment 20

[Henry Jia]

In trunk.

Comment 21

[Michael Klose]

I am seeing a slight ACL issue with this.

(Fastmail.fm cyrus account with ACLs enabled)

When NOT USEING a server directory, clicking on properties of the folder is
telling me:
"This is a personal mail folder. It has been shared. You have the following
permissions: Read, Write, Insert (Copy Into), Set Read/Unread, Delete Messages,
Create Subfolder, Post"

When USEING a server directory, I get the message for the exact same folder:
"This is a personal mailfolder. It is not shared". "You have the following
permissions: Full control".

When logging into the IMAP account with TELNET, and doing a GETACL on the
folder, I see that I and admin have these priveledges.

I just think it is strange that it is showing me two different things for the
same folder.

Comment 22

[Henry Jia]

Michael Klose,

I can verify what you said, it should be a new bug. I've filed Bug 160291
according to your description. :-)

Henry

Comment 23

[Henry Jia]

Apply for a= for branchOEM

Comment 24

[Henry Jia]

Done in OEM branch.