diff options
author | Douwe Maan <douwe@selenight.nl> | 2016-06-20 20:15:44 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2016-07-07 01:50:59 +0300 |
commit | a27462a5c6da0182f6b3a55c9417e6405f2c0415 (patch) | |
tree | 6bdf936568c0f9274cbf24db7f8b6517b2200980 | |
parent | 375193455aa5cb752f1035a6cc69160170a58477 (diff) |
Extract parts of LegacyDiffNote into DiffOnNote concern and move part of responsibility to other classes
-rw-r--r-- | app/helpers/diff_helper.rb | 4 | ||||
-rw-r--r-- | app/helpers/notes_helper.rb | 7 | ||||
-rw-r--r-- | app/models/concerns/note_on_diff.rb | 53 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 69 | ||||
-rw-r--r-- | app/models/note.rb | 4 | ||||
-rw-r--r-- | app/models/sent_notification.rb | 16 | ||||
-rw-r--r-- | app/views/projects/diffs/_line.html.haml | 1 | ||||
-rw-r--r-- | app/views/projects/notes/discussions/_diff_with_notes.html.haml | 4 | ||||
-rw-r--r-- | lib/api/entities.rb | 6 | ||||
-rw-r--r-- | lib/gitlab/diff/file.rb | 10 | ||||
-rw-r--r-- | lib/gitlab/diff/highlight.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/diff/line.rb | 16 | ||||
-rw-r--r-- | lib/gitlab/diff/parallel_diff.rb | 16 |
13 files changed, 120 insertions, 102 deletions
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 346b04e40f0..c7c291516fc 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 1a97f884508..721dfcf265f 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 00000000000..b511f33b8fa --- /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 33d2a69ebaf..790dfd4d480 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 81b5c47b738..0c265064630 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 a2df899d012..928873fb5c3 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 f1577e8a47b..dbdbb6eb754 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 b924ed31b42..3866de0f7fa 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 8cc4368b5c2..db877d2eeb0 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 eaa6fdff8aa..c73208329d5 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 3ad68728d65..44ea6bf6102 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 03730b435ad..c6189d660c2 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 74f9b3c050a..2d15b64fdb0 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 |