calRecurrenceRule tries to set a non-numeric BYMONTHDAY rule

RESOLVED FIXED in 4.0.0.1

Status

Calendar
ICAL.js Integration
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
4.0.0.1
Bug Flags:
in-testsuite +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
calRecurrenceRule.setComponent converts numbers to MO,TU, etc., event if it shouldn't. Probably related to bug 890547 in some way, but I'm adding a new bug anyway as I don't have time to investigate further right now.
(Assignee)

Comment 1

3 years ago
Created attachment 8543141 [details] [diff] [review]
1116882-1.diff
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8543141 - Flags: review?(philipp)
Comment on attachment 8543141 [details] [diff] [review]
1116882-1.diff

Review of attachment 8543141 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/backend/icaljs/calRecurrenceRule.js
@@ +137,5 @@
>      get weekStart() this.innerObject.wkst - 1,
>      set weekStart(val) this.innerObject.wkst = val + 1,
>  
>      getComponent: function(aType, aCount) {
> +        let values = this.innerObject.getComponent(aType, aCount);

Could you give me a sample of what this outputs when aType == BYMONTHDAY and when not? It seems to me that the getComponent code doesn't behave like it does for libical, where we don't do anything except for cloning the array.

Maybe its possible to fix this in ical.js instead?
(Assignee)

Comment 3

3 years ago
It looks like getComponent and setComponent should only be using numbers (calIRecurrenceRule.idl says so). So I'll change them to pass the numeric values straight through.

To get string values I'll change ICAL.Recur.toString to special-case BYDAY parts. Can't think of anywhere else that needs changing.
(Assignee)

Updated

3 years ago
Attachment #8543141 - Flags: review?(philipp)
(Assignee)

Comment 4

3 years ago
Created attachment 8543553 [details] [diff] [review]
1116882-test-1.diff

Had a bit of a rethink about this bug. I should fix it for all the cases set/getComponent might be used for. This patch adds a unit test for all the cases that Lightning can set a rule for (it's adapted from calendar-event-dialog-recurrence.js). Currently the JS implementation passes the only first four.

(In reply to Geoff Lankow (:darktrojan) from comment #3)
> It looks like getComponent and setComponent should only be using numbers
> (calIRecurrenceRule.idl says so). So I'll change them to pass the numeric
> values straight through.
> 
> To get string values I'll change ICAL.Recur.toString to special-case BYDAY
> parts. Can't think of anywhere else that needs changing.

Rethought this too. We should pass strings to ICAL.js when necessary, because it's a lot more obvious what's going on. This

> a.setComponent('BYDAY', ['2MO']);

is a lot easier to read than

> a.setComponent('BYDAY', [18]);

and the only reason we're using numbers in the first place is because libical stores components as short. So I'll fix calRecurrenceRule.[gs]etComponent, not ICAL.Recur.[gs]etComponent.
Attachment #8543553 - Flags: review?(philipp)
(Assignee)

Comment 5

3 years ago
Created attachment 8543554 [details] [diff] [review]
1116882-2.diff
Attachment #8543141 - Attachment is obsolete: true
Attachment #8543554 - Flags: review?(philipp)
Comment on attachment 8543554 [details] [diff] [review]
1116882-2.diff

Review of attachment 8543554 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp with these small comments:

::: calendar/base/backend/icaljs/calRecurrenceRule.js
@@ +138,5 @@
>      set weekStart(val) this.innerObject.wkst = val + 1,
>  
>      getComponent: function(aType, aCount) {
> +        let values = this.innerObject.getComponent(aType);
> +        if (aType == "BYDAY") {

Could you add a comment on why BYDAY is special cased?

@@ +149,5 @@
> +                values[i] = ICAL.Recur.icalDayToNumericDay(match[3]);
> +                if (match[2]) {
> +                    values[i] += 8 * match[2];
> +                }
> +                if (match[1] == '-') {

Also, could you add one or two quick comments as to what match[1] and match[2] is? This way we don't have to read it out of the regex.

@@ +166,5 @@
> +            if (aType == "BYDAY") {
> +                var absValue = Math.abs(aValues[i]);
> +                if (absValue > 7) {
> +                    var sign = aValues[i] < 0 ? -1 : 1;
> +                    var ordinal = Math.floor(absValue / 8) * sign;

Hmm isn't this the same as:

var ordinal = Math.trunc(aValues[i]);

If not, you can use Math.sign() to determine the sign instead.

@@ +173,5 @@
> +                } else {
> +                    values[i] = ICAL.Recur.numericDayToIcalDay(aValues[i]);
> +                }
> +            } else {
> +                values[i] = aValues[i];

So for the non-BYDAY case you are just copying the array. If it needs to be copied, then I'd suggest taking this out of the loop and using Array.concat or similar, this might be slightly faster than manually copying in a loop. If no copy needs to be made, you can also take it out of the loop and just pass the original array if its not BYDAY.
Attachment #8543554 - Flags: review?(philipp) → review+
Attachment #8543553 - Flags: review?(philipp) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8553666 [details] [diff] [review]
1116882-3.diff
Attachment #8543554 - Attachment is obsolete: true
Attachment #8553666 - Flags: review?(philipp)
Attachment #8553666 - Flags: review?(philipp) → review+
(Assignee)

Comment 8

3 years ago
Pushed both patches as https://hg.mozilla.org/comm-central/rev/f2179a0632e6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → 4.0
You need to log in before you can comment on or make changes to this bug.