[FIXES BVILD] Further improvement to diff.rb (I hope not to touch this beast again any time soon); See Changes ripped out from WikiController#show, will become a separate action

app/controllers/wiki_controller.rb

@@ -49,6 +49,10 @@ class WikiController < ApplicationController
     @authors = @page_names_by_author.keys.sort
   end
   
+  def changes
+    raise "Not implemented yet"
+  end
+  
   def export_html
     stylesheet = File.read(File.join(RAILS_ROOT, 'public', 'stylesheets', 'instiki.css'))
     export_pages_as_zip('html') do |page| 

app/views/wiki/page.rhtml

@@ -7,20 +7,6 @@
   <%= @renderer.display_content %>
 </div>
 
-
-<% if @page.revisions.length > 1 %>
-  <div id="changes" style="display: none">
-    <p style="background: #eee; padding: 3px; border: 1px solid silver">
-      <small>
-        Showing changes from revision #<%= @page.revisions.size - 2 %> to #<%= @page.revisions.size - 1 %>:
-        <ins class="diffins">Added</ins> | <del class="diffdel">Removed</del>
-      </small>
-    </p>
-
-    <%= @renderer.display_diff %>
-  </div>
-<% end %>
-
 <div class="byline">
   <%= @page.revisions? ? "Revised" : "Created" %> on <%= format_date(@page.revised_at) %> 
   by <%= author_link(@page) %>
@@ -60,12 +46,10 @@
   <% end %>
 
   <% if @page.revisions.length > 1 %>
-    <span id="show_changes">
+    <%= link_to('See changes', 
-      | <a href="#" name="see_changes" onClick="toggleChanges(); return false;">See changes</a>
+          {:web => @web.address, :action => 'changes', :id => @page.name},
-    </span>
+          {:class => 'navlink', :accesskey => 'C', :name => 'see_changes'})
-    <span id="hide_changes" name="hide_changes" style="display: none">
+    %>
-      | <a href="#" onClick="toggleChanges(); return false;">Hide changes</a>
-    </span>
   <% end %>
 
   <small>
@@ -85,19 +69,3 @@
 
   <%= render :partial => 'inbound_links' %>
 </div>
-
-<script language="Javascript" type="text/Javascript">
-  function toggleChanges() {
-    if (document.getElementById("changes").style.display == "none") {
-      document.getElementById("changes").style.display  = "block";
-      document.getElementById("revision").style.display = "none";
-      document.getElementById("show_changes").style.display  = "none";
-      document.getElementById("hide_changes").style.display = "inline";
-    } else {
-      document.getElementById("changes").style.display  = "none";
-      document.getElementById("revision").style.display = "block";
-      document.getElementById("show_changes").style.display  = "inline";
-      document.getElementById("hide_changes").style.display = "none";
-    }
-  }
-</script>

lib/diff.rb

@@ -33,8 +33,8 @@ module HTMLDiff
     end
 
     def index_new_words
-      @word_indices = {}
+      @word_indices = Hash.new { |h, word| h[word] = [] }
-      @new_words.each_with_index { |word, i| (@word_indices[word] ||= []) << i }
+      @new_words.each_with_index { |word, i| @word_indices[word] << i }
     end
 
     def operations
@@ -70,10 +70,12 @@ module HTMLDiff
                   position_in_new, match.start_in_new)
           operations << operation_upto_match_positions
         end
+        if match.size != 0
           match_operation = Operation.new(:equal, 
               match.start_in_old, match.end_in_old, 
               match.start_in_new, match.end_in_new)
           operations << match_operation
+        end
 
         position_in_old = match.end_in_old
         position_in_new = match.end_in_new
@@ -104,41 +106,63 @@ module HTMLDiff
     end
 
     def find_match(start_in_old, end_in_old, start_in_new, end_in_new)
-      besti, bestj, bestsize = start_in_old, start_in_new, 0
 
-      j2len = {}
+      best_match_in_old = start_in_old
+      best_match_in_new = start_in_new
+      best_match_size = 0
       
-      (start_in_old..end_in_old).step do |i|
+      match_length_at = Hash.new { |h, index| h[index] = 0 }
-        newj2len = {}
-        (@word_indices[@old_words[i]] || []).each do |j|
-          next  if j < start_in_new
-          break if j >= end_in_new
       
-          k = newj2len[j] = (j2len[j - 1] || 0) + 1
+      start_in_old.upto(end_in_old - 1) do |index_in_old|
-          if k > bestsize
+
-            besti, bestj, bestsize = i - k + 1, j - k + 1, k
+        new_match_length_at = Hash.new { |h, index| h[index] = 0 }
+
+        @word_indices[@old_words[index_in_old]].each do |index_in_new|
+          next  if index_in_new < start_in_new
+          break if index_in_new >= end_in_new
+
+          new_match_length = match_length_at[index_in_new - 1] + 1
+          new_match_length_at[index_in_new] = new_match_length
+
+          if new_match_length > best_match_size
+            best_match_in_old = index_in_old - new_match_length + 1
+            best_match_in_new = index_in_new - new_match_length + 1
+            best_match_size = new_match_length
           end
         end
