278096 - Show quota usage in main window

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Ben Bucksch (:BenB)]

Main mail window should show the quota usage in the status bar, if the user is
on quota on the (IMAP) server. Otherwise user won't notice his usage (and can't
adapt his usage patters to the permitted storage) until it's too late.

I am implementing this.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

Screenshot at <https://bugzilla.mozilla.org/attachment.cgi?id=171088>.


I decided against a general-purpose progressmeter with label (bug 278159), for
now.

Comment 2

[Ben Bucksch (:BenB)]

Spec:
It shows the quota as progressmeter in the lower right corner of the window, in
the staus bar, next to the "Total" msg count. Progressmeter is small, to save
screen real estate. Shows value in percent as label (with shadow, so that it's 
    visible independent of background - bar or remainder). If you click on it,
the folder properties dialog opens with the Quota tab, as a discovery feature
for people to find out what it means at all.

The display will be updated together with the Unread/Total msg count, which
happens only when you change the folder (it seems). An additional lag is
introduced by the IMAP backend code, so you may have to switch folders a few
times for it to update.

Comment 3

[Ben Bucksch (:BenB)]

Oh, and you can supress the display of the progressmeter, if the quota is below
a certain threshold, to not encourage users to use up their quota. There are
also thresholds when to make the bar yellow and red. (Defaults are 20, 80 and 95
percent, respectively.) If there's no quota information from the server, no
progressmeter is shown either.

Comment 4

[[e-mail]]

Comment on attachment 171104 [details] [diff] [review]
Fix, v1

I don't have access to a server that supports quotas...

>+pref("mailnews.quota.mainwindow_threshold.show", 20);
>+pref("mailnews.quota.mainwindow_threshold.warning", 80);
>+pref("mailnews.quota.mainwindow_threshold.red", 95);
Perhaps "critical" rather than "red"...

>+var UpdateStatusMessageCountsCache;
This variable name is a bit meaningless... perhaps gQuotaFeedback or something?

> function UpdateStatusMessageCounts(folder)
> {
>+  // get element references and prefs
>   var unreadElement = GetUnreadCountElement();
>   var totalElement = GetTotalCountElement();
>-  if(folder && unreadElement && totalElement)
>+  if(!(folder && unreadElement && totalElement))
>+    return;
>+  if (typeof(UpdateStatusMessageCountsCache) != "object")
Why not call an init function from CreateMailWindowGlobals?

>+  var cache = UpdateStatusMessageCountsCache;
I don't think you gain a lot by making a local reference.

>+  var imapFolderSink = folder
>+                  .QueryInterface(Components.interfaces.nsIImapMailFolderSink);
>+  if (imapFolderSink &&
>+      imapFolderSink.folderQuotaDataIsValid)
QueryInterface does not return null on error. Use instanceof or verify the
server type first instead. Additionally you need to hide the quota panel when
switching to account central (presumably in ShowAccountCentral) or to non-IMAP
folders.

>+      cache.quotaMeter.setAttribute("value", percent);
>+           // do not use value property, because that is inprecise (3%)
>+           // for optimization that we don't need here
"imprecise". Maybe we can get rid of that 'optimization'.

>+      var label = gMessengerBundle.getFormattedString("percent", [percent]);
nsMsgStatusFeedback.showProgress just appends a % sign (but check with l10n
folks)

>-<tabbox>
>+<tabbox id="folderPropTabBox">
You could remove the id from the <tabpanels>.

>+/**
>+ @param msgFolder  if null: selected
>+ @param tabID  initial tab
>+ */
I can't see this comment being helpful, sorry

>+                    tabID:tabID?tabID:"", name:name});
The ?: clause is unnecessary, because the dialog doesn't care about nulls.

>+#quotaMeter {
>+  max-width: 4em;
>+}
The min-width set by progressmeter.css should override this, but setting
min-width here would probably work.

