Closed Bug 397866 Opened 15 years ago Closed 15 years ago

Errors are getting announced indefinitely

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: michael.buettner, Assigned: michael.buettner)

Details

Attachments

(1 file, 1 obsolete file)

Processing a calendar query can result in an error being announced by the calendar manager. This brings up an error box that shows the message the provider returned. Even if the same message has been announced previously new message windows are getting opened indefinitely. In case where the auto-reload feature has been enabled with at least one network calendar registered but the network connection has been dropped, this produces a continuous stream of message boxes.
Attached patch patch v1 (obsolete) — Splinter Review
This patch teaches the error announcer to remember previously raised messages in order to prevent duplicates being raised. As soon as a message has been acknowledged by closing the error window the message will come up again if the error happens again. It just silently swallows messages that already have been announced as long as the appropriate window is still open.
Attachment #282663 - Flags: review?(daniel.boelzle)
Status: NEW → ASSIGNED
Comment on attachment 282663 [details] [diff] [review]
patch v1

>+        QueryInterface: function mBL_QueryInterface(aIID) {
>+            if (!aIID.equals(Components.interfaces.nsIWindowMediatorListener) &&
>+                !aIID.equals(Components.interfaces.calIObserver) &&
>+                !aIID.equals(Components.interfaces.nsISupports)) {
>+                throw Components.results.NS_ERROR_NO_INTERFACE;
>+            }
>+            return this;
>+        },
Please use ensureIid from calUtils.js for brevity.

>+                // remove the message that has been shown from
>+                // the list of all announced messages.
>+                announcer.announcedMessages = announcer.announcedMessages.filter(
>+                    function(announcedMessage) {
>+                        if (announcedMessage.GetString(0) == args.GetString(0) &&
>+                            announcedMessage.GetString(1) == args.GetString(1) &&
>+                            announcedMessage.GetString(2) == args.GetString(2)) {
>+                            return false;
>+                        }
>+                        return true;
>+                    });
Please spend e.g. function equalMessage(msg1, msg2) and use it here.

>+            } catch(e) {
>+            }
worth a Components.utils.reportError(e)?

>     paramBlock.SetString(0, errMsg);
>     paramBlock.SetString(1, errCode);
>     paramBlock.SetString(2, message);
>+
>+    // silently don't do anything if this message already has
>+    // been announed without being acknowledged.    
>+    if (this.announcedMessages.some(
>+        function(element, index, array) {
>+            return (element.GetString(0) == errMsg &&
>+                    element.GetString(1) == errCode &&
>+                    element.GetString(2) == message);
>+        })) {
please use equalMessage(paramBlock, element) here

I'd prefer to bring the window to top, too. What about storing the windows in the array, checking their arguments?
>+        return;

other than that the patch looks good.
Attachment #282663 - Flags: review?(daniel.boelzle)
Comment on attachment 282663 [details] [diff] [review]
patch v1

Mickey, could you please rework the patch the way that the error window comes to top?
Moreover, there is some trailing whitespace and mixed LF / CRLF in the patch.
Attachment #282663 - Flags: review-
Attached patch patch v2Splinter Review
New version of the patch that addresses the previous comments.

> Please use ensureIid from calUtils.js for brevity.
Done.

Please spend e.g. function equalMessage(msg1, msg2) and use it here.
> Done. Introduced top-level function equalMessage().

> worth a Components.utils.reportError(e)?
Done.

> I'd prefer to bring the window to top, too.
Done. I'm now using the 'always raised' flag.
Attachment #282663 - Attachment is obsolete: true
Attachment #286265 - Flags: review?(daniel.boelzle)
(In reply to comment #4)
> > I'd prefer to bring the window to top, too.
> Done. I'm now using the 'always raised' flag.

How does that work? It looks like it just exits, but doesn't bring the (existing) window to top:

+    if (this.announcedMessages.some(
+        function(element, index, array) {
+            return equalMessage(paramBlock, element);
+        })) {
+        return;
+    }

IMO that's sensible since it's an error box.
Comment on attachment 286265 [details] [diff] [review]
patch v2

Ah, I understand now. Looks good and works as advertised; r=dbo.
Attachment #286265 - Flags: review?(daniel.boelzle) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
You need to log in before you can comment on or make changes to this bug.