-        j2len = newj2len
+        match_length_at = new_match_length_at
       end
 
-      while besti > start_in_old and bestj > start_in_new and @old_words[besti - 1] == @new_words[bestj - 1]
+#      best_match_in_old, best_match_in_new, best_match_size = add_matching_words_left(
-        besti, bestj, bestsize = besti - 1, bestj - 1, bestsize + 1
+#          best_match_in_old, best_match_in_new, best_match_size, start_in_old, start_in_new)
+#      best_match_in_old, best_match_in_new, match_size = add_matching_words_right(
+#          best_match_in_old, best_match_in_new, best_match_size, end_in_old, end_in_new)
+
+      return (best_match_size != 0 ? Match.new(best_match_in_old, best_match_in_new, best_match_size) : nil)
     end
 
-      while besti + bestsize < end_in_old and bestj + bestsize < end_in_new and
+    def add_matching_words_left(match_in_old, match_in_new, match_size, start_in_old, start_in_new)
-          @old_words[besti + bestsize] == @new_words[bestj + bestsize]
+      while match_in_old > start_in_old and 
-        bestsize += 1
+            match_in_new > start_in_new and 
+            @old_words[match_in_old - 1] == @new_words[match_in_new - 1]
+        match_in_old -= 1
+        match_in_new -= 1
+        match_size += 1
+      end
+      [match_in_old, match_in_new, match_size]
     end
 
-      if bestsize == 0 
+    def add_matching_words_right(match_in_old, match_in_new, match_size, end_in_old, end_in_new)
-        return nil
+      while match_in_old + match_size < end_in_old and 
-      else 
+            match_in_new + match_size < end_in_new and
-        return Match.new(besti, bestj, bestsize)
+            @old_words[match_in_old + match_size] == @new_words[match_in_new + match_size]
+        match_size += 1
       end
+      [match_in_old, match_in_new, match_size]
     end
     
     VALID_METHODS = [:replace, :insert, :delete, :equal]
+
     def perform_operation(operation)
       @operation = operation
       self.send operation.action, operation
@@ -158,7 +182,7 @@ module HTMLDiff
     end
     
     def equal(operation)
-      # no tags to insert, simply copy the matching words from onbe of the versions
+      # no tags to insert, simply copy the matching words from one of the versions
       @content += @new_words[operation.start_in_new...operation.end_in_new]
     end
   
@@ -196,9 +220,9 @@ module HTMLDiff
     # new: '<p>ab</p><p>c</b>'
     # diff result: '<p>a<ins>b</ins></p><p><ins>c</ins></p>'
     # this still doesn't guarantee valid HTML (hint: think about diffing a text containing ins or
-    # del tags), but handles correctly more cases than earlier version.
+    # del tags), but handles correctly more cases than the earlier version.
     # 
-    # PS: Spare a thought for people who write HTML browsers. They live in this ... every day.
+    # P.S.: Spare a thought for people who write HTML browsers. They live in this ... every day.
 
     def insert_tag(tagname, cssclass, words)
       loop do

test/unit/diff_test.rb

@@ -73,10 +73,14 @@ class DiffTest < Test::Unit::TestCase
   def test_html_diff_with_multiple_paragraphs
     a = "<p>this was the original string</p>"
     b = "<p>this is</p>\r\n<p> the new string</p>\r\n<p>around the world</p>"
+
+    # Some of this expected result is accidental to implementation. 
+    # At least it's well-formed and more or less correct.
     assert_equal(
-        "<p>this <del class=\"diffmod\">was</del><ins class=\"diffmod\">is</ins></p>\r\n<p> the " +
+        "<p>this <del class=\"diffmod\">was</del><ins class=\"diffmod\">is</ins></p>"+
-        "<del class=\"diffmod\">original </del>" + 
+        "<ins class=\"diffmod\">\r\n</ins><p> the " +
-        "<ins class=\"diffmod\">new </ins>string</p>\r\n" +
+        "<del class=\"diffmod\">original</del><ins class=\"diffmod\">new</ins>" +
+        " string</p><ins class=\"diffins\">\r\n</ins>" +
         "<p><ins class=\"diffins\">around the world</ins></p>",
         diff(a, b))
   end
@@ -96,4 +100,11 @@ class DiffTest < Test::Unit::TestCase
     assert_equal '<div><ins class="diffins">foo</ins></div>', diff(a, b)
   end
   
+  def test_diff_for_tag_change
+    a = "<a>x</a>"
+    b = "<b>x</b>"
+    # FIXME sad, but true - this case produces an invalid XML. If handle this you can, strong your foo is.
+    assert_equal '<a><b>x</a></b>', diff(a, b)
+  end
+
 end