>+  color: white;
Modern theme style is to use #FFFFFF.

>+  color: white;
Classic theme should always use a system colour.

Comment 5

[[e-mail]]

(In reply to comment #4)
>nsMsgStatusFeedback.showProgress just appends a % sign
Although sendProgress and printProgress do support i18n

Comment 6

[Ben Bucksch (:BenB)]

(In reply to comment #4)
> (From update of attachment 171104 [details] [diff] [review] [edit])
> I don't have access to a server that supports quotas...

I think I could supply you with an account/pw.

> Perhaps "critical" rather than "red"...

Right. I couldn't remember the word.

> >+var UpdateStatusMessageCountsCache;
> This variable name is a bit meaningless... perhaps gQuotaFeedback or something?

Well, the variable is only for this function, thus the name of the function in
there. (I forgot the prefix "g").

> > function UpdateStatusMessageCounts(folder)
> > {
> >+  // get element references and prefs
> >   var unreadElement = GetUnreadCountElement();
> >   var totalElement = GetTotalCountElement();
> >-  if(folder && unreadElement && totalElement)
> >+  if(!(folder && unreadElement && totalElement))
> >+    return;
> >+  if (typeof(UpdateStatusMessageCountsCache) != "object")
> Why not call an init function from CreateMailWindowGlobals?

I wanted to keep things local, in one place. Maybe I'll change it, though.

> >+  var cache = UpdateStatusMessageCountsCache;
> I don't think you gain a lot by making a local reference.

The idea is not speed, but code readability.

> >+  var imapFolderSink = folder
> >+                  .QueryInterface(Components.interfaces.nsIImapMailFolderSink);
> >+  if (imapFolderSink &&
> >+      imapFolderSink.folderQuotaDataIsValid)
> QueryInterface does not return null on error.

Uh. I thought so. What *does* it return on error?

> Use instanceof

hmpf, that's crap.

> Additionally you need to hide the quota panel when
> switching to account central (presumably in ShowAccountCentral) or to non-IMAP
> folders.

hm, true.

> "imprecise"

OK.

> You could remove the id from the <tabpanels>.

OK.

> >+/**
> >+ @param msgFolder  if null: selected
> >+ @param tabID  initial tab
> >+ */
> I can't see this comment being helpful, sorry

I should have said "initial tab. if null: first tab." It was important to
specify that the parameters are optional and what happens, if they are missing,
because all current callers call without arguments.

> >+                    tabID:tabID?tabID:"", name:name});
> The ?: clause is unnecessary, because the dialog doesn't care about nulls.

Good catch, I saw that after I submitted the patch.

What does document.getElementById(null) do? throw? return null?
document.getElementById("nonexistingID") returns null, right?

> >+#quotaMeter {
> >+  max-width: 4em;
> >+}
> The min-width set by progressmeter.css should override this, but setting
> min-width here would probably work.

I don't understand. This works fine for me, and the rule does have an effect (it
makes the difference between the normal size and the mini size you can see on
the screenshot). |width| didn't work because of the rules in progressmeter.css.

> >+  color: white;
> Modern theme style is to use #FFFFFF.

Gack. And I have to use the same uglyness? OK....

> >+  color: white;
> Classic theme should always use a system colour.

Which one?

Comment 7

[David :Bienvenu]

Or, you could just implement nsIMsgImapFolderProps in your extension and call
nsImapMailFolder::FillInFolderProps(nsIMsgImapFolderProps *aFolderProps)

then you don't have to touch any core code.

Comment 8

[Ben Bucksch (:BenB)]

Feels like somebody dragged the carpet from under my feet just when I'm done. :(
*sigh* I think the attributes were just fine, exactly as they should be, only in
the wrong interface (BTW: Nothing in nsIImapMailFolderSink tells me that I
should not use that).

> nsImapMailFolder::FillInFolderProps

