2011-01-06 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Jan 2011 01:15:54 +0000 (01:15 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 8 Jan 2011 01:15:54 +0000 (01:15 +0000)
        Reviewed by Adam Barth.

        side-by-side diffs in the code review tool
        https://bugs.webkit.org/show_bug.cgi?id=52019

        Support for conversion from the formatted diff to a side-by-side diff.
        Maintains comments and new comments can be added.

        The main architectural change is that Line elements are no longer necessarily
        siblings. Each physical line is now in a LineContainer and LineContainers are
        siblings. Each Line corresponds to a Line in the unified diff and has an id (e.g. line12).
        A Line can be a LineContainer or a child of a LineContainer.

        In this way, converting to side-by-side and, in the future, back to unified is non-lossy.

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

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@75295 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 1d68dfc8c0a6d10437a85f249dccf175a579af01..8fc98958c6f822c31216370297235f8eb61ac004 100644 (file)
@@ -1,3 +1,23 @@
+2011-01-06  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        side-by-side diffs in the code review tool
+        https://bugs.webkit.org/show_bug.cgi?id=52019
+
+        Support for conversion from the formatted diff to a side-by-side diff.
+        Maintains comments and new comments can be added.
+
+        The main architectural change is that Line elements are no longer necessarily
+        siblings. Each physical line is now in a LineContainer and LineContainers are
+        siblings. Each Line corresponds to a Line in the unified diff and has an id (e.g. line12).
+        A Line can be a LineContainer or a child of a LineContainer.
+
+        In this way, converting to side-by-side and, in the future, back to unified is non-lossy.
+
+        * PrettyPatch/PrettyPatch.rb:
+        * code-review.js:
+
 2011-01-06  Ojan Vafai  <ojan@chromium.org>
 
         Fix line context when replying to comments.
index cfdbc64b00dbff1280fadf9d333c1ec01c931e9a..512bc49a6c8040e5a1d800772224e23be4c8d9a5 100644 (file)
@@ -133,12 +133,21 @@ h1 :hover {
     background-color: #eee;
 }
 
+.DiffLinks {
+    float: right;
+}
+
 .DiffSection {
     background-color: white;
     border: solid #ddd;
     border-width: 1px 0px;
 }
 
+.LineSide {
+    display:inline-block;
+    width:50%;
+}
+
 .lineNumber, .expansionLineNumber {
     border-bottom: 1px solid #998;
     border-right: 1px solid #ddd;
@@ -362,7 +371,7 @@ body {
 }
 </style>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
-<script src="code-review.js?version=18"></script>
+<script src="code-review.js?version=19"></script>
 EOF
 
     def self.revisionOrDescription(string)
index 3c45701abec0c037ac7ca227599094a00e823e7f..09166c65f62d3a737ec8cd64e2cdf95d70fd0a01 100644 (file)
   var original_file_contents = {};
   var patched_file_contents = {};
   var WEBKIT_BASE_DIR = "http://svn.webkit.org/repository/webkit/trunk/";
+  // FIXME: Serialize this to the URL fragment for permalinking goodness, e.g.,
+  // so the bug page can link directly to the side-by-side diff.
+  var url_fragment = {};
+  var SIDE_BY_SIDE_DIFFS_KEY = 'sidebysidediffs';
 
   function idForLine(number) {
     return 'line' + number;
     this.id = nextLineID();
   }
 
+  function containerify() {
+    $(this).addClass('LineContainer');
+  }
+
   function hoverify() {
     $(this).hover(function() {
       $(this).addClass('hot');
     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));
+  }
+
   function previousCommentsFor(line) {
     // Scope to the diffSection as a performance improvement.
     return $('div[data-comment-for~="' + line[0].id + '"].previousComment', diffSectionFrom(line));
 
   function addPreviousComment(line, author, comment_text) {
     var line_id = line.attr('id');
-    var comment_block = $('<div data-comment-for="' + line_id + '" class="previousComment"></div>');    var author_block = $('<div class="author"></div>').text(author + ':');
+    var comment_block = $('<div data-comment-for="' + line_id + '" class="previousComment"></div>');
+    var author_block = $('<div class="author"></div>').text(author + ':');
     var text_block = $('<div class="content"></div>').text(comment_text);
     comment_block.append(author_block).append(text_block).each(hoverify).click(addCommentField);
     addDataCommentBaseLine(line, line_id);
   }
 
   function crawlDiff() {
-    $('.Line').each(idify).each(hoverify);
+    $('.Line').each(idify).each(hoverify).each(containerify);
     $('.FileDiff').each(function() {
       var file_name = $(this).children('h1').text();
       files[file_name] = this;
 
     // Don't show the links to expand upwards/downwards if the patch starts/ends without context
     // lines, i.e. starts/ends with add/remove lines.
-    var first_line = file_diff.querySelector('.Line');
+    var first_line = file_diff.querySelector('.LineContainer');
 
     // If there is no element with a "Line" class, then this is an image diff.
     if (!first_line)
 
     $('br').replaceWith(expandBarHtml(file_name));
 
-    var last_line = file_diff.querySelector('.Line:last-of-type');
+    var last_line = file_diff.querySelector('.LineContainer:last-of-type');
     // Some patches for new files somehow end up with an empty context line at the end
     // with a from line number of 0. Don't show expand links in that case either.
     if (!$(last_line).hasClass('add') && !$(last_line).hasClass('remove') && fromLineNumber(last_line) != 0)
         (amount ? amount + " " : "") + direction + "</a>";
   }
 
-  function handleExpandLinkClick(target) {
-    var expand_bar = $(target).parents('.ExpandBar');
+  function handleExpandLinkClick() {
+    var expand_bar = $(this).parents('.ExpandBar');
     var file_name = expand_bar.parents('.FileDiff').children('h1')[0].textContent;
-    var expand_function = partial(expand, expand_bar[0], file_name, target.getAttribute('data-direction'), Number(target.getAttribute('data-amount')));
+    var expand_function = partial(expand, expand_bar[0], file_name, this.getAttribute('data-direction'), Number(this.getAttribute('data-amount')));
     if (file_name in original_file_contents)
       expand_function();
     else
       getWebKitSourceFile(file_name, expand_function, expand_bar);
-  };
-
-  $(window).bind('click', function (e) {
-    var target = e.target;
+  }
 
-    switch(target.className) {
-    case 'ExpandLink':
-      handleExpandLinkClick(target);
-      break;
-    }
-  });
+  function handleSideBySideLinkClick() {
+    $('.FileDiff').each(sideBySideify);
+  }
 
   function getWebKitSourceFile(file_name, onLoad, expand_bar) {
     function handleLoad(contents) {
     var above_last_line = above_expansion.querySelector('.ExpansionLine:last-of-type');
     if (!above_last_line) {
       var diff_section = expand_bar.previousElementSibling;
-      above_last_line = diff_section.querySelector('.Line:last-of-type');
+      above_last_line = diff_section.querySelector('.LineContainer:last-of-type');
     }
 
     var above_last_line_num, above_last_from_line_num;
     if (!below_first_line) {
       var diff_section = expand_bar.nextElementSibling;
       if (diff_section)
-        below_first_line = diff_section.querySelector('.Line');
+        below_first_line = diff_section.querySelector('.LineContainer');
     }
 
     var below_first_line_num, below_first_from_line_num;
     insertLines(file_name, expansion_area, direction, start_line_num, end_line_num, start_from_line_num);
   }
 
+  function unifiedDiffExpansionLine(line_number, contents) {
+    var line = $('<div class="ExpansionLine">' +
+        '<span class="from expansionlineNumber">' + line_number +
+        '</span><span class="to expansionlineNumber">' + line_number +
+        '</span> <span class="text"></span>' +
+        '</div>');
+    // Use text instead of innerHTML to avoid evaluting HTML.
+    $('.text', line).text(contents);
+    return line;
+  }
+
+  function sideBySideDiffExpansionLine(line_number, contents) {
+    var line = $('<div class="ExpansionLine"></div>');
+    line.append(lineSide('from', contents, true, line_number));
+    line.append(lineSide('to', contents, true, line_number));
+    return line;
+  }
+
+  function lineSide(side, contents, is_expansion_line, opt_line_number, opt_attributes, opt_class) {
+    var class_name = '';
+    if (opt_attributes || opt_class) {
+      class_name = 'class="';
+      if (opt_attributes)
+        class_name += is_expansion_line ? 'ExpansionLine' : 'Line';
+      class_name += ' ' + (opt_class || '') + '"';
+    }
+
+    var attributes = opt_attributes || '';
+
+    var line_side = $('<div class="LineSide">' +
+        '<div ' + attributes + ' ' + class_name + '>' +
+          '<span class="' + side + ' ' + (is_expansion_line ? 'expansionLineNumber' : 'lineNumber') + '">' +
+              (opt_line_number || '&nbsp;') +
+          '</span>' +
+          '<span class="text"></span>' +
+        '</div>' +
+        '</div>');
+
+    // Use text instead of innerHTML to avoid evaluting HTML.
+    $('.text', line_side).text(contents);
+    return line_side;
+  }
+
   function insertLines(file_name, expansion_area, direction, start_line_num, end_line_num, start_from_line_num) {
     var fragment = document.createDocumentFragment();
+    var file_diff_index = $('.FileDiff').index(files[file_name]);
+    var is_side_by_side = isDiffSideBySide(file_diff_index);
+
     for (var i = 0; i < end_line_num - start_line_num; i++) {
-      var line = document.createElement('div');
-      line.className = 'ExpansionLine';
       // FIXME: from line numbers are wrong
-      line.innerHTML = '<span class="from expansionlineNumber">' + (start_from_line_num + i + 1) +
-          '</span><span class="to expansionlineNumber">' + (start_line_num + i + 1) +
-          '</span> <span class="text"></span>';
-      line.querySelector('.text').textContent = patched_file_contents[file_name][start_line_num + i];
-      fragment.appendChild(line);
+      var line_number = start_from_line_num + i + 1;
+      var contents = patched_file_contents[file_name][start_line_num + i];
+      var line = is_side_by_side ? sideBySideDiffExpansionLine(line_number, contents) : unifiedDiffExpansionLine(line_number, contents);
+      fragment.appendChild(line[0]);
     }
 
     if (direction == BELOW)
   }
 
   function textContentsFor(line) {
-    return $('.text', line).text();
+    // Just get the first match since a side-by-side diff has two lines with text inside them for
+    // unmodified lines in the diff.
+    return $('.text', line).first().text();
   }
 
   function lineNumberForFirstNonContextLine(patched_file, line, prev_line, context, hunk_num) {
   $(document).ready(function() {
     crawlDiff();
     fetchHistory();
-    $(document.body).prepend('<div id="message"><div class="help">Select line numbers to add a comment.</div><div class="commentStatus"></div></div>');
+    $(document.body).prepend('<div id="message">' +
+        '<div class="help">Select line numbers to add a comment.' +
+          '<div class="DiffLinks">' +
+            '<a href="javascript:" class="side-by-side-link">side-by-side</a>' +
+          '</div>' +
+        '</div>' +
+        '<div class="commentStatus"></div>' +
+        '</div>');
     $(document.body).append('<div id="toolbar">' +
         '<div class="overallComments">' +
             '<textarea placeholder="Overall comments"></textarea>' +
     updateToolbarAnchorState();
   });
 
+  function isDiffSideBySide(file_diff_index) {
+    if (!url_fragment[SIDE_BY_SIDE_DIFFS_KEY])
+      url_fragment[SIDE_BY_SIDE_DIFFS_KEY] = [];
+    return url_fragment[SIDE_BY_SIDE_DIFFS_KEY].indexOf(file_diff_index) != -1;
+  }
+
+  function sideBySideify(file_diff_index) {
+    if (isDiffSideBySide(file_diff_index))
+      return;
+
+    url_fragment[SIDE_BY_SIDE_DIFFS_KEY].push(file_diff_index);
+
+    $('.Line', this).each(sideBySideifyLine);
+    $('.ExpansionLine', this).each(sideBySideifyExpansionLine);
+  }
+
+  // FIXME: Put removed lines to the left of their corresponding added lines.
+  // FIXME: Provide a way to go back to the unified diff.
+  // FIXME: Allow for converting an individual file to side-by-side.
+  function sideBySideifyLine() {
+    var from = fromLineNumber(this);
+    var to = toLineNumber(this);
+    var contents = textContentsFor(this);
+
+    var classParts = this.className.split(' ');
+    var classBuffer = [];
+    for (var i = 0; i < classParts.length; i++) {
+      var part = classParts[i];
+      if (part != 'LineContainer' && part != 'Line')
+        classBuffer.push(part);
+    }
+    var classNames = classBuffer.join(' ');
+
+    var id = this.id;
+    var attributesBuffer = ['id=' + id];
+    // Make sure to keep all data- attributes.
+    $(this.attributes).each(function() {
+      if (this.name.indexOf('data-') == 0)
+        attributesBuffer.push(this.name + '=' + this.value);
+    });
+    var attributes = attributesBuffer.join(' ');
+
+    var from_class = '';
+    var to_class = '';
+    var from_attributes = '';
+    var to_attributes = '';
+    var from_contents = contents;
+    var to_contents = contents;
+
+    if (from && !to) { // This is a remove line.
+      from_class = classNames;
+      from_attributes = attributes;
+      to_contents = '';
+    } else if (to && !from) { // This is an add line.
+      to_class = classNames;
+      to_attributes = attributes;
+      from_contents = '';
+    }
+
+    var container_class = 'LineContainer';
+    var container_attributes = '';
+    if (!to_attributes && !from_attributes) {
+      container_attributes = attributes;
+      container_class += ' Line ' + classNames;
+    }
+
+    var new_line = $('<div ' + container_attributes + ' class="' + container_class + '"></div>');
+    new_line.append(lineSide('from', from_contents, false, from, from_attributes, from_class));
+    new_line.append(lineSide('to', to_contents, false, to, to_attributes, to_class));
+
+    $(this).replaceWith(new_line);
+    tranferCommentsFor($(document.getElementById(id)));
+  }
+
+  function sideBySideifyExpansionLine() {
+    var contents = textContentsFor(this);
+    var line_number = fromLineNumber(this);
+    var new_line = sideBySideDiffExpansionLine(line_number, contents);
+    $(this).replaceWith(new_line);
+  }
+
+  function tranferCommentsFor(line) {
+    var fragment = document.createDocumentFragment();
+
+    previousCommentsFor(line).each(function() {
+      fragment.appendChild(this);
+    });
+
+    var active_comments = activeCommentFor(line);
+    var num_active_comments = active_comments.size();
+    if (num_active_comments > 0) {
+      if (num_active_comments > 1)
+        console.log('ERROR: There is more than one active comment for ' + line.attr('id') + '.');
+
+      var parent = active_comments[0].parentNode;
+      var frozenComment = parent.nextSibling;
+      fragment.appendChild(parent);
+      fragment.appendChild(frozenComment);
+    }
+
+    line.after(fragment);
+  }
+
   function discardComment() {
     var line_id = $(this).parentsUntil('.comment').parent().find('textarea').attr('data-comment-for');
     var line = $('#' + line_id)
     $(this).remove();
   }
 
+  $('.side-by-side-link').live('click', handleSideBySideLinkClick);
+  $('.ExpandLink').live('click', handleExpandLinkClick);
   $('.comment .discard').live('click', discardComment);
+  $('.frozenComment').live('click', unfreezeComment);
 
   $('.comment .ok').live('click', function() {
     var comment_textarea = $(this).parentsUntil('.comment').parent().find('textarea');
     findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
   });
 
-  $('.frozenComment').live('click', unfreezeComment);
-
   function focusOn(comment) {
     $('.focused').removeClass('focused');
     if (comment.length == 0)
     event.preventDefault();
   });
 
-  $('.Line').live('mouseenter', function() {
+  $('.LineContainer').live('mouseenter', function() {
     if (!in_drag_select)
       return;