841906 - TB violates RFC2087 (RFC 2087 for QUOTAROOT/QUOTA is not fully suppported yet)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, enhancement)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/536.26.17 (KHTML, like Gecko) Version/6.0.2 Safari/536.26.17

Steps to reproduce:

RFC 2087 (http://tools.ietf.org/html/rfc2087) - IMAP4 QUOTA extension, specifies the method of retrieving quota information for mailboxes.

Whoever implemented it must have misread, because it clearly says:

The GETQUOTAROOT command takes the name of a mailbox and returns the
   list of quota roots for the mailbox in an untagged QUOTAROOT
   response.  For each listed quota root, it also returns the quota
   root's resource usage and limits in an untagged QUOTA response.


Actual results:

Thunderbird takes the LAST quota root returned in the response and displays only that, thus violatin the RFC. This means that if a user receives two quota roots; "User quota" "Group quota" in that order, then Thunderbird's interface will only display Group quota, even if the user's mailbox is full.


Expected results:

Thunderbird needs to do these things:

1. Receive all quota roots and store them.
2. Calculate the percentage of utilization for EACH quota root.
3. For the percentage bar in the bottom right corner of Thunderbird (the one which shows up after 75% or more quota utilization), use the HIGHEST utilization percentage of ALL quota roots. So if User quota is 10% and Group quota is 79%, then show the bar as 79%.
4. When right clicking a folder and going into settings - Quotas, list ALL quota roots by name and utilization, in the order they were received.
5. When holding the mouse over the 75%+ quota root progress bar in the bottom right of the main UI, show a popup listing all quota roots and their utilization.

A temporary workaround is for a server to always send the User's own quota as the last listed quota root, thereby ensuring that it's the single one that Thunderbird displays.

Comment 1

[[e-mail]]

Also note that quota root names have no meaning; they are just human-readable strings. I could call the user and group quotas "Foo" and "Bar" respectively. That is why we must make no distinction between labels, and do point 3 (progress bar) calculations using whichever quota is the closest to being filled, because the client has no way of knowing which quota is the most important. All we can do is list whichever is the closest to full.

Comment 2

[[e-mail]]

To test this behavior, one simply has to install a Dovecot imap server and enable the quota + imap_quota plugins and set up two quota roots as follows (also remember to set a user and group quota on the system):

mail_plugins = quota
protocol imap {
  mail_plugins = $mail_plugins imap_quota
}
plugin {
  quota = fs:User quota:user
  quota2 = fs:Group quota:group
}

This will expose two quota roots over IMAP - and until TB is brought in line with the spec, it will only show the last quota root.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: safe patch

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Created attachment 715240 [details] [diff] [review]
> safe patch

err, wrong bug sorry

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #0)
> Actual results:
> Thunderbird takes the LAST quota root returned in the response(snip)

It's probably slightly inaccurate. Perhaps "Last QUOTA which contains STORAGE first".
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.cpp#2989
> 2989 // RFC2087:  quotaroot_response = "QUOTAROOT" SP astring *(SP astring)
> 2990 //           quota_response = "QUOTA" SP astring SP quota_list
> 2991 //           quota_list     = "(" [quota_resource *(SP quota_resource)] ")"
> 2992 //           quota_resource = atom SP number SP number
> 2993 // Only the STORAGE resource is considered.  The current implementation is
> 2994 // slightly broken because it assumes that STORAGE is the first resource;
> 2995 // a reponse   QUOTA (MESSAGE 5 100 STORAGE 10 512)   would be ignored.
Only Quote data for STORAGE is saved in an entry of an object for an IMAP server.
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.cpp#3025
And, as you already know, only one Quota information is shown at Folder Properties/Quota.

Bug 178758 is for RFC 2087 support, and status is RESOLVED/FIXED.
Confusing but "RESOLVED/FIXED" in B.M.O is far from "VERIFIED/FIXED", and accurate meaning of "RESOLVED/FIXED" is probably "patch has been proposed and is approved by other developers, and 'patch is landed or not' is unknown unless someone writes or indicates 'landed' in the bug" :-)
Current implementation of QUOTAROOT/QUOTA support looks partial.
Confirming.

Comment 6

[WADA:World Anti-bad-Duping Agency]

At least enhancements like following are needed.
(1) Responce parser enhancement, data save location enhancement, are needed, to support, any number of quotaroot name with any quotaroot name in QUOTAROOT response, and, any number of "resource name + usage + limit" with any "resource name" in QUOTA response.
Any number of QUOTAROOT responses/QUOTA responses is permitted in a command response sequence?
If JavaScript Object, Obj[ServerName][MboxName][quotaroot_name][resource_name]["Usage"]=UU/["limit"]=LL like one is very easy and possible. Easy in C++ object too?
Where such data should be stored? incomingServer object? msgFolder object? msgDatabase object? Or other place such as Folder Cache(panacea.dat) or XUL document's attribute for Folder Properties panels?
Easy to make it "xpconnect wrapped object" if new C++ object, for JavaScript modules? 
(2) Enhancement of Quota panel of Folder Properties is needed. In case of many many "resource name", tabbing, scrolling etc. should be considered.

Comment 7

[Arkadiusz Miśkiewicz]

Example how other clients displaying this for user. Storage and messages count quota.
No infinite limits handling though (no scrolls etc).

