From 993454fc291cb96dc7e2f8817a09356ae4a44f38 Mon Sep 17 00:00:00 2001 From: Alexander Machehin Date: Thu, 11 Oct 2012 23:50:40 +0600 Subject: [PATCH] [refs #579] alpha version of the pull discussion --- .../projects/comments_controller.rb | 21 +++++--- .../projects/pull_requests_controller.rb | 10 ++-- app/helpers/diff_helper.rb | 50 +++++++++++-------- app/helpers/pull_request_helper.rb | 4 +- app/models/comment.rb | 30 +++++++++++ app/models/pull_request.rb | 1 + .../git/commits/_commit_diff.html.haml | 2 +- .../pull_requests/_activity.html.haml | 19 ++----- .../_discussion_comments.html.haml | 13 +++++ .../pull_requests/_pull_diff.html.haml | 2 +- app/views/projects/wiki/_diff_data.html.haml | 2 +- 11 files changed, 102 insertions(+), 52 deletions(-) create mode 100644 app/views/projects/pull_requests/_discussion_comments.html.haml diff --git a/app/controllers/projects/comments_controller.rb b/app/controllers/projects/comments_controller.rb index cdf4755f5..94b780dba 100644 --- a/app/controllers/projects/comments_controller.rb +++ b/app/controllers/projects/comments_controller.rb @@ -68,21 +68,30 @@ class Projects::CommentsController < Projects::BaseController diff = pull.diff repo, to_commit, from_commit diff_path = diff.select {|d| d.a_path == params[:path]} - return false if diff_path.blank? - comment_line = params[:line].to_i - return false if comment_line == 0 # NB! also dont want create comment to the diff header + return false unless @comment.actual_inline_comment?(diff, true) - line_number, line_presence = -1, false + comment_line, line_number, strings = params[:line].to_i, -1, [] diff_path[0].diff.each_line do |line| line_number = line_number.succ # Save 2 lines above and bottom of the diff comment line break if line_number > comment_line + 2 if (comment_line-2..comment_line+2).include? line_number @comment.data["line#{line_number-comment_line}"] = line.chomp - line_presence = true + end + + # Save lines from the closest header for rendering in the discussion + if line_number < comment_line - 2 + # Header is the line like "@@ -47,9 +50,8 @@ def initialize(user)" + if line =~ /^@@ [+-]([0-9]+)(?:,([0-9]+))? [+-]([0-9]+)(?:,([0-9]+))? @@/ + strings = line + else + strings << line + end end end - return line_presence + @comment.data[:strings] = strings + @comment.data[:view_path] = h(diff_path[0].renamed_file ? "#{diff_path[0].a_path.rtruncate 60} -> #{diff_path[0].b_path.rtruncate 60}" : diff_path[0].a_path.rtruncate(120)) + return true end end end diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb index f634b5013..58512f8e5 100644 --- a/app/controllers/projects/pull_requests_controller.rb +++ b/app/controllers/projects/pull_requests_controller.rb @@ -128,16 +128,16 @@ class Projects::PullRequestsController < Projects::BaseController end def load_diff_commits_data - repo = Grit::Repo.new(@pull.path) + @repo = Grit::Repo.new(@pull.path) @base_commit = @pull.common_ancestor - @head_commit = repo.commits(@pull.head_branch).first + @head_commit = @repo.commits(@pull.head_branch).first - @commits = repo.commits_between(repo.commits(@pull.to_ref).first, @head_commit) + @commits = @repo.commits_between(@repo.commits(@pull.to_ref).first, @head_commit) @total_commits = @commits.count @commits = @commits.last(100) - @diff = @pull.diff repo, @base_commit, @head_commit - @stats = @pull.diff_stats repo, @base_commit, @head_commit + @diff = @pull.diff @repo, @base_commit, @head_commit + @stats = @pull.diff_stats @repo, @base_commit, @head_commit @comments = @issue.comments @commentable = @issue end diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 8fd7d4f36..d1a2ce889 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -21,18 +21,29 @@ module DiffHelper end #include Git::Diff::InlineCallback - def render_diff(diff, diff_counter) - diff_display ||= Diff::Display::Unified.new(diff.diff) + def render_diff(diff, diff_counter, comments, diffpath = nil) + if diff.respond_to?(:diff) + filepath = diff.a_path + diff = diff.diff + in_discussion = false + comments = comments.select{|c| c.data.try('[]', :path) == filepath} + else + filepath = diffpath + in_discussion = true + end + + diff_display ||= Diff::Display::Unified.new(diff) url = if @pull @pull.id ? polymorphic_path([@project, @pull]) : '' elsif @commit commit_path @project, @commit end - prepare(diff, url, diff_counter) + prepare(url, diff_counter, filepath, comments, in_discussion) res = "" res += "" res += renderer diff_display.data #diff_display.render(Git::Diff::InlineCallback.new comments, path) + res += tr_line_comments(comments) if in_discussion res += "" res += "
" res.html_safe @@ -41,9 +52,9 @@ module DiffHelper ######################################################## # FIXME: Just to dev, remove to lib ######################################################## - def prepare(diff, url, diff_counter) - @diff, @num_line, @filepath, @url, @diff_counter = diff, -1, diff.a_path, url, diff_counter - @line_comments = @comments.select{|c| c.data.try('[]', :path) == @filepath} + def prepare(url, diff_counter, filepath, comments, in_discussion) + @num_line, @url, @diff_counter, @in_discussion = -1, url, diff_counter, in_discussion + @filepath, @line_comments = filepath, comments end def headerline(line) @@ -216,29 +227,26 @@ module DiffHelper end def render_line_comments - comments = @line_comments.select do |c| - next false if c.data.try('[]', :line) != @num_line.to_s - next true if c.commentable_type == 'Grit::Commit' - #diff = Diff::Display::Unified.new(@diff.diff) - res, ind = true, 0 - @diff.diff.each_line do |line| - res = false if (@num_line-2..@num_line+2).include?(ind) && c.data.try('[]', "line#{ind-@num_line}") != line.chomp - ind = ind + 1 + unless @in_discussion + comments = @line_comments.select do |c| + c.data.try('[]', :line) == @num_line.to_s && c.actual_inline_comment?(@diff) end - res + tr_line_comments(comments) if comments.count > 0 end + end + + def td_line_link id, num + "#{num}" + end + + def tr_line_comments comments " #{comments.count} #{render("projects/comments/line_list", :list => comments, :project => @project, :commentable => @commentable)} #{link_to t('layout.comments.new_inline'), new_comment_path, :class => 'new_inline_comment button'} - - " if comments.count > 0 - end - - def td_line_link id, num - "#{num}" + " end def new_comment_path diff --git a/app/helpers/pull_request_helper.rb b/app/helpers/pull_request_helper.rb index cfc0e9ce3..264d10db2 100644 --- a/app/helpers/pull_request_helper.rb +++ b/app/helpers/pull_request_helper.rb @@ -1,9 +1,9 @@ # -*- encoding : utf-8 -*- module PullRequestHelper def merge_activity comments, commits - issue_comments = @issue.comments.map{ |c| [c.created_at, c] } + pull_comments = comments.group_by(&:data).map{|data, c| [c.first.created_at, [data || {}, [c].flatten]]} commits = @commits.map{ |c| [(c.committed_date || c.authored_date), c] } - (issue_comments + commits).sort_by{ |c| c[0] }.map{ |c| c[1] } + (pull_comments + commits).sort_by{ |c| c[0] }.map{ |c| c[1] } end def pull_status_label pull diff --git a/app/models/comment.rb b/app/models/comment.rb index 39956b622..1923eab9b 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -54,6 +54,36 @@ class Comment < ActiveRecord::Base User.find(subscribe.user).notifier.new_comment && User.find(subscribe.user).notifier.can_notify end + def actual_inline_comment?(diff, force = false) + return data[:actual] if data[:actual].present? && !force + filepath, line_number = data[:path], data[:line] + diff_path = diff.select {|d| d.a_path == data[:path]} + comment_line = data[:line].to_i + # NB! also dont create a comment to the diff header + return data[:actual] = false if diff_path.blank? || comment_line == 0 + return data[:actual] = true if commentable_type == 'Grit::Commit' + res, ind = true, 0 + diff_path[0].diff.each_line do |line| + if self.persisted? && (comment_line-2..comment_line+2).include?(ind) && data.try('[]', "line#{ind-comment_line}") != line.chomp + break res = false + end + ind = ind + 1 + end + if ind < comment_line + return data[:actual] = false + else + return data[:actual] = res + end + end + + def inline_diff(repo) + text = data[:strings] + Rails.logger.debug "Comment id is #{id}; text class is #{text.class.name}; text is #{text}" + closest = [] + (-2..0).each {|shift| closest << data["line#{shift}"]} + text << closest.join("\n") + end + protected def subscribe_on_reply diff --git a/app/models/pull_request.rb b/app/models/pull_request.rb index ffd0346a5..6f92784dc 100644 --- a/app/models/pull_request.rb +++ b/app/models/pull_request.rb @@ -70,6 +70,7 @@ class PullRequest < ActiveRecord::Base else send(new_status) end + self.comments.each {|c| c.data[:actual]=nil; c.save} # maybe need add new column 'actual'? else self.status = new_status == 'block' ? 'blocked' : new_status end diff --git a/app/views/projects/git/commits/_commit_diff.html.haml b/app/views/projects/git/commits/_commit_diff.html.haml index ce9178850..f745bfa47 100644 --- a/app/views/projects/git/commits/_commit_diff.html.haml +++ b/app/views/projects/git/commits/_commit_diff.html.haml @@ -6,4 +6,4 @@ - if commit_diff.b_path.present? .r= link_to "view file @ #{short_hash_id(commit_id)}", blob_path(@project, commit_id, commit_diff.b_path) .clear - .diff_data= render_diff(commit_diff, commit_diff_counter) unless (@project.repo.tree(commit_id) / commit_diff.b_path).binary? + .diff_data= render_diff(commit_diff, commit_diff_counter, @comments) unless (@project.repo.tree(commit_id) / commit_diff.b_path).binary? diff --git a/app/views/projects/pull_requests/_activity.html.haml b/app/views/projects/pull_requests/_activity.html.haml index 870edb92d..32f0e873f 100644 --- a/app/views/projects/pull_requests/_activity.html.haml +++ b/app/views/projects/pull_requests/_activity.html.haml @@ -4,19 +4,8 @@ - commits_queue = [] - merge_activity(@comments, @commits).each do |item| # - - if item.is_a?(Comment) - - if commits_queue.present? - = render 'projects/git/commits/commits_small', :commits => commits_queue - - commits_queue = [] - = render 'projects/comments/comment', :comment => item, :project => @project, :commentable => @issue, :add_id => nil - - else - - commits_queue << item - - comments_for_commit = Comment.for_commit(item) - - if comments_for_commit.present? - = render 'projects/git/commits/commits_small', :commits => commits_queue - - comments_for_commit.each do |comment| - = render 'projects/comments/comment', :comment => comment, :project => @project, :commentable => item, :add_id => nil - - commits_queue = [] -- if commits_queue.present? - = render 'projects/git/commits/commits_small', :commits => commits_queue + - if item.is_a? Grit::Commit + = render 'projects/git/commits/commits_small', :commits => [item] + - elsif item.is_a? Array + = render 'projects/pull_requests/discussion_comments', :item => item, :project => @project, :commentable => @issue, :add_id => nil diff --git a/app/views/projects/pull_requests/_discussion_comments.html.haml b/app/views/projects/pull_requests/_discussion_comments.html.haml new file mode 100644 index 000000000..e875b7292 --- /dev/null +++ b/app/views/projects/pull_requests/_discussion_comments.html.haml @@ -0,0 +1,13 @@ +-if item[0].blank? + -item[1].each do |comment| + = render 'projects/comments/comment', :comment => comment, :project => @project, :commentable => @issue, :add_id => nil +-else + -comment = item[1].first + .file + %a{:name => "#"} + .top + .l=comment.data[:view_path] + .r=link_to "view full changes", '#' + .clear + .diff_data=render_diff(comment.inline_diff(@repo), 0, item[1], comment.data[:path]) + diff --git a/app/views/projects/pull_requests/_pull_diff.html.haml b/app/views/projects/pull_requests/_pull_diff.html.haml index a5c5c8816..b4c249c86 100644 --- a/app/views/projects/pull_requests/_pull_diff.html.haml +++ b/app/views/projects/pull_requests/_pull_diff.html.haml @@ -6,4 +6,4 @@ - if pull_diff.b_path.present? .r= link_to "view file @ #{short_hash_id(commit_id)}", blob_path(@project, commit_id, pull_diff.b_path) .clear - .diff_data= render_diff(pull_diff, pull_diff_counter) unless (Grit::Repo.new(@pull.path).tree(commit_id) / pull_diff.b_path).binary? + .diff_data= render_diff(pull_diff, pull_diff_counter, @comments) unless (Grit::Repo.new(@pull.path).tree(commit_id) / pull_diff.b_path).binary? diff --git a/app/views/projects/wiki/_diff_data.html.haml b/app/views/projects/wiki/_diff_data.html.haml index 1c00ff781..897a05f5f 100644 --- a/app/views/projects/wiki/_diff_data.html.haml +++ b/app/views/projects/wiki/_diff_data.html.haml @@ -1,4 +1,4 @@ .blob_header .size= h(diff.deleted_file ? diff.a_path : diff.b_path) .clear -.diff_data.highlight= render_diff(diff) \ No newline at end of file +.diff_data.highlight= render_diff(diff, diff_counter, [])