392561 - Need to revise provider error notifications

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Daniel Boelzle [:dbo]]

Client code calling on providers should care about how to handle a specific error notified via calIOperationListener. Client code knows far better whether a specific error should be notified (e.g. in an error box) or could be ignored. E.g. I would consider every error caused by a direct user interaction to be notified in an error box while I usually won't accept error boxes during frequent agenda/alarm updates. I'd expect that a silent retry is scheduled in that case. In essence only the client code could decide on that, the provider doesn't know the overall context an operation is running in, and calIObserver::onError should generally log silently, omitting an error box.

Comment 1

[Berend Cornelius]

I think a good first step to head in Daniel's proposed direction is to handle Network-errors in a "softer" way. A possible solution would be to disable the respective calendars in the calendar-pane and probably provide a tooltip. 
We would then also have to decide how we denote and implement resurrecting calendars (e.g. after a reload).
Christian do you already have an opinion on this?

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: [checked in] adding calendar parameter to calICalendar::onError

When receiving onError events via calICompositeObserver, you usually have no clue what calendar has fired the error. This patch adds a calendar parameter.

Comment 3

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 320171 [details] [diff] [review]
[checked in] adding calendar parameter to calICalendar::onError

Looks good r=philipp

Comment 4

[Berend Cornelius]

Attachment: patch v. #2

This is a second Step for the error notifications. Errors are now transferred to the error console and a tooltip above the according entry in the calendar list displays whether a calendar is readOnly or disabled. The user is no longer bothered with frequent message prompts. Christian only wanted a genera error message text.For more detailed information the user may see the error console. I invested some time in the ui changes of the calendar list to better depict the error conditions but I will file a follow-up bug for it.

Comment 5

[Daniel Boelzle [:dbo]]

Berend, the patch doesn't work on my Mac build. It only shows a warning in the console:

Warning: There has been an error reading data for calendar: Frank Loehmann.  However, this error is believed to be minor, so the program will attempt to continue. Error number:0x804a021c. Description: The user is denied access.

What irritates me a bit is that it always states "... an error reading data...", even if I try to modify the calendar.

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

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

>+                    var cbCol = this.treebox.columns
>                             .getNamedColumn("calendar-list-tree-checkbox");
Indent second line correctly

>+    onMouseMove: function cLTV_onMouseMove(event) {
>+        var row = this.treebox.getRowAt(event.clientX, event.clientY);
>+        if (row > -1) {

>+            if (!this.mCalendarProps) {
>+                var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                    .getService(Components.interfaces.nsIStringBundleService);
>+                this.mCalendarProps = sbs.createBundle("chrome://calendar/locale/calendar.properties");
>+            }
Why not use calGetString() ?


>+    <tooltip id="calendar-tooltip">
>+        <label value=""/>
>+    </tooltip>
...
>+            if (tooltipText != null) {
>+                var tooltip = document.getElementById("calendar-tooltip");
>+                tooltip.firstChild.value = tooltipText;
>+                treeChildren.setAttribute("tooltip", "calendar-tooltip");
>+            } else {
>+                treeChildren.removeAttribute("tooltip");
>+            }
>+        }
>+    },


Instead, you could use the tooltiptext attribute and drop the <tooltip> element. See http://developer.mozilla.org/en/docs/XUL:tooltip



>-        wWatcher.openWindow(null,
>-                            "chrome://calendar/content/calErrorPrompt.xul",
>-                            "_blank",
>-                            "chrome,dialog=yes,alwaysRaised=yes",
>-                            paramBlock);
Please file a follow-up bug to decide on the future error strategy, if we want to use the calErrorPrompt and such, or if we want to remove it.

>+errornumber=Error number:
I believe the common phrase is "Error code". Also please camelCase, i.e errorNumber and errorDescription.


r=philipp

Comment 7

[Berend Cornelius]

Attachment: patch v. #3

unbitrotted last patch...

Comment 8

[Berend Cornelius]

Attachment: patch v. #4

This patch represents my current state of the art. I ran into the following problem: When I committed a transaction on a corrupted ics calendar I could not query the error status on operationComplete" and thus implement an appropriate error handling. Daniel wanted to take care about this.