Bar shows storage quota with lowest free space. Clicking on it shows details for all quotas.

https://user-images.githubusercontent.com/1481324/39646253-7adafc5e-4fe3-11e8-9036-6cb17ac227d1.png

Comment 8

[Shawn Hughes]

Still a pain point: popular IMAP platform: dovecot on cpanel reports both cPanel quota and mailbox quota. Both are important. Because Thunderbird only sees the cpanel quota, user receives no warning before mailbox stops accepting mail for exceeding quota, nor any notice that mailbox is over quota. The only indicators are: no new mail, and IF/When user tries to send a message (AND if "Sending, Place a copy in Sent" is set) then error "unable to save a copy to Sent folder" will occur. I had a mailbox bouncing messages for days before I discovered it was full.

Comment 9

[gene smith]

(In reply to Shawn Hughes from comment #8)

Still a pain point: popular IMAP platform: dovecot on cpanel reports both cPanel quota and mailbox quota. Both are important.

Can you show me an example imap getquotaroot response for dovecot on cpanel? You can record an IMAP:5 log using procedure here to see it: https://wiki.mozilla.org/MailNews:Logging

Comment 2 also shows a way to get multiple quota responses to quotaroot. I'll see if I can get that to work with just dovecot without cpanel.

I think all my existing accounts just have one quota type in response to getquotaroot.

Comment 10

[gene smith]

I enabled some quotas on my test imap server. I think I have two quotaroots: User and Disk, with attributes "storage" and "message". I only see these returned for "User quota" and not for "Disk quota". Here's what I see with the IMAP:5 log:

S-INBOX/newerF:SendData: 11 getquotaroot "INBOX/newerF"
S-INBOX/newerF:CreateNewLineFromSocket: * QUOTAROOT INBOX/newerF "User quota" "Disk quota"
S-INBOX/newerF:CreateNewLineFromSocket: * QUOTA "User quota" (STORAGE 1879422 10485760 MESSAGE 17622 8192)
S-INBOX/newerF:CreateNewLineFromSocket: 11 OK Getquotaroot completed (0.008 + 0.000 + 0.007 secs).

Shawn Hughes: Can you post a similar IMAP:5 snippet that you see for the dovecot on cpanel server? I'm no expert on this an really don't know what I am doing so to see a "real" example would be helpful. Thanks.

P/S: It is pointed out above in comment 5 that tb only considers the "storage" value and ignores other possible attributes. Maybe only in the last "* QUOTA" in the response too? Also, maybe a big UI change to show all the attributes with bar charts.

Comment 11

[Arkadiusz Miśkiewicz]

Example from https://github.com/roundcube/roundcubemail/issues/1999

QUOT1 GETQUOTAROOT "INBOX"

• QUOTAROOT "INBOX" "User quota" "Group quota"
• QUOTA "User quota" (STORAGE 148 102400)
• QUOTA "Group quota" (STORAGE 1499188 4096000)
  QUOT1 OK Getquotaroot completed.

And there also can be MESSAGE limits in these.

Comment 12

[Shawn Hughes]

Attachment: MOZ_LOG IMAP:5 snippets related to getquotaroot

(in reply to Gene Smith from comment #9)
Providing IMAP Log snips (v 68.4.1 on Debian) for getquotaroot for multiple IMAP accounts hosted at:
tuffmail.com; GMail(personal);  GMail(G-Suite); and dovecot/cpanel hosted by a2hosting.com and asmallorange.com

re: comment #2:  relates to altering dovecot behavior, yes?  I cannot control dovecot settings on any of my accounts.

IMAP LOGS:

Comment 13

[gene smith]

Thanks to you both for the "real world" examples.

re: comment #2: relates to altering dovecot behavior, yes? I cannot control dovecot settings on any of my accounts.

I wasn't suggesting you change anything in dovecot. I just used the info in comment 2 to change my test dovecot server to produce multiple quota responses. (My dovecot is just for testing tb stuff, not a production server.)

And there also can be MESSAGE limits in these.

Yes, as in my dovecot response:

S-INBOX/newerF:CreateNewLineFromSocket: * QUOTA "User quota" (STORAGE 1879422 10485760 MESSAGE 17622 8192)

Several other "limit names" are possible at least with dovecot other than just storage and messages. Note: my MESSAGE quote is greater than 100% but it's fake.

Comment 14

[gene smith]

Attachment: quotaroot-fix.diff

This proposes a fix for the quota issue described in this bug. This now looks at all the quota items reported for the selected folder and reports the one having the highest percentage used. Also, it no longer restricts the quota resource to just storage so if the server reports a quota for the number of messages or number of mailboxes or other resourse, they will also be candidates for reporting the worst case usage.

This makes a few changes to the quota tab of the folder properties screen. Instead of showing the quotaroot name, I now show the quotaroot name and the resource (if resource is storage, "in KB" is appended). The usage line no longer shows "in KB" but just gives numbers since resources like number of messages or mailboxes are typically unit-less. The status bar-graph is unchanged.

This fixes a problem with status messages that appear when quota capability is not supported or if the folder is not yet selected and quota info is not available. It also fixes a problem that when a server reports capability in the initial imap "greeting" the password is not retrieved until the folder is re-selected. This caused the quota report to claim quota is not yet available.

I have tested this with multiple servers, some with multiple resources per quotaroot and it works well.

Note: A more ideal fix is to list all the quota items in the quota tab rather than just the worst case one. This would require more changes to the quota tab UI that I'm not sure how to do and might not be worth the effort since most servers only report one quota item per folder.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9133092 [details] [diff] [review]
quotaroot-fix.diff

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

::: mail/locales/en-US/chrome/messenger/folderProps.dtd
@@ +58,5 @@
>  <!ENTITY permissionsDesc.label                   "You have the following permissions:">
>  <!ENTITY folderType.label                        "Folder Type:">
>  
>  <!ENTITY folderQuotaTab.label                    "Quota">
> +<!ENTITY folderQuotaRoot.label                   "Quotaroot/Resource:">

the localization key always needs to be changed when you change the value

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +199,5 @@
>  of connections to this server. If so, use the Advanced IMAP Server Settings dialog to \
>  reduce the number of cached connections.
>  
> +imapQuotaStatusFolderNotOpen=Quota is not available because the folder is not open. \
> +Close this dialog and select the folder and try again.

would not change this

@@ +204,2 @@
>  
> +imapQuotaStatusNotSupported=This folder does not support the quota capability.

would not change this

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1980,5 @@
> +  // fetched from the server after initial connection. When CAPABILITY is
> +  // explicitly requested, m_stringBundle is initialized before the stored
> +  // password is retrieved. NS_ENSURE_STATE is not actually needed because
> +  // if m_stringBundle is null here, it is still intialized in
> +  // GetStringBundle() below. --gds

Ok, but let's just remove it all (this uncommented code)

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +176,5 @@
>        m_downloadingFolderForOfflineUse(false),
>        m_filterListRequiresBody(false),
>        m_folderQuotaUsedKB(0),
> +      m_folderQuotaMaxKB(0),
> +      m_folderQuotaPercentTimesTen(0) {

Strange name. Why do we want that?

@@ +5526,5 @@
>    // if for some bizarre reason this fails, we'll still fall through to the
>    // normal sharing code
>    if (NS_SUCCEEDED(rv)) {
> +    // get the latest committed imap capabilities bit mask.
> +    eIMAPCapabilityFlags capability;

initialize to false to make sure it's set later

@@ +7933,5 @@
>  NS_IMETHODIMP nsImapMailFolder::SetFolderQuotaData(
>      const nsACString &aFolderQuotaRoot, uint32_t aFolderQuotaUsedKB,
>      uint32_t aFolderQuotaMaxKB) {
> +  if (aFolderQuotaUsedKB == UINT32_MAX && aFolderQuotaMaxKB == UINT32_MAX) {
> +    // Reset to initialize evaluation of  a new quotaroot imap response; nothing

nit: double space here

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2583,3 @@
>  void nsImapServerResponseParser::quota_data() {
>    if (!PL_strcasecmp(fNextToken, "QUOTAROOT")) {
> +    // ignore QUOTAROOT response (except to invalidate previously stored data).

Please capitalize Ignore

@@ +2602,5 @@
> +      AdvanceToNextToken();
> +      if (fNextToken) {
> +        if (fNextToken[0] == '(')
> +          fNextToken++;
> +        // Should have one or more resource|used|max triplet per quotaroot.

Maybe you can add some examples in here

@@ +2619,5 @@
> +            maxToken.SetLength(strlen(fNextToken)-1);
> +          max = atoi(maxToken.get());
> +          // Some servers don't define a quotaroot name, so provide one.
> +          if (quotaroot.IsEmpty())
> +            quotaroot.Append("noname");

Maybe better to leave it empty?

@@ +2624,5 @@
> +          nsCString quotaRootResource(quotaroot);
> +          quotaRootResource.Append(" / ");
> +          quotaRootResource.Append(resource);
> +          if (resource.Equals("STORAGE", nsCaseInsensitiveCStringComparator()))
> +            quotaRootResource.Append(" (KB)");

why the parenthesis?

@@ +2625,5 @@
> +          quotaRootResource.Append(" / ");
> +          quotaRootResource.Append(resource);
> +          if (resource.Equals("STORAGE", nsCaseInsensitiveCStringComparator()))
> +            quotaRootResource.Append(" (KB)");
> +          fServerConnection.UpdateFolderQuotaData(quotaRootResource, used, max);

I think it would be better if this method took some object as input, so we could store the stuff we found, if there were more than one resource. (Don't worry about the UI, we can handle that later.)

Comment 16

[gene smith]

Attachment: quotaroot-fix2.diff

I think it would be better if this method took some object as input, so we could store the stuff we found, if there were more than one resource. (Don't worry about the UI, we can handle that later.)

Magnus, I have changed the items you requested. However, I haven't changed the above item completely as I think you requested. I did change it so that that all the quota items are saved but in 3 separate arrays rather than as an array of a struct containing 3 items. I'm not real familiar with arrays in mozilla code but it it seems like nsTArray is appropriate so I used that and the code still works OK.

•  m_folderQuotaPercentTimesTen(0) {
  

Strange name. Why do we want that?

I did this in case two quota item are tied in percent for worst case, e.g., both are at 86% full. If I multiply by 100 I only get a percent. But if I multiply to 1000 I get one more decimal place to compare to, so I might see 861 vs. 864 so I take the quotaroot/resource with 864 for display.

Here's how I calculate the percent-time10:

+    uint32_t percentTimesTen = 0;
+    if (aFolderQuotaMaxKB > 0)
+      percentTimesTen = ((static_cast<uint64_t>(aFolderQuotaUsedKB) * 1000) +
+        (aFolderQuotaMaxKB/2)) / aFolderQuotaMaxKB;

Anyhow, maybe just multiply by 100 is OK too and worrying about the extra decimal is nit-picking.

I just realized that my variables ...UsedKB and ...MaxKB should really drop the KB since the same calculation and comparisons apply to non-STORAGE resources too.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9133423 [details] [diff] [review]
quotaroot-fix2.diff

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

You could just put something like this into the same idl you're modifying. 

interface nsMsgIQuota : nsISupports
{
  attribute ACString name;
  unsigned long usage;
  unsigned long limit
};

Otherwise the information will be really hard to use.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +456,5 @@
>  # Used to separate names/email addresses in a list.  Note the trailing space ', '
>  macBiffNotification_separator=,\u0020
>  
>  # For the Quota tab in the mail folder properties dialog
> +quotaUsedFree=%S of %S used

need to change the l10n key too

@@ +461,4 @@
>  quotaPercentUsed=%S%% full
>  # for quota in main window (commandglue.js)
>  percent=%S%%
> +quotaTooltip=IMAP quota: %S used of %S total. Click for details.

need to change the key

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2620,5 @@
> +            maxToken.SetLength(strlen(fNextToken)-1);
> +          max = atoi(maxToken.get());
> +          // Some servers don't define a quotaroot name, so display "".
> +          if (quotaroot.IsEmpty())
> +            quotaroot.Append("\"\"");

Wouldn't it be better to just leave it empty?

Comment 18

[gene smith]

You could just put something like this into the same idl you're modifying.

interface nsMsgIQuota : nsISupports
{
  attribute ACString name;
  unsigned long usage;
  unsigned long limit
};

Sounds good but when I do this and I define an nsTArray of type nsMsgIQuota in the .cpp I see the compile error "nsMsgIQuota is a virtual class". Haven't figured out yet how to get around this. Also, added "attribute" to usage and limit didn't help.

Comment 19

[Jorg K (CEST = GMT+2)]

Hmm, looks like there are a few typos here:

interface nsMsgIQuota : nsISupports
{
  attribute ACString name;
  unsigned long usage;
  unsigned long limit
};

That should be nsIMsgQuota since we always start with "nsI", I for interface. Then there is attribute missing twice, as you pointed out.

This is a new interface, so something needs to inherit from that, like here:
https://searchfox.org/comm-central/rev/771b361ca1a33dd9ba3afdfdadfedf4621c98873/mailnews/base/util/nsMsgMailNewsUrl.h#40

In your case, I guess you want to add it here:
https://searchfox.org/comm-central/rev/771b361ca1a33dd9ba3afdfdadfedf4621c98873/mailnews/imap/src/nsImapIncomingServer.h#23

And then nsImapIncomingServer needs to implement the Get and Set methods.

Comment 20

[Magnus Melin [:mkmelin]]

Sorry about the typos!

Comment 21

[gene smith]

Attachment: quotaroot-fix3.diff

I ended associating the interface with the folder class instead of the incoming server class since it needs the array of quota items for each folder, not per server. Anyhow, it builds and seems to work OK, but I need to re-test to make sure it is completely working (assuming I'm even on the right track).

I had big problems at line 506 of the raw diff getting a pointer to the interface. I found some documentation that I followed and the only thing I could get to work at that point was to cast to "this" pointer. I'm 99% sure that's not the best way but everything else I tried didn't compile or crashed.

Comment 22

[Jorg K (CEST = GMT+2)]

There are two print statements left in the code. As for the cast, there is positively no line 506 anywhere, maybe it's like 506 of the patch, but we can't see that in the review. I guess you mean this:

//nsCOMPtr<nsIMsgQuota> quotaItem
      //getter_AddRefs(static_cast<nsIMsgQuota>(this));
    //do_GetInterface(getter_AddRefs(quotaItem));
    nsIMsgQuota *quotaItem = static_cast<nsIMsgQuota*>(this);
    quotaItem->SetQuotaRootResource(aFolderQuotaRoot);

Why do you need this at all? Isn't this a folder and a quota item at the same time? So just this->SetQuotaRootResource(aFolderQuotaRoot); and of course the this is implicit, so just SetQuotaRootResource(aFolderQuotaRoot);. If that doesn't compile:

nsCOMPtr<nsIMsgQuota> quotaItem(do_QueryInterface(this));

Comment 23

[gene smith]

Attachment: quotaroot-fix4.diff

Sorry for the confusion; I was referring to line 506 of the quotaroot-fix3.diff attachment but you found the right place.
Anyhow, thanks for wake-up on "this" meaning. So I didn't have to declare a "quotaItem" at all, just set the values into the folder object using the new "set" methods.

I have an array of nsIMsgQuota items that I defined in the folder .h file:

nsCOMArray<nsIMsgQuota> m_folderQuota;

I set the 3 values into the folder object then then append an element to m_folderQuota array:

SetQuotaRootResource(aFolderQuotaRoot);
SetResourceUsed(aFolderQuotaUsed);
SetResourceMax(aFolderQuotaMax);
m_folderQuota.AppendElement(this);

Is this correct? It seems strange to append "this" at this point, but it seems to work.

I fixed the "keys" and display empty when there is no quotaroot name as requested in comment 17. Also, I assume the string changes are needed in the "suite" so I made the corresponding changes there too. And got rid of the printfs.

I'll submit the formal patch with the appropriate headers if this is OK and after a bit more testing.
Edit: And will run clang-format.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9133968 [details] [diff] [review]
quotaroot-fix4.diff

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

AppendElement(this); is used in the code base.

Disclaimer: Drive-by comments at 12:40 AM, not a full review.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +7970,5 @@
> +    // clears any previous array data and marks the quota data for this folder
> +    // invalid.
> +    m_folderQuotaPercentTimesTen = 0;
> +    m_folderQuotaDataIsValid = false;
> +    m_folderQuota.RemoveObjectsAt(0, m_folderQuota.Count());

m_folderQuota.Clear() ?

::: mailnews/imap/src/nsImapMailFolder.h
@@ +566,5 @@
> +  // Quota support.
> +  nsCString m_quotaRootResource;
> +  uint32_t m_resourceUsed;
> +  uint32_t m_resourceMax;
> +  nsCOMArray<nsIMsgQuota> m_folderQuota;

Should that be a nsTArray<RefPtr<nsIMsgQuota>>?

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2613,5 @@
> +          AdvanceToNextToken();
> +          used = atoi(fNextToken);
> +          AdvanceToNextToken();
> +          nsAutoCString maxToken(fNextToken);
> +          if (fNextToken[strlen(fNextToken)-1] == ')')

I think clang-format will insist on spaces around the minus. Something for the next rev.

@@ +2618,5 @@
> +            maxToken.SetLength(strlen(fNextToken)-1);
> +          max = atoi(maxToken.get());
> +          // Some servers don't define a quotaroot name which display blank.
> +          nsCString quotaRootResource(quotaroot);
> +          quotaRootResource.Append(" / ");

AppendLiteral, and 2x below.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 9133968 [details] [diff] [review]
quotaroot-fix4.diff

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

::: mail/locales/en-US/chrome/messenger/folderProps.dtd
@@ +58,5 @@
>  <!ENTITY permissionsDesc.label                   "You have the following permissions:">
>  <!ENTITY folderType.label                        "Folder Type:">
>  
>  <!ENTITY folderQuotaTab.label                    "Quota">
> +<!ENTITY folderQuotaRoot2.label                  "Quotaroot/Resource:">

RFC 2087 talks about name, I think that would be less jargon to use here too

::: mailnews/imap/public/nsIMsgImapMailFolder.idl
@@ +171,2 @@
>     */
> +  void getQuota(out boolean valid, out unsigned long used, out unsigned long max);

You could let this now be

Array<nsIMsgQuota> getQuota()

An empty array would mean the same as (in)valid.

@@ +219,5 @@
>  };
> +
> +[scriptable, uuid(9db60f97-45c1-45c2-8ab1-395690228f3f)]
> +interface nsIMsgQuota : nsISupports {
> +  attribute ACString quotaRootResource; /* "<quotaroot> / <resource>" */

Please document using /** */ style above the relevant item you're documenting. 

Since rfc 2087 talks about "names, usages, and limits" I think name, usage, and limit would be preferable name. It's easier to follow the concepts if the names are consistent.

::: mailnews/imap/src/nsImapMailFolder.h
@@ +380,5 @@
> +  NS_IMETHOD SetResourceUsed(uint32_t aResourceUsed) override;
> +  NS_IMETHOD GetResourceMax(uint32_t *aResourceMax) override;
> +  NS_IMETHOD SetResourceMax(uint32_t aResourceMax) override;
> +
> +

nit: extra blank line

Comment 26

[gene smith]

Attachment: quotaroot-fix5.diff

I think I have found the correct way to add the nsIMsgQuota interface. It is based on a similar interface and function returning an Array found in mailnews/base/src/nsMsgTagService.idl. The details can be found using this search link for "nsIMsgTag":

https://searchfox.org/comm-central/search?q=nsIMsgTag%5Cb&case=false&regexp=true&path=

As is done for nsMsgTagService, I now create a new class called nsMsgQuota that inherits from nsIMsgQuota (nsImapMailFolder no longer inherits from nsIMsgQuota as in previous diffs). For the two .js functions associated with this, one now returns the array and the other accepts it as an input parameter.

The determination of the array element with the maximum percent used now occurs in the .js files instead of the .cpp and eliminates the need to determine the "percentTimeTen" and the array index of the worst case percent usage.

With future changes to the .xhtml and .js this should allow the quota tab to display all the quota items and not just the item with highest usage. This is the original goal of this bug.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9134857 [details] [diff] [review]
quotaroot-fix5.diff

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

Please remove the print statement and run clang-format on this.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2616,5 @@
> +            maxToken.SetLength(strlen(fNextToken) - 1);
> +          max = atoi(maxToken.get());
> +          // Some servers don't define a quotaroot name which display blank.
> +          nsCString quotaRootResource(quotaroot);
> +          quotaRootResource.Append(" / ");

AppendLiteral.

Comment 28

[gene smith]

Attachment: quotaroot-fix6.diff

A few more updates. Addressed Jorg's comments. Added a parameter aAction to SetFolderQuotaData() and others so can invalidate, store and validate rather than using other parameters to determine the action. Plus a few other misc. clean-ups.
I've only run clang-format on cpp/h file. Should others be run through it too, like .js files? (Notice for those it recommended a lot of changes to stuff I didn't touch.)

Comment 29

[Jorg K (CEST = GMT+2)]

clang-format is for C++ files, JS files are treated with eslint/prettier. If you have it set up correctly, you just run ../mach eslint mail/base/content for example. There is also a --fix switch.

Comment 30

[gene smith]

Thanks, didn't know about eslint.
I should also mention that fix6 now marks the quota array valid only after the OK response occurs for the getquotaroot imap command. This is necessary since there can be multiple QUOTA untagged responses, each with multiple resources. Before we only saved the first STORAGE resource data and immediately marked it valid before the OK response occurred.

Comment 31

[gene smith]

Doing some more testing on this today with daily/trunk, it appears that when I set the advanced server setting "cached connections" from 5 down to 1, 2, 3 or 4 that it is ignored and remains at the default value of 5. Don't know if this is a known bug or not.
Edit: I couldn't find anything reported about this so I submitted Bug 1624393.

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 9134936 [details] [diff] [review]
quotaroot-fix6.diff

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

::: mailnews/base/content/folderProps.xhtml
@@ +201,5 @@
>                        readonly="readonly"
>                        class="input-inline"
> +                      aria-labelledby="quotaRootLabel"
> +                      size="32"/>
> +          <description id="quotaUsedFree2"/>

id of the actual element should not change

::: mailnews/imap/public/nsIMsgImapMailFolder.idl
@@ +19,5 @@
>  /**
> + * This interface defines the quota for a resource within a quota root. It
> + * consisting of three attributes:
> + * name:  Concatenation of the quota root name and the resource name separated
> + *        by a slash, i.e., "<quotaroot name> / <resource name>"

If quotaroot is emtpy, maybe we just give <resoutce name>
It looks rather odd when root is emtpy (like gmail).

@@ +29,5 @@
> +[scriptable, uuid(9db60f97-45c1-45c2-8ab1-395690228f3f)]
> +interface nsIMsgQuota : nsISupports {
> +  readonly attribute ACString name;
> +  readonly attribute unsigned long usage;
> +  readonly attribute unsigned long limit;

actually, these don't need to be readonly
(it would only make testing harder)

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +7999,5 @@
> +    nsTArray<RefPtr<nsIMsgQuota>> &aArray) {
> +  if (m_folderQuotaDataIsValid) {
> +    // Only set aArray if quota data is valid. If not currently valid,
> +    // aArray is not set and returns an empty array when this function called
> +    // from javascript.

it would be the same for c++, so maybe don't need this comment

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +2625,5 @@
> +          nsCString quotaRootResource(quotaroot);
> +          quotaRootResource.AppendLiteral(" / ");
> +          quotaRootResource.Append(resource);
> +          if (resource.Equals("STORAGE", nsCaseInsensitiveCStringComparator()))
> +            quotaRootResource.AppendLiteral(" KB");

I guess we should still just have the number, not KB tagged onto the name.
It's not clear to me we even want to show the actual numbers anywhere (or at least rather hidden). It could be better to just show the percentage, like

STORAGE: [****        ] 34%

Comment 33

[Magnus Melin [:mkmelin]]

Attachment: bug841906_quota.extra.patch

Did some cleanup on top of your patch. (Feel free to merge this with yours if you like.)

Comment 34

[Arkadiusz Miśkiewicz]

(In reply to Magnus Melin [:mkmelin] from comment #32)

I guess we should still just have the number, not KB tagged onto the name.
It's not clear to me we even want to show the actual numbers anywhere (or at
least rather hidden). It could be better to just show the percentage, like

STORAGE: [**** ] 34%

Users (here) have no idea what their limits are unless limit is shown somewhere. 34% give no knowledge in such case (34% of 10MB, 10GB or something else?)

Comment 35

[Magnus Melin [:mkmelin]]

They don't, but RFC 2087 doesn't say what it is, and ATM we just show very large numbers there so that is also not very user friendly.

Comment 36

[Arkadiusz Miśkiewicz]

It says "implementation defined" and gives example of size/number of messages. Does anyone know implementation that returns other things than bytes and number of messages there?

Comment 37

[Magnus Melin [:mkmelin]]

Attachment: bug841906_quota-showall.patch

Makes all the types of quota resources we know of show. Typically just storage.

Comment 38

[Magnus Melin [:mkmelin]]

Attachment: folderquotas.png

Comment 39

[Alessandro Castellani [:aleca]]

Comment on attachment 9136332 [details] [diff] [review]
bug841906_quota-showall.patch

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

We need some CSS to properly style this section.

#quotaDetails {
  padding-inline-start: 6px;
  list-style: none;
}

Update the #quotaPercentageBar with:

#quotaPercentageBar {
  border-color: ThreeDDarkShadow;
  border-radius: 99px;
  overflow: hidden;
  margin-inline 1em;
}

::: mailnews/base/content/folderProps.js
@@ +76,2 @@
>  
> +      let progress = document.createElement("progress");

This needs the quotaPercentageBar id as it was needed to style it.

@@ +78,5 @@
> +      progress.setAttribute("value", quota.usage);
> +      progress.setAttribute("max", quota.limit);
> +      if (/STORAGE/i.test(quota.name)) {
> +        li.setAttribute("title", `${quota.usage} / ${quota.limit} KB`);
> +      }

Instead of having this as a title, we should convert put it inline the progress bar, and use the % as title for the progress bar.

Storage: [progress bar with % title/tooltip] usage of limit

And we should have a converter in place that automatically switches the quota from KB, to MB, to GB, to TB if the values reach those limits.

@@ +79,5 @@
> +      progress.setAttribute("max", quota.limit);
> +      if (/STORAGE/i.test(quota.name)) {
> +        li.setAttribute("title", `${quota.usage} / ${quota.limit} KB`);
> +      }
> +      progress.style.margin = "0 1em";

Let's add this to the #quotaPercentageBar css instead of inline, and it should be margin-inline: 1em;

Comment 40

[gene smith]

If we only show the bargraph should we also show the quotaroot name in front like:
Mailbox / STORAGE [-- ] 10% full
or
cPanel Account / STORAGE [-------- ] 50% full

Re: Comment 8 where they had two competing quotaroots for STORAGE and with current tb only saw the one that was smaller. (My % doesn't match the data provided in comment 8, so just an example:

dovecot cpanel ASMALLORANGE.COM bcg

I/IMAP 0x7f1bdadb1000:simply.asoshared.com:S-INBOX:SendData: 8 getquotaroot "INBOX"
D/IMAP ReadNextLine [stream=0x7f1bd8e34710 nb=44 needmore=0]
I/IMAP 0x7f1bdadb1000:simply.asoshared.com:S-INBOX:CreateNewLineFromSocket: * QUOTAROOT INBOX Mailbox "cPanel Account"
D/IMAP ReadNextLine [stream=0x7f1bd8e34710 nb=38 needmore=0]
I/IMAP 0x7f1bdadb1000:simply.asoshared.com:S-INBOX:CreateNewLineFromSocket: * QUOTA Mailbox (STORAGE 3505 51200)
D/IMAP ReadNextLine [stream=0x7f1bd8e34710 nb=48 needmore=0]
I/IMAP 0x7f1bdadb1000:simply.asoshared.com:S-INBOX:CreateNewLineFromSocket: * QUOTA "cPanel Account" (STORAGE 20 10485760)
D/IMAP ReadNextLine [stream=0x7f1bd8e34710 nb=51 needmore=0]
I/IMAP 0x7f1bdadb1000:simply.asoshared.com:S-INBOX:CreateNewLineFromSocket: 8 OK Getquotaroot completed (0.001 + 0.000 secs).

Anyhow, I think even if the real number are not user friendly, they could be useful if and when a uses bumps up against a quota, so I would vote for keeping them.

Comment 41

[gene smith]

(In reply to Magnus Melin [:mkmelin] from comment #35)

They don't, but RFC 2087 doesn't say what it is, and ATM we just show very large numbers there so that is also not very user friendly.

The rfc 2087 implies the storage is in kb and message is in number of messages as example resources:

STORAGE Sum of messages' RFC822.SIZE, in units of 1024 octets
MESSAGE Number of messages

From comment 39:

And we should have a converter in place that automatically switches the quota from KB, to MB, to GB, to TB if the values reach those limits.

This may make the user unfriendly numbers more meaningful.

Comment 42

[Magnus Melin [:mkmelin]]

Attachment: folderprops2.png

Comment 43

[Magnus Melin [:mkmelin]]

Attachment: bug841906_quota-showall.patch

<progress> can't have inline data.

Comment 44

[gene smith]

Attachment: multiple-quota.png

Now see my 4 quota items that I set up on dovecot server! However, no data for storage is shown. Is this the inline data problem you mention?

Comment 45

[Magnus Melin [:mkmelin]]

No, it that would be something else - I put that data to the left instead, like shown in the screenshot. But I don't understand why that string would ever become empty. Can you check why the usage.textContent = ${usage} / ${limit}}; doesn't work?

Comment 46

[Alessandro Castellani [:aleca]]

Comment on attachment 9136575 [details] [diff] [review]
bug841906_quota-showall.patch

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

Thanks for the updates.
I found a couple of small issues that need to be fixed. You can set an r+ after that as everything else works and looks good.

::: mail/themes/shared/mail/folderProps.css
@@ +4,5 @@
> +
> +#quotaDetails {
> +  padding-inline-start: 6px;
> +  list-style: none;
> +}

Let's add a little spacing since we could have multiple li elements.
#quotaDetails > li {
    margin-block-end: 4px;
}

::: mailnews/base/content/folderProps.js
@@ +93,5 @@
> +      li.appendChild(document.createTextNode(" — "));
> +
> +      let usage = document.createElement("span");
> +      if (/STORAGE/i.test(quota.name)) {
> +        let usage = messenger.formatFileSize(quota.usage * 1024);

I think this is the problem for the empty string as you already declared a variable called `usage` for the span element.

@@ +95,5 @@
> +      let usage = document.createElement("span");
> +      if (/STORAGE/i.test(quota.name)) {
> +        let usage = messenger.formatFileSize(quota.usage * 1024);
> +        let limit = messenger.formatFileSize(quota.limit * 1024);
> +        usage.textContent = `${usage} / ${limit}}`;

There's an extra } at the end that should be removed.

Comment 47

[Magnus Melin [:mkmelin]]

Attachment: bug841906_quota-showall.patch

Sorry about that, must have forgot to refresh before uploading the patch.

Comment 48

[gene smith]

Attachment: it-works.png

Looks good now. I set the threshold down low enough to trigger the status bar display and clicking on it brings up the quota display with my several quota items.
Magnus, should I do anything else on this?
Edit: Should the status bar-graph also have rounded corners?

Comment 49

[Magnus Melin [:mkmelin]]

(In reply to gene smith from comment #48)

Magnus, should I do anything else on this?

Just the smaller updates for comment 32

Edit: Should the status bar-graph also have rounded corners?

I would keep that one as it is.

Comment 50

[gene smith]

(In reply to Magnus Melin [:mkmelin] from comment #32)

Comment on attachment 9134936 [details] [diff] [review]
quotaroot-fix6.diff

::: mailnews/base/content/folderProps.xhtml
@@ +201,5 @@
                        readonly="readonly"
                        class="input-inline"
 +                      aria-labelledby="quotaRootLabel"
 +                      size="32"/>
 +          <description id="quotaUsedFree2"/>

id of the actual element should not change

Your changes make this moot.

 ::: mailnews/imap/public/nsIMsgImapMailFolder.idl
 @@ +19,5 @@
  /**
 + * This interface defines the quota for a resource within a quota root. It
 + * consisting of three attributes:
 + * name:  Concatenation of the quota root name and the resource name separated
 + *        by a slash, i.e., "<quotaroot name> / <resource name>"
 
 If quotaroot is emtpy, maybe we just give <resoutce name>
 It looks rather odd when root is emtpy (like gmail).

I changed the comment.

@@ +29,5 @@
 +[scriptable, uuid(9db60f97-45c1-45c2-8ab1-395690228f3f)]
 +interface nsIMsgQuota : nsISupports {
 +  readonly attribute ACString name;
 +  readonly attribute unsigned long usage;
 +  readonly attribute unsigned long limit;
 
 actually, these don't need to be readonly
 (it would only make testing harder)

I removed the readonly but linker requires new Set functions as well as Gets. I defined them in nsImapMailFolder.cpp. The sets and gets are never called, at least by the application. I assume you mean these might be needed for test functions.

::: mailnews/imap/src/nsImapMailFolder.cpp
 @@ +7999,5 @@
 +    nsTArray<RefPtr<nsIMsgQuota>> &aArray) {
 +  if (m_folderQuotaDataIsValid) {
 +    // Only set aArray if quota data is valid. If not currently valid,
 +    // aArray is not set and returns an empty array when this function called
 +    // from javascript.
 
 it would be the same for c++, so maybe don't need this comment

I removed all 3 lines of this comment.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
 @@ +2625,5 @@
 +          nsCString quotaRootResource(quotaroot);
 +          quotaRootResource.AppendLiteral(" / ");
 +          quotaRootResource.Append(resource);
 +          if (resource.Equals("STORAGE", nsCaseInsensitiveCStringComparator()))
 +            quotaRootResource.AppendLiteral(" KB");
 
 I guess we should still just have the number, not KB tagged onto the name.
 It's not clear to me we even want to show the actual numbers anywhere (or at
 least rather hidden). It could be better to just show the percentage, like
 
STORAGE: [****        ] 34%

Your changes make this moot.

I'll attach the complete patch next.

Comment 51

[gene smith]

Attachment: bug841906_quota-complete.patch

Not sure if you want this but here is the complete patch against comm-central trunk (pulled/updated last night). It includes all the changes and I've tested it with my fairly extensive set of imap servers. (Only my localhost based dovecot has multple quotaroots.) Also, the scaling of the KB, MB and GB numbers look good. (Not sure if it handles TeraB but don't have a server with that much quota.)
The patch also includes the added file mail/themes/shared/mail/folderProps.css

Comment 52

[Magnus Melin [:mkmelin]]

Comment on attachment 9136938 [details] [diff] [review]
bug841906_quota-complete.patch

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

Will land it later today with some minor adjustments like below

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +210,5 @@
> +imapQuotaStatusNoQuota2=This folder reports no quota information.
> +
> +# Folder properties were requested by the user (right-click) before the getquotaroot
> +# command was sent.
> +imapQuotaStatusInProgress=Quota information not yet available. Get folder properties again.

Would remove the second sentence.

::: mailnews/imap/public/nsIMsgImapMailFolder.idl
@@ +21,5 @@
> + * consisting of three attributes:
> + * name:  If quota root name not empty, the concatenation of the quota root name
> + *        and the resource name separated by a slash , e.g.,
> + *        "User Quota / MESSAGE" or with empty quota root name, just resource
> + *        name, e.g., "STORAGE"

These should be documented along side the attributes. I'll update it

::: suite/locales/en-US/chrome/mailnews/folderProps.dtd
@@ +58,5 @@
>  <!ENTITY permissionsDesc.label                   "You have the following permissions:">
>  <!ENTITY folderType.label                        "Folder Type:">
>  
>  <!ENTITY folderQuotaTab.label                    "Quota">
> +<!ENTITY folderQuotaRoot2.label                  "Name/Resource:">

we don't use this anymore

Comment 53

[Magnus Melin [:mkmelin]]

Attachment: bug841906_quota-complete.patch

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/54a83a246c9e
make all the quotas the IMAP server sends us show up in the folder props. r=aleca,mkmelin