313502 - importer should notify observer with onBatchStart

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

Importing events is slow, because it doesn't tell anything that a batch of changes is coming. This means that the views will refresh after every item. This is very, very slow. Even slow enough to make this bug block 0.3a1

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

THis is one way of solving the problems. It moves the problem to the callers, which need to call startBatch. It also forces all providers to forward the call.
I think we have to go with this for now.
There are some other ways to solve the redrawing issue, but those are more complicated, and won't be fixed for 0.3a1.

(those other ways may be: making the views autodetect mass changes, making the view only redraw when the changed item is actually in the viewed daterange)

(other future improvement: a different observer infrastrure where there is a central 'hub', so that for example the batch notifications don't go through the providers)

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200526 [details] [diff] [review]
patch v1

>Index: base/content/import-export.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/content/import-export.js,v
>retrieving revision 1.4
>diff -u -9 -p -d -r1.4 import-export.js
>--- base/content/import-export.js	28 Sep 2005 18:28:24 -0000	1.4
>+++ base/content/import-export.js	23 Oct 2005 17:58:39 -0000
>@@ -110,21 +110,35 @@ function loadEventsFromFile()
>            alert(getStringBundle().GetStringFromName("unableToRead") + filePath + "\n"+ex );
>         }
> 
>         // XXX Ask for a calendar to import into
>         var destCal = getDefaultCalendar();
> 
>         // XXX This might not work in lighting
>         startBatchTransaction();
>         
>+        destCal.startBatch();
>+

Since there are now two "startBatch"-type and two "endBatch"-type calls in a row, can you add commentary explaining that the outer one is for the undo/redo transaction manager batch?  

>+        // This listener is needed to find out when the last addITem really
>+        // finished. Using a counter o find the last item (which might not
>+        // be the last item added)
>+        var count = 0;
>+        var listener = {
>+            onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+                count++;
>+                if (count == items.length)
>+                    destCal.endBatch();
>+            }
>+        }
>+        
>         for each (item in items) {
>             // XXX prompt when finding a duplicate.
>-            destCal.addItem(item, null);
>+            destCal.addItem(item, listener);
>         }

I think we need some sort of error-handling here.  Can you put a try block around the addItem call as well as test the status in the callback, and cause an error dialog to be show if there's a problem in either case?

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated import-export.js

updated import-export.js to review comments. Also remove the use of alert(), and replaced it with nsIPromptService.alert()

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200631 [details] [diff] [review]
updated import-export.js

>+        // This listener is needed to find out when the last addItem really
>+        // finished. Using a counter o find the last item (which might not
>+        // be the last item added)
>+        var count = 0;
>+        var failedCount = 0;
>+        var lastError;
>+        var listener = {
>+            onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail) {
>+                count++;
>+                // See if it is time to end the calendar's batch.
>+                if (count == items.length)
>+                    destCal.endBatch();
>+                if (failedCount)
>+                    showError(failedCount+" items failed to import. The last error was: "+lastError.toString());
>+            }
>+        }

There still needs to be a check of aStatus here, because errors are at least as likely to be called back this way than thrown during the initial call to addItem.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: v3

updated patch to do an extra error check

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200785 [details] [diff] [review]
v3

As nearly as I can tell, this patch is completely missing the diffs for the file that would have changed since the last version.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: now with import-export.js

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 200911 [details] [diff] [review]
now with import-export.js

Almost there!

>+        // This listener is needed to find out when the last addItem really
>+        // finished. Using a counter o find the last item (which might not

I bet "counter o" wants to be "counter to".
    
>         for each (item in items) {
>             // XXX prompt when finding a duplicate.
>-            destCal.addItem(item, null);
>+            try {
>+                destCal.addItem(item, listener);
>+            } catch(e) {
>+                failedCount++;
>+                lastError = e;
>+                // Call the listener's operationComplete, to increase the
>+                // counter and not miss failed items. Otherwise, endBatch might
>+                // never be called.
>+                listener.onOperationComplete(null, null, null, null, null);
>+                throw (e);
>+            }
>         }

If that catch clause is triggered on any item other than the last one, endBatch is guaranteed to never be called, because you throw immediately, so the for loop will never finish.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated patch

replaced throw with Components.utils.reportError. That way there will still be a list of all errors somewhere, without showing 1000 dialogs

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 201158 [details] [diff] [review]
updated patch

r=dmose

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in