Mediawiki code and UI for presenting differences

this *TOTALLY* kicks ass. ;) this is such a VASTLY superior UI, i don't even know where to start in praising it's mightiness.

i also confirmed that even though you're diff'ing against the user's input, not the output (which is a feature), that the resulting diff output isn't a security hole. so that's good. ;)

however, a few nits:

1. the auto-adjusting weight stuff doesn't seem to work, and is needlessly complicated. just add a diff.install file that implements diff_install() which does the db_query() for you. for extra credit, add a diff_update_1() to set the weight for sites upgrading from older (dark-ages) versions of diff.module. ;)
2. "Show Diff" should be "Show diff" for consistency with the rest of Drupal.
3. see attached patch (relative to your tarball) that IMHO improves the UI even more -- on the revisions tab, instead of having the "Show diff" button float at the bottom of the page, and the radio buttons being totally unlabeled, I used the button as a header for the 2 columns of radios in the table. It's a little bit non-standard for a Drupal UI, but I think it's the most clear. i also experimented with adding a normal header to those 2 columns, and adding a separate row at the bottom of the table that put the submit button under the radios, but i think the attached version is better.
4. there are a bunch of code-style problems in here ("} else {" on the same line, lots of spacing and indentation stuff, etc). see http://drupal.org/node/318 for details.
5. to be consistent with other parts of core, it'd be nice to use › for the prev/next diff links, instead of >>, etc.
6. i'm not sure what i think of manually outputting a raw HTML table to view the diff. i mean, in a way, it's perfectly obvious that's what you'd want to do, but i can imagine some theme-fanatics getting all mad about it. ;) on the other hand, theme('table', ...) does way too much junk you don't want, so i certainly don't fault you for this. just wanted to point it out in case anyone had any better ideas.
7. it's unclear why hook_diff() passes $old and $new by reference. seems like these hooks should never be modifying the data, so passing by reference seems unsafe. however, since they're objects, perhaps using references is best given php's weird object handling...
8. at the end of diff_node_revisions_submit(), instead of the drupal_goto(), you can just return the path you want, and FAPI takes care of the redirect for you. not sure if that's actually better (the drupal_goto() is almost clearer, IMHO), but that's how FAPI works, so it's probably a good idea to stick to that.
9. cck.inc should probably be conditionally included only if content.module is enabled.
10. taxonomy.inc isn't included at all. again, should be if taxonomy.module is enabled.

after playing with it pretty heavily, and a quick look at the code, that's all i came up with. there's probably more that would be found by someone who wasn't looking at 5am without sleep. ;) someone should therefore do a more careful line by line review (preferably after you'd already fixed all of these issues) before we actually commit this, but i'm *VERY* impressed, and very much in favor of moving to this version ASAP. i haven't looked at cck.inc or taxonomy.inc, but upload.inc and node.inc work great.

once this gets committed and people start to see this, i predict diff.module will become one of the most popular downloads on drupal.org. ;)

thanks so much!
-derek

whoops, here's that patch i promised.

and here's a screenshot of the resulting revisions tab.

for comparison, a screenshot of the original UI from the tar.gz in comment #2 above.

oh right, Heine reminded me why drupal_goto() is evil in a submit handler (point #8 in my list above): if you do that, none of the other submit handlers that might be added to this form via hook_form_alter() will get called. ;) i knew there must have been a reason...

just since rDouglass is too lazy to install this for himself ;) i'm posting a screen shot of the diff view which spans a few revisions, and shows off some of the coolness of node.inc and upload.inc...

Well, I installed it, and it rocks, I agree. My revisions links are now broken, however: q=node/28/revisions/38/view

Could this be a byproduct of the module hijacking the Revisions tab?

once we address these issues, i will overwrite current diff with this one (HEAD and D5).

FYI, I think a separate patch which brought taxonomy into the revisions system would be welcome. The current system is a known limitation, not a deliberate and permanent choice.

Found a bug, see screenshot.

In this screenshot, I only have 2 revisions. That screenshot produces a URL in the form: node/9/revisions/view/9/10

If I swap the radios to do an inverse diff, the URL is *still* : node/9/revisions/view/9/10

However, if you swap it manually in the URL, it works as specified.

The culprit is:

function diff_node_revisions_submit($form_id, $form_values) {
  // the ids are ordered so the old revision is always on the left
  $old_vid = min($form_values['old'], $form_values['new']);
  $new_vid = max($form_values['old'], $form_values['new']);
  return 'node/'.$form_values['nid'].'/revisions/view/'.$old_vid.'/'.$new_vid;
}  

It always puts the old first, then new. This is wrong, it should be smart enough to know which one is selected first. Might be tricky though, not sure of a good idea for a patch atm.

Sounds like jQuery to the rescue!

Good point, makes sense to follow the biggest diff-er out there ;-)

i just finished a major cleaning of the code. mostly code style, but also a little bit of logic cleanups, t() fixes, minor code-reorg, and a few UI changes (hopefully for the better).

http://drupal.org/files/issues/diff-070130-dww.tar.gz

(ahh, the joys of shell access to d.o) ;)

now committed to HEAD and DRUPAL-5. DRUPAL-5 version installed on scratch.drupal.org for testing.
hopefully we can get some quick feedback to ensure it's working for everyone, then i'll make a 5.x-1.1 official release for this.