Geoff Lankow (:darktrojan) Reporter
	Description • 10 years ago

Last update was in February.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 1 • 10 years ago

I took a short look into this, we need to do some changes to the wrapper too because I merged the jCal final pull request. Tests are failing for me, but then again so are the libical tests so I may need to do a clobber. Running this now, if its working for you and you want to take over feel free to leave a comment.

	Geoff Lankow (:darktrojan) Reporter
	Comment 2 • 9 years ago

Here's a recent version with all the changes needed to make the unit tests pass. In some cases "passing" means "not run at all".

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 3 • 9 years ago

Comment on attachment 8577000 [details] [diff] [review]
1115667-1.diff

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

f+, good work on getting this started. One thing I recall is that the ICAL.parse API isn't really solid. I believe it returns an array of components if there are multiple components and the single component if its just one. This is not really developer friendly, maybe we should split it up into ICAL.parse returning the first component and a new function ICAL.parseAll that returns an array of all top level components. Maybe you can fix that while you are at it?

::: calendar/base/backend/icaljs/calICSService-worker.js
@@ +14,5 @@
>  
>  onmessage = function onmessage(event) {
>      try {
>          let comp = ICAL.parse(event.data);
> +        comp = ["icalendar", comp];

Can we just post comp as retrieved from ICAL.parse here and handle the rest in the caller?

::: calendar/base/backend/icaljs/calICSService.js
@@ +49,5 @@
>      },
>  
>      get valueAsIcalString() {
>          let type = this.innerObject.type;
> +        return ICAL.stringify.value(this.innerObject.jCal[3], type);

What about multivalue properties? Could you explain why the previous code wasn't working? I'd rather not directly access the jCal member if possible.

::: calendar/base/backend/icaljs/calPeriod.js
@@ +48,5 @@
>  
>      get icalString() this.innerObject.toICALString(),
>      set icalString(val) {
>          let str = ICAL.parse._parseValue(val, "period");
> +        this.innerObject = ICAL.Period.fromString(str.join("/"));

so _parseValue returns an array now. Maybe you can rename the variable from str to something else.

::: calendar/base/modules/ical.js
@@ +39,5 @@
>  }
>  
>  // -- start ical.js --
>  
> +var ICAL = {};

Just to make sure, did you compare the ical.js we have to the one in the changeset mentioned at the top? We may have some local modifications and should make sure they have been upstreamed.

::: calendar/test/unit/test_recur.js
@@ +441,5 @@
>      equal(rparts.get("FREQ"), "WEEKLY");
>      equal(rparts.get("COUNT"), "6");
>      equal(rparts.get("BYDAY"), "TU,WE");
> +    ok(checkritems.get("EXDATE").value.match(/^2002-?04-?03T11:?45:?00Z$/));
> +    ok(checkritems.get("RDATE").value.match(/^2002-?04-?01T11:?45:?00Z$/));

I guess this is work in progress? Outside of ical.js it should probably keep looking like an icalString.

	Geoff Lankow (:darktrojan) Reporter
	Comment 4 • 9 years ago

Comment on attachment 8577000 [details] [diff] [review]
1115667-1.diff

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

::: calendar/base/backend/icaljs/calICSService.js
@@ +49,5 @@
>      },
>  
>      get valueAsIcalString() {
>          let type = this.innerObject.type;
> +        return ICAL.stringify.value(this.innerObject.jCal[3], type);

What I saw happening was x.toString returning

> FREQ=WEEKLY;COUNT=6;BYDAY=TU,WE

which ICAL.stringify.value turned into

> 0=F;1=R;2=E;3=Q;4==;5=W;6=E;7=E;8=K;9=L;10=Y;11=;; ... etc.

(this was in test_recur.js, test_interface). I'm not saying this is the right way to fix it, but it did pass the test without breaking any of the others. None of the tests that call this have multiple-value properties.

	Geoff Lankow (:darktrojan) Reporter
	Comment 5 • 9 years ago

Okay, making some more progress at last. These are the changes needed to pass the tests. I haven't included the new ical.js in the patch for readability's sake.

I will need to add ICAL.UtcOffset.toICALString, that's the cleanest way to get values in the format "+1200" instead of "+12:00".

In test_attachment.js, IIRC that code does throw, but with a different exception. I haven't looked at that for a while.

In test_ics_service.js I've changed from PROP to DESCRIPTION because description expects the type to be "text". Otherwise it's "unknown" and doesn't escape properly. (Ditto X-FOO.) Given we only have the difference in escaping behaviour for UI reasons I *think* this is okay.

	Geoff Lankow (:darktrojan) Reporter
	Comment 6 • 9 years ago

It appears test_attachment just works now.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 7 • 9 years ago

Comment on attachment 8590026 [details] [diff] [review]
1115667-2.diff

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

(In reply to Geoff Lankow (:darktrojan) from comment #5)
> Okay, making some more progress at last. These are the changes needed to
> pass the tests. I haven't included the new ical.js in the patch for
> readability's sake.
Good call, thanks!
 
> I will need to add ICAL.UtcOffset.toICALString, that's the cleanest way to
> get values in the format "+1200" instead of "+12:00".
Sure thing, we should probably have that in most every type class.

> In test_ics_service.js I've changed from PROP to DESCRIPTION because
> description expects the type to be "text". Otherwise it's "unknown" and
> doesn't escape properly. (Ditto X-FOO.) Given we only have the difference in
> escaping behaviour for UI reasons I *think* this is okay.
It probably should, but its of course a possible source of regressions later on. We'll have to keep an eye on this. I think we used to have a mozmill test that checks special characters in the event dialog description field, maybe you can test that manually with this patch applied. The other thing to check would be importing and exporting ics that has X-Props with characters that are usually escaped.

::: calendar/base/backend/icaljs/calICSService.js
@@ +57,5 @@
> +            }
> +            if ("toICALString" in x) {
> +                return x.toICALString();
> +            }
> +            return x.toString();

Not too happy about this special casing, but then again I don't see many other options.

We could introduce a getICALValues() function to ical.js, but on the other hand this will just complicate things. I always get confused between the value and  valueAsIcalString getters and making ical.js have that same issue isn't ideal.

Would it be sensible to move the special casing into ICAL.stringify.value? We could even defer it to ICAL.design and friends.

If that doesn't make sense, lets keep it as you have it for now.

@@ -469,1 @@
>          return new calIcalComponent(new ICAL.Component(comp));

Ah, this was the part I was thinking about when I wrote my last initial f+ comment. I believe the way I changed ICAL.parse was to either return an array of components if it doesn't contain a root but multiple component in succession, or just the component if there is only one. While this might save one array creation, I'm not sure its a good idea because it can be a source of confusion.

What do you think about ICAL.parse() always returning the first component and ICAL.parseMulti() returning an array of components. For ICAL.parse() we could either silently drop the remaining components, or raise an error if there is more than one.

	Geoff Lankow (:darktrojan) Reporter
	Comment 8 • 9 years ago

This brings things as far as commit 399afd7, which is just before some big changes I don't want to deal with. Again I've skipped the actual ical.js changes.

	Geoff Lankow (:darktrojan) Reporter
	Comment 9 • 9 years ago

This is a diff of the changes between build/ical.js and comm-central's ical.js. It includes the patch from bug 1146500 and a change needed to make the tests pass, which is now in later versions of ical.js.

	aleth [:aleth]
	Comment 10 • 9 years ago

Might be a good idea to do this before the merge?

	[:MakeMyDay]
	Comment 11 • 9 years ago

This is likely to happen during the TB45 beta cycle, not before. There are still some optimizations to do upstream before merging into cc. Philipp is on that.

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 12 • 8 years ago

At least part three will need bug 1258835 applied. I've split this one into multiple parts for easier review. I suspect the first patch will not need a big review, just a sanity check.

	[:MakeMyDay]
	Comment 15 • 8 years ago

Comment on attachment 8733546 [details] [diff] [review]
Part1: pull in the latest ical.js

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

Ok, lets go with that. Just some minor nits you may want to consider here.

::: calendar/base/modules/ical.js
@@ +8,5 @@
>   *
>   * If you would like to change anything in ical.js, it is required to do so
>   * upstream first.
>   *
> + * Current ical.js git revision: f4fb1b9564f990f7b718253f2251bb30f58c9a5a

I know you haven't tagged a release on GH for this, but it's probably not a bad idea to do so and also reference that release number here.

@@ +42,5 @@
>  
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + * Portions Copyright (C) Philipp Kewisch, 2011-2015 */

2016

	Philipp Kewisch [:Fallen] [:📆][:🧩] Assignee
	Comment 16 • 8 years ago

(In reply to MakeMyDay from comment #15)
> I know you haven't tagged a release on GH for this, but it's probably not a
> bad idea to do so and also reference that release number here.

Created v1.2.0 and adapted the file.


> > + * Portions Copyright (C) Philipp Kewisch, 2011-2015 */
> 
> 2016

Actually, that specific file was probably last changed in '15. I've changed the top header (which includes 2012) to remove attribution dates though, everything else needs to be fixed in ical.js.

	:aceman
	Comment 18 • 8 years ago

Is this really ready for checkin, as bug 1258835 is not yet done?

	:aceman
	Comment 19 • 8 years ago

Sorry it is done, checkin needed too :)

	:aceman
	Comment 20 • 8 years ago

There was a slight conflict in test_items.js. Hopefully I fixed it correctly:
https://hg.mozilla.org/comm-central/rev/307947d546f5e3466ba6ab33405910301bcf16a8
https://hg.mozilla.org/comm-central/rev/441f2364e9dbed732b08150e1d858ab72cf1e7ab
https://hg.mozilla.org/comm-central/rev/8825d5dc84106c26369c3748b508551a8e5921a4