Closed Bug 330371 Opened 14 years ago Closed 13 years ago

Need calIPeriod

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Assigned: mvl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

freebusy needs period support. So we need to implement calIPeriod.idl
rfc2445 4.3.9
Attached patch patch v1 (obsolete) — Splinter Review
Initial patch. Pretty straight-forward.
Assignee: base → mvl
Status: NEW → ASSIGNED
Attachment #214935 - Flags: first-review?(dmose)
Attachment #214935 - Flags: second-review?(dmose)
Attachment #214935 - Flags: first-review?(dmose)
Attachment #214935 - Flags: first-review?(daniel.boelzle)
(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 on attachment 214935 [details] [diff] [review]
patch v1

r- for now as commented
Attachment #214935 - Flags: first-review?(daniel.boelzle) → first-review-
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.
Attachment #214935 - Flags: second-review?(dmose)
Attached patch patch v2Splinter Review
finally, a new version of the patch with review comments addressed.
Attachment #214935 - Attachment is obsolete: true
Attachment #246201 - Flags: first-review?(daniel.boelzle)
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?
Attachment #246201 - Flags: first-review?(daniel.boelzle) → first-review+
plus

> +calPeriod::calPeriod(struct icalperiodtype *aPeriodPtr)
> +{
icalperiodtype const*
patch checked in witch comments from daniel fixed, except the 'const' changed. They don't work well with the definitions in the idl file.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.