[Wiki] Better diffing view

2023-10-15 13:08:28 +02:00 · 2023-10-15 13:08:28 +02:00 · 2620ea983e
commit 2620ea983e
parent 80b26fbe16
13 changed files with 118 additions and 98 deletions
--- a/2
+++ b/2
@ -1,6 +1,6 @@
 FROM ruby:3.2.2-alpine3.18 as ruby-builder

-RUN apk --no-cache add build-base git glib-dev postgresql15-dev
+RUN apk --no-cache add build-base cmake git glib-dev postgresql15-dev

 COPY Gemfile Gemfile.lock ./
 RUN gem i foreman && BUNDLE_IGNORE_CONFIG=true bundle install -j$(nproc) \
--- a/4
+++ b/4
@ -8,7 +8,6 @@ gem "dalli", :platforms => :ruby
 gem "simple_form"
 gem 'active_model_serializers', '~> 0.10.0'
 gem 'ruby-vips'
-gem 'diff-lcs', :require => "diff/lcs/array"
 gem 'bcrypt', :require => "bcrypt"
 gem 'draper'
 gem 'streamio-ffmpeg'
@ -30,6 +29,9 @@ gem 'redis'
 gem 'request_store'
 gem 'newrelic_rpm'

+gem "diffy"
+gem "rugged"
+
 gem 'opensearch-ruby'

 gem 'mailgun-ruby'
--- a/Gemfile.lock
+++ b/Gemfile.lock
@ -112,6 +112,7 @@ GEM
    dalli (3.2.5)
    date (3.3.3)
    diff-lcs (1.5.0)
+    diffy (3.4.2)
    domain_name (0.5.20190701)
      unf (>= 0.0.5, < 1.0.0)
    dotenv (2.8.1)
@ -299,6 +300,7 @@ GEM
    ruby-vips (2.1.4)
      ffi (~> 1.12)
    ruby2_keywords (0.0.5)
+    rugged (1.7.1)
    semantic_range (3.0.0)
    shoulda-context (2.0.0)
    shoulda-matchers (5.3.0)
@ -374,7 +376,7 @@ DEPENDENCIES
  bcrypt
  bootsnap
  dalli
-  diff-lcs
+  diffy
  dotenv-rails
  draper
  dtext_rb!
@ -400,6 +402,7 @@ DEPENDENCIES
  rubocop-erb
  rubocop-rails
  ruby-vips
+  rugged
  shoulda-context
  shoulda-matchers
  sidekiq (~> 7.0)
--- a/app/helpers/text_helper.rb
+++ b/app/helpers/text_helper.rb
@ -1,51 +1,5 @@
 module TextHelper
-  def lcs_diff(this, that)
-    pattern = Regexp.new('(?:<.+?>)|(?:\w+)|(?:[ \t]+)|(?:\r?\n)|(?:.+?)')
-
-    thisarr = this.scan(pattern)
-    otharr = that.scan(pattern)
-
-    cbo = Diff::LCS::ContextDiffCallbacks.new
-    diffs = thisarr.diff(otharr, cbo)
-
-    escape_html = ->(str) { str.gsub("&", "&amp;").gsub("<", "&lt;").gsub(">", "&gt;") }
-
-    output = thisarr
-    output.each {|q| q.replace(escape_html[q])}
-
-    diffs.reverse_each do |hunk|
-      newchange = hunk.max { |a, b| a.old_position <=> b.old_position }
-      newstart = newchange.old_position
-      oldstart = hunk.min { |a, b| a.old_position <=> b.old_position }.old_position
-
-      if newchange.action == "+"
-        output.insert(newstart, "</ins>")
-      end
-
-      hunk.reverse_each do |chg|
-        case chg.action
-        when "-"
-          oldstart = chg.old_position
-          output[chg.old_position] = "<br>" if chg.old_element.match(/^\r?\n$/)
-        when "+"
-          if chg.new_element.match(/^\r?\n$/)
-            output.insert(chg.old_position, "<br>")
-          else
-            output.insert(chg.old_position, escape_html[chg.new_element].to_s)
-          end
-        end
-      end
-
-      if newchange.action == "+"
-        output.insert(newstart, "<ins>")
-      end
-
-      if hunk[0].action == "-"
-        output.insert(newstart == oldstart || newchange.action != "+" ? newstart + 1 : newstart, "</del>")
-        output.insert(oldstart, "<del>")
-      end
-    end
-
-    output.join.gsub(/\r?\n/, "<br>").html_safe
+  def text_diff(this, that)
+    Diffy::Diff.new(this, that, ignore_crlf: true).to_s(:html).html_safe
  end
 end
