516787 - Message size in the main window should format large numbers (message size)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[mad.engineer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)
Build Identifier: 

Thunderbird 3.x or future versions should display large integer values in appropriate units with prefixes such as Mega (M), Giga (G) etc rather than showing it in the default option. Other email clients like Pine/Alpine/Outlook have this feature and it makes managing the InBox very easy based on the message size and spot large messages among others quickly. I posted this once in the respective newsgroups and the word that I received is that what constitutes a K, 1024 or 1000. Whatever the case may be, this feature should be considered.

Reproducible: Always

Comment 1

[Ludovic Hirlimann [:Usul]]

Marking new. I don't really see the point as we don't show the size of embeded attachment yet. But once we do that might be interesting to have.

Comment 2

[mad.engineer]

There is already an add-on for TB (Attachment Sizes 0.0.5) that allow the user to view the size of the attached files. I don't see a reason why this cannot be done for the message size itself, either directly or via an add-on.

Comment 4

[mad.engineer]

I noticed that this bug is marked as a duplicate of another one, but is this other fixed in the latest nightly builds of Thunderbird?

Comment 5

[[e-mail]]

Can you clarify? This bug doesn't seem to be marked as a dup?

Comment 6

[mad.engineer]

I mean to say that Bug 526879 has been marked as a duplicate of this bug. So I'm not sure if this bug will ever get fixed? Thanks

Comment 7

[[e-mail]]

That one is marked dup of this one, but that doesn't mean this won't get fixed.

Comment 8

[mad.engineer]

Is this bug on the "to be fixed list" for upcoming RC1 or beyond?

Comment 9

[[e-mail]]

I'm not the person to ask, but can you explain why you marked this as a major severity? I believe that's usually reserved for things that cause major confusion or problems for users. This but, while not great, is probably more of an enhancement than a major problem, unless I'm missing something...

Comment 10

[u142003]

(In reply to comment #1)
> Marking new. I don't really see the point as we don't show the size of embeded
> attachment yet. But once we do that might be interesting to have.

I don't know whether I misinterpreted the OP. But I interpret this bug as for the Size column in the thread pane only showing KB rather than showing sizes over 1000 KB (or 1024 KB) as MB. 

It would be better if the Size column said "28.6 MB" instead of "28648KB"

Comment 11

[mad.engineer]

This is exactly what I mean by this bug. Thunderbird only shows the message size in the size column in Kb and this is just plain not right. I hate to keep bringing up other email clients, but outlook and Pine both have this capability by default built-in, making the process of managing large mails very easy.

Comment 12

[Jim Porter (:squib)]

Attachment: Use MB in size column when >=1024KB

Here's a simple patch that uses the already-present localized string for MB. Maybe there's a function that does this formatting already, but I didn't see it.

Comment 13

[mad.engineer]

Unfortunately, I'm not a programmer but just an end user, so can't do much about this patch. May be you can check this patch in or give it to the Thunderbird developers, who can then review and add this to the upcoming 3.1a version?. That way, there's a good chance that it will make it in the next release.

Comment 14

[Jim Porter (:squib)]

Well, that's why dmose is the reviewer for the patch.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for the patch, Jim, and sorry for the delay getting to it.  Since this patch changes the user experience in way that's not purely a bugfix, it needs ui-review.  

In general, it's worth attaching some mockups or ASCII art prior to writing code, since the UI owner may make suggestions that are much easier to implement early on in the process.  In this case, I suspect that's not a big problem.  :-)
I'll post a before & after screenshot momentarily and request ui-review from Bryan on that.

Additionally, since the beginning of the year, we've switched over to requiring automated tests with patches in order for them to land in the tree.  We understand that this makes contributions require more upfront effort, but it saves a lot of pain later on.  Is this something you'd be up for doing for this bug?  

https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing is a reasonable starting point, and I and others are happy to help improve the documentation and the test-writing process if you run into roadblocks there.

In an ideal world, it would be easy to tweak the XPCShell test_nsMsgDBView.js, but <http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsMsgDBView.js#97> makes me pessimistic about that.  This could certainly be done with a MozMill test, however.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 423084 [details] [diff] [review]
Use MB in size column when >=1024KB

