Calendar:Architecture: Difference between revisions

Jump to navigation Jump to search

Calendar:Architecture (view source)

Revision as of 14:58, 20 February 2006

3,549 bytes removed ,  20 February 2006
→‎jminta's first-glance comments
No edit summary
Line 168: Line 168:


=== jminta's first-glance comments ===
=== jminta's first-glance comments ===
Let me begin by saying that I don't believe any of the issues I'm going to comment on are unsolvable, but for the most part, they merely reflect areas that I am unclear on after a first reading of this proposal.  Clarification on proposed solutions to these issues would be helpful, in my mind.
I've removed the majority of my old comments, because, for the most part, they have been cleared up by further explanation of the notification system.  The big issue I was misunderstanding here was what was being passed to onModifyItem.  Since onModifyItem gets a transaction-object that contains all of the old-field values that changed as well as the new object, it has enough information to handle the cases I was concerned about.


* My biggest concern is that there are certain instances where transaction information is not meaningful without having knowledge of the original object.  The current implementation avoids the problem by giving listeners both the original and new objects, which they can compare in any way they see fit. Consider the following (somewhat artificial) scenario:
==== New issues ====
** A user is currently viewing today in the Day View.  Via the agenda-tree, unifinder, or some other method, the user edits an event currently schedule for 4pm-5pm tomorrow.  He modifies the event to begin at 4pm '''yesterday'''.  Now we have a transaction that contains only a new startDate.  The Day View, upon receiving this transaction, has no way of knowing whether or not such changes affect it, because it has no knowledge of the original event, and hence, no knowledge of the start time.
I've had some more time to ruminate on this model, now that I have a better understanding of the notification system.  Some new concerns that come to mind.
** A more concrete example might be the alarmService, which only cares about changes to objects with alarm information.  If a user changes the start-time, the service will need to retrieve the object to learn if it had an alarm on it, and then determine whether that alarm is affected by the change. '''Re: If we understand your concern correctly, you are talking about how to find out differences between the old and the new item.  That's why we have just added a proposal about a different [[#Notifications|Notification]].''' '''jminta says:''' My general concern isn't just notifications (although this section helps a lot), but rather that a set of modified attributes doesn't always tell the whole story.  In simple terms: The day-view always needs to know start *and* end in order to make a decision on whether it should update.  The alarmService needs to know start *and* alarmOffset.  Transactions oftentimes will only provide 1 of these pieces of information, and it isn't clear how to obtain the other. '''Re: Can't see a problem here. The notification { item, change-set } gives the observer all information it needs. The (already committed, up-to-date) item, and the set of attributes which have changed. If the alarm service needs the alarm-offset, read it out.  The item is up-to-date. This also relates to one of your concerns later in this article about the event dialog: You need not use the change-set (i.e. <code>calICommittedTransaction</code>.  You can always right away read out all item attributes, even only a single one has changed. But the change-set can be used to optimize this. And, to assure that the change-set is actually correct in some point in time, the providers need to assure that notifications are sent in order they have been committed.'''
* '''Shared calendars''' - This system works fairly well for changes that a user makes within a system.  However, when an outside user changes a shared calendar, coming up with a calICommittedTransaction for that set of changes is highly non-trivial.  In fact, it's likely to be even more expensive than the 'deep-copy' that clone() requires now, because what we now need is a 'deep-comparison'.  For enterprise calendaring, this may have a performance impact.
* Each calICalendar, under this model, is going to be required to construct one and only one calIItemBase for each item in its store, as requested. Therefore, when subsequent getItems calls are made, it is going to have to consult its list of previous dispatch items, to determine whether it was previously constructed.  Not only could this search be a performance hit, but it keeping arrays/hash-tables of objects sent out could impact memory-usage.


