Last Comment Bug 768621 - show the size of a mail folder on disk somewhere in the Thunderbird UI
: show the size of a mail folder on disk somewhere in the Thunderbird UI
Status: RESOLVED FIXED
[parity-Outlook]
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 17.0
Assigned To: :aceman
:
Mentors:
Depends on: 786022 786023
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-26 13:22 PDT by :aceman
Modified: 2012-08-27 13:16 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (3.90 KB, patch)
2012-06-26 14:27 PDT, :aceman
bwinton: ui‑review-
Details | Diff | Review
patch v2 (5.85 KB, patch)
2012-06-27 12:06 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v3 (5.64 KB, patch)
2012-06-27 14:45 PDT, :aceman
iann_bugzilla: review+
bwinton: ui‑review+
Details | Diff | Review
patch v4 (5.56 KB, patch)
2012-07-18 13:25 PDT, :aceman
squibblyflabbetydoo: review+
Details | Diff | Review
patch v5 (6.28 KB, patch)
2012-07-30 12:21 PDT, :aceman
squibblyflabbetydoo: review+
iann_bugzilla: feedback+
Details | Diff | Review
patch v6 (6.40 KB, patch)
2012-08-04 10:25 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review

Description :aceman 2012-06-26 13:22:35 PDT
Show the size of a mail folder on disk somewhere in the Thunderbird UI, my proposal is for folder -> Properties... -> General information.

I understand this information was intentionally removed from the folder pane. But there should at least be some hidden way to show the size to the user. Even Outlook has this possibility.

Maybe this is applicable to Seamonkey, even though it still has the size in the folder pane.
Comment 1 :aceman 2012-06-26 14:27:38 PDT
Created attachment 636875 [details] [diff] [review]
WIP patch

This is a first stab at this. I am not sure if the values should be in a textbox, but that allows them to be selectable (copyable) by mouse. Is there a way to make a label selectable?

(Of course I will add the strings into the locale dtd, this patch is just to get UI discussion.)
Comment 2 Mark Banner (:standard8) 2012-06-26 14:31:01 PDT
A nicer (though more complex) way would be to integrate Mail Summaries into Thunderbird...
Comment 3 Jim Porter (:squib) 2012-06-26 15:03:42 PDT
aceman, you should probably be using nsIMessenger.formatFileSize to display the size. That'll give you the most sensible units instead of just using KB.

(In reply to Mark Banner (:standard8) from comment #2)
> A nicer (though more complex) way would be to integrate Mail Summaries into
> Thunderbird...

One day... :) (Though Mail Summaries doesn't show all folders, and I don't expect it ever will.)
Comment 4 :aceman 2012-06-26 22:03:32 PDT
Thanks. But I am not sure I want the unit to change per folder. There are enough complaints about similar feature of Extra folder columns at https://addons.mozilla.org/sk/thunderbird/addon/extra-folder-columns/reviews/ .
Comment 5 Jim Porter (:squib) 2012-06-26 22:12:47 PDT
(In reply to :aceman from comment #4)
> Thanks. But I am not sure I want the unit to change per folder. There are
> enough complaints about similar feature of Extra folder columns at
> https://addons.mozilla.org/sk/thunderbird/addon/extra-folder-columns/reviews/
> .

Since this is the properties dialog, which should only show one folder at a time, the criticisms there shouldn't apply. Besides that, we use variable units for pretty much every other file size in Thunderbird, so we should be consistent. Add-ons are, of course, free to change the behavior.
Comment 6 :aceman 2012-06-26 22:18:48 PDT
OK, anything is better than nothing :)

How can I retrieve the messenger object from inside the folderProps.js file?
Comment 7 Jim Porter (:squib) 2012-06-26 22:22:38 PDT
You'll want:

let gMessenger = Cc["@mozilla.org/messenger;1"].
                   createInstance(Ci.nsIMessenger);
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-06-27 09:15:58 PDT
Comment on attachment 636875 [details] [diff] [review]
WIP patch

In general, I like the idea, but I've got a few suggestions before I give it the ui-r+…

I think we should tighten up the space between the size and the "KB", and get that second phrase to line up with the end of the text box above it.  So it would end up looking something like http://dl.dropbox.com/u/2301433/Screenshots/diskSize.png

As for making the text selectable, I guess that would be a good idea, but I'm not sure how many people would use the information, and what they would use it for.  Having to type out "17 MB" doesn't seem that terrible to me, and perhaps if it's actually useful information, we might want to put it in Troubleshooting Information…

Thanks,
Blake.
Comment 9 :aceman 2012-06-27 12:06:35 PDT
Created attachment 637206 [details] [diff] [review]
patch v2

Thanks, I think I addressed all issues.
Comment 10 Blake Winton (:bwinton) (:☕️) 2012-06-27 13:44:11 PDT
Comment on attachment 637206 [details] [diff] [review]
patch v2

