Commit b6d545df authored by Tim Masliuchenko's avatar Tim Masliuchenko

Add unfold links for Side-by-Side view

parent 95e5b463
...@@ -59,6 +59,7 @@ v 8.11.0 (unreleased) ...@@ -59,6 +59,7 @@ v 8.11.0 (unreleased)
- Fix RequestProfiler::Middleware error when code is reloaded in development - Fix RequestProfiler::Middleware error when code is reloaded in development
- Catch what warden might throw when profiling requests to re-throw it - 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 - 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 v 8.10.3
- Fix Import/Export issue importing milestones and labels not associated properly. !5426 - Fix Import/Export issue importing milestones and labels not associated properly. !5426
......
...@@ -10,7 +10,7 @@ ...@@ -10,7 +10,7 @@
$(document).off('click', '.js-unfold'); $(document).off('click', '.js-unfold');
$(document).on('click', '.js-unfold', (function(_this) { $(document).on('click', '.js-unfold', (function(_this) {
return function(event) { 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); target = $(event.target);
unfoldBottom = target.hasClass('js-unfold-bottom'); unfoldBottom = target.hasClass('js-unfold-bottom');
unfold = true; unfold = true;
...@@ -31,14 +31,16 @@ ...@@ -31,14 +31,16 @@
unfold = false; unfold = false;
} }
} }
link = target.parents('.diff-file').attr('data-blob-diff-path'); file = target.parents('.diff-file');
link = file.data('blob-diff-path');
params = { params = {
since: since, since: since,
to: to, to: to,
bottom: unfoldBottom, bottom: unfoldBottom,
offset: offset, offset: offset,
unfold: unfold, unfold: unfold,
indent: 1 indent: 1,
view: file.data('view')
}; };
return $.get(link, params, function(response) { return $.get(link, params, function(response) {
return target.parent().replaceWith(response); return target.parent().replaceWith(response);
...@@ -48,26 +50,13 @@ ...@@ -48,26 +50,13 @@
} }
Diff.prototype.lineNumbers = function(line) { Diff.prototype.lineNumbers = function(line) {
var i, l, len, line_number, line_numbers, lines, results;
if (!line.children().length) { if (!line.children().length) {
return [0, 0]; return [0, 0];
} }
lines = line.children().slice(0, 2);
line_numbers = (function() { return line.find('.diff-line-num').map(function() {
var i, len, results; return parseInt($(this).data('linenumber'));
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 Diff; return Diff;
......
...@@ -76,6 +76,8 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -76,6 +76,8 @@ class Projects::BlobController < Projects::ApplicationController
end end
def diff def diff
apply_diff_view_cookie!
@form = UnfoldForm.new(params) @form = UnfoldForm.new(params)
@lines = Gitlab::Highlight.highlight_lines(repository, @ref, @path) @lines = Gitlab::Highlight.highlight_lines(repository, @ref, @path)
@lines = @lines[@form.since - 1..@form.to - 1] @lines = @lines[@form.since - 1..@form.to - 1]
......
...@@ -13,12 +13,11 @@ module DiffHelper ...@@ -13,12 +13,11 @@ module DiffHelper
end end
def diff_view def diff_view
@diff_view ||= begin
diff_views = %w(inline parallel) diff_views = %w(inline parallel)
diff_view = cookies[:diff_view]
if diff_views.include?(cookies[:diff_view]) diff_view = diff_views.first unless diff_views.include?(diff_view)
cookies[:diff_view] diff_view.to_sym
else
diff_views.first
end end
end end
...@@ -33,12 +32,23 @@ module DiffHelper ...@@ -33,12 +32,23 @@ module DiffHelper
options options
end end
def unfold_bottom_class(bottom) def diff_match_line(old_pos, new_pos, text: '', view: :inline, bottom: false)
bottom ? 'js-unfold js-unfold-bottom' : '' 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 end
def unfold_class(unfold) html.html_safe
unfold ? 'unfold js-unfold' : ''
end end
def diff_line_content(line, line_type = nil) def diff_line_content(line, line_type = nil)
...@@ -67,11 +77,11 @@ module DiffHelper ...@@ -67,11 +77,11 @@ module DiffHelper
end end
def inline_diff_btn def inline_diff_btn
diff_btn('Inline', 'inline', diff_view == 'inline') diff_btn('Inline', 'inline', diff_view == :inline)
end end
def parallel_diff_btn def parallel_diff_btn
diff_btn('Side-by-side', 'parallel', diff_view == 'parallel') diff_btn('Side-by-side', 'parallel', diff_view == :parallel)
end end
def submodule_link(blob, ref, repository = @repository) def submodule_link(blob, ref, repository = @repository)
...@@ -103,7 +113,8 @@ module DiffHelper ...@@ -103,7 +113,8 @@ module DiffHelper
commit = commit_for_diff(diff_file) commit = commit_for_diff(diff_file)
{ {
blob_diff_path: namespace_project_blob_diff_path(project.namespace, project, 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 end
......
- if @lines.present? - if @lines.present?
- line_class = diff_view == :inline ? '' : diff_view
- if @form.unfold? && @form.since != 1 && !@form.bottom? - if @form.unfold? && @form.since != 1 && !@form.bottom?
%tr.line_holder %tr.line_holder{ class: line_class }
= render "projects/diffs/match_line", { line: @match_line, = diff_match_line @form.since, @form.since, text: @match_line, view: diff_view
line_old: @form.since, line_new: @form.since, bottom: false, new_file: false }
- @lines.each_with_index do |line, index| - @lines.each_with_index do |line, index|
- line_new = index + @form.since - line_new = index + @form.since
- line_old = line_new - @form.offset - 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 } } %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}" = link_to raw(line_old), "##{line_old}"
%td.new_line.diff-line-num{ data: { linenumber: line_old } } = line_content
= link_to raw(line_new) , "##{line_old}" %td.new_line.diff-line-num{data: { linenumber: line_new }}
%td.line_content.noteable_line==#{' ' * @form.indent}#{line} = link_to raw(line_new), "##{line_new}"
= line_content
- if @form.unfold? && @form.bottom? && @form.to < @blob.loc - if @form.unfold? && @form.bottom? && @form.to < @blob.loc
%tr.line_holder{ id: @form.to } %tr.line_holder{ id: @form.to, class: line_class }
= render "projects/diffs/match_line", { line: @match_line, = diff_match_line @form.to, @form.to, text: @match_line, view: diff_view, bottom: true
line_old: @form.to, line_new: @form.to, bottom: true, new_file: false }
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
.nothing-here-block.diff-collapsed{data: { diff_for_path: url } } .nothing-here-block.diff-collapsed{data: { diff_for_path: url } }
This diff is collapsed. Click to expand it. This diff is collapsed. Click to expand it.
- elsif diff_file.diff_lines.length > 0 - 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 = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
- else - else
= render "projects/diffs/text_file", diff_file: diff_file = render "projects/diffs/text_file", diff_file: diff_file
......
- show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
- diff_files = diffs.diff_files - diff_files = diffs.diff_files
- if diff_view == 'parallel' - if diff_view == :parallel
- fluid_layout true - fluid_layout true
.content-block.oneline-block.files-changed .content-block.oneline-block.files-changed
......
...@@ -4,8 +4,7 @@ ...@@ -4,8 +4,7 @@
%tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } } %tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } }
- case type - case type
- when 'match' - when 'match'
= render "projects/diffs/match_line", { line: line.text, = diff_match_line line.old_pos, line.new_pos, text: line.text
line_old: line.old_pos, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file }
- when 'nonewline' - when 'nonewline'
%td.old_line.diff-line-num %td.old_line.diff-line-num
%td.new_line.diff-line-num %td.new_line.diff-line-num
......
%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
/ Side-by-side diff view / Side-by-side diff view
%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ data: diff_view_data } %div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ data: diff_view_data }
%table %table
- last_line = 0
- diff_file.parallel_diff_lines.each do |line| - diff_file.parallel_diff_lines.each do |line|
- left = line[:left] - left = line[:left]
- right = line[:right] - right = line[:right]
- last_line = right.new_pos if right
%tr.line_holder.parallel %tr.line_holder.parallel
- if left - if left
- if left.meta? - if left.meta?
%td.old_line.diff-line-num.empty-cell = diff_match_line left.old_pos, nil, text: left.text, view: :parallel
%td.line_content.parallel.match= left.text
- else - else
- left_line_code = diff_file.line_code(left) - left_line_code = diff_file.line_code(left)
- left_position = diff_file.position(left) - left_position = diff_file.position(left)
...@@ -21,8 +22,7 @@ ...@@ -21,8 +22,7 @@
- if right - if right
- if right.meta? - if right.meta?
%td.old_line.diff-line-num.empty-cell = diff_match_line nil, right.new_pos, text: left.text, view: :parallel
%td.line_content.parallel.match= left.text
- else - else
- right_line_code = diff_file.line_code(right) - right_line_code = diff_file.line_code(right)
- right_position = diff_file.position(right) - right_position = diff_file.position(right)
...@@ -37,3 +37,5 @@ ...@@ -37,3 +37,5 @@
- discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file)
- if discussion_left || discussion_right - if discussion_left || discussion_right
= render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: 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
...@@ -15,6 +15,5 @@ ...@@ -15,6 +15,5 @@
- if discussion - if discussion
= render "discussions/diff_discussion", discussion: discussion = render "discussions/diff_discussion", discussion: discussion
- if last_line > 0 - if !diff_file.new_file && last_line > 0
= render "projects/diffs/match_line", { line: "", = diff_match_line last_line, last_line, bottom: true
line_old: last_line, line_new: last_line, bottom: true, new_file: diff_file.new_file }
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
- page_description @merge_request.description - page_description @merge_request.description
- page_card_attributes @merge_request.card_attributes - page_card_attributes @merge_request.card_attributes
- if diff_view == 'parallel' - if diff_view == :parallel
- fluid_layout true - fluid_layout true
.merge-request{'data-url' => merge_request_path(@merge_request)} .merge-request{'data-url' => merge_request_path(@merge_request)}
......
...@@ -236,6 +236,15 @@ Feature: Project Merge Requests ...@@ -236,6 +236,15 @@ Feature: Project Merge Requests
And I unfold diff And I unfold diff
Then I should see additional file lines 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 @javascript
Scenario: I show comments on a merge request side-by-side diff with comments in multiple files 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 Given project "Shop" have "Bug NS-05" open merge request with diffs inside
......
...@@ -477,6 +477,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps ...@@ -477,6 +477,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
step 'I click Side-by-side Diff tab' do step 'I click Side-by-side Diff tab' do
find('a', text: 'Side-by-side').trigger('click') find('a', text: 'Side-by-side').trigger('click')
# Waits for load
expect(page).to have_css('.parallel')
end end
step 'I should see comments on the side-by-side diff page' do step 'I should see comments on the side-by-side diff page' do
......
...@@ -15,19 +15,19 @@ describe DiffHelper do ...@@ -15,19 +15,19 @@ describe DiffHelper do
it 'returns a valid value when cookie is set' do it 'returns a valid value when cookie is set' do
helper.request.cookies[:diff_view] = 'parallel' helper.request.cookies[:diff_view] = 'parallel'
expect(helper.diff_view).to eq 'parallel' expect(helper.diff_view).to eq :parallel
end end
it 'returns a default value when cookie is invalid' do it 'returns a default value when cookie is invalid' do
helper.request.cookies[:diff_view] = 'invalid' helper.request.cookies[:diff_view] = 'invalid'
expect(helper.diff_view).to eq 'inline' expect(helper.diff_view).to eq :inline
end end
it 'returns a default value when cookie is nil' do it 'returns a default value when cookie is nil' do
expect(helper.request.cookies).to be_empty expect(helper.request.cookies).to be_empty
expect(helper.diff_view).to eq 'inline' expect(helper.diff_view).to eq :inline
end end
end end
...@@ -59,26 +59,6 @@ describe DiffHelper do ...@@ -59,26 +59,6 @@ describe DiffHelper do
end end
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 describe '#diff_line_content' do
it 'should return non breaking space when line is empty' do it 'should return non breaking space when line is empty' do
expect(diff_line_content(nil)).to eq(' &nbsp;') expect(diff_line_content(nil)).to eq(' &nbsp;')
...@@ -105,4 +85,56 @@ describe DiffHelper do ...@@ -105,4 +85,56 @@ describe DiffHelper do
expect(marked_new_line).to be_html_safe expect(marked_new_line).to be_html_safe
end end
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment