diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 346b04e40f0412b734249f4a984a40c5a8b693dc..c7c291516fc63420c5d0762cf0d674b5442ee9e0 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -34,10 +34,6 @@ module DiffHelper diffs.decorate! { |diff| Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) } end - def generate_line_code(file_path, line) - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) - end - def unfold_bottom_class(bottom) bottom ? 'js-unfold-bottom' : '' end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 1a97f8845083558aa8d2609de723c13953d3e8d0..721dfcf265f625c5859ef48b91be10e603f12a9c 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -60,10 +60,9 @@ module NotesHelper } if note.diff_note? - data.merge!( - line_code: note.line_code, - note_type: LegacyDiffNote.name - ) + data[:note_type] = note.type + + data.merge!(note.diff_attributes) end button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb new file mode 100644 index 0000000000000000000000000000000000000000..b511f33b8fac16ab125c725782d2964e9627cf4b --- /dev/null +++ b/app/models/concerns/note_on_diff.rb @@ -0,0 +1,53 @@ +module NoteOnDiff + extend ActiveSupport::Concern + + NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + + included do + delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true + end + + def diff_note? + true + end + + def diff_file + raise NotImplementedError + end + + def diff_line + raise NotImplementedError + end + + def for_line?(line) + raise NotImplementedError + end + + def diff_attributes + raise NotImplementedError + end + + def can_be_award_emoji? + false + end + + def truncated_diff_lines + prev_match_line = nil + prev_lines = [] + + highlighted_diff_lines.each do |line| + if line.meta? + prev_lines.clear + prev_match_line = line + else + prev_lines << line + + break if for_line?(line) + + prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + prev_lines + end +end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 33d2a69ebaffaa74d9c362f35fec0923305b748a..790dfd4d4804fb6ea2c9b5c13465a144be3fb144 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -1,4 +1,6 @@ class LegacyDiffNote < Note + include NoteOnDiff + serialize :st_diff validates :line_code, presence: true, line_code: true @@ -11,12 +13,12 @@ class LegacyDiffNote < Note end end - def diff_note? + def legacy_diff_note? true end - def legacy_diff_note? - true + def diff_attributes + { line_code: line_code } end def discussion_id @@ -27,61 +29,20 @@ class LegacyDiffNote < Note line_code.split('_')[0] if line_code end - def diff_old_line - line_code.split('_')[1].to_i if line_code - end - - def diff_new_line - line_code.split('_')[2].to_i if line_code - end - def diff @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map) end - def diff_file_path - diff.new_path.presence || diff.old_path - end - - def diff_lines - @diff_lines ||= Gitlab::Diff::Parser.new.parse(diff.diff.each_line) + def diff_file + @diff_file ||= Gitlab::Diff::File.new(diff, repository: self.project.repository) if diff end def diff_line - @diff_line ||= diff_lines.find { |line| generate_line_code(line) == self.line_code } + @diff_line ||= diff_file.line_for_line_code(self.line_code) end - def diff_line_text - diff_line.try(:text) - end - - def diff_line_type - diff_line.try(:type) - end - - def highlighted_diff_lines - Gitlab::Diff::Highlight.new(diff_lines).highlight - end - - def truncated_diff_lines - max_number_of_lines = 16 - prev_match_line = nil - prev_lines = [] - - highlighted_diff_lines.each do |line| - if line.type == "match" - prev_lines.clear - prev_match_line = line - else - prev_lines << line - - break if generate_line_code(line) == self.line_code - - prev_lines.shift if prev_lines.length >= max_number_of_lines - end - end - - prev_lines + def for_line?(line) + !line.meta? && diff_file.line_code(line) == self.line_code end # Check if this note is part of an "active" discussion @@ -102,7 +63,7 @@ class LegacyDiffNote < Note if noteable_diff parsed_lines = Gitlab::Diff::Parser.new.parse(noteable_diff.diff.each_line) - @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line_text } + @active = parsed_lines.any? { |line_obj| line_obj.text == diff_line.text } else @active = false end @@ -110,10 +71,6 @@ class LegacyDiffNote < Note @active end - def award_emoji_supported? - false - end - private def find_diff @@ -149,10 +106,6 @@ class LegacyDiffNote < Note self.class.where(attributes).last.try(:diff) end - def generate_line_code(line) - Gitlab::Diff::LineCode.generate(diff_file_path, line.new_pos, line.old_pos) - end - # Find the diff on noteable that matches our own def find_noteable_diff diffs = noteable.diffs(Commit.max_diff_options) diff --git a/app/models/note.rb b/app/models/note.rb index 81b5c47b7381551cc34c2e473826886b3ba07aa8..0c2650646308be886eb77815abe00fac89de9b1b 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -193,7 +193,7 @@ class Note < ActiveRecord::Base end def award_emoji? - award_emoji_supported? && contains_emoji_only? + can_be_award_emoji? && contains_emoji_only? end def emoji_awardable? @@ -204,7 +204,7 @@ class Note < ActiveRecord::Base self.line_code = nil if self.line_code.blank? end - def award_emoji_supported? + def can_be_award_emoji? noteable.is_a?(Awardable) end diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index a2df899d012caac8504228875d5274fb8019ec01..928873fb5c3afcc734701303cababa6870be9bf6 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -20,7 +20,7 @@ class SentNotification < ActiveRecord::Base find_by(reply_key: reply_key) end - def record(noteable, recipient_id, reply_key, params = {}) + def record(noteable, recipient_id, reply_key, attrs = {}) return unless reply_key noteable_id = nil @@ -31,7 +31,7 @@ class SentNotification < ActiveRecord::Base noteable_id = noteable.id end - params.reverse_merge!( + attrs.reverse_merge!( project: noteable.project, noteable_type: noteable.class.name, noteable_id: noteable_id, @@ -40,13 +40,17 @@ class SentNotification < ActiveRecord::Base reply_key: reply_key ) - create(params) + create(attrs) end - def record_note(note, recipient_id, reply_key, params = {}) - params[:line_code] = note.line_code + def record_note(note, recipient_id, reply_key, attrs = {}) + if note.diff_note? + attrs[:note_type] = note.type - record(note.noteable, recipient_id, reply_key, params) + attrs.merge!(note.diff_attributes) + end + + record(note.noteable, recipient_id, reply_key, attrs) end end diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index f1577e8a47b4cb85363e11a7df53e477f14fe404..dbdbb6eb75426fa043b0ed051693a048979f6a89 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -1,3 +1,4 @@ +- line_code = diff_file.line_code(line) - type = line.type %tr.line_holder{ id: line_code, class: type } - case type diff --git a/app/views/projects/notes/discussions/_diff_with_notes.html.haml b/app/views/projects/notes/discussions/_diff_with_notes.html.haml index b924ed31b42a2da14811678b794a3a9b61745b4f..3866de0f7fa9f291ae8fe9274bdacf928dbd8fe7 100644 --- a/app/views/projects/notes/discussions/_diff_with_notes.html.haml +++ b/app/views/projects/notes/discussions/_diff_with_notes.html.haml @@ -12,7 +12,7 @@ %table - note.truncated_diff_lines.each do |line| - type = line.type - - line_code = generate_line_code(note.diff_file_path, line) + - line_code = diff_file.line_code(line) %tr.line_holder{ id: line_code, class: "#{type}" } - if type == "match" %td.old_line.diff-line-num= "..." @@ -23,5 +23,5 @@ %td.new_line.diff-line-num{ data: { linenumber: type == "old" ? " ".html_safe : line.new_pos } } %td.line_content{ class: ['noteable_line', type, line_code], line_code: line_code }= diff_line_content(line.text, type) - - if line_code == note.line_code + - if note.for_line?(line) = render "projects/notes/diff_notes_with_reply", notes: discussion_notes diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8cc4368b5c26e56ca1658378cdd64d0d8919bfa5..db877d2eeb01a23b3b305d8cd48b16a515a0f67e 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -240,9 +240,9 @@ module API class CommitNote < Grape::Entity expose :note - expose(:path) { |note| note.diff_file_path if note.legacy_diff_note? } - expose(:line) { |note| note.diff_new_line if note.legacy_diff_note? } - expose(:line_type) { |note| note.diff_line_type if note.legacy_diff_note? } + expose(:path) { |note| note.diff_file.try(:file_path) if note.diff_note? } + expose(:line) { |note| note.diff_line.try(:new_line) if note.diff_note? } + expose(:line_type) { |note| note.diff_line.try(:type) if note.diff_note? } expose :author, using: Entities::UserBasic expose :created_at end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index eaa6fdff8aaf7d4505054decd78a9f7b09ccb223..c73208329d559261d17b09ec6d576025bc8ab3d5 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -13,6 +13,16 @@ module Gitlab @diff_refs = diff_refs end + def line_code(line) + return if line.meta? + + Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) + end + + def line_for_line_code(code) + diff_lines.find { |line| line_code(line) == code } + end + def content_commit return unless diff_refs diff --git a/lib/gitlab/diff/highlight.rb b/lib/gitlab/diff/highlight.rb index 3ad68728d65fc1b52fb0ec1f7ab929aba2da4989..44ea6bf61022342b5926be665d60f4867d0b723a 100644 --- a/lib/gitlab/diff/highlight.rb +++ b/lib/gitlab/diff/highlight.rb @@ -42,10 +42,9 @@ module Gitlab line_prefix = diff_line.text.match(/\A(.)/) ? $1 : ' ' - case diff_line.type - when 'new', nil + if diff_line.unchanged? || diff_line.added? rich_line = new_lines[diff_line.new_pos - 1] - when 'old' + elsif diff_line.removed? rich_line = old_lines[diff_line.old_pos - 1] end @@ -60,19 +59,12 @@ module Gitlab def old_lines return unless diff_file - @old_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:old)) + @old_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_old_ref, diff_old_path) end def new_lines return unless diff_file - @new_lines ||= Gitlab::Highlight.highlight_lines(*processing_args(:new)) - end - - def processing_args(version) - ref = send("diff_#{version}_ref") - path = send("diff_#{version}_path") - - [self.repository, ref, path] + @new_lines ||= Gitlab::Highlight.highlight_lines(self.repository, diff_new_ref, diff_new_path) end end end diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 03730b435adc5fdceaf404198ee07ccf9da58ace..c6189d660c2b6304eab2899abab26702da9e4b4e 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -9,6 +9,18 @@ module Gitlab @old_pos, @new_pos = old_pos, new_pos end + def old_line + old_pos unless added? || meta? + end + + def new_line + new_pos unless removed? || meta? + end + + def unchanged? + type.nil? + end + def added? type == 'new' end @@ -16,6 +28,10 @@ module Gitlab def removed? type == 'old' end + + def meta? + type == 'match' || type == 'nonewline' + end end end end diff --git a/lib/gitlab/diff/parallel_diff.rb b/lib/gitlab/diff/parallel_diff.rb index 74f9b3c050a616a62cb9a4c1e2bafb0b17734c90..2d15b64fdb04af63946b903022c2644ec1da3400 100644 --- a/lib/gitlab/diff/parallel_diff.rb +++ b/lib/gitlab/diff/parallel_diff.rb @@ -15,7 +15,7 @@ module Gitlab highlighted_diff_lines.each do |line| full_line = line.text type = line.type - line_code = generate_line_code(diff_file.file_path, line) + line_code = diff_file.line_code(line) line_new = line.new_pos line_old = line.old_pos @@ -23,9 +23,9 @@ module Gitlab if next_line next_line = highlighted_diff_lines[next_line.index] - next_line_code = generate_line_code(diff_file.file_path, next_line) + full_next_line = next_line.text + next_line_code = diff_file.line_code(next_line) next_type = next_line.type - next_line = next_line.text end case type @@ -59,8 +59,8 @@ module Gitlab right: { type: next_type, number: line_new, - text: next_line, - line_code: next_line_code + text: full_next_line, + line_code: next_line_code, } } skip_next = true @@ -108,12 +108,6 @@ module Gitlab end lines end - - private - - def generate_line_code(file_path, line) - Gitlab::Diff::LineCode.generate(file_path, line.new_pos, line.old_pos) - end end end end