I saw that interface and intentionally didn't use it, because it's ugly: push
from backend to frontend, and even worse with backend having knowledge of UI,
knowing which exact fields will be needed.

I guess I have to re-add the getters to nsIMsgImapMailFolder. I prefer single
attributes (cleaner, much nicer from JS), but oh well, I can implement a single
3-value getter. (The pushing of all these unnecessary values using 7 functions
in nsIMsgImapFolderProps, would surely be far worse than getting 3 attributes as
I do here.) Will do that next.

Comment 9

[Ben Bucksch (:BenB)]

Did so. I followed bienvenu's advice in bug 209545 comment 9 as far as I understood it.

---

(In reply to comment #4)
> QueryInterface does not return null on error. Use instanceof

fixed

> Additionally you need to hide the quota panel when
> switching to account central (presumably in ShowAccountCentral) or to non-IMAP
> folders.

fixed

> >+#quotaMeter {
> >+  max-width: 4em;
> >+}
> The min-width set by progressmeter.css should override this, but setting
> min-width here would probably work.

This comment didn't apply to Mozilla back then, but does apply now to current toolkit/. Fixed.

Comment 10

[Ben Bucksch (:BenB)]

Attachment: Fix, v2, Thunderbird

Ported Fix v1 to Thunderbird 2.0 branch. Readded interface to get quota as discussed above. Also fixed review comments.

Requesting review, please.

Comment 11

[Ben Bucksch (:BenB)]

Attachment: Screenshots

Screenshots of Thunderbird statusbar with no quota, 40% (normal) and 80% (warning). critical is the same as warning, just red instead of orange.

Also a screenshot of the folder properties dialog's quota pane. This opens when you click on the quota progressmeter. I have not modified the layout of this dialog, the screenshot is for illustration purposes only.

The changes for Seamonkey should be identical. I plan to do them when I passed review, to avoid work duplication.

Comment 12

[David :Bienvenu]

Comment on attachment 238718 [details] [diff] [review]
Fix, v2, Thunderbird

cool, generally looks OK. a few nits:

nsIMsgImapMailFolder.idl

when you change an interface, you need to generate a new uuid.

I don't quite understand the three vars for holding the quota information:

+    var c = new Object();

gUpdateStatusQuote_Cache

and var cache = gUpdateStatusQuota_Cache;

Do we really need all three, or can we just use one : gUpdateStatusQuota_Cache?

I haven't had a chance to try this patch out - I'll try it out as soon as I can.

Comment 13

[Ben Bucksch (:BenB)]

> cool, generally looks OK. a few nits:

Great!

> when you change an interface, you need to generate a new uuid.

Sure, I didn't know that.

> Do we really need all three [variables], or can we just use one :
> gUpdateStatusQuota_Cache?

c and cache are just aliases, because gUpdateStatusQuota_Cache is so ugly and long, and I need it a lot. I thought it was clear but I guess not. Should I make a comment or do you have a better idea to achieve the same?

> I haven't had a chance to try this patch out
> I'll try it out as soon as I can.

Thanks.

Comment 14

[David :Bienvenu]

Comment on attachment 238718 [details] [diff] [review]
Fix, v2, Thunderbird

you could have separate global vars - gQuotaLabel, gQuataMeter, etc. instead of having a top level object. The way the code is now is just needlessly obscure...or just use gUpdateStatusQuota...

Comment 15

[Ben Bucksch (:BenB)]

7 global vars would be a bit much. I'll rename it "gQuotaUICache" or so, remove the redundant "quota" from the members, and use it directly.

Comment 16

[Ben Bucksch (:BenB)]

Attachment: Fix, v3

Fixed above comments. I assume I need to fix the UUID only in the IDL, I haven't found any other instance.
Also fixed hiding the meter in one edgecase and moved the class for more flexible theming.

If you need a test IMAP account with quota, tell me and I can supply you with one.

Comment 17

[David :Bienvenu]

Thx for doing this, Ben! It seems to basically work but there are a few issues that I think should be addressed.

