38014 - FTP dates should be shown as localized dates

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P3)

Description

[Henrik Gemal]

When you go to fx:
ftp://ftp.mozilla.org/pub/mozilla/nightly/latest/
the dates on the files should be shown as set up in my windows control panel.

So because I'm running a english version 2k localized to Danish, the dates 
should be shown like this:
02-05-2000
instead of:
05-02-2000

Build 2000050215

Comment 1

[Gagan]

->jud

Comment 2

[Judson Valeski]

Frank - I'm using the "GetApplicationLocale()" method to generate the date in
FTP for the http index output (see
http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsFTPDirListingConv.cpp#393).
Should I be using something else?

Chris - could this formatting be getting lost in the dir viewer?
http://lxr.mozilla.org/seamonkey/source/xpfe/components/directory/nsDirectoryViewer.cpp#802

Comment 3

[Sebastian Späth]

Ftp dates are not working all the time anyway as there are problems with some 
international Win versions to display these dates at all.
(bug 30994 FTP listing does not show "last modified")

Comment 4

[Gagan]

robert-- welcome to necko!

Comment 5

[Gagan]

Hi dougt, welcome to necko :)

Comment 6

[Henrik Gemal]

Has something been done? I'm now getting "04-01-2001" as the date, which seems 
right.

Comment 7

[Karl Ove Hufthammer]

Shoudln't dates be shown according to the *Mozilla* locale (i.e., current 
language pack), not the system locale?

Comment 8

[Henrik Gemal]

*Mozilla* locale makes no sense. I always install english version of all 
software. But I like that my English Excel is using the settings from my Region 
settings in my Control Panel when showing numbers, etc...

Comment 9

[Doug Turner (:dougt)]

according the reporters comments on 2001-01-11, this has been fixed.

Comment 10

[Henrik Gemal]

can someone with a us machine please verify that the dates gets shown correct.
If I go to:
ftp://ftp.mozilla.org/
the dates shown on my locale=danish gets shown like this:
"README 0 11/23/99 12:00:00 am symbolic-link"

Comment 11

[[e-mail]]

I see US datetimes in the FTP listing, and I have US Windows set to UK region.

Comment 12

[Henrik Gemal]

not fixed.
If I go to ftp://ftp.mozilla.org
the dates are shown as "11/23/99"
I'd expect "23/11/99"

Comment 13

[benc]

qa to me.

Comment 14

[Doug Turner (:dougt)]

reassigning to [e-mail].

Comment 15

[Bradley Baetz (:bbaetz)]

Done as part of 78148

Comment 16

[Bradley Baetz (:bbaetz)]

Mass change: Bug 78148 has been checked in, and so it and these dependancies
should have been fixed. If you disagree, please reopen.

Note that the xul view is still enabled for file because of i18n problems. The
bug to fix this is bug 102812.

Comment 17

[Henrik Gemal]

not really fixed.
Dates are now shown, using an english Win2k box localized to Danish, like this:
Mozilla     : 10/02/01 11:51:00
Netscape 4.x: Tue Oct 02 11:51:00 2001
IE          : 02-10-2001 11:51

The dateformat "10/02/01" is not correct. It should be like IE style 
"02-10-2001"

Comment 18

[Bradley Baetz (:bbaetz)]

This definately works for me on linux, when I set the locale to nl_NL. (I don't
know what the ISO country code for denmark is)

I just ask for the application locale from the OS. How did you localise moz?
Using the windows regional settings stuff, or something else?

Comment 19

[Henrik Gemal]

My english Win2k is localized using the control panels "windows regional 
settings"...

Comment 20

[Bradley Baetz (:bbaetz)]

This should work, then. You are using today's nightly, right?

Do they appear correctly in the xul view? (prefs->debug->networking panel has
the option)

I'll ask some i18n people.

Comment 21

[Bradley Baetz (:bbaetz)]

So do any dates in mozilla appear localised properly? If so, let me know which
ones, and I'll go look at that code. If not, this is not an ftp bug.

