909183 - calIDateTime.compare returns incorrect result with floating timezone

Status: RESOLVED FIXED

(Calendar :: ICAL.js Integration, defect)

Description

[Geoff Lankow (:darktrojan)]

> let a = cal.now();
> a.timezone = cal.getTimezoneService().getTimezone('Pacific/Auckland')
>
> let b = a.clone()
> b.timezone = cal.floating();
>
> a.compare(b)

This code returns 0 with libical and -1 with js ical.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 909183-1.diff

Comment 2

[Mohit Kanwal [:redDragon]]

Comment on attachment 809060 [details] [diff] [review]
909183-1.diff

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

Looks good to me. Can a corresponding test be incorporated so that we don't break this in the future :)

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 809060 [details] [diff] [review]
909183-1.diff

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

Sorry to r- this, but I think we should discuss it first, and a test would indeed be nice. As this is an ical.js change, the best way to do it would be to clone https://github.com/mozilla-comm/ical.js and create a test for it there. Then we can use this bug (or bug 932217) to update the Lightning code to what we have in ical.js.

::: calendar/base/modules/ical.js
@@ +3689,5 @@
>      compare: function icaltime_compare(other) {
> +      var a = this;
> +      var b = other;
> +
> +      if (this.zone.tzid == "floating" || other.zone.tzid == "floating") {

I think it would be better to compare this.zone == ICAL.Timezone.localTimezone, as the "floating" tzid is more of an implementation detail.

@@ +3709,5 @@
>        }
> +
> +      if (this.zone.tzid == "floating" || other.zone.tzid == "floating") {
> +        tz = ICAL.Timezone.localTimezone;
> +      }

If we change it this way, then the tz parameter will be ignored if one of the timezones is local. Is this really the right thing to do? Unfortunately I don't have a good suggestion on how this can be fixed smoothly.

I'm also not sure this change still matches the behavior of what compareDateOnlyTz was intended for. Just from looking, if this.zone is local, then other.convertToZone(tz) and this.convertToZone(tz) should still be converting to the same timezone, right? Maybe you could tell me what I am missing...

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> > +      if (this.zone.tzid == "floating" || other.zone.tzid == "floating") {
> > +        tz = ICAL.Timezone.localTimezone;
> > +      }
> 
> If we change it this way, then the tz parameter will be ignored if one of
> the timezones is local. Is this really the right thing to do? Unfortunately
> I don't have a good suggestion on how this can be fixed smoothly.

Based on the comment at http://hg.mozilla.org/comm-central/file/825e087ea3f6/calendar/base/backend/libical/calIDateTime.idl#l180 (and also the same comment in calIDateTimeJS.idl), I figure if libical does it that way, then ical.js should too.

> I'm also not sure this change still matches the behavior of what
> compareDateOnlyTz was intended for. Just from looking, if this.zone is
> local, then other.convertToZone(tz) and this.convertToZone(tz) should still
> be converting to the same timezone, right? Maybe you could tell me what I am
> missing...

I don't know either. I think I did it because I thought both functions should be changed. I can't decide if that change should happen or not. :-/

I wrote some tests, since there are none for the compare functions. Do any of them do something you would not expect? https://github.com/darktrojan/ical.js/commit/b628f5dd4f70d6686fb486e7c21bfc6f32dbcaa3

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

If you can change the timezone comparison to use this.zone == ICAL.Timezone.localTimezone instead and it works, I'm happy to r+ it. Sorry for the long delay, I just wasn't quite sure how to answer.


(In reply to Geoff Lankow (:darktrojan) from comment #4)
> (In reply to Philipp Kewisch [:Fallen] from comment #3)
> > > +      if (this.zone.tzid == "floating" || other.zone.tzid == "floating") {
> > > +        tz = ICAL.Timezone.localTimezone;
> > > +      }
> > 
> > If we change it this way, then the tz parameter will be ignored if one of
> > the timezones is local. Is this really the right thing to do? Unfortunately
> > I don't have a good suggestion on how this can be fixed smoothly.
> 
> Based on the comment at
> http://hg.mozilla.org/comm-central/file/825e087ea3f6/calendar/base/backend/
> libical/calIDateTime.idl#l180 (and also the same comment in
> calIDateTimeJS.idl), I figure if libical does it that way, then ical.js
> should too.

My main concern was that the gaia calendar app uses this method and we would be breaking something. I recalled that lightsofapollo had made some changes to those methods, but after an intensive look at git blame, all that was done was to make the compare method independent of compareDateOnlyTz and it is us who actually need that method. I've also checked https://github.com/mozilla-b2g/gaia and a simple github search doesn't show any use of compareDateOnlyTz, so we are free to change it as we like.

I think back at the time b2g was also using the master tag when getting the repo, by now they use @0.0.2

As you've suggested, go ahead and make the change. Maybe you can add some jsdoc to the compare and compareDateOnlyTz methods while you are at it.

> 
> I wrote some tests, since there are none for the compare functions. Do any
> of them do something you would not expect?
> https://github.com/darktrojan/ical.js/commit/
> b628f5dd4f70d6686fb486e7c21bfc6f32dbcaa3
The tests look great, thanks! Feel free to also send this as a pull request to github.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 909183-2.diff

I looked at the C implementation of this, and realised I'd fixed it in the wrong place (despite identifying the right place when posting the bug). Now I've done the zone conversion in the right place.

I also fixed another inconsistency: we should use compareDateOnlyTz if either or both sides are dates (previously we did it if only one was a date). Actually I think the result is the same, but it's more obvious what's going on now and it's the same as in calDateTime.cpp.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8541518 [details] [diff] [review]
909183-2.diff

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

I like it, r=philipp.

Since you've already written tests and I believe we didn't have any before, could you send an ical.js pull request to add those tests as far as they make sense?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ah you already did, good thinking!

Comment 9

[Richard Marti (:Paenglab)]

https://hg.mozilla.org/comm-central/rev/202b212de644