Merge branch 'side-by-side-unfold' into 'master'

Add unfold links for Side-by-Side view

## What does this MR do?

Add unfold links for Side-by-Side view and refactor some diff related code

## What are the relevant issue numbers?

Closes #3877 

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5415

CHANGELOG

@@ -68,6 +68,7 @@ v 8.11.0 (unreleased)
   - Fix RequestProfiler::Middleware error when code is reloaded in development
   - Catch what warden might throw when profiling requests to re-throw it
   - Speed up and reduce memory usage of Commit#repo_changes, Repository#expire_avatar_cache and IrkerWorker
+  - Add unfold links for Side-by-Side view. !5415 (Tim Masliuchenko)
 
 v 8.10.5 (unreleased)
 

app/assets/javascripts/diff.js

@@ -10,7 +10,7 @@
       $(document).off('click', '.js-unfold');
       $(document).on('click', '.js-unfold', (function(_this) {
         return function(event) {
-          var line_number, link, offset, old_line, params, prev_new_line, prev_old_line, ref, ref1, since, target, to, unfold, unfoldBottom;
+          var line_number, link, file, offset, old_line, params, prev_new_line, prev_old_line, ref, ref1, since, target, to, unfold, unfoldBottom;
           target = $(event.target);
           unfoldBottom = target.hasClass('js-unfold-bottom');
           unfold = true;
@@ -31,14 +31,16 @@
               unfold = false;
             }
           }
-          link = target.parents('.diff-file').attr('data-blob-diff-path');
+          file = target.parents('.diff-file');
+          link = file.data('blob-diff-path');
           params = {
             since: since,
             to: to,
             bottom: unfoldBottom,
             offset: offset,
             unfold: unfold,
-            indent: 1
+            indent: 1,
+            view: file.data('view')
           };
           return $.get(link, params, function(response) {
             return target.parent().replaceWith(response);
@@ -48,26 +50,13 @@
     }
 
     Diff.prototype.lineNumbers = function(line) {
-      var i, l, len, line_number, line_numbers, lines, results;
       if (!line.children().length) {
         return [0, 0];
       }
-      lines = line.children().slice(0, 2);
-      line_numbers = (function() {
-        var i, len, results;
-        results = [];
-        for (i = 0, len = lines.length; i < len; i++) {
-          l = lines[i];
-          results.push($(l).attr('data-linenumber'));
-        }
-        return results;
-      })();
-      results = [];
-      for (i = 0, len = line_numbers.length; i < len; i++) {
-        line_number = line_numbers[i];
-        results.push(parseInt(line_number));
-      }
-      return results;
+
+      return line.find('.diff-line-num').map(function() {
+        return parseInt($(this).data('linenumber'));
+      });
     };
 
     return Diff;

app/controllers/projects/blob_controller.rb

@@ -76,6 +76,8 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def diff
+    apply_diff_view_cookie!
+
     @form  = UnfoldForm.new(params)
     @lines = Gitlab::Highlight.highlight_lines(repository, @ref, @path)
     @lines = @lines[@form.since - 1..@form.to - 1]

app/helpers/diff_helper.rb

@@ -13,12 +13,11 @@ module DiffHelper
   end
 
   def diff_view
+    @diff_view ||= begin
       diff_views = %w(inline parallel)
-
-    if diff_views.include?(cookies[:diff_view])
-      cookies[:diff_view]
-    else
-      diff_views.first
+      diff_view = cookies[:diff_view]
+      diff_view = diff_views.first unless diff_views.include?(diff_view)
+      diff_view.to_sym
     end
   end
 
@@ -33,12 +32,23 @@ module DiffHelper
     options
   end
 
-  def unfold_bottom_class(bottom)
-    bottom ? 'js-unfold js-unfold-bottom' : ''
+  def diff_match_line(old_pos, new_pos, text: '', view: :inline, bottom: false)
+    content = content_tag :td, text, class: "line_content match #{view == :inline ? '' : view}"
+    cls = ['diff-line-num', 'unfold', 'js-unfold']
+    cls << 'js-unfold-bottom' if bottom
+
+    html = ''
+    if old_pos
+      html << content_tag(:td, '...', class: cls + ['old_line'], data: { linenumber: old_pos })
+      html << content unless view == :inline
+    end
+
+    if new_pos
+      html << content_tag(:td, '...', class: cls + ['new_line'], data: { linenumber: new_pos })
+      html << content
     end
 
-  def unfold_class(unfold)
-    unfold ? 'unfold js-unfold' : ''
+    html.html_safe
   end
 
   def diff_line_content(line, line_type = nil)
@@ -67,11 +77,11 @@ module DiffHelper
   end
 
   def inline_diff_btn
-    diff_btn('Inline', 'inline', diff_view == 'inline')
+    diff_btn('Inline', 'inline', diff_view == :inline)
   end
 
   def parallel_diff_btn
-    diff_btn('Side-by-side', 'parallel', diff_view == 'parallel')
+    diff_btn('Side-by-side', 'parallel', diff_view == :parallel)
   end
 
   def submodule_link(blob, ref, repository = @repository)
@@ -103,7 +113,8 @@ module DiffHelper
     commit = commit_for_diff(diff_file)
     {
       blob_diff_path: namespace_project_blob_diff_path(project.namespace, project,
-                                                       tree_join(commit.id, diff_file.file_path))
+                                                       tree_join(commit.id, diff_file.file_path)),
+      view: diff_view
     }
   end
 

app/views/projects/blob/diff.html.haml

 - if @lines.present?
+  - line_class = diff_view == :inline ? '' : diff_view
   - if @form.unfold? && @form.since != 1 && !@form.bottom?
-    %tr.line_holder
-      = render "projects/diffs/match_line", { line: @match_line,
-        line_old: @form.since, line_new: @form.since, bottom: false, new_file: false }
+    %tr.line_holder{ class: line_class }
+      = diff_match_line @form.since, @form.since, text: @match_line, view: diff_view
 
   - @lines.each_with_index do |line, index|
     - line_new = index + @form.since
     - line_old = line_new - @form.offset
-    %tr.line_holder{ id: line_old }
+    - line_content = capture do
+      %td.line_content.noteable_line{ class: line_class }==#{' ' * @form.indent}#{line}
+    %tr.line_holder{ id: line_old, class: line_class }
+      - case diff_view
+      - when :inline
         %td.old_line.diff-line-num{ data: { linenumber: line_old } }
+          %a{href: "##{line_old}", data: { linenumber: line_old }}
+        %td.new_line.diff-line-num{ data: { linenumber: line_new } }
+          %a{href: "##{line_new}", data: { linenumber: line_new }}
+        = line_content
+      - when :parallel
+        %td.old_line.diff-line-num{data: { linenumber: line_old }}
           = link_to raw(line_old), "##{line_old}"
-      %td.new_line.diff-line-num{ data: { linenumber: line_old } }
-        = link_to raw(line_new) , "##{line_old}"
-      %td.line_content.noteable_line==#{' ' * @form.indent}#{line}
+        = line_content
+        %td.new_line.diff-line-num{data: { linenumber: line_new }}
+          = link_to raw(line_new), "##{line_new}"
+        = line_content
 
   - if @form.unfold? && @form.bottom? && @form.to < @blob.loc
-    %tr.line_holder{ id: @form.to }
-      = render "projects/diffs/match_line", { line: @match_line,
-        line_old: @form.to, line_new: @form.to, bottom: true, new_file: false }
+    %tr.line_holder{ id: @form.to, class: line_class }
+      = diff_match_line @form.to, @form.to, text: @match_line, view: diff_view, bottom: true

app/views/projects/diffs/_content.html.haml

@@ -13,7 +13,7 @@
       .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
         This diff is collapsed. Click to expand it.
     - elsif diff_file.diff_lines.length > 0
-      - if diff_view == 'parallel'
+      - if diff_view == :parallel
         = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
       - else
         = render "projects/diffs/text_file", diff_file: diff_file

app/views/projects/diffs/_diffs.html.haml

 - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
 - diff_files = diffs.diff_files
-- if diff_view == 'parallel'
+- if diff_view == :parallel
   - fluid_layout true
 
 .content-block.oneline-block.files-changed

app/views/projects/diffs/_line.html.haml

@@ -4,8 +4,7 @@
 %tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } }
   - case type
   - when 'match'