r+sr=dmose for the patch itself (this needs sr because it's in mailnews/).  Note that I can easily review simple patches to nsMsgDBView like this, but for future cases that are more complex, I think asuth and bienvenu are better choices, as they have their heads around that code much more deeply than I do, which means that they're likely to offer more comprehensive reviews more quickly than I can.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: old UI

UI as it looks today

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: proposed new UI

Comment 19

[[e-mail]]

Just a nitpick, but it looks like you're rounding down to 1MB in these images when it seems like rounding up is probably a better approach. That, or showing 1.6MB as opposed to 1650kb. 

Thanks for the patch though, this improvement (however it's implemented) is long overdue.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Good catch; rounding up does sound more right to me as well.  That's really a UI-review call, though, so my judgment isn't really the interesting one here.

Comment 21

[Jim Porter (:squib)]

Thinking about it more, I'm curious as to what Firefox does, since it needs a way of formatting sizes for the Downloads window. Maybe it would make more sense to piggyback on that, if possible.

Comment 22

[Jim Porter (:squib)]

Ok, the function is convertByteUnits: http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/downloads/src/DownloadUtils.jsm#382

Now I just need to figure out how to call that in C++ (or move the formatting out to Javascript).

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 425135 [details]
proposed new UI

kickass!

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Jim: this is such a small amount of code duplication, I'm not totally convinced that it's worth the effort (there's no easy way to call into JS modules from C++ that I'm aware of).  If it were me, I'd probably just switch to rounding up rather than rounding down, live with the duplication, and devote the time saved to writing an automated test.

Comment 25

[Jim Porter (:squib)]

Attachment: Use appropriate units for file sizes

Ok, I finally got around to working on this some more. I still haven't figured out how the tests work, though. I gather that I'll need to make a compiled test, (probably in mailnews/test), but the Makefile doesn't seem to be getting generated, and I have no idea how to run the tests anyway.

Anyway, I ended up reimplementing that convertByteUnits function in C++, which makes the formatting more consistent with Firefox (and most file browsers). I also found another place where this formatting happens and changed it there too: <http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#1844>. I don't actually know where that gets used, though... maybe it's a relic from when the folder pane had columns?

Comment 26

[Jim Porter (:squib)]

Attachment: Even newer UI

Here's what it looks like now.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #25)
> Created an attachment (id=426581) [details]
> Use appropriate units for file sizes
> 
> Ok, I finally got around to working on this some more. I still haven't figured
> out how the tests work, though. I gather that I'll need to make a compiled
> test, (probably in mailnews/test), but the Makefile doesn't seem to be getting
> generated, and I have no idea how to run the tests anyway.
> 

If the information on and linked to from <https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing#Core_Tests_expanded_for_Thunderbird> isn't sufficient, can you elaborate on what's missing there?

Personally, I'd bias towards writing a MozMill functional test in JS rather than a C++ unit test, but both are valuable, so I'll leave the choice up to you.

> I also found another place where this formatting happens and changed it there
> too:
> <http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#1844>.
> I don't actually know where that gets used, though... maybe it's a relic from
> when the folder pane had columns?

I'm sad to see that we still have RDF glop entrained in the folder code these days at all.  I suspect you're right that this is leftover from before the folder pane re-write.  bienvenu will likely know what it's still used for, if anything, so I'm adding him to the CC.

Comment 28

[Jim Porter (:squib)]

(In reply to comment #27)
> If the information on and linked to from
> <https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing#Core_Tests_expanded_for_Thunderbird>
> isn't sufficient, can you elaborate on what's missing there?
> 
> Personally, I'd bias towards writing a MozMill functional test in JS rather
> than a C++ unit test, but both are valuable, so I'll leave the choice up to
> you.

Well, my only problem (so far) is that comm-central/mailnews/test/Makefile isn't getting generated during the configure process, so the build system never realized I had a C++ test. I looked over the output from ./configure --help, but I didn't see anything relevant to my issue.

If I do a MozMill test though, this is probably all irrelevant.  Since I have no real preference either way, I'll probably try to do it via MozMill.

> I'm sad to see that we still have RDF glop entrained in the folder code these
> days at all.  I suspect you're right that this is leftover from before the
> folder pane re-write.  bienvenu will likely know what it's still used for, if
> anything, so I'm adding him to the CC.

I installed the custom folder columns add-on, thinking that it might grab the size from that function, but changing it didn't seem to do anything, so it might actually be dead code.

Comment 29

[Jim Porter (:squib)]

Hm, nevermind. I needed to put the C++ test in comm-central/mailnews/base/test. One question though: since this is mailnews code (and thus used by Seamonkey, I assume), would MozMill tests be enough?

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

It would be enough: the test-requirements come from the Thunderbird rules.  You're right that in an ideal world, we'd have both functional and unit tests.  If you're inclined to write both, go nuts!  That said, we're still new to the mandatory patch requirement world, and so we're trying not to make the barrier to landing too high until our patch-writing environments become significantly smoother.

Comment 31

[Jim Porter (:squib)]

Attachment: Now with MozMill test

Ok, here's the previous patch with MozMill tests for the byte, KB, and MB formatting. I figured it wouldn't be wise to great a 1 GB message for testing. :)

Comment 32

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 426582 [details]
Even newer UI

looks fine to me, thanks Jim!

Comment 33

[mad.engineer]

Would this bug make it into TB 3.1 final release?

Comment 34

[Jim Porter (:squib)]

Comment on attachment 426774 [details] [diff] [review]
Now with MozMill test

bienvenu, could you take a look at the nsMsgFolderDataSource stuff? I don't think GetFolderSizeNode is actually used anywhere (I couldn't find references to it, anyway). If it's unused, the diff is quite a bit simpler, which is always a good thing.

Thanks.

Comment 35

[David :Bienvenu]

Comment on attachment 426774 [details] [diff] [review]
Now with MozMill test

I think you'll need to change the corresponding suite version of messenger.properties to avoid breaking SeaMonkey.

Comment 36

[David :Bienvenu]

Comment on attachment 426774 [details] [diff] [review]
Now with MozMill test

thx very much for the patch.

need spaces around '=' and '<' here (two places):

+    for(PRUint32 i=0; i<sizeof(kByteUnitString)/sizeof(kByteUnitString[0]); i++)

r=me, modulo those nits, and adding the suite strings.

When you submit a new patch fixing those issues, I'd suggest requesting an sr from [e-mail] or standard8, so we can have the folder data source changes test with SeaMonkey.

Oh, and I need to go try the mozmill test...

Comment 37

[Jim Porter (:squib)]

Attachment: Fix nits, add suite strings

Ok, here's what I changed:
* fixed spacing nits
* add suite strings
* split the size format string out into two pieces to let localizers format the 
  size as "<units> <value>" (it had to be two format strings since it seems 
  like you can't mix positional specifiers with "*" for precision)
* update the test to fail when parseFloat on the size string returns a NaN

Comment 38

[[e-mail]]

Comment on attachment 430757 [details] [diff] [review]
Fix nits, add suite strings

>+byteAbbreviation=bytes
>+kiloByteAbbreviation2=KB
>+megaByteAbbreviation2=MB
>+gigaByteAbbreviation=GB
This is annoying, I wonder whether we should include the 2 suffix on them all.

>+byteSizeValue=%.*f
Why is this localised?

>+byteSizeAbbreviation=%1$S %2$S
Or the other alternative is to include the %S in the abbreviation above i.e.
%S bytes, %S KB, %S MB, %S GB.

>+    for(PRUint32 i = 0; i < sizeof(kByteUnitString)/sizeof(kByteUnitString[0]); i++)
Use NS_ARRAY_LENGTH here and in other similar places.

Comment 39

[[e-mail]]

Comment on attachment 430757 [details] [diff] [review]
Fix nits, add suite strings

>+byteAbbreviation=bytes
To me, showing sizes in bytes looks odd. Does anyone object to starting in KB?

Comment 40

[David :Bienvenu]

(In reply to comment #39)
> (From update of attachment 430757 [details] [diff] [review])
> >+byteAbbreviation=bytes
> To me, showing sizes in bytes looks odd. Does anyone object to starting in KB?

I tend to agree - plus the "bytes" string is long enough that you often can't see the whole column.

Comment 41

[[e-mail]]

(In reply to comment #40)
>(In reply to comment #39)
>>(From update of attachment 430757 [details] [diff] [review])
>>>+byteAbbreviation=bytes
>>To me, showing sizes in bytes looks odd. Does anyone object to starting in KB?
>I tend to agree - plus the "bytes" string is long enough that you often can't
>see the whole column.
Guess how I noticed it ;-)

Comment 42

[David :Bienvenu]

So, unless Bryan has a different opinion, Jim, we'd like to start with KB...it's relatively uncommon to have a message significantly less than 1KB anyway.

Comment 43

[Jim Porter (:squib)]

(In reply to comment #38)
> (From update of attachment 430757 [details] [diff] [review])
> >+byteAbbreviation=bytes
> >+kiloByteAbbreviation2=KB
> >+megaByteAbbreviation2=MB
> >+gigaByteAbbreviation=GB
> This is annoying, I wonder whether we should include the 2 suffix on them all.

Yeah, I wasn't sure what to do about that. In an ideal world, I'd be able to use the names sans number, but oh well.

> >+byteSizeValue=%.*f
> Why is this localised?

Oops, now that you mention it, that's probably not necessary. It's just a consequence of me trying to get something like "%1$.*f %2$S" working, then realizing I'd need two format strings. It never occurred to me that the size formatting isn't locale-specific!

> >+byteSizeAbbreviation=%1$S %2$S
> Or the other alternative is to include the %S in the abbreviation above i.e.
> %S bytes, %S KB, %S MB, %S GB.

That could work too. I don't really have a preference.

> >+    for(PRUint32 i = 0; i < sizeof(kByteUnitString)/sizeof(kByteUnitString[0]); i++)
> Use NS_ARRAY_LENGTH here and in other similar places.

Good, I knew there had to be a macro like that somewhere!

Comment 44

[[e-mail]]

Re: the discussion of whether to show bytes, I'll just say that nobody will care if a message is 52 bytes or if it's 1KB. It's all the same, really.

Relatedly, I wonder if the only time people care about message size is when they get attachments. Not to throw too much of a wrench in the works, but might it make sense to tie this in with the attachment column? I wonder how many people really care if the message they're reading is 100KB or 10. 

What if when there is an attachment, in the attachment column, you can see something like (paperclip_icon), 2MB. And the rest of the time, you don't need a column for both whether there is an attachment and to see how big that attachment is?

Comment 45

[Jim Porter (:squib)]

Originally, I wanted to be consistent with Firefox (and Linux/Windows file browsers), but that's probably not actually important. I could just give a minimum size of 0.1 KB and change the formatting a bit. It'd be pretty hard to get a message smaller than 100 bytes anyway.

From a personal perspective, I do actually use the size column (it's one of the first things I turn on in Thunderbird) as a quick way of determining whether I want to read particular emails right at that moment. Basically, it's an approximate measure of the amount of content in the email I'd need to read through if I clicked on it.

Comment 46

[[e-mail]]

Hmmm..that's interesting, since much of the message size is often a series of replies.

I can't help but wonder if that's just an edge case. TB is a rather cluttered browser - pulling something out of it  might make it seem smarter and more human-centric, rather than looking more like a spreadsheet app.

Just sayin'.

Comment 47

[Jim Porter (:squib)]

Luckily, a lot of the people I email are technical folks, so they're polite enough to trim emails when replying. That said, there probably is a better way to represent most of the data in the message list. I know I remove most of the default columns, since they present no useful information to me.

It might be worth making a separate bug for that, since I think there could be a lot of value in trying different organization schemes. For instance, collapsed threads could use some special handling, since their data in the message list is for the root message, but clicking on it shows data for the entire thread.

Comment 48

[Jim Porter (:squib)]

Attachment: Address review comments

Ok, this addresses the review comments from Neil. Bienvenu, I wasn't sure if I needed another regular review on this (or if I could just pull forward the r+ from before), so I erred on the side of caution. :)

Comment 49

[Jim Porter (:squib)]

Attachment: (Hopefully) final UI

Also, for completeness, here are all the ways numbers can be formatted. (Well, ok, not quite all. It excludes 10- and 100-MB messages, as well as anything in the GB range. But it should be a representative list.)

Comment 50

[David :Bienvenu]

Comment on attachment 431185 [details] [diff] [review]
Address review comments

oops, comment in unit test is wrong -

+/*
+ * Test that we can open and close a standalone message display window from the
+ *  folder pane.
+ */

I don't feel a great need to look at the patch again, unless Neil thinks I should (though I would like the comment fixed).

Comment 51

[Jim Porter (:squib)]

Attachment: Fix inaccurate unit test comment

Comment 52

[[e-mail]]

Comment on attachment 431200 [details] [diff] [review]
Fix inaccurate unit test comment

>-kiloByteAbbreviation=%dKB
>-megaByteAbbreviation=%dMB
>+# These are displayed in the message and folder pane windows
>+# LOCALIZATION NOTE %.*f is the abbreviated size in the appropriate units
>+kiloByteAbbreviation2=%.*f KB
>+megaByteAbbreviation2=%.*f MB
>+gigaByteAbbreviation2=%.*f GB
[Personally I don't like %.*f but then again it's no worse than %d!]

>-    if(msgSize < 1024)
>-      msgSize = 1024;

>+    if(unitSize < 0.1)
>+      unitSize = 0.1;
Nit: You were copying bad code but correct style has a space after the if.

>+                              (unitSize < 100) ? 1:0,
Nits: 1. more spacing please i.e. 1 : 0
      2. you don't need those ()s
      3. surely the comparison should be against 99.95 to allow for rounding?
Value	Patch	Desired
99.93	99.9	99.9
99.94	99.9	99.9
99.95	100.0	100
99.96	100.0	100
99.97	100.0	100
99.98	100.0	100
99.99	100.0	100
100.00	100	100
100.01	100	100

Comment 53

[Jim Porter (:squib)]

Attachment: Fix review comments

Comment 54

[Blake Winton (:bwinton) (:☕️)]

Hey Jim, I'ld really like to add the same sort of formatting to a few strings in a different patch.  Is there any chance you could expose a function to javascript-land that takes in a PRInt64 of the size in bytes, and returns some sort of nicely-formatted string?

(Also, adding GBs would be much nicer for my uses, if you can slide them in.)

Thanks,
Blake.

Comment 55

[Jim Porter (:squib)]

(In reply to comment #54)
> Hey Jim, I'ld really like to add the same sort of formatting to a few strings
> in a different patch.  Is there any chance you could expose a function to
> javascript-land that takes in a PRInt64 of the size in bytes, and returns some
> sort of nicely-formatted string?

If someone tells me how to do this, I could probably manage it.
 
> (Also, adding GBs would be much nicer for my uses, if you can slide them in.)

Luckily for you, this is already in there. It just doesn't appear in the images because I didn't want to create a 1 GB message. :)

Comment 56

[Blake Winton (:bwinton) (:☕️)]

I think if you move the code that does the formatting into a separate function, you can basically copy the implementation of GetMessageServiceFromURI.

http://mxr.mozilla.org/comm-central/ident?i=GetMessageServiceFromURI has many details.

(Define the function in mailnews/base/util/nsMsgUtils.cpp.
 Add a method in mailnews/base/src/nsMessenger.cpp which calls it.
 Add the signature in mailnews/base/public/nsIMessenger.idl so that js code can call it.
 I think you can copy the signature for getMsgUriAtNavigatePos.)

I'm not an expert, but if you run into any questions, I'll do my best to help.

Also, I saw the 1GB stuff in the code, _after_ I posted my comment.  Doh.  :)

Thanks,
Blake.

Comment 57

[Jim Porter (:squib)]

A question and a comment:

Question: Is there a reason I should prefer PRInt64 to PRUint64? File size *should* only be non-negative, but I'm not familiar enough with TB's C++ to know if it matters.

Comment: If I'm going to make this a generic function, I think I'll also add in a boolean parameter to show small values in bytes or KB (e.g. "100 bytes" vs "0.1 KB"). The latter is necessary for this patch, but may not be universally applicable.

Comment 58

[Blake Winton (:bwinton) (:☕️)]

Uh, it looks like you can use "unsigned" in the IDL, so perhaps it'll be okay.  On the other hand, I might use this to display how much space you'll save, which could be negative if you won't save any space, so perhaps letting it take negative numbers is a good idea.  On the other other hand, I should probably be handling that in my code, and displaying completely different text, with positive values in both cases.

tl;dr: Nah, either PRInt64 or PRUInt64 would work for me.

Thanks,
Blake.

Comment 59

[Jim Porter (:squib)]

Attachment: WIP of scriptified version

Here's a WIP of the version with JS bindings. I'm not actually sure this is the ideal way to do it, so I'm putting it up for people to look at.

1) I get strings out of a string bundle every time I call FormatFileSize. Is there a better way to do this? (Maybe put all the code in nsMessenger?)

2) I'm not sure what the difference between all the various string types are. I just picked the one that matches nsTextFormatter::ssprintf.

3) I'm not sure how to specify the value of an optional argument in IDL. Is it always false/null? (In that case I'd have to switch showBytes to useKB or something).

Comment 60

[Blake Winton (:bwinton) (:☕️)]

Attachment: New Migration Assistant.

Well, it looks like I can call the method from my code, so I'm happy with it, for what that's worth.  ;)

Thanks,
Blake.

Comment 61

[Jim Porter (:squib)]

Comment on attachment 433171 [details] [diff] [review]
WIP of scriptified version

Neil, could you take a look at this again? Especially re: my concerns in comment 59. Thanks!

Comment 62

[[e-mail]]

(In reply to comment #59)
> 1) I get strings out of a string bundle every time I call FormatFileSize. Is
> there a better way to do this? (Maybe put all the code in nsMessenger?)
nsMessenger isn't a service, so this wouldn't work, but you could arrange to only get the string that you actually use.

> 2) I'm not sure what the difference between all the various string types are. I
> just picked the one that matches nsTextFormatter::ssprintf.
There are basically 5 IDL string types; string maps to char*; wstring maps to PRUnichar*; ACString and AUTF8String both map to nsACString& but AUTF8String must be UTF-8 while ACString is everything else, and AString maps to nsAString&. Note that you can't actually create strings of these abstract types, instead the caller typically passes in an ns(C)(Auto)String object.

> 3) I'm not sure how to specify the value of an optional argument in IDL. Is it
> always false/null? (In that case I'd have to switch showBytes to useKB or
> something).
Yes, it's always null/false/0/""/whatever. (And whether you switch it depends of course on what you want the default to be.)

Comment 63

[[e-mail]]

