Last Comment Bug 1116882 - calRecurrenceRule tries to set a non-numeric BYMONTHDAY rule
: calRecurrenceRule tries to set a non-numeric BYMONTHDAY rule
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: ICAL.js Integration (show other bugs)
: unspecified
: All All
-- normal (vote)
: 4.0.0.1
Assigned To: Geoff Lankow (:darktrojan)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-31 15:03 PST by Geoff Lankow (:darktrojan)
Modified: 2015-01-27 01:06 PST (History)
2 users (show)
geoff: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1116882-1.diff (1.44 KB, patch)
2015-01-01 01:01 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
1116882-test-1.diff (5.34 KB, patch)
2015-01-03 17:13 PST, Geoff Lankow (:darktrojan)
philipp: review+
Details | Diff | Splinter Review
1116882-2.diff (2.51 KB, patch)
2015-01-03 17:14 PST, Geoff Lankow (:darktrojan)
philipp: review+
Details | Diff | Splinter Review
1116882-3.diff (2.66 KB, patch)
2015-01-23 03:00 PST, Geoff Lankow (:darktrojan)
philipp: review+
Details | Diff | Splinter Review

Description User image Geoff Lankow (:darktrojan) 2014-12-31 15:03:04 PST
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.
Comment 1 User image Geoff Lankow (:darktrojan) 2015-01-01 01:01:27 PST
Created attachment 8543141 [details] [diff] [review]
1116882-1.diff
Comment 2 User image Philipp Kewisch [:Fallen] 2015-01-02 03:27:39 PST
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?
Comment 3 User image Geoff Lankow (:darktrojan) 2015-01-02 04:56:08 PST
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.
Comment 4 User image Geoff Lankow (:darktrojan) 2015-01-03 17:13:12 PST
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.
Comment 5 User image Geoff Lankow (:darktrojan) 2015-01-03 17:14:11 PST
Created attachment 8543554 [details] [diff] [review]
1116882-2.diff
Comment 6 User image Philipp Kewisch [:Fallen] 2015-01-18 08:05:26 PST
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.
Comment 7 User image Geoff Lankow (:darktrojan) 2015-01-23 03:00:17 PST
Created attachment 8553666 [details] [diff] [review]
1116882-3.diff
Comment 8 User image Geoff Lankow (:darktrojan) 2015-01-27 01:06:26 PST
Pushed both patches as https://hg.mozilla.org/comm-central/rev/f2179a0632e6

Note You need to log in before you can comment on or make changes to this bug.