Closed
Bug 392561
Opened 17 years ago
Closed 16 years ago
Need to revise provider error notifications
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
VERIFIED
FIXED
0.9
People
(Reporter: dbo, Assigned: berend.cornelius09)
Details
Attachments
(9 files, 1 obsolete file)
31.86 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
16.52 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
16.59 KB,
patch
|
Details | Diff | Splinter Review | |
20.62 KB,
patch
|
Details | Diff | Splinter Review | |
100.54 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
19.03 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
697 bytes,
image/png
|
Details | |
13.63 KB,
patch
|
Fallen
:
review+
chris.j.bugzilla
:
ui-review+
|
Details | Diff | Splinter Review |
109.92 KB,
image/png
|
Details |
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.
Reporter | ||
Updated•16 years ago
|
Flags: wanted-calendar0.9+
Assignee | ||
Comment 1•16 years ago
|
||
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?
Reporter | ||
Comment 2•16 years ago
|
||
When receiving onError events via calICompositeObserver, you usually have no clue what calendar has fired the error. This patch adds a calendar parameter.
Attachment #320171 -
Flags: review?(philipp)
Updated•16 years ago
|
Assignee: nobody → daniel.boelzle
Updated•16 years ago
|
Status: NEW → ASSIGNED
Comment 3•16 years ago
|
||
Comment on attachment 320171 [details] [diff] [review] [checked in] adding calendar parameter to calICalendar::onError Looks good r=philipp
Attachment #320171 -
Flags: review?(philipp) → review+
Reporter | ||
Updated•16 years ago
|
Attachment #320171 -
Attachment description: adding calendar parameter to calICalendar::onError → [checked in] adding calendar parameter to calICalendar::onError
Reporter | ||
Updated•16 years ago
|
Assignee: daniel.boelzle → Berend.Cornelius
Status: ASSIGNED → NEW
Assignee | ||
Comment 4•16 years ago
|
||
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.
Attachment #322910 -
Flags: ui-review?(daniel.boelzle)
Attachment #322910 -
Flags: review?(philipp)
Reporter | ||
Comment 5•16 years ago
|
||
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•16 years ago
|
||
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
Attachment #322910 -
Flags: review?(philipp) → review+
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•16 years ago
|
||
unbitrotted last patch...
Assignee | ||
Comment 8•16 years ago
|
||
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.
Reporter | ||
Comment 9•16 years ago
|
||
- fires additional errors to signal that reading or modification has failed. - gdata missing
Attachment #325982 -
Flags: review?(philipp)
Updated•16 years ago
|
Attachment #325982 -
Flags: review?(philipp) → review+
Reporter | ||
Comment 10•16 years ago
|
||
gdata, caldav untested
Attachment #325982 -
Attachment is obsolete: true
Attachment #326893 -
Flags: review?(philipp)
Updated•16 years ago
|
Attachment #326893 -
Flags: review?(philipp) → review+
Comment 11•16 years ago
|
||
Would be good to get caldav tested. I don't currently have a caldav server running to get this tested.
Reporter | ||
Comment 12•16 years ago
|
||
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
Attachment #326893 -
Attachment description: more calIErrors, adding currentStatus property, consolidating error handling into base → [checked in] more calIErrors, adding currentStatus property, consolidating error handling into base
Assignee | ||
Comment 13•16 years ago
|
||
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.
Attachment #327099 -
Flags: review?(daniel.boelzle)
Reporter | ||
Updated•16 years ago
|
Attachment #327099 -
Flags: review?(daniel.boelzle) → review?(philipp)
Comment 14•16 years ago
|
||
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.
Attachment #327099 -
Flags: review?(philipp) → review+
Comment 15•16 years ago
|
||
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•16 years ago
|
||
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
Updated•16 years ago
|
Target Milestone: --- → 0.9
Assignee | ||
Comment 17•16 years ago
|
||
>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
->
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 18•16 years ago
|
||
Comment on attachment 322910 [details] [diff] [review] patch v. #2 Removing ui-review request because bug is closed.
Attachment #322910 -
Flags: ui-review?(daniel.boelzle)
Reporter | ||
Comment 19•16 years ago
|
||
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).
Status: RESOLVED → REOPENED
Flags: wanted-calendar0.9+ → blocking-calendar0.9+
Resolution: FIXED → ---
Updated•16 years ago
|
Assignee: Berend.Cornelius → philipp
Status: REOPENED → NEW
Comment 20•16 years ago
|
||
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?
Reporter | ||
Comment 21•16 years ago
|
||
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.
Updated•16 years ago
|
Whiteboard: [needs patch]
Assignee | ||
Updated•16 years ago
|
Assignee: philipp → Berend.Cornelius
Assignee | ||
Comment 22•16 years ago
|
||
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.
Assignee | ||
Comment 23•16 years ago
|
||
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.
Assignee | ||
Comment 24•16 years ago
|
||
Comment 25•16 years ago
|
||
Comment on attachment 336059 [details] [diff] [review] patch v. #7 Codewise, I´d give this patch r+.
Attachment #336059 -
Flags: review+
Comment 26•16 years ago
|
||
Berend, are you going to check this in, or is something still missing?
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Attachment #336059 -
Flags: ui-review?(christian.jansen)
Assignee | ||
Comment 27•16 years ago
|
||
Comment on attachment 336059 [details] [diff] [review] patch v. #7 Ooops, I forgot to query for ui-review
Comment 28•16 years ago
|
||
Comment on attachment 336059 [details] [diff] [review] patch v. #7 ui=Christian
Attachment #336059 -
Flags: ui-review?(christian.jansen) → ui-review+
Assignee | ||
Comment 29•16 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH -> fixed
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Whiteboard: [needs patch]
Comment 30•16 years ago
|
||
A visible scrollbar in the calendar list covers the read only icons.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 31•16 years ago
|
||
remaining issue is not blocking 0.9 and deferred into bug 453409
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 32•16 years ago
|
||
Checked in lightning build 2008090303 and sunbird 20080902 -> VERIFIED
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•