350
edits
Calendar:Architecture: Difference between revisions
Jump to navigation
Jump to search
answeres to jminta
m (→Big Picture) |
(answeres to jminta) |
||
| Line 129: | Line 129: | ||
*A concurrent transaction has written the same item attribute | *A concurrent transaction has written the same item attribute | ||
*A concurrent transaction has written a different item attribute, but the item's constraints, e.g. <code>{ DTSTART <= DTEND }</code>, would be violated. | *A concurrent transaction has written a different item attribute, but the item's constraints, e.g. <code>{ DTSTART <= DTEND }</code>, would be violated. | ||
===Undo/Redo=== | |||
The previous approach can handle undo/redo easily because it keeps old items and jump back to a previous generation if requested. Since with the transaction system in place we don't provide copies of the items this approach is no longer possible. But undo/redo is essentially nothing more than a history log of changes, which fits into the transaction mechanism. | |||
Upon <code>commit()</code>, the transaction system needs to store the last value of an attribute to be modified, before before committing the new one. That way, we could implement a rollback mechanism for items. Because each <code>commit()</code> transits the item from one consistent state into another, this won't violate data integrity. However, the number possible rollbacks needs to be configurable. xxx todo: where? '''jminta:''' probably use nsITransactionManager::maxTransactionCount | |||
===Notifications=== | |||
It becomes obvious that the current notification scheme cannot be supported anymore, i.e. <code>onModifyItem()</code>, because there is no old item anymore. | |||
Easing the oberver implementations, it makes sense to provide information about what attributes have changed. Then, the observing instance does not need to update all reflected information. This perfectly embraces the history log described [[#Undo.2FRedo|above]]. The new <code>onModifyItem()</code> would look like this: | |||
// objects of this can be queried for either calIEvent/calITodo: | |||
interface calICommittedTransaction : nsISupports { | |||
void getModifiedAttributes( | |||
out PRUint32 count, [array, size_is(count), retval] out string keys ); | |||
}; | |||
void onModifyItem( in calIItemBase item, in calICommittedTransaction diff ); | |||
===Implementation Approach=== | ===Implementation Approach=== | ||
| Line 135: | Line 149: | ||
Transaction objects will implement the <code>calIItemBase</code> interface, they'll appear as regular items to clients, internally they'll just record what attributes have been set, and are to be modified. Upon construction transaction, objects are basically ''empty'', they don't modify the item upon <code>commit()</code>. Clients that are setting attributes will flesh out the transaction object which actual content; ''which'' attributes are to be modified, no more, no less. Answere to "What are the actual modifications (the difference)?". | Transaction objects will implement the <code>calIItemBase</code> interface, they'll appear as regular items to clients, internally they'll just record what attributes have been set, and are to be modified. Upon construction transaction, objects are basically ''empty'', they don't modify the item upon <code>commit()</code>. Clients that are setting attributes will flesh out the transaction object which actual content; ''which'' attributes are to be modified, no more, no less. Answere to "What are the actual modifications (the difference)?". | ||
Providers need to notify observers in commit order. Being thread-safe, this would require a notification queue run by a seperate thread. | |||
===Pros/Cons=== | ===Pros/Cons=== | ||
| Line 141: | Line 157: | ||
The necessary interface changes can be kept to a minimum. Basically we need to get rid of <code>modifyItem()</code> and <code>adoptItem()</code> in <code>calICalendar</code>, adding the aboce sketched: | The necessary interface changes can be kept to a minimum. Basically we need to get rid of <code>modifyItem()</code> and <code>adoptItem()</code> in <code>calICalendar</code>, adding the aboce sketched: | ||
calIItemBase getTransaction( | calIItemBase getTransaction( in calIItemBase item ); | ||
commit( | void commit( in calIItemBase item ); | ||
Additionally, <code>onModifyItem()</code> would only take one parameter, i.e. the item. Cloning should be removed from the current <code>calIItemBase</code>, because it conflicts with this new mimic. xxx todo, what more? | Additionally, <code>onModifyItem()</code> would only take one parameter, i.e. the item. Cloning should be removed from the current <code>calIItemBase</code>, because it conflicts with this new mimic. xxx todo, what more? | ||
| Line 149: | Line 165: | ||
The sketched transaction system will elegantly solve the disadvantages of the original architecture with a minimum of changes. Error handling remains ''as is'', in case of a commit conflict. | The sketched transaction system will elegantly solve the disadvantages of the original architecture with a minimum of changes. Error handling remains ''as is'', in case of a commit conflict. | ||
== Comments are welcome == | == Comments are welcome == | ||
| Line 161: | Line 174: | ||
* 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: | * 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: | ||
** 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. | ** 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. | ||
** 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. | ** 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]].''' | ||
* 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") | ** 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''.''' | ||
** 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. | ** 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?''' | ||
** 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? | ** 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.''' | ||
* 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. | * 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. | ** 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.''' | ||
* 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. | * 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.''' | ||
* 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. | * 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.''' | ||
* 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. | * 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.''' | ||
* 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>. | * 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.''' | ||
* 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. | * 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.''' | ||
==== Big Picture ==== | ==== Big Picture ==== | ||
| Line 193: | Line 206: | ||
| Method of listener to solve disadvantage || Enumerate occurrences || Enumerate properties | | Method of listener to solve disadvantage || Enumerate occurrences || Enumerate properties | ||
|} | |} | ||
'''Re: We think you miss our point that transaction '''vanish''' after being committed.''' | |||
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. | 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. | ||