325103 - list of possible properties in getProperty documentation in calIItemBase erroneous

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Reid Rankin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Sunbird 0.3a

When retreiving properties from an item via getProperty, the "location" property is null, and the "LOCATION" property is the location. The documentation states that the proper property name is location, and this should be fixed.

I suspect that there are other documented properties in lowercase that can only be retreived in uppercase... The real fix of all this should be to make the properties case-insensitive.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Reid Rankin]

Sorry, I didn't notice until just now how the properties are stored in a HashPropertyBag. I suppose we just need better documentation.

Comment 2

[Joey Minta]

From rfc2445 
"  All names of properties, property parameters, enumerated property
   values and property parameter values are case-insensitive. However,
   all other property values are case-sensitive, unless otherwise
   stated."

So, I think this does need to be fixed to make getProperty case-insensitive.  Confirming.

Comment 3

[Reid Rankin]

Attachment: Patch v1

(In reply to comment #1)
> Sorry, I didn't notice until just now how the properties are stored in a
> HashPropertyBag. I suppose we just need better documentation.
> 

I suppose the interfaces to the HashPropertyBag can just be set to convert prop. names to lowercase for indexing purposes, effectively making it case-insensitive.

Adding proposed patch.

Comment 4

[Joey Minta]

Comment on attachment 210023 [details] [diff] [review]
Patch v1

Thanks for the patch!  This looks pretty good.  A couple of things:

-  // 'location' - location (string)
+  // 'LOCATION' - location (string)

Now that we have the toLowerCase(), this change isn't necessary is it?

We should also include a comment in the idl documentation that this is in fact case-insensitive.

-            return this.mProperties.getProperty(aName);
+            return this.mProperties.getProperty(aName.toLowerCase());
         } catch (e) {
             try {
                 if (this.mIsProxy) {
-                    return this.mParentItem.getProperty(aName);
+                    return this.mParentItem.getProperty(aName.toLowerCase());

For performance, it'd probably be better to convert this to lower case once, and use the converted string in both places.

I'm afraid of what effect these changes are going to have on our ics serialization/export/publishing, given that the current code relies on the property bag matching the icalendar properties so closely.  Won't this patch, for example, take
SUMMARY:My event
and turn it into
summary:My event 
when the file is serialized next?  Although I don't think that's technically invalid, almost every other ics file I've seen uses uppercase letters, and we should probably do the same.  (If I'm misunderstanding these effects, can you please say why this problem won't happen when you submit the next version of the patch.)

A good start, though, looking forward to version 2.

Comment 5

[Reid Rankin]

Attachment: Patch v2

This patch is greatly enhanced from the last version. The unneccessary comment edit in calIItemBase.idl is gone, but documentation on the changes to the functions has been added. I found another set of property interfaces in calAttendee.js that needed updating, and fixed them. The performance issue in getProperty has been corrected, and all property names will now be stored in uppercase to workaround the ICS serialization problem. I also fixed the case-sensitivity of isPropertyPromoted and added a comment above itemBasePromotedProps to make sure all future edits use uppercase property names.

I hope this does it!

Comment 6

[Joey Minta]

Comment on attachment 210053 [details] [diff] [review]
Patch v2

Sweet! This looks a lot better.  Did you test a patched version of Sunbird to confirm that it doesn't change our output?

Some nits:
-
+    
+	// This method is case-insensitive.
     isPropertyPromoted: function (name) {
Some ugly indenting/whitespace changes there.

+    
+    // The has/get/getUnproxied/set/deleteProperty methods are case-insensitive
All of these blank lines seem to have extra white space in them, can you get rid of that, please?

+  // If you use the has/get/getUnproxied/set/deleteProperty
+  // methods, property names are case-insensitive.
+  // 
+  // For purposes of serialization, all property names in
+  // the hashbag are in uppercase.
+  //
This should make it clear that we're talking about ics serialization, since there are other ways to use this data that the comment might not apply to.

Please add a similar comment to the calIAttendee idl.

Comment 7

[Reid Rankin]

Attachment: Patch v3

This fixes the whitespace issues, and adds a descriptive comment to calIAttendee.idl explaining the case-insensitivity.

I haven't tested a build with the patch though... I'm just getting into the development community, and I'm used to doing extensions, which are all JS and XUL, and don't require compiliation. I had never used CVS until yesterday, when I was asked to patch a file on #calendar. As such, I have NO IDEA how to build a patched version of SB. (Well, I do, but my dev environment is currently on windoze, and syncing changes and stuff with a linux system would be hell.) If anyone can give me a shove in the right direction, I'd appreciate it!

Thanks in advance!
Reid

Comment 8

[Reid Rankin]

Comment on attachment 210072 [details] [diff] [review]
Patch v3

forgot requestee field

Comment 9

[Joey Minta]

Comment on attachment 210072 [details] [diff] [review]
Patch v3

Everything looks pretty good here.  However, when I try to apply the patch to do further testing, I get: 
patching file calendar/base/public/calIItemBase.idl
patch: **** malformed patch at line 134: Index: calendar/base/public/calIAttendee.idl

Can you look into that?  Thanks.

Comment 10

[Joey Minta]

Reid: ping

See previous comment.

Comment 11

[Reid Rankin]

Sorry, I thought I replied to your last comment, but apparently I didn't log in after submitting the comment.

Anyhow, all I did to build that patch was generate several patches with TortoiseCVS and stick them together. I don't know why the patch was invalid, but I'll look into it (real life has reared its ugly head, but I'll have something to you by the end of the week).

Comment 12

[Joey Minta]

Attachment: un-bitrotted and un-malformed

OK, I spent some time to manually hack the malformed bits of the attached patch and unbitrot this patch.  Reid, can you check to make sure this patch is what you intended it to be?  If so, r=jminta and we'll land as soon as the tree opens after 0.3a2.

Comment 13

[Reid Rankin]

It looks fine to me. Sorry I couldn't fix that myself, but I have minimal experience with patches, and no idea how to apply one.

Anyhow, it looks good, but I don't have the privs to set second-review+ apparently, so if you could do that, jminta, I'd be grateful.

Comment 14

[Joey Minta]

(In reply to comment #13)
> It looks fine to me. Sorry I couldn't fix that myself, but I have minimal
> experience with patches, and no idea how to apply one.
Within your source tree, you should usually be able to do something like:
  patch -p0 < props.diff

Comment 15

[Matthew (lilmatt) Willis]

patch checked in on trunk and MOZILLA_1_8_BRANCH

Comment 16

[Reed Loden [:reed]]

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