Changing calIDateTime for the javascript ical parser

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Changing calIDateTime for the javascript ical parser

Philipp Kewisch-2
Hello All,

I am working on bug 677982, where I want to get rid of our binary
components to make the upgrade process a bit easier, since we can have
one version of Lightning support multiple versions of Thunderbird.

I have made the js libical clone with browser compatibility in mind, so
maybe it can be released as a browser-compatible js library afterwards.

There is one issue I've come across though: in calIDateTime, the
attribute month accepts a range from 0 (January) to 11 (December). I
don't very much like this, since its much more intuitive to use 1
(January) to 12 (December). There are two options here:

Changing the interface: This is the method I prefer. We would change
calIDateTime's documentation and implementation to dictate a 1-based
month number:
   Pro: Makes code much cleaner and is more intuitive
   Con: This will break all extensions that use calIDateTime and directly
        manipulate the month attribute. Even worse, it will probably
        break them silently, since it doesn't make sense to rename the
        attribute here.

The other option would be to adapt the parser to accept months 0 - 11.
Pro/Con are flipped here. The biggest plus is that it doesn't break
extensions, but it will be really annoying to do it right in the code.

I will be changing a lot of code, very deep inside Lightning with this
change. I think this is the best point in time to make sure the API is
at its best and the only point in time where we can do changes like this.

Extensions wanting to be compatible with an older & newer lightning
version could work around using this code to get/set the month:

function setMonth(dt, val) {
   const OLD_ID = "{04139dff-a6f0-446d-9aec-2062df887ef2}";
   if (Components.interfaces.calIDateTime.number == OLD_ID) {
     dt.month = val - 1;
   } else {
     dt.month = val;
   }
   return val;
}

function getMonth(dt, val) {
   const OLD_ID = "{04139dff-a6f0-446d-9aec-2062df887ef2}";
   if (Components.interfaces.calIDateTime.number == OLD_ID) {
     return dt.month + 1;
   } else {
     return dt.month;
   }
}

To further mitigate, we might be able to use the addon compatibility
reporter to not bump Thunderbird extensions that use calIDateTime
automaticaly. I haven't asked the experts on this though.


Now I'd love to hear your input on this topic. Do you think its a good
idea to change the API now?

Philipp
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

gNeandr-6
Hi Philipp,

I'm far way to have knowledge of the inside of Lightning code. That
said, I'm promoting to stay with month numbering from 0 .. 11. That's
the normal behavior with JS, eg. .get/.setMonth(). Not only the JS is
like that, but all libraries and RFCs go that way.

So it's a pro for your 'CON' ;)

Günter
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Philipp Kewisch-2
On 2/4/12 12:37 AM, gNeandr wrote:

> Hi Philipp,
>
> I'm far way to have knowledge of the inside of Lightning code. That
> said, I'm promoting to stay with month numbering from 0 .. 11. That's
> the normal behavior with JS, eg. .get/.setMonth(). Not only the JS is
> like that, but all libraries and RFCs go that way.
>
> So it's a pro for your 'CON' ;)
>
> Günter

I am aware jsdate goes that way, but could you give me some supporting
evidence that other RFCs do it? rfc5545 uses 1 in the date formats for
example.

The library I'm creating will have methods to convert from/to jsdate, so
the user will not have to think when plugging in a jsdate.

Philipp
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

gNeandr-6
Am 04.02.2012 09:08, schrieb Philipp Kewisch:

> On 2/4/12 12:37 AM, gNeandr wrote:
>> Hi Philipp,
>>
>> I'm far way to have knowledge of the inside of Lightning code. That
>> said, I'm promoting to stay with month numbering from 0 .. 11. That's
>> the normal behavior with JS, eg. .get/.setMonth(). Not only the JS is
>> like that, but all libraries and RFCs go that way.
>>
>> So it's a pro for your 'CON' ;)
>>
>> Günter
>
> I am aware jsdate goes that way, but could you give me some supporting
> evidence that other RFCs do it? rfc5545 uses 1 in the date formats for
> example.
>
> The library I'm creating will have methods to convert from/to jsdate, so
> the user will not have to think when plugging in a jsdate.
>
> Philipp

OK, your point ;)
My mistake, the rfc count from 1 ..12. So leaving the JS argument.
Have a look into another great lib:
http://keith-wood.name/datepick.html
go down to "A note on Date"

Günter
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Simon Paquet-2
In reply to this post by Philipp Kewisch-2
Philipp Kewisch wrote on 03. Feb 2012:

> Now I'd love to hear your input on this topic. Do you think its a
> good idea to change the API now?

Now, as you say, is the best time to do stuff like that. So if you
want to get rid of this, then I would advise to do it now.

Cya
Simon

