Add a difference engine for processing items

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Blocks 1 bug)

Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch WiP - v1 (obsolete) β€” β€” Splinter Review
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 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.
Attachment #604856 - Flags: feedback+
Posted patch Fix v1 (obsolete) β€” β€” Splinter Review
Updated patch with my comments, I'll ask review for my changes (and please check if I did the user and commit message right).
Attachment #604856 - Attachment is obsolete: true
Attachment #607804 - Flags: review?(philipp)
(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.
Posted patch Fix v2 β€” β€” Splinter Review
Separates the old and new versions of modified items. Returns items in a HashedArray.
Attachment #607804 - Attachment is obsolete: true
Attachment #614615 - Flags: review?(philipp)
Attachment #607804 - Flags: review?(philipp)
Blocks: 745081
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 <mozilla@kewis.ch>
> + * 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/
Attachment #614615 - Flags: review?(philipp) → review+
(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?
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.
Pushed to comm-central - http://hg.mozilla.org/comm-central/rev/5e80e777ae2d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.