Closed Bug 322831 Opened 14 years ago Closed 14 years ago
Extra parameters in properties not preserved
If we try to roundtrip something like LOCATION;X-LOCATION-ID=V0firstname.lastname@example.org:Delta Center the X-LOCATION-ID part is lost. It comes out as LOCATION:Delta Center
Right now, all properties are essentially objects implementing nsIVariant. In JS, we're essentially handing around properties as regular objects of various types (both native JS types and JS interface implementors) and letting XPConnect do the conversion to nsIVariant. I think a possible approach to fix this is make it so that property objects that have X- parameters also implement nsIPropertyBag in addition to nsIVariant. Initially we could do this by creating JS wrappers that do this only for properties that arrive from the ICS service with X- params so that they can be preserved. Arbitrary creation/editing/adding of such things could come later. One possible issue with this approach is performance.
(In reply to comment #1) > I think a possible approach to fix this is make it so that property objects > that have X- parameters also implement nsIPropertyBag in addition to > nsIVariant. Wouldn't this approach require us to fix all consumers of properties to QI to get the regular property value? My thought was to keep a simple behind-the-scenes js-object of objects, which we can keep throwing properties+parameters at, something like: propObject[propName][paramName] = paramValue; Then we can add methods to calItemBase AString getPropertyParam(in AString aPropName, in AString aParamName); void setPropertyParam(in AString aPropName, in AString aParamName, in AString aNewValue); This has the advantage of not creating a new interface and not requiring us to change our consumers. Since params will be found/used relatively rarely, I think the perf impact will also be less. Thoughts?
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
No one seemed to object to the js-object proposal, so I've gone ahead and implemented it. As far as I can tell, there's no need to really expose these values through an interface, so simply keeping them around behind the scenes should be sufficient.
Comment on attachment 233174 [details] [diff] [review] using js-objects Index: calendar/base/src/calItemBase.js +dump("checking property:"+prop.propertyName+'\n'); Let's lose the dump before checkin. Otherwise, this appears to work! r=lilmatt
Attachment #233174 - Flags: first-review?(mattwillis) → first-review+
Comment on attachment 233174 [details] [diff] [review] using js-objects An elegant architectural choice because of its simplicity; nice work. A couple of minor things: It is my belief that basic manipulation of stuff descended from calIItemBase is (at least almost) entirely scriptable via any XPIDL accessible language. Given how trivial it is to maintain that invariant, I'd like to propose that we do so. >+ this.mPropertyParams[iprop.name][paramName] In most (all?) uses of the above pattern in this patch, the only dereference (out of the four being made) that is actually interesting to the reader is the very last one (paramName). This suggests that the code would be significantly easier to read with judicious use of JS 1.7's let statement.
Attachment #233174 - Flags: second-review?(dmose) → second-review-
(In reply to comment #6) > This suggests that the code would be significantly > easier to read with judicious use of JS 1.7's let statement. (Without knowing what the JS 1.7's let statement does): We should ensure that the code also works with Lightning in Thunderbird 1.5 aka 1.8.0 branch.
Patch updated to include IDL access to the params and extra variable declarations to avoid too much  notation.
Comment on attachment 235335 [details] [diff] [review] extra vars and idl r=dmose; nice work. Please file another bug for implementing other relevant parameter methods (enumerate, exists(?), set).
Attachment #235335 - Flags: second-review?(dmose) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk. -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.