Closed Bug 469053 Opened 16 years ago Closed 15 years ago

Autosync progress should be shown on the Activity Manager window

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: bugmil.ebirol, Assigned: davida)

References

(Depends on 1 open bug)

Details

(Whiteboard: [no l10n impact])

Attachments

(2 files, 9 obsolete files)

Currently autosync displays its progress on the status bar. Status bar messages need to be replaced by activity manager processes and events.

Each process should be grouped by account.
Flags: blocking-thunderbird3?
Attached patch Take 1 (obsolete) — Splinter Review
Essentially it is done. Waiting for activity manager to be completed before submitting the final cut.
Attached patch Rev 2 (obsolete) — Splinter Review
Localization is completed as well. Strings are not approved by clarbw yet.

When applied after bug 257942, it can be used for a test harness.
Attachment #352461 - Attachment is obsolete: true
Attachment #353757 - Flags: review?(bugzilla)
Assignee: nobody → bugmil.ebirol
Attachment #353757 - Flags: review?(bugzilla) → review-
Comment on attachment 353757 [details] [diff] [review]
Rev 2

> var gAutoSyncManager;
>-const nsIAutoSyncMgrListener = Components.interfaces.nsIAutoSyncMgrListener;
>+var gActivityManager;

I don't think this should be a global. Only gAutoSyncMonitor uses it, and therefore we should be defining it in there. Although extensions may wish to use it, it is not hard for them to keep their own copy, and it saves more confusion in the global namespace.

