74714 - don't hardcode "KB"/"MB" in MailNews

Status: VERIFIED FIXED

(MailNews Core :: Localization, defect)

Description

[Peter Lairo]

Mozilla/5.0 (Windows; U; WinNT4.0; en-US; 0.8.1)

Message "Size" should have a space between the number and "kb" to make it more
legible. 

The k looks kinda like a 7 or 1 and the b looks like an 8 when so close to other
numbers.

Comment 1

[Peter Lairo]

so 718kb becomes 718 kb

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

In every representation of kilobyte(s) that I've seen, xkb (where x is a number)
is how it's done.  I'm sure we don't want to break tradition (in fact, there
might surely exist an RFC out there that deals with this issue.) It's the same
thing with ohms, volts, etc.  You see 9V battery, not 9 V battery.  invalid.

Comment 3

[Peter Lairo]

Yes, and most people use M$ IE too. Mozilla is not meant to be a "traditional"
browser. We are here to make something *new*, and not repeat the mistakes of the
past. Actually, things like m, m², km are separated from the value (in most
countries). Unless someone can find the actual FRC that prohibits this, I think
this bug should remain open.

The point is that it is difficult to read that way. We need to be bold and do
what is *best* and not what *most* do. Reopen.

Comment 4

[Håkan Waara]

-> Localization

Comment 5

[rchen]

Please be more specific, which message or in the xx file or how to reproduce the 
problem so that we can narrow down the area.

Comment 6

[rchen]

This is not a localization problem. It is the string appending issue. I can 
see the string is hardcoded in line 350 nsMsgDBView.cpp. We need to move it out 
to a .properties or .dtd.

hwaara, can you fix it?

Reassigned it to hwaara.

Comment 7

[Håkan Waara]

I think I can move it out to a properties/dtd file - but that's not the actual
problem here.  

I don't think we should have a space between the number and the KB/K. We don't
in other places in this app, so why be inconsistent?

If you want, I can morph this bug into "make the KB localizable" but I WONTFIX
the space.

Comment 8

[robinf]

FWIW, the American Heritage Dictionary (which we use as a department reference)
lists the abbreviation as "KB" (uppercase letters).

Comment 9

[Peter Lairo]

Then why not change it in the rest of the app . Ehy not make everything better
insted of the most visible implementation of "kb" worse, just because it is easier.

Well, if you're all going to be stubborn about this and keep it difficult to
read, then maybe at least you can make the size collumn *right justified*.

That way all kb's will be aligned and the size of the file is clearly determined
by how far it extends to the left.

This would be standard conform because all spreadsheets (e.g. Excel) show number
collumns right justified. How about it?

BTW: what does "make the KB localizable" mean?

Comment 10

[jglick]

4.x right justifies, and it does look better than left justified.

Comment 11

[Håkan Waara]

File a new bug (not on me) about that then.  The inital bug description is no
longer current as to what you want fixed.

Comment 12

[(not reading, please use [e-mail] instead)]

> "Well, if you're all going to be stubborn about this"

peter, keep it civil.  thanks.

Comment 13

[Matthew T (active 1999-2002)]

The following things disagree with Håkan's resolution:
*   Outlook Express 5.0 for Mac OS;
*   Internet Explorer 5.0 for Mac OS;
*   Nautilus (GNOME file manager);
*   the Finder (Mac OS file manager);
*   the /American Institute of Physics Style Manual/, fourth edition, section 3
    (and if anyone would know about units of measurement, it would be them,
    right?);
*   bug 65710;
*   me.