1. Clicking on a top-level account (e.g., local folders) doesn't remove the quota widget.

2. I think having a tooltip for the quota widget is a must - otherwise, users will have no idea what it is. The tooltip should say something about IMAP quota usage. Maybe it should mention the server, but I don't think it should mention the folder (see item 3)

3. I have a server which has an account-based quota, not a per-folder quota (which is probably by far the most common setup). When I go look at the quota info in the quota tab, I see the same information for each folder. But the quota widget seems to vary a bit based on which folder is selected. Certain folders seem to lag behind a bit. I don't know why that would be...

At first I thought the quota widget should be to the right of the status/progress bar, so the status/progress bar wouldn't move, but now I kinda think it should be next to the folder totals, the way you have it now.

Comment 18

[David :Bienvenu]

4 - the first time I select a folder in the current run of TB, it doesn't show the quota information. If I click away and then back, it shows it. That was confusing the heck out of me, but now I think I see the pattern.

Comment 19

[David :Bienvenu]

Comment on attachment 239099 [details] [diff] [review]
Fix, v3

if you put these in mailnews.js,
+pref("mail.quota.mainwindow_threshold.show", 20);
+pref("mail.quota.mainwindow_threshold.warning", 80);
+pref("mail.quota.mainwindow_threshold.critical", 95);
+

you won't need the catch part of this code:

+    try {
+      gQuotaUICache.showTreshold = gPrefBranch.getIntPref(kBranch + "show");
+      gQuotaUICache.warningTreshold = gPrefBranch.getIntPref(kBranch + "warning");
+      gQuotaUICache.criticalTreshold = gPrefBranch.getIntPref(kBranch + "critical");
+    } catch (e) {
+      gQuotaUICache.showTreshold = 0;
+      gQuotaUICache.warningTreshold = 80;
+      gQuotaUICache.criticalTreshold = 95;


because you'll always get a value.

Comment 20

[David :Bienvenu]

Attachment: update folder quota status ui after the folder loads, and we have the quota info from the server

can you try this and roll it into your patch if it works? You might want to remove the old call, though I'm not sure...

Comment 21

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

- Incorporated bienvenu's fix for problem 4. (Thanks a lot!)
- Fixed problem 1 by adding a call to ChangeFolderURI, isServer case.
- Changed the default prefs and removed try/catch per comment 19.
- Tried to add tooltip (problem 2).
  It doesn't show up, I don't know why, String / property content is OK.
- No idea about problem 3, I have never seen that.

Comment 22

[Ben Bucksch (:BenB)]

I found the tooltip problem(s):
1. caps was wrong, tooltipText is correct for the property.
2. It needs to be on the label, not the meter, then it shows up even when next to the text. This is most likely because of the <stack>.
Replacing the existing tooltip line with the following makes it work:
      gQuotaUICache.label.tooltipText = tooltip;
Also, the string should be (it wrongly had percent signs):
quotaTooltip=IMAP quota: %S KB used of %S KB total. Click for details.
Not attaching a new patch just yet.

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

> Not attaching a new patch just yet.

Actually, I do, to make it easier for bienvenu.

Comment 24

[David :Bienvenu]

great, thx, I will try to look at this tomorrow...

Comment 25

[David :Bienvenu]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

lastest patch seems to be working well for me. Thx, Ben!

Comment 26

[David :Bienvenu]

I've landed the backend imap changes for this on the trunk and branch. 

Comment 27

[Scott MacGregor]

You'd earn major brownie points with me in the quota UI was hidden until you start to get say within 75% (or some other numbe) of your quota. The status bar is prime real estate and I like to keep in uncluttered as much as possible. 

Comment 28

[Ben Bucksch (:BenB)]

> You'd earn major brownie points with me in the quota UI was hidden until you
> start to get say within 75% (or some other numbe) of your quota.

Exactly that is already implemented :) See comment 3 (and start of comment 4).
I'll collect the brownie points at next occasion ;-P