Comment 9

[Daniel Boelzle [:dbo]]

Attachment: more calIErrors, removing WCAP-specific SUPPRESS_ONERROR

- fires additional errors to signal that reading or modification has failed.
- gdata missing

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: [checked in] more calIErrors, adding currentStatus property, consolidating error handling into base

gdata, caldav untested

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

Would be good to get caldav tested. I don't currently have a caldav server running to get this tested.

Comment 12

[Daniel Boelzle [:dbo]]

Comment on attachment 326893 [details] [diff] [review]
[checked in] more calIErrors, adding currentStatus property, consolidating error handling into base

- checked in on HEAD and MOZILLA_1_8_BRANCH
- added following to calCalendarManager.js for now, Berend will remove that block:

    announceError: function(aErrNo, aMessage) {
        // XXX swallow MODIFICATION_FAILED and READ_FAILED for now unless Berend has finished his work...
        switch (aErrNo) {
            case Components.interfaces.calIErrors.READ_FAILED:
            case Components.interfaces.calIErrors.MODIFICATION_FAILED:
                return;
        }

- fixed one typo

Comment 13

[Berend Cornelius]

Attachment: patch v. #6

This patch handles the error as discussed: Writing errors throw a messagebox once a the status of the calendars is displayed in a tooltip of the calendar list. In a later step we can display an icon.
All errors are sent as warnings to the error console.

Comment 14

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 327099 [details] [diff] [review]
patch v. #6

>+    <tooltip id="calendar-tooltip">
>+        <label value=""/>
>+    </tooltip>
This is probably not needed, use the tooltiptext attribute instead. http://developer.mozilla.org/en/docs/XUL:Attribute:tooltiptext


>+        onmousemove="calendarListTreeView.onMouseMove(event);"
>     onPropertyChanged: function cMO_onPropertyChanged(aCalendar,
>                                                       aName,
>                                                       aValue,
>-                                                      aOldValue) {},
>+                                                      aOldValue) {
>+        if (aName == "currentStatus") {
>+            if (Components.isSuccessCode(aValue)) {
>+                // insert 'Successful' - image implementation here
>+            } else {
>+                // insert 'unsuccessful' - image implementation here                
>+            }
>+            WARN("PROEPRTY " + aName + " of calendar " + aCalendar.name + 
>+                 "has been changed to " + aValue);
>+        }                          
>+    },
>+    
Please add TODO to the comments here and remove the WARN() since its quite common that calendar props will change and we dont want a warning for each.

>+errorCode=Error code:
>+errorDescription=Description:
These should possibly contain %1$S, since rtl languages might need it the other way around.

>+errorWriting=An error occurred when writing to the calendar '%1$S'!
>+tooltipCalendarDisabled=The calendar %1$S is momentarily not available
>+tooltipCalendarReadOnly=The calendar %1$S is readonly
Either all messages with '' or none. I prefer none, but others may think differently.


r=philipp with comments considered.

Comment 15

[Hubert Gajewski]

I don't see these changes in last patch:

calendar.properties:
changeMasterPassword=Change Master Password\u00E2\u0080\u00A6                   
updatesItem_default=Check for Updates\u00E2\u0080\u00A6                         
updatesItem_defaultFallback=Check for Updates\u00E2\u0080\u00A6                 
updatesItem_downloading=Downloading %S\u00E2\u0080\u00A6                        
updatesItem_downloadingFallback=Downloading Update\u00E2\u0080\u00A6            
updatesItem_resume=Resume Downloading %S\u00E2\u0080\u00A6                      
updatesItem_resumeFallback=Resume Downloading Update\u00E2\u0080\u00A6          
updatesItem_pending=Apply Downloaded Update Now\u00E2\u0080\u00A6               
updatesItem_pendingFallback=Apply Downloaded Update Now\u00E2\u0080\u00A6       
updatesItem_default=Check for Updates\u00E2\u0080\u00A6                         
updatesItem_defaultFallback=Check for Updates\u00E2\u0080\u00A6                 
updatesItem_downloading=Downloading %S\u00E2\u0080\u00A6                        
updatesItem_downloadingFallback=Downloading Update\u00E2\u0080\u00A6            
updatesItem_resume=Resume Downloading %S\u00E2\u0080\u00A6                      
updatesItem_resumeFallback=Resume Downloading Update\u00E2\u0080\u00A6          
updatesItem_pending=Apply Downloaded Update Now\u00E2\u0080\u00A6               
updatesItem_pendingFallback=Apply Downloaded Update Now\u00E2\u0080\u00A6       