-    = render "projects/diffs/match_line", { line: line.text,
-      line_old: line.old_pos, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file }
+    = diff_match_line line.old_pos, line.new_pos, text: line.text
   - when 'nonewline'
     %td.old_line.diff-line-num
     %td.new_line.diff-line-num

app/views/projects/diffs/_match_line.html.haml

-%td.old_line.diff-line-num{data: {linenumber: line_old},
- class: [unfold_bottom_class(bottom), unfold_class(!new_file)]}
-  \...
-%td.new_line.diff-line-num{data: {linenumber: line_new},
- class: [unfold_bottom_class(bottom), unfold_class(!new_file)]}
-  \...
-%td.line_content.match= line

app/views/projects/diffs/_parallel_view.html.haml

 / Side-by-side diff view
 %div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ data: diff_view_data }
   %table
+    - last_line = 0
     - diff_file.parallel_diff_lines.each do |line|
       - left = line[:left]
       - right = line[:right]
+      - last_line = right.new_pos if right
       %tr.line_holder.parallel
         - if left
           - if left.meta?
-            %td.old_line.diff-line-num.empty-cell
-            %td.line_content.parallel.match= left.text
+            = diff_match_line left.old_pos, nil, text: left.text, view: :parallel
           - else
             - left_line_code = diff_file.line_code(left)
             - left_position = diff_file.position(left)
