358498 - calAttendee::icalProperty bug with rsvp

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Daniel Boelzle [:dbo]]

calIAttendee::rsvp is defined as a boolean whereas calAttendee.js implements that attribute as a string ("TRUE", "FALSE") from the ical property parameter.
When transferring calIAttendee inbetween different js contexts or to native code, that attribute will always be "true".
We have to separate out rsvp from icalAttendeePropMap.

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: fixing rsvp and cloning

- fixed rsvp attribute
- fixed cloning: loss of non-promoted attributes

Comment 2

[Matthew (lilmatt) Willis]

Comment on attachment 250846 [details] [diff] [review]
fixing rsvp and cloning

>+++ mozilla/calendar/base/src/calAttendee.js	2007-01-08 15:16:32.355198400 +0100
>         for (var i in allProps)
>             a[allProps[i]] = this[allProps[i]];
>-        // clone properties!
>+        
This line ^^ has trailing spaces.


>     mIsOrganizer: false,
>     get isOrganizer() { return this.mIsOrganizer; },
>     set isOrganizer(bool) { this.mIsOrganizer = bool; },
> 
>+    get rsvp() { return this.mRsvp == "TRUE"; },
>+    set rsvp(b) { this.mRsvp = (b ? "TRUE" : "FALSE"); },
>+
Not that I'm a big fan of "bool" as a variable name, but rather than using "b" you can either be consistent with isOrganizer above it and use "bool", or use the more recent convention of "aValue" or "aVal".  "b" is especially confusing since we're using "a" elsewhere in this file.


> var makeMemberAttr;
> if (makeMemberAttr) {
>     makeMemberAttr(calAttendee, "mId", null, "id");
>     makeMemberAttr(calAttendee, "mCommonName", null, "commonName");
>-    makeMemberAttr(calAttendee, "mRsvp", null, "rsvp");

One thing to keep in mind:
.rsvp needs to be able to be "TRUE", "FALSE", or not there at all.  For example in METHOD:REPLY messages, there should be no "RSVP=" for the attendee.  Currently, aAttendee.deleteProperty("RSVP") doesn't work, and the workaround has been to create a new attendee, and copy in the data (except .rsvp) from the old attendee.  As tested, this code doesn't change the behaviour (or fix the "can't delete this property" bug), however I just wanted you to know about it as you touch this code.


These are just nits.  r=lilmatt with those.

Comment 3

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

Comment on attachment 250846 [details] [diff] [review]
fixing rsvp and cloning

r2=mvl

Comment 4

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
> One thing to keep in mind:
> .rsvp needs to be able to be "TRUE", "FALSE", or not there at all.  For example
> in METHOD:REPLY messages, there should be no "RSVP=" for the attendee. 
> Currently, aAttendee.deleteProperty("RSVP") doesn't work, and the workaround
> has been to create a new attendee, and copy in the data (except .rsvp) from the
> old attendee.  As tested, this code doesn't change the behaviour (or fix the
> "can't delete this property" bug), however I just wanted you to know about it
> as you touch this code.

So rsvp is effectively a tribool { true, false, undefined } and modelled incorrectly in IDL. We could model it as a string like the other properties. Additionally, we could then implement all props asProperty (-> makeMemberAttr), which enables us to undefine a property using deleteProperty and we could get rid of the special promoted props code in calAttendee.js then, too.
What do you think?

Comment 5

[Matthew (lilmatt) Willis]

