446303 - use the "RELATED-TO" property

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Fred Jendrzejewski]

The rfc2445 defines the related-to property. It defines the relation of calendar components with each other. Any task, event or journal can have this property. The following rules are given:
1.) The value must be the UID (is this already introduced ? ) of the related component.
2.) An event can only be at the top a relation
3.) Any operation on a component should be applied on its childs too.

Why is this interesting ? It helps to work on bug 194863 . And we could have a good way to couple components whenever needed.

I'll attach a testcase laterly.

Comment 1

[Bas van den Bosch]

1) every event or task has an UID already, you can check an exported ics. So that's covered.
2) from
http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-08#section-3.8.4.5
and
http://tools.ietf.org/html/rfc2445#section-4.8.4.5
I don't see why an event can only be a parent, not a child. It would be logical to be able to define events als childs of the parent. 
3) Not any operation should be applied to it's childs, it can be applied to it's childs if needed. The RFC states: "It is up to the target calendar system to maintain any property implications of this relationship." and " Changes to a calendar component referenced by this property can have an implicit impact on the related calendar component."

What do you want to accomplish in this bug, isn't it the same as bug 194863?

Comment 2

[Fred Jendrzejewski]

> 1) every event or task has an UID already, you can check an exported ics. So
> that's covered.
Found it now, thx.

> I don't see why an event can only be a parent, not a child. It would be logical
> to be able to define events als childs of the parent. 
http://tools.ietf.org/html/rfc2445#section-4.6.1
On page 54 it is said, that an event mustn't be nested. I understand it in this way: It isn't possible to have a relation with the type "Parent", but only as a "Child" or "Sybling", for an event.

> What do you want to accomplish in this bug, isn't it the same as bug 194863?
bug 194863 can be implemented properly by introducing this property. But it should be possible to do it without, as there seems to be a parentitem property.
The ability to serialize/unserialize the "related-to" property doesn't depend on the task tree.
Relations can be used for then just subtasks I think. It is nice to have this backend for extension developers, even if this is not used for the task-tree.

Comment 3

[Fred Jendrzejewski]

Attachment: Testcase

A testcase with two relations for one task. One with the reltype definitio and one without.

Comment 4

[Bas van den Bosch]

> On page 54 it is said, that an event mustn't be nested. I understand it in this
> way: It isn't possible to have a relation with the type "Parent", but only as a
> "Child" or "Sybling", for an event.

I think you read this the wrong way. A VEVENT can't be nested means there can't be a VEVENT within another VEVENT. They can be related though, as it said on the same page. The same is said for VTODO's : "The "VTODO" calendar component cannot be nested within another calendar component.However, "VTODO" calendar components can be related to each other or to a "VTODO" or to a "VJOURNAL" calendar   component with the "RELATED-TO" property.". Also the same holds for vjournal-components. This means these three make up the main items of the ics-definitions in rfc 2445. 

> 
> > What do you want to accomplish in this bug, isn't it the same as bug 194863?
> bug 194863 can be implemented properly by introducing this property. But it
> should be possible to do it without, as there seems to be a parentitem
> property.
> The ability to serialize/unserialize the "related-to" property doesn't depend
> on the task tree.
> Relations can be used for then just subtasks I think. It is nice to have this
> backend for extension developers, even if this is not used for the task-tree.
> 

I think no-one will want to implement subtasks without the correct ics-definition so we'll follow the approach of rfc 2445 if someone finds the time to do this.

Comment 5

[Fred Jendrzejewski]

Attachment: testcase2

Comment 6

[Fred Jendrzejewski]

Attachment: calIDuration - v1

