diff options
| -rw-r--r-- | app/assets/javascripts/copy_as_gfm.js | 61 | ||||
| -rw-r--r-- | app/assets/javascripts/diff.js | 15 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/diff.scss | 12 | ||||
| -rw-r--r-- | app/helpers/diff_helper.rb | 16 | ||||
| -rw-r--r-- | app/views/projects/blob/diff.html.haml | 31 | ||||
| -rw-r--r-- | app/views/projects/diffs/_parallel_view.html.haml | 16 | ||||
| -rw-r--r-- | changelogs/unreleased/dm-copy-parallel-diff.yml | 5 | ||||
| -rw-r--r-- | features/steps/shared/diff_note.rb | 2 | ||||
| -rw-r--r-- | spec/features/copy_as_gfm_spec.rb | 128 | ||||
| -rw-r--r-- | spec/features/merge_requests/diff_notes_avatars_spec.rb | 22 |
10 files changed, 208 insertions, 100 deletions
diff --git a/app/assets/javascripts/copy_as_gfm.js b/app/assets/javascripts/copy_as_gfm.js index e3e2c798570..93b0cbf4209 100644 --- a/app/assets/javascripts/copy_as_gfm.js +++ b/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) { diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 6a008112203..ae8338f5fd2 100644 --- a/app/assets/javascripts/diff.js +++ b/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'); } diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index e4bd783c8bc..fb23343b966 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/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 { diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 28f591a4e22..4e4a66e8a02 100644 --- a/app/helpers/diff_helper.rb +++ b/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 diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index d1d448f0d4c..ea7a71792a3 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/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 } diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 56d63250714..1f0ca211074 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/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 diff --git a/changelogs/unreleased/dm-copy-parallel-diff.yml b/changelogs/unreleased/dm-copy-parallel-diff.yml new file mode 100644 index 00000000000..96a65007661 --- /dev/null +++ b/changelogs/unreleased/dm-copy-parallel-diff.yml @@ -0,0 +1,5 @@ +--- +title: Only copy old/new code when selecting left/right side of parallel diff +merge_request: +author: +type: added diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb index 2c59ec5bb06..c872bd6f861 100644 --- a/features/steps/shared/diff_note.rb +++ b/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 diff --git a/spec/features/copy_as_gfm_spec.rb b/spec/features/copy_as_gfm_spec.rb index dfeba722ac6..ebcd0ba0dcd 100644 --- a/spec/features/copy_as_gfm_spec.rb +++ b/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); diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb index 9bcb78d5206..4766cdf716f 100644 --- a/spec/features/merge_requests/diff_notes_avatars_spec.rb +++ b/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 |