330371 - Need calIPeriod

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

freebusy needs period support. So we need to implement calIPeriod.idl
rfc2445 4.3.9

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

Initial patch. Pretty straight-forward.

Comment 2

[Daniel Boelzle [:dbo]]

(In reply to comment #1)
> Created an attachment (id=214935) [edit]
> patch v1

> Index: base/public/calIPrintFormatter.idl
I assume calIPrintFormatter does not relate to this patch...

> +calPeriod::calPeriod(const calPeriod& cpt)
> +{
> +    cpt.mStart->Clone(getter_AddRefs(mStart));
> +    cpt.mEnd->Clone(getter_AddRefs(mEnd));
This crashes if the cpt still has no start/end, i.e. cpt is default constructed.

> +NS_IMETHODIMP calPeriod::GetStart(calIDateTime **_retval)
> +{
> +    //XXX clone?
I wouldn't clone here (as you are not doing) but make the dateTime objects immutable when they are set (i.e. in SetStart, SetEnd). This doesn't hurt performance, especially because those dateTime objects are freshly by calling GetInTimezone():

> +NS_IMETHODIMP calPeriod::SetStart(calIDateTime *aValue)
> +{
> +    NS_ENSURE_ARG_POINTER(aValue);
> +    if (mImmutable) return NS_ERROR_CALENDAR_IMMUTABLE;
> +    // rfc2445 says that periods are always in utc. libical ignore that,
> +    // so we need the conversion here.
> +    aValue->GetInTimezone(NS_LITERAL_CSTRING("UTC"), getter_AddRefs(mStart));
return mStart->makeImmutable() here

> +NS_IMETHODIMP calPeriod::GetEnd(calIDateTime **_retval)
> +{
> +    //XXX clone?
[...]
> +    aValue->GetInTimezone(NS_LITERAL_CSTRING("UTC"), getter_AddRefs(mEnd));
same here.

> +NS_IMETHODIMP_(void)
> +calPeriod::ToIcalPeriod(struct icalperiodtype *icalp)
> +{
> +    mStart->ToIcalTime(&icalp->start);
> +    mEnd->ToIcalTime(&icalp->end);
Make robust. mStart, mEnd may not yet be set.
> +    return;
obsolete.

> +void
> +calPeriod::FromIcalPeriod(const struct icalperiodtype * const icalp)
> +{
> +    return;
still unfinished. If you need calPeriod(icalperiodtype const*), I would suggest something like:
 mStart = new calDateTime(&icalp->start);
 mStart->makeImmutable();
 mEnd = new calDateTime(&icalp->end);
 mEnd->makeImmutable();

> +NS_IMETHODIMP
> +calPeriod::SetIcalString(const nsACString& aIcalString)
> +{
> +    struct icalperiodtype ip;
> +    ip = icalperiodtype_from_string(nsPromiseFlatCString(aIcalString).get());
> +    //XXX Shortcut. Assumes our calIDateTime impl is the only one.
> +    //    Should use NS_NEWXPCOM()
> +    mStart = new calDateTime(&ip.start);
> +    mEnd = new calDateTime(&ip.end);
just call FromIcalPeriod(ip) for short.

> +class calPeriod : public calIPeriod
[...]
> +    calPeriod (const struct icalperiodtype * const aPeriodPtr);
I suggest to make this ctor explicit.
Remove that last const, calPeriod(icalperiodtype const*) is const enough: more const is redundant and just confuses the reader of your interface (referring to Sutter/Alexandrescu C++ Coding Guidelines, item #15, page 31 here).

> +protected:
I suggest to add a protected virtual dtor here, so users of this class are forced to stick to XPCOM mimics of destruction.

> +    //struct icaldurationtype mPeriod;
> +    //nsCOMPtr<calIDuration> mDuration;
obsolete.
+    
+    void FromIcalPeriod(const struct icalperiodtype * const icalp);
remove latter const (+ cpp file).

Comment 3

[Daniel Boelzle [:dbo]]

Comment on attachment 214935 [details] [diff] [review]
patch v1

r- for now as commented

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 214935 [details] [diff] [review]
patch v1

There's enough changes requested in the first review that it would be really helpful to see an updated patch before reviewing.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

finally, a new version of the patch with review comments addressed.

Comment 6

[Daniel Boelzle [:dbo]]

Comment on attachment 246201 [details] [diff] [review]
patch v2

I am actually waiting for calIPeriod and can suspend calIWcapFreeBusyEntry when it's in...

r=dbo, only minor style/comments, no bugs:

> +    // copies are always mutable
> +    mImmutable = PR_FALSE;
Place mImmutable(PR_FALSE) in initializer list of calPeriod(calPeriod const&) and calPeriod(icalperiodtime *) instead.

> +NS_IMETHODIMP calPeriod::GetStart(calIDateTime **_retval)
> +{
> +    //XXX clone?
I wouldn't clone either, but always keep mStart/mEnd immutable like you do.
IMO you can remove the XXX comment.

> +NS_IMETHODIMP_(void)
> +calPeriod::ToIcalPeriod(struct icalperiodtype *icalp)
> +{
> +    // makes no sense to create a duration without a start or end
> +    if (!mStart || !mEnd)
> +        return;
would setting
*icalp = icalperiodtype_is_null_period();
be sensible?

> +void
> +calPeriod::FromIcalPeriod(struct icalperiodtype *icalp)
icalperiod const* ?

> +{
> +NS_IMETHODIMP
> +calPeriod::GetIcalString(nsACString& aResult)
> +{
> +    if (!mStart || !mEnd)
> +        return NS_ERROR_FAILURE;
Hmm, this means that we cannot get a string repr out a default constructed calPeriod... With the above change in toIcalPeriod(), you can remove this if-statement, thus GetIcalString will return a null-period then.

> +    //XXX Shortcut. Assumes our calIDateTime impl is the only one.
> +    //    Should use NS_NEWXPCOM()
IMO it doesn't matter whether there are other calIDateTime implementations as long as nobody relies on a concrete implementation, applies casts on pointers etc.
Maybe I miss your point here?

> +class calPeriod : public calIPeriod
> +{
> +public:
> +    calPeriod ();
> +    calPeriod (const calPeriod& cpt);
> +    calPeriod (struct icalperiodtype *aPeriodPtr);
make the latter two ctors explicit?

Comment 7

[Daniel Boelzle [:dbo]]

plus

> +calPeriod::calPeriod(struct icalperiodtype *aPeriodPtr)
> +{
icalperiodtype const*

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in witch comments from daniel fixed, except the 'const' changed. They don't work well with the definitions in the idl file.