The patch passes the written test, relations can be imported and exported properly.
Now I understood the thing with the nested events I think, but where do we search for those relations ? The parent (child, sybling) of this item can be defined in two ways.
1) PARENT relation of this item
2.) CHILD relation of the parent child.
We can solve this in two ways:
a) Normally we have to look for both, but this can become a very lengthy process if there are a lot of items and relations.
b) We only look for on the relations of the item and we save the relation for both items ( so we save the relation two times, but we don't need to do this cross-search)

Comment 7

[Daniel Boelzle [:dbo]]

Comment on attachment 331407 [details] [diff] [review]
calIDuration - v1

>Index: base/public/calIRelation.idl
>===================================================================
>RCS file: base/public/calIRelation.idl
>diff -N base/public/calIRelation.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ base/public/calIRelation.idl	25 Jul 2008 18:51:12 -0000
>@@ -0,0 +1,65 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
I think the header differs from the other ones, where's the contributor line?

>+[scriptable,uuid(a84f2a74-c7fd-42dc-ac4b-00aa77624039)]
>+interface calIRelation : nsISupports
>+{
>+    /**
>+     * The original item of the relation
>+     */
>+    attribute calIItemBase item;
Why do we need this? This would add an extra cycle reference, since the item holds references on its relations.

>+
>+    /**
>+     * The type of the relation between the items;
>+     */
>+    attribute AUTF8String relType;
You should document the allowed strings, maybe an unsigned long with predefined constants suits, too.

>+
>+    /**
>+     * The id of the related item
>+     **/
>+
>+    attribute AUTF8String relId;
>+
>+    /**
>+     * The calIIcalProperty corresponding to this object.  Can be used for
>+     * serializing/unserializing from ics files.
>+     */
>+
>+    attribute calIIcalProperty icalProperty;
>+
>+    /**
>+     * For accessing additional parameters, such as x-params.
>+     */
>+    AUTF8String getParamater(in AString name);
>+    void setParamater(in AString name, in AUTF8String value);
>+    void deleteParameter(in AString name);
>+};
I don't yet see a real need for an extra IDL interface. If we can cope with multi-properties on items (which we have to fix for CATEGORIES either way), then there's little extra sugar having this type.

Anyway, thanks for driving work on modeling relations. My comments shouldn't discourage you to go on ;-)

Comment 8

[Fred Jendrzejewski]

> I think the header differs from the other ones, where's the contributor line?
Yes, I forget to mention it. What do I write here, because I only found things from oracle, sun etc.
> Why do we need this? This would add an extra cycle reference, since the item
> holds references on its relations.
I saw that there is something like a parentItem, but I didn't understand this property at all.
> You should document the allowed strings, maybe an unsigned long with predefined
> constants suits, too.
You have an example of this ? Because i took the same type like for id's etc, so I thought that this is the standard :(
> If we can cope with multi-properties on items (which we have to fix for 
> CATEGORIES either way),then there's little extra sugar having this type.
I think CATEGORIES is something like CATEGORIES=cat1, cat2 etc. But there is only one category with multiple values. This isn't the case for a relation, this is more like an attachment.
>Anyway, thanks for driving work on modeling relations. My comments shouldn't
> discourage you to go on ;-)
I posted already in the mailing list, because I saw that I am a bitlost and that this one seems to be quite complex.:)

Comment 9

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

