Closed
Bug 397866
Opened 17 years ago
Closed 17 years ago
Errors are getting announced indefinitely
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: michael.buettner, Assigned: michael.buettner)
Details
Attachments
(1 file, 1 obsolete file)
4.89 KB,
patch
|
dbo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•17 years ago
|
||
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)
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 2•17 years ago
|
||
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 3•17 years ago
|
||
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-
Assignee | ||
Comment 4•17 years ago
|
||
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)
Comment 5•17 years ago
|
||
(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 6•17 years ago
|
||
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+
Assignee | ||
Comment 7•17 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•