351330 - Should be able to set other datatypes for calIICalProperty.value

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

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

It should be possible to set a calIDateTime or calIPeriod as the value of a property in ical.
Part of a solution could be to make calIIcalProperty.value an nsIVariant, just like calIItemBase.setProperty takes.

(see bug 351184 for why this is needed)

Comment 1

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

Attachment: add a new calIIcalValue

Using an nsIVariant didn't work, because it's not easy to make a calIDateTime an nsIVariant. At serialization, you need to know the exact type. But when using the nsISupports in nsIVariant, you don't know the type anymore.

So I made the relevant interface inherit from a new interface, calIIcalValue. Using that interface, you can serialize the property value without knowing the exact value.

Comment 2

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

Attachment: testcase

Attaching a xpshell test script. It shows how the interface can be used. In fact, you don't directly use the new interface at all. It's all simple: set an calIPeriod or calIDateTime as .icalValue. The new interface is just for the behind-the-screen black magic.

Comment 3

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

Comment on attachment 249583 [details] [diff] [review]
add a new calIIcalValue

Whoops, this patch isn't finished.

Comment 4

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

Attachment: new patch, no new interfaces

This patch takes a different route. It's more simple and more easy to understand, in my opinion.
It is not as pluggable as the previous patch, but I don't think we will be adding many more value types anyway. So this should work just as well.

Comment 5

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

Attachment: new patch, no new interfaces, now works

Previous patch had a small error, that made it not compile. This patch has that fixed.

Comment 6

[Daniel Boelzle [:dbo]]

Comment on attachment 249635 [details] [diff] [review]
new patch, no new interfaces, now works

I am a bit indifferent whether to favor
- a variant-based solution
- or to just offer serialization helpers, e.g. calIDateTime has a method
  calIIcalProperty createIcalProperty(in AUTF8String propName);
- or this explicit version of support for multiple data types

However, this version ought to have some kind of readonly attribute stating what's in it. Anything else would clients leave alone testing.