@@ -21,8 +22,7 @@
 
         - if right
           - if right.meta?
-            %td.old_line.diff-line-num.empty-cell
-            %td.line_content.parallel.match= left.text
+            = diff_match_line nil, right.new_pos, text: left.text, view: :parallel
           - else
             - right_line_code = diff_file.line_code(right)
             - right_position = diff_file.position(right)
@@ -37,3 +37,5 @@
         - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file)
         - if discussion_left || discussion_right
           = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right
+    - if !diff_file.new_file && last_line > 0
+      = diff_match_line last_line, last_line, bottom: true, view: :parallel

app/views/projects/diffs/_text_file.html.haml

@@ -15,6 +15,5 @@
       - if discussion
         = render "discussions/diff_discussion", discussion: discussion
 
-  - if last_line > 0
-    = render "projects/diffs/match_line", { line: "",
-      line_old: last_line, line_new: last_line, bottom: true, new_file: diff_file.new_file }
+  - if !diff_file.new_file && last_line > 0
+    = diff_match_line last_line, last_line, bottom: true

app/views/projects/merge_requests/_show.html.haml

@@ -2,7 +2,7 @@
 - page_description     @merge_request.description
 - page_card_attributes @merge_request.card_attributes
 
-- if diff_view == 'parallel'
+- if diff_view == :parallel
   - fluid_layout true
 
 .merge-request{'data-url' => merge_request_path(@merge_request)}

features/project/merge_requests.feature

@@ -236,6 +236,15 @@ Feature: Project Merge Requests
     And I unfold diff
     Then I should see additional file lines
 
+  @javascript
+  Scenario: I unfold diff in Side-by-Side view
+    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And I visit merge request page "Bug NS-05"
+    And I click on the Changes tab
+    And I click Side-by-side Diff tab
+    And I unfold diff
+    Then I should see additional file lines
+
   @javascript
   Scenario: I show comments on a merge request side-by-side diff with comments in multiple files
     Given project "Shop" have "Bug NS-05" open merge request with diffs inside

features/steps/project/merge_requests.rb