Comment on attachment 433171 [details] [diff] [review]
WIP of scriptified version

>+    AString formatFileSize(in unsigned long long aPos, [optional] in boolean aShowBytes);
You need to change the uuid as well.

>+    FormatFileSize(msgSize, false, formattedSizeString);
>+    NS_ENSURE_SUCCESS(rv, rv);
You forgot to set rv ;-)

>+  // XXX: perhaps this should be done once, instead of every time this is called?
Maybe if the caller passed in a sizeAbbr array, and if you didn't find the abbreviation that you wanted already in the array then you could look it up?

>+  float unitSize = size;
JavaScript doesn't actually have a 64-bit type, and uses float internally, which XPConnect then converts to a 64-bit int for the call. On the other hand, I guess you don't want people trying to format 2.5 bytes ;-)

Comment 64

[Jim Porter (:squib)]

Attachment: Scriptified Version

Ok, here's a somewhat better-organized version of the previous patch. It gets rid of the unnecessary retrievals from the stringbundle and makes the default for FormatFileSize to be to show bytes. This version has the side benefit of making the size formatting functions in nsMsgDBView and nsMsgFolderDataSource much simpler.

I could probably add an xpcshell test for the new nsIMessenger function, but I've had about enough of this patch, so I'll hold off unless someone thinks it necessary. :)

