469050 - Compacting operation should be shown on Activity Manager window

Status: NEW

(Thunderbird :: General, enhancement, P3)

Description

[Emre Birol]

This operation should be represented to the user in form of activity manager events.

Comment 1

[David Ascher (:davida)]

wanted, not blocking.

Comment 2

[Steffen Wilberg]

Would be nice since compacting takes a lot of time, and sometimes stalls.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to comment #2)
> Would be nice since compacting takes a lot of time, and sometimes stalls.

and causes issues which, if time stamped, might help resolve problems

Comment 5

[Marcel Guzman]

Hi all, I have a patch for this. Just got it working at 2am so will work on polishing it off and submitting it for review in the next couple of days!

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #5)
> Hi all, I have a patch for this. Just got it working at 2am so will work on
> polishing it off and submitting it for review in the next couple of days!

I'm assigning the bug to you - so others know someone is working on this.

Comment 7

[Marcel Guzman]

Attachment: Adds Folder Compacting to Activity Manager

Thanks Ludovic :) Here's the patch, there are no tests that go along with it, but I need to submit this now for my course. If you guys need some unit tests to go with this, I'll still be available after my course is over. Cheers

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 440405 [details] [diff] [review]
Adds Folder Compacting to Activity Manager

>+++ b/mail/components/activity/modules/folderCompacting.js
>@@ -0,0 +1,166 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging.

This should be either "The Mozilla Foundation.", or "Marcel Guzman.", as you like.  (You wrote it, you can claim it, if you want.)

>+ * Portions created by the Initial Developer are Copyright (C) 2009

This should be 2010.

>+const EXPORTED_SYMBOLS = ['folderCompactingModule'];

Please use "s instead of 's.

