383272 - [Proto] Recurrence dialog: weekly and monthly recurrencepattern is not in sync with minimonths

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Berend Cornelius]

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.

Comment 1

[Berend Cornelius]

Attachment: preliminary patch

This is not a patch that can be applied. I just added some code snippets that might solve the problem

Comment 2

[Berend Cornelius]

removing this issue from the 0.7 blocker list

Comment 3

[Daniel Boelzle [:dbo]]

confirming: minor UI issue, not blocking

Comment 4

[Berend Cornelius]

Attachment: patch that should solve the problem

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".

Comment 5

[Michael Büttner (no reviews TFN)]

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.

Comment 6

[Berend Cornelius]

Attachment: new patch to be reviewed

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.

Comment 7

[Berend Cornelius]

Attachment: cleaned up patch

Removed to unncessary hunks from patch...

Comment 8

[Michael Büttner (no reviews TFN)]

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.

Comment 9

[Berend Cornelius]

Attachment: final patch

patch checked in on trunk and MOZILLA_1_8_BRANCH

Comment 10

[Omar Bajraszewski]

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