--- a/app/javascript/src/styles/base/_colors.scss
+++ b/app/javascript/src/styles/base/_colors.scss
@ -60,11 +60,15 @@ $spoiler-hover-color: white;
 // Tag types
 $tag-selected-color: white;

-// Diffs (wiki/post versions)
-$diff-added-color: green;
-$diff-added-obsolete-color: darkGreen;
-$diff-removed-color: red;
-$diff-removed-obsolete-color: darkRed;
+// Diffs
+$diff-tag-added-color: green;
+$diff-tag-added-obsolete-color: darkGreen;
+$diff-tag-removed-color: red;
+$diff-tag-removed-obsolete-color: darkRed;
+$diff-text-added-color: rgba(46, 160, 67, 0.25);
+$diff-text-added-strong-color: rgba(46, 160, 67, 0.4);
+$diff-text-removed-color: rgba(248, 81, 73, 0.25);
+$diff-text-removed-strong-color: rgba(248 ,81, 73, 0.4);

 // Notices
 $error-notice-color: #a00;
@ -143,8 +147,6 @@ $post-mode-delete: #4e1010;
 // Edit History / Post Versions
 $post-version-grid-border: black;
 $post-version-content-background: inherit;
-$post-version-diff-added-color: $state-success-color;
-$post-version-diff-removed-color: $state-error-color;

 // Posts
 $post-rating-explicit-color: $red-color;
--- a/app/javascript/src/styles/common/diffs.scss
+++ b/app/javascript/src/styles/common/diffs.scss
@ -2,48 +2,82 @@

 .diff-list {
  .added, .added a {
-    color: $diff-added-color;
+    color: $diff-tag-added-color;
    text-decoration: none;
    margin-right: 0.5em;
  }

  .added.obsolete, .added.obsolete a {
-    color: $diff-added-obsolete-color;
+    color: $diff-tag-added-obsolete-color;
    text-decoration: line-through;
  }

  .removed, .removed a {
-    color: $diff-removed-color;
+    color: $diff-tag-removed-color;
    text-decoration: none;
    margin-right: 0.5em;
  }

  .removed.obsolete, .removed.obsolete a {
    text-decoration: line-through;
-    color: $diff-removed-obsolete-color;
+    color: $diff-tag-removed-obsolete-color;
  }
 }

 .diff-list {
  ins, ins a {
-    color: $diff-added-color;
+    color: $diff-tag-added-color;
    text-decoration: none;
    margin-right: 0.5em;
  }

  del, del a {
-    color: $diff-removed-color;
+    color: $diff-tag-removed-color;
    text-decoration: none;
    margin-right: 0.5em;
  }

  ins.obsolete, ins.obsolete a {
    text-decoration: line-through;
-    color: $diff-added-obsolete-color;
+    color: $diff-tag-added-obsolete-color;
  }

  del.obsolete, del.obsolete a {
    text-decoration: line-through;
-    color: $diff-removed-obsolete-color;
+    color: $diff-tag-removed-obsolete-color;
+  }  
+}
+
+// diffy
+.diff {
+  ins, del {
+    text-decoration: none;
+  }
+
+  ins {
+    background-color: $diff-text-added-color;
+
+    strong {
+      background-color: $diff-text-added-strong-color;
+    }
+  }
+
+  del {
+    background-color: $diff-text-removed-color;
+
+    strong {
+      background-color: $diff-text-removed-strong-color;
+    }
+  }
+
+  // Make empty lines visible
+  li {
+    > span:empty:before {
+      content: "\200b"; // unicode zero width space character
+    }
+
+    > ins > strong:empty:before, > del > strong:empty:before {
+      content: "¶";
+    }
  }
 }