The general rule is that there should be a space between a number and its unit 
of measurement (e.g. `13 cm'), unless the unit is before the number (e.g. 
`NZ$0.02'). So, you don't write `67kilobytes', you write `67 kilobytes'. And 
you don't write `67kb', you write `67 k'. (For kilobytes we omit the `B', to 
help in visually distinguishing between `k' and `MB' in a column of sizes ... 
but that's another bug.)

The only app I can find which doesn't put a space between the number and the 
unit is Windows Explorer. So if you want to make this platform-specific, then 
you can omit the space on Windows. But if you're not going to make it
platform-specific (and I can understand perfectly why you wouldn't want to 
bother:-), it should have a space. Reopening.

Comment 14

[now [e-mail]]

Matthew, I could not have said it better myself.  There definately _should_ be 
a space between the number and the unit.  Of all the computer applications I 
have seen, the Opera browser seems to display measurements most correctly 
(according to SI), perhaps those involved could have a look at that as a rough 
guide.

I believe that this property should not depend on either platform or locale, as 
it is internationally correct.

On a related note, the correct symbol / abbreviation for second is "s", 
not "sec" as was used in Netscape Communicator.

Comment 15

[rchen]

I propose let's move it to a properties/dtd file so that people can do whatever 
they think it right - add space or change it to KB.  Hardcode in cpp is just not 
the right way. Thanks.

Comment 16

[Håkan Waara]

I have better stuff to do - reassign to module owner and reset bug info.

Comment 17

[rchen]

Changed the title to "Hardcoded string in nsMsgDBView.cpp".  Please remove it to 
a properties or dtd file.

Reassigned to sspitzer.  

Comment 18

[(not reading, please use [e-mail] instead)]

there are two of them.  yes, we should be using the string bundle for these.

Comment 19

[Karl Ove Hufthammer]

According to the offical Norwegian standard NS-ISO 31, there should be a space
between the number and the unit. Therefore, this *needs* to be localizable.

Comment 20

[(not reading, please use [e-mail] instead)]

I'm not so sure about "Re: ", mscott tells me it is always "Re: ".

(it was hard coded in 4.x)

Comment 21

[Karl Ove Hufthammer]

Yes, it's *always* 'Re: '. In Outlook Express, 'Re: ' was translated in
localized versions, which has caused tons of problems (e.g. subjects like 'Re:
SV: SV: Re: Aw: Re: Subject').

The latest news standard draft, <URL:
http://www.landfield.com/usefor/drafts/draft-ietf-usefor-article-04.txt >, even
explicitly states this (no doubt because of all the problems OE has caused):

   Followup agents MUST NOT use any other string except "Re: " as a back
   reference. Specifically, a translation of "Re: " into a local
   language or usage MUST NOT be used.

Note that 'MUST NOT' is a requirement.

Comment 22

[Håkan Waara]

I agree with Karl - let's have Re: hardcoded.

I once used the swedish version of Outlook, and every time I replied to someone
in a thread - it would break the thread because it used "SV:" (short for "Svar"
-- Reply in swedish) instead of "Re:".

Please look at bug 13025 , bug 29179 and bug 21267 .

Duplicates or merely siblings to this bug? I'll let QA decide.

Comment 23

[(not reading, please use [e-mail] instead)]

updating summary.  "Re: " is to remain hard coded.

Comment 24

[Seungbeom Kim]

There seems to have been some confusion whether there should be a space between
the number and the unit symbol "KB". AFAIK there should be one. I'll give you
some references:

* http://euler9.tripod.com/analysis/si.html
"Always put a space between the numeric value and its following unit symbol (SAE
TSB-003, Sect. 7.3.10; ISO 1000 [4], Sect. 6.1; ASTM SI-10, Sect. 3.5.1.e)."

* http://www.iee.org/EduCareers/ProfDev/Guides/UNITSANDSYMBOLSFORELECTRICAL.doc
"A space is set between the number and its unit symbol (e.g. 240 V, not 240V). 
The decimal multiples and submultiples given below are prefixed, without a
space, to the unit symbols (e.g. 6.6 kV)."

* http://www.microlink.co.uk/units.html
"Unit symbols are placed after the numerical value, leaving a space between the
value and the symbol. For example 5 V not 5V."

I hope this helps.

Comment 25

[Seungbeom Kim]

The proper capitalization of the unit should be "KB", not "kb" or "kB".
- Lowercase 'k' means 1000, uppercase 'K' means 1024.
- Lowercase 'b' means bit, uppercase 'B' means byte.
ref: http://www.poynton.com/PDFs/Writing_SI_units_(USL).pdf

Comment 26

[Mark Banner (:standard8)]

Attachment: Patch to allow KB/MB to be localized

This patch takes the hardcoded "KB" and "MB" from nsMsgDBView.cpp and
nsMsgFolderDataSource.cpp (there were two of them) and places them in the
localizeable file messenger.properties.

From the way the comments were going, the space should be left to the
localization teams to decide.

Comment 27

[Mike Kaply [:mkaply]]

We shouldn't be appending KB on the end, we should be allowing substitution.

In some languages, the KB would be written before the number.

Comment 28

[[e-mail]]

Comment on attachment 163240 [details] [diff] [review]
Patch to allow KB/MB to be localized

>+
>+kiloByteAbbreviation=KB
>+megaByteAbbreviation=MB
Some documentation would be nice.

>+    static const char propertyURL[] = MESSENGER_STRING_URL;
You don't need a static const char for this, the original literal works fine.
Yes, I realize you copied the mistake...

>+  if (mMessengerStringBundle)
>+    res = mMessengerStringBundle->GetStringFromName(aStringName, &ptrv);
>+  
>+  if ( NS_SUCCEEDED(res) && (ptrv) )
>+    return ptrv;
>+  else
>+    return nsCRT::strdup(aStringName);
Well, it's not how I would have implemented it, I didn't like the original in
nsMsgDBView.cpp either, although it's not so bad here because the data source
should be a singleton service.

>     // On OS/2, we have an issue where temporaries get destructed in
>     // conditionals. Solution is to break it out
>     //
>     // XXX todo
>     // can we catch this problem at compile time?
>     // see #179234
This comment is out of date for this code. You can in fact switch this back to
?: because you're safe with PRUnichar* strings.
>     if (sizeInMB)
>-      sizeString.AppendLiteral(" MB");
>+      sizeString.Append(kMegaByteString);
>     else
>-      sizeString.AppendLiteral(" KB");
>+      sizeString.Append(kKiloByteString);

Comment 29

[Mark Banner (:standard8)]

Attachment: Revised patch to allow any possible localization of KB/MB

A revised patch that incorporates Neil's and Michael's comments, and allows the
localization teams to put the space in if they want (they couldn't before).

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 163747 [details] [diff] [review]
Revised patch to allow any possible localization of KB/MB

Neil, I know if you set review+ and we make minor changes you don't want to
look at it again, but on this patch I've made a couple of what I consider
bigger changes - using a format string to cope with the abbreviation and  a
reworked string bundle.

Do you want to look at it again for r or shall I re-use your existing?

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 163747 [details] [diff] [review]
Revised patch to allow any possible localization of KB/MB

Cancelling review, minor update coming.

Comment 32

[Mark Banner (:standard8)]

Attachment: Minor updates

Add check for null pointer and add a couple of comments about the localization
as requested by timeless.

Comment 33

[Mark Banner (:standard8)]

Comment on attachment 163755 [details] [diff] [review]
Minor updates

Neil: See notes on previous request, just added a couple of items as requested
by timeless on IRC.

Comment 34

[Mark Banner (:standard8)]

Marking assigned, removing helpwanted and updating title to reflect actual
changes. previous title: "don't hardcode "KB" nsMsgDBView.cpp" 

Comment 35

[[e-mail]]

Comment on attachment 163747 [details] [diff] [review]
Revised patch to allow any possible localization of KB/MB

>+# LOCALIZATION NOTES(kiloByteAbbreviation): Do not translate %d below, it is the size of the message/folder
>+kiloByteAbbreviation=%dKB
>+# LOCALIZATION NOTES(megaByteAbbreviation): Do not translate %d below, it is the size of the message/folder
>+megaByteAbbreviation=%dMB
Not sure what l10n policy is but it looks odd to duplicate the note!

>+    nsresult res = NS_OK;
>+    if (!mMessengerStringBundle)
>+    {
>+      res = InitStringBundle();
>+    }
This code only runs once so you can inline InitStringBundle using a local
variable.

>+    if (NS_SUCCEEDED(res) && mMessengerStringBundle)
>+    {
>+      mMessengerStringBundle->GetStringFromName(NS_LITERAL_STRING("kiloByteAbbreviation").get(), &kKiloByteString);
>+      mMessengerStringBundle->GetStringFromName(NS_LITERAL_STRING("megaByteAbbreviation").get(), &kMegaByteString);
>+    }
So what if these fail?

Comment 36

[Mark Banner (:standard8)]

Attachment: Revised patch v4

Incorporate Neil's comments and cope with failing to get the localized strings
- it displays the localization string name with is what nsMsgDBView.cpp did.

Comment 37

[[e-mail]]

Comment on attachment 163973 [details] [diff] [review]
Revised patch v4

>-  *aSizeString = ToNewUnicode(formattedSizeString);
>+  if (aSizeString)
>+    *aSizeString = ToNewUnicode(formattedSizeString);
>+  else
>+    return NS_ERROR_OUT_OF_MEMORY;
Stop me if I'm wrong, but aren't you supposed to check the *result* of
ToNewUnicode to see if you ran out of memory?

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 163973 [details] [diff] [review]
Revised patch v4

That'll teach me to do these things when I'm half asleep. New patch coming.

Comment 39

[Mark Banner (:standard8)]

Attachment: Revised patch v5

Ok, corrected the check for output of ToNewUnicode.

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 163240 [details] [diff] [review]
Patch to allow KB/MB to be localized

Obsoleting original patch, now revised one is reviewed.

Comment 41

[David :Bienvenu]

Comment on attachment 164109 [details] [diff] [review]
Revised patch v5

Neil, shouldn't

 kKiloByteString =
nsCRT::strdup(NS_LITERAL_STRING("kiloByteAbbreviation").get());

be instead:

kKiloByteString = ToNewUnicode(NS_LITERAL_STRING("kiloByteAbbreviation"));

+  if (!*aSizeString)
+    return NS_ERROR_OUT_OF_MEMORY;
+  else
+    return NS_OK;

this can be:

return (*aSizeString) ? NS_OK : NS_ERROR_OUT_OF_MEMORY

Comment 42

[David :Bienvenu]

Comment on attachment 164109 [details] [diff] [review]
Revised patch v5

in addition to the afore-mentioned change, you need to make a corresponding
change to the free code - it needs to be freed with nsMemory::Free

Comment 43

[Mark Banner (:standard8)]

Attachment: Patch v6

Revised patch with bienview's comments.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 164737 [details] [diff] [review]
Patch v6

Neil is happy not to r this patch again, therefore re-using his r.
Now requesting sr.

Comment 45

[David :Bienvenu]

Comment on attachment 164737 [details] [diff] [review]
Patch v6

thx!

Comment 46

[Mark Banner (:standard8)]

Fixed -> Patch checked into truck 2004-11-05 10:08.

Comment 47

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED: we now use kMegaByteString and kKiloByteString to represent MB
and KB hardcoded strings, respectively.  I used LXR for code inspection, and
looked at the various places we use/display them, and we're fine.

SeaMonkey build 2005-07-10-05 on Windows XP.