[bugzilla-gnome-org-extensions] Make comments ADDED/REMOVED/CHANGED
- From: Krzesimir Nowak <krnowak src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [bugzilla-gnome-org-extensions] Make comments ADDED/REMOVED/CHANGED
- Date: Thu, 20 Nov 2014 22:17:47 +0000 (UTC)
commit b00061cde7a6b957f9aa4f6c82cbcc53d4712149
Author: Owen W. Taylor <otaylor fishsoup net>
Date: Sat Sep 12 10:29:31 2009 -0400
Make comments ADDED/REMOVED/CHANGED
Have three different types of comments:
Patch.ADDED: Comments on new lines
Patch.REMOVED: Comments on removed lines
Patch.CHANGED: Comments on changed segments
ADDED/REMOVED are represented in the review format by ommitting
the - or the + of @@ -I,N +J,M @@ hunk intro.
'... N more ...' abbrevations are introduced into the review
format to allow representing a comment on an entire changed
segment without getting really long.
The CSS and HTML is adjusted so that the comments appear in
three overlapping vertical columns based on their type; this is
done by adding a single column-spanned columnArea div under
the line and then using floats within it.
Patch.File.getLocation() now allows either oldLine or newLine
to be null to look up by only one or the other.
docs/REVIEW_FORMAT.txt | 129 +++++++++++++++++---
js/patch.js | 33 ++---
js/review.js | 309 ++++++++++++++++++++++++++++++++++++------------
js/splinter.js | 175 +++++++++++++++++++---------
tests/review.jst | 92 +++++++++++++--
web/index.html | 2 +
web/splinter.css | 44 +++++---
7 files changed, 588 insertions(+), 196 deletions(-)
---
diff --git a/docs/REVIEW_FORMAT.txt b/docs/REVIEW_FORMAT.txt
index e566e70..a063e50 100644
--- a/docs/REVIEW_FORMAT.txt
+++ b/docs/REVIEW_FORMAT.txt
@@ -1,31 +1,122 @@
-Reviews are comments that start with the text "Review of attachment <attachment_id>:"
+A review is based upon a diff (conventionally a unified diff,
+but the actual format doesn't matter. All lines mentioned in the
+review must, however, be in the diff, either as an edited line
+or as context.)
-The review can start with free form comments that apply to the whole patch.
-A line of the form ::: <filename> introduces comments about a particular file
+In Bugzilla, reviews are represented Bugzilla comments that start with
+the text "Review of attachment <attachment_id>:" The attachment
+referred to must be a patch.
-Each comment is introduced by a small amount of context, which is formatted as
-a unified diff hunk.
+The review can start with free form comments that apply to the whole
+patch. A line of the form ::: <filename> introduces comments about a
+particular file
-Example:
+Comments are represented in a form somewhat similar to a "hunk" of a
+unified diff. A comment is of three types, distinguished by the
+leading text of the intro:
-==========
-Review of attachment 123042:
+REMOVED: @@ -I,J @@
+
+ A comment about the line I+J-1 in the old version of the file.
+
+ADDED: @@ +K,L @@
+
+ A comment about the line K+L-1 in the new version of the file.
+
+CHANGED: @@ -I,J +K,L @@
+
+ A comment about the lines in the range [I,J) in the old version
+ of the file being turned into the range [K,L) in the new version
+ of the file.
+
+As with a unified diff, the header is followed by lines beginning
+with:
+
+ ' ' - a line of context
+ '-' - a removed line
+ '+' - an added line
+ '\' - suppresses the newline on the previous line
+
+ J = lines of context + added lines
+ L = lines of context + removed lines
+
+The actually text following the first character isn't important for
+the automated parsing of the review - it's there to make the review
+human readable. It would be normally be the line from the old or new
+version as appropriate, though long lines might possibly be truncated
+or white space normalized.
+
+In addition, an abbrevation is supported: if a line begins with
+
+ ... N <possibly more text>
-<overall comments>
+Then that means that there are N lines of the previous type omitted.
-::: foo/bar/somefile.c
-@@ -264,2 +264,3 @@ gjs_invoke_c_function(JSContext *context,
-+ return_values = NULL; /* Quiet gcc warning about initialization */
- if (n_return_values > 0) {
- if (invoke_ok) {
+After all the lines specified by the header have been used up, following
+lines up to the next line beginning with "@@" or ":::" is the text
+of the comment. (Leading and trailing whitespace on the comment is
+ignore.)
-<comment on change >
-@@ -318,1 +317,1 @@ import_file(JSContext *context,
-- if (!finish_import(context, obj))
-+ if (!finish_import(context, name))
+Example
+=======
+
+Given the unified diff
+
+==========
+--- animal-list
++++ animal-list
+@ -10,9 +10,7
+ Coati
+-Capybara
+-Dromedary
+-Kangaroo
+-Koala
+-Kookaburra
++Okapi
++Oppossum
+ Ptarmigan
+ Quail
+ Quetzalcoatl
+-Sturgeon
++Vole
+\ No newline at end of file
+==========
+
+A review might look like:
-<comment on change>
==========
+I don't really understand the rhyme and reason behind this change,
+it seems pretty random.
+
+::: animal-list
+@@ -10,6 +10,3 @@
+ Coati
+-Capybara
+-Dromedary
+... 3 more ...
++Okapi
++Oppossum
+Here you've removed three Australian animals without any replacements
+
+@@ -14,2 @@
+-Koala
+-Kookaburra
+
+I'm particularly sad to see the Kookaburra. It has two K's.
+
+@@ +15,2 @@
+ Quetzalcoatl
++Vole
+\ No newline at end of file
+
+The vole is a nice addition to the list. The last line should end
+with a newline too to keep 'vi' users happy.
+==========
+Note that it's OK for lines to appear in multiple comments - the lines
+'-Koala' '-Kookaburra' are in both the first comment and in the second
+comment. The one restriction is that two reviews of the same type
+can't end with the same line. (For a changed review, the pair of
+old-line and new-line must be distinct.)
diff --git a/js/patch.js b/js/patch.js
index 98afb2d..b2384e5 100644
--- a/js/patch.js
+++ b/js/patch.js
@@ -45,17 +45,12 @@ const CHANGED = 1 << 2; // Part of some other segmnet
const NEW_NONEWLINE = 1 << 3; // Old line doesn't end with \n
const OLD_NONEWLINE = 1 << 4; // New line doesn't end with \n
-// Note that we use this constructor both when parsing a patch and when
-// parsing a review in review.js
-function Hunk(oldStart, oldCount, newStart, newCount, text, parseComment) {
- this._init(oldStart, oldCount, newStart, newCount, text, parseComment);
+function Hunk(oldStart, oldCount, newStart, newCount, text) {
+ this._init(oldStart, oldCount, newStart, newCount, text);
}
Hunk.prototype = {
- _init : function(oldStart, oldCount, newStart, newCount, text, parseComment) {
- if (parseComment == null)
- parseComment = false;
-
+ _init : function(oldStart, oldCount, newStart, newCount, text) {
var rawlines = text.split("\n");
if (rawlines.length > 0 && Utils.strip(rawlines[rawlines.length - 1]) == "")
rawlines.pop(); // Remove trailing element from final \n
@@ -130,14 +125,10 @@ Hunk.prototype = {
} else if (op == '\\') {
// Handled with preceding line
} else {
- Utils.assertNotReached();
- }
-
- // When parsing a review, we stop when we've gotten all the lines described
- // in the chunk header - anything after that is the comment
- if (parseComment && totalOld >= oldCount && totalNew >= newCount) {
- this.comment = rawlines.slice(i + 1).join("\n");
- break;
+ // Junk in the patch - hope the patch got line wrapped and just ignoring
+ // it produces something meaningful. (For a patch displayer, anyways.
+ // would be bad for applying the patch.)
+ // Utils.assertNotReached();
}
}
@@ -192,13 +183,17 @@ File.prototype = {
getLocation : function(oldLine, newLine) {
for (var i = 0; i < this.hunks.length; i++) {
var hunk = this.hunks[i];
- if (hunk.oldStart > oldLine)
+ if (oldLine != null && hunk.oldStart > oldLine)
+ continue;
+ if (newLine != null && hunk.oldStart > newLine)
continue;
- if (oldLine < hunk.oldStart + hunk.oldCount) {
+ if ((oldLine != null && oldLine < hunk.oldStart + hunk.oldCount) ||
+ newLine != null && newLine < hunk.newStart + hunk.newCount) {
var location = -1;
hunk.iterate(function(loc, oldl, oldText, newl, newText, flags) {
- if (oldl == oldLine && newl == newLine)
+ if ((oldLine == null || oldl == oldLine) &&
+ (newLine == null || newl == newLine))
location = loc;
});
diff --git a/js/review.js b/js/review.js
index 39a0689..dc8ba4e 100644
--- a/js/review.js
+++ b/js/review.js
@@ -11,13 +11,14 @@ function _removeFromArray(a, element) {
}
}
-function Comment(file, location, comment) {
- this._init(file, location, comment);
+function Comment(file, location, type, comment) {
+ this._init(file, location, type, comment);
}
Comment.prototype = {
- _init : function(file, location, comment) {
+ _init : function(file, location, type, comment) {
this.file = file;
+ this.type = type;
this.location = location;
this.comment = comment;
},
@@ -34,11 +35,19 @@ function _noNewLine(flags, flag) {
return ((flags & flag) != 0) ? "\n\ No newline at end of file" : "";
}
+function _lineInSegment(line) {
+ return (line[2] & (Patch.ADDED | Patch.REMOVED | Patch.CHANGED)) != 0;
+}
+
function _compareSegmentLines(a, b) {
- var op1 = a.substr(0, 1);
- var op2 = b.substr(0, 1);
- if (op1 == op2)
+ var op1 = a[0];
+ var op2 = b[0];
+ if (op1 == op2)
return 0;
+ else if (op1 == ' ')
+ return -1;
+ else if (op2 == ' ')
+ return 1;
else
return op1 == '-' ? -1 : 1;
}
@@ -54,27 +63,32 @@ File.prototype = {
this.comments = [];
},
- addComment : function(location, comment) {
+ addComment : function(location, type, comment) {
var hunk = this.patchFile.getHunk(location);
var line = hunk.lines[location - hunk.location];
- comment = new Comment(this, location, comment);
+ comment = new Comment(this, location, type, comment);
if (line.reviewComments == null)
line.reviewComments = [];
line.reviewComments.push(comment);
for (var i = 0; i <= this.comments.length; i++) {
- if (i == this.comments.length || this.comments[i].location > location) {
+ if (i == this.comments.length ||
+ this.comments[i].location > location ||
+ (this.comments[i].location == location && this.comments[i].type > type)) {
this.comments.splice(i, 0, comment);
break;
- } else if (this.comments[i].location == location) {
+ } else if (this.comments[i].location == location &&
+ this.comments[i].type == type) {
throw "Two comments at the same location";
- break;
}
}
+
+ return comment;
},
- getComment : function(location, comment) {
+ getComment : function(location, type) {
for (var i = 0; i < this.comments.length; i++)
- if (this.comments[i].location == location)
+ if (this.comments[i].location == location &&
+ this.comments[i].type == type)
return this.comments[i];
return null;
@@ -86,7 +100,6 @@ File.prototype = {
str += '\n';
var first = true;
- var lastCommentLocation = 0;
for (var i = 0; i < this.comments.length; i++) {
if (first)
first = false;
@@ -94,65 +107,145 @@ File.prototype = {
str += '\n';
var comment = this.comments[i];
var hunk = this.patchFile.getHunk(comment.location);
- var context = Math.min(comment.location - lastCommentLocation - 1,
- comment.location - hunk.location,
- 2);
- var patchOldStart, patchNewStart;
+ // Find the range of lines we might want to show. That's everything in the
+ // same segment as the commented line, plus up two two lines of non-comment
+ // diff before.
+
+ var contextFirst = comment.location - hunk.location;
+ if (_lineInSegment(hunk.lines[contextFirst])) {
+ while (contextFirst > 0 && _lineInSegment(hunk.lines[contextFirst - 1]))
+ contextFirst--;
+ }
+
+ var j;
+ for (j = 0; j < 2; j++)
+ if (contextFirst > 0 && !_lineInSegment(hunk.lines[contextFirst - 1]))
+ contextFirst--;
+
+ // Now get the diff lines (' ', '-', '+' for that range of lines)
+
+ var patchOldStart = null;
+ var patchNewStart = null;
var patchOldLines = 0;
var patchNewLines = 0;
+ var unchangedLines = 0;
var patchLines = [];
- hunk.iterate(function(loc, oldLine, oldText, newLine, newText, flags) {
- if (loc == comment.location - context) {
- patchOldStart = oldLine;
- patchNewStart = newLine;
- }
+ function addOldLine(oldLine) {
+ if (patchOldLines == 0)
+ patchOldStart = oldLine;
+ patchOldLines++;
+ }
+
+ function addNewLine(newLine) {
+ if (patchNewLines == 0)
+ patchNewStart = newLine;
+ patchNewLines++;
+ }
- if (loc >= comment.location - context && loc <= comment.location) {
- if (oldText != null)
- patchOldLines++;
- if (newText != null)
- patchNewLines++;
- if ((flags & (Patch.ADDED | Patch.REMOVED | Patch.CHANGED)) != 0) {
- if (oldText != null)
+ hunk.iterate(function(loc, oldLine, oldText, newLine, newText, flags) {
+ if (loc >= hunk.location + contextFirst && loc <= comment.location) {
+ if ((flags & (Patch.ADDED | Patch.REMOVED | Patch.CHANGED)) == 0) {
+ patchLines.push(' ' + oldText + _noNewLine(flags, Patch.OLD_NONEWLINE |
Patch.NEW_NONEWLINE));
+ addOldLine(oldLine);
+ addNewLine(newLine);
+ unchangedLines++;
+ } else {
+ if ((comment.type == Patch.REMOVED || comment.type == Patch.CHANGED) &&
oldText != null) {
patchLines.push('-' + oldText +_noNewLine(flags,
Patch.OLD_NONEWLINE));
- if (newText != null)
+ addOldLine(oldLine);
+ }
+ if ((comment.type == Patch.ADDED || comment.type == Patch.CHANGED) &&
newText != null) {
patchLines.push('+' + newText + _noNewLine(flags,
Patch.NEW_NONEWLINE));
- } else {
- patchLines.push(' ' + oldText + _noNewLine(flags, Patch.OLD_NONEWLINE |
Patch.NEW_NONEWLINE));
+ addNewLine(newLine);
+ }
}
}
});
- var segStart = 0;
- for (var k = 0; k <= patchLines.length; k++) {
- if (k == patchLines.length || patchLines[k].substr(0, 1) == ' ') {
- if (segStart < k) {
- var segmentLines = patchLines.slice(segStart, k);
- segmentLines.sort(_compareSegmentLines);
- for (var l = 0; l < segmentLines.length; l++)
- patchLines[segStart + l] = segmentLines[l];
- }
- segStart = k + 1;
- }
- }
+ // Sort them into global order ' ', '-', '+'
+ patchLines.sort(_compareSegmentLines);
- while (Utils.strip(patchLines[0]) == '') {
+ // Completely blank context isn't useful so remove it
+ while (patchLines[0].match(/^\s*$/)) {
patchLines.shift();
patchOldStart++;
patchNewStart++;
patchOldLines--;
patchNewLines--;
+ unchangedLines--;
}
- str += '@@ -' + patchOldStart + ',' + patchOldLines + ' +' + patchNewStart + ',' + patchNewLines
+ ' @@\n';
- str += patchLines.join("\n");
- str += "\n\n";
+ if (comment.type == Patch.CHANGED) {
+ // For a CHANGED comment, we have to show the the start of the hunk - but to save
+ // in length we can trim unchanged context before it
+
+ if (patchOldLines + patchNewLines - unchangedLines > 5) {
+ var toRemove = Math.min(unchangedLines, patchOldLines + patchNewLines - unchangedLines -
5);
+ patchLines.splice(0, toRemove);
+ patchOldStart += toRemove;
+ patchNewStart += toRemove;
+ patchOldLines -= toRemove;
+ patchNewLines -= toRemove;
+ unchangedLines -= toRemove;
+ }
+
+ str += '@@ -' + patchOldStart + ',' + patchOldLines + ' +' + patchNewStart + ',' +
patchNewLines + ' @@\n';
+
+ // We will use up to 8 lines more:
+ // 4 old lines or 3 old lines and a "... <N> more ... " line
+ // 4 new lines or 3 new lines and a "... <N> more ... " line
+
+ var patchRemovals = patchOldLines - unchangedLines;
+ var showPatchRemovals = patchRemovals > 4 ? 3 : patchRemovals;
+ var patchAdditions = patchNewLines - unchangedLines;
+ var showPatchAdditions = patchAdditions > 4 ? 3 : patchAdditions;
+
+ j = 0;
+ while (j < unchangedLines + showPatchRemovals) {
+ str += patchLines[j];
+ str += "\n";
+ j++;
+ }
+ if (showPatchRemovals < patchRemovals) {
+ str += "... ";
+ str += patchRemovals - showPatchRemovals;
+ str += " more ...\n";
+ j += patchRemovals - showPatchRemovals;
+ }
+ while (j < unchangedLines + patchRemovals + showPatchAdditions) {
+ str += patchLines[j];
+ str += "\n";
+ j++;
+ }
+ if (showPatchAdditions < patchAdditions) {
+ str += "... ";
+ str += patchAdditions - showPatchAdditions;
+ str += " more ...\n";
+ j += patchAdditions - showPatchAdditions;
+ }
+ } else {
+ // We limit Patch.ADDED/Patch.REMOVED comments strictly to 3 lines after the header
+ if (patchOldLines + patchNewLines - unchangedLines > 3) {
+ var toRemove = patchOldLines + patchNewLines - unchangedLines - 3;
+ patchLines.splice(0, toRemove);
+ patchOldStart += toRemove;
+ patchNewStart += toRemove;
+ patchOldLines -= toRemove;
+ patchNewLines -= toRemove;
+ }
+
+ if (comment.type == Patch.REMOVED)
+ str += '@@ -' + patchOldStart + ',' + patchOldLines + ' @@\n';
+ else
+ str += '@@ +' + patchNewStart + ',' + patchNewLines + ' @@\n';
+ str += patchLines.join("\n");
+ str += "\n";
+ }
+ str += "\n";
str += comment.comment;
str += "\n";
-
- lastCommentLocation = comment.location;
}
return str;
@@ -174,7 +267,7 @@ const FILE_START_RE = /^:::[ \t]+(\S+)[ \t]*\n/mg;
//
// Hunk start: @@ -23,12 +30,11 @@
// Followed by: lines that don't start with @@ or :::
-const HUNK_RE = /^@@[ \t]+-(\d+),(\d+)[ \t]+\+(\d+),(\d+)[ \t]+@@.*\n((?:(?!@@|:::).*\n?)*)/mg;
+const HUNK_RE = /^@@[ \t]+(?:-(\d+),(\d+)[ \t]+)?(?:\+(\d+),(\d+)[ \t]+)?@@.*\n((?:(?!@@|:::).*\n?)*)/mg;
Review.prototype = {
_init : function(patch) {
@@ -216,34 +309,94 @@ Review.prototype = {
break;
pos = HUNK_RE.lastIndex;
- var oldStart = parseInt(m2[1]);
- var oldCount = parseInt(m2[2]);
- var newStart = parseInt(m2[3]);
- var newCount = parseInt(m2[4]);
-
- var hunk = new Patch.Hunk(oldStart, oldCount, newStart, newCount, m2[5], true);
-
- // Numbering of old/new line numbers is
- //
- // A B
- // 1 1 2 3 3 4 5 5 5 6 7
- // * * * * * * *
- //
- // Where the * represent lines actually in that version of the file.
- // So, the difference in line numbers between A and B is the number of
- // *'s from A to B, not including B.
-
- var oldLine = hunk.oldStart + hunk.oldCount;
- var newLine = hunk.newStart + hunk.newCount;
-
- var lastLine = hunk.lines[hunk.lines.length - 1];
- if (lastLine[0] != null)
+
+ var oldStart, oldCount, newStart, newCount;
+ if (m2[1] != null) {
+ oldStart = parseInt(m2[1]);
+ oldCount = parseInt(m2[2]);
+ } else {
+ oldStart = oldCount = null;
+ }
+
+ if (m2[3] != null) {
+ newStart = parseInt(m2[3]);
+ newCount = parseInt(m2[4]);
+ } else {
+ newStart = newCount = null;
+ }
+
+ var type;
+ if (oldStart != null && newStart != null)
+ type = Patch.CHANGED;
+ else if (oldStart != null)
+ type = Patch.REMOVED;
+ else if (newStart != null)
+ type = Patch.ADDED;
+ else
+ throw "Either old or new line numbers must be given";
+
+ var oldLine = oldStart;
+ var newLine = newStart;
+
+ var rawlines = m2[5].split("\n");
+ if (rawlines.length > 0 && rawlines[rawlines.length - 1].match('^/s+$'))
+ rawlines.pop(); // Remove trailing element from final \n
+
+ var commentText = null;
+
+ var lastSegmentOld = 0;
+ var lastSegmentNew = 0;
+ for (var i = 0; i < rawlines.length; i++) {
+ var line = rawlines[i];
+ var count = 1;
+ if (i < rawlines.length - 1 && rawlines[i + 1].match(/^... \d+\s+/)) {
+ var m3 = /^\.\.\.\s+(\d+)\s+/.exec(rawlines[i + 1]);
+ count += parseInt(m3[1]);
+ i += 1;
+ }
+ if (line.match(/^ /)) {
+ oldLine += count;
+ newLine += count;
+ lastSegmentOld = 0;
+ lastSegmentNew = 0;
+ } else if (line.match(/^-/)) {
+ oldLine += count;
+ lastSegmentOld += count;
+ } else if (line.match(/^\+/)) {
+ newLine += count;
+ lastSegmentNew += count;
+ } else if (line.match(/^\\/)) {
+ // '\ No newline at end of file' - ignore
+ } else {
+ throw "Bad content in hunk: " + line;
+ }
+
+ if ((oldStart == null || oldLine == oldStart + oldCount) &&
+ (newStart == null || newLine == newStart + newCount)) {
+ commentText = rawlines.slice(i + 1).join("\n");
+ break;
+ }
+ }
+
+ if (commentText == null)
+ throw "No comment found in hunk";
+
+
+ var location;
+ if (type == Patch.CHANGED) {
+ if (lastSegmentOld >= lastSegmentNew)
+ oldLine--;
+ if (lastSegmentOld <= lastSegmentNew)
+ newLine--;
+ location = file.patchFile.getLocation(oldLine, newLine);
+ } else if (type == Patch.REMOVED) {
oldLine--;
- if (lastLine[1] != null)
+ location = file.patchFile.getLocation(oldLine, null);
+ } else if (type == Patch.ADDED) {
newLine--;
-
- var location = file.patchFile.getLocation(oldLine, newLine);
- file.addComment(location, Utils.strip(hunk.comment));
+ location = file.patchFile.getLocation(null, newLine);
+ }
+ file.addComment(location, type, Utils.strip(commentText));
}
FILE_START_RE.lastIndex = pos;
diff --git a/js/splinter.js b/js/splinter.js
index 508be32..bfd003a 100644
--- a/js/splinter.js
+++ b/js/splinter.js
@@ -129,66 +129,123 @@ function getQueryParams() {
return params;
}
+function ensureCommentArea(row) {
+ if (!row.nextSibling || row.nextSibling.className != "comment-area")
+ $("<tr class='comment-area'><td colSpan='3'>"
+ + "</td></tr>")
+ .insertAfter(row);
+
+ return row.nextSibling.firstChild;
+}
+
+function getTypeClass(type) {
+ switch (type) {
+ case Patch.ADDED:
+ return "comment-added";
+ case Patch.REMOVED:
+ return "comment-removed";
+ case Patch.CHANGED:
+ return "comment-changed";
+ }
+
+ return null;
+}
-function saveComment(row, editorQuery, file, location) {
+function getSeparatorClass(type) {
+ switch (type) {
+ case Patch.ADDED:
+ return "comment-separator-added";
+ case Patch.REMOVED:
+ return "comment-separator-removed";
+ }
+
+ return null;
+}
+
+function addCommentDisplay(row, comment, commentorIndex) {
+ var commentArea = ensureCommentArea(row);
+
+ var separatorClass = getSeparatorClass(comment.type);
+ if (separatorClass)
+ $("<div></div>")
+ .addClass(separatorClass)
+ .addClass("comment-"+ commentorIndex)
+ .appendTo(commentArea);
+
+ $("<div class='comment'>"
+ + "<div class='comment-frame'>"
+ + "<div class='comment-text'></div>"
+ + "</div>"
+ + "</div>")
+ .find(".comment-text").text(comment.comment).end()
+ .addClass(getTypeClass(comment.type))
+ .addClass("comment-"+ commentorIndex)
+ .appendTo(commentArea)
+ .dblclick(function() {
+ insertCommentEditor(row, comment.type);
+ });
+}
+
+function saveComment(row, file, location, type) {
+ var commentArea = ensureCommentArea(row);
var reviewFile = theReview.getFile(file.filename);
- var comment = reviewFile.getComment(location);
+ var comment = reviewFile.getComment(location, type);
- var value = Utils.strip(editorQuery.find("textarea").val());
+ var value = Utils.strip($(commentArea).find("textarea").val());
if (value != "") {
if (comment)
comment.comment = value;
else
- reviewFile.addComment(location, value);
+ comment = reviewFile.addComment(location, type, value);
- $("<tr class='my-comment'><td colSpan='3'>"
- + "<div></div>"
- + "</td></tr>")
- .find("div").text(value).end()
- .insertBefore(editorQuery)
- .dblclick(function() {
- insertCommentEditor(row);
- });
+ addCommentDisplay(row, comment, 0);
} else {
if (comment)
comment.remove();
}
- editorQuery.remove();
+ if (reviewFile.comments.length == 0) {
+ $(commentArea).parent().remove();
+ } else {
+ $(commentArea).find(".comment-editor").remove();
+ }
}
-function insertCommentEditor(row) {
- var file = $(row).data('patchFile');
- var location = $(row).data('patchLocation');
-
- var insertAfter = row;
- while (insertAfter.nextSibling) {
- if (insertAfter.nextSibling.className == "comment-editor")
- return;
- if (insertAfter.nextSibling.className == "my-comment") {
- $(insertAfter.nextSibling).remove();
- if (!insertAfter.nextSibling)
- break;
- }
- if (insertAfter.nextSibling.className != "comment")
- break;
- insertAfter = insertAfter.nextSibling;
- }
+function insertCommentEditor(clickRow, clickType) {
+ var file = $(clickRow).data('patchFile');
+ var clickLocation = $(clickRow).data('patchLocation');
+
+ var row = clickRow;
+ var location = clickLocation;
+ var type = clickType;
var reviewFile = theReview.getFile(file.filename);
- var comment = reviewFile.getComment(location);
- var editorRow = $("<tr class='comment-editor'><td colSpan='3'>"
- + "<div>"
- + "<textarea></textarea>"
- + "</div>"
- + "</td></tr>");
- editorRow.insertAfter(insertAfter);
- editorRow.find('textarea')
- .text(comment ? comment.comment : "")
- .blur(function() {
- saveComment(row, editorRow, file, location);
- })
- .each(function() { this.focus(); });
+ var comment = reviewFile.getComment(location, type);
+
+ var commentArea = ensureCommentArea(row);
+
+ var typeClass = getTypeClass(type);
+ var separatorClass = getSeparatorClass(type);
+
+ if (comment) {
+ if (separatorClass)
+ $(commentArea).find(".comment-0." + separatorClass).remove();
+ $(commentArea).find(".comment-0." + typeClass).remove();
+ }
+
+ if (separatorClass)
+ $("<div class='comment-editor'></div>")
+ .addClass(separatorClass)
+ .appendTo(commentArea);
+ $("<div class='comment-editor'><textarea></textarea></div>")
+ .addClass(typeClass)
+ .appendTo(commentArea)
+ .find('textarea')
+ .val(comment ? comment.comment : "")
+ .blur(function() {
+ saveComment(row, file, location, type);
+ })
+ .each(function() { this.focus(); });
}
function EL(element, cls, text) {
@@ -255,23 +312,29 @@ function addPatchFile(file) {
$(tr).data('patchFile', file);
$(tr).data('patchLocation', loc);
- $(tr).dblclick(function() {
- insertCommentEditor(this);
+ $(tr).dblclick(function(e) {
+ var leftX = this.offsetLeft;
+ var parent = this.offsetParent;
+ while (parent != document.body) {
+ leftX += parent.offsetLeft;
+ parent = parent.offsetParent;
+ }
+ var delta = e.pageX - (leftX + this.offsetWidth/2);
+ var type;
+ if (delta < - 20)
+ type = Patch.REMOVED;
+ else if (delta < 20)
+ type = Patch.CHANGED;
+ else
+ type = Patch.ADDED;
+ insertCommentEditor(this, type);
});
tbody.appendChild(tr);
- if (line.reviewComments != null) {
- for (var k = 0; k < line.reviewComments.length; k++) {
- var comment = line.reviewComments[k];
-
- $("<tr class='comment'><td colSpan='3'>"
- + "<div></div>"
- + "</td></tr>")
- .find("div").text(comment.comment).end()
- .appendTo(tbody);
- }
- }
+ if (line.reviewComments != null)
+ for (var k = 0; k < line.reviewComments.length; k++)
+ addCommentDisplay(tr, line.reviewComments[k], 1);
});
}
}
diff --git a/tests/review.jst b/tests/review.jst
index 3bc3edf..1e24584 100644
--- a/tests/review.jst
+++ b/tests/review.jst
@@ -53,12 +53,12 @@ I like this patch
let argC = r.getFile('gi/arg.c');
let loc = argC.patchFile.getLocation(216,215);
-r.getFile('gi/arg.c').addComment(loc, 'Should you have removed elem?');
+r.getFile('gi/arg.c').addComment(loc, Patch.REMOVED, 'Should you have removed elem?');
assertEquals(<<<
I like this patch
::: gi/arg.c
-@@ -214,3 +214,1 @@
+@@ -214,3 @@
{
- guint32 i;
- jsval elem;
@@ -67,12 +67,12 @@ Should you have removed elem?
, r.toString());
let loc = argC.patchFile.getLocation(1130,1128);
-r.getFile('gi/arg.c').addComment(loc, 'This comment seems unnecessary');
+r.getFile('gi/arg.c').addComment(loc, Patch.CHANGED, 'This comment seems unnecessary');
assertEquals(<<<
I like this patch
::: gi/arg.c
-@@ -214,3 +214,1 @@
+@@ -214,3 @@
{
- guint32 i;
- jsval elem;
@@ -90,12 +90,12 @@ This comment seems unnecessary
, r.toString());
loc = argC.patchFile.getLocation(1128,1126);
-argC.addComment(loc, "Why this transfer?");
+argC.addComment(loc, Patch.CHANGED, "Why this transfer?");
assertEquals(<<<
I like this patch
::: gi/arg.c
-@@ -214,3 +214,1 @@
+@@ -214,3 @@
{
- guint32 i;
- jsval elem;
@@ -108,7 +108,8 @@ Should you have removed elem?
Why this transfer?
-@@ -1129,2 +1127,2 @@
+@@ -1128,3 +1126,3 @@
+ if (transfer == GI_TRANSFER_EVERYTHING)
- /* We're done */
- return;
+ /* Success! */
@@ -117,14 +118,87 @@ Why this transfer?
This comment seems unnecessary
, r.toString());
-
let r2 = new Review.Review(p);
r2.parse(r.toString());
assertEquals(r.toString(), r2.toString());
loc = argC.patchFile.getLocation(216,215);
-let comment = argC.getComment(loc);
+let comment = argC.getComment(loc, Patch.REMOVED);
assertEquals(loc, comment.location);
+assertEquals(Patch.REMOVED, comment.type);
comment.remove();
assertEquals(null, argC.getComment(loc));
+
+let patch_text = <<<
+diff a/foo.c b/foo.c
+--- a/foo.c
++++ b/foo.c
+@@ -1,16 +1,16 @@
+ C1
+ C2
+ C3
+ C4
+-A5
+-A6
+-A7
+-A8
+-A9
+-A10
+-A11
+-A12
++B5
++B6
++B7
++B8
++B9
++B10
++B11
++B12
+ C13
+ C14
+ C15
+ C16
+>>>;
+
+p = new Patch.Patch(patch_text);
+r = new Review.Review(p);
+
+let f = r.getFile('foo.c');
+f.addComment(4, Patch.REMOVED, "Line A5 was removed")
+f.addComment(5, Patch.ADDED, "Line B6 was added")
+f.addComment(11, Patch.CHANGED, "8 lines were changed")
+
+assertEquals(<<<
+::: foo.c
+@@ -3,3 @@
+ C3
+ C4
+-A5
+
+Line A5 was removed
+
+@@ +4,3 @@
+ C4
++B5
++B6
+
+Line B6 was added
+
+@@ -5,8 +5,8 @@
+-A5
+-A6
+-A7
+... 5 more ...
++B5
++B6
++B7
+... 5 more ...
+
+8 lines were changed
+>>>, r.toString())
+
+let r2 = new Review.Review(p);
+r2.parse(r.toString());
+assertEquals(r.toString(), r2.toString());
+
diff --git a/web/index.html b/web/index.html
index b2d2eec..ea8693b 100644
--- a/web/index.html
+++ b/web/index.html
@@ -39,6 +39,8 @@
</table>
</div>
<div id="controls" style="display: none;">
+ <div id="patchIntro">
+ </div>
<div id="oldReviews">
</div>
<div>
diff --git a/web/splinter.css b/web/splinter.css
index e2ed0c9..1ae9185 100644
--- a/web/splinter.css
+++ b/web/splinter.css
@@ -120,31 +120,29 @@ body {
width: 3px;
}
-.my-comment {
- border: 1px solid black;
+.comment-removed {
+ width: 50%;
+ float: left;
}
-.my-comment td {
- padding: 0px;
+.comment-changed {
+ margin-left: 25%;
+ margin-right: 25%;
+ clear: both;
}
-.my-comment div {
- padding: 0.5em;
- border-left: 10px solid green;
- white-space: pre-wrap;
+.comment-added {
+ width: 50%;
+ float: right;
}
-.comment {
+.comment-frame {
border: 1px solid black;
+ margin: 5px;
}
-.comment td {
- padding: 0px;
-}
-
-.comment div {
+.comment-text {
padding: 0.5em;
- border-left: 10px solid blue;
white-space: pre-wrap;
}
@@ -152,3 +150,19 @@ body {
width: 100%;
height: 10em;
}
+
+.comment-0 .comment-text {
+ border-left: 10px solid green;
+}
+
+.comment-1 .comment-text {
+ border-left: 10px solid blue;
+}
+
+.comment-separator-removed {
+ clear: left;
+}
+
+.comment-separator-added {
+ clear: right;
+}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
