398117 - Location: of mail folder properties doesn't display path information correctly when local mail folder name contains Japanese character

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Location: of mail folder properties doesn't display mailbox: URL correctly, when mail folder name contains Japanese character.

(Environment)
OS : MS Win XP SP2, Japanese version 
Thunderbird : latest-trunk, version 3.0a1pre (2007093003)
              Patch for Bug 180546 is already applied. 
Mail folder name : 日本語 (Shift_JIS : 0x93FA,   0x967B,   0x8CEA)
                          (UTF-8     : 0xE697A5, 0xE69CAC, 0xE8AA9E
File name        : 日本語 / 日本語.msf 
msgFilterRules.dat
  actionValue="mailbox://[e-mail]/Test-2/%E6%97%A5%E6%9C%AC%E8%AA%9E"

(Result)
%E6%97%A5%E6%9C%AC%E8%AA%9E part in displayed mailbox: URL becomes garbage.
Unescaped binary looks to be set in Location: field.
Location filed should display unescaped version, as msgFileterRules.dat does.
Note:
This is not new problem after Bug 180546.
This problem existed when "Copy Folder Location", but I'was expecting that the problem will be resolved if enhancement of Bug 180546 will be implemented, but it was not...

(Off-topic)
Guessing of file path from mailbox: URL is difficult when Japanese character is involved in mail folder name(then file name).
This is one of the reasons why I requested "File Path" information in property in Bug 180546, in addition to Location:(mailbox: URL when local mail folder).

Comment 1

[WADA:World Anti-bad-Duping Agency]

It should be next. Sorry for spam.
Location filed should display ESCAPED version, as msgFileterRules.dat does.

Comment 2

[Magnus Melin [:mkmelin]]

Linux too.

Comment 3

[WADA:World Anti-bad-Duping Agency]

New problem in Location: field of folder property) exists.
  Location: field display is different from value by "Copy Folder Location".
(Example of Copy Folder Location of Seamonkey 1.1.4)
mailbox:/C|/wada/MAIL-NEWS/Mail/dion.00.japan.com/<Garbage>
(Example of Location: field in folder property of Thunderbird trunk)
mailbox:C:\Documents(snip)\Mail\aaa.aaa.aaa\Test-2.sbd\<Garbage>

Both "Copy Folder Location" and "Location: in folder property" is different from representation in msgFilterRules.dat.
What should be displayed in Location: field of folder property?

Comment 4

[WADA:World Anti-bad-Duping Agency]

Location: field for local folder was already implemented as exact file path format even when MS Win("mailbox:" is simply added before file path).
Thanks to developer of Bug 180546.
So problem is "garbage" only. And it is should not be displayed as escaped format. It should be converted to appropriate character code to display.
Sorry for my confusion and spam.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix (checked in)

This fixes the problems with the japanese characters for me - tested on linux. 

The other part this fixes is that the url apparently was unescaped now, fallout from the string cleanup it seems. So we now had urls like mailbox:C:\path\to\mailbox - that can't be right... 

I'm not sure where else than the folder properties this is used, to check it doesn't regress anything.

Comment 6

[Magnus Melin [:mkmelin]]

Oh, and removed an unused var.

Comment 7

[David :Bienvenu]

Magnus,

thx for the patch.

We use the folder url when opening attachments from compose windows. You should check that that still works on paths that need to be escaped (e.g., the windows ones that have spaces in them by default)

http://mxr.mozilla.org/mailnews/source/mail/components/compose/content/MsgComposeCommands.js#2872

We also use the folder URL when composing a new newsgroup message:

http://mxr.mozilla.org/mailnews/source/mail/base/content/mailCommands.js#193 but that doesn't use the nsLocalMailFolder version of the method.

Comment 8

[Magnus Melin [:mkmelin]]

I did some testing with folders having spaces. The backslashes are a bit hard to come by on linux;) 
Didn't find any problem, maybe a bit surprisingly not with current trunk either - though the location it displays contains spaces.

Comment 9

[David :Bienvenu]

Comment on attachment 283405 [details] [diff] [review]
proposed fix (checked in)

ok, thx, Magnus.

Comment 10

[Magnus Melin [:mkmelin]]

Hmm, I got some second thought on this. 

The value we would get now  would now be 
mailbox:/home/magnus/.thunderbird/.... though as per comment 1, in filterrules we use something like actionValue="mailbox://user@host/Folder/%E6%97%A5%E6%9C%AC%E8%AA%9E"

Should this be in this format too? And are these kind of urls really clickable on windows?

Comment 11

[Magnus Melin [:mkmelin]]

Seems my testing earlier was incorrect. If I drag-drop a local message to attach it, it can't be opened - not on branch, not on trunk and not with this patch in place either. (IMAP works fine though.)

Seems it's been mailbox: since forever. I would have expected it to be mailbox:// - no dice with that either...

Comment 12

[Magnus Melin [:mkmelin]]

Opening attached messages from the compose window has never worked it seem - bug 271094.

Comment 13

[Magnus Melin [:mkmelin]]

Given that it never worked, I think the patch is ok after all. At least we get back to (almost) branch behavior. I'll check it in at some point if no one objects.

Comment 14

[Magnus Melin [:mkmelin]]

Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.566; previous revision: 1.565
done

->FIXED

Comment 15

[WADA:World Anti-bad-Duping Agency]

Re-opening, because problem is not resolved correctly.

Following is test result with Tb latest-trunk (on MS Win-XP).
 Thunderbird version 3.0a1pre (2007111403)
