Closed
Bug 383272
Opened 17 years ago
Closed 17 years ago
[Proto] Recurrence dialog: weekly and monthly recurrencepattern is not in sync with minimonths
Categories
(Calendar :: Internal Components, defect)
Calendar
Internal Components
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: berend.cornelius09, Assigned: berend.cornelius09)
References
Details
Attachments
(1 file, 4 obsolete files)
13.15 KB,
patch
|
Details | Diff | Splinter Review |
If you set a monthly or weekly recurrence pattern in the edit event dialog and customize it afterwards by means of the "Repeat" listbox the recurrence dialog will pop up with the weekly (or monthly) recurrence datepickers being presented, so that the user can customize his recurrence pattern. However the datepickers should be preselected according to the preselection of the minimonths at the bottom of the dialog. But this is not the case.
Flags: blocking-calendar0.5-
Assignee | ||
Comment 1•17 years ago
|
||
This is not a patch that can be applied. I just added some code snippets that might solve the problem
Updated•17 years ago
|
Summary: Recurrence dialog: weekly and monthly recurrencepattern is not in sync with minimonths → [Proto] Recurrence dialog: weekly and monthly recurrencepattern is not in sync with minimonths
Updated•17 years ago
|
Flags: blocking-calendar0.7?
Updated•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Assignee | ||
Comment 2•17 years ago
|
||
removing this issue from the 0.7 blocker list
Flags: blocking-calendar0.7+ → blocking-calendar0.7-
Comment 3•17 years ago
|
||
confirming: minor UI issue, not blocking
Assignee | ||
Comment 4•17 years ago
|
||
Basically I changed the following: For the recurrence-rule-types "WEEKLY" "MONTHLY" and "YEARLY" I initialized the respective daypicker controls of the recurrence dialog. This logic is run through "onload".
Attachment #267250 -
Attachment is obsolete: true
Attachment #282553 -
Flags: review?(michael.buettner)
Comment 5•17 years ago
|
||
Comment on attachment 282553 [details] [diff] [review] patch that should solve the problem In general, it is the correct strategy to initialize all controls independent of what components have been attached to the recurrence rule. It was plain wrong to initialize only those controls that have a direct relationship to what has been found during the one-time inspection of the rules/components. I think this makes the switch-statement superfluous and you can absorb the minor bits and pieces that are left in the new block that you introduced. I would suggest to rigorously initialize all controls of the dialog, which is still not the case with the new code that you're about to introduce. >+ var byDayRule = rule.getComponent("BYDAY", {}); >+ var byMonthDayRule = rule.getComponent("BYMONTHDAY", {}); >+ var byMonthRule = rule.getComponent("BYMONTH", {}); While you're at it, please rename these variable to by?Component, 'rule' is just plain wrong, I didn't do that right in the first place. >+ // "DAILY" ruletype >+ // byDayRules may have been set priorily by "MONTHLY"- ruletypes >+ // where they have a different context- >+ // that's why we also query the current rule-type >+ if ((byDayRule.length == 0) || (rule.type != "DAILY")){ >+ document.getElementById("daily-group").selectedIndex = 0; >+ } else { >+ document.getElementById("daily-group").selectedIndex = 1; >+ } 'daily-group' should be initialized always, not only for daily types. >+ // "WEEKLY" ruletype >+ if ((byDayRule.length == 0) || (rule.type != "WEEKLY")) { >+ var days = new Array(); >+ days.push(startDate.weekday + 1) >+ document.getElementById("daypicker-weekday").days = days; This can be written as document.getElementById("daypicker-weekday").days = [ startDate.weekday + 1 ]; This is less verbose, shorter, easier to read and produces less code to interpret, which makes it faster. >+ } >+ else { >+ document.getElementById("daypicker-weekday").days = byDayRule; >+ } Same here, always initialize 'daypicker-weekday'. >+ // "MONTHLY" ruletype >+ if (((byDayRule.length == 0) && (byMonthDayRule.length == 0)) || >+ (rule.type != "MONTHLY")) { I would prefer to group the separate components if this statement at the same column. >+ var days = new Array(); >+ days.push(startDate.day); >+ document.getElementById("monthly-group").selectedIndex = 1; >+ document.getElementById("monthly-days").days = days; You can write document.getElementById("monthly-days").days = [ startDate.day ]; >+ } >+ else { >+ if (byDayRule.length > 0) { >+ document.getElementById("monthly-group").selectedIndex = 0; >+ var byday = byDayRule[0]; >+ var occurrence = (byday - (byday % 8)) / 8; >+ var weekday = byday % 8; >+ setElementValue("monthly-ordinal", occurrence); >+ setElementValue("monthly-weekday", Math.abs(weekday)); >+ } >+ else { >+ if (byMonthDayRule.length > 0) { >+ if (byMonthDayRule.length == 1 && byDayRule[0] == -1) { >+ document.getElementById("monthly-group").selectedIndex = 0; >+ setElementValue("monthly-ordinal", byMonthDayRule[0]); >+ setElementValue("monthly-weekday", byMonthDayRule[0]); >+ } else { >+ document.getElementById("monthly-group").selectedIndex = 1; >+ document.getElementById("monthly-days").days = byMonthDayRule; >+ } >+ } >+ } >+ } Again, not all controls are getting initialized unconditionally. >+ // "YEARLY" ruletype >+ if ((byMonthRule.length == 0) || (rule.type != "YEARLY")) { >+ setElementValue("yearly-days", startDate.day); >+ setElementValue("yearly-month-ordinal", startDate.month + 1); >+ } >+ else { >+ if (byMonthDayRule.length > 0) { >+ document.getElementById("yearly-group").selectedIndex = 0; >+ setElementValue("yearly-month-ordinal", byMonthRule[0]); >+ setElementValue("yearly-days", byMonthDayRule[0]); >+ } else if (byDayRule.length > 0) { >+ document.getElementById("yearly-group").selectedIndex = 1; >+ var byday = byDayRule[0]; >+ var occurrence = (byday - (byday % 8)) / 8; >+ var weekday = byday % 8; >+ setElementValue("yearly-ordinal", occurrence); >+ setElementValue("yearly-weekday", weekday); >+ setElementValue("yearly-month-rule", byMonthRule[0]); >+ } >+ } The same applies here.
Updated•17 years ago
|
Attachment #282553 -
Flags: review?(michael.buettner) → review-
Assignee | ||
Comment 6•17 years ago
|
||
I changed the variablenames and the assignment of the arrays. Regarding the initialization of some of the controls I think this was actually not part of the bug that was supposed to only deal with the synchronisation of the minimonths and the daypickers. The controls in question were disabled onload. Enabling this controls immediately resynchronized also the minimonths. But anyway in my latest patch I gave these controls also reasonable defaultvalues, based on the event startdate.
Attachment #282553 -
Attachment is obsolete: true
Attachment #282700 -
Flags: review?(michael.buettner)
Assignee | ||
Comment 7•17 years ago
|
||
Removed to unncessary hunks from patch...
Attachment #282700 -
Attachment is obsolete: true
Attachment #282707 -
Flags: review?(michael.buettner)
Attachment #282700 -
Flags: review?(michael.buettner)
Comment 8•17 years ago
|
||
Comment on attachment 282707 [details] [diff] [review] cleaned up patch >+ var day = Math.floor((startDate.day - 1) / 7) + 1; ... >+ var byday = byDayRuleComponent[0]; >+ var occurrence = (byday - (byday % 8)) / 8; >+ var weekday = byday % 8; These lines could be consolidated, each of them can be found at least twice in the patch. >+ var ruleComponentsEmpty = ((byDayRuleComponent.length == 0) && >+ (byMonthDayRuleComponent.length == 0)); >+ if (ruleComponentsEmpty || (rule.type != "MONTHLY")) { 'ruleComponentsEmpty' is nowhere else referenced, I would suggest to combine these expressions into the corresponding if-statement. The initialization code is not getting executed in case the item doesn't have a recurrence info. In that case not even the 'period-list' element gets initialized. Even worse, the daypicker controls are not getting initialized in that case. Their initialization depends on whether or not the item initially has a recurrence info. You could circumvent this problem easily with creating a recurrence info on-the-fly if there's none present in the arguments. Apart from this minor issue the patch works as advertised. r=mickey with the above issues being addressed.
Attachment #282707 -
Flags: review?(michael.buettner) → review+
Updated•17 years ago
|
Flags: blocking-calendar0.7-
Flags: blocking-calendar0.5-
Target Milestone: 0.7 → ---
Version: Lightning 0.5 → unspecified
Updated•17 years ago
|
Whiteboard: [checkin-needed after 0.7]
Assignee | ||
Comment 9•17 years ago
|
||
patch checked in on trunk and MOZILLA_1_8_BRANCH
Attachment #282707 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Whiteboard: [checkin-needed after 0.7]
Target Milestone: --- → 0.8
Comment 10•17 years ago
|
||
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9pre) Gecko/20071028 Calendar/0.8pre and Lt 2007102805
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•