Comment 65

[[e-mail]]

Comment on attachment 434366 [details] [diff] [review]
Scriptified Version

>+static const PRUnichar *gSizeAbbrNames[] = {
>+  NS_LITERAL_STRING("byteAbbreviation2").get(),
>+  NS_LITERAL_STRING("kiloByteAbbreviation2").get(),
>+  NS_LITERAL_STRING("megaByteAbbreviation2").get(),
>+  NS_LITERAL_STRING("gigaByteAbbreviation2").get(),
>+};
I'm not actually sure that this is supported in general (obviously it may work on specific compilers and/or build options, such as yours.)

Comment 66

[Jim Porter (:squib)]

I can't speak to what actual compilers do, but barring any preprocessor wackiness, that should be valid C++. Nevertheless, if there's a better/more idiomatic way to do that, let me know and I'll change it.

Comment 67

[Mark Banner (:standard8)]

(In reply to comment #65)
> (From update of attachment 434366 [details] [diff] [review])
> >+static const PRUnichar *gSizeAbbrNames[] = {
> >+  NS_LITERAL_STRING("byteAbbreviation2").get(),
> >+  NS_LITERAL_STRING("kiloByteAbbreviation2").get(),
> >+  NS_LITERAL_STRING("megaByteAbbreviation2").get(),
> >+  NS_LITERAL_STRING("gigaByteAbbreviation2").get(),
> >+};
> I'm not actually sure that this is supported in general (obviously it may work
> on specific compilers and/or build options, such as yours.)

