157169 - Calendar needs SEQUENCE and TRANSP properties

Status: RESOLVED WORKSFORME

(Calendar :: Provider: Local Storage, enhancement)

Description

[ajbu]

I have a patch to add SEQUENCE and TRANSP properties, I'll upload it here
Maybe it would be useful to have the SEQUENCE number increased on every call 
to ModifyEvent?

Comment 1

[Mostafa Hosseini]

These properties are not mandatory according to the standard. So why should this
bug block 129660?

Comment 2

[ajbu]

You are right, although TRANSP says ti SHOULD be set to OPAQUE if the event is 
to take up time, SEQUENCE is only a MUST if there is an organiser. I did set 
the block because Sequence was mentioned in bug 129660
I'll remove the block, and might add a new tracker bug for the iTip rfc if one 
isn't already there.

Comment 3

[ajbu]

Attachment: Add transp and sequence properties

Adds TRANSP and SEQUENCE

I renamed the property to sequenceNumber, 'sequence' seems to be reserved for
the idl.
Sequence is defined in libical as int, I used PRUint32 as type, not sure if
there is a better match in idl?

Comment 4

[Mostafa Hosseini]

I didn't find a better place to discuss this other than in this bug.
I was thinking maybe we are at the point where we can add a general purpose
setter and getter for calendar component properties rather than using a seperate
interface for each property. This will relieve us from worrying about all
unimplemented properties and will reduce the code effectively in contrast to
adding a set of functions for each property defined in the standard.
If agreed maybe we can leave SEQUENCE and TRANSP and the rest of optional and
non-significant properties to be dealt with through those functions.

Comment 5

[ajbu]

Sounds like a good suggestion, funny that I was thinking about the same thing 
this weekend.
It looks like support for RFC 2445 is now complete, and 2446 implementation is 
almost there. Properties like SEQUENCE can probably be set with a good default 
from libxpical so those are not really needed often in the interface. A 
generic get/set property could save adding those properties to the interface, 
and at a later time when Calendar is stable we can look at what is actually 
used and optimize the interface when needed.

Comment 6

[Mike Potter]

*** Bug 167553 has been marked as a duplicate of this bug. ***

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> I didn't find a better place to discuss this other than in this bug.
> I was thinking maybe we are at the point where we can add a general purpose
> setter and getter for calendar component properties rather than using a seperate
> interface for each property.

I think this is a great idea; it would also simplify the implementation of new
calendar back-ends (e.g. bug 263620 for storage, or caldav, etc.), because for
initial impls we can implement a much smaller subset of properties than will be
generally supported.

However, with such an impl, how would you handle properties having different types?

Comment 8

[Mostafa Hosseini]

(In reply to comment #7)
> However, with such an impl, how would you handle properties having different
types?

Maybe a function for each data type?
Maybe a common AUTF8String argument with conversions on both ends?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #8)
> (In reply to comment #7)
> > However, with such an impl, how would you handle properties having different
> types?
> 
> Maybe a function for each data type?
> Maybe a common AUTF8String argument with conversions on both ends?

I don't like the conversion; seems like a lot of overhead just to keep the API
pure in some fashion.  A function for each data type wouldn't be too bad; the
API can just return an error if it's not a valid property for that type..

Comment 10

[Justin Wood (:Callek)]

How about a structure with a union + member where the union has a list of
however many DataTypes we need, and the member just says which data-type is
being used, this structure can be passed back and forth without incident,
(similar to how CSSValues are stored, if I remember right)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Would using nsIVariant be a good solution?

Comment 12

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 13

[Joey Minta]

So... it's certainly possible to add these properties already, using the current calIItemBase.  Since this bug was opened for the purpose making it possible to set these properties, we either need to morph this, or close it.

Comment 14

[Matthew (lilmatt) Willis]

Comment on attachment 91170 [details] [diff] [review]
Add transp and sequence properties

R.I.P. libxpical

Comment 15

[Matthew (lilmatt) Willis]

bug 333688 adds sequence to the sqlite schema. Does transparency need its own column, or should it just get thrown into the property bag?

Comment 16

[Matthew (lilmatt) Willis]

This is something ctalbert needs...

-> ctalbert

Comment 17

[Joey Minta]

Why isn't set/getProperty("SEQUENCE") sufficient here? -> WORKSFORME?

Comment 18

[cmtalbert]

(In reply to comment #17)
> Why isn't set/getProperty("SEQUENCE") sufficient here? -> WORKSFORME?
> 
We will need the ability to search the calendar for events with a given UID, and then for any UID's that match our query, we must analyze the Sequence ID. So, it doesn't sound like it would need to be an indexed field, per se. So, given that, I don't have any reason why it cannot be part of the property bag.

The only caveat to this is that the iTIP spec maintains that the SEQUENCE id is the secondary key used to uniquely identify any event. So, in the future we might want it promoted to an indexable column and a first order property, but I think we should start out with it in the property bag and then optimize later, if needed.

I think TRANSP should just be a property as well.