>+/**
>+ * The main purpose of this global object is to monitor auto-sync
>+ * activities, and representing them on the Activity Manager window.
>+ * (As a side benefit it can be used for debugging purposes by setting
>+ * _logEnabled to true.)
>+ *
>+ * Not every auto-sync activity is directly  mapped to a process or event.

nit: double space

>+  get msgWindow()
>+    {
>+      return this._msgWindow;
>+    },

These brackets (and the rest in the file) should be like:

get msgWindow() {
  return this._msgWindow;
}

>+  createSyncMailEvent : function(syncItem)
>+    {
>+      // extract the relevant parts
>+      let process = syncItem.activity;
>+      let folder = syncItem.syncFolder;
>+      

nit: whitespace on blank lines need fixing please (various places, you can check here: http://beaufour.dk/jst-review/)

>+      // create an activity event
>+      let event = new nsActEvent(this.formatEventDisplayTextString(folder.server.prettyName), 
>+                                 gAutoSyncManager,
>+                                 this.formatEventStatusTextString(
>+                                    this._syncInfoPerServer[folder.server].totalDownloads),
>+                                 this._syncInfoPerServer[folder.server].startTime, 
>+                                 Date.now());               // completion time

Given the close links between the information here and in the activity process, would it be an idea to change the init method on nsIActivityEvent so you can pass it an nsIActivityProcess that then is used to get the start time/total downloads, thus saving them being stored separately.

>-          this.inQFolderList.push(folder);
>-          this.log("***Auto_Sync OnFolderAddedIntoQ [" + this.inQFolderList.length + "] " + 
>+          try {
...
>+          }
>+          catch(e) {
>+            dump("$$$ Auto_Sync Error in onFolderAddedIntoQ: " + e + "\n");
>+          }

That's a big try catch that we won't be getting errors in the error console for - which may hide hidden mistakes etc, any particular reason for it? (there's two others here as well).

>+                // a folder removed notification even before a donwload

nit: download

>+            // if the process has not beed added to activity manager already, add now

nit: been

>+            process.state = Components.interfaces.nsIActivityProcess.STATE_INPROGRESS;
>+            let percent = (syncItem.totalDownloaded/syncItem.pendingMsgCount)*100;
>+            if (percent < syncItem.percentComplete)
>+              percent = syncItem.percentComplete;
>+            else
>+              syncItem.percentComplete = percent;
>+            
>+            let folderList = [folder.prettiestName];
>+            let accountList = [folder.server.prettyName];
>+            
>+            process.setProgress(percent,
>+                                this.formatProcessProgressString(syncItem.totalDownloaded,
>+                                                                 syncItem.pendingMsgCount, 
>+                                                                 folder.prettiestName),
>+                                numOfMessages, totalPending);
>+                                            

This seems like the wrong model for calculation, surely the nsIActivityProcess should be calculating the percentage?

>+  formatProcessProgressString : function(itemNo, itemCount, folderName)
>+    {
>+      if (!gMessengerBundle)
>+        gMessengerBundle = document.getElementById("bundle_messenger");
>+          
>+      return gMessengerBundle.getFormattedString("autosyncProcessProgress",
>+                                                 [itemNo, itemCount, folderName]);

I really don't like these. We should either be initialising gMessengerBundle straight off, or we should be working differently. As I know we're trying to reduce the amount of global variables, I suggest you make a bundle member variable in gAutoSyncMonitor, initialise it straight away and use that.

>+# LOCALIZATION NOTE: Do not translate the word "%1$S", "%2$S" and "%3$S" below.

I believe these are normally done like:

# LOCALIZATION NOTE(autosyncProcessProgress): Do not translate the word "%1$S", "%2$S" and "%3$S" below.

>+# Place the word %1$S in your translation where the number of the message being downloaded should appear.
>+# Place the word %2$S in your translation where the total number of pending messages should appear.
>+# Place the word %3$S in your translation where the name of the folder being processed should appear.
>+autosyncProcessProgress=Downloading %1$S of %2$S in %3$S
>+# Place the word %S in your translation where the name of the folder should appear.

You need to make these localization notes for each one of these strings.

>+autosyncProcessDisplayText=Syncing folder %S

I think this should be "Synchronizing folder %S" - Syncing isn't a real word, and we've got plenty of space here.

>+# Place the word %S in your translation where the name of account should appear.

nit: name of the account
I spotted two problems:

 1) This patch shows the on-going tasks as completed on the progress bar and this causes confusion. For example, if auto-sync downloads a large file as the last task, progress bar shows 100% while downloading the message that makes the user thinks that the operation is completed (same if auto-sync goes into sleep , this time the user sees 'Paused' and 100% completed message). I think this is what you have experienced.

 2) With MoMo's Zimbra and Cyrus accounts, when I move messages between folders, I see some messages get downloaded automatically even when auto-sync is disabled. I see this behavior the first time. Probably a recent patch causing it but I need to do more investigation. Although it doesn't affect auto-sync, it makes activity manager show wrong number of messages.
Wanted so bad we can taste it, but like the other parts, not blocking.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
Attached patch Combined revised patch (obsolete) — Splinter Review
I haven't handled all of the review comments yet, but this is a better version I think.  I've switched to log4moz (see comments in bug 257942 and see bug 475033), made some word changes w/ clarkbw, fixed localization and pluralization issues. 

attaching to help review of the main activity mgr patch.

(note: this patch also includes a revision of the changes in the patch for bug 469052, since keeping the two separate was hard)
Attachment #353757 - Attachment is obsolete: true
Blocks: 469052
This patch fixes a known issue with autosync activities. Also, sorts download queue without changing its size which is required for TB-Idle patch.
- Uses the new AutoSyncState to show the right number of pending messages. 
- Sets the progress bar after the messages are downloaded. 
- Puts sibling folder into Pause state if its priority changes. 

The only problem I see so far is "Paused. Will resume when Thunderbird is idle." message. It should be replaced with something more generic since we puts the activities on hold when another high priority folder is scheduled as well.
Attachment #358549 - Attachment is obsolete: true
Blocks: 471208
Attachment #358755 - Attachment is obsolete: true
I've just realised we've got a problem with where the autosync listener code is located. If anyone opens up more than one main window, or one main window and a standalone message window, then we're going to get multiple notifications in the activity manager of the same activity.

You can see this happening without the patch by turning on logging in gAutoSyncMonitor and opening more than one window.

The current code is situated fine for updating the status bar, but its not good for activity manager.

I'm still trying to think of ways around this, but my thoughts are currently that we're going to have to have a service that listens to the progress and pushes it to activity manager. For the autosync updates, we could include the code in the existing autosync manager, however that feels like we're merging UI and backend, and we'd need a set of ifdefs to go with it (as SM doesn't have the activity manager).

I'm going to be looking at something for bug 476698 today (send later progress), that may give some more ideas on how we can do this.
(In reply to comment #10)
> I'm going to be looking at something for bug 476698 today (send later
> progress), that may give some more ideas on how we can do this.

I've just attached a patch to that I think fixes this nicely by using a js module implementation to only load once for the running of the app.
(In reply to comment #11)

> I've just attached a patch to that I think fixes this nicely by using a js
> module implementation to only load once for the running of the app.

did you attach it to this bug or another?
(In reply to comment #12)
> (In reply to comment #11)
> 
> > I've just attached a patch to that I think fixes this nicely by using a js
> > module implementation to only load once for the running of the app.
> 
> did you attach it to this bug or another?

Sorry, I though the quoting of comment 10 would have made it clear - I attached the patch to bug 476698.
ah, somehow this was below my radar. What did you need my help with again?
Comment on attachment 359922 [details] [diff] [review]
revised patch using new way of setting contextDisplayText

>+let gActivityManager = Components.classes["@mozilla.org/activity-manager;1"].
>+  getService(Components.interfaces.nsIActivityManager);

nit: . on the start of the line please.

> var gAutoSyncMonitor = {
...
>+  _msgWindow : null,
>+  _inQFolderList : new Array(),
>+  _runnning : false,
>   
>+  _initLogging : function() {
>+    
>+    this.log = Log4Moz.getConfiguredLogger("autosync");

If we're changing the others to be prefixed by _ we should do the same here.

>+  get msgWindow()
>+    {
>+      return this._msgWindow;
>+    },

nit: use the:

get msgWindow() {
}

style and in the other places in this file.

>+      process.contextDisplayText = "Synchronizing Mail for " + folder.server.prettyName; // XXX l10n

We should fix this and the other XXX l10n for this patch.

>+  createSyncMailEvent : function(syncItem)
>+    {
>+      // extract the relevant parts
>+      let process = syncItem.activity;
>+      let folder = syncItem.syncFolder;
>+      
>+      // create an activity event
>+      let event = new nsActEvent(this.formatEventDisplayTextString(folder.server.prettyName), 

nit: whitespace on end of lines and whitespace on blank lines throughout the patch need fixing.

>+          process.state = Components.interfaces.nsIActivityProcess.STATE_INPROGRESS;
>+          let percent = (syncItem.totalDownloaded/syncItem.pendingMsgCount)*100;
>+          // XXX explain this?
>+          if (percent < syncItem.percentComplete)
>+            percent = syncItem.percentComplete;
>+          else
>+            syncItem.percentComplete = percent;

Hmm looks like this is trying to avoid going backwards in progress. Is it possible that more messages get added to the list as we proceed, and therefore this would make the percentage go in the wrong direction?

It can also just be:

if (percent > syncItem.percentComplete)
  syncItem.percentComplete = percent;

>+      if (!gMessengerBundle)
>+        gMessengerBundle = document.getElementById("bundle_messenger");
>+          
>+      return gMessengerBundle.getFormattedString("autosyncProcessProgress",
>+                                                 [itemNo, itemCount, folderName]);

We're going to be moving this to a module, so let's do this the Firefox way:

  get bundle() {
    delete this.bundle;
    let bundleSvc = Cc["@mozilla.org/intl/stringbundle;1"]
                      .getService(Ci.nsIStringBundleService);

    return this.bundle = bundleSvc
      .createBundle("chrome://messenger/locale/messenger.properties");
  },

This will mean it doesn't get loaded on startup, and when we do want it, it'll load the bundle for us.

Note that (again due to moving to a module) I think its best not to rely on document.getElementById and hence you'll have to change the function names to use GetStringFromName and formatStringFromName iirc (sendLater.js uses some of these).

>+      // get the folder of the moved/copied messages
>+      this.log.info("getting folder");
>+      let folder = aSrcMsgList.queryElementAt(0, Components.interfaces.nsIMsgDBHdr).folder;
>+      this.log.info("got folder");

Not sure we need this additional logging here.

>+      let statusText = '';
>+      // TODO: localize this string
>+      if (folder.server != aDestFolder.server)
>+      {
>+        statusText = this.getString("fromServerToServer");
>+        statusText = statusText.replace("#1", folder.server.prettyName);
>+        statusText = statusText.replace("#2", aDestFolder.server.prettyName);
>+      }
>+      else
>+      {
>+        statusText = folder.server.prettyName;
>+      }

This seems localized to me.

>+      let displayText;
>+      if (aMove)
>+        displayText = PluralForm.get(count, this.getString("movedMessages"));
>+      else
>+        displayText = PluralForm.get(count, this.getString("copiedMessages"));

nit: slightly cleaner:

let displayText = PluralForm.get(count,
                                 aMove ? this.getString("movedMessages") :
                                         this.getString("copiedMessages"));

>+# LOCALIZATION NOTE: Do not translate the word "%1$S", "%2$S" and "%3$S" below.

You need to make this:

# LOCALIZATION NOTE (autosyncProcessProgress): Do not translate the word "%1$S", "%2$S" and "%3$S" below.
>+# Place the word %1$S in your translation where the number of the message being downloaded should appear.
>+# Place the word %2$S in your translation where the total number of pending messages should appear.
>+# Place the word %3$S in your translation where the name of the folder being processed should appear.
>+autosyncProcessProgress=Downloading %1$S of %2$S in %3$S

>+# Place the word %S in your translation where the name of the folder should appear.
>+autosyncProcessDisplayText=Bringing folder %S up to date
>+# Place the word %S in your translation where the name of account should appear.
>+autosyncEventDisplayText=%S is up to date
>+# Place the word %S in your translation where the total number of downloaded messages should appear.
>+autosyncEventStatusText=Total number of messages downloaded: %S

Please add an appropriate LOCALIZATION NOTE (...): prefix to these as well.

>+# Message actions that show up in activity manager
>+deletedMessages=Deleted #1 message;Deleted #1 messages
>+movedMessages=Moved #1 message from #2 to #3;Moved #1 messages from #2 to #3
>+copiedMessages=Copied #1 message from #2 to #3;Copied #1 messages from #2 to #3
>+fromServerToServer=from #1 to #2
>+deletedFolder=Deleted folder #1
>+movedFolder=Moved folder #1 into folder #2
>+copiedFolder=Copied folder #1 into folder #2
>+renamedFolder=Renamed folder #1 to #2

Again, these should have localization notes associated with them.
This patch uses the new model for dealing w/ activities, which is much nicer.

Only regression is temporarily removing status bar notifications, as the module doesn't have trivial access to the status bars.  We're going to be replacing the status bar system with something that derives from the activity manager system, so I don't think this is a serious regression.  Also, I suspect that we'll just reuse existing strings, to fixing that regression will not have l10n impact.
Assignee: bugmil.ebirol → david.ascher
Attachment #359922 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #361691 - Flags: review?(bugzilla)
Attached patch more nits addressed (obsolete) — Splinter Review
whitespace cleanup among other minor tweaks.
Attachment #361691 - Attachment is obsolete: true
Attachment #361693 - Flags: review?(bugzilla)
Attachment #361691 - Flags: review?(bugzilla)
Emre, is the 4th patch on this bug ready for review?  If so, ask bienvenu for it.
Ok, I don't normally like doing this, but I'm strapped for time - so I've landed the strings with r=me.

With the patch I'm getting:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "numOfMessages is not defined" {file: "file:///Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/activity/autosync.js" line: 142}]' when calling method: [nsIAutoSyncMgrListener::onFolderRemovedFromQ]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

when its synchronising. Possibly simple to fix, but like I said I'm strapped for time.
Attached patch patch w/ fix (obsolete) — Splinter Review
sorry, that was a stupid bug.  this one fixes it.  seems to work fine too =)
Attachment #361693 - Attachment is obsolete: true
Attachment #361716 - Flags: review?(bugzilla)
Attachment #361693 - Flags: review?(bugzilla)
Flags: blocking-thunderbird3- → blocking-thunderbird3+
Whiteboard: [no l10n impact]
Comment on attachment 358754 [details] [diff] [review]
Adding a new attribute to AutoSyncState to get the size of the download queue. Also sort the newly added messages without changing the size of the download queue.

This patch adds a new attribute to auto-sync to simplify activity manager's job to show the progress. I have updated David's patch while ago to demonstrate the usage (see patch "Fixes wrong progress bar value and pending message count hack"). 

If David's latest patch is not based on "Fixes wrong progress bar value and pending message count hack" patch, it will require an update after this patch is landed.
Attachment #358754 - Flags: superreview?(bienvenu)
Attachment #358754 - Flags: review?(bienvenu)
Comment on attachment 358754 [details] [diff] [review]
Adding a new attribute to AutoSyncState to get the size of the download queue. Also sort the newly added messages without changing the size of the download queue.

this overwrites the prev definition&value of rv, which I don't think is intended. Since this is an accessor that never fails, you could just treat it as void.

     rv = aAutoSyncStateObj->DownloadMessagesForOffline(messagesToDownload);
     
+    PRInt32 totalCount;
+    nsresult rv = aAutoSyncStateObj->GetTotalMessageCount(&totalCount);
+    NS_ENSURE_SUCCESS(rv, rv);
+    

this:

         if (mOffset > 0)
-        {
-          // get rid of the downloaded message keys and reset the offsets
-          mDownloadQ.RemoveElementsAt(0, mOffset);
-          mOffset = mLastOffset = 0;
-        }
+          rv = SortQueueBasedOnStrategy(mDownloadQ, mOffset);
+        else
+          rv = SortQueueBasedOnStrategy(mDownloadQ);

could just be

rv = (mOffset > 0) ? SortQueueBasedOnStrategy(mDownloadQ, mOffset) : SortQueueBasedOnStrategy(mDownloadQ);
;

Part of me thinks the flavor of SortQueueBasedOnStrategy that takes an offset should be called SortSubQueue, but I want to poke around a little bit and see if there's an easier way to this.
Comment on attachment 358754 [details] [diff] [review]
Adding a new attribute to AutoSyncState to get the size of the download queue. Also sort the newly added messages without changing the size of the download queue.

I'll address the comments myself since I have to test this patch anyway, and land it if it's OK...
in the interest of time, I've made the changes myself, and this is what I'll check in. DavidA, please let me know if this doesn't fix the problem you're seeing...
Attachment #358754 - Attachment is obsolete: true
Attachment #361840 - Flags: superreview?(bienvenu)
Attachment #361840 - Flags: review?(bienvenu)
Attachment #358754 - Flags: superreview?(bienvenu)
Attachment #358754 - Flags: review?(bienvenu)
Attachment #361840 - Attachment description: fix addressing my comments, and some whitespace cleanup → fix addressing my comments, and some whitespace cleanup - checked in
Attachment #361840 - Flags: superreview?(bienvenu)
Attachment #361840 - Flags: superreview+
Attachment #361840 - Flags: review?(bienvenu)
Attachment #361840 - Flags: review+
Comment on attachment 361840 [details] [diff] [review]
fix addressing my comments, and some whitespace cleanup - checked in

thx for the patch, Emre!
Comment on attachment 361716 [details] [diff] [review]
patch w/ fix

after talking to davida, I'll look at this, but if you have any thoughts, Standard8, please let me know...
Attachment #361716 - Flags: review?(bugzilla) → review?(bienvenu)
Comment on attachment 361716 [details] [diff] [review]
patch w/ fix

so far, with this patch on windows, I'm not seeing any autosync output, not in the main window, nor in the activity manager. I'll poke around later. I am seeing output from move/copy operations, though.
Comment on attachment 361716 [details] [diff] [review]
patch w/ fix

I've not had problems invoking auto sync, the best way I've found is to move or copy a large message from a local folder to an imap server.

>+/**
>+ * This code aims to mediate between the auto-sync code and the activity mgr.
>+ *
>+ * Not every auto-sync activity is directly  mapped to a process or event.
>+ * To prevent a possible event overflow, Auto-Sync monitor generates one
>+ * sync'd event per account when after all its _pending_ folders are sync'd,
>+ * rather than generating one event per folder sync.
>+**/