I'd be a bit concerned that it would cause leaks to be reported. iirc you're not meant to have static ns*String as it doesn't get cleaned up properly. NS_LITERAL_STRING uses ns*String as its basis so we may run into trouble here.

Comment 68

[Jim Porter (:squib)]

Would static const char *gSizeAbbrNames[] = { "byteAbbreviation2", ... }; be suitable (then doing the NS_LITERAL_STRING stuff when I load the real string from the bundle)? That was my first instinct, but I went this way because it seemed that char* was frowned upon.

What's the overall priority on this bug? It seems like Blake wants to use it for the migration doohickey, and that's one of the things holding up Lanikai Beta 2, so is this indirectly holding things up? Maybe this should block that bug?

Comment 69

[Blake Winton (:bwinton) (:☕️)]

I think it would be a pretty big win to get it in, but if it doesn't land, I'll just change my code to do the classic "x/1024/1024+'MB'" trick, so I don't think it's blocks, per se.

But Bryan might disagree with me, so we should check with him first.

Thanks,
Blake.

Comment 70

[[e-mail]]

Comment on attachment 434366 [details] [diff] [review]
Scriptified Version

If memory serves me right, the issue with static NS_LITERAL_STRING is that the destructor lives in a different dynamic library and you can't guarantee library unload order.

Perhaps instead of adding formatFileSize to nsMsgUtils you could add it to nsIMsgMailSession, since it is a service so your other users can easily reference it.

Comment 71

[Jim Porter (:squib)]

Would it make sense to make the strings non-static and just put them inside that function? I ask because this would be easier for me. :) Though if nsIMsgMailSession is truly the best place for something like this to go, I won't object to moving it.

Comment 72

[[e-mail]]

