From b6d545df51d7c3b4e572b40cdafb7efeb78edaf4 Mon Sep 17 00:00:00 2001 From: Tim Masliuchenko Date: Thu, 21 Jul 2016 21:44:12 +0300 Subject: Add unfold links for Side-by-Side view --- CHANGELOG | 1 + app/assets/javascripts/diff.js | 29 +++----- app/controllers/projects/blob_controller.rb | 2 + app/helpers/diff_helper.rb | 39 +++++++---- app/views/projects/blob/diff.html.haml | 34 ++++++---- app/views/projects/diffs/_content.html.haml | 2 +- app/views/projects/diffs/_diffs.html.haml | 2 +- app/views/projects/diffs/_line.html.haml | 3 +- app/views/projects/diffs/_match_line.html.haml | 7 -- app/views/projects/diffs/_parallel_view.html.haml | 10 +-- app/views/projects/diffs/_text_file.html.haml | 5 +- app/views/projects/merge_requests/_show.html.haml | 2 +- features/project/merge_requests.feature | 9 +++ features/steps/project/merge_requests.rb | 3 + spec/helpers/diff_helper_spec.rb | 80 ++++++++++++++++------- 15 files changed, 139 insertions(+), 89 deletions(-) delete mode 100644 app/views/projects/diffs/_match_line.html.haml diff --git a/CHANGELOG b/CHANGELOG index 73812e16d6f..89336cf484c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -59,6 +59,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.3 - Fix Import/Export issue importing milestones and labels not associated properly. !5426 diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 298f3852085..3dd7ceba92f 100644 --- a/app/assets/javascripts/diff.js +++ b/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; diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index eda3727a28d..19d051720e9 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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] diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index cc7121b1163..f3c9ea074b4 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -13,12 +13,11 @@ module DiffHelper end def diff_view - diff_views = %w(inline parallel) - - if diff_views.include?(cookies[:diff_view]) - cookies[:diff_view] - else - diff_views.first + @diff_view ||= begin + diff_views = %w(inline parallel) + 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' : '' - 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 + + 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 diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index 5926d181ba3..a79ae53c780 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -1,20 +1,30 @@ - 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 } - %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 = 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}" + = 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 diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index a1b071f130c..d37961c4e40 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/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 diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 20dc280c3b2..f707fa0ee5a 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,6 +1,6 @@ - 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 diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index 4d3af905b58..2d6a370b848 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/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 diff --git a/app/views/projects/diffs/_match_line.html.haml b/app/views/projects/diffs/_match_line.html.haml deleted file mode 100644 index d6dddd97879..00000000000 --- a/app/views/projects/diffs/_match_line.html.haml +++ /dev/null @@ -1,7 +0,0 @@ -%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 diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 7f30faa20d8..28aad3f4725 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,14 +1,15 @@ / 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 diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 5970b9abf2b..ab5463ba89d 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/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 diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 873ed9b59ee..269198adf91 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/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)} diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c17..6bac6011467 100644 --- a/features/project/merge_requests.feature +++ b/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 diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index da848afd48e..a02a54923a5 100644 --- a/features/steps/project/merge_requests.rb +++ b/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 diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 4949280d641..b6554de1c64 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -15,22 +15,22 @@ 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 - + describe 'diff_options' do it 'should return no collapse false' do expect(diff_options).to include(no_collapse: false) @@ -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 -- cgit v1.2.1