Comment 29

[David :Bienvenu]

I think we'd much prefer that we not show the quota meter at all until the user gets to 75% instead of 20%, by default.

Comment 30

[Ben Bucksch (:BenB)]

Well, that's a policy and easily adjusted.
But I think that this would make it relatively useless. If you're at 75%, it's almost "too late", you're almost full and will probably whine to your admin to get more space, or have to delete half of your mails. When you start to see it at a lower value, you can adjust your usage according to the quota. I think a user at 40% quota will care about how much he can still use and if he needs to change his patterns.
That's why I created the 4 states:
- No bar when there's no problem: no quota or less than 20%. If you have a gigabyte, you won't care when you have 150 MB used after a year. If you see only 3%, because admins gave a lot of freedom, you are encouraged to use up more space, which would drive admins nuts.
- If you are at 40%, you probably do want to know about it. Esp. if it's after 6 months, because you only have 20 MB of quota (which is quite common). That's the normal case, which will show the bar normally.
- If you are at 80%, you should take action. That's when the bar gets yellow - warning.
- If you are at 95%, you must takre immediate action or will lose mails. That's when the bar gets red - criticial.
So, I think the current settings are sensible. You could increate the "show" value to 40%, which would show the bar less often. If you put it at 75%, the feature would be reduced to the warning function only, and would not allow people adapt to the quota early on.

Comment 31

[Ben Bucksch (:BenB)]

To emphasise: I think that 20 MB quota and a user who doesn't understand the implications of *copy* to trash and the necessity to "compress folders" is quite common. If she sees she's at 78%, and decides to delete half of her mails, e.g. everything from last year and before, she'll get problems, going over quota just during the delete action, and get very confused. That's why she probably shouldn't even come close to 75% quota, if she has e.g. 100 MB quota, and that's what the main point of this feature is: stopping her from reaching 80% quota in the first place. That also means admins can give more quota.
Anyways, your decision.

Comment 32

[Ben Bucksch (:BenB)]

What are we going to do now?

Comment 33

[Scott MacGregor]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

In the interest of makin g a decision so we can get this fix in, David and I talked this over and we'd like it set at 75%. If we decide down the line that this is too late, we can adjust it later. Thank you for fixing this Ben!

Comment 34

[Scott MacGregor]

Hmm I was going to nominate this patch for thunderbid 2 approval, but can't seem to find the flag for that. 

Comment 35

[Ben Bucksch (:BenB)]

Moving bug to Thunderbird product, maybe that gives me the flag (grr)

Comment 36

[Scott MacGregor]

Ben, can you check this into the trunk? Then ping me in a couple days and if it still looks good on the trunk, I'll approve it for the branch.

Comment 37

[Ben Bucksch (:BenB)]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

(Sorry for the delay.)

Checked in on trunk.

Thanks a lot to bienvenu for finding that one bug and the detailed and quick review.

Will let bake on trunk, then re-request approval for TB2.

QA: You need to actually reach 75% quota usage to see anything, or reduce the threshold in prefs (search for "quota"). Also try switching folders back and forth.

Comment 38

[Scott MacGregor]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

Ben, I think we are ready to move this to the branch now. But I'd like to see the patch in Bug 360591 land with it.

Comment 39

[David :Bienvenu]

did this miss 2.0? It has a string change, which means it's too late, unless we make a very special exception

Comment 40

[Scott MacGregor]

your right David, I thought this had already gone into the branch. Sadly we can't take it anymore unless we take out the string change part.

Comment 41

[Ben Bucksch (:BenB)]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

Rats! I also thought this was long done.

The only localization change is:
+# for quota in main window (commandglue.js)
+percent=%S%%
+quotaTooltip=IMAP quota: %S KB used of %S KB total. Click for details.