nit: only one star on final line.

>+  onFolderAddedIntoQ : function(queue, folder) {
>+    try {
>+      if (folder instanceof Components.interfaces.nsIMsgFolder)
>+      {

nit: although we've copied and pasted this, let's fix the rest of the brackets to our normal js style.

if () {
}
else {
}

etc.

>+  init: function() {
>+    // XXX when do we need to remove ourselves?
>+    this.log.info('initing');
>+    let autoSyncManager = Components.classes["@mozilla.org/imap/autosyncmgr;1"]
>+                                         .getService(Components.interfaces.nsIAutoSyncManager);
>+    autoSyncManager.addListener(this);
>+  },

I'm not 100% convinced yet, but I think we get away without removing ourselves. If we did want to do it, then the way to do it would be to observe xpcom-shutdown and remove ourselves then. If we did this it would be best to do it in the central activityModules.js.

Probably watching the bloat boxes when we check in is the best way of determining a leak.
I've copied the pinstripe account manager theme stuff to qute and it works OK, though it obviously doesn't look very qute.
Comment on attachment 361716 [details] [diff] [review]
patch w/ fix

in addition to standard8's comments:

+            this._inQFolderList.splice(i,1);

need space after "i,"

don't need temp var here:

+    let autoSyncManager = Components.classes["@mozilla.org/imap/autosyncmgr;1"]
+                                         .getService(Components.interfaces.nsIAutoSyncManager);
+    autoSyncManager.addListener(this);

these vars could be lets:

+  getFolderListString : function() {
+    var folderList;
+    if (this._inQFolderList.length > 0)
+      folderList = this._inQFolderList[0].prettiestName;
+
+    for (var i = 1; i < this._inQFolderList.length; i++)
+      folderList = folderList + ", " + this._inQFolderList[i].prettiestName;
+
+    return folderList;
+  },
+
+  getAccountListString : function() {
+    var accountList;
+    if (this._inQFolderList.length > 0)
+      accountList = this._inQFolderList[0].server.prettyName;
+
+    for (var i = 1; i < this._inQFolderList.length; i++)
+    {
+      // no do repeat already existing account names
+      if (accountList.search(this._inQFolderList[i].server.prettyName) == -1)
+        accountList = accountList + ", " + this._inQFolderList[i].server.prettyName;
+    }
+    return accountList;
+  },
+

these local vars don't appear to be used:

+          let folderList = [folder.prettiestName];
+          let accountList = [folder.server.prettyName];
+          let msg = this.bundle.formatStringFromName("autosyncProcessProgress",

no need for temp var here:

+              let event = this.createSyncMailEvent(syncItem);
+              this.activityMgr.addActivity(event);

same holds for syncItem here:

+          let syncItem = this._syncInfoPerFolder[folder.URI];
+          let process = syncItem.activity;
+          if (process instanceof Components.interfaces.nsIActivityProcess)

davida, I can make these changes and standard8's myself and land - let me know if that's not OK...
this is what I propose to check in.

I have not dealt with the possible need to unregister the activity manager as a listener to autosync...
Attachment #361716 - Attachment is obsolete: true
Attachment #362515 - Flags: review?(bienvenu)
Attachment #361716 - Flags: review?(bienvenu)
this has been checked in - it won't show up until the activity manager is actually turned on, however.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 362515 [details] [diff] [review]
patch addressing comments - checked in.

checked in
Attachment #362515 - Flags: review?(bienvenu) → review+
Attachment #362515 - Attachment description: patch addressing comments → patch addressing comments - checked in.
You need to log in before you can comment on or make changes to this bug.