--- a/app/javascript/src/styles/specific/edit_history.scss
+++ b/app/javascript/src/styles/specific/edit_history.scss
@ -1,20 +1,7 @@
-
-
 #c-edit-history {
  .edit-item {
-    display: flex;
-    &>div {
-      margin-right: 1em;
-      display: inline-block;
-    }
-  }
-
-  ins {
-    background-color: $post-version-diff-added-color;
-    font-weight: bold;
-  }
-  del {
-    background-color: $post-version-diff-removed-color;
-    font-weight: bold;
+    display: grid;
+    grid-template-columns: auto 1fr;
+    column-gap: 1em;
  }
 }
--- a/app/javascript/src/styles/specific/wiki_page_versions.scss
+++ b/app/javascript/src/styles/specific/wiki_page_versions.scss
@ -1,18 +1,4 @@
-
-
 div#c-wiki-page-versions {
-  #a-diff {
-    del {
-      background: $diff-removed-color;
-      text-decoration: none;
-    }
-
-    ins {
-      background: $diff-added-color;
-      text-decoration: none;
-    }
-  }
-
  #a-index {
    table {
      margin-bottom: 1em;
--- a/app/logical/diffy_no_subprocess.rb
+++ b/app/logical/diffy_no_subprocess.rb
@ -0,0 +1,13 @@
+# Diffy normally calls out to git diff, this makes it use libgit2 instead
+module DiffyNoSubprocess
+  # https://github.com/samg/diffy/blob/85b18fa6b659f724937dea58ebbc0564f4475c8c/lib/diffy/diff.rb#L43-L77
+  def diff
+    @diff ||= Rugged::Patch.from_strings(@string1, @string2, context_lines: 10_000).to_s.force_encoding("UTF-8")
+  end
+
+  # https://github.com/samg/diffy/blob/85b18fa6b659f724937dea58ebbc0564f4475c8c/lib/diffy/diff.rb#L79-L100
+  def each(&)
+    # The first 5 lines are git diff header lines
+    diff.lines.drop(5).each(&)
+  end
+end
--- a/app/views/edit_histories/show.html.erb
+++ b/app/views/edit_histories/show.html.erb
@ -15,7 +15,7 @@
          <div class="content">
            <div class="body">
              <% if edit.version > 1 %>
-                <%= lcs_diff(@edits[idx-1].body, edit.body) %>
+                <%= text_diff(@edits[idx-1].body, edit.body) %>
              <% else %>
                <%= edit.body %>
              <% end %>
--- a/app/views/wiki_page_versions/diff.html.erb
+++ b/app/views/wiki_page_versions/diff.html.erb
@ -6,7 +6,7 @@
      <p>Showing differences between <%= compact_time @thispage.updated_at %> (<%= link_to_user @thispage.updater %>) and <%= compact_time @otherpage.updated_at %> (<%= link_to_user @otherpage.updater %>)</p>

      <div>
-        <%= lcs_diff(@thispage.body, @otherpage.body) %>
+        <%= text_diff(@thispage.body, @otherpage.body) %>
      </div>
    <% else %>
      <p>The artist requested removal of this page.</p>
--- a/config/initializers/diffy.rb
+++ b/config/initializers/diffy.rb
@ -0,0 +1,3 @@
+Rails.configuration.to_prepare do
+  Diffy::Diff.prepend DiffyNoSubprocess
+end
--- a/test/helpers/text_helper_test.rb
+++ b/test/helpers/text_helper_test.rb
@ -0,0 +1,36 @@
+require "test_helper"
+
+class TextHelperTest < ActionView::TestCase
+  should "not call diff" do
+    Open3.expects(:capture3).never
+    text_diff("abc", "def")
+  end
+
+  should "strip the header info" do
+    expected = <<~HTML.chomp
+      <div class="diff">
+        <ul>
+          <li class="del"><del><strong>abc</strong></del></li>
+          <li class="ins"><ins><strong>def</strong></ins></li>
+        </ul>
+      </div>
+
+    HTML
+    actual = text_diff("abc", "def")
+    assert_equal(expected, actual)
+  end
+
+  should "escape html entities" do
+    expected = <<~HTML.chomp
+      <div class="diff">
+        <ul>
+          <li class="del"><del>&lt;<strong>s&gt;&lt;/s</strong>&gt;</del></li>
+          <li class="ins"><ins>&lt;<strong>b&gt;&lt;/b</strong>&gt;</ins></li>
+        </ul>
+      </div>
+
+    HTML
+    actual = text_diff("<s></s>", "<b></b>")
+    assert_equal(expected, actual)
+  end
+end