Commit 1414c99f authored by abarth@webkit.org's avatar abarth@webkit.org
Browse files

2010-09-06 Adam Barth <abarth@webkit.org>

        Reviewed by Eric Seidel.

        [reviewtool] Add a field for overall comments
        https://bugs.webkit.org/show_bug.cgi?id=45273

        This patch does a couple logically separate things that could be
        separated into smaller patches:

        1) This patch adds an "overall comments" field where you can enter
           overall comments about the patch.  These comments appear at the top
           of the bugzilla posting.  Currently, these aren't redisplayed when
           viewing the patch, but I plan to add that in a future patch.

        2) This patch renames some of the CSS classes to more consistently
           follow the camelCase style that PrettyPatch uses.

        3) This patch moves the "prepare comments" button to the left of the
           toolbar and renames is to "publish comments".  This makes more sense
           when you scroll to the bottom of the page and enter in some overall
           comments.

        4) When you attempt to add a comment to a line that already has a
           "frozen" comment, we now unfreeze the comment instead of doing
           nothing.  The old behavior was kind of frustrating if you didn't
           know that you could unfreeze a comment by clicking on it.

        * PrettyPatch/PrettyPatch.rb:
            - Update CSS.
        * code-review.js:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@66849 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent c6c1079a
2010-09-06 Adam Barth <abarth@webkit.org>
Reviewed by Eric Seidel.
[reviewtool] Add a field for overall comments
https://bugs.webkit.org/show_bug.cgi?id=45273
This patch does a couple logically separate things that could be
separated into smaller patches:
1) This patch adds an "overall comments" field where you can enter
overall comments about the patch. These comments appear at the top
of the bugzilla posting. Currently, these aren't redisplayed when
viewing the patch, but I plan to add that in a future patch.
2) This patch renames some of the CSS classes to more consistently
follow the camelCase style that PrettyPatch uses.
3) This patch moves the "prepare comments" button to the left of the
toolbar and renames is to "publish comments". This makes more sense
when you scroll to the bottom of the page and enter in some overall
comments.
4) When you attempt to add a comment to a line that already has a
"frozen" comment, we now unfreeze the comment instead of doing
nothing. The old behavior was kind of frustrating if you didn't
know that you could unfreeze a comment by clicking on it.
* PrettyPatch/PrettyPatch.rb:
- Update CSS.
* code-review.js:
2010-09-06 Adam Barth <abarth@webkit.org>
[reviewtool] Tweak the ok button to cancel the comment if the comment
......
......@@ -188,12 +188,6 @@ h1 :hover {
/* Support for inline comments */
.previous_comment {
border: inset 1px;
padding: 5px;
background-color: #ffd;
}
.author {
font-style: italic;
}
......@@ -206,7 +200,7 @@ h1 :hover {
position: relative;
}
.comment textarea {
.comment textarea, .overallComments textarea {
width: 100%;
height: 6em;
}
......@@ -226,6 +220,10 @@ body {
}
#toolbar .actions {
float: left;
}
#toolbar .message {
float: right;
}
......@@ -262,11 +260,6 @@ body {
height: 100%;
}
.help {
color: gray;
font-style: italic;
}
.commentContext .lineNumber {
background-color: yellow;
}
......@@ -276,6 +269,27 @@ body {
border-bottom-color: #69F;
border-right-color: #69F;
}
.help {
color: gray;
font-style: italic;
}
.description {
font-style: italic;
}
.comment, .overallComments, .previousComment, .frozenComment {
background-color: #ffd;
}
.overallComments {
padding: 5px;
}
.previousComment, .frozenComment {
border: inset 1px; padding: 5px;
}
</style>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script>
<script src="code-review.js?version=5"></script>
......
......@@ -45,7 +45,7 @@
function findCommentPositionFor(line) {
var position = line;
while (position.next() && position.next().hasClass('previous_comment'))
while (position.next() && position.next().hasClass('previousComment'))
position = position.next();
return position;
}
......@@ -62,8 +62,12 @@
}
function addCommentFor(line) {
if (line.attr('data-has-comment'))
if (line.attr('data-has-comment')) {
// FIXME: This query is overly complex because we place comment blocks
// after Lines. Instead, comment blocks should be children of Lines.
findCommentPositionFor(line).next().next().filter('.frozenComment').each(unfreezeComment);
return;
}
line.attr('data-has-comment', 'true');
line.addClass('commentContext');
......@@ -82,7 +86,7 @@
var files = {}
function addPreviousComment(line, author, comment_text) {
var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previous_comment"></div>');
var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previousComment"></div>');
var author_block = $('<div class="author"></div>').text(author + ':');
comment_block.text(comment_text).prepend(author_block).each(hoverify).click(addCommentField);
insertCommentFor(line, comment_block);
......@@ -180,8 +184,9 @@
$(document).ready(function() {
crawlDiff();
fetchHistory();
$(document.body).prepend('<div id="toolbar"><div class="actions"><button id="post_comments">Prepare comments</button></div><span class="commentStatus"></span> <span class="help">Double-click a line to add a comment.</span></div>');
$(document.body).prepend('<div id="toolbar"><div class="actions"><button id="post_comments">Publish comments</button></div><div class="message"><span class="commentStatus"></span> <span class="help">Double-click a line to add a comment.</span></div></div>');
$(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe src="attachment.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
$(document.body).append('<div class="overallComments"><div class="description">Overall comments:</div><textarea></textarea></div>');
});
function cancelComment() {
......@@ -192,6 +197,11 @@
trimCommentContextToBefore(line);
}
function unfreezeComment() {
$(this).prev().show();
$(this).remove();
}
$('.comment .cancel').live('click', cancelComment);
$('.comment .ok').live('click', function() {
......@@ -202,13 +212,10 @@
}
var line_id = comment_textarea.attr('data-comment-for');
var line = $('#' + line_id)
findCommentBlockFor(line).hide().after($('<div class="frozen-comment"></div>').text(comment_textarea.val()));
findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
});
$('.frozen-comment').live('click', function() {
$(this).prev().show();
$(this).remove();
});
$('.frozenComment').live('click', unfreezeComment);
function focusOn(comment) {
$('.focused').removeClass('focused');
......@@ -218,7 +225,7 @@
}
function focusNextComment() {
var comments = $('.previous_comment');
var comments = $('.previousComment');
if (comments.length == 0)
return;
var index = comments.index($('.focused'));
......@@ -227,7 +234,7 @@
}
function focusPreviousComment() {
var comments = $('.previous_comment');
var comments = $('.previousComment');
if (comments.length == 0)
return;
var index = comments.index($('.focused'));
......@@ -346,13 +353,16 @@
if (line.attr('data-has-comment') != 'true')
return;
var snippet = snippetFor(line);
var comment = findCommentBlockFor(line).children('textarea').val();
var comment = findCommentBlockFor(line).children('textarea').val().trim();
if (comment == '')
return;
comments_in_context.push(snippet + '\n' + comment);
});
$('#comment_form').removeClass('inactive');
var comment = comments_in_context.join('\n\n');
var comment = $('.overallComments textarea').val().trim();
if (comment != '')
comment += '\n\n';
comment += comments_in_context.join('\n\n');
if (comment != '')
comment = 'View in context: ' + window.location + '\n\n' + comment;
$('#comment_form').find('iframe').contents().find('#comment').val(comment);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment