173825 - add folder size to folder pane

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[David :Bienvenu]

It would be very useful to know how much disk space folders are taking up. For
local folders, this would be the file size of the mail folder on disk (including
deleted messages); for imap, the sum of the sizes of the messages in the folder.
Coming up is a first stab at getting this working. There are issues that need to
be dealt with, but it's actually pretty useful already.

Comment 1

[David :Bienvenu]

Attachment: first stab

this basically works. The issues include:

updating the size of an imap folder when new headers are downloaded/added
updating the size of local folders after a compact, and updating the size of
local folders when messages are copied into them.

deleting isn't a big issue since deleting doesn't by itself reduce the size of
a folder.

Comment 2

[selmer (gone)]

Thanks David!

Comment 3

[David :Bienvenu]

Attachment: proposed fix

this patch handles updating the folder size in a few more places (e.g., getting
pop3 mail or move/copying pop3 messages to another folder). It's pretty close
to final, I believe. The main outstanding issue is that we're showing the
folder size column by default (not sure if we want to). I don't know how to
make it hidden by default.

Comment 4

[David :Bienvenu]

adding Jen to cc list. Basically, the way I've implemented this, there's a third
column in the folder pane, labelled "Size" that displays the size in MB or kb,
depending on how big the folder is, e.g., 4MB, or 89kb. Empty folders and
servers leave the column blank. We need to decide if this column should be on by
default, and if not, how to turn it off by default. I have other stuff to do, so
I can't drive this feature in at the moment (and we might as well wait for the
tree to open for 1.3 to start checking it in). I do find it to be a very handy
feature, however.

Comment 5

[jglick]

I think it should be Off by default, just like Unread and Total are off by
default. Provide a more simple UI by default and let users in the know customize
and add as desired. You should be able to view the column using the column
selector widget, like you use it to show/hide Unread and Total.

Comment 6

[David :Bienvenu]

OK, that sounds good - with some help from Seth, I think I know how to hide it
by default for existing and new profiles. I'll attach a new patch.

Comment 7

[David :Bienvenu]

Attachment: hide the folder size column by default

this makes it so we hide the folder size column by default.

Comment 8

[David :Bienvenu]

Cavin, can I get a review? thx. Jen, you're OK with the UI as long as we hide
this column by default? To recap, the column header is "Size", and the size is
displayed as xxx kb or xxx MB.

Comment 9

[jglick]

Yup :-)

Comment 10

[David :Bienvenu]

Attachment: hide for new profiles too

thx to Seth for pointing this out.

Comment 11

[Cavin Song]

Comment on attachment 103469 [details] [diff] [review]
proposed fix

r=cavin.

Comment 12

[Cavin Song]

Comment on attachment 104162 [details] [diff] [review]
hide for new profiles too

r=cavin.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 103469 [details] [diff] [review]
proposed fix

sorry for the delay.

one question:

we go from a PRUint32 to a PRInt32.  But that's fine, as it would take a
massively huge folder to cause us to overflow, right? (like a 1 or 2 GB
folder?)

1)


 nsresult
+nsMsgFolderDataSource::createFolderSizeNode(nsIMsgFolder *folder, nsIRDFNode
**target)
+{
+  nsresult rv;
+  
+  
+  PRBool isServer;
+  rv = folder->GetIsServer(&isServer);

why not:

+  PRBool isServer;
+  nsresult rv = folder->GetIsServer(&isServer);

2)

+  if (NS_FAILED(rv)) return rv;

if you don't expect it to fail, can you use:

NS_ENSURE_SUCCESS(rv,rv); instead?

If it is expected to fail:

if (NS_FAILED(rv))
  return rv;

(waterson-nit about this way you can set a breakpoint)

3)

+  
+  PRInt32 folderSize;
+  if(isServer)
+    folderSize = -2;

:\builds\ishmail\mozilla\mailnews\base\src\nsMsgFolderDataSource.cpp(1564):     

totalMessages = -2;
C:\builds\ishmail\mozilla\mailnews\base\src\nsMsgFolderDataSource.cpp(1634):    

totalUnreadMessages = -2;
C:\builds\ishmail\mozilla\mailnews\base\util\nsMsgFolder.h(274):	        

	   unknown; -2 means unknown but we already

should we just #define something for this -2 special value, and use it?

4)

+  else
+  {
+    rv = folder->GetSizeOnDisk((PRUint32 *) &folderSize);
+    if(NS_FAILED(rv)) return rv;

waterson-nit.  (can you use NS_ENSURE_SUCCESS(rv,rv);?)

+  }
+  GetFolderSizeNode(folderSize, target);

should we be checking the return value of GetFolderSizeNode()?

5)

+nsMsgFolderDataSource::GetFolderSizeNode(PRInt32 folderSize, nsIRDFNode
**node)
+{
+  if(folderSize > 0)
+  {
+    nsAutoString sizeString;
+    if (folderSize < 1024)
+      folderSize = 1024; // make at least 1 k;
+    folderSize /= 1024;  // normalize into k;
+    PRBool sizeInMB = (folderSize > 1024);
+    sizeString.AppendInt((sizeInMB) ? folderSize / 1024 : folderSize);
+    sizeString.Append((sizeInMB) ? NS_LITERAL_STRING(" MB") :
NS_LITERAL_STRING(" kb"));
+    createNode(sizeString.get(), node, getRDFService());
+  }
+  else if(folderSize == -1)
+    createNode(NS_LITERAL_STRING("???").get(), node, getRDFService());
+  else
+    createNode(NS_LITERAL_STRING("").get(), node, getRDFService());
+  return NS_OK;
+}

a) would it be too old fashion of me to ask that we don't assign to folderSize
(the in arg)
b) how about aFolderSize (and aNode)?
c) do we have to localize " MB" and " kb", or are they universal?

6)

+      char *keywords;
+      if (NS_SUCCEEDED(flagState->GetCustomFlags(uidOfMessage, &keywords)) &&
keywords)
+      {
	 if (dbHdr && NS_SUCCEEDED(rv))
	 {
	   dbHdr->SetStringProperty("keywords", keywords);
	 }
       }
     }


The old code (and this moved code) will leak keywords.	Can you use an
nsXPIDLCString instead?

7)

+NS_IMETHODIMP nsMsgLocalMailFolder::GetSizeOnDisk(PRUint32* aSize)
+{
+    NS_ENSURE_ARG_POINTER(aSize);
+    nsresult rv = NS_OK;
+    if (!mFolderSize)
+    {
+      nsCOMPtr <nsIFileSpec> fileSpec;
+      nsresult rv = GetPath(getter_AddRefs(fileSpec));
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = fileSpec->GetFileSize(&mFolderSize);
+    }
+    *aSize = mFolderSize;
+    return rv;
+}

you've declared nsresult rv twice, so you're always going to return NS_OK.

8)

For:

+  RefreshSizeOnDisk();

and

+  RefreshSizeOnDisk();

and

+  localFolder->RefreshSizeOnDisk();

and 

+    localFolder->RefreshSizeOnDisk();

should we check the rv, or (void) the call.

Comment 14

[David :Bienvenu]

Good catch on the keyword leak - that's the only caller of that method, so it's
not leaking elsewhere. I've fixed the rv things (though GetSizeOnDisk would have
returned an error if GetPath failed, which I think is really the most likely
failure point). I'll change the assignment to folderSize (though this isn't an
NS_IMETHOD - I thought we were mainly using the aArg convention for interface
methods). I've made all the RefreshSizeOnDisk() calls (void).

I can do the -2 thing, but it's not really a magic number in the sense you might
think - any negative number other than -1 will show up as blank. I think that
might be a mistake. Maybe we *should* make -2 a special number, and then we
could use a PRUint32 and show folder sizes up to 2^32 -2. I'll rework that part
of the patch.  When we convert to using nsIFile, then we can go to a PRInt64.

Comment 15

[David :Bienvenu]

Attachment: patch addressing Seth's comments

this addresses Seth's comments.

Comment 16

[Cavin Song]

Comment on attachment 105107 [details] [diff] [review]
patch addressing Seth's comments

r=cavin.

Comment 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 105107 [details] [diff] [review]
patch addressing Seth's comments

sr=sspitzer

Comment 18

[David :Bienvenu]

fix checked in. FYI, we don't show any size for newsgroups. Also, re Seth's
comment, I'm not currently allowing translation of MB or kb - I don't know if we
want those translated or not. Cc'ing Naoki for his opinion.

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 20

[Henrik Gemal]

Great work David!, but the Folder Size should have been right aligned:
Inbox   256 kb
Spam    11 kb
should be:
Inbox   256 kb
Spam     11 kb

I've opened bug:
http://bugzilla.mozilla.org/show_bug.cgi?id=178817

Comment 21

[[e-mail]]

> hide the folder size column by default

What's wrong with <treecol id="folderSizeCol" hidden="true"> ?

Comment 22

[David :Bienvenu]

we would like to be able to have it show persistently if the user decides to
show it. AFAIK, hidden=true would make the user have to select it from the
column picker every time they run mozilla. Maybe in conjunction with persist =
"hidden" that wouldn't be true, but I don't believe it's as simple as you say.
Otherwise, why would we go through all the localstore.rdf grief that we do?

Comment 23

[[e-mail]]

> why would we go through all the localstore.rdf grief that we do?

I can't answer that; bookmarks and history use hidden="true" persist="hidden"

Comment 24

[R.K.Aa.]

can this fix have caused bug 178827? (Columns can't be resized anymore)

Comment 25

[David :Bienvenu]

yeah, it could have. I just noticed that. I have no idea why, however.

Comment 26

[laurel]

Verified.  Folder size column appears, using nov 20 commercial trunk build:
win98, linux rh8.0, Mac OS 10.2

Comment 27

[R.K.Aa.]

*** Bug 182996 has been marked as a duplicate of this bug. ***

Comment 28

[Jo Hermans]

*** Bug 182887 has been marked as a duplicate of this bug. ***

Comment 29

[laurel]

As stated in comment #26, column now appears in folder pane. It is off by default.
Any other issues with the folder size column will be reported separately and
will be tested in conjunction with IMAP/POP and other account types separately. 
cc gchan and esther, who will keep an eye on the column relative to some of
their testing for IMAP/POP.