This doesn't really describe the function. Might be better as a comment.

Also, "Access"

Trailing whitespace.

Should be removed before committing.

Same here.

Should also be removed.

Trailing whitespace.

I don't know that the comment is really necessary.

"WS" -> "whitespace"

Space after the :

Also, on the comments, there should be a space after #, and the comment should end with a period.

Would you mind adding some comments explaining this code? It's sufficiently complex.

Space on both sides of +

When does r ever change?

Since we do each range twice, we might as well store this in variables and reuse it.

Space before "left"

Can be: 0px 15px;

Space before "right"

Please add a documentation comment above this describing the function.

Since we reference table.sidebyside instances many times, we should store the result of $("table.sidebyside") in a variable and reference that variable for all other uses.

Can you put comments on hteir own lines? With a blank line preceding them.

Blank line before this.

Two blank lines here.

No space after parent().

Should have translation for the "Moved from ..."

Translation here too.

This is probably part of the other change, but rather than having onclick, I'd prefer that diffviewer.js register these actions for these links. You can give them IDs to reference them.

Hey Eduardo,

Can you update this diff now that the whitespace change is in?

I'm trying to think of ways to optimize the algorithm so we don't loop so much at the end. I think we can do this a few different ways.

While doing the initial iteration and deciding whether to dump the range into an inserts or deletes bucket, we could generate some sort of a hash of the data and store it in a dictionary. The dictionary would map the hash to a list of groups. This may only be needed for the removes bucket.

Then, after putting them into buckets, we can go through the inserts list and see if its generated hash exists in the removes bucket. If it does, loop through the list associated with that hash and do the comparison.

This has the benefit of only introducing one mandatory loop, the one for the inserts list. Instead of looping through all the remaining removes on each insert, we only need to do one cheap lookup, and then iterate through a small list (probably 1 or 2) of items for comparison.

The hash tag could be something as simple as "%d-%d-%s-%s" % (i2 - i1, j2 - j1, differ.a[:10], differ.a[-10:]), which would potentially have more matches to search through, but would be simple and quick to generate. Probably better than going through all the data and generating a checksum, which would cost more than a couple extra small loops.

So, something like:


    for ... in differ.get_opcodes():
        ...
        if tag == 'delete':
            key = "%d-%d-%s-%s" % (j2 - j1, i2 - i1, differ.b[10:], differ.b[-10:])

            if not key in removes:
                removes[key] = []

            removes[key].append(groups[-1])
        else:
            key = "%d-%d-%s-%s" % (i2 - i1, j2 - j1, differ.a[:10], differ.a[-10:])

            if not key in inserts:
                inserts[key] = []

            inserts[key].append(groups[-1])

    for insert_key, groups in inserts.iteritems():
        if insert_key in removes:
            # Do comparisons here


This is just a rough idea at 1 in the morning, so it's probably buggy at best, but try out a version of this and see if you can make it work. I want to be sure this algorithm is as optimal as possible.

Thanks for your hard work on this so far!

For this, you can use:

for r in xrange(0, len(removes)):

As an optimization, I think we can check to see if the length of each range is the same before doing any data comparison.

This pretty much has the same effect as:

return groups