749099 - Show the space that will be or has been saved after compacting folders

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, enhancement)

Description

[Marco A.G.Pinto [:marcoagpinto]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

When it asked to compressed folders I selected yes.


Actual results:

It compressed the e-mail folders, but no report about how much space it was saved.


Expected results:

It should be improved in order to say how many space was saved.

Comment 1

[rsx11m]

Sounds useful, confirming as enhancement request. There is some indication how much space will be (or has been) freed up by the settings of the compact threshold, but some explicit feedback would be desirable (and at least for the compact dialog, that number should be available as it has just been calculated for that prompt).

Comment 2

[:aceman]

Yes, would be nice.

Comment 3

[:aceman]

We actually know the space to be saved even before the compaction starts, it is a property of the folder. So it isn't even needed to count it. This is just a matter of displaying it somewhere. As the compaction can be invoked manually without the prompt, I'd suggest to also show it when compaction ends. There already should be some "Done" message, so it can be appended there.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

A possible attempt.

Comment 5

[:aceman]

Comment on attachment 8417977 [details] [diff] [review]
Patch v1

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

Don't forget the Seamonkey strings.

I have not run with this yet, but it looks like doing the stuff this bug proposes. (When CompactedBytes is hooked up.)

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +66,5 @@
>  folderCreationFailed=The folder could not be created because the folder name you specified contains an unrecognized character. Please enter a different name and try again.
>  
>  compactingFolder=Compacting folder %S…
> +# LOCALIZATION NOTE(doneCompacting): %1$S is the compaction gain.
> +doneCompacting=Done compacting (approx. %1$S bytes).

(approx. %1$S bytes saved)

@@ +71,3 @@
>  autoCompactAllFoldersTitle=Compact Folders
> +# LOCALIZATION NOTE(autoCompactAllFolders): %1$S is the compaction gain.
> +autoCompactAllFolders=Do you wish to compact all local and offline folders to save disk space? (approx. %1$S bytes).

Do you wish to compact all local and offline folders to save disk space? This will save about %1$S bytes.

I wonder if we should just show rounded to megabytes. We do not save precisely the amount of bytes stated because of filesystem blocks/extents.

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +558,5 @@
>        
>    return rv;
>  }
>  
> +void nsFolderCompactState::CompactedBytes(nsIMsgFolder *folder)

Where is this actually called? Looks like it should be before each compacted folder?

@@ +596,5 @@
> +    nsCOMPtr <nsIStringBundle> bundle;
> +    nsCOMPtr<nsIStringBundleService> bundleService =
> +      mozilla::services::GetStringBundleService();
> +    bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
> +                                getter_AddRefs(bundle));

I don't like how we need to open-code this here. But semantically it is the right thing to do. The original code was parasiting on m_folder->GetStringWithFolderNameFromBundle while it didn't need "folder name" and actually had nothing to do with the specific "m_folder". It just happened that it produced the right result for now.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #5)
> I wonder if we should just show rounded to megabytes. We do not save
> precisely the amount of bytes stated because of filesystem blocks/extents.

Or KiB, but bytes are just hard to read...

Comment 7

[:aceman]

Or use FormatFileSize() from nsMsgUtils and it will do the magic;)

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Made the suggested changes.
Thanks.

Comment 9

[:aceman]

Comment on attachment 8419378 [details] [diff] [review]
Patch v2

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

This is getting good now :)

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +596,5 @@
> +    nsCOMPtr <nsIStringBundle> bundle;
> +    GetBaseStringBundle(getter_AddRefs(bundle));
> +    nsAutoString expungedBytes;
> +    FormatFileSize(m_totalExpungedBytes, true, expungedBytes);
> +    const char16_t* params[] = { expungedBytes.get() };

The expungedBytes variable no longer contains bytes (it may be KB, MB). So maybe name it expungedAmount or something.

::: mailnews/base/src/nsMsgFolderCompactor.h
@@ +65,5 @@
>  
>     // sum of the sizes of the messages, accumulated as we visit each msg.
>    uint64_t m_totalMsgSize;
> +  // bytes that can be expunged while compacting.
> +  uint32_t m_totalExpungedBytes;

Please make this uint64_t. We aim for 4GB+ folders :)

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +1944,5 @@
>            nsString checkboxText;
>            nsString buttonCompactNowText;
> +          nsAutoString compactSize;
> +          compactSize.AppendInt(totalExpungedBytes);
> +          const char16_t* params[] = { compactSize.get() };

FormatFileSize somewhere.

::: suite/locales/en-US/chrome/mailnews/messenger.properties
@@ +63,5 @@
>  folderCreationFailed=The folder could not be created because the folder name you specified contains an unrecognized character. Please enter a different name and try again.
>  
>  compactingFolder=Compacting folder %S…
> +# LOCALIZATION NOTE(doneCompacting): %1$S is the compaction gain.
> +compactingDone=Done compacting (approx. %1$S saved).

Unify doneCompacting and compactingDone .

Comment 10

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.4

Made the suggested changes.
Requesting review as I think this patch is ready.

Thanks.

Comment 11

[[e-mail]]

Comment on attachment 8420116 [details] [diff] [review]
Patch v2.4

>+autoCompactAllFoldersDialog=Do you wish to compact all local and offline folders to save disk space? This will save about %1$S.
autoCompactAllFoldersDialog looks odd, how about Text or Message?

>+  m_totalExpungedBytes = 0;
>   NS_ENSURE_ARG_POINTER(folder);
>   m_listener = aListener;
>   if (!m_compactingOfflineFolders && !aOfflineStore)
>   {
>     nsCOMPtr <nsIMsgImapMailFolder> imapFolder = do_QueryInterface(folder);
>     if (imapFolder)
>       return imapFolder->Expunge(this, aMsgWindow);
>   }
>+
>+   CompactedBytes(folder);
Not sure what you're trying to achieve here, why not call GetExpungedBytes directly? Also, how is this a total?

>+  bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
>+                                 getter_AddRefs(bundle));
>+  bundle.swap(*aBundle);
>+  return NS_OK;
return bundleService->CreateBundle(..., aBundle);

>+          compactSize.AppendInt(totalExpungedBytes);
???

Comment 12

[:aceman]

Comment on attachment 8420116 [details] [diff] [review]
Patch v2.4

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

I have tested this now and at least the "compact done" message works fine.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +1944,5 @@
>            nsString checkboxText;
>            nsString buttonCompactNowText;
> +          nsAutoString compactSize;
> +          compactSize.AppendInt(totalExpungedBytes);
> +          FormatFileSize(totalExpungedBytes, true, compactSize);

Yeah, compactSize.AppendInt() probably remnant from previous version. It is now overwritten by FormatFileSize so the appending can be dropped.

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.5

(In reply to [e-mail] from comment #11)
> >+  m_totalExpungedBytes = 0;
> >+
> >+   CompactedBytes(folder);
> Not sure what you're trying to achieve here, why not call GetExpungedBytes
> directly? Also, how is this a total?
Ya, this isn't a total in case of manual compact trigger..But do we need a total in that case? I mean, I can replicate the code for getting total expunged bytes for all the folders as in nsMsgDBFolder.cpp but that would be triggered only in case of a root folder (?) and another query would be, if one triggers a compact action on a folder that has subfolders, are we required to show the total expunged bytes for the entire folder+subfolders? Or, will this be covered by the getExpungedBytes() itself as the subfolder(s) are already inside the main folder we carried out our action on?
If yes, we don't need a total in this case as well. So, what should be done?

> >+          compactSize.AppendInt(totalExpungedBytes);
(In reply to :aceman from comment #12)
> Yeah, compactSize.AppendInt() probably remnant from previous version. It is
> now overwritten by FormatFileSize so the appending can be dropped.
Sorry, got rid of it.

Thanks.

Comment 14

[[e-mail]]

Comment on attachment 8421697 [details] [diff] [review]
Patch v2.5

(In reply to Suyash Agarwal from comment #13)
> Ya, this isn't a total in case of manual compact trigger..But do we need a
> total in that case? I mean, I can replicate the code for getting total
> expunged bytes for all the folders as in nsMsgDBFolder.cpp but that would be
> triggered only in case of a root folder (?) and another query would be, if
> one triggers a compact action on a folder that has subfolders, are we
> required to show the total expunged bytes for the entire folder+subfolders?
> Or, will this be covered by the getExpungedBytes() itself as the
> subfolder(s) are already inside the main folder we carried out our action on?
> If yes, we don't need a total in this case as well. So, what should be done?

There are two cases.
The first case directly invokes nsFolderCompactState::Compact with a single folder. The second case invokes nsFolderCompactState::CompactFolders which accepts multiple folders. This version actually invokes the first case for each folder to do the actual work of compaction. ShowDoneStatus is however only called once at the very end, so you will actually only end up showing the status for the last folder. This means that you should not reset the total expunged bytes (the object is created once per compact so you should clear the variable in the constructor).

I still don't think it's worth creating a separate function to increment the total expunged bytes.

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.7

Tried to make the suggested changes.

Comment 16

[:aceman]

Comment on attachment 8423734 [details] [diff] [review]
Patch v2.7

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

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +594,5 @@
> +    const char16_t* params[] = { expungedAmount.get() };
> +    nsresult rv =
> +      bundle->FormatStringFromName(MOZ_UTF16("compactingDone"),
> +                                   params, 1, getter_Copies(statusString));
> +    m_totalExpungedBytes = 0;

It appeared to me Neil considered this line useless as the object is destroyed shortly after this. You do not see any other member variables being cleared here.

Comment 17

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.8

(In reply to :aceman from comment #16)
> Review of attachment 8423734 [details] [diff] [review]:
Thanks for having a look at it.
> 
> @@ +594,5 @@
> > +    m_totalExpungedBytes = 0;
> 
> It appeared to me Neil considered this line useless as the object is
> destroyed shortly after this. You do not see any other member variables
> being cleared here.

Ya, I was wondering if I should remove it, but then left it as is.
Removing it now, thanks.

Comment 18

[[e-mail]]

Comment on attachment 8423810 [details] [diff] [review]
Patch v2.8

>+  bundleService->CreateBundle("chrome://messenger/locale/messenger.properties",
>+                              aBundle);
>+  return NS_OK;
Just noticed that this should be return bundleService->CreateBundle(...); [and check for failure in the caller].

>+compactingDone=Done compacting (approx. %1$S saved).
Hmm, this trailing full stop looks wrong somehow, although I was unable to determine any sort of consistency amongst other status messages.

Comment 19

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the suggested changes, thanks.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Compact prompt

Requesting ui-r.
Part 1

Comment 21

[Suyash Agarwal (:sshagarwal)]

Attachment: Compact statusbar message

ui-r part 2.
So, this patch provides the approximate compaction gain.
First screenshot shows the compaction suggestion prompt and second is the
status bar message that user gets after compaction.
Also, it'll be nice if you can help us in figuring out the text concern in
comment #18.
Please let me know if anything needs to be changed.

Thanks.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8424609 [details] [diff] [review]
Patch v3

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

I'm okay with the period at the end. If we are working on other texts without period we should look to change them.

Only one question because I don't saw the dialog on my tests. On your dialog screenshot (attachment 8424611 [details]) the "MB" is on a new line. Is this only on your system or does this always happen? Maybe it's possible with a non breaking space between the value and the "MB". If not it could be better to move the second sentence to the second line.
ui-r+ with a good solution for this.

Comment 23

[Suyash Agarwal (:sshagarwal)]

Attachment: anotherscreen.PNG

Ya, it was just on my screen I think.
I tried this on Windows and here they appear on the same line.

Going for check-in.

Thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/eb1f16aaeb50