_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Markus Adrario
In reply to this post by Philipp Kewisch-2
Hi,

I vote for the change.
I always found it awkward in programming, if you have the months
numbered starting at 0.
Did anyone ever think about numbering the days from 0 to 30 instead of
1 to 31? I dare to guess no.
It makes coding much more intuitive and so if we have the opportunity
now, lets do it - even if JS is the other way round.

Sure there might be some extension maintainers who hate it - but in
the long run I'm sure it's worth it.

Markus
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Stefan Sitter-2
In reply to this post by Philipp Kewisch-2
I want to raise some concerns here. Replacing libical with a self
written library sounds like a long term and high risk project to me.
Before the new library can be enabled it must be at least feature
complete to libical to ensure that nothing regresses. Maybe both
libraries should be shipped in Lightning for some time with a hidden
preference to toggle between them. This might be the only way to get
feedback on it. In addition it might be worth to check what libical
unittest cases exist and create similar tests for the new library.
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Philipp Kewisch-2
On 2/5/12 10:39 PM, Stefan Sitter wrote:
> I want to raise some concerns here. Replacing libical with a self
> written library sounds like a long term and high risk project to me.
> Before the new library can be enabled it must be at least feature
> complete to libical to ensure that nothing regresses. Maybe both
> libraries should be shipped in Lightning for some time with a hidden
> preference to toggle between them. This might be the only way to get
> feedback on it. In addition it might be worth to check what libical
> unittest cases exist and create similar tests for the new library.

I will make sure that the new library has unit tests that cover a large
portion (>90%) of the library code. I have put a substantial amount of
time into this to date and before I land it I will make sure its feature
complete. I've also thought about looking which testcases libical has,
but I haven't looked into that yet.

When I started on this, I first rewrote the parser based on the RFC (not
based on libical). Then I looked at which functions of libical we were
using and translated *all* code in libical (with all its dependencies),
so there shouldn't be any regressions because of missing functionality.
Of course that doesn't mean no regressions because of my typos or
problems with the actual parser.

Given the amount of changes needed to move to a js-only parser (i.e
rewriting all our C++ xpcom components into javascript), I don't see a
possibility to ship both libraries at the same time. The only thing that
comes to mind now is a separate extension for people having issues that
holds the C++ components and overwrites the javascript registration.
This might introduce more complexity than needed though.

I totally understand your concerns and I agree this will be quite a big
step, but given discussion on prohibiting binary components on AMO and
also the vast number of advantages we get with js-only code, I think its
worth it.

Philipp
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Philipp Kewisch-2
In reply to this post by Philipp Kewisch-2
Some more information and a heads up:

the old libical uses 1-based months, our xpcom components seem to
translate that into 0-based.

I found a way to wrap the new jsical in a way that I could keep the old
interface!

To alleviate Stefan's concerns a bit it might be best to first migrate
to a 1-based jsical and 0-based xpcom components as it was before, and
then change the interface in a second step when we see that the change
is stable.

There are about 1284 lines of code that contain the word "month" in
calendar code, so the risk of regressions here is quite high.

Philipp
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Robert Kaiser
In reply to this post by Philipp Kewisch-2
Philipp Kewisch schrieb:
> There is one issue I've come across though: in calIDateTime, the
> attribute month accepts a range from 0 (January) to 11 (December). I
> don't very much like this, since its much more intuitive to use 1
> (January) to 12 (December).

I basically agree with you, but incidentally, this matches JavaScript
convention of the getMonth() function on the Date object, see
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date/getMonth

In that light, I'd be tempted to advocate for actually leaving it as it
is, with 0-11, for being compatible with both the libical side and
getMonth().

Robert Kaiser
_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar
Reply | Threaded
Open this post in threaded view
|

Re: Changing calIDateTime for the javascript ical parser

Marcel Stör
In reply to this post by Philipp Kewisch-2
On 03.02.2012 23:52, Philipp Kewisch wrote:
> There is one issue I've come across though: in calIDateTime, the
> attribute month accepts a range from 0 (January) to 11 (December). I
> don't very much like this, since its much more intuitive to use 1
> (January) to 12 (December).

Personally I don't like that either. Unfortunately with some programming
languages it's 0-11 (e.g. JavaScript, Java) with others it's 1-12 (e.g.
Pythom, Ruby). Since you're programming in JavaScript I'd definitely opt
for sticking with the language default. Reasons: consistency, least
surprises for whoever else reads your code.

+1 for 0-11

Cheers,
Marcel

--
Marcel Stör, http://www.frightanic.com
O< ascii ribbon campaign - stop html mail - www.asciiribbon.org
-> I kill Google posts: http://twovoyagers.com/improve-usenet.org/

_______________________________________________
dev-apps-calendar mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-apps-calendar