2011-02-14 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Feb 2011 05:46:44 +0000 (05:46 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Feb 2011 05:46:44 +0000 (05:46 +0000)
        Reviewed by Adam Barth.

        improve line selection in the code review tool
        https://bugs.webkit.org/show_bug.cgi?id=54430

        -shift+click now extends the comment context
        -selecting outside of a diff section no longer clears the selected lines.
         Instead it just restricts the selected lines to that diff section.

        * code-review.js:

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

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

index 9adc3d7..04c5875 100644 (file)
@@ -1,3 +1,16 @@
+2011-02-14  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        improve line selection in the code review tool
+        https://bugs.webkit.org/show_bug.cgi?id=54430
+
+        -shift+click now extends the comment context
+        -selecting outside of a diff section no longer clears the selected lines.
+         Instead it just restricts the selected lines to that diff section.
+
+        * code-review.js:
+
 2011-02-10  Ojan Vafai  <ojan@chromium.org>
 
         Reviewed by Adam Barth.
index 4434828..df5a2ee 100644 (file)
@@ -102,6 +102,10 @@ var CODE_REVIEW_UNITTEST;
     return $(line).parents('.FileDiff');
   }
 
+  function diffSectionFor(line) {
+    return $(line).parents('.DiffSection');
+  }
+
   function activeCommentFor(line) {
     // Scope to the diffSection as a performance improvement.
     return $('textarea[data-comment-for~="' + line[0].id + '"]', fileDiffFor(line));
@@ -1408,11 +1412,6 @@ var CODE_REVIEW_UNITTEST;
 
   var drag_select_start_index = -1;
 
-  function stopDragSelect() {
-    $('.selected').removeClass('selected');
-    drag_select_start_index = -1;
-  }
-
   function lineOffsetFrom(line, offset) {
     var file_diff = line.parents('.FileDiff');
     var all_lines = $('.Line', file_diff);
@@ -1428,17 +1427,68 @@ var CODE_REVIEW_UNITTEST;
     return lineOffsetFrom(line, 1);
   }
 
-  $('.lineNumber').live('click', function() {
+  $(document.body).bind('mouseup', processSelectedLines);
+
+  $('.lineNumber').live('click', function(e) {
     var line = lineFromLineDescendant($(this));
     if (line.hasClass('commentContext'))
       trimCommentContextToBefore(previousLineFor(line), line.attr('data-comment-base-line'));
-  }).live('mousedown', function() {
+    else if (e.shiftKey)
+      extendCommentContextTo(line);
+  }).live('mousedown', function(e) {
+    // preventDefault to avoid selecting text when dragging to select comment context lines.
+    // FIXME: should we use user-modify CSS instead?
+    event.preventDefault();
+    if (e.shiftKey)
+      return;
+
     var line = lineFromLineDescendant($(this));
     drag_select_start_index = numberFrom(line.attr('id'));
     line.addClass('selected');
-    event.preventDefault();
   });
 
+  $('.LineContainer').live('mouseenter', function(e) {
+    if (drag_select_start_index == -1 || e.shiftKey)
+      return;
+    selectToLineContainer(this);
+  }).live('mouseup', function(e) {
+    if (drag_select_start_index == -1 || e.shiftKey)
+      return;
+
+    selectToLineContainer(this);
+    processSelectedLines();
+  });
+
+  function extendCommentContextTo(line) {
+    var diff_section = diffSectionFor(line);
+    var lines = $('.Line', diff_section);
+    var lines_to_modify = [];
+    var have_seen_start_line = false;
+    var data_comment_base_line = null;
+    lines.each(function() {
+      if (data_comment_base_line)
+        return;
+
+      have_seen_start_line = have_seen_start_line || this == line[0];
+      
+      if (have_seen_start_line) {
+        if ($(this).hasClass('commentContext'))
+          data_comment_base_line = $(this).attr('data-comment-base-line');
+        else
+          lines_to_modify.push(this);
+      }
+    });
+    
+    // There is no comment context to extend.
+    if (!data_comment_base_line)
+      return;
+    
+    $(lines_to_modify).each(function() {
+      $(this).addClass('commentContext');
+      $(this).attr('data-comment-base-line', data_comment_base_line);
+    });
+  }
+
   function selectTo(focus_index) {
     var selected = $('.selected').removeClass('selected');
     var is_backward = drag_select_start_index > focus_index;
@@ -1452,20 +1502,29 @@ var CODE_REVIEW_UNITTEST;
 
   function selectToLineContainer(line_container) {
     var line = lineFromLineContainer(line_container);
+
+    // Ensure that the selected lines are all contained in the same DiffSection.
+    var selected_lines = $('.selected');
+    var selected_diff_section = diffSectionFor(selected_lines.first());
+    var new_diff_section = diffSectionFor(line);
+    if (new_diff_section[0] != selected_diff_section[0]) {
+      var lines = $('.Line', selected_diff_section);
+      if (numberFrom(selected_lines.first().attr('id')) == drag_select_start_index)
+        line = lines.last();
+      else
+        line = lines.first();
+    }
+    
     selectTo(numberFrom(line.attr('id')));
   }
 
-  $('.LineContainer').live('mouseenter', function() {
-    if (drag_select_start_index == -1)
-      return;
-    selectToLineContainer(this);
-  }).live('mouseup', function() {
-    if (drag_select_start_index == -1)
-      return;
-
-    selectToLineContainer(this);
+  function processSelectedLines() {
+    drag_select_start_index = -1;
 
     var selected = $('.selected');
+    if (!selected.size())
+      return;
+    
     var already_has_comment = selected.last().hasClass('commentContext');
     selected.addClass('commentContext');
 
@@ -1480,10 +1539,11 @@ var CODE_REVIEW_UNITTEST;
 
     selected.each(function() {
       addDataCommentBaseLine(this, comment_base_line);
+      $(this).removeClass('selected');
     });
 
     saveDraftComments();
-  });
+  }
 
   function addDataCommentBaseLine(line, id) {
     var val = $(line).attr('data-comment-base-line');
@@ -1525,8 +1585,6 @@ var CODE_REVIEW_UNITTEST;
     return line;
   }
 
-  $('.DiffSection').live('mouseleave', stopDragSelect).live('mouseup', stopDragSelect);
-
   function contextSnippetFor(line, indent) {
     var snippets = []
     contextLinesFor(line.attr('id'), fileDiffFor(line)).each(function() {