Much better.  Not quite there, as you can see at the right of http://dl.dropbox.com/u/2301433/Screenshots/BetterDiskSize.png but I trust you can fix it without further meddling by me.  ;)  So ui-r=me.

Thanks,
Blake.
Comment 11 David :Bienvenu 2012-06-27 13:51:44 PDT
just a note that maildir doesn't implement folder size (cuz it's a bit more complicated that just fstat'ing a mailbox file). I think the maildir impl would probably just sum up the message sizes in the .msf file instead of fstating all the message files.
Comment 12 :aceman 2012-06-27 14:22:21 PDT
Bwinton, I can't actually fix that easily as there is a <textbox> with hidden borders that has a size = length("2.9 MB") in you case. That uses some average font width and is always larger than the value it contains. It is visible in all the other textboxes of type="number" (with the 2 small arrows). They are larger than the max value they take by 1 digit.

Can I align the contents of the textbox to right?
Comment 13 Blake Winton (:bwinton) (:☕️) 2012-06-27 14:26:07 PDT
I think it's possible.  Or just replace the whole thing with a right-aligned label, since I don't think we need to let people copy/paste that text…
Comment 14 :aceman 2012-06-27 14:45:24 PDT
Created attachment 637266 [details] [diff] [review]
patch v3
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-06-27 18:56:58 PDT
Comment on attachment 637266 [details] [diff] [review]
patch v3

Yep, looks good.  ui-r=me.

Later,
Blake.
Comment 16 Ian Neal 2012-07-07 07:46:13 PDT
Comment on attachment 637266 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/content/folderProps.js
>+  // set folder sizes
>+  let numberOfMsgsElem = document.getElementById("numberOfMessages");
>+  let numberOfMsgs = gMsgFolder.getTotalMessages(false);
>+  numberOfMsgsElem.value = numberOfMsgs < 0 ? "???" : numberOfMsgs;
>+
>+  let sizeOnDiskElem = document.getElementById("sizeOnDisk");
>+  let sizeOnDisk;
>+  try {
>+    sizeOnDisk = Components.classes["@mozilla.org/messenger;1"]
>+                           .createInstance(Components.interfaces.nsIMessenger)
>+                           .formatFileSize(gMsgFolder.sizeOnDisk, true);
>+  } catch (e) {
>+    sizeOnDisk = "???";
>+  }
>+  sizeOnDiskElem.value = sizeOnDisk;
Nit: You could inline sizeOnDiskElem

>+++ b/mailnews/base/content/folderProps.xul
>+  onload="folderPropsOnLoad();" style="width: 36em;"
Nit: Shouldn't widths be measured in ch rather than em?

r=me with those addressed / fixed.
Comment 17 :aceman 2012-07-07 10:45:33 PDT
No idea about the "ch". It is not my code and I can't answer that. Maybe bwinton knows about these theme guidelines.
Comment 18 :aceman 2012-07-18 13:25:11 PDT
Created attachment 643554 [details] [diff] [review]
patch v4

Thanks, changed to '56ch' (seemed about the same width at my font) and inlined both element variables (those were remnants from previous versions where they were needed).
Comment 19 Jim Porter (:squib) 2012-07-29 16:44:29 PDT
Comment on attachment 643554 [details] [diff] [review]
patch v4

Review of attachment 643554 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! r=me with the comments below addressed.

::: mailnews/base/content/folderProps.js
@@ +265,5 @@
>    document.getElementById("retention.useDefault").checked = retentionSettings.useServerDefaults;
>  
> +  // set folder sizes
> +  let numberOfMsgs = gMsgFolder.getTotalMessages(false);
> +  document.getElementById("numberOfMessages").value = numberOfMsgs < 0 ? "???" : numberOfMsgs;

This string should be localized; I'd prefer "unknown" instead.

