Merge branch 'dm-copy-parallel-diff' into 'master'

Only copy old/new code when selecting left/right side of parallel diff

See merge request gitlab-org/gitlab-ce!14561

app/assets/javascripts/copy_as_gfm.js

@@ -298,7 +298,7 @@ class CopyAsGFM {
     const documentFragment = getSelectedFragment();
     if (!documentFragment) return;
 
-    const el = transformer(documentFragment.cloneNode(true));
+    const el = transformer(documentFragment.cloneNode(true), e.currentTarget);
     if (!el) return;
 
     e.preventDefault();
@@ -338,55 +338,64 @@ class CopyAsGFM {
   }
 
   static transformGFMSelection(documentFragment) {
-    const gfmEls = documentFragment.querySelectorAll('.md, .wiki');
-    switch (gfmEls.length) {
+    const gfmElements = documentFragment.querySelectorAll('.md, .wiki');
+    switch (gfmElements.length) {
       case 0: {
         return documentFragment;
       }
       case 1: {
-        return gfmEls[0];
+        return gfmElements[0];
       }
       default: {
-        const allGfmEl = document.createElement('div');
+        const allGfmElement = document.createElement('div');
 
-        for (let i = 0; i < gfmEls.length; i += 1) {
-          const lineEl = gfmEls[i];
-          allGfmEl.appendChild(lineEl);
-          allGfmEl.appendChild(document.createTextNode('\n\n'));
+        for (let i = 0; i < gfmElements.length; i += 1) {
+          const gfmElement = gfmElements[i];
+          allGfmElement.appendChild(gfmElement);
+          allGfmElement.appendChild(document.createTextNode('\n\n'));
         }
 
-        return allGfmEl;
+        return allGfmElement;
       }
     }
   }
 
-  static transformCodeSelection(documentFragment) {
-    const lineEls = documentFragment.querySelectorAll('.line');
+  static transformCodeSelection(documentFragment, target) {
+    let lineSelector = '.line';
 
-    let codeEl;
-    if (lineEls.length > 1) {
-      codeEl = document.createElement('pre');
-      codeEl.className = 'code highlight';
+    if (target) {
+      const lineClass = ['left-side', 'right-side'].filter(name => target.classList.contains(name))[0];
+      if (lineClass) {
+        lineSelector = `.line_content.${lineClass} ${lineSelector}`;
+      }
+    }
+
+    const lineElements = documentFragment.querySelectorAll(lineSelector);
+
+    let codeElement;
+    if (lineElements.length > 1) {
+      codeElement = document.createElement('pre');
+      codeElement.className = 'code highlight';
 
-      const lang = lineEls[0].getAttribute('lang');
+      const lang = lineElements[0].getAttribute('lang');
       if (lang) {
-        codeEl.setAttribute('lang', lang);
+        codeElement.setAttribute('lang', lang);
       }
     } else {
-      codeEl = document.createElement('code');
+      codeElement = document.createElement('code');
     }
 
-    if (lineEls.length > 0) {
-      for (let i = 0; i < lineEls.length; i += 1) {
-        const lineEl = lineEls[i];
-        codeEl.appendChild(lineEl);
-        codeEl.appendChild(document.createTextNode('\n'));
+    if (lineElements.length > 0) {
+      for (let i = 0; i < lineElements.length; i += 1) {
+        const lineElement = lineElements[i];
+        codeElement.appendChild(lineElement);
+        codeElement.appendChild(document.createTextNode('\n'));
       }
     } else {
-      codeEl.appendChild(documentFragment);
+      codeElement.appendChild(documentFragment);
     }
 
-    return codeEl;
+    return codeElement;
   }
 
   static nodeToGFM(node, respectWhitespaceParam = false) {

app/assets/javascripts/diff.js

@@ -24,7 +24,8 @@ class Diff {
     if (!isBound) {
       $(document)
         .on('click', '.js-unfold', this.handleClickUnfold.bind(this))
-        .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this));
+        .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this))
+        .on('mousedown', 'td.line_content.parallel', this.handleParallelLineDown.bind(this));
       isBound = true;
     }
 
@@ -100,6 +101,18 @@ class Diff {
     this.highlightSelectedLine();
   }
 
+  handleParallelLineDown(e) {
+    const line = $(e.currentTarget);
+    const table = line.closest('table');
+
+    table.removeClass('left-side-selected right-side-selected');
+
+    const lineClass = ['left-side', 'right-side'].filter(name => line.hasClass(name))[0];
+    if (lineClass) {
+      table.addClass(`${lineClass}-selected`);
+    }
+  }
+
   diffViewType() {
     return $('.inline-parallel-buttons a.active').data('view-type');
   }

app/assets/stylesheets/pages/diff.scss

@@ -77,6 +77,18 @@
           word-wrap: break-word;
         }
       }
+
+      &.left-side-selected {
+        td.line_content.parallel.right-side {
+          @include user-select(none);
+        }
+      }
+
+      &.right-side-selected {
+        td.line_content.parallel.left-side {
+          @include user-select(none);
+        }
+      }
     }
 
     tr.line_holder.parallel {

app/helpers/diff_helper.rb

@@ -33,19 +33,21 @@ module DiffHelper
   end
 
   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
+    content_line_class = %w[line_content match]
+    content_line_class << 'parallel' if view == :parallel
+
+    line_num_class = %w[diff-line-num unfold js-unfold]
+    line_num_class << '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
+      html << content_tag(:td, '...', class: [*line_num_class, 'old_line'], data: { linenumber: old_pos })
+      html << content_tag(:td, text, class: [*content_line_class, 'left-side']) if view == :parallel
     end
 
     if new_pos
-      html << content_tag(:td, '...', class: cls + ['new_line'], data: { linenumber: new_pos })
-      html << content
+      html << content_tag(:td, '...', class: [*line_num_class, 'new_line'], data: { linenumber: new_pos })
+      html << content_tag(:td, text, class: [*content_line_class, ('right-side' if view == :parallel)])
     end
 
     html.html_safe

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

@@ -5,25 +5,24 @@
       = 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
-    - line_content = capture do
-      %td.line_content.noteable_line{ class: line_class }==#{' ' * @form.indent}#{line}
-    %tr.line_holder.diff-expanded{ id: line_old, class: line_class }
+    - line_number_new = index + @form.since
+    - line_number_old = line_number_new - @form.offset
+    - line[0, 0] = ' ' * @form.indent
+    %tr.line_holder.diff-expanded{ id: line_number_old, class: line_class }
       - case diff_view
       - when :inline
-        %td.old_line.diff-line-num{ data: { linenumber: line_old } }
-          %a{ href: "#", data: { linenumber: line_old }, disabled: true }
-        %td.new_line.diff-line-num{ data: { linenumber: line_new } }
-          %a{ href: "#", data: { linenumber: line_new }, disabled: true }
-        = line_content
+        %td.old_line.diff-line-num{ data: { linenumber: line_number_old } }
+          %a{ href: "#", data: { linenumber: line_number_old }, disabled: true }
+        %td.new_line.diff-line-num{ data: { linenumber: line_number_new } }
+          %a{ href: "#", data: { linenumber: line_number_new }, disabled: true }
+        %td.line_content.noteable_line{ class: line_class }= line
       - when :parallel
-        %td.old_line.diff-line-num{ data: { linenumber: line_old } }
-          %a{ href: "##{line_old}", data: { linenumber: line_old }, disabled: true }
-        = line_content
-        %td.new_line.diff-line-num{ data: { linenumber: line_new } }
-          %a{ href: "##{line_new}", data: { linenumber: line_new }, disabled: true }
-        = line_content
+        %td.old_line.diff-line-num{ data: { linenumber: line_number_old } }
+          %a{ href: "##{line_number_old}", data: { linenumber: line_number_old }, disabled: true }
+        %td.line_content.noteable_line.left-side{ class: line_class }= line
+        %td.new_line.diff-line-num{ data: { linenumber: line_number_new } }
+          %a{ href: "##{line_number_new}", data: { linenumber: line_number_new }, disabled: true }
+        %td.line_content.noteable_line.right-side{ class: line_class }= line
 
   - if @form.unfold? && @form.bottom? && @form.to < @blob.lines.size
     %tr.line_holder{ id: @form.to, class: line_class }

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

@@ -14,20 +14,20 @@
             = diff_match_line left.old_pos, nil, text: left.text, view: :parallel
           - when 'old-nonewline', 'new-nonewline'
             %td.old_line.diff-line-num
-            %td.line_content.match= left.text
+            %td.line_content.match.left-side= left.text
           - else
             - left_line_code = diff_file.line_code(left)
             - left_position = diff_file.position(left)
-            %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } }
+            %td.old_line.diff-line-num.js-avatar-container{ class: left.type, data: { linenumber: left.old_pos } }
               = add_diff_note_button(left_line_code, left_position, 'old')
               %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } }
               - discussion_left = discussions_left.try(:first)
               - if discussion_left && discussion_left.resolvable?
                 %diff-note-avatars{ "discussion-id" => discussion_left.id }
