Last Comment Bug 322831 - Extra parameters in properties not preserved
: Extra parameters in properties not preserved
Status: RESOLVED FIXED
[swag: 1d][patch in hand]
: dataloss
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Joey Minta
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-09 10:04 PST by Joey Minta
Modified: 2006-08-31 06:08 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
using js-objects (4.83 KB, patch)
2006-08-10 16:31 PDT, Joey Minta
mattwillis: first‑review+
dmose: second‑review-
Details | Diff | Review
extra vars and idl (7.24 KB, patch)
2006-08-24 17:23 PDT, Joey Minta
jminta: first‑review+
dmose: second‑review+
Details | Diff | Review

Description Joey Minta 2006-01-09 10:04:04 PST
If we try to roundtrip something like
LOCATION;X-LOCATION-ID=V0-001-000102689-1@evdb.com:Delta Center

the X-LOCATION-ID part is lost.  It comes out as
LOCATION:Delta Center
Comment 1 Dan Mosedale (:dmose) 2006-01-10 11:56:27 PST
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.
Comment 2 Joey Minta 2006-06-13 17:56:25 PDT
(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?

Comment 3 Reed Loden [:reed] (use needinfo?) 2006-07-19 21:13:54 PDT
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Comment 4 Joey Minta 2006-08-10 16:31:20 PDT
Created attachment 233174 [details] [diff] [review]
using js-objects

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 5 Matthew (lilmatt) Willis 2006-08-10 18:02:32 PDT
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
Comment 6 Dan Mosedale (:dmose) 2006-08-22 19:43:38 PDT
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.
Comment 7 Stefan Sitter 2006-08-23 00:13:55 PDT
(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.
Comment 8 Joey Minta 2006-08-24 17:23:46 PDT
Created attachment 235335 [details] [diff] [review]
extra vars and idl

Patch updated to include IDL access to the params and extra variable declarations to avoid too much [] notation.
Comment 9 Dan Mosedale (:dmose) 2006-08-25 18:35:08 PDT
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).
Comment 10 Matthew (lilmatt) Willis 2006-08-31 06:08:48 PDT
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Note You need to log in before you can comment on or make changes to this bug.