(In reply to comment #4)
Given the issues that modeling rsvp as a string addresses, I think I agree with modeling it to a string.

Since we need it to be undefined in some cases, having it as a bool doesn't seem correct to me.

Since I undoubtedly don't have all the answers, I'd like to get input from some or all of dmose, jminta, mvl, and ctalbert, or even shaver if he's around and has an opinion on this.  We should probably talk about this some in today's meeting.

Comment 6

[Matthew (lilmatt) Willis]

So the agreement in the phone meeting was to model it as a string for now, but plan to move it to constants at some later point for better type safety.

Daniel,
Could you make an updated patch reflecting that?

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: changing rsvp to string, basing all attributes on properties, adopting storage calendar

- changing rsvp to string
- removing the promoted props code => all attributes are based on the property bag which makes it possible to delete properties
- adopting storage provider

I have grepped for rsvp and will take care for WCAP, but what about e.g. Google? I will put Philipp on CC.

Comment 8

[Matthew (lilmatt) Willis]

Once we make a property UNpromoted, doesn't that mean we can no longer set/get it using attendee.rsvp and will need to use attendee.getProperty("RSVP");?

If so, I'm not sure if this is exactly what we want to do.

I think we need some architecture discussion, preferrably including jminta, dmose and mvl.

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
> Once we make a property UNpromoted, doesn't that mean we can no longer set/get
> it using attendee.rsvp and will need to use attendee.getProperty("RSVP");?

Matt, you can of course still use the attribute, eg attendee.rsvp = "TRUE".

> If so, I'm not sure if this is exactly what we want to do.
> 
> I think we need some architecture discussion, preferrably including jminta,
> dmose and mvl.

If you like you can of course discuss this, but IMO there is little need.
Only calIAttendee::rsvp's type has changed from boolean to string. The rest of the interface is untouched and can be used like before. (To recap: the cause for this interface change is that rsvp has three states: TRUE|FALSE|<undefined>.)

Comment 10

[Matthew (lilmatt) Willis]

(In reply to comment #9)
> > Once we make a property UNpromoted, doesn't that mean we can no longer set/get
> > it using attendee.rsvp and will need to use attendee.getProperty("RSVP");?
> 
> Matt, you can of course still use the attribute, eg attendee.rsvp = "TRUE".

I just checked this with dmose, and I had checked with jminta earlier.
If you remove rsvp from the promoted properties, you CAN NOT access it like aAttendee.rsvp but you MUST use aAttendee.getProperty("RSVP")

See http://lxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIItemBase.idl#176

I don't want to unpromote ALL the properties.  That's bad times.

> If you like you can of course discuss this, but IMO there is little need.
> Only calIAttendee::rsvp's type has changed from boolean to string. The rest of
> the interface is untouched and can be used like before. (To recap: the cause
> for this interface change is that rsvp has three states:
> TRUE|FALSE|<undefined>.)

Changing rsvp to a string isn't my largest concern.  My concern is unpromoting it.


Maybe I'm misunderstanding. What do _you_ think "promotion" means, and how do you think it works?  Why would removing these properties from being promoted NOT break the foo.property usage?  I think answering that will help us figure out where we're confused.

Comment 11

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> (In reply to comment #9)
> > > Once we make a property UNpromoted, doesn't that mean we can no longer set/get
> > > it using attendee.rsvp and will need to use attendee.getProperty("RSVP");?
> > 
> > Matt, you can of course still use the attribute, eg attendee.rsvp = "TRUE".
> 
> I just checked this with dmose, and I had checked with jminta earlier.
> If you remove rsvp from the promoted properties, you CAN NOT access it like
> aAttendee.rsvp but you MUST use aAttendee.getProperty("RSVP")
> 
> See
> http://lxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIItemBase.idl#176

Matt, please have a look at the patch. What I have stated in comment #4 (and what might have been misleading in comment #7) was:
"we could get rid of the special promoted props code in calAttendee.js", stressing "special" here. I.e. code from line 125 and following incl the icalAttendeePropMap. We could get rid of that bunch of code by just implementing all attributes property-based using plain makeMemberAttr(asProperty=true), assuming rsvp is modelled as STRING. The patch effectively reduces the code, it has more '-' than '+'es.
Of course all that props are still promoted to attributes the same sense as in calIItemBase. I hope this clarifies.

Comment 12

[Matthew (lilmatt) Willis]

Comment on attachment 251282 [details] [diff] [review]
changing rsvp to string, basing all attributes on properties, adopting storage calendar

After much discussion and contemplation, I think moving all these to the property bag is the wrong way to go.  We already have documented performance problems with the property bag.  I have a patch that solves the issue differently, without adding stuff to the property bag.

Comment 13

[Matthew (lilmatt) Willis]

Attachment: rsvp as int, using constants

This is my version. jminta was working with me on some slick one using a propmap and stuff, but it's not working yet and I didn't want to lose this one.

Comment 14

[Daniel Boelzle [:dbo]]

(In reply to comment #12)
> (From update of attachment 251282 [details] [diff] [review])
> After much discussion and contemplation, I think moving all these to the
> property bag is the wrong way to go.  We already have documented performance
> problems with the property bag.  I have a patch that solves the issue
> differently, without adding stuff to the property bag.

1. AFAIK and remember, Joey's performance improvements were related to lazy reading of properties in the storage calendar. I don't remember any documented facts explicitly stating that the hash property bag causes performance trouble.
Even if, I wonder why you don't attack that property bag implementation itself, identify whether the bottleneck is the language boundary or whatever... and try to improve it: items (which have by far the most properties) would benefit from that, too.

2. I can hardly understand why you are making the calAttendee code more obscure, adding more banana code and conditionals for rsvp. Reading the IRC chat between you and Joey, you clearly outlined the impression that the existing item and attendee code is hard to understand. And I feel the same: it took me some debugging lessons to get into it.

3. IMO we should clearly stick to rfc2445, the data properties and parameters modeled there can most often be mapped directly to IDL attributes. So why don't we do that where possible?

4. I feel kind of ignored and locked out from your IRC discussions and decisions. IRC isn't everybody's darling, and I live in a different timezone than you.
It would at least help a lot to bring up arguments in the bug/mail/newsgroup again for another round rather than just stating "we had discussion, your patch is denied". This would increase acceptance of the review process and doesn't leave a bad taste.

5.
[03:50]	<jminta>	promoted/unpromoted is a question of mVar vs. property bag
[03:50]	<jminta>	while asProperty is the .foo vs getProperty("foo")
[03:53]	<jminta>	wait, that second part is false
[03:53]	<lilmatt>	ya. the 2nd part should be attribute
[03:54]	<jminta>	yeah, makeMemberAttr always gives you .foo
[03:55]	<jminta>	asProperty just protects it from also being accessed as .getProperty()
[03:55]	<jminta>	so that's another objection i might make

What's wrong about attendee.role vs. attendee.getProperty("ROLE") ?
Please elaborate.

Comment 15

[Joey Minta]

(In reply to comment #14)
See bug 362762 for the perf problems with the property bag.

Comment 16

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> (In reply to comment #14)
> See bug 362762 for the perf problems with the property bag.

IMO comment #3 of that bug shows up that the language barrier seems to be the problem, not the property-bag itself:

"As suggested by timeless, I modified the testcase slightly, to be:
      var prop = e.getNext();
      var val = prop.nsIProperty.value;
Now the loop is twice as fast, and stays fast. So the bug now doesn't show
anymore."

Thus I don't see any urging reason to avoid using a hash-property-bag with the mentioned tweak (if still needed at all).
Moreover I doubt that using it in calAttendee causes any performance trouble until proven. That has been stated in your IRC discussion.

Comment 17

[Daniel Boelzle [:dbo]]

Just to finalize my dedication to this bug:

In the phone call 10-01-2007 (<http://wiki.mozilla.org/Calendar:Status_Meetings:2007-01-10>), we agreed to model rsvp as STRING for short term, then later revise the whole calIAttendee interface to use constants (for rsvp, role, partStat, userType), preserving integrity.

However, if you and Joey start doing it for rsvp now (which is IMO ok), either with 
- the attached patch
- or Joey's <http://pastebin.mozilla.org/3624>,
I would greatly appreciate if we could avoid doing senseless work in the future. 

Moreover, IMO you and Joey still owe me some valid arguments about the hash-property-bag performance and comment #14, 5).

Comment 18

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

We agreed on the phone to use strings for now. But then the updated patch also does a lot more. Then another patch was submitted, doing the extra stuff from patch 2 differently.
In this bug, we should only try to fix just this bug, not a ton of other things. Then you get what we have now: nothing really gets done.

(about the hash-property performance problem: that's because the hash property bag itself is c++, while the caller (itemBase) is JS. So you see it anyway, from whatever language you call itemBase.)

Comment 19

[Daniel Boelzle [:dbo]]

(In reply to comment #18)
Michiel, I am really astonished what you call "a ton of other things". All submitted patches are IMO no big deal.
The foremost benefit of the second revised patch was that it actually makes code a lot easier, *reducing* the file.
Of course, we can continue fixing just little nits here and there trying not to touch anything, but making the overall code quality worse and worse. Just have a look at calRecurrenceInfo, especially calculateDates(). I can imagine how that monster got to life...

I am more than often wondering why this project doesn't get agile and actually more liberal to changes. This project is several years old, but yet full of bugs. Often, not even Joey or you can overlook the current code or foresee the effect of changes (e.g. referring to the "proxy/exception" irc chat with Philipp).

But what's most frustrating to me is that there are lots of good fixes attached to bugs without getting in. Even if a patch makes a whole code area a lot better and fixes almost 80% of the sketched bug, it is often denied. I can assure you, I am not the only one that has fixed some code, not knowing that there has already been a fix attached to some bug.

From more than 8 years programming on OpenOffice/StarOffice (which has multi million lines of code back to the late 80ies), I can only recommend to suspend or rewrite code if it is obscure/crippled or too coupled, not well modularized.

Comment 20

[Daniel Boelzle [:dbo]]

(In reply to comment #18)
> (about the hash-property performance problem: that's because the hash property

I was expecting the language barrier being the problem, not the hash-property-bag implementation and I stated that.

My point is that nobody has showed up that the hash-property-bag usage in calAttendee is a real bottleneck; I agree that it is for items.
Even if it is, why not either
- switch over the core code (items, attendees, recurrence-info) to C++
- or implement a property-bag in javascript
?

Comment 21

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

My point about fixing other things in this patch is that it is supposed to be a temporary patch. So it makes sense to me to make it as small as possible and get it finished as soon as possible.
The review comments are all about thing that don't effect the part of the patch that actually the bug.

Comment 22

[Daniel Boelzle [:dbo]]

Nominating for 0.5 to get "fixing rsvp and cloning" in; we should reopen this bug or spin off a new one for the rsvp API change.

Comment 23

[Daniel Boelzle [:dbo]]

"fixing rsvp and cloning" checked in with lilmatt's nits minded on both HEAD and MOZILLA_1_8_BRANCH. The patch fixes the current implementation, although the API most likely will change in the future; bug stays open to address that.

Comment 24

[Daniel Boelzle [:dbo]]

Attachment: modeling rsvp as string

This patch models rsvp as a string that might be null/void.

Comment 25

[Daniel Boelzle [:dbo]]

BTW: the unit tests have passed (except for test_recur.js).

Comment 26

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

Comment on attachment 345305 [details] [diff] [review]
modeling rsvp as string

>-        this.mProperties.setProperty(aName.toUpperCase(), aValue);
>+        if (aValue) {
>+            this.mProperties.setProperty(aName.toUpperCase(), aValue);
>+        } else {
>+            this.mProperties.deleteProperty(aName.toUpperCase());
>+        }

Won't this cause the same problems we've had with aValue = 0 ? Please solve similar to calIItemBase.

Comment 27

[Daniel Boelzle [:dbo]]

Fixed the number issue for calAttendee, too.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/252720b87e3f>

-> FIXED

Comment 28

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".