(In reply to comment #8)
> > I think the header differs from the other ones, where's the contributor line?
> Yes, I forget to mention it. What do I write here, because I only found things
> from oracle, sun etc.
http://www.mozilla.org/MPL/boilerplate-1.1/


> > You should document the allowed strings, maybe an unsigned long with predefined
> > constants suits, too.
> You have an example of this ? Because i took the same type like for id's etc,
> so I thought that this is the standard :(
We don't really have a standard on that right now. You could document the needed strings as in http://mxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIAttendee.idl#74 or you can use UL constants similar to http://mxr.mozilla.org/mozilla1.8/source/calendar/base/public/calICalendar.idl#568


I'll take a closer look later :-)

Comment 10

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 331407 [details] [diff] [review]
calIDuration - v1

I talked with Daniel about this issue, and we are not sure how to best continue. The question that came to mind, RELATED-TO is just another quite simple property that can have params.

The best solution we came up with is that since there are some more of these properties around it might be better to generalize the API and just allow setting relations similar to this:

var prop = createExtendedProperty();
prop.setParam("RELTYPE", ...);
prop.setParam("X-whatever", "value");
prop.value = relId;

item.addProperty("RELATED-TO", prop);

We should probably even (re-)use calIIcalProperty for this instead.

Possible additions to calIItemBase would include (but are not limited to):
* Add an arbitrary amount of properties of the same name to the
  item (similar to the addProperty above)
* Add toString() to calIIcalProperty, so that we can keep most code that
  calls getProperty() the way it is. 

This would allow getting rid of some interfaces we have around.


On the other hand, we have modeled almost all properties in use as an interface and since most of this code is already written, it probably doesn't make sense to burden you with this task at this point. I'm certain the above should be done in a followup bug, I guess its up to you to decide if you agree and would like to fix that aspect first or not. In any case, above changes are a post-0.9 thing.


Maybe Daniel has some more opinions, feel free to correct me if I misinterpreted or forgot some things.



 
>+
>+        var bagenum = this.mProperties.enumerator;
>+        while (bagenum.hasMoreElements()) {
>+            var iprop = bagenum.getNext()
>+                               .QueryInterface(Components.interfaces.nsIProperty);
>+            icalatt.setParameter(iprop.name, iprop.value);
>+        }
Instead of using the enumerator, you can use the new

var outKeys = {};
var outValues = {};
this.mProperties.getAllProperties(outKeys, outValues);
for (var i = 0; i < outKeys.value.length; i++) {
    icalatt.setParameter(outKeys.value[i], outValues.value[i]);
}

Which doesn't require the magic QueryInterface.


>+        for (var paramname = attProp.getFirstParameterName();
>+             paramname;
>+             paramname = attProp.getNextParameterName()) {
>+            if (paramname == "RELTYPE") {
>+                this.mType = attProp.getParameter("RELTYPE");
>+                continue;
>+            }
>+
>+            this.setProperty(paramname, attProp.getParameter(paramname));
Don't you want this.setParameter() here?


>+    setParameter: function (aName, aValue) {
>+        this.mProperties.setProperty(aName, aValue);
go ahead and do return this.mProperties.setProperty(...), while the property bag doesn't provide for this yet, I think chaining of calls works better if aValue is returned at the end and if we fix that at some point then we won't have as much work.

>+    deleteParameter: function (aName) {
The same goes for deleteParameter()


>         m.mAttachments = this.mAttachments;
> 
>+        m.mRelations = this.mRelations;
>+

I'm not quite sure, but I think at this point we want:

m.mRelations = this.mRelations.concat([]); 

to make sure the new array isn't just a reference to the old array. The same needs to be fixed for mAttachments.

>+            // Could we have the same item as parent and as child ?
>+            if (this.mRelations[attIndex].relId == aRelation.relId) {
Just in case, go ahead and check for the reltype too.

>+function run_test() {
>+    const Cc = Components.classes;
>+    const Ci = Components.interfaces;
I think these two are defined in head_consts.js which is loaded beforehand

>+
>+    var eventClass = Cc["@mozilla.org/calendar/event;1"];
>+    var eventIID = Ci.calIEvent;
>+
>+    var relationClass = Cc["@mozilla.org/calendar/relation;1"];
>+    var relationIID = Ci.calIRelation;
>+
>+    // Create Relation 
>+    var r1 = createRelation();//relationClass.createInstance(relationIID);
>+
>+    // Create Items 
>+    var e1 = eventClass.createInstance(eventIID);
>+    var e2 = eventClass.createInstance(eventIID);
I assume you created a createRelation() helper in calUtils? I must have missed it. Please remove the comments after var r1 = ...

Also, since calUtils.js is loaded for tests, go ahead and use createEvent() to instanciate e1 and e2.


>-    DB_SCHEMA_VERSION: 11,
>+    DB_SCHEMA_VERSION: 12,
Update schema version, it has advanced in the meantime (and will do so for a recent caldav patch, I believe the next number is 14, but please doublecheck)



r- for now to await comments from dbo. In case he agrees that we should probably go ahead and take this for now and you have a new patch with above comments considered, I'll happily r+ that one :-)

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
No, I haven't anything to add, Philipp; you sum'ed it up correctly. Makes sense to continue like you proposed.

Comment 12

[Fred Jendrzejewski]

I'll apply your comments, so I can go ahead for the subtasks.
But I'll try to look how to implement the extended properties, it interests me pretty much. So you can use new properties without new idl's and it becomes easier to create extensions. But we will see.

Comment 13

[Fred Jendrzejewski]

Attachment: calIRelation - v2

> Don't you want this.setParameter() here?
I did it as in the calIAttachment and I didn't grep any usage of this.setParameter in the hole base/src directory don't know what you mean.
> r- for now to await comments from dbo. In case he agrees that we should
> probably go ahead and take this for now and you have a new patch with above
> comments considered, I'll happily r+ that one :-)
I did my best and will look forwards for the api change. Is there already a bug for this one ?

Comment 14

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 333163 [details] [diff] [review]
calIRelation - v2


>             calIRecurrenceRule.idl \
>+	  				 calIRelation.idl \
Use spaces instead of tabs



> * The Original Code is Mozilla Calendar code.
> * 
> * The Initial Developer of the Original Code is
> *   Fred Jendrzejewski <[e-mail]>
> * Portions created by the Initial Developer are Copyright (C) 2008
> * the Initial Developer. All Rights Reserved.
I think we've been doing it like this, I'll double check though.


>+     * SYBLING 
SIBLING

>     calRecurrenceInfo.js \
>+	   calRelation.js \
Spaces instead of tabs


>+            if (this.mRelations[attIndex].relId == aRelation.relId && this.mRelations[attIndex].relType == aRelation.relType) {
wrap into two lines



Other than that it looks good :-)

We might want to examine if the attachment code is correct. While we are using a "property bag", we are using it to store the parameters of the ical property. The setParameter method can be found here: http://mxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIICSService.idl#230

There is no bug yet for the extended properties, just some ideas Daniel and I have been talking about.


I'll be fixing those comments before checkin myself, no need for further action.

r=philipp

Comment 15

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 16

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

There are testcases for this bug (in-testsuite+)

Comment 17

[Martin Schröder [:mschroeder]]

calRelation.js has to be added to packages-static afaik

Comment 18

[seshu.parvataneni]

Hi.

Does this mean that we can save and publish calendars wherever https is supported? Meaning this could be used as an alternate to webdav to securely publish and access ics files? Will this support ability to use username and passwords? Something like publishing the ics files to in yahoo's mail account and then accessing from the mail account.

Comment 19

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Attachment: [checked in] (Bustage) Fixes

Thanks for the reminder about packages-static, also another problem showed up when cloning. Noone is around to review, but I'm checking this in anyway so the next nightly isn't broken. Consider it a bustage fix ;-)

Comment 20

[seshu.parvataneni]

Please ignore my comments in18. I meant to put my comments for 429061. I am not sure how my comments ended up for this bug. Sorry about that.

Comment 21

[Daniel Boelzle [:dbo]]

Comment on attachment 333163 [details] [diff] [review]
calIRelation - v2

I've backed out the calStorageCalendar changes, because they break our efforts on bug 449401. (Even though we already have a nightly build for this monday,) I back it out to prevent once another upgrade code section to correct the changes.
We should be more careful doing schema changes, because those cause a headache on correction.
I suggest we require a second review for schema changes.

Comment 22

[Stefan Sitter [:ssitter]]

(In reply to comment #21)
I suggest that you blog about the changes next time to avoid more bugs like Bug 450256 or Bug 450268 being filed.

Comment 23

[Daniel Boelzle [:dbo]]

I think people forget that they are actually using nightly development versions that may break their data. I'll blog about that and stress that out.

Comment 24

[Fred Jendrzejewski]

There is the end of the test_relation.js missing in the trunk. (the last 9 lines)

Comment 25

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Strange...I'll take a look later on.

Fred, could you update the calStorageCalendar part of your patch to incorporate changes made in bug 449401 ?

Comment 26

[Fred Jendrzejewski]

Attachment: storage updated

I calStorage is loaded but I don't get the test to be updated. I have the complete file in the tree but it stays broken in the obj-directory.

Comment 27

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Comment on attachment 334511 [details] [diff] [review]
storage updated

Looks good, r=philipp

Requesting addl. review from daniel due to storage schema changes.

Comment 28

[Daniel Boelzle [:dbo]]

Comment on attachment 334511 [details] [diff] [review]
storage updated

>+    writeRelations: function (item, olditem) {
>+        var relations = item.getRelations({});
>+        if (relations && relations.length > 0) {
>+            for each (rel in relations) {
for each (var rel ...

r2=dbo

Comment 29

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 30

[Stefan Sitter [:ssitter]]

Comment on attachment 334511 [details] [diff] [review]
storage updated

>-    DB_SCHEMA_VERSION: 13,
>+    DB_SCHEMA_VERSION: 14,

>+        if (oldVersion < 13) {

Shouldn't this be "if (oldVersion < 14) {" to trigger the database update? See Bug 451827.