323952 - no progress indicator when reloading remote calendars

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Calum Mackay]

Linux/x86, my build from cvs; 20060118

With the old views, the Calendars tab in the sidebar used to show a nice little revolving progress indicator when reloading remote calendars, one for each calendar.

The new views seem to have no indication of progress, or even whether the reload ever happened.

Comment 1

[Joey Minta]

This shouldn't be view related.  Are you sure this working in 0.3a1?

Comment 2

[Calum Mackay]

That's a good point, sorry; it's late here.

I remember it having a nice indicator before, and now it doesn't. Of course, that doesn't mean that it went away when the new views came in.

So this is a general regression against earlier code, but not sure off-hand when it was last there...

Comment 3

[Stefan Sitter [:ssitter]]

Related to Bug 318016?

Comment 4

[Calum Mackay]

Certainly related, although with the reload-on-startup bug I'm not sure they would be able to open the calendar sidebar to see any progress indicator, if it were there.

Given that, and that I see this as a regression, since the indicator was there, once, I'm inclined to suggest that we need both bugs. Thoughts?

Comment 5

[Joey Minta]

I'm not really sure what benefit Bug 318016 would provide once this bug is fixed.  I'm inclined to leave that bug sit and re-evaluate it after this one is fixed.  This one should be a high priority though.  I had to hack up some dump()s at calconnect to work around it, once I realized how useful it was.  I looked at this a bit last night.  To me, the way to solve it would be to add an onStartLoad to calIObserver, that can fire when a load begins.  With this, we can simply bring back the old code from 0.2, (the css classes and icons seem to be in tact).  mvl may have more to say on this though, since he does a lot of the ics stuff.

Comment 6

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

Adding onStartLoad sounds fine to be, as long as we make sure (and document in the  .idl) that if a providers calls onStartLoad, it must call onFinishLoad. onLoad might not be usuable for this, for cases where the load fails.

Comment 7

[Gary van der Merwe]

Attachment: Patch 0.1

Ok - this is what I have done so far:
* Added onStartLoad, onFinishLoad, onLoadProgress to calICalendar
* Added these methods to all classes that implement calICalendar
* calICSCalendar and calCompositeCalendar call onStartLoad and onFinishLoad appropriately.
* Made the main app throbber work.

Still to do:
* Make throbbers in list work.
* Call observers from other providers (CalDAV, ???)
* Implement onLoadProgress, and add progress bar to status bar.

But first I need to get some sleep... :-)

Comment 8

[Matthew (lilmatt) Willis]

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

Comment 9

[Gary van der Merwe]

>the css classes and icons seem to be in tact

The css rules for the main app throbber were still there - but I can't find any for the calendar list - so I'm going to have to add them.

Comment 10

[Gary van der Merwe]

Attachment: Patch 0.2

+ Calendar List Throbbers
+ Throbbers work on app startup

Still to do
* CalDAV
* Progress Bar

Comment 11

[Gary van der Merwe]

Attachment: Patch 0.3

+ Progress bar
+ CalDAV
+ No longer relies on calCompositeCalendar

Still ToDo:
* Progress bar for CalDAV - A little harder because it is done over multiple requests.

Comment 12

[Gary van der Merwe]

Bug 308567 will give us the ability to attach notificationCallbacks to channels created by nsWebDAVService which is needed here to get the progress bar working in CalDAV.

Comment 13

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!

Comment 14

[Martin Schröder [:mschroeder]]

No progress for more than a year -> Re-assigning to nobody.

Comment 15

[Daniel Boelzle [:dbo]]

We definitely need progress UI for 0.9.

Comment 16

[Berend Cornelius]

Attachment: patch v. 1.0

I admit I had a hard time to implement this! One major problem was that our calendar calls are asynchronously and they may conflict the the thunderbird calls. But we certainly want to try to avoid that. Therefor I came to the following solution: 
Basically I only display an undetermined progress indicated by the busy navigator-throbber in the upper right. This progress display is triggererd on every getItems call in the composite Calendar.
When an explicit refresh is demanded I also display a determined progress bounded by the number of registered calendars where the statusvalue is increased on the completion of each calendar.

What I did not like so much but for which I did not find a solution is that on my SunRay-machine the repaint is very slow and normally even slower than the summary of the actual server calls. But the repaint is not considered in my implementation The progress display stops on the last "operationCompleted" call. 

Probably my patch is not yet the last conclusion of wisdom. But I hope we can take it anyway as a first step.

Comment 17

[Christian Jansen]

Comment on attachment 320422 [details] [diff] [review]
patch v. 1.0

Thanks for the patch.
I have reviewed patch on mac. The following issue occurred:

The busy
navigator-throbber in the upper right is not displayed. It would be great if we can get this fixed before this patch gets in. Thanks. r=christian

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 320422 [details] [diff] [review]
patch v. 1.0

>+    QueryInterface: function (aIID) {
>+        if (!aIID.equals(Components.interfaces.nsISupports) &&
>+            !aIID.equals(calIStatusObserver)) {
>+            throw Components.results.NS_ERROR_NO_INTERFACE;
>+        }
>+        return this;
>+    },
use doQueryInterface from calUtils

