Code review UI improvements and bugfixes (#4682)

* Code review UI improvements

* More fixes to dark theme

* Style fix

* Fix to allow add code review comments only on review files tab

* More readability dark style fixes

* Fix commenting on deleted files. Fixes #4752

* Fix line blame getting for multiple corner cases

models/git_diff.go

@@ -273,7 +273,7 @@ func (diff *Diff) NumFiles() int {
 }
 
 // Example: @@ -1,8 +1,9 @@ => [..., 1, 8, 1, 9]
-var hunkRegex = regexp.MustCompile(`^@@ -([0-9]+),([0-9]+) \+([0-9]+),([0-9]+) @@`)
+var hunkRegex = regexp.MustCompile(`^@@ -([0-9]+),([0-9]+) \+([0-9]+)(,([0-9]+))? @@`)
 
 func isHeader(lof string) bool {
 	return strings.HasPrefix(lof, cmdDiffHead) || strings.HasPrefix(lof, "---") || strings.HasPrefix(lof, "+++")
@@ -319,7 +319,11 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 				otherLine = com.StrTo(groups[3]).MustInt64()
 			} else {
 				begin = com.StrTo(groups[3]).MustInt64()
-				end = com.StrTo(groups[4]).MustInt64()
+				if groups[5] != "" {
+					end = com.StrTo(groups[5]).MustInt64()
+				} else {
+					end = 0
+				}
 				// init otherLine with begin of opposite side
 				otherLine = com.StrTo(groups[1]).MustInt64()
 			}

models/issue_comment.go

@@ -392,7 +392,7 @@ func (c *Comment) checkInvalidation(e Engine, doer *User, repo *git.Repository,
 	if err != nil {
 		return err
 	}
-	if c.CommitSHA != commit.ID.String() {
+	if c.CommitSHA != "" && c.CommitSHA != commit.ID.String() {
 		c.Invalidated = true
 		return UpdateComment(doer, c, "")
 	}
@@ -824,17 +824,18 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree
 	if err != nil {
 		return nil, fmt.Errorf("OpenRepository: %v", err)
 	}
-	// FIXME differentiate between previous and proposed line
-	var gitLine = line
-	if gitLine < 0 {
-		gitLine *= -1
-	}
+
 	// FIXME validate treePath
 	// Get latest commit referencing the commented line
-	commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(gitLine))
-	if err != nil {
-		return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, gitLine, err)
+	// No need for get commit for base branch changes
+	if line > 0 {
+		commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line))
+		if err != nil {
+			return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
+		}
+		commitID = commit.ID.String()
 	}
+
 	// Only fetch diff if comment is review comment
 	if reviewID != 0 {
 		headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName())