Well, I guess I could be unpopular and suggest using GetStringByID, otherwise the other alternative seems to be a bunch of ?:s (which is similar to your suggestion but avoids "creating" strings you don't use.)

Comment 73

[Jim Porter (:squib)]

If I'm "allowed" to create a static const char* array, I could just call NS_LITERAL_STRING on the relevant char* string. I'm not sure if that's bad style in Mozilla, though.

Comment 74

[[e-mail]]

Sorry, but NS_LITERAL_STRING doesn't actually work like that; where supported it uses the preprocessor to generate a UTF16 string directly.

dmose came up with a number of ideas that would have worked had base/util been a module, which it unfortunately is not. The only one of any use is a shutdown observer, which basically means writing a private object whose job it is to own the strings, and registering it to get notified about shutdown so that it can delete them. This is slightly easier than getting nsIMsgMailSession to do it in that the other two callers can still call the function directly.

Comment 75

[Jim Porter (:squib)]

Well, if that's the easiest I can do that. How do I register the object so all that works? Also, is this actually worth the work compared to making the array of key strings a local variable? I'm not sure how much slower that would be.

Comment 76

[Jim Porter (:squib)]

Attachment: Fix NS_LITERAL_STRING issues

How about this (making the array of key strings a local variable)? It's not the most efficient way to go about this, but it's simple and doesn't cause any actual performance problems in my testing.

This patch also fixes some minor bitrot.

Comment 77

[[e-mail]]

(In reply to comment #75)
> Well, if that's the easiest I can do that. How do I register the object so all
> that works? Also, is this actually worth the work compared to making the array
> of key strings a local variable? I'm not sure how much slower that would be.
Sorry, but I've been busy recently and forgot to keep up with this bug. I'm not actually sure it would be worth the work.

Comment 78

[[e-mail]]

Comment on attachment 440302 [details] [diff] [review]
Fix NS_LITERAL_STRING issues

>+  const PRUnichar *sizeAbbrNames[] = {
>+    NS_LITERAL_STRING("byteAbbreviation2").get(),
>+    NS_LITERAL_STRING("kiloByteAbbreviation2").get(),
>+    NS_LITERAL_STRING("megaByteAbbreviation2").get(),
>+    NS_LITERAL_STRING("gigaByteAbbreviation2").get(),
>+  };
You can't actually use them inline like that (the return value of .get may not outlive the string which dies at the end of the statement.) You would have to declare them as separate NS_NAMED_LITERAL_STRING local variables first.

Comment 79

[[e-mail]]

Comment on attachment 440302 [details] [diff] [review]
Fix NS_LITERAL_STRING issues

>diff -r 329eb8f77774 mail/test/mozmill/message-header/test-display-names.js
[This doesn't seem to be the right test?]

>@@ -99,7 +100,6 @@
> #define ILLEGAL_FOLDER_CHARS ";#"
> #define ILLEGAL_FOLDER_CHARS_AS_FIRST_LETTER "."
> #define ILLEGAL_FOLDER_CHARS_AS_LAST_LETTER  ".~ "
>-
> #define FOUR_K 4096
Nit: don't delete that blank line

sr=me if you switch to NS_NAMED_LITERAL_STRING

Comment 80

[Jim Porter (:squib)]

Attachment: Address review comments

(In reply to comment #79)
> (From update of attachment 440302 [details] [diff] [review])
> >diff -r 329eb8f77774 mail/test/mozmill/message-header/test-display-names.js
> [This doesn't seem to be the right test?]

Wow, I attached the wrong tests in two different bugs now! Fixed.

> >@@ -99,7 +100,6 @@
> > #define ILLEGAL_FOLDER_CHARS ";#"
> > #define ILLEGAL_FOLDER_CHARS_AS_FIRST_LETTER "."
> > #define ILLEGAL_FOLDER_CHARS_AS_LAST_LETTER  ".~ "
> >-
> > #define FOUR_K 4096
> Nit: don't delete that blank line

Fixed.

> sr=me if you switch to NS_NAMED_LITERAL_STRING

This should be fixed, too. (I hope.)

Comment 81

[Ludovic Hirlimann [:Usul]]

(In reply to comment #80)
> Created an attachment (id=440548) [details]
> Address review comments

So do you want a new review ? or should this be marked checkin-needed ?

Comment 82

[Jim Porter (:squib)]

I don't actually know. I guess it's checkin-needed now, but I'm still not 100% sure about review policies, etc.

Comment 83

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 440548 [details] [diff] [review]
Address review comments

The r+ and sr+ were given in ways that suggest they were intended to be carried forward.  So the only reason to get a re-review is if, in your judgment, things have changed enough to that it's likely to be particularly helpful.

Otherwise, just set r+ and sr+ on this patch, and add the checkin-needed keyword.

Because we're already after string freeze, we're only taking critical bugs with strings now, which means that this can land on comm-central (but not comm-central-1.9.2) after we branch and will first ship in the 3.2 release.

Comment 84

[Jim Porter (:squib)]

Ok, it's done. I didn't figure this would make 3.1 (even with the delay for B2), but it'll be good to have for 3.2. (I already miss the formatting when I go back to my "real" TB installation!)

Comment 85

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/11e0ed535d81

Comment 86

[[e-mail]]

Bada boom!

Comment 87

[Rimas Kudelis [use needinfo to ping]]

Is there any chance to have the "%.*f bytes" string support plural forms? Note: since it's bytes, it will never have a decimal part.

Comment 88

[Mark Banner (:standard8)]

(In reply to comment #87)
> Is there any chance to have the "%.*f bytes" string support plural forms? Note:
> since it's bytes, it will never have a decimal part.

That would require bug 477831...

Comment 89

[Rimas Kudelis [use needinfo to ping]]

doh, I thought since it's a .properties file, this would work...

Comment 90

[Jim Porter (:squib)]

(In reply to comment #87)
> Note: since it's bytes, it will never have a decimal part.

Yeah I kinda cheated with that. It could technically be %d, but then I'd have to special-case the handling for bytes even more than I already did. It'd probably be worth changing that if there were a plural form, though. (Maybe by then this code will be in core too (!), since it would resolve a couple minor bugs with Firefox's handling of file sizes.)

Comment 91

[Rimas Kudelis [use needinfo to ping]]

Adding dependency on bug 477831. If that will ever be fixed, we may want to update this bug too.

Comment 92

[Jim Porter (:squib)]

Comment on attachment 440548 [details] [diff] [review]
Address review comments

Since bug 559559 is looking to get into 3.2, this will need to come along for the ride. Setting flags.

Comment 95

[Hugo Osvaldo Barrera]

As far as I understand, this calculates 1KB = 1024B?
That's WRONG, 1KB = 1000B, 1KiB = 1024B.
Please see

http://en.wikipedia.org/wiki/International_System_of_Units
http://en.wikipedia.org/wiki/Kibibyte

You can't use one system of measurement for the calculation, and another to display along that number in the UI, this will show information that is WRONG.
Most OSes, file managers, etc, use KiB and MiB nowdays.

Comment 96

[Blake Winton (:bwinton) (:☕️)]

Hugo,

since this bug has been resolved, this is the wrong place to discuss your change request.  Please open a new bug, and make your case there.

(Although, I will note that the operating system I use displays KB, MB, and GB for powers of 1024, so you might want to provide screenshots of several operating systems which use KiB, MiB, etc…)

Thank you,
Blake.

Comment 97

[Hugo Osvaldo Barrera]

Please type
sudo fdisk -l

Here's an example:
---
fdisk -l /dev/sdb1
Disk /dev/sdb1: 2000.4 GB, 2000397795328 bytes
[...]
---

If you do the math:
2000397795328/1000000000 = 2000.4
Clearly, they are using this SI measures properly.

Did you look at these wikipedia articles?  They clearly describe how much 1K is, and that is 1000.  Kids see this at school as well (excepto in 3 countries in the world which do not use the metric system).

There isn't much to argue on this issue; the sizes are calculated properly using kibi/mebibytes, but the UI shows the wrong units.

Comment 98

[Blake Winton (:bwinton) (:☕️)]

This bug is not the place for this discussion.  Feel free to file a new bug (and be sure to CC me on it) if you'ld like to continue the discussion.  Either way, this will be my last comment in this bug.

Thanks,
Blake.

Comment 99

[Rimas Kudelis [use needinfo to ping]]

I think the right thing would be to actually calculate in KB and not in KiB.

Hugo, please CC me on that bug too (or just mention its number here).

Comment 100

[mad.engineer]

Adding this comment because I'm not sure if a new bug should be opened for this or else. I can open a new if someone confirms this first. Noticed today that a message with one single attachment file show different sizes in both TB7 and Outlook 2007. So, I'm not sure if TB is using some wrong calculation to show the size or else?. The message when opened in TB7 (IMAP, W7) show an excel file attachment of size 10.7MB with the overall message size seen in the main InBox as 14.6MB. Whereas the exact same messages when opened using Outlook 2007 show the overall message size of just 11MB ?. That's a whole lot of difference between the two of 3.6MB !. Can someone shed some light here?. Thanks

Comment 101

[Jim Porter (:squib)]

The attachment is almost certainly base64-encoded, which has an overhead of approximately 33%, or 3.56 MB in this case. Chances are Outlook is calculating the total size after base64-decoding, which is either more or less accurate, depending on your point of view. I prefer Thunderbird's way though, since the base64-encoded size is the on-disk size of the whole message, so it's what your mail server will use when calculating quotas.

Comment 102

[Djordje Dobrijevic]

Personaly, this change in format of message size was disappointing.
For me, it is much better to have message size in KB for each message. Then it is very easy to compare size of messages, which is very useful in case of great number of messages in one folder.
I was trying and to trying to find possibility to get back old message size format (size only in KBs). There is nothing in program options, I tried to find something in Config Editor and also I tried to find some plug-in, but without success.

Since there are so many opinions format of message size should be offered as option. I propose to add possibility for user to choose what kind of format to use (bytes, KB, MB or combination of units depending on size).

Comment 103

[Blake Winton (:bwinton) (:☕️)]

Djordje, this bug is closed so I'm afraid we aren't going to consider further proposals here.  If you'ld like to file a new bug asking for this, please cc me on it, and we can talk about it there.  :)

(Having said that, my gut feeling is that when people want to compare message sizes, they'll sort the messages by size first, making this feature less likely to be needed.  I'm also not a huge fan of adding options to Thunderbird, largely because we have the ability for people to write and install add-ons.)

Thanks,
Blake.

Comment 104

[Sebastian Lisken]

To anybody coming to this bug in the hope of getting the new format reversed: I raised the question of solving it via an add-on in bug #614566 and Jim Porter very kindly produced one from some defunct code he had lying around. That should solve it. See https://addons.mozilla.org/en-US/thunderbird/addon/size-in-kb/ (works for SeaMonkey too).

Comment 105

[Hugo Osvaldo Barrera]

(In reply to Sebastian Lisken from comment #104)
> To anybody coming to this bug in the hope of getting the new format
> reversed: I raised the question of solving it via an add-on in bug #614566
> and Jim Porter very kindly produced one from some defunct code he had lying
> around. That should solve it. See
> https://addons.mozilla.org/en-US/thunderbird/addon/size-in-kb/ (works for
> SeaMonkey too).

Currently, thunderbird calculates sizes en KiB (1024B = 1KiB), and the labels it as kB (1000B = 1kB).

Does this addon make the the calculation consistent with the labels? Otherwise, could you be a bit more specific as to what this addon does, it seems a bit unclear to me.