Closed
Bug 401362
Opened 17 years ago
Closed 17 years ago
[Proto] Recurrence dialog: uncaught exception if creating new event
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: ssitter, Assigned: berend.cornelius09)
Details
Attachments
(1 file, 2 obsolete files)
18.73 KB,
patch
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.9pre) Gecko/20071027 Calendar/0.8pre Creating new repeating event with custom recurrence rule causes the following error: Error: [Exception... "'Component not initialized' when calling method: [calIRecurrenceInfo::getRecurrenceItems]" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: chrome://calendar/content/calendar-dialog-utils.js :: splitRecurrenceRules :: line 584" data: no] Source File: chrome://calendar/content/calendar-dialog-utils.js Line: 584 Regression Range: WORKS in Calendar 0.8pre (2007102605) FAILS in Calendar 0.8pre (2007102705)
Comment 1•17 years ago
|
||
I'm not sure if it's related, but after I close the recurrence dialog, the cursor changes to an hourglass and doesn't return to the normal cursor until I close the main event dialog (Lightning 0.8pre 2007-10-28-05 on WinXP).
Reporter | ||
Comment 2•17 years ago
|
||
(In reply to comment #1) This is also caused by the error from above.
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Flags: wanted-calendar0.8+
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → Berend.Cornelius
Status: ASSIGNED → NEW
Assignee | ||
Comment 3•17 years ago
|
||
As discussed I introduced an initial daily rule when the recInfo is empty or null.
Attachment #286659 -
Flags: review?(michael.buettner)
Updated•17 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Comment 4•17 years ago
|
||
Comment on attachment 286659 [details] [diff] [review] patch v. 1 The exception gets thrown simply because the recurrence info hasn't been correctly initialized. The appropriate fixs looks like this: > if (!recinfo) { > recinfo = createRecurrenceInfo(); >+ recinfo.item = item; > } This doesn't initialize the controls but leaves them with their default values. One of the last statements in onLoad() calls updatePreview() which in turn calls onSave(item) which causes a new (valid) recurrence info to be constructed. So, basically everything just works as expected with this fix. I'd just take the one-line patch from above, but if you feel so inclined you can always initialize the controls based on a default rule. In that case I would like to extract the control initialization from onLoad() into a separate function, e.g. initializeControls(rule). The contents of the function would be: > switch (rule.type) { > case "DAILY": > document.getElementById("period-list").selectedIndex = 0; > ... > // Convert the datetime from UTC to localtime. > endDate = endDate.getInTimezone(calendarDefaultTimezone()); > setElementValue("recurrence-duration", "until"); > setElementValue("repeat-until-date", endDate.jsDate); > } > } The business of this function would be to initialize all controls of the dialog based on the recurrence rule passed as argument. onLoad() could then look like this: > // make sure we're talking about the series... > if (item.parentItem != item) { > item = item.parentItem; > } > > // make sure we have a valid recurrence info > if (!recinfo) { > recinfo = createRecurrenceInfo(); > recinfo.item = item; > } > > // Split out rules and exceptions > var rrules = splitRecurrenceRules(recinfo); > var rules = rrules[0]; > var exceptions = rrules[1]; > > // Deal with the rules, construct a default rule if necessary > var rule = rules.length > 0 ? rules[0] : null; > rule = rule instanceof Components.interfaces.calIRecurrenceRule ? rule : null; > if (!rule) { > var rule = createRecurrenceRule(); > rule.type = 'DAILY'; > rule.interval = 1; > rule.count = -1; > } > > // initialize all controls > initializeControls(rule); I'm fine with either of the above mentioned solutions, just draw from one of them...
Attachment #286659 -
Flags: review?(michael.buettner) → review-
Assignee | ||
Comment 5•17 years ago
|
||
Basically it is not enough to set the item at the recurrenceInfo, it is still necessary to set a default rule in order to initialize the controls. I did not take over all of mickey's proposals: In case that no recurrenceInfo is available there is still no reason to create one - only a recurrence rule is needed to initialize all controls. As the root of the bug is a clean recurrenceInfo without any item I wrapped some exception handling around the 'splitRecurrenceRules(recinfo)', that is bound to fail in that case. I extended the utility function "createRecurrenceInfo()" in calUtils so that it now demands a parameter "item", so this should not occur - but you never know.
Attachment #286659 -
Attachment is obsolete: true
Attachment #286847 -
Flags: review?(philipp)
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 6•17 years ago
|
||
Since mickey did the first review and has taken a deeper look, doesn't he also want to review the second patch?
Comment 7•17 years ago
|
||
Comment on attachment 286847 [details] [diff] [review] Patch v. 2 >+ function createDailyRule() { >+ var recRule = createRecurrenceRule(); >+ recRule.type = 'DAILY'; >+ recRule.interval = 1; >+ recRule.count = -1; >+ return recRule >+ } Since this function is only used once, go ahead and put it directly into the if() block that follows. >+ // Split out rules and exceptions >+ try { > } >+ catch (ex) { >+ } I'm still not quite sure why we need to catch and ignore the exception here, but if you are doing so, please at least add a comment in the catch block explaining why its safe to ignore the error. The problems I see are that silent catching of this error may cause some headaches in case more code is put into this block, which throws an error that does something bad but doesn't get noticed. Also please align the catch() part like } catch (ex) {} >+ // We only handle 1 rule currently >+ if (rule[0] instanceof Components.interfaces.calIRecurrenceRule) { >+ rule = rules[0]; We handle only one rule, but in which cases is the first rule not a calIRecurrenceRule? Should we skip to the first available calIRecurrenceRule if there is more than one? r+ with these comments addressed
Attachment #286847 -
Flags: review?(philipp) → review+
Comment 8•17 years ago
|
||
I think that silently swallowing exceptions is not correct, not even with a comment or anything else.
Assignee | ||
Comment 9•17 years ago
|
||
>I'm still not quite sure why we need to catch and ignore the exception here, >but if you are doing so, please at least add a comment in the catch block >explaining why its safe to ignore the error. The problems I see are that silent >catching of this error may cause some headaches in case more code is put into >this block, which throws an error that does something bad but doesn't get >noticed. My idea to set exceptionhandling was that I want to make sure that my controls all get initialized even if an error occurs. Yet it is correct that such an error should also be well propagated and therefor I modified the implementation that now the error is sent to the error console. >We handle only one rule, but in which cases is the first rule not a >calIRecurrenceRule? Should we skip to the first available calIRecurrenceRule if >there is more than one? I submitted bug 402526 to deal with this issue.
Assignee | ||
Comment 10•17 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH
Attachment #286847 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → 0.8
Comment 11•17 years ago
|
||
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.10pre) Gecko/20071105 Calendar/0.8pre and Lightning 2007110510
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•