2011-01-12 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jan 2011 23:27:42 +0000 (23:27 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jan 2011 23:27:42 +0000 (23:27 +0000)
        Reviewed by Adam Barth.

        Bugzilla: Add keyboard shortcuts to jump to next change
        https://bugs.webkit.org/show_bug.cgi?id=52305

        Comments and diff blocks go in the same queue. If you have a
        comment focused, then j/k will focus the next/prev diff block
        with respect to that comment.

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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@75646 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/bugs.webkit.org/ChangeLog
Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
Websites/bugs.webkit.org/code-review.js

index 6e2ee04..6b5f678 100644 (file)
@@ -2,6 +2,20 @@
 
         Reviewed by Adam Barth.
 
+        Bugzilla: Add keyboard shortcuts to jump to next change
+        https://bugs.webkit.org/show_bug.cgi?id=52305
+
+        Comments and diff blocks go in the same queue. If you have a 
+        comment focused, then j/k will focus the next/prev diff block
+        with respect to that comment.
+
+        * PrettyPatch/PrettyPatch.rb:
+        * code-review.js:
+
+2011-01-12  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
         show shared lines on both sides in code review tool
         https://bugs.webkit.org/show_bug.cgi?id=52308
 
index 84b1ff2..6c8cb3c 100644 (file)
@@ -374,7 +374,8 @@ body {
 }
 
 .focused {
-  border: 1px solid blue;
+  outline: 1px solid blue;
+  outline-offset: -1px;
 }
 
 .statusBubble {
index b0786f7..696a08a 100644 (file)
     });
   }
 
-  function diffSectionFrom(line) {
+  function fileDiffFor(line) {
     return line.parents('.FileDiff');
   }
 
   function activeCommentFor(line) {
     // Scope to the diffSection as a performance improvement.
-    return $('textarea[data-comment-for~="' + line[0].id + '"]', diffSectionFrom(line));
+    return $('textarea[data-comment-for~="' + line[0].id + '"]', fileDiffFor(line));
   }
 
   function previousCommentsFor(line) {
     // Scope to the diffSection as a performance improvement.
-    return $('div[data-comment-for~="' + line[0].id + '"].previousComment', diffSectionFrom(line));
+    return $('div[data-comment-for~="' + line[0].id + '"].previousComment', fileDiffFor(line));
   }
 
   function findCommentPositionFor(line) {
         addPreviousComment(line, author, comment_text);
       });
     }
-    if (comments.length == 0)
+
+    var help_text = 'Scroll though diffs with the "j" and "k" keys.';
+    if (comments.length == 0) {
+      $('#message .commentStatus').text(help_text);
       return;
+    }
+
     descriptor = comments.length + ' comment';
     if (comments.length > 1)
       descriptor += 's';
-    $('#message .commentStatus').text('This patch has ' + descriptor + '.  Scroll through them with the "n" and "p" keys.');
+    $('#message .commentStatus').text('This patch has ' + descriptor + '.  Scroll through them with the "n" and "p" keys. ' + help_text);
   }
 
   function scanForComments(author, text) {
     findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
   });
 
-  function focusOn(comment) {
+  function focusOn(node) {
     $('.focused').removeClass('focused');
-    if (comment.length == 0)
+    if (node.length == 0)
       return;
-    $(document).scrollTop(comment.addClass('focused').position().top - window.innerHeight/2);
+    $(document).scrollTop(node.addClass('focused').position().top - window.innerHeight / 2);
   }
 
-  function focusNextComment() {
-    var comments = $('.previousComment');
-    if (comments.length == 0)
-      return;
-    var index = comments.index($('.focused'));
-    // Notice that -1 gets mapped to 0.
-    focusOn($(comments.get(index + 1)));
+  function diffBlockEndPoint(focusable_nodes, line, line_offset, is_backward) {
+    if (!line_offset && is_backward)
+      return line;
+
+    var offset = is_backward ? -1 : 1;
+
+    // If we're at a comment, get a Line in the diff block that contains the comment.
+    if (line.hasClass('previousComment')) {
+      var line_for_comment = $('#' + line.attr('data-comment-for'));
+      line_offset = focusable_nodes.index(line_for_comment);
+
+      // If the comment is not inside a diff block, return the comment node.
+      if (line_offset == -1)
+        return line;
+
+      line = line_for_comment;
+    }
+
+    // Find the Line at the beginning/end of this contiguous block of lines.
+    // Contiguous blocks of lines have contiguous IDs and are contained in the same FileDiff.
+    // Skip over comment nodes.
+    var id = numberFrom(line.attr('id'));
+    var next_node = $(focusable_nodes[line_offset + offset]);
+    while (next_node.size()
+           && (!next_node.hasClass('Line') || next_node.attr('id') == 'line' + (id + offset))
+           && fileDiffFor(line)[0] == fileDiffFor(next_node)[0]) {
+      if (next_node.hasClass('Line')) {
+        line = next_node;
+        id += offset;
+      }
+
+      line_offset += offset;
+      next_node = $(focusable_nodes[line_offset + offset]);
+    }
+    return line;
   }
 
-  function focusPreviousComment() {
-    var comments = $('.previousComment');
-    if (comments.length == 0)
-      return;
-    var index = comments.index($('.focused'));
-    if (index == -1)
-      index = comments.length;
-    if (index == 0) {
-      focusOn([]);
+  function focusNext(className, is_backward) {
+    var focusable_nodes = $('.previousComment,.Line.add,.Line.remove');
+    var focused_node = $('.focused');
+    var index = focusable_nodes.index(focused_node);
+    if (is_backward && (!index || index == -1))
       return;
+
+    if (focused_node.size() && className == 'Line') {
+      focused_node = diffBlockEndPoint(focusable_nodes, focused_node, index, is_backward);
+      index = focusable_nodes.index(focused_node);
+    }
+
+    var end = is_backward ? 0 : focusable_nodes.size();
+    var offset = is_backward ? -1 : 1;
+    for (var i = index + offset; i != end; i = i + offset) {
+      var node = $(focusable_nodes[i]);
+      if (node.hasClass(className)) {
+        if (className == 'Line') {
+          // Pass in true for is_backward because we always want to focus the start of the diff block.
+          node = diffBlockEndPoint(focusable_nodes, node, i, true);
+        }
+        focusOn(node);
+        return;
+      }
     }
-    focusOn($(comments.get(index - 1)));
   }
 
   var kCharCodeForN = 'n'.charCodeAt(0);
   var kCharCodeForP = 'p'.charCodeAt(0);
+  var kCharCodeForJ = 'j'.charCodeAt(0);
+  var kCharCodeForK = 'k'.charCodeAt(0);
 
   $('body').live('keypress', function() {
     // FIXME: There's got to be a better way to avoid seeing these keypress
     // events.
     if (event.target.nodeName == 'TEXTAREA')
       return;
-    if (event.charCode == kCharCodeForN)
-      focusNextComment();
-    else if (event.charCode == kCharCodeForP)
-      focusPreviousComment();
+
+    switch (event.charCode) {
+    case kCharCodeForN:
+      focusNext('previousComment', false);
+      break;
+
+    case kCharCodeForP:
+      focusNext('previousComment', true);
+      break;
+
+    case kCharCodeForJ:
+      focusNext('Line', false);
+      break;
+
+    case kCharCodeForK:
+      focusNext('Line', true);
+      break;
+    }
   });
 
   function contextLinesFor(line_id) {
   }
 
   function fileNameFor(line) {
-    return line.parentsUntil('.FileDiff').parent().find('h1').text();
+    return fileDiffFor(line).find('h1').text();
   }
 
   function indentFor(depth) {