A mistake?

Comment 16

[Hubert Gajewski]

Those changes were added (probably by mistake) during last commit:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/calendar/locales/en-US/chrome/calendar&command=DIFF_FRAMESET&file=calendar.properties&rev1=1.109&rev2=1.110&root=/cvsroot

Comment 17

[Berend Cornelius]

>This is probably not needed, use the tooltiptext attribute instead.

Yes it works with the tooltiptext attribute. The tooltip element was dispensible indeed. I did not intentionally oversaw this comment after your first review.

I checked in the patch on trunk and MOZILLA_1_8_BRANCH after I addressed philipps comments and I also restored UTF-8 in calendar.properties
->

Comment 18

[Martin Schröder [:mschroeder]]

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

Removing ui-review request because bug is closed.

Comment 19

[Daniel Boelzle [:dbo]]

Christian, AFAIR different icons are still missing in the calendar list, so users get to know which calendar has problems in case of an error, and removing the interim error box (MODIFICATION_FAILED).

Comment 20

[Simon Paquet [:sipaq]]

What is still missing in the bug and who is in charge for fixing it? The assignee is Philipp, but comment 19 seems to imply that Christian is the one, who should fix this bug.

I also don't understand why this is a blocker. Daniel, can you elaborate?

Comment 21

[Daniel Boelzle [:dbo]]

There's still nothing that indicates that an error has happened except for the error console (which I assume hardly anyone is aware of). We need a different icon/indicator in the calendar list. AFAIK Christian and Philipp are already working on this.

Comment 22

[Berend Cornelius]

Attachment: notification icons

Christian gave me an image with the two icons to be displayed when
a) ...the calendar is readonly. He was very unhappy with the current black/white padlock that we display in the calendar-color cell, so he wanted to have this replaced. The current image "calendar-readOnly.png" will consequently be removed from the cvs repository.

b) ...the calendar is not available because reading from it failed. In this case we will use an icon conforming to the standard warning symbol.

When discussing the problem over we noticed the following implications.
The icons should not be displayed over a colored background because they might under certain colors not being distinguishable. That's why we put the icons in a fourth tree column on the right hand side of the calendar list. So far the column we use is a "cycler column" where the user can toggle the readonly state by clicking on it. We abandoned this functionality as it is no longer perceivable in our new column that is in most of the times empty.

Comment 23

[Berend Cornelius]

Attachment: patch v. #7

This patch hopefully resolves the demands of this issue. I will ask Andreas for testing it on Monday before giving it to review. Of course any prior comments are highly appreciated.

Comment 24

[Berend Cornelius]

Attachment: screenshot with the latest patch...

Comment 25

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 336059 [details] [diff] [review]
patch v. #7

Codewise, I´d give this patch r+.

Comment 26

[Martin Schröder [:mschroeder]]

Berend, are you going to check this in, or is something still missing?

Comment 27

[Berend Cornelius]

Comment on attachment 336059 [details] [diff] [review]
patch v. #7

Ooops, I forgot to query for ui-review

Comment 28

[Christian Jansen]

Comment on attachment 336059 [details] [diff] [review]
patch v. #7

ui=Christian

Comment 29

[Berend Cornelius]

patch checked in on trunk and MOZILLA_1_8_BRANCH
-> fixed

Comment 30

[Andreas Treumann]

A visible scrollbar in the calendar list covers the read only icons.

Comment 31

[Daniel Boelzle [:dbo]]

remaining issue is not blocking 0.9 and deferred into bug 453409

Comment 32

[Andreas Treumann]

Checked in lightning build 2008090303 and sunbird 20080902 -> VERIFIED