Various changes and additions

Very nice! As I am testing on a 4.7.6 system, I wont be able to check it out right away. But give me some time and I'll test it on a 5.1 system.

Thx!

Very nice, I like the magic you're cooking up here rotzi.

3) This is probably the best way to switch off showing the "Preview changes" buttons for incompatible modules (e.g. webform). Very flexible so well done.

4) I somewhat agree with this notion, but I believe this still causes some confusion. Consider a drupal install with Preview set to required (admin > content > post settings). When creating or editing a node, you are presented with two sets of buttons for "Preview" and "Preview changes". This may not be straight forward for the average user - and I can see this stopping this module from going into core. How about merging the function with the existing Drupal "Preview" routine?

That's all, keep up the great work.

i read through the diff and it looks good to me ... i would love for all tables to be emitted by theme('table') if possible. i'm not likely able to test this pagtch, so hopefully some others will. once we get some positive reviews (people who actually carefully tested it, not mindless +1), roetzi should feel free to commit this.

i'll take a very close look in the near future. based on the description, this all sounds very promising. ;)

however, in general, i vastly prefer smaller issues and smaller patches... it gets really confusing dealing with lots of different threads of discussion/review/status in a single issue. and revision control works better with smaller, self-contained commits, instead of big commits that touch a bunch of unrelated things (e.g. if we decide to backout a single change, etc).

so far, this mostly looks great. sadly, it doesn't apply on DRUPAL-5 cleanly, only HEAD, so this will need some backporting at some point before it's actually RTBC. however, there are a few things that caught my eye when reading the diff:

1. line 492 (where you're adding the "Show preview changes button" to the workflow settings on the node-type edit form):
   

   $form_id == 'node_type_form' && isset($form['identity']['type']) && module_exists('comment')
   

   what do we care if comment module is enabled? doesn't diff work fine on node revisions, even if there are no comments? this seems unneccessary to me.

2. just below that, line 497, seems a little wonky to me, too:
   

   '#prefix' => '<strong>' . t('Preview changes') . '</strong>',
   

   There's got to be a cleaner way to do that. I'm not sure this "heading" is really necessary at all, in fact.

3. There are typos in the description for the filter_admin_configure altering, and for consistency, your fieldset should be collapsible, too. also, it's not clear why you're adding weight -5 to your fieldset, it doesn't strike me that the difference type is the most important setting on the filter configure page...
4. line 532: typos and formatting badness in a comment
5. node.inc: node_diff_strip_html() is both a lot of code, and not even quite right in some cases (e.g. <br/> vs. <br />, etc). this would be better: preg_replace('@</?(li|p|td|div|br)[^>]*>@i', "\n", ...). it also converts the opening tags to newlines, but that's probably a good thing anyway, to give more visual clarity about what's going on. could probably simplify some of the other code in there, too. we should also consider a single call to preg_replace() with arrays of regexps and replacements, instead of N calls that run through the whole text entirely.

just now starting to read through the diff_engine.patch -- so far so good, but i've gotta run. i'll return to that later.

i couldn't handle trying to read the DiffEngine.php patch itself -- too hard to quickly figure out if all that changed was the code style and formatting stuff. and, i don't have the time or energy to actually grok this entire file right now. so, i just did a pass through the patched file, and corrected the (fairly small handful) of other code-style issues i found.

it's also not clear to me if there are some classes in this file that we don't need at all, and could therefore be removed, to simplify maintainence for ourselves, and to shrink the footprint should this ever make it into core. i'd have to look a lot closer to be sure, but it certainly seems that way. rötzi, hopefully you can comment on this.

anyway, i'd say my patch here is RTBC, and we can always further simplify or cleanup DiffEngine.php in the future. meanwhile, the patch against the module code itself still needs the work i mentioned in my previous commit, so i'm leaving the status of the issue itself as "needs work".

Has there been any more progress on getting this patch ported to Drupal 5? I really need this module in order to allow users to use tinymce and be able to view useful diff information. Thanks for all of your work on this!

I'm very interested in the feature of stripping out HTML, that would be very usefull.

For what it's worth, here's my version of the HTML stripping portion of the code, which I did for 5.x before knowing about rotzi's patch. Note that it does not include the option to turn stripping off: HTML is always stripped.

Patch #9 no longer cleanly applies. Since it's a massive chunk (and there's no way to easily see what the patch changes), I request it gets re-rolled.

I couldn't wrap my brain around the monolithic patch, so I've broken it up into parts:

Part 1: DocBlocks - http://drupal.org/node/198914
Part 2: Preview Changes button - http://drupal.org/node/198917
Part 3: Table output through themes - http://drupal.org/node/198918

Two issues still need to be opened/patches created:
Part 4: Diff can be set to remove HTML tags (somewhat intelligent)
part 5: DiffEngine.php changed to follow Drupal style conventions

I hope this helps to move things forward.

Lovely, nice work. Thanks!

I wonder if rötzi is still around at all, or if diff.module has basically fallen back into a nearly abandoned state. :( Perhaps I'll have to carve out some time to plow through this issue queue again in the near future... It's certainly a critical module (used on d.o, in fact).

All the subpatches have been committed.

is this still working for for this module and the latest version of it and drupal 5.x?

Same question here. I tried and failed to use this patch with recent versions of D5.x (although I am a newbie in php, so it may just be me).

Is this working with the current 5.x-2.1 or does diff for D6.x integrate a way to hide HTML tags ?

I don't know why it says this is closed or that it was rolled into the current development release.

I found several errors in the patch code for modifying DIFF to strip HTML and work with WYSIWYG editors. I have created a fixed version of /diff/node.inc that works on my 5.x system to make WYSIWYG and DIFF play nice.

This is a critical component for us and I hope it helps someone. I'm not competent with patches so I'll upload the whole file.

Hope this helps someone, if not please help direct me into why I wasn't able to get it to work without modifying the code.

For those using Drupal 6 version of this module and are looking for a way to strip out the html coding in the comparison of revisions, go to this thread. The patch works very well.
http://drupal.org/node/372957
geoff