Comment 22

[benc]

+testcase - QA will check dates as part of functional tests now.

Comment 23

[Bradley Baetz (:bbaetz)]

smfr said that it was wrong for him on mac, too. He was going to try changing
the call to mDateTie to pass in nsnull as a locale, which is what everyone else
does, even though thats not what the docs say.

smfr?

Comment 24

[Simon Fraser [no longer active]]

Yes, you should just pass nsnull as the first parameter to FormatPRTime().

Comment 25

[Simon Fraser [no longer active]]

Attachment: Patch to fix dates, and other FTP list problems

Comment 26

[Simon Fraser [no longer active]]

That patch fixes a number of issues:
1. It passes nsnull as the first param of FormatPRTime, thus using system
   date/time settings.
2. It fixes the bad HTML (<pre> around everything replaced with <tt>, quoting
   of attributes, consistent attribute case).
3. It splits date and time into separate table columns, so that they can be
   nicely right-aligned.
4. It shows file sizes as multiples of 1K, which is much more readable.
5. It fixes the alignment of the item label text w.r.t the image.
6. It add some cellpadding for a bit of extra space around items.
7. It collapses some pairs of pushBuffer.Append() calls into single calls for
   efficiency.

FTP listings now look pretty good. The few things I'd like to see fixed now:
 i) symlinks should have a different icon (or icons: symlink to file, symlink 
    to dir).
ii) more icon types?

Comment 27

[Henrik Gemal]

shouldn't " KB" be in some external resource file?

Comment 28

[Simon Fraser [no longer active]]

> shouldn't " KB" be in some external resource file?
I'd be suprised if this is something that would be localized. "KB" is a pretty 
standard measure of file size in all languages, no?
Of course, there are other strings here that need localization, like "Index of".

Comment 29

[Simon Fraser [no longer active]]

Attachment: Minor tweak to the patch: show symlink names in italics

Comment 30

[Bradley Baetz (:bbaetz)]

This file is localised as of last Friday, so you'll need to merge. :)

Not all the ftp backends identify symlinks in directory listings (and not all
servers tell us about them)

Re icon types, I have a bug somewhere in my "rewrite the xul viewer" list to use
the icon: url stuff. I need examples of that, though. Mozclassic did extention
matching to display what it though the type was (from the mime handlers). I
don't know if doing that is worth it.

-    pushBuffer.Append(NS_LITERAL_STRING("\"> <img border=\"0\"
align=\"absbottom\" src=\""));
+    pushBuffer.Append(NS_LITERAL_STRING("\"> <img border=\"0\" align=\"middle\"
hspace=\"2\" src=\""));

ns4 does absbottom - I just copied.

If a file's size < 1024, we should display the size in bytes. Possibly try once
more, and do Mb, too? I don't think that it does need translating, but we should
check with someone.

You also don't need the extra protected: in the .h.

Apart from that, looks great! Thanks a lot.

Comment 31

[Bradley Baetz (:bbaetz)]

Simon, do you want to take this, or do you want me to merge it in? Note that if
we're not going to use mlocale, then you can remove the member variable and the
stuff from Init.

Comment 32

[Simon Fraser [no longer active]]

bbaetz: it's all yours. I'll leave it to you to remove the mLocale stuff. Let me 
know if you need sr.

Comment 33

[Bradley Baetz (:bbaetz)]

Attachment: updated patch

Comment 34

[Bradley Baetz (:bbaetz)]

OK, I've updated smfr's patch to apply to the trunk. After talking to him on
IRC, I agree with how he's done the formatting of the output.

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 55688 [details] [diff] [review]
updated patch

I like it! sr=sfraser

Comment 36

[Darin Fisher]

Comment on attachment 55688 [details] [diff] [review]
updated patch

>Index: nsIndexedToHTML.cpp

>-    nsCOMPtr<nsILocaleService> localeServ = do_GetService(NS_LOCALESERVICE_CONTRACTID);
>-    localeServ->GetApplicationLocale(getter_AddRefs(mLocale));