The "%" sign could be hardcoded, or does any language need to translate that?
The tooltip could be removed. The meaning of the progress bar could still be kind-of discoverable, because clicking on it goes to the Quota tab of the Folder properties. (You usually can't click on status bar items, though.)
I'd adapt the patch and do the checkin.

What do you think?

Comment 42

[Scott MacGregor]

I'd rather have this feature with the % hard coded (I don't think locales would be changing that anyway) and without a tooltip than not have this at all on the branch. Let's go that route.

Comment 43

[Ben Bucksch (:BenB)]

OK. It's suboptimal, to introduce a feature without tooltip, but if that's the only way... I'll work on it ASAP (fetching branch tree now). Do you want fries with that? (comment 38)

Comment 44

[Scott MacGregor]

heh, yes I'd like to land 360591 with it too, I like french fries.

If you still want the tooltip, another possibility is to write JS that sets the tooltip that doesn't assume the tooltip is in the string bundle.

i.e. var tooltipString = get the tooltip string
     if (tooltipString)
       set the tooltip string on the quote box

That way if a localization doesn't have the translated tooltip, things still work. cc'ing axel to make sure that's kosher.

No tooltip at all is probably easiest and safest though depending.  

Comment 45

[Ben Bucksch (:BenB)]

OK, if that is a reasonable approach, i.e. some important translations would have the string, I'd like to go that route.
(I like french fries, too. And dual core for building.)

Comment 46

[Axel Hecht [:Pike]]

On a general note on the patch, I don't think that changing hwaara's real name in some of the files is a good idea. Check your encodings?

I don't think that we should go into 2.0 with optional strings, nor am I convinced that quota display is a big enough thing to break string freeze.

Another route in addition to having this non-localizable would be a patch checking in the English strings for all locales and post to .l10n. That way, l10n folks can at least fix that for 2.0.0.3 or 4 or whatever the first minor update would be.

Comment 47

[Scott MacGregor]

I agree with Axel, we don't want to break the string freeze for this, hence the idea of an optional string. Given Axel's comments about that too, let's just not have the tooltip on the branch Ben.

Comment 48

[Scott MacGregor]

Hi Ben, any updates on a string free patch for this? Would be great to get this in before the beta 2 deadline tonight.

Comment 49

[Ben Bucksch (:BenB)]

Attachment: Fix, v5, but without locale strings, for 2.0 branch

Yes, I adapted the code and tested it, it works and is ready for checkin. I held back, though, because I didn't have time for the fries, yet.

Comment 50

[Scott MacGregor]

Comment on attachment 251537 [details] [diff] [review]
Fix, v5, but without locale strings, for 2.0 branch

cool. I can land the fries part later today if you don't have time. Approving this patch for the branch so you can land it. Thanks Ben!

Comment 51

[Ben Bucksch (:BenB)]

Comment on attachment 239739 [details] [diff] [review]
Fix, v5

(clearing obsolete flag)

OK, great. I'll check in once my tree is updated and rebuilt and retested.

Comment 52

[Ben Bucksch (:BenB)]

Checked in to branch.

Thanks to all! :)

Comment 53

[Scott MacGregor]

thanks again Ben.

Comment 54

[Marcia Knous [:marcia]]

verified using version 2 beta 2 (20070116). Although my box wasn't at quote, I tweaked the about:config pref to show the progress meter, and it looks great. Thanks for the feature Ben.

Comment 55

[Peter Lairo]

I could really use *thousands separators* for the amount used and total amount in the tooltip (especially for huge quotas, like gmail): 

  1,000,000,000 kibi used of 2,000,000,000 kibi total.

Better yet, switch to MiBi or GiBi when the numbers get so large as to make those units more sensible.

Comment 56

[Ben Bucksch (:BenB)]

FYI, 2 GB = 2000000KB

Comment 57

[Ben Bucksch (:BenB)]

(eh, I mean 2097152, and yes, I know about the kiwi discussion.)