@@ -477,6 +477,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
 
   step 'I click Side-by-side Diff tab' do
     find('a', text: 'Side-by-side').trigger('click')
+
+    # Waits for load
+    expect(page).to have_css('.parallel')
   end
 
   step 'I should see comments on the side-by-side diff page' do

spec/helpers/diff_helper_spec.rb

@@ -15,19 +15,19 @@ describe DiffHelper do
     it 'returns a valid value when cookie is set' do
       helper.request.cookies[:diff_view] = 'parallel'
 
-      expect(helper.diff_view).to eq 'parallel'
+      expect(helper.diff_view).to eq :parallel
     end
 
     it 'returns a default value when cookie is invalid' do
       helper.request.cookies[:diff_view] = 'invalid'
 
-      expect(helper.diff_view).to eq 'inline'
+      expect(helper.diff_view).to eq :inline
     end
 
     it 'returns a default value when cookie is nil' do
       expect(helper.request.cookies).to be_empty
 
-      expect(helper.diff_view).to eq 'inline'
+      expect(helper.diff_view).to eq :inline
     end
   end
 
@@ -59,26 +59,6 @@ describe DiffHelper do
     end
   end
 
-  describe 'unfold_bottom_class' do
-    it 'should return empty string when bottom line shouldnt be unfolded' do
-      expect(unfold_bottom_class(false)).to eq('')
-    end
-
-    it 'should return js class when bottom lines should be unfolded' do
-      expect(unfold_bottom_class(true)).to include('js-unfold-bottom')
-    end
-  end
-
-  describe 'unfold_class' do
-    it 'returns empty on false' do
-      expect(unfold_class(false)).to eq('')
-    end
-
-    it 'returns a class on true' do
-      expect(unfold_class(true)).to eq('unfold js-unfold')
-    end
-  end
-
   describe '#diff_line_content' do
     it 'should return non breaking space when line is empty' do
       expect(diff_line_content(nil)).to eq('  ')
@@ -105,4 +85,56 @@ describe DiffHelper do
       expect(marked_new_line).to be_html_safe
     end
   end
+
+  describe "#diff_match_line" do
+    let(:old_pos) { 40 }
+    let(:new_pos) { 50 }
+    let(:text) { 'some_text' }
+
+    it "should generate foldable top match line for inline view with empty text by default" do
+      output = diff_match_line old_pos, new_pos
+
+      expect(output).to be_html_safe
+      expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...'
+      expect(output).to have_css "td:nth-child(2):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(3):not(.parallel).line_content.match', text: ''
+    end
+
+    it "should allow to define text and bottom option" do
+      output = diff_match_line old_pos, new_pos, text: text, bottom: true
+
+      expect(output).to be_html_safe
+      expect(output).to have_css "td:nth-child(1).diff-line-num.unfold.js-unfold.js-unfold-bottom.old_line[data-linenumber='#{old_pos}']", text: '...'
+      expect(output).to have_css "td:nth-child(2).diff-line-num.unfold.js-unfold.js-unfold-bottom.new_line[data-linenumber='#{new_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(3):not(.parallel).line_content.match', text: text
+    end
+
+    it "should generate match line for parallel view" do
+      output = diff_match_line old_pos, new_pos, text: text, view: :parallel
+
+      expect(output).to be_html_safe
+      expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text
+      expect(output).to have_css "td:nth-child(3):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(4).line_content.match.parallel', text: text
+    end
+
+    it "should allow to generate only left match line for parallel view" do
+      output = diff_match_line old_pos, nil, text: text, view: :parallel
+
+      expect(output).to be_html_safe
+      expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text
+      expect(output).not_to have_css 'td:nth-child(3)'
+    end
+
+    it "should allow to generate only right match line for parallel view" do
+      output = diff_match_line nil, new_pos, text: text, view: :parallel
+
+      expect(output).to be_html_safe
+      expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...'
+      expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text
+      expect(output).not_to have_css 'td:nth-child(3)'
+    end
+  end
 end