-            %td.line_content.parallel.noteable_line{ class: left.type }= diff_line_content(left.text)
+            %td.line_content.parallel.noteable_line.left-side{ id: left_line_code, class: left.type }= diff_line_content(left.text)
         - else
           %td.old_line.diff-line-num.empty-cell
-          %td.line_content.parallel
+          %td.line_content.parallel.left-side
 
         - if right
           - case right.type
@@ -35,20 +35,20 @@
             = diff_match_line nil, right.new_pos, text: left.text, view: :parallel
           - when 'old-nonewline', 'new-nonewline'
             %td.new_line.diff-line-num
-            %td.line_content.match= right.text
+            %td.line_content.match.right-side= right.text
           - else
             - right_line_code = diff_file.line_code(right)
             - right_position = diff_file.position(right)
-            %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } }
+            %td.new_line.diff-line-num.js-avatar-container{ class: right.type, data: { linenumber: right.new_pos } }
               = add_diff_note_button(right_line_code, right_position, 'new')
               %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } }
               - discussion_right = discussions_right.try(:first)
               - if discussion_right && discussion_right.resolvable?
                 %diff-note-avatars{ "discussion-id" => discussion_right.id }
-            %td.line_content.parallel.noteable_line{ class: right.type }= diff_line_content(right.text)
+            %td.line_content.parallel.noteable_line.right-side{ id: right_line_code, class: right.type }= diff_line_content(right.text)
         - else
           %td.old_line.diff-line-num.empty-cell