==== Old issues ====
This concern is still valid, I believe.  It concerns issues with the actual editing transaction, not the commitedTransaction.  Trying to delete properties and objects from an empty calIItemBase (as calITransaction is defined to be) will be problematic.  Some examples, and the previous discussion concerning them follow:
* The variety of transactions that can take place is extremely broad, and it is not clear that all such changes can be easily represented.  Some types of changes the may prove difficult to embody in a transaction:
* The variety of transactions that can take place is extremely broad, and it is not clear that all such changes can be easily represented.  Some types of changes the may prove difficult to embody in a transaction:
** deleteProperty("X-PROP1"). '''Re: The granularity of changes are the attributes of an item.  If ical properties have been changed in a transaction, all depending attributes have to be marked as ''modified''. If you reinitiliaze an item using the <code>icalString</code> attribute, the whole item, i.e. all of its attributes are marked as being ''modified''.''' '''jminta says:''' I don't think I was clear here.  My issue is that if a calITransaction is empty, you can't simply call deleteProperty("FOO") on it (like you could on a clone), because it doesn't have the property.  Furthermore, how would you distinguish between a property that has been deleted and a property whose value is simply blank?
** deleteProperty("X-PROP1"). '''Re: The granularity of changes are the attributes of an item.  If ical properties have been changed in a transaction, all depending attributes have to be marked as ''modified''. If you reinitiliaze an item using the <code>icalString</code> attribute, the whole item, i.e. all of its attributes are marked as being ''modified''.''' '''jminta says:''' I don't think I was clear here.  My issue is that if a calITransaction is empty, you can't simply call deleteProperty("FOO") on it (like you could on a clone), because it doesn't have the property.  Furthermore, how would you distinguish between a property that has been deleted and a property whose value is simply blank?
** changing an event into a task - would the transaction implement calIToDo or calIEvent?  This requires that certain code areas that only care about one of these still need to watch other transactions. '''Re: Why would someone ever want to switch an event into a task or vice versa?''' '''jminta says:''' it's actually a pretty common request, bug 277731.
** changing an event into a task - would the transaction implement calIToDo or calIEvent?  This requires that certain code areas that only care about one of these still need to watch other transactions. '''Re: Why would someone ever want to switch an event into a task or vice versa?''' '''jminta says:''' it's actually a pretty common request, bug 277731.
** changing the calendar of an item - could be handled with add+delete, but really, only the item's .calendar property is changing, which makes it seem like a commitTransaction() would work.
** changing the calendar of an item - could be handled with add+delete, but really, only the item's .calendar property is changing, which makes it seem like a commitTransaction() would work.
** adding an attendee - If a transaction contains a single attendee, how can a listener determine whether this is a modifcation of a previous attendee or an addition of a new one? '''Re: As stated above, we currently think it is sufficient, that the granularity is about attributes of an item. In this case, you would receive two different sets of attendees and have to diff out by yourself; well, as it is now.''' '''jminta says:''' Right now you don't have to diff them out, you can just ignore the old set and use the new one, since it's assured to be complete.  Diffing them out is much harder. '''Re: Since the item is up-to-date, you can do so with the new approach, ignoring the change-set.'''
* I'm not entirely clear how this new model would change (if at all) our current occurrences model.  As I understand the proposal, if I want to modify a single occurrence, I would ask for a transaction ticket on that occurrence.  Committing that transaction would require the item's calendar to properly update the parentItem so it contains a proper set of occurrences. '''Re: Yes.'''
** Problematic(?) case: 'Modifications of all future occurrences' has recieved some attention lately.  If I want to perform this type of change, it seems I would need a ticket from the parentItem?  The semantics seem rather vague here regarding from whom I should ask for the ticket. '''Re: From our understanding of the ical standard, 4.2.13 says that this has to be specified as a property parameter of the <code>RECURRENCE-ID</code>. So the obvious approach would be to add this feature to proxy items.''' '''jminta says:''' maybe we need to leave recurrence for another discussion :-/
* It is my opinion that the second cloning in modifyItem is a bug that we can probably fix by enforcing that only mutable items be passed in to modifyItem.  (I don't think anyone ever even tries to pass an an immutable one.)  In that case, we have the same issue in both models, one XPCOM object is required to be created for each change. '''Re: Since cloning items is a deep copy (remember to clone all exceptions etc.), this is not true. And moreover, the new approach does not require ''any'' cloning. It just creates a cheap transaction object.''' '''jminta says:''' OK, this makes some sense.


Keeping this around, just because it's a point I want to emphasize.
* The listener's currently passed in for making modifications, at least in theory, ought to be told when errors occur and what kind of error it was.  This is advantageous, in my opinion, and if this new model were to be adopted, I would like to see commitTransaction accept a listener argument as well. '''Re: Agreed, has been omitted for brevity.''' '''jminta says:''' OK, as long as it's there in the final form.
* The listener's currently passed in for making modifications, at least in theory, ought to be told when errors occur and what kind of error it was.  This is advantageous, in my opinion, and if this new model were to be adopted, I would like to see commitTransaction accept a listener argument as well. '''Re: Agreed, has been omitted for brevity.''' '''jminta says:''' OK, as long as it's there in the final form.


This may be related to the first new point.  In both cases, we're dealing with a set of largely unknown changes to an event, and trying to then diff them out to create a calITransaction.
* This model puts a lot of stress on the event-dialog.  It seems to need to figure out exactly what changed, rather than being able to simply rebuild the object from scratch and re-submit it. '''Re: Where is the difference to the current model? Currently we are creating and writing a new event reading out the whole dialog, and with the new proposal, there would be no code change; just using a transaction object. But if we like, we need only commit changes to UI elements that have actually changed. Being efficient for the first time.'''  '''jminta says:''' My point was that I don't see how you can *not* only submit the changed fields.  Figuring out which fields changed isn't trivial. '''Re: The  attributes that have changed are derived from the actual transaction that has been committed, which is easy. When committing, the provider has to backup those attributes and construct a change-set for <code>onModifyItem</code>'''
* This model puts a lot of stress on the event-dialog.  It seems to need to figure out exactly what changed, rather than being able to simply rebuild the object from scratch and re-submit it. '''Re: Where is the difference to the current model? Currently we are creating and writing a new event reading out the whole dialog, and with the new proposal, there would be no code change; just using a transaction object. But if we like, we need only commit changes to UI elements that have actually changed. Being efficient for the first time.'''  '''jminta says:''' My point was that I don't see how you can *not* only submit the changed fields.  Figuring out which fields changed isn't trivial. '''Re: The  attributes that have changed are derived from the actual transaction that has been committed, which is easy. When committing, the provider has to backup those attributes and construct a change-set for <code>onModifyItem</code>'''


Keeping this around because I think it's very important.  It may also not be as trivial as it sounds, given that enforcing permissions such that the provider may change objects but other code may not is not exactly easy.
* This model needs to be '''absolutely certain''' that it has iron-clad enforcement of the rule that it's impossible to change an item other than by a transaction.  Otherwise, I'm very scared of how easily a novice extension writer could completely break this model by following the intuitive idea of coding <code>item.title = "new title"</code>. '''Re: Items will be immutable in the new approach. So this is no problem.''' '''jminta says:''' Good.  Just be certain.
* This model needs to be '''absolutely certain''' that it has iron-clad enforcement of the rule that it's impossible to change an item other than by a transaction.  Otherwise, I'm very scared of how easily a novice extension writer could completely break this model by following the intuitive idea of coding <code>item.title = "new title"</code>. '''Re: Items will be immutable in the new approach. So this is no problem.''' '''jminta says:''' Good.  Just be certain.


* Finally, <code>calITransaction</code> will run into problems in truly implementing <code>calIItemBase</code>.  Certain properties of <code>calIItemBase</code> depend on certain other properties being present.  In the atomic model, this is not assured.  The most critical example of this is <code>icalString</code>.  Currently, the ics provider can be certain that it has all the information about an event if it calls icalString.  However, calling icalString on a transaction will likely throw an error.  Providers will need to be very smart in order to assure that they have all of the changes truly included in a transaction. '''Re: There seems to be a misunderstanding. A transaction object is not supposed to be passed around, it usually lives only for short time, minimizing the risk to fail upon <code>commitTransaction()</code> (becoming inconsistent). After a transaction object has been committed, it must not be used anymore, leading to an error.''' '''jminta says:''' My point was merely that you can't truly implement an interface if you don't support everything that interface does, ie. <code>icalString</code> for <code>calIItemBase</code> or <code>duration</code> for <code>calIEvent</code>.  If you implement an interface, consumers ought to be able to count on being able to use the properties of that interface. '''Re: Yes, and IMO the notification approach can somehow improved, i.e. <code>onModifyItem( upToDateItem, commitedChangeSet )</code>. The observer needs know what attributes have changed, and the old values. Retrieving old values from the change-set object, the observer can only call for attributes marked as ''modified'' (&rarr;<code>getModifiedAttributes()</code>), calling on <code>calIEvent/calITodo</code> in a type-safe manner. Calling on any other attribute leads to an exception. An alternative approach would be some kind of <code>nsIProperties</code>, which IMO doesn't do this job any better. Opinions?'''
Still not clear on this one:
 
* Finally, <code>calITransaction</code> will run into problems in truly implementing <code>calIItemBase</code>.  Certain properties of <code>calIItemBase</code> depend on certain other properties being present.  In the atomic model, this is not assured.  The most critical example of this is <code>icalString</code>.  Currently, the ics provider can be certain that it has all the information about an event if it calls icalString.  However, calling icalString on a transaction will likely throw an error.  Providers will need to be very smart in order to assure that they have all of the changes truly included in a transaction. '''Re: There seems to be a misunderstanding. A transaction object is not supposed to be passed around, it usually lives only for short time, minimizing the risk to fail upon <code>commitTransaction()</code> (becoming inconsistent). After a transaction object has been committed, it must not be used anymore, leading to an error.''' '''jminta says:''' My point was merely that you can't truly implement an interface if you don't support everything that interface does, ie. <code>icalString</code> for <code>calIItemBase</code> or <code>duration</code> for <code>calIEvent</code>.  If you implement an interface, consumers ought to be able to count on being able to use the properties of that interface. '''Re: Yes, and IMO the notification approach can somehow improved, i.e. <code>onModifyItem( upToDateItem, commitedChangeSet )</code>. The observer needs know what attributes have changed, and the old values. Retrieving old values from the change-set object, the observer can only call for attributes marked as ''modified'' (&rarr;<code>getModifiedAttributes()</code>), calling on <code>calIEvent/calITodo</code> in a type-safe manner. Calling on any other attribute leads to an exception. An alternative approach would be some kind of <code>nsIProperties</code>, which IMO doesn't do this job any better. Opinions?''' '''jminta says:''' - I'm not sure. This could be related to the first old-issue, in that both deal with how to express these changes in the most coherent manner.  My point is that calIItemBase may not be the ideal solution. nsIProperties may be possible, if it can also accept XPCOM objects as arguments, since sometimes the changed property will be an additional attendee, etc. (I don't know enough about nsIProperties to really evaluate this issue.)
==== Big Picture ====
To me, the main difference seems to be able to be summarized in the following table.  If this is an incomplete or unfair representation of either model, please let me know.
{| border="1" cellpadding="5"
| Modifying item || cloned object || calITransaction
|-
| Modified object contains all item properties? || Yes || No
|-
| Disadvantage || Stale references are possible || Difficult to ensure that all changes have been extracted from transaction
|-
| Method of listener to solve disadvantage || Enumerate occurrences || Enumerate properties
|-
| Function to call || getOccurrencesBetween() || getModifiedAttributes()
|}
 
'''Re: We think you miss our point that transaction '''vanish''' after being committed.'''
 
'''jminta says:''' This table is written from the point of view of an observer receiving a notification of a change, in which case the transaction will still be around. '''Re: No, calling on a committed transaction object leads to an exception, and is bug.'''
 
In short, both methods involve some type of enumeration.  What we need to examine, in my mind, is which enumeration is easier from a coding/performance perspective.


I'd like to thank everyone who worked on this model.  There are some good ideas here, and regardless of whether this model is adopted or not, I think discussion of the proposals here will signifcantly benefit calendar development.
I'd like to thank everyone who worked on this model.  There are some good ideas here, and regardless of whether this model is adopted or not, I think discussion of the proposals here will signifcantly benefit calendar development.


=== meta comments from mvl ===
=== meta comments from mvl ===
Jminta
289

edits

Retrieved from "https://wiki.mozilla.org/Special:MobileDiff/17893"

Navigation menu