>+
>+     initialize: function initialize() {
>+        this.mWm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                       .getService(Components.interfaces.nsIWindowMediator);
>+        this.mWindow = this.mWm.getMostRecentWindow("calendarMainWindow") ||
>+                    this.mWm.getMostRecentWindow("mail:3pane");
As discussed, this might not always get us the right window. Should be fixed.

>+        this.mStatusText = this.mWindow.document.getElementById("statusText");
>+        this.mStatusBar = this.mWindow.document.getElementById('statusbar-icon');
Lets stay consistant with quotes. I'd suggest double quotes.

>+        this. mInitialized = true;
Extra space

>+     get Spinning() {
>+         return this.mProgressMode;
>+     },
if possible, lower case S.

>+                 this.mCalendarStep = parseInt(100/this.mCalendarCount);
Spaces around operators. Also, add 10 as a second parameter to make sure the right conversion is used.

>+             if (this.mThrobber){
Space

>+     stopMeteors: function stopMeteors() {
Please unanonymize the function and also the getters/setters

>+     }
>+     catch (ex) {
>+       // can get here if closing window when running filters
>+     }},
Function is incorrectly indented? Or why are there two }}.

>+//                 var promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+//                                                .getService(Components.interfaces.nsIPromptService);
>+//                 promptSvc.alert(this.mWindow, aCalendar.name, this.mCurIndex);
Remove comments

>+[scriptable, uuid(60160f68-4514-41b4-a19d-2f2cf0143426)]
>+interface calIStatusObserver : nsISupports
calICalendar.idl is already full of different things. Please create a new file.

>+  /**
>+   * A constant that denots that no operation is running
>+   */
>+  const unsigned long NO_PROGRESS = 3;
Wouldn't it make sense to make NO_PROGRESS = 0, since its the default?

>+    getCompositeCalendar().statusObserver = gCalendarStatusFeedback;
>+    getCompositeCalendar().statusObserver = gCalendarStatusFeedback;

I'd suggest to set up the status observer inside getCompositeCalendar() when it is instanciated. This way it only has to be done once and not for each app.

> # Master Password
>-changeMasterPassword=Change Master Password…
>+changeMasterPassword=Change Master Password\u00E2\u0080\u00A6
Something is wrong with this file, UTF8 broken?

>+
>+progressmeter {
>+  -moz-appearance: progressbar;
>+  margin: 2px 4px;
>+  min-width: 128px;
>+  height: 12px;
>+}
>+
>+/* ::::: statusbar progressmeter ::::: */
>+.progressmeter-statusbar {
>+  margin: 0;
>+  border-width: 1px;
>+}
Why does sunbird need the progressmeter styles here?



r- for now to fix the window

Comment 19

[Berend Cornelius]

Attachment: patch v. #2

overworked the patch...

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 322080 [details] [diff] [review]
patch v. #2

>+        var aChromeWindow = window.QueryInterface(Components.interfaces.nsIDOMChromeWindow);    
>+        gCompositeCalendar.addStatusObserver(gCalendarStatusFeedback, aChromeWindow);
I'd prefere to drop the a (i.e var chromeWindow), since aChromeWindow is not an argument of the function itself.

>+ var gCalendarStatusFeedback = {
>+     mCalendarValue : 0,
>+     mCalendarStep : 0,
>+     mCalendarCount : 0,
>+     mWindow : null,
>+     mStatusText : null,
>+     mStatusBar : null,
>+     mStatusProgressPanel : null,
>+     mThrobber : null,
>+     mProgressMode : Components.interfaces.calIStatusObserver.NO_PROGRESS,
>+     mCurIndex : 0,
>+     mInitialized : false,
>+     mProps : null,
Remove spaces

>+
>+    QueryInterface: function (aIID) {
Make QI non-anonymous, and any other functions that might be anonymous. In my code I'd call the function names something like gCSF_QueryInterface(), I guess thats up to you.

>+     initialize: function initialize(aWindow) {
>+            this. mInitialized = true;
There is an extra space here.

>+++ base/public/calIStatusObserver.idl	22 May 2008 09:29:33 -0000
>+ * Portions created by the Initial Developer are Copyright (C) 2005
This code is from 2005?

>+#include "nsISupports.idl"
>+#include "calICalendar.idl"
I believe calICalendar only needs to be forward-defined

>+ void initialize( in nsIDOMChromeWindow aWindow );
remove spaces

>+
>+ /**
>+  * starts the display of an operation to check a series of calendars
>+  * This operation may either be determined or undetermined
>+  * @param aProgressMode an integer value that can accept DETERMINED_PROGRESS,
>+  * UNDETERMINED_PROGRESS or NO_PROGRESS
Please indent wrapped @param lines and add some more spaces after aProgressMode. Here and in other places. For bonus points, use capital letters at the beginning of sentances. (sorry for nitting grammar, but I think code looks much more professional if capitalization is correct)

>+    addStatusObserver: function(aStatusObserver, aWindow){
>+        this.mStatusObserver = aStatusObserver;
>+        this.mStatusObserver.initialize(aWindow);
If this function only sets a status observer, then this function could be named differently, since its not possible to add multiple status observers. Also, there are some anonymous functions here that could deserve names.

>Index: resources/content/calendar.js
>+    getCompositeCalendar().statusObserver = gCalendarStatusFeedback;
I must have missed it, do you do this for both apps? I think this is better put in calendar-mangement.js where getCompositeCalendar() is defined. Set the status observer there directly. If you don't like the fact that setting the status feedback is far away from the status code, I'd at least suggest to put it in calendar-chrome-startup.js' commonInitCalendar().


r=philipp

Comment 21

[Berend Cornelius]

Attachment: patch v. #3

Worked in the issues that philipp mentioned. Attached is the modified patch that I just checked in. Thank you Philipp for your effort.

patch checked in on trunk and MOZILLA_1_8_BRANCH

->FIXED

Comment 22

[Hubert Gajewski]

Why did you use ASCII instead of UTF-8 in gettingCalendarInfoCommon?

Comment 23

[Andreas Treumann]

Checked in lightning 2008090403 and sunbird 20080903 -> VERIFIED.

I talked with Berend, encoding issue is solved.