Closed Bug 323952 Opened 16 years ago Closed 14 years ago

no progress indicator when reloading remote calendars

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: calum.mackay, Assigned: berend.cornelius09)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
This shouldn't be view related.  Are you sure this working in 0.3a1?
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...
No longer blocks: 321164
Related to Bug 318016?
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?
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.
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.
Status: NEW → ASSIGNED
Assignee: mostafah → garyvdm
Status: ASSIGNED → NEW
Attached patch Patch 0.1 (obsolete) — Splinter Review
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... :-)
OS: Linux → All
Hardware: PC → All
*** Bug 268602 has been marked as a duplicate of this bug. ***
>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.
Blocks: 249277
Attached patch Patch 0.2 (obsolete) — Splinter Review
+ Calendar List Throbbers
+ Throbbers work on app startup

Still to do
* CalDAV
* Progress Bar
Attachment #218074 - Attachment is obsolete: true
Attached patch Patch 0.3 (obsolete) — Splinter Review
+ Progress bar
+ CalDAV
+ No longer relies on calCompositeCalendar

Still ToDo:
* Progress bar for CalDAV - A little harder because it is done over multiple requests.
Attachment #218376 - Attachment is obsolete: true
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.
Depends on: 308567
Component: Sunbird and Calendar-Extension Front End → Base
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: sunbird → base
Blocks: 318016
No progress for more than a year -> Re-assigning to nobody.
Assignee: garyvdm → nobody
Version: Trunk → unspecified
We definitely need progress UI for 0.9.
Flags: wanted-calendar0.9+
Status: NEW → ASSIGNED
Assignee: nobody → Berend.Cornelius
Status: ASSIGNED → NEW
Attached patch patch v. 1.0 (obsolete) — Splinter Review
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.
Attachment #320422 - Flags: ui-review?(christian.jansen)
Attachment #320422 - Flags: review?(philipp)
Status: NEW → ASSIGNED
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
Attachment #320422 - Flags: ui-review?(christian.jansen) → ui-review+
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
Attachment #320422 - Flags: review?(philipp) → review-
Attached patch patch v. #2 (obsolete) — Splinter Review
overworked the patch...
Attachment #320422 - Attachment is obsolete: true
Attachment #322080 - Flags: review?(philipp)
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
Attachment #322080 - Flags: review?(philipp) → review+
Attached patch patch v. #3Splinter Review
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
Attachment #218647 - Attachment is obsolete: true
Attachment #322080 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Why did you use ASCII instead of UTF-8 in gettingCalendarInfoCommon?
Checked in lightning 2008090403 and sunbird 20080903 -> VERIFIED.

I talked with Berend, encoding issue is solved.
Status: RESOLVED → VERIFIED
Depends on: 516681
You need to log in before you can comment on or make changes to this bug.