>RCS file: /cvsroot/mozilla/calendar/base/src/calICSService.cpp,v
>+calIcalProperty::GetValueAsDateTime(calIDateTime **aValueAsDateTime)
>+{
if (!aValueAsDateTime)
    return NS_ERROR_NULL_POINTER;
>+    struct icaltimetype idt = 
>+        icalvalue_get_datetime(icalproperty_get_value(mProperty));
- need more error handling here
- we need differentiation between DATE and DATETIME here, else we get false errors for DATEs when calling icalvalue_get_datetime, don't we?
- proposal:
icalvalue const* const val = icalproperty_get_value(mProperty);
switch (icalvalue_isa(val)) {
case ICAL_DATE_VALUE:
    idt = icalvalue_get_date(val);
    break;
case ICAL_DATETIME_VALUE:
    idt = icalvalue_get_datetime(val);
    break;
default:
    return NS_ERROR_NOT_AVAILABLE;
}

>+calIcalProperty::SetValueAsDateTime(calIDateTime *aValueAsDateTime)
>+{
if (!aValueAsDateTime)
    return NS_ERROR_NULL_POINTER;
>+    struct icaltimetype idt;
>+    aValueAsDateTime->ToIcalTime(&idt);
same differentiation here?
if (idt.is_date) {
    icalproperty_set_value(mProperty, icalvalue_new_date(idt));
}
else {
    icalproperty_set_value(mProperty, icalvalue_new_datetime(idt));
}

>+calIcalProperty::GetValueAsPeriod(calIPeriod **aValueAsPeriod)
>+{
if (!aValueAsPeriod)
    return NS_ERROR_NULL_POINTER;
icalvalue const* const val = icalproperty_get_value(mProperty);
if (icalvalue_isa(val) != ICAL_PERIOD_VALUE)
    return NS_ERROR_NOT_AVAILABLE;
icalperiodtype ip =  icalvalue_get_period(val);
>+    calPeriod *cp = new calPeriod(&ip);

>+calIcalProperty::SetValueAsPeriod(calIPeriod *aValueAsPeriod)
>+{
if (!aValueAsPeriod)...

>+calIcalProperty::GetValueAsDuration(calIDuration **aValueAsDuration)
>+{
if (!aValueAsDuration)...
- test for ICAL_DURATION_VALUE

>+calIcalProperty::SetValueAsDuration(calIDuration *aValueAsDuration)
>+{
if (!aValueAsDuration)...



Although I don't know what the below property implementation has to do with the overall bug (and code above), I am continuing...

>RCS file: /cvsroot/mozilla/calendar/base/src/calItemBase.js,v
>+/*
>+// A property is a combination of a name and a value, and is xpcom-compatible
>+// with nsIProperty. Using our own implementation, because of bug 
>+function property(aName, aValue) {
why not capitalize, ie "Property" or prefix all with "js": jsProperty, jsPropertyEnumerator, ...?

>+    this.mName = aName;
>+    this.mValue = aValue;
>+}
>+
>+property.prototype = {
>+    QueryInterface: function() {
make unanonymous
>+        if (!aIID.equals(Components.interfaces.nsISupports) &&
>+            !aIID.equals(Components.interfaces.nsIProperty))
>+        {
>+            throw Components.results.NS_ERROR_NO_INTERFACE;
>+        }
>+        return this;
>+    },
>+    get name() {
>+        return this.mName;
>+    },
>+    get value() {
>+        return this.mValue;
>+    }
>+}
>+
>+// Enumerate properties
>+function propertyEnumerator(aArray) {
>+    this.mProperties = [];
>+    var i=0;
>+    for (key in aArray) {
>+        this.mProperties[i] = new property(key, aArray[key]);
>+        i++;
>+    }
>+    this.mCounter = 0;
>+};
aArray seems to be misleading, why not just "obj"?

>+propertyEnumerator.prototype = {
>+    hasMoreElements: function() {
make unanonymous
>+        return (this.mCounter < this.mProperties.length - 1);
why -1?
>+    },
>+    
>+    getNext: function() {
make unanonymous
>+        return this.mProperties[this.mCounter++];
>+    }
>+}
>+
>++function jsProperties() {
>++    this.mData = [];
>++}
>++
>++jsProperties.prototype = {
>++    setProperty: function(aName, aValue) {
make unanonymous
>++        this.mData[aName] = aValue;
>++    },
>++  
>++    getProperty: function(aName) {
make unanonymous
>++        return this.mData[aName];
>++    },
>++
>++    get enumerator() {
make unanonymous
>++        return new jsPropertyEnumerator(this.mData);
copy of all values just to get an enumerator... hmm.
Wouldn't it be better to do a two-step lookup, i.e.
- an obj { property-name -> array-index } for assoc index lookup
- an array { index -> value } for the actual value lookup
That way we could enumerate the array (the enumerator would just hold the counter and a reference to the hosting object) whereas preserving assoc lookup.
However, removing a property would be painful, correcting indices.
Enumeration seems to be a pain point (IMO because of the massive cloning of items), so I would vote against copying all properties when creating an enumerator.
What do you think?

> function calItemBase() {
>     this.mPropertyParams = {};
> }
> 
> calItemBase.prototype = {
>     mPropertyParams: null,
unused

>     mIsProxy: false,
> 
>@@ -135,20 +202,22 @@ calItemBase.prototype = {
>         if (this.mOrganizer)
>             this.mOrganizer.makeImmutable();
>         if (this.mAttendees) {
>             for (var i = 0; i < this.mAttendees.length; i++)
>                 this.mAttendees[i].makeImmutable();
>         }
> 
>         var e = this.mProperties.enumerator;
>         while (e.hasMoreElements()) {
>-            var prop = e.getNext().QueryInterface(Components.interfaces.nsIProperty);
>-            var val = prop.value;
>+            //var prop = e.getNext().QueryInterface(Components.interfaces.nsIProperty);
>+            //var val = prop.value;
>+            var prop = e.getNext();
>+            var val = prop.nsIProperty.value;

Ok, the code seems unfinished.

The new property bag is not in use. However, when it is in use, can't we remove the above tweak for xpconnect (prop.nsIProperty)?
We can straight-forward use the js implementation then.

Comment 7

[Daniel Boelzle [:dbo]]

(In reply to comment #1)
> Using an nsIVariant didn't work, because it's not easy to make a calIDateTime
> an nsIVariant. At serialization, you need to know the exact type. But when

My experience always was that xpconnect handles that for you. I.e. it wraps around an nsVariant when calling native code expecting nsIVariant.

Comment 8

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

(In reply to comment #7)
> My experience always was that xpconnect handles that for you. I.e. it wraps
> around an nsVariant when calling native code expecting nsIVariant.

But I don't want to trust on xpconnect magic. With components written in c++, there is no xpconnect. That's why I did not use nsIVariant.

And sorry for screwing up the patch. I included stuff that should not have been there :(

Comment 9

[Daniel Boelzle [:dbo]]

mvl, is this bug obsolete since we have an attribute calIIcalProperty::valueAsDatetime? I think specially typed attributes for calIIcalProperty make only sense if a property has additional parameters that come along with the value (e.g. date-time comes along with VALUE or TZID parameter). I don't see the need for periods; the string attribute (::value/::valueAsIcalString) should be ok for those.

Comment 10

[Martin Schröder [:mschroeder]]

mvl, Daniel, can this bug be marked as resolved INVALID/WONTFIX/WORKSFORME, or is still a patch needed?

Comment 11

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

Lets close this until we need PERIOD support, this will happen automatically if we need it.