1136696 - Folder pane Size column shows "0 KB" for empty folders

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Starting with 2.33b1, the folder pane Size column (the last one, not sure whether it's shown by default) shows "0 KB" for empty folders. With previous SM versions (up to/including 2.32), the column was empty for such cases.

I assume the back-end changed (now returning a different value in such cases than before) and that we special-case the old value in the suite code.

Questions:
1. Can anyone confirm?
2. Is this still present on trunk?
3. Is the new behavior intended? [for me, it looks wrong/disturbing]

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Now I'm pretty sure the regression was caused by bug 789679 (see below), and since the relevant code hasn't been fixed since, this bug is still valid for trunk.

Regression cause:
http://hg.mozilla.org/comm-central/rev/a228fcd72731#l5.92

The folderSize == 0 check was removed - starting with bug 789679 patch "make GetSizeOnDisk 64 bit v5", with no justification given for the change between v4 (bug 789679 comment 110) and v5 (bug 789679 comment 124).

Comment 2

[Rainer Bielefeld]

REPRODUCIBLE with German SeaMonkey 2.33.B1 Build 20150223174142 on German WIN7 64bit
I see that behavior as a bug. "Total" value and "Unread" Number disappear if value changes 0 "0", but size value remains.

@Reporter:
Can you please modify Summary to something like "Folder size display "0KB" remains instead to disappear like "0 Total" and "0 Unread"" no make clear that the problem is the visibility and not the numeric value? I do not have permissions.

Comment 3

[Rainer Bielefeld]

I can't find similar display columns in Firefox?

Comment 4

[Rainer Bielefeld]

oops:
I can't find similar display columns in THUNDERBIRD?

Comment 5

[[e-mail]]

Thunderbird doesn't have that column, and it uses a different method to display its columns anyway.

nsMsgFolderDataSource::GetFolderSizeNode lost its zero check.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Rainer Bielefeld from comment #2)
> Can you please modify Summary to something like "Folder size display "0KB"
> remains instead to disappear like "0 Total" and "0 Unread"" no make clear
> that the problem is the visibility and not the numeric value?

Sorry, but I don't see the need for that. The summary is supposed to explain what the problem is, as short and clear as possible. Your observation is correct, but really more a coincidence since what regressed is only the Size column display values, not the other ones.

Once this bug has been fixed, the problem should be gone either way. I already identified the (likely) cause of the regression, so chances are this can happen quite fast - TB is not using the RDF-based folder pane implementation anymore, so they shouldn't object.

(In reply to Rainer Bielefeld from comment #4)
> I can't find similar display columns in THUNDERBIRD?

The TB devs had removed those columns (I think when they moved to a JS-based folder pane implementation). Since then, TB users had to install an add-on to get the columns back. I recall having read somewhere recently that they are going to bring them back to TB itself.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #5)
> nsMsgFolderDataSource::GetFolderSizeNode lost its zero check.

I thought I had already written that. ;-)

Can you provide a fix please? It'd take me quite some time to get my build environment up to speed.

Comment 8

[:aceman]

InvisibleSmiley, what TB version does SM 2.33 correspond to? Probably TB37 is it is a beta? (So it doesn't yet have code from bug 464973.)
How comes there is a Folder size column in Seamonkey? Is bug 510685 no longer valid?
Or maybe you are using the "Extra folder columns extension"?
Also note bug 464973 so we have the columns back in TB (nighly, earlybird of TB38). So it may affect TB now too.
The intention is to show blank for empty folders, which it does for me in TB. It is possible the TB frontend code differs from the extension (if you are really using that).
I'd take this if it is caused by bug 789679.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #8)
> InvisibleSmiley, what TB version does SM 2.33 correspond to?

TB 36 (TB version is always our minor version + 3).

> How comes there is a Folder size column in Seamonkey?

Because it has always been there.

> Is bug 510685 no longer valid?

I'm not sure it ever was.

> Or maybe you are using the "Extra folder columns extension"?

No, no need.

> Also note bug 464973 so we have the columns back in TB (nighly, earlybird of
> TB38). So it may affect TB now too.

No because this bug is about a regression in RDF code which (AFAIK) TB is no longer using.

> The intention is to show blank for empty folders, which it does for me in
> TB.

So TB *is* using a different implementation. Point proven. :-)

> I'd take this if it is caused by bug 789679.

I'm pretty sure it is, and so is Neil.

Comment 10

[:aceman]

Oh so you use nsMsgFolderDataSource::GetFolderSizeNode to generate the display (the Empty and ??? logic) in the UI, because SM's folder pane is RDF? Then I can see the problem. Yeah, TB handles this display logic in folderPane.js. Sorry about that. So if that function is only used in SM I can put back the checks.

-nsMsgFolderDataSource::GetFolderSizeNode(int32_t aFolderSize, nsIRDFNode **aNode)
+nsMsgFolderDataSource::GetFolderSizeNode(int64_t aFolderSize, nsIRDFNode **aNode)
 {
   nsresult rv;
-  uint32_t folderSize = aFolderSize;
-  if (folderSize == kDisplayBlankCount || folderSize == 0)
+  if (aFolderSize == kDisplayBlankCount64)
     createNode(EmptyString().get(), aNode, getRDFService());

Comment 11

[:aceman]

Attachment: patch

Compiles and doesn't break TB in any obvious way:) But please test in SM.

Comment 12

[[e-mail]]

Comment on attachment 8570032 [details] [diff] [review]
patch

I hope that test request wasn't directed at me.

Comment 13

[:aceman]

Feel free to forward to anybody who can build Seamonkey :)

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8570032 [details] [diff] [review]
patch

Just succeeded building (had to upgrade my Windows build environment to VC++ 2013 first). As expected, the patch fixes the problem. Thanks!

BTW if anyone reading this knows what the equivalent to the former "make tier_app" is, please let me know via PM. I just used "make mailnews; make mailnews/build; make toolkit/library; make chrome" now.

Comment 15

[Philip Chee]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #14)

> BTW if anyone reading this knows what the equivalent to the former "make
> tier_app" is, please let me know via PM. I just used "make mailnews; make
> mailnews/build; make toolkit/library; make chrome" now.
Hmm. Try make binaries

Comment 16

[:aceman]

Thanks for testing the patch.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8570032 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): Bug 789679
User impact if declined: Lots of "0 KB" in MailNews folder pane
Testing completed (on m-c, etc.): Local build
Risk to taking this patch (and alternatives if risky): Low (former state restored)
String changes made by this patch: None

[2.33 final is delayed, so I thought why not try to get this into it still]

Comment 18

[Philip Chee]

Comment on attachment 8570032 [details] [diff] [review]
patch

a=me a=CLOSED TREE

Comment 19

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/8bb27a4f5408
Pushed to branches:
https://hg.mozilla.org/releases/comm-aurora/rev/3c16d99d9998
https://hg.mozilla.org/releases/comm-beta/rev/f80ed9184e2d
Pushed to release:
https://hg.mozilla.org/releases/comm-release/rev/dca6247fed66