@@ +273,5 @@
> +    sizeOnDisk = Components.classes["@mozilla.org/messenger;1"]
> +                           .createInstance(Components.interfaces.nsIMessenger)
> +                           .formatFileSize(gMsgFolder.sizeOnDisk, true);
> +  } catch (e) {
> +    sizeOnDisk = "???";

Ditto here; it's more important to localize this one, since newsgroups don't report a size.
Comment 20 David :Bienvenu 2012-07-29 17:22:09 PDT
just an FYI - the size on disk attribute isn't yet supported for the maildir pluggable store. My suggestion for its implementation would be to simply sum up the message size of the headers in the .msf file, rather than statting all the .eml files.
Comment 21 :aceman 2012-07-29 22:14:20 PDT
I don't think I am able to do what you suggest, David. Will maildir return the "unknown" reply (or exception) in the current state? If yes, I think it would be enough here.
If maildir has unimplemented .sizeOnDisk then it should be fixed there, not in this bug.
Comment 22 :aceman 2012-07-30 12:21:00 PDT
Created attachment 647247 [details] [diff] [review]
patch v5

Would this work? I use entities as I didn't want to put it in some .properties file where it would be disconnected from the added strings in folderProps.dtd. As the translation depends heavily on the strings after which they come in the dialog. That is also why I added a string for each "unknown" (the size one may be translated differently than the number one).
Comment 23 Jim Porter (:squib) 2012-08-04 01:34:37 PDT
Comment on attachment 647247 [details] [diff] [review]
patch v5

Review of attachment 647247 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following changes (note that I didn't run Thunderbird with this latest version, but it's a minor change). You may want to ping bwinton to see if he prefers "unknown" vs. "Unknown" for the strings. I (not strongly) prefer "Unknown".

::: mailnews/base/content/folderProps.js
@@ +275,5 @@
> +                           .createInstance(Components.interfaces.nsIMessenger)
> +                           .formatFileSize(gMsgFolder.sizeOnDisk, true);
> +  } catch (e) { }
> +  if (sizeOnDisk)
> +    document.getElementById("sizeOnDisk").value = sizeOnDisk;

I this this would make a lot more sense like so:

  try {
    let sizeOnDisk = Components.classes["@mozilla.org/messenger;1"]
                               .createInstance(Components.interfaces.nsIMessenger)
                               .formatFileSize(gMsgFolder.sizeOnDisk, true);
    document.getElementById("sizeOnDisk").value = sizeOnDisk;
  } catch (e) {}
Comment 24 Ian Neal 2012-08-04 05:51:52 PDT
Comment on attachment 647247 [details] [diff] [review]
patch v5

>+<!-- LOCALIZATION NOTE: When the number of messages can't be determined, this string is displayed as the number -->
>+<!ENTITY numberUnknown.label                     "unknown">
>+<!-- LOCALIZATION NOTE: When the size can't be determined, this string is displayed as the size -->
>+<!ENTITY sizeUnknown.label                       "unknown">
Do we really need two entities for unknown? I'll defer to Blake though.

>+  let sizeOnDisk = null;
>+  try {
>+    sizeOnDisk = Components.classes["@mozilla.org/messenger;1"]
>+                           .createInstance(Components.interfaces.nsIMessenger)
>+                           .formatFileSize(gMsgFolder.sizeOnDisk, true);
>+  } catch (e) { }

>+    document.getElementById("sizeOnDisk").value = sizeOnDisk;
As already suggested move this and the let into the try.
Comment 25 :aceman 2012-08-04 10:25:53 PDT
Created attachment 649012 [details] [diff] [review]
patch v6

The reasoning for "unknown" in lowercase is that is appears after a colon.
The reason for having 2 entities of "unknown" is that in some languages the translation depends on the sentence before the colon. E.g. the "size" and "number" subjects have different gender.
Bwinton, what do you think?
Comment 26 Blake Winton (:bwinton) (:☕️) 2012-08-08 06:12:28 PDT
Comment on attachment 649012 [details] [diff] [review]
patch v6

I think I agree with the reasoning behind the lowercase unknown, and having the two different unknowns.  ui-r=me.

Thanks,
Blake.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-08-13 18:21:31 PDT
https://hg.mozilla.org/comm-central/rev/3c84748b73e9
Comment 28 Stefan Plewako [:stef] 2012-08-22 08:25:00 PDT
For imap folders without sync/offline support enabled size on disk info seems to be completely wrong (at least for me with Daily 17 pl Mac 10.6.8).

Additionally, since it's labeled as folder size on disc and not all messages size, we should count .msf file size too.
Comment 29 :aceman 2012-08-26 14:10:23 PDT
Then you should file a new bug for that because you want to change how the backend .sizeOnDisk function works.
Comment 30 Stefan Plewako [:stef] 2012-08-27 03:04:07 PDT
(In reply to :aceman from comment #29)
> Then you should file a new bug for that because you want to change how the
> backend .sizeOnDisk function works.

No I don't, I just want the correct info…
Comment 31 :aceman 2012-08-27 03:12:24 PDT
But correct info is not the job of this bug. It just exposes whatever .sizeOnDisk function returns, with all its current limitations.
Comment 32 Stefan Plewako [:stef] 2012-08-27 03:55:18 PDT
(In reply to :aceman from comment #31)
> But correct info is not the job of this bug. It just exposes whatever
> .sizeOnDisk function returns, with all its current limitations.

I read summary of this bug and comment #0 differently - nothing about blindly reusing some function.

Some ways to fix this:
- display true size of the folder on disk (incorporating maybe .msf file size if significant)
- change the label to reflect what is currently displayed
- display total messages size on server and folder size on disk…
Comment 33 Wayne Mery (:wsmwk, NI for questions) 2012-08-27 05:07:19 PDT
Stefan, please file a new bug

Note You need to log in before you can comment on or make changes to this bug.