Location: display of local mail folder shouldn't be escaped.
Should be converted to system-used-charset for display of text string. 

>(Properties/General Information)
> Name : 日本語
> Location: mailbox:C%3A%5CDocuments%20and%20Settings%5Cwada%5CApplication%20Data%5CThunderbird%5CProfiles%5Ccbz7k273.Trunk-Test%5CMail%5Caaa.aaa.aaa%5CTest-2.sbd%5C%E6%97%A5%E6%9C%AC%E8%AA%9E
>(\日本語 part in Location: display) 
> %5C%E6%97%A5%E6%9C%AC%E8%AA%9E
>(Real file path for this mail folder)
> C:\Documents and Settings\wada\Application Data\Thunderbird\Profiles\cbz7k273.Trunk-Test\Mail\aaa.aaa.aaa\Test-2.sbd\日本語
>(folder path in messageFilterRules.dat)
> actionValue="mailbox://[e-mail]/Test-2/%E6%97%A5%E6%9C%AC%E8%AA%9E"

Comment 16

[WADA:World Anti-bad-Duping Agency]

If "Location:" in Properties/"General Information" for "local mail folder", which is mailbox: format, is internal URI for the local mail folder, and if application of usual escaping of URI character is right action, I think current representation and display is definitely proper.
And this won't produce problem when imap: URI and usually won't produce problem when news URI, because IMAP defines "mail folder name" in ascii or equivallence only, and because non-ascii newsgroup name is very very rare and special.
Sorry but I don't know RSS case well.

Above is one of main reasons why we requested "mail folder file path" field for local mail folder by Bug 180546, instead of opening bug for phenomenon of "Copy folder location produces garbage when local mail folder has Japanese name".
But you already morphed Bug 180546 and closed it as FIXED.
Shall we request "mail folder file path" field again by new/separated bug for usability improvement, after closing this bug as FIXED again?
(apparently, the dirty "garbage" itself is already resolved by you.)

 

Comment 17

[Magnus Melin [:mkmelin]]

No, I think you were correct to reopen this. It's still wrong on windows due to  backslashes:( 
URLs can't contain non-ascii chars (that's what really causes the original problem), but we could perhaps unescape it before displaying.

Comment 18

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #17)
> still wrong on windows due to backslashes:( 
> but we could perhaps unescape it before displaying.

Please note that problem is not only "escaped backslash/space/colon(drive letter separator)". "escaped Unicode binary for non-ascii folder name part" is also a problem. Our expectation is "real file path" like display on MS Windows, in order to reduce number of questions of "where is my mail folder file?" and unwanted/needless bug reports.

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed better fix

So after the previous fix windows paths are still quite broken, esp. due to backslashes. 

While nsEscapeNativePath could have fixed part of it, I'm not too keen on building the path myself... I'm not sure UNC filepaths are even supported here, but that's when it would have become interesting:) So I suggest we just borrow the file url creation, and smack mailbox: to the start of it instead of file:.

I also suggest we unescape the local mailbox path for display.

This patch also fixes MsgEscapeString which wasn't passing along the escape type it's given, fallout from bug 379070 attachment 269995 [details] [diff] [review].

Comment 20

[Karsten Düsterloh]

Comment on attachment 292455 [details] [diff] [review]
proposed better fix

Looks basically good (short of the problem below), but I can only check Mac and Linux, not Windows...

>Index: mailnews/base/resources/content/folderProps.js
>===================================================================
>+    // Decode the displayed mailbox:// URL as it's useful primarily for debugging,
>+    // whereas imap:// and nntp:// urls are sent around.
>+    locationTextbox.value = (gServerTypeFolder == "pop3" || gServerTypeFolder == "none") ?
>+        decodeURI(gMsgFolder.folderURL): gMsgFolder.folderURL;

This does not suffice, because the folder types "movemail" and "rss" do use mailbox: URIs as well, which won't be decoded - but I think they should be...

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: proposed better fix, v2

Good catch! Only skip encoding for news and imap.

Comment 22

[David :Bienvenu]

Comment on attachment 293662 [details] [diff] [review]
proposed better fix, v2

Magnus, thx for the patch. If you haven't already, you should make sure that opening attachments works in all the affected cases since we use the folderURL for that - http://mxr.mozilla.org/mailnews/source/mail/components/compose/content/MsgComposeCommands.js#2871

Comment 23

[Magnus Melin [:mkmelin]]

Opening attached local messages never worked (or perhaps pre tb0.9 sometimes) -  bug 271094. It just opens an empty window. This bug doesn't fix that...

Checking in mailnews/base/resources/content/folderProps.js;
/cvsroot/mozilla/mailnews/base/resources/content/folderProps.js,v  <--  folderProps.js
new revision: 1.28; previous revision: 1.27
done
Checking in mailnews/base/util/nsMsgUtils.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.cpp,v  <--  nsMsgUtils.cpp
new revision: 1.143; previous revision: 1.142
done
Checking in mailnews/base/util/nsMsgUtils.h;
/cvsroot/mozilla/mailnews/base/util/nsMsgUtils.h,v  <--  nsMsgUtils.h
new revision: 1.70; previous revision: 1.69
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.567; previous revision: 1.566
done

->FIXED

Comment 24

[David :Bienvenu]

Magnus, I meant opening any attachments, e.g., an attached .txt file,  not just attached messages - sorry if i wasn't clear.

Comment 25

[Magnus Melin [:mkmelin]]

Oh, yeah, those work fine for me.