-          %td.line_content.parallel
+          %td.line_content.parallel.right-side
 
       - if discussions_left || discussions_right
         = render "discussions/parallel_diff_discussion", discussions_left: discussions_left, discussions_right: discussions_right

changelogs/unreleased/dm-copy-parallel-diff.yml

+---
+title: Only copy old/new code when selecting left/right side of parallel diff
+merge_request:
+author:
+type: added

features/steps/shared/diff_note.rb

@@ -232,7 +232,7 @@ module SharedDiffNote
   end
 
   def click_parallel_diff_line(code, line_type)
-    find(".line_holder.parallel .diff-line-num[id='#{code}']").trigger 'mouseover'
+    find(".line_holder.parallel td[id='#{code}']").find(:xpath, 'preceding-sibling::*[1][self::td]').trigger 'mouseover'
     find(".line_holder.parallel button[data-line-code='#{code}']").trigger 'click'
   end
 end

spec/features/copy_as_gfm_spec.rb

@@ -446,7 +446,7 @@ describe 'Copy as GFM', js: true do
     def verify(label, *gfms)
       aggregate_failures(label) do
         gfms.each do |gfm|
-          html = gfm_to_html(gfm)
+          html = gfm_to_html(gfm).gsub(/\A
|
\z/, '')
           output_gfm = html_to_gfm(html)
           expect(output_gfm.strip).to eq(gfm.strip)
         end
@@ -463,42 +463,98 @@ describe 'Copy as GFM', js: true do
     let(:project) { create(:project, :repository) }
 
     context 'from a diff' do
-      before do
-        visit project_commit_path(project, sample_commit.id)
-      end
+      shared_examples 'copying code from a diff' do
+        context 'selecting one word of text' do
+          it 'copies as inline code' do
+            verify(
+              '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"] .line .no',
 
-      context 'selecting one word of text' do
-        it 'copies as inline code' do
-          verify(
-            '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"] .line .no',
+              '`RuntimeError`',
 
-            '`RuntimeError`'
-          )
+              target: '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'
+            )
+          end
         end
-      end
 
-      context 'selecting one line of text' do
-        it 'copies as inline code' do
-          verify(
-            '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"] .line',
+        context 'selecting one line of text' do
+          it 'copies as inline code' do
+            verify(
+              '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]',
 
-            '`raise RuntimeError, "System commands must be given as an array of strings"`'
-          )
+              '`raise RuntimeError, "System commands must be given as an array of strings"`',
+
+              target: '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'
+            )
+          end
+        end
+
+        context 'selecting multiple lines of text' do
+          it 'copies as a code block' do
+            verify(
+              '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"]',
+
+              <<-GFM.strip_heredoc,
+                ```ruby
+                      raise RuntimeError, "System commands must be given as an array of strings"
+                    end
+                ```
+              GFM
+
+              target: '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]'
+            )
+          end
         end
       end
 
-      context 'selecting multiple lines of text' do
-        it 'copies as a code block' do
-          verify(
-            '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"]',
+      context 'inline diff' do
+        before do
+          visit project_commit_path(project, sample_commit.id, view: 'inline')
+        end
 
-            <<-GFM.strip_heredoc,
-              ```ruby
-                    raise RuntimeError, "System commands must be given as an array of strings"
-                  end
-              ```
-            GFM
-          )
+        it_behaves_like 'copying code from a diff'
+      end
+
+      context 'parallel diff' do
+        before do
+          visit project_commit_path(project, sample_commit.id, view: 'parallel')
+        end
+
+        it_behaves_like 'copying code from a diff'
+
+        context 'selecting code on the left' do
+          it 'copies as a code block' do
+            verify(
+              '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"]',
+
+              <<-GFM.strip_heredoc,
+                ```ruby
+                    unless cmd.is_a?(Array)
+                      raise "System commands must be given as an array of strings"
+                    end
+                ```
+              GFM
+
+              target: '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"].left-side'
+            )
+          end
+        end
+
+        context 'selecting code on the right' do
+          it 'copies as a code block' do
+            verify(
+              '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"], [id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10"]',
+
+              <<-GFM.strip_heredoc,
+                ```ruby
+                    unless cmd.is_a?(Array)
+                      raise RuntimeError, "System commands must be given as an array of strings"
+                    end
+                ```
+              GFM
+
+              target: '[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8"].right-side'
+            )
+          end
         end
       end
     end
@@ -587,9 +643,9 @@ describe 'Copy as GFM', js: true do
       end
     end
 
-    def verify(selector, gfm)
+    def verify(selector, gfm, target: nil)
       html = html_for_selector(selector)
-      output_gfm = html_to_gfm(html, 'transformCodeSelection')
+      output_gfm = html_to_gfm(html, 'transformCodeSelection', target: target)
       expect(output_gfm.strip).to eq(gfm.strip)
     end
   end
@@ -605,15 +661,21 @@ describe 'Copy as GFM', js: true do
     page.evaluate_script(js)
   end
 
-  def html_to_gfm(html, transformer = 'transformGFMSelection')
+  def html_to_gfm(html, transformer = 'transformGFMSelection', target: nil)
     js = <<-JS.strip_heredoc
       (function(html) {
         var transformer = window.gl.CopyAsGFM[#{transformer.inspect}];
 
         var node = document.createElement('div');
-        node.innerHTML = html;
+        $(html).each(function() { node.appendChild(this) });
+
+        var targetSelector = #{target.to_json};
+        var target;
+        if (targetSelector) {
+          target = document.querySelector(targetSelector);
+        }
 
-        node = transformer(node);
+        node = transformer(node, target);
         if (!node) return null;
 
         return window.gl.CopyAsGFM.nodeToGFM(node);

spec/features/merge_requests/diff_notes_avatars_spec.rb

@@ -84,7 +84,7 @@ feature 'Diff note avatars', js: true do
       end
 
       it 'shows note avatar' do
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           find('.diff-notes-collapse').click
 
           expect(page).to have_selector('img.js-diff-comment-avatar', count: 1)
@@ -92,7 +92,7 @@ feature 'Diff note avatars', js: true do
       end
 
       it 'shows comment on note avatar' do
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           find('.diff-notes-collapse').click
 
           expect(first('img.js-diff-comment-avatar')["data-original-title"]).to eq("#{note.author.name}: #{note.note.truncate(17)}")
@@ -100,13 +100,13 @@ feature 'Diff note avatars', js: true do
       end
 
       it 'toggles comments when clicking avatar' do
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           find('.diff-notes-collapse').click
         end
 
         expect(page).to have_selector('.notes_holder', visible: false)
 
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           first('img.js-diff-comment-avatar').click
         end
 
@@ -122,7 +122,7 @@ feature 'Diff note avatars', js: true do
 
         wait_for_requests
 
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           expect(page).not_to have_selector('img.js-diff-comment-avatar')
         end
       end
@@ -138,7 +138,7 @@ feature 'Diff note avatars', js: true do
           wait_for_requests
         end
 
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           find('.diff-notes-collapse').trigger('click')
 
           expect(page).to have_selector('img.js-diff-comment-avatar', count: 2)
@@ -158,7 +158,7 @@ feature 'Diff note avatars', js: true do
           end
         end
 
-        page.within find("[id='#{position.line_code(project.repository)}']") do
+        page.within find_line(position.line_code(project.repository)) do
           find('.diff-notes-collapse').trigger('click')
 
           expect(page).to have_selector('img.js-diff-comment-avatar', count: 3)
@@ -176,7 +176,7 @@ feature 'Diff note avatars', js: true do
         end
 
         it 'shows extra comment count' do
-          page.within find("[id='#{position.line_code(project.repository)}']") do
+          page.within find_line(position.line_code(project.repository)) do
             find('.diff-notes-collapse').click
 
             expect(find('.diff-comments-more-count')).to have_content '+1'
@@ -185,4 +185,10 @@ feature 'Diff note avatars', js: true do
       end
     end
   end
+
+  def find_line(line_code)
+    line = find("[id='#{line_code}']")
+    line = line.find(:xpath, 'preceding-sibling::*[1][self::td]') if line.tag_name == 'td'
+    line
+  end
 end