734817 - Add a difference engine for processing items

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v1

This patch implements a simple form of a difference engine. Plug in an array of items, pass a second array and it will tell you what has been added, removed or modified.

The patch is work in progress and this could be used for bug 728759 and bug 466742.

Comment 1

[Matthew Mecca [:mmecca]]

Comment on attachment 604856 [details] [diff] [review]
WiP - v1

Review of attachment 604856 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't do any testing in the calendar views, but it seems to work well with the patch in Bug 728759. I tested completing a single occurrence of a repeating task with 100 visible occurrences, and got about a 10% performance increase over the first patch in Bug 728759 using the difference engine.

A few comments, otherwise r=mmecca on the itemDiff part.

::: calendar/base/modules/calItemUtils.jsm
@@ +84,5 @@
> +     *
> +     * @param item      The item to load
> +     */
> +    load1: function load1(item) {
> +        this._expectState(this.STATE_INITIAL | this.STATE_LOADING);

There shouldn't be a need to call _expectState or to update this.state for every item when we use load() on an array. We could skip this if we do all the work in the load() function, and have load1(item) just call load([item]). I think we could even get rid of the load1 function altogether, and just use load([item]) in the calling code if we need to pass a single item, I'll leave that up to you. The same applies to the difference / difference1 functions.

@@ +142,5 @@
> +    },
> +
> +
> +    /** @return a hash with |item.id -> item| containing modified items */
> +    get modifiedItems() {

this actually returns |item.id -> [oldItem, item]|, which I think is fine as long as we update the function comment.

Comment 2

[Matthew Mecca [:mmecca]]

Attachment: Fix v1

Updated patch with my comments, I'll ask review for my changes (and please check if I did the user and commit message right).

Comment 3

[Matthew Mecca [:mmecca]]

(In reply to Matthew Mecca [:mmecca] from comment #1)
> > +    /** @return a hash with |item.id -> item| containing modified items */
> > +    get modifiedItems() {
> 
> this actually returns |item.id -> [oldItem, item]|, which I think is fine as
> long as we update the function comment.

I take that back, I think this will be easier to work with if we use a separate function to return the old versions of the modified items.

Also I think it would be more convenient if the items were returned in an array, so a HashedArray would be better return type.

Comment 4

[Matthew Mecca [:mmecca]]

Attachment: Fix v2

Separates the old and new versions of modified items. Returns items in a HashedArray.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 614615 [details] [diff] [review]
Fix v2

Review of attachment 614615 [details] [diff] [review]:
-----------------------------------------------------------------

> There shouldn't be a need to call _expectState or to update this.state for
> every item when we use load() on an array. We could skip this if we do all
> the work in the load() function, and have load1(item) just call
> load([item]). I think we could even get rid of the load1 function
> altogether, and just use load([item]) in the calling code if we need to pass
> a single item, I'll leave that up to you. The same applies to the difference
> / difference1 functions.

The main reason I added both methods is that using load([item]) creates an extra array which is thrown away afterwards, which I wanted to avoid. On the other hand, I understand your concern that calling expectState each time isn't the right thing to do either.

Lets go with doing the work in the load function and having load1 call load([item]), as you have already done in the patch. Calling load1 will probably be the exception anyway.

I'm happy to see this go in, we can do some pretty cool things with it :-)

r=philipp, feel free to push any time!

::: calendar/base/modules/calItemUtils.jsm
@@ +14,5 @@
> + * The Original Code is Mozilla Calendar code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Philipp Kewisch <[e-mail]>
> + * Portions created by the Initial Developer are Copyright (C) 2011

2011-2012. Or you could just use the new MPL2 headers like here: http://www.mozilla.org/MPL/headers/

Comment 6

[Matthew Mecca [:mmecca]]

(In reply to Philipp Kewisch [:Fallen] from comment #5)
> 2011-2012. Or you could just use the new MPL2 headers like here:
> http://www.mozilla.org/MPL/headers/

It looks to me like the MPL2 header doesn't have any Initial Developer or Contributor(s) section - is that correct?

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, correct. They decided to remove it because its never 100% accurate. I'm not totally happy with it, but unfortunately its too late to change that aspect of the MPL2.

You should apply for being listed in http://www.mozilla.org/credits/ instead.

Comment 8

[Matthew Mecca [:mmecca]]

Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/5e80e777ae2d