can mLocale be removed then?


>     buffer.Append(NS_LITERAL_STRING("</H1>\n"));
>-    buffer.Append(NS_LITERAL_STRING("<hr><table border=0>\n"));
>+    buffer.Append(NS_LITERAL_STRING("<hr><table border=\"0\" cellpadding=\"2\">\n"));

how about combining these two literal appends?


>+    pushBuffer.Append(NS_LITERAL_STRING("</tt></a></td>\n"));
>+    pushBuffer.Append(NS_LITERAL_STRING(" <td align=\"right\"><tt>"));

how about combining these two literal appends too?


>-        pushBuffer.AppendInt(size);
>+        nsAutoString  sizeString;
>+        FormatSizeString(size, sizeString);
>+        pushBuffer.Append(sizeString);
>+        pushBuffer.Append(NS_LITERAL_STRING("&nbsp;"));   // for a little extra space
>     } else {
>         pushBuffer.Append(NS_LITERAL_STRING("&nbsp;"));
>     }
>-
>-    pushBuffer.Append(NS_LITERAL_STRING("</td>\n"));
> 
>-    pushBuffer.Append(NS_LITERAL_STRING(" <td>"));
>-    
>+    pushBuffer.Append(NS_LITERAL_STRING("</tt></td>\n <td align=\"right\"><tt>"));

how about combining the Append for the "&nbsp;" with this one?



>     if (t == -1) {
>         pushBuffer.Append(NS_LITERAL_STRING("&nbsp;"));
>+        pushBuffer.Append(NS_LITERAL_STRING("</tt></td>\n <td><tt>"));
>+        pushBuffer.Append(NS_LITERAL_STRING("&nbsp;"));

how about combining these?



>     } else {
>-        nsAutoString formatted;
>-        mDateTime->FormatPRTime(mLocale,
>+        nsAutoString formattedDate;
>+        mDateTime->FormatPRTime(nsnull,
>                                 kDateFormatShort,
>+                                kTimeFormatNone,
>+                                t,
>+                                formattedDate);
>+        pushBuffer.Append(formattedDate);
>+        pushBuffer.Append(NS_LITERAL_STRING("</tt></td>\n <td align=\"right\"><tt>"));
>+        nsAutoString formattedTime;

why not reuse formattedDate?  and call it something more generic perhaps.


>+        mDateTime->FormatPRTime(nsnull,
>+                                kDateFormatNone,
>                                 kTimeFormatSeconds,
>                                 t,
>-                                formatted);
>-        pushBuffer.Append(formatted);
>+                                formattedTime);
>+        pushBuffer.Append(formattedTime);

Comment 37

[Bradley Baetz (:bbaetz)]

> can mLocale be removed then?
It is, I just forgot to diff the header file. Oops.

> how about combining these two literal appends too?

OK, will do; new patch tomorrow.

Comment 38

[Bradley Baetz (:bbaetz)]

This also fixes bug 99169. New patch coming

Comment 39

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 40

[Simon Fraser [no longer active]]

Comment on attachment 56208 [details] [diff] [review]
patch

lowercase the tags (H1, HREF) and sr=sfraser

Comment 41

[Darin Fisher]

Comment on attachment 56208 [details] [diff] [review]
patch

r=darin

Comment 42

[Bradley Baetz (:bbaetz)]

Fixed.

Comment 43

[Henrik Gemal]

v 20020222

Comment 44

[benc]

+testcase - more work for FTP test suite.

Comment 45

[Constantine A. Murenin]

(In reply to comment #28)
> > shouldn't " KB" be in some external resource file?
> I'd be suprised if this is something that would be localized. "KB" is a pretty 
> standard measure of file size in all languages, no?
> Of course, there are other strings here that need localization, like "Index of".

It actually does need to be localisable. Not every language uses the Latin
alphabet, therefore, the string will look different in Cyrillic, let's say. 

I think we should add a common interface for "KB", so that it's localised only
in one place, and used all over mozilla.