From 218219abbdfdc3bc0bc1a9c95cfc0e0ddb5861dd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 8 Sep 2014 21:54:52 +0300 Subject: Refactoring inside refactoring. We need to go deeper Signed-off-by: Dmitriy Zaporozhets --- app/controllers/projects/edit_tree_controller.rb | 2 +- app/helpers/commits_helper.rb | 56 ------------------ app/helpers/diff_helper.rb | 69 ++++++++++++++++++++-- app/models/note.rb | 22 +++++-- app/views/projects/diffs/_diff_file.html.haml | 47 --------------- app/views/projects/diffs/_diff_stats.html.haml | 41 ------------- app/views/projects/diffs/_diff_warning.html.haml | 19 ------ app/views/projects/diffs/_diffs.html.haml | 10 ++-- app/views/projects/diffs/_file.html.haml | 47 +++++++++++++++ app/views/projects/diffs/_stats.html.haml | 41 +++++++++++++ app/views/projects/diffs/_text_file.html.haml | 2 +- app/views/projects/diffs/_warning.html.haml | 19 ++++++ app/views/projects/edit_tree/_diff.html.haml | 13 ---- app/views/projects/edit_tree/preview.html.haml | 7 +-- app/views/projects/notes/_diff_note_link.html.haml | 10 ---- .../projects/notes/discussions/_diff.html.haml | 7 ++- 16 files changed, 204 insertions(+), 208 deletions(-) delete mode 100644 app/views/projects/diffs/_diff_file.html.haml delete mode 100644 app/views/projects/diffs/_diff_stats.html.haml delete mode 100644 app/views/projects/diffs/_diff_warning.html.haml create mode 100644 app/views/projects/diffs/_file.html.haml create mode 100644 app/views/projects/diffs/_stats.html.haml create mode 100644 app/views/projects/diffs/_warning.html.haml delete mode 100644 app/views/projects/edit_tree/_diff.html.haml delete mode 100644 app/views/projects/notes/_diff_note_link.html.haml (limited to 'app') diff --git a/app/controllers/projects/edit_tree_controller.rb b/app/controllers/projects/edit_tree_controller.rb index baae12d92df..72a41f771c0 100644 --- a/app/controllers/projects/edit_tree_controller.rb +++ b/app/controllers/projects/edit_tree_controller.rb @@ -31,7 +31,7 @@ class Projects::EditTreeController < Projects::BaseTreeController diffy = Diffy::Diff.new(@blob.data, @content, diff: '-U 3', include_diff_info: true) - @diff_lines = Gitlab::Diff::Parser.new.parse(diffy.diff.scan(/.*\n/), @path, @path) + @diff_lines = Gitlab::Diff::Parser.new.parse(diffy.diff.scan(/.*\n/)) render layout: false end diff --git a/app/helpers/commits_helper.rb b/app/helpers/commits_helper.rb index 49226a37d08..90829963e84 100644 --- a/app/helpers/commits_helper.rb +++ b/app/helpers/commits_helper.rb @@ -16,62 +16,6 @@ module CommitsHelper commit_person_link(commit, options.merge(source: :committer)) end - def parallel_diff(diff_file, index) - lines = [] - skip_next = false - - # Building array of lines - # - # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content] - # - diff_file.diff_lines.each do |line| - - full_line = line.text - type = line.type - line_code = line.code - line_new = line.new_pos - line_old = line.old_pos - - next_line = diff_file.next_line(line.index) - - if next_line - next_type = next_line.type - next_line = next_line.text - end - - line = [type, line_old, full_line, next_type, line_new] - if type == 'match' || type.nil? - # line in the right panel is the same as in the left one - line = [type, line_old, full_line, type, line_new, full_line] - lines.push(line) - elsif type == 'old' - if next_type == 'new' - # Left side has text removed, right side has text added - line.push(next_line) - lines.push(line) - skip_next = true - elsif next_type == 'old' || next_type.nil? - # Left side has text removed, right side doesn't have any change - line.pop # remove the newline - line.push(nil) # no line number on the right panel - line.push(" ") # empty line on the right panel - lines.push(line) - end - elsif type == 'new' - if skip_next - # Change has been already included in previous line so no need to do it again - skip_next = false - next - else - # Change is only on the right side, left side has no change - line = [nil, nil, " ", type, line_new, full_line] - lines.push(line) - end - end - end - lines - end - def image_diff_class(diff) if diff.deleted_file "deleted" diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 7feb07eeb3b..c2a19e4ac10 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -1,16 +1,16 @@ module DiffHelper - def safe_diff_files(project, diffs) + def safe_diff_files(diffs) if diff_hard_limit_enabled? diffs.first(Commit::DIFF_HARD_LIMIT_FILES) else diffs.first(Commit::DIFF_SAFE_FILES) end.map do |diff| - Gitlab::Diff::File.new(project, @commit, diff) + Gitlab::Diff::File.new(diff) end end - def show_diff_size_warninig?(project, diffs) - safe_diff_files(project, diffs).size < diffs.size + def show_diff_size_warninig?(diffs) + safe_diff_files(diffs).size < diffs.size end def diff_hard_limit_enabled? @@ -21,4 +21,65 @@ module DiffHelper false end end + + def generate_line_code(file_path, line) + Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + end + + def parallel_diff(diff_file, index) + lines = [] + skip_next = false + + # Building array of lines + # + # [left_type, left_line_number, left_line_content, right_line_type, right_line_number, right_line_content] + # + diff_file.diff_lines.each do |line| + + full_line = line.text + type = line.type + line_code = generate_line_code(diff_file.file_path, line) + line_new = line.new_pos + line_old = line.old_pos + + next_line = diff_file.next_line(line.index) + + if next_line + next_type = next_line.type + next_line = next_line.text + end + + line = [type, line_old, full_line, next_type, line_new] + if type == 'match' || type.nil? + # line in the right panel is the same as in the left one + line = [type, line_old, full_line, type, line_new, full_line] + lines.push(line) + elsif type == 'old' + if next_type == 'new' + # Left side has text removed, right side has text added + line.push(next_line) + lines.push(line) + skip_next = true + elsif next_type == 'old' || next_type.nil? + # Left side has text removed, right side doesn't have any change + line.pop # remove the newline + line.push(nil) # no line number on the right panel + line.push(" ") # empty line on the right panel + lines.push(line) + end + elsif type == 'new' + if skip_next + # Change has been already included in previous line so no need to do it again + skip_next = false + next + else + # Change is only on the right side, left side has no change + line = [nil, nil, " ", type, line_new, full_line] + lines.push(line) + end + end + end + lines + end + end diff --git a/app/models/note.rb b/app/models/note.rb index 77e3a528f96..fa5fdea4eb0 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -209,7 +209,7 @@ class Note < ActiveRecord::Base noteable.diffs.each do |mr_diff| next unless mr_diff.new_path == self.diff.new_path - lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a, mr_diff.old_path, mr_diff.new_path) + lines = Gitlab::Diff::Parser.new.parse(mr_diff.diff.lines.to_a) lines.each do |line| if line.text == diff_line @@ -233,6 +233,14 @@ class Note < ActiveRecord::Base diff.new_path if diff end + def file_path + if diff.new_path.present? + diff.new_path + elsif diff.old_path.present? + diff.old_path + end + end + def diff_old_line line_code.split('_')[1].to_i end @@ -241,12 +249,18 @@ class Note < ActiveRecord::Base line_code.split('_')[2].to_i end + def generate_line_code(line) + Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + end + def diff_line return @diff_line if @diff_line if diff diff_lines.each do |line| - @diff_line = line.text if line.code == self.line_code + if generate_line_code(line) == self.line_code + @diff_line = line.text + end end end @@ -259,7 +273,7 @@ class Note < ActiveRecord::Base prev_lines = [] diff_lines.each do |line| - if line.code != self.line_code + if generate_line_code(line) != self.line_code if line.type == "match" prev_lines.clear prev_match_line = line @@ -275,7 +289,7 @@ class Note < ActiveRecord::Base end def diff_lines - @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a, diff.old_path, diff.new_path) + @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.lines.to_a) end def discussion_id diff --git a/app/views/projects/diffs/_diff_file.html.haml b/app/views/projects/diffs/_diff_file.html.haml deleted file mode 100644 index aa68becc4dd..00000000000 --- a/app/views/projects/diffs/_diff_file.html.haml +++ /dev/null @@ -1,47 +0,0 @@ -- return unless diff_file.blob_exists? -- blob = diff_file.blob -- blob_diff_path = diff_project_blob_path(project, tree_join(@commit.id, diff_file.new_path)) -.diff-file{id: "diff-#{i}", data: {blob_diff_path: blob_diff_path }} - .diff-header{id: "file-path-#{hexdigest(diff_file.new_path || diff_file.old_path)}"} - - if diff_file.deleted_file - %span= diff_file.old_path - - .diff-btn-group - - if @commit.parent_ids.present? - = view_file_btn(@commit.parent_id, diff_file, project) - - else - %span= diff_file.new_path - - if diff_file.mode_changed? - %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" - - .diff-btn-group - %label - = check_box_tag nil, 1, false, class: "js-toggle-diff-line-wrap" - Wrap text -   - = link_to "#", class: "js-toggle-diff-comments btn btn-small" do - %i.icon-chevron-down - Diff comments -   - - - if @merge_request && @merge_request.source_project - = link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff_file.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do - Edit -   - - = view_file_btn(@commit.id, diff_file, project) - - .diff-content - -# Skipp all non non-supported blobs - - return unless blob.respond_to?('text?') - - if blob.text? - - if params[:view] == 'parallel' - = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i - - else - = render "projects/diffs/text_file", diff_file: diff_file, index: i - - elsif blob.image? - - old_file = project.repository.prev_blob_for_diff(@commit, diff_file) - = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i - - else - .nothing-here-block No preview for this file type - diff --git a/app/views/projects/diffs/_diff_stats.html.haml b/app/views/projects/diffs/_diff_stats.html.haml deleted file mode 100644 index 8ef7cc6e086..00000000000 --- a/app/views/projects/diffs/_diff_stats.html.haml +++ /dev/null @@ -1,41 +0,0 @@ -.js-toggle-container - .commit-stat-summary - Showing - %strong.cdark #{pluralize(diffs.count, "changed file")} - - if current_controller?(:commit) - - unless @commit.has_zero_stats? - with - %strong.cgreen #{@commit.stats.additions} additions - and - %strong.cred #{@commit.stats.deletions} deletions -   - = link_to '#', class: 'btn btn-small js-toggle-button' do - Show diff stats - %i.icon-chevron-down - .file-stats.js-toggle-content.hide - %ul.bordered-list - - diffs.each_with_index do |diff, i| - %li - - if diff.deleted_file - %span.deleted-file - %a{href: "#diff-#{i}"} - %i.icon-minus - = diff.old_path - - elsif diff.renamed_file - %span.renamed-file - %a{href: "#diff-#{i}"} - %i.icon-minus - = diff.old_path - \-> - = diff.new_path - - elsif diff.new_file - %span.new-file - %a{href: "#diff-#{i}"} - %i.icon-plus - = diff.new_path - - else - %span.edit-file - %a{href: "#diff-#{i}"} - %i.icon-adjust - = diff.new_path - diff --git a/app/views/projects/diffs/_diff_warning.html.haml b/app/views/projects/diffs/_diff_warning.html.haml deleted file mode 100644 index ee85956d876..00000000000 --- a/app/views/projects/diffs/_diff_warning.html.haml +++ /dev/null @@ -1,19 +0,0 @@ -.bs-callout.bs-callout-warning - %h4 - Too many changes. - .pull-right - - unless diff_hard_limit_enabled? - = link_to "Reload with full diff", url_for(params.merge(force_show_diff: true)), class: "btn btn-small btn-warning" - - - if current_controller?(:commit) or current_controller?(:merge_requests) - - if current_controller?(:commit) - = link_to "Plain diff", project_commit_path(@project, @commit, format: :diff), class: "btn btn-warning btn-small" - = link_to "Email patch", project_commit_path(@project, @commit, format: :patch), class: "btn btn-warning btn-small" - - elsif @merge_request && @merge_request.persisted? - = link_to "Plain diff", project_merge_request_path(@project, @merge_request, format: :diff), class: "btn btn-warning btn-small" - = link_to "Email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "btn btn-warning btn-small" - %p - To preserve performance only - %strong #{safe_diff_files(@project, diffs).size} of #{diffs.size} - files displayed. - diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 80a6d8a5691..c4eb7815866 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,6 +1,6 @@ .row .col-md-8 - = render 'projects/diffs/diff_stats', diffs: diffs + = render 'projects/diffs/stats', diffs: diffs .col-md-4 %ul.nav.nav-tabs %li.pull-right{class: params[:view] == 'parallel' ? 'active' : ''} @@ -11,12 +11,12 @@ - params_copy[:view] = 'inline' = link_to "Inline Diff", url_for(params_copy), {id: "commit-diff-viewtype"} -- if show_diff_size_warninig?(project, diffs) - = render 'projects/diffs/diff_warning', diffs: diffs +- if show_diff_size_warninig?(diffs) + = render 'projects/diffs/warning', diffs: diffs .files - - safe_diff_files(project, diffs).each_with_index do |diff_file, i| - = render 'projects/diffs/diff_file', diff_file: diff_file, i: i, project: project + - safe_diff_files(diffs).each_with_index do |diff_file, index| + = render 'projects/diffs/file', diff_file: diff_file, i: index, project: project - if @diff_timeout .alert.alert-danger diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml new file mode 100644 index 00000000000..be0301e75f8 --- /dev/null +++ b/app/views/projects/diffs/_file.html.haml @@ -0,0 +1,47 @@ +- blob = project.repository.blob_for_diff(@commit, diff_file.diff) +- return unless blob +- blob_diff_path = diff_project_blob_path(project, tree_join(@commit.id, diff_file.file_path)) +.diff-file{id: "diff-#{i}", data: {blob_diff_path: blob_diff_path }} + .diff-header{id: "file-path-#{hexdigest(diff_file.new_path || diff_file.old_path)}"} + - if diff_file.deleted_file + %span= diff_file.old_path + + .diff-btn-group + - if @commit.parent_ids.present? + = view_file_btn(@commit.parent_id, diff_file, project) + - else + %span= diff_file.new_path + - if diff_file.mode_changed? + %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}" + + .diff-btn-group + %label + = check_box_tag nil, 1, false, class: "js-toggle-diff-line-wrap" + Wrap text +   + = link_to "#", class: "js-toggle-diff-comments btn btn-small" do + %i.icon-chevron-down + Diff comments +   + + - if @merge_request && @merge_request.source_project + = link_to project_edit_tree_path(@merge_request.source_project, tree_join(@merge_request.source_branch, diff_file.new_path), from_merge_request_id: @merge_request.id), { class: 'btn btn-small' } do + Edit +   + + = view_file_btn(@commit.id, diff_file, project) + + .diff-content + -# Skipp all non non-supported blobs + - return unless blob.respond_to?('text?') + - if blob.text? + - if params[:view] == 'parallel' + = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i + - else + = render "projects/diffs/text_file", diff_file: diff_file, index: i + - elsif blob.image? + - old_file = project.repository.prev_blob_for_diff(@commit, diff_file) + = render "projects/diffs/image", diff_file: diff_file, old_file: old_file, file: blob, index: i + - else + .nothing-here-block No preview for this file type + diff --git a/app/views/projects/diffs/_stats.html.haml b/app/views/projects/diffs/_stats.html.haml new file mode 100644 index 00000000000..8ef7cc6e086 --- /dev/null +++ b/app/views/projects/diffs/_stats.html.haml @@ -0,0 +1,41 @@ +.js-toggle-container + .commit-stat-summary + Showing + %strong.cdark #{pluralize(diffs.count, "changed file")} + - if current_controller?(:commit) + - unless @commit.has_zero_stats? + with + %strong.cgreen #{@commit.stats.additions} additions + and + %strong.cred #{@commit.stats.deletions} deletions +   + = link_to '#', class: 'btn btn-small js-toggle-button' do + Show diff stats + %i.icon-chevron-down + .file-stats.js-toggle-content.hide + %ul.bordered-list + - diffs.each_with_index do |diff, i| + %li + - if diff.deleted_file + %span.deleted-file + %a{href: "#diff-#{i}"} + %i.icon-minus + = diff.old_path + - elsif diff.renamed_file + %span.renamed-file + %a{href: "#diff-#{i}"} + %i.icon-minus + = diff.old_path + \-> + = diff.new_path + - elsif diff.new_file + %span.new-file + %a{href: "#diff-#{i}"} + %i.icon-plus + = diff.new_path + - else + %span.edit-file + %a{href: "#diff-#{i}"} + %i.icon-adjust + = diff.new_path + diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 43be43cc6e9..81f726c8e4f 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -7,7 +7,7 @@ - diff_file.diff_lines.each_with_index do |line, index| - type = line.type - last_line = line.new_pos - - line_code = line.code + - line_code = generate_line_code(diff_file.file_path, line) - line_old = line.old_pos %tr.line_holder{ id: line_code, class: "#{type}" } - if type == "match" diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml new file mode 100644 index 00000000000..ee85956d876 --- /dev/null +++ b/app/views/projects/diffs/_warning.html.haml @@ -0,0 +1,19 @@ +.bs-callout.bs-callout-warning + %h4 + Too many changes. + .pull-right + - unless diff_hard_limit_enabled? + = link_to "Reload with full diff", url_for(params.merge(force_show_diff: true)), class: "btn btn-small btn-warning" + + - if current_controller?(:commit) or current_controller?(:merge_requests) + - if current_controller?(:commit) + = link_to "Plain diff", project_commit_path(@project, @commit, format: :diff), class: "btn btn-warning btn-small" + = link_to "Email patch", project_commit_path(@project, @commit, format: :patch), class: "btn btn-warning btn-small" + - elsif @merge_request && @merge_request.persisted? + = link_to "Plain diff", project_merge_request_path(@project, @merge_request, format: :diff), class: "btn btn-warning btn-small" + = link_to "Email patch", project_merge_request_path(@project, @merge_request, format: :patch), class: "btn btn-warning btn-small" + %p + To preserve performance only + %strong #{safe_diff_files(@project, diffs).size} of #{diffs.size} + files displayed. + diff --git a/app/views/projects/edit_tree/_diff.html.haml b/app/views/projects/edit_tree/_diff.html.haml deleted file mode 100644 index cf044feb9a4..00000000000 --- a/app/views/projects/edit_tree/_diff.html.haml +++ /dev/null @@ -1,13 +0,0 @@ -%table.text-file - - each_diff_line(diff, 1) do |line, type, line_code, line_new, line_old, raw_line| - %tr.line_holder{ id: line_code, class: "#{type}" } - - if type == "match" - %td.old_line= "..." - %td.new_line= "..." - %td.line_content.matched= line - - else - %td.old_line - = link_to raw(type == "new" ? " " : line_old), "##{line_code}", id: line_code - %td.new_line= link_to raw(type == "old" ? " " : line_new) , "##{line_code}", id: line_code - %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw diff_line_content(line) - diff --git a/app/views/projects/edit_tree/preview.html.haml b/app/views/projects/edit_tree/preview.html.haml index f3fd94b0a3f..e7c3460ad78 100644 --- a/app/views/projects/edit_tree/preview.html.haml +++ b/app/views/projects/edit_tree/preview.html.haml @@ -12,15 +12,14 @@ - unless @diff_lines.empty? %table.text-file - @diff_lines.each do |line| - %tr.line_holder{ id: line.code, class: "#{line.type}" } + %tr.line_holder{ class: "#{line.type}" } - if line.type == "match" %td.old_line= "..." %td.new_line= "..." %td.line_content.matched= line.text - else %td.old_line - = link_to raw(line.type == "new" ? " " : line.old_pos), "##{line.code}", id: line.code - %td.new_line= link_to raw(line.type == "old" ? " " : line.new_pos) , "##{line.code}", id: line.code - %td.line_content{class: "noteable_line #{line.type} #{line.code}", "line.code" => line.code}= raw diff_line_content(line.text) + %td.new_line + %td.line_content{class: "#{line.type}"}= raw diff_line_content(line.text) - else .nothing-here-block No changes. diff --git a/app/views/projects/notes/_diff_note_link.html.haml b/app/views/projects/notes/_diff_note_link.html.haml deleted file mode 100644 index 377c926a204..00000000000 --- a/app/views/projects/notes/_diff_note_link.html.haml +++ /dev/null @@ -1,10 +0,0 @@ -- note = @project.notes.new(@comments_target.merge({ line_code: line_code })) -= link_to "", - "javascript:;", - class: "add-diff-note js-add-diff-note-button", - data: { noteable_type: note.noteable_type, - noteable_id: note.noteable_id, - commit_id: note.commit_id, - line_code: note.line_code, - discussion_id: note.discussion_id }, - title: "Add a comment to this line" diff --git a/app/views/projects/notes/discussions/_diff.html.haml b/app/views/projects/notes/discussions/_diff.html.haml index 228af785f73..da71220af17 100644 --- a/app/views/projects/notes/discussions/_diff.html.haml +++ b/app/views/projects/notes/discussions/_diff.html.haml @@ -12,7 +12,8 @@ .diff-content %table - note.truncated_diff_lines.each do |line| - %tr.line_holder{ id: line.code } + - line_code = generate_line_code(note.file_path, line) + %tr.line_holder{ id: line_code } - if line.type == "match" %td.old_line= "..." %td.new_line= "..." @@ -20,7 +21,7 @@ - else %td.old_line= raw(line.type == "new" ? " " : line.old_pos) %td.new_line= raw(line.type == "old" ? " " : line.new_pos) - %td.line_content{class: "noteable_line #{line.type} #{line.code}", "line_code" => line.code}= raw "#{line.text}  " + %td.line_content{class: "noteable_line #{line.type} #{line_code}", "line_code" => line_code}= raw "#{line.text}  " - - if line.code == note.line_code + - if line_code == note.line_code = render "projects/notes/diff_notes_with_reply", notes: discussion_notes -- cgit v1.2.1