@@ -846,7 +847,6 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree
 			return nil, fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %v", err, gitRepo.Path, pr.MergeBase, headCommitID, treePath)
 		}
 		patch = CutDiffAroundLine(strings.NewReader(patchBuf.String()), int64((&Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
-		commitID = commit.ID.String()
 	}
 	return CreateComment(&CreateCommentOptions{
 		Type:      CommentTypeCode,

public/js/index.js

@@ -780,7 +780,7 @@ function initPullRequestReview() {
         $("#show-outdated-" + id).removeClass('hide');
     });
 
-    $('.comment-form-reply').on('click', function (e) {
+    $('button.comment-form-reply').on('click', function (e) {
         e.preventDefault();
         $(this).hide();
         var form = $(this).parent().find('.comment-form')
@@ -2649,7 +2649,7 @@ function cancelCodeComment(btn) {
     var form = $(btn).closest("form");
     if(form.length > 0 && form.hasClass('comment-form')) {
         form.addClass('hide');
-        form.parent().find('.comment-form-reply').show();
+        form.parent().find('button.comment-form-reply').show();
     }else {
         console.log("Hey");
         form.closest('.comment-code-cloud').remove()

public/less/_review.less

@@ -42,14 +42,21 @@
         top: -13px;
     }
 
-    .attached.tab {
-        border: none;
-        padding: 0;
-        margin: 0;
-
-        &.markdown {
-            padding: 1em;
-            min-height: 168px;
+    .attached
+    {
+        &.tab {
+            border: none;
+            padding: 0;
+            margin: 0;
+
+            &.markdown {
+                padding: 1em;
+                min-height: 168px;
+            }
+        }
+
+        &.header {
+            padding: .1rem 1rem;
         }
     }
 
@@ -92,8 +99,12 @@
         }
     }
 
-    .comment-form-reply {
-        margin: 0.5em !important;
+    button.comment-form-reply {
+        margin: 0.5em 0.5em 0.5em 4.5em;
+    }
+
+    form.comment-form-reply {
+        margin: 0 0 0 4em;
     }
 }
 

public/less/themes/arc-green.less

@@ -182,6 +182,9 @@
      border: 2px solid #353945;
      color: #dbdbdb;
 }
+ .ui.accordion .title:not(.ui) {
+     color: #dbdbdb;
+}
  .ui.label {
      color: #dbdbdb;
      background-color: #404552;
@@ -195,10 +198,15 @@
  .issue.list > .item {
      border-bottom: 1px dashed #475767;
 }
- .ui.green.label, .ui.green.labels .label {
+ .ui.green.label, .ui.green.labels .label, .ui.basic.green.label {
      background-color: #2d693b!important;
      border-color: #2d693b!important;
 }
+ .ui.basic.green.labels a.label:hover, a.ui.basic.green.label:hover {
+     background-color: #16ab39 !important;
+     border-color: #16ab39 !important;
+     color: #fff !important;
+}
  .issue.list > .item .comment {
      color: #129c92;
 }
@@ -555,10 +563,17 @@
  .ui.attached.info.message, .ui.info.message {
      box-shadow: 0 0 0 1px #4b5e71 inset, 0 0 0 0 transparent;
 }
+ .ui.positive.message {
+     background-color: #2c662d;
+     color: #fcfff5;
+}
  .ui.info.message {
      background-color: #2c3b4a;
      color: #9ebcc5;
 }
+ .CodeMirror div.CodeMirror-cursor {
+     border-left: 1px solid #9e9e9e;
+}
  .ui .warning.header {
      background-color: #5d3a22 !important;
      border-color: #794f31;
@@ -767,8 +782,34 @@
 }
 
 .repository .diff-detail-box {
-    background-color: inherit;
+    background-color: #383c4a;
     .detail-files {
         background-color: inherit;
     }
 }
+
+.comment-code-cloud {
+    .ui.attached.tabular.menu {
+        background: none transparent;
+        border: none;
+    }
+    .footer .markdown-info {
+        color: inherit;
+    }
+}
+
+.file-comment {
+    color: #888;
+}
+
+.ui.comments .comment {
+    .author {
+        color: #dbdbdb;
+    }
+    .metadata {
+        color: #808084;
+    }
+    .text {
+        color: #9e9e9e;
+    }
+}

routers/repo/pull_review.go

@@ -128,13 +128,23 @@ func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) {
 		}
 	}
 
-	if form.HasEmptyContent() {
+	review, err = models.GetCurrentReview(ctx.User, issue)
+	if err == nil {
+		review.Issue = issue
+		if errl := review.LoadCodeComments(); errl != nil {
+			ctx.ServerError("LoadCodeComments", err)
+			return
+		}
+	}
+
+	if ((err == nil && len(review.CodeComments) == 0) ||
+		(err != nil && models.IsErrReviewNotExist(err))) &&
+		form.HasEmptyContent() {
 		ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))
 		ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
 		return
 	}
 
-	review, err = models.GetCurrentReview(ctx.User, issue)
 	if err != nil {
 		if !models.IsErrReviewNotExist(err) {
 			ctx.ServerError("GetCurrentReview", err)

templates/repo/diff/box.tmpl

@@ -125,7 +125,7 @@
 
 														</td>
 														<td class="lines-code lines-code-old halfwidth">
-															{{if and $.root.SignedUserID $line.CanComment}}
+															{{if and $.root.SignedUserID $line.CanComment $.root.PageIsPullFiles}}
 																<a class="ui green button add-code-comment add-code-comment-left" data-path="{{$file.Name}}" data-side="left" data-idx="{{$line.LeftIdx}}">+</a>
 															{{end}}
 															<pre><code class="wrap {{if $highlightClass}}language-{{$highlightClass}}{{else}}nohighlight{{end}}">{{if $line.LeftIdx}}{{$section.GetComputedInlineDiffFor $line}}{{end}}</code></pre>

templates/repo/diff/comment_form.tmpl

@@ -2,7 +2,7 @@
 	{{if $.hidden}}
 		<button class="comment-form-reply ui green labeled icon tiny button"><i class="reply icon"></i> {{$.root.i18n.Tr "repo.diff.comment.reply"}}</button>
 	{{end}}
-	<form class="ui form {{if $.hidden}}hide comment-form{{end}}" action="{{$.root.Issue.HTMLURL}}/files/reviews/comments" method="post">
+	<form class="ui form {{if $.hidden}}hide comment-form comment-form-reply{{end}}" action="{{$.root.Issue.HTMLURL}}/files/reviews/comments" method="post">
 	{{$.root.CsrfTokenHtml}}
 		<input type="hidden" name="side" value="{{if $.Side}}{{$.Side}}{{end}}">
 		<input type="hidden" name="line" value="{{if $.Line}}{{$.Line}}{{end}}">
@@ -34,8 +34,10 @@
 								class="ui submit green tiny button btn-start-review">{{$.root.i18n.Tr "repo.diff.comment.start_review"}}</button>
 					{{end}}
 				{{end}}
+				{{if not $.root.CurrentReview}}
 				<button type="submit"
 					class="ui submit tiny basic button btn-add-single">{{$.root.i18n.Tr "repo.diff.comment.add_single_comment"}}</button>
+				{{end}}
 				{{if or (not $.HasComments) $.hidden}}
 					<button type="button" class="ui submit tiny basic button btn-cancel" onclick="cancelCodeComment(this);">{{$.root.i18n.Tr "cancel"}}</button>
 				{{end}}

templates/repo/diff/section_unified.tmpl

@@ -16,7 +16,7 @@
 			</td>
 			{{end}}
 			<td class="lines-code {{if (not $line.RightIdx)}}lines-code-old{{end}}">
-				{{if and $.root.SignedUserID $line.CanComment}}
+				{{if and $.root.SignedUserID $line.CanComment $.root.PageIsPullFiles}}
 					<a class="ui green button add-code-comment add-code-comment-{{if $line.RightIdx}}right{{else}}left{{end}}" data-path="{{$file.Name}}" data-side="{{if $line.RightIdx}}right{{else}}left{{end}}" data-idx="{{if $line.RightIdx}}{{$line.RightIdx}}{{else}}{{$line.LeftIdx}}{{end}}">+</a>
 				{{end}}
 				<pre><code class="wrap {{if $highlightClass}}language-{{$highlightClass}}{{else}}nohighlight{{end}}">{{$section.GetComputedInlineDiffFor $line}}</code></pre>