From 2620ea983e06321bc431596beedcc8b080e9717d Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Sun, 15 Oct 2023 13:08:28 +0200 Subject: [PATCH] [Wiki] Better diffing view --- Dockerfile | 2 +- Gemfile | 4 +- Gemfile.lock | 5 +- app/helpers/text_helper.rb | 50 +------------------ app/javascript/src/styles/base/_colors.scss | 16 +++--- app/javascript/src/styles/common/diffs.scss | 50 ++++++++++++++++--- .../src/styles/specific/edit_history.scss | 19 ++----- .../styles/specific/wiki_page_versions.scss | 14 ------ app/logical/diffy_no_subprocess.rb | 13 +++++ app/views/edit_histories/show.html.erb | 2 +- app/views/wiki_page_versions/diff.html.erb | 2 +- config/initializers/diffy.rb | 3 ++ test/helpers/text_helper_test.rb | 36 +++++++++++++ 13 files changed, 118 insertions(+), 98 deletions(-) create mode 100644 app/logical/diffy_no_subprocess.rb create mode 100644 config/initializers/diffy.rb create mode 100644 test/helpers/text_helper_test.rb diff --git a/Dockerfile b/Dockerfile index 06b23f8ee..a4b076d20 100644 --- a/Dockerfile +++ b/Dockerfile @@ -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) \ diff --git a/Gemfile b/Gemfile index 4315fa9ec..ffbc3fd31 100644 --- a/Gemfile +++ b/Gemfile @@ -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' diff --git a/Gemfile.lock b/Gemfile.lock index 6c1782422..00aee2dd8 100644 --- 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) diff --git a/app/helpers/text_helper.rb b/app/helpers/text_helper.rb index 439beb51f..ff0ef2a5f 100644 --- 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("&", "&").gsub("<", "<").gsub(">", ">") } - - 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, "") - end - - hunk.reverse_each do |chg| - case chg.action - when "-" - oldstart = chg.old_position - output[chg.old_position] = "
" if chg.old_element.match(/^\r?\n$/) - when "+" - if chg.new_element.match(/^\r?\n$/) - output.insert(chg.old_position, "
") - else - output.insert(chg.old_position, escape_html[chg.new_element].to_s) - end - end - end - - if newchange.action == "+" - output.insert(newstart, "") - end - - if hunk[0].action == "-" - output.insert(newstart == oldstart || newchange.action != "+" ? newstart + 1 : newstart, "") - output.insert(oldstart, "") - end - end - - output.join.gsub(/\r?\n/, "
").html_safe + def text_diff(this, that) + Diffy::Diff.new(this, that, ignore_crlf: true).to_s(:html).html_safe end end diff --git a/app/javascript/src/styles/base/_colors.scss b/app/javascript/src/styles/base/_colors.scss index 3e619b13f..fac007664 100644 --- 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; diff --git a/app/javascript/src/styles/common/diffs.scss b/app/javascript/src/styles/common/diffs.scss index ff5de4839..c55b0384e 100644 --- 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: "ΒΆ"; + } } } diff --git a/app/javascript/src/styles/specific/edit_history.scss b/app/javascript/src/styles/specific/edit_history.scss index 516210f46..a04e977cc 100644 --- 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; } } diff --git a/app/javascript/src/styles/specific/wiki_page_versions.scss b/app/javascript/src/styles/specific/wiki_page_versions.scss index 4a88600d0..468613ca2 100644 --- 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; diff --git a/app/logical/diffy_no_subprocess.rb b/app/logical/diffy_no_subprocess.rb new file mode 100644 index 000000000..75c0e3b36 --- /dev/null +++ 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 diff --git a/app/views/edit_histories/show.html.erb b/app/views/edit_histories/show.html.erb index e92f42066..54588500e 100644 --- a/app/views/edit_histories/show.html.erb +++ b/app/views/edit_histories/show.html.erb @@ -15,7 +15,7 @@
<% if edit.version > 1 %> - <%= lcs_diff(@edits[idx-1].body, edit.body) %> + <%= text_diff(@edits[idx-1].body, edit.body) %> <% else %> <%= edit.body %> <% end %> diff --git a/app/views/wiki_page_versions/diff.html.erb b/app/views/wiki_page_versions/diff.html.erb index 87b510616..e33515362 100644 --- a/app/views/wiki_page_versions/diff.html.erb +++ b/app/views/wiki_page_versions/diff.html.erb @@ -6,7 +6,7 @@

Showing differences between <%= compact_time @thispage.updated_at %> (<%= link_to_user @thispage.updater %>) and <%= compact_time @otherpage.updated_at %> (<%= link_to_user @otherpage.updater %>)

- <%= lcs_diff(@thispage.body, @otherpage.body) %> + <%= text_diff(@thispage.body, @otherpage.body) %>
<% else %>

The artist requested removal of this page.

diff --git a/config/initializers/diffy.rb b/config/initializers/diffy.rb new file mode 100644 index 000000000..b9c589d96 --- /dev/null +++ b/config/initializers/diffy.rb @@ -0,0 +1,3 @@ +Rails.configuration.to_prepare do + Diffy::Diff.prepend DiffyNoSubprocess +end diff --git a/test/helpers/text_helper_test.rb b/test/helpers/text_helper_test.rb new file mode 100644 index 000000000..8371f8040 --- /dev/null +++ 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 +
+
    +
  • abc
    • +
  • def
    • +
+
+ + HTML + actual = text_diff("abc", "def") + assert_equal(expected, actual) + end + + should "escape html entities" do + expected = <<~HTML.chomp +
+
    +
  • <s></s>
    • +
  • <b></b>
    • +
+
+ + HTML + actual = text_diff("", "") + assert_equal(expected, actual) + end +end