[snip…]
>+  get bundle() {
>+    delete this.bundle;
>+    let bundleSvc = Cc["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Ci.nsIStringBundleService);
>+
>+    return this.bundle = bundleSvc
>+      .createBundle("chrome://messenger/locale/activity.properties");

Please put the .createBundle on the same line as the bundleSvc, and the "chrome…" on the next line.

>+  getString: function(stringName) {

This function needs a (-nother) name.  Try:
  getString: function fcm_getString(stringName) {
instead.  (Also, please name your parameters like "aStringName", or in this case, just "aName".)

>+  itemEvent: function(aItem, aEvent, aData) {
>+    let aFolder = null;
>+
>+    try {
>+      aFolder = aItem.QueryInterface(Components.interfaces.nsIMsgFolder);
>+    } catch (e) {
>+      // this is not a folder item event.
>+      return;
>+    }

Shouldn't we just check for "aFolder == null" after the QueryInterface?  (I'm worried that this might hide actual exceptions.)

>+    if (aEvent == "FolderCompactStart")
>+    {
>+      let displayText = this.getString("compactingFolder").replace("#1", aFolder.prettiestName);

Please put the ".replace" on its own line, lined up with the ".getString".

>+    else if (aEvent == "FolderCompactProgress")
>+    {
>+      try {
>+        let aDataAsNumber = aData.QueryInterface(Components.interfaces.nsISupportsPRUint32);

Can you use "Ci.nsISupportsPRUint32" here, to make this line shorter?

>+        let displayText = this.getString("compactingFolderProgress").replace("#1", aFolder.prettiestName)
>+                                                                    .replace("#2", aDataAsNumber);
>+

Please have both the ".replace" calls on their own lines, lined up with the ".getString".

>+        this.currentJobs[aFolder.folderURL].process.state = Ci.nsIActivityProcess.STATE_INPROGRESS;
>+        this.currentJobs[aFolder.folderURL].process.setProgress(displayText, aDataAsNumber.data, 100);

Perhaps put the "setProgress" on its own line.


>+  init: function() {
>+    let notificationService = Components.classes["@mozilla.org/messenger/msgnotificationservice;1"]

I think you can use Cc here.

>+                                 .getService(Components.interfaces.nsIMsgFolderNotificationService);

and Ci here.

>+++ b/mailnews/base/src/nsMsgFolderCompactor.cpp
>@@ -986,16 +988,33 @@ nsOfflineStoreCompactState::OnStopReques
>   }
> 
>   if (m_window)
>   {
>     m_window->GetStatusFeedback(getter_AddRefs(statusFeedback));
>     if (statusFeedback)
>       statusFeedback->ShowProgress (100 * m_curIndex / m_size);
>   }
>+  

You've got some trailing spaces on this line (and on some others) which should be removed.

>+    nsCOMPtr<nsISupportsPRUint32> container =
>+        do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID, &rv);

Two-space indents, please.

>@@ -1139,22 +1168,50 @@ nsFolderCompactState::EndCopy(nsISupport
>+    // notify observers of our compacting progress

And here.

>+  notifier = do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID);
>+  if (notifier)
>+  {
>+    nsCOMPtr<nsISupportsPRUint32> container =
>+        do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID, &rv);

And here.

So, next up, fix those changes, and post a new patch, asking a second person (perhaps bienvenu) for review , and since you've changed a file in mailnews, you'll need to get a superreview from someone (standard8 or bienvenu, maybe).

Also, you should really have tests, but we can review those in a separate patch.

Thanks,
Blake.

Comment 9

[Wayne Mery (:wsmwk)]

xref Bug 161752 - Implement modal progress for compacting local folders - which seems like a bad idea. perhaps wontfix after this bug is fixed?

Comment 10

[Wayne Mery (:wsmwk)]

Marcel,
Do you have an updated patch?
It would be nice to get this in for version 3.3. :)


(In reply to comment #8)
> So, next up, fix those changes, and post a new patch, asking a second person
> (perhaps bienvenu) for review , and since you've changed a file in mailnews,
> you'll need to get a superreview from someone (standard8 or bienvenu, maybe).
> 
> Also, you should really have tests, but we can review those in a separate
> patch.
> 
> Thanks,
> Blake.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Marcel, still working on this? I hope to have a patch that will de-bitrot some of the lines as well as add on bwinton's suggestions, but it may not be a complete fix.

Tests are also another tricky issue.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2 ignoring whitespace differences

This patch has ignored whitespace differences..

I've de-bitrotted mguzman's patch along with bwinton's suggested changes. Running this past Standard8 and bienvenu for their feedback as suggested.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #13)
> Created attachment 527543 [details] [diff] [review]
> patch v2 ignoring whitespace differences

Try server builds will hopefully be available soon at:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-859bf62801cc

Comment 15

[David :Bienvenu]

this all looks fine - my one worry is that we might be slowing down the compact process with all the events we're sending. I'd prefer that we don't send a notification unless the percentage has changed, i.e.,

100 * m_curIndex / m_size

is different from the last notification.

Comment 16

[David :Bienvenu]

Comment on attachment 527543 [details] [diff] [review]
patch v2 ignoring whitespace differences

minusing because I'd like to see that small optimization, but otherwise, it seems to work fine.

Given that we do already display compact progress and status on the status bar, I'm wondering from a UI p.o.v should we remove the status bar progress and status? Or have both?

Comment 17

[Blake Winton (:bwinton) (:☕️)]

We've got a few other things that show up in both, for instance <http://dl.dropbox.com/u/2301433/StatusBar.png>, so I don't see any problem having this in both places.

One other small optimization I think I would like to see is to also not send the update event unless an amount of time has passed.  (i.e. I don't want to see 100 updates, one per percentage point, for an operation that only takes 100ms.  In that case, I'ld prefer to see, say 5-10 updates, which works out to one every 10-20ms.)

Later,
Blake.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 527543 [details] [diff] [review]
patch v2 ignoring whitespace differences

Clearing feedback request as this needs a new patch.

Comment 20

[:aceman]

Marcel, are you going to finish this patch?

Comment 21

[Wayne Mery (:wsmwk)]

should not be assigned. still patchlove

Comment 22

[Thomas D. (:thomas8)]

Compacting would be quite important to have on activity manager, as it can interrupt workflows, so it's important to be able to check what's going on.