Commit b4cc04d7e17eeefe6d89bbb72661a43d7d9e3e2e
1 parent
73efb837
Exists in
master
and in
4 other branches
Commit diff fixes, per-line comments fixed
Showing
5 changed files
with
66 additions
and
83 deletions
Show diff stats
app/helpers/commits_helper.rb
| @@ -7,16 +7,6 @@ module CommitsHelper | @@ -7,16 +7,6 @@ module CommitsHelper | ||
| 7 | 7 | ||
| 8 | end | 8 | end |
| 9 | 9 | ||
| 10 | - def diff_line_class(line) | ||
| 11 | - if line[0] == "+" | ||
| 12 | - "new" | ||
| 13 | - elsif line[0] == "-" | ||
| 14 | - "old" | ||
| 15 | - else | ||
| 16 | - nil | ||
| 17 | - end | ||
| 18 | - end | ||
| 19 | - | ||
| 20 | def more_commits_link | 10 | def more_commits_link |
| 21 | offset = params[:offset] || 0 | 11 | offset = params[:offset] || 0 |
| 22 | limit = params[:limit] || 100 | 12 | limit = params[:limit] || 100 |
| @@ -42,11 +32,56 @@ module CommitsHelper | @@ -42,11 +32,56 @@ module CommitsHelper | ||
| 42 | preserve out | 32 | preserve out |
| 43 | end | 33 | end |
| 44 | 34 | ||
| 45 | - def build_line_code(line, index, line_new, line_old) | ||
| 46 | - if diff_line_class(line) == "new" | ||
| 47 | - "NEW_#{index}_#{line_new}" | 35 | + def diff_line_class(line) |
| 36 | + if line[0] == "+" | ||
| 37 | + "new" | ||
| 38 | + elsif line[0] == "-" | ||
| 39 | + "old" | ||
| 48 | else | 40 | else |
| 49 | - "OLD_#{index}_#{line_old}" | 41 | + nil |
| 42 | + end | ||
| 43 | + end | ||
| 44 | + | ||
| 45 | + def build_line_code(line, index, line_new, line_old) | ||
| 46 | + "#{index}_#{line_old}_#{line_new}" | ||
| 47 | + end | ||
| 48 | + | ||
| 49 | + def each_diff_line(diff_arr, index) | ||
| 50 | + line_old = 1 | ||
| 51 | + line_new = 1 | ||
| 52 | + type = nil | ||
| 53 | + | ||
| 54 | + lines_arr = diff_arr | ||
| 55 | + lines_arr.each do |line| | ||
| 56 | + full_line = html_escape(line.gsub(/\n/, '')) | ||
| 57 | + | ||
| 58 | + next if line.match(/^--- \/dev\/null/) | ||
| 59 | + next if line.match(/^--- a/) | ||
| 60 | + next if line.match(/^\+\+\+ b/) | ||
| 61 | + if line.match(/^@@ -/) | ||
| 62 | + next if line_old == 1 && line_new == 1 | ||
| 63 | + type = "match" | ||
| 64 | + | ||
| 65 | + line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0 | ||
| 66 | + line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0 | ||
| 67 | + | ||
| 68 | + yield(nil, type, nil, nil, nil) | ||
| 69 | + next | ||
| 70 | + else | ||
| 71 | + type = diff_line_class(line) | ||
| 72 | + line_code = build_line_code(line, index, line_new, line_old) | ||
| 73 | + yield(full_line, type, line_code, line_new, line_old) | ||
| 74 | + end | ||
| 75 | + | ||
| 76 | + | ||
| 77 | + if line[0] == "+" | ||
| 78 | + line_new += 1 | ||
| 79 | + elsif line[0] == "-" | ||
| 80 | + line_old += 1 | ||
| 81 | + else | ||
| 82 | + line_new += 1 | ||
| 83 | + line_old += 1 | ||
| 84 | + end | ||
| 50 | end | 85 | end |
| 51 | end | 86 | end |
| 52 | end | 87 | end |
app/models/note.rb
| @@ -57,23 +57,6 @@ class Note < ActiveRecord::Base | @@ -57,23 +57,6 @@ class Note < ActiveRecord::Base | ||
| 57 | rescue | 57 | rescue |
| 58 | nil | 58 | nil |
| 59 | end | 59 | end |
| 60 | - | ||
| 61 | - def line_file_id | ||
| 62 | - @line_file_id ||= line_code.split("_")[1].to_i if line_code | ||
| 63 | - end | ||
| 64 | - | ||
| 65 | - def line_type_id | ||
| 66 | - @line_type_id ||= line_code.split("_").first if line_code | ||
| 67 | - end | ||
| 68 | - | ||
| 69 | - def line_number | ||
| 70 | - @line_number ||= line_code.split("_").last.to_i if line_code | ||
| 71 | - end | ||
| 72 | - | ||
| 73 | - def for_line?(file_id, old_line, new_line) | ||
| 74 | - line_file_id == file_id && | ||
| 75 | - ((line_type_id == "NEW" && line_number == new_line) || (line_type_id == "OLD" && line_number == old_line )) | ||
| 76 | - end | ||
| 77 | end | 60 | end |
| 78 | # == Schema Information | 61 | # == Schema Information |
| 79 | # | 62 | # |
app/views/commits/_text_file.html.haml
| 1 | %table | 1 | %table |
| 2 | - - line_old = 0 | ||
| 3 | - - line_new = 0 | ||
| 4 | - - diff_str = diff.diff | ||
| 5 | - - lines_arr = diff_str.lines.to_a | ||
| 6 | - - lines_arr.each do |line| | ||
| 7 | - - next if line.match(/^--- \/dev\/null/) | ||
| 8 | - - next if line.match(/^--- a/) | ||
| 9 | - - next if line.match(/^\+\+\+ b/) | ||
| 10 | - - if line.match(/^@@ -/) | ||
| 11 | - - unless line_old.zero? && line_new.zero? | ||
| 12 | - %tr.line_holder | ||
| 13 | - %td.old_line= "..." | ||
| 14 | - %td.new_line= "..." | ||
| 15 | - %td.line_content | ||
| 16 | - | ||
| 17 | - - line_old = line.match(/\-[0-9]*/)[0].to_i.abs rescue 0 | ||
| 18 | - - line_new = line.match(/\+[0-9]*/)[0].to_i.abs rescue 0 | ||
| 19 | - - next | ||
| 20 | - | ||
| 21 | - - full_line = html_escape(line.gsub(/\n/, '')) | 2 | + - each_diff_line(diff.diff.lines.to_a, index) do |line, type, line_code, line_new, line_old| |
| 22 | %tr.line_holder | 3 | %tr.line_holder |
| 23 | - %td.old_line | ||
| 24 | - = link_to raw(diff_line_class(line) == "new" ? " " : line_old), "#OLD#{index}-#{line_old}", :id => "OLD#{index}-#{line_old}" | ||
| 25 | - %td.new_line | ||
| 26 | - = link_to raw(diff_line_class(line) == "old" ? " " : line_new) , "#NEW#{index}-#{line_new}", :id => "NEW#{index}-#{line_new}" | ||
| 27 | - %td.line_content{:class => "#{diff_line_class(full_line)} #{build_line_code(line, index, line_new, line_old)}", "line_code" => build_line_code(line, index, line_new, line_old)}= raw "#{full_line} " | ||
| 28 | - - comments = @line_notes.select { |n| n.for_line?(index, line_old, line_new) }.sort_by(&:created_at).reverse | ||
| 29 | - - unless comments.empty? | ||
| 30 | - - comments.each do |note| | ||
| 31 | - = render "notes/per_line_show", :note => note | ||
| 32 | - - if line[0] == "+" | ||
| 33 | - - line_new += 1 | ||
| 34 | - - elsif line[0] == "-" | ||
| 35 | - - line_old += 1 | ||
| 36 | - - else | ||
| 37 | - - line_new += 1 | ||
| 38 | - - line_old += 1 | 4 | + - if type == "match" |
| 5 | + %td.old_line= "..." | ||
| 6 | + %td.new_line= "..." | ||
| 7 | + %td.line_content | ||
| 8 | + - else | ||
| 9 | + %td.old_line= link_to raw(type == "new" ? " " : line_old), "##{line_code}", :id => line_code | ||
| 10 | + %td.new_line= link_to raw(type == "old" ? " " : line_new) , "##{line_code}", :id => line_code | ||
| 11 | + %td.line_content{:class => "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line} " | ||
| 12 | + | ||
| 13 | + - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at).reverse | ||
| 14 | + - unless comments.empty? | ||
| 15 | + - comments.each do |note| | ||
| 16 | + = render "notes/per_line_show", :note => note | ||
| 17 | + - @line_notes.reject!{ |n| n == note } |
app/views/commits/show.html.haml
| @@ -29,7 +29,7 @@ | @@ -29,7 +29,7 @@ | ||
| 29 | 29 | ||
| 30 | :javascript | 30 | :javascript |
| 31 | $(document).ready(function(){ | 31 | $(document).ready(function(){ |
| 32 | - $(".line_content").live("dblclick", function(e) { | 32 | + $(".noteable_line").live("dblclick", function(e) { |
| 33 | var form = $(".per_line_form"); | 33 | var form = $(".per_line_form"); |
| 34 | $(this).parent().after(form); | 34 | $(this).parent().after(form); |
| 35 | form.find("#note_line_code").val($(this).attr("line_code")); | 35 | form.find("#note_line_code").val($(this).attr("line_code")); |
spec/models/note_spec.rb
| @@ -42,27 +42,13 @@ describe Note do | @@ -42,27 +42,13 @@ describe Note do | ||
| 42 | :project => project, | 42 | :project => project, |
| 43 | :noteable_id => commit.id, | 43 | :noteable_id => commit.id, |
| 44 | :noteable_type => "Commit", | 44 | :noteable_type => "Commit", |
| 45 | - :line_code => "OLD_1_23" | 45 | + :line_code => "0_16_1" |
| 46 | end | 46 | end |
| 47 | 47 | ||
| 48 | it "should save a valid note" do | 48 | it "should save a valid note" do |
| 49 | @note.noteable_id.should == commit.id | 49 | @note.noteable_id.should == commit.id |
| 50 | @note.target.id.should == commit.id | 50 | @note.target.id.should == commit.id |
| 51 | end | 51 | end |
| 52 | - | ||
| 53 | - it { @note.line_type_id.should == "OLD" } | ||
| 54 | - it { @note.line_file_id.should == 1 } | ||
| 55 | - it { @note.line_number.should == 23 } | ||
| 56 | - | ||
| 57 | - it { @note.for_line?(1, 23, 34).should be_true } | ||
| 58 | - it { @note.for_line?(1, 23, nil).should be_true } | ||
| 59 | - it { @note.for_line?(1, 23, 0).should be_true } | ||
| 60 | - it { @note.for_line?(1, 23, 23).should be_true } | ||
| 61 | - | ||
| 62 | - it { @note.for_line?(1, nil, 34).should be_false } | ||
| 63 | - it { @note.for_line?(1, 24, nil).should be_false } | ||
| 64 | - it { @note.for_line?(1, 24, 0).should be_false } | ||
| 65 | - it { @note.for_line?(1, 24, 23).should be_false } | ||
| 66 | end | 52 | end |
| 67 | 53 | ||
| 68 | describe :authorization do | 54 | describe :authorization do |