Revert changes to Diffusion blame view

Summary:
Ref PHI174. This reverts most of these changes:

- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452

These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.

I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.

In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.

I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".

I'm going to follow this up with some additional changes:

  - Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
  - Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
  - Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
  - Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.

Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.

{F5251019}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18746

resources/celerity/map.php

@@ -73,7 +73,7 @@ return array(
     'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e',
     'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6',
     'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec',
-    'rsrc/css/application/diffusion/diffusion-source.css' => '69ac9399',
+    'rsrc/css/application/diffusion/diffusion-source.css' => '3a1056d8',
     'rsrc/css/application/diffusion/diffusion.css' => '45727264',
     'rsrc/css/application/feed/feed.css' => 'ecd4ec57',
     'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948',
@@ -572,7 +572,7 @@ return array(
     'diffusion-icons-css' => '0c15255e',
     'diffusion-readme-css' => '419dd5b6',
     'diffusion-repository-css' => 'ee6f20ec',
-    'diffusion-source-css' => '69ac9399',
+    'diffusion-source-css' => '3a1056d8',
     'diviner-shared-css' => '896f1d43',
     'font-fontawesome' => 'e838e088',
     'font-lato' => 'c7ccd872',

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -1127,6 +1127,10 @@ final class DiffusionBrowseController extends DiffusionController {
         ));
     }
 
+    $skip_text = pht('Skip Past This Commit');
+    $skip_icon = id(new PHUIIconView())
+      ->setIcon('fa-caret-square-o-left');
+
     foreach ($display as $line_index => $line) {
       $row = array();
 
@@ -1142,14 +1146,12 @@ final class DiffusionBrowseController extends DiffusionController {
       $revision_link = null;
       $commit_link = null;
       $before_link = null;
-      $commit_date = null;
 
-      $style = 'border-right: 3px solid '.$line['color'].';';
+      $style = 'background: '.$line['color'].';';
 
       if ($identifier && !$line['duplicate']) {
         if (isset($commit_links[$identifier])) {
-          $commit_link = $commit_links[$identifier]['link'];
+          $commit_link = $commit_links[$identifier];
-          $commit_date = $commit_links[$identifier]['date'];
         }
 
         if (isset($revision_map[$identifier])) {
@@ -1160,10 +1162,6 @@ final class DiffusionBrowseController extends DiffusionController {
         }
 
         $skip_href = $line_href.'?before='.$identifier.'&view=blame';
-        $skip_text = pht('Skip Past This Commit');
-        $icon = id(new PHUIIconView())
-          ->setIcon('fa-caret-square-o-left');
-
         $before_link = javelin_tag(
           'a',
           array(
@@ -1175,7 +1173,7 @@ final class DiffusionBrowseController extends DiffusionController {
               'size'    => 300,
             ),
           ),
-          $icon);
+          $skip_icon);
       }
 
       if ($show_blame) {
@@ -1186,34 +1184,26 @@ final class DiffusionBrowseController extends DiffusionController {
           ),
           $before_link);
 
-        $row[] = phutil_tag(
+        $object_links = array();
-          'th',
+        $object_links[] = $commit_link;
-          array(
+        if ($revision_link) {
-            'class' => 'diffusion-rev-link',
+          $object_links[] = phutil_tag('span', array(), '/');
-          ),
+          $object_links[] = $revision_link;
-          $commit_link);
-
-        if ($revision_map) {
-          $row[] = phutil_tag(
-            'th',
-            array(
-              'class' => 'diffusion-blame-revision',
-            ),
-            $revision_link);
         }
 
         $row[] = phutil_tag(
           'th',
           array(
-            'class' => 'diffusion-blame-date',
+            'class' => 'diffusion-rev-link',
           ),
-          $commit_date);
+          $object_links);
       }
 
       $line_link = phutil_tag(
         'a',
         array(
           'href' => $line_href,
+          'style' => $style,
         ),
         $line_number);
 
@@ -1536,6 +1526,33 @@ final class DiffusionBrowseController extends DiffusionController {
     return head($parents);
   }
 
+  private function renderRevisionTooltip(
+    DifferentialRevision $revision,
+    $handles) {
+    $viewer = $this->getRequest()->getUser();
+
+    $date = phabricator_date($revision->getDateModified(), $viewer);
+    $id = $revision->getID();
+    $title = $revision->getTitle();
+    $header = "D{$id} {$title}";
+
+    $author = $handles[$revision->getAuthorPHID()]->getName();
+
+    return "{$header}\n{$date} \xC2\xB7 {$author}";
+  }
+
+  private function renderCommitTooltip(
+    PhabricatorRepositoryCommit $commit,
+    $author) {
+
+    $viewer = $this->getRequest()->getUser();
+
+    $date = phabricator_date($commit->getEpoch(), $viewer);
+    $summary = trim($commit->getSummary());
+
+    return "{$summary}\n{$date} \xC2\xB7 {$author}";
+  }
+
   protected function markupText($text) {
     $engine = PhabricatorMarkupEngine::newDiffusionMarkupEngine();
     $engine->setConfig('viewer', $this->getRequest()->getUser());
@@ -1743,6 +1760,9 @@ final class DiffusionBrowseController extends DiffusionController {
         ->setViewer($viewer)
         ->withRepository($repository)
         ->withIdentifiers($identifiers)
+        // TODO: We only fetch this to improve author display behavior, but
+        // shouldn't really need to?
+        ->needCommitData(true)
         ->execute();
       $commits = mpull($commits, null, 'getCommitIdentifier');
     } else {
@@ -1754,27 +1774,25 @@ final class DiffusionBrowseController extends DiffusionController {
 
   private function renderCommitLinks(array $commits, $handles) {
     $links = array();
-    $viewer = $this->getViewer();
     foreach ($commits as $identifier => $commit) {
-      $date = phabricator_date($commit->getEpoch(), $viewer);
+      $tooltip = $this->renderCommitTooltip(
-      $summary = trim($commit->getSummary());
+        $commit,
+        $commit->renderAuthorShortName($handles));
 
-      $commit_link = phutil_tag(
+      $commit_link = javelin_tag(
         'a',
         array(
           'href' => $commit->getURI(),
+          'sigil' => 'has-tooltip',
+          'meta'  => array(
+            'tip'   => $tooltip,
+            'align' => 'E',
+            'size'  => 600,
           ),
-        $summary);
-
-      $commit_date = phutil_tag(
-        'a',
-        array(
-          'href' => $commit->getURI(),
         ),
-        $date);
+        $commit->getLocalName());
 
-      $links[$identifier]['link'] = $commit_link;
+      $links[$identifier] = $commit_link;
-      $links[$identifier]['date'] = $commit_date;
     }
 
     return $links;
@@ -1785,10 +1803,19 @@ final class DiffusionBrowseController extends DiffusionController {
 
     foreach ($revisions as $revision) {
       $revision_id = $revision->getID();
-      $revision_link = phutil_tag(
+
+      $tooltip = $this->renderRevisionTooltip($revision, $handles);
+
+      $revision_link = javelin_tag(
         'a',
         array(
           'href' => '/'.$revision->getMonogram(),
+          'sigil' => 'has-tooltip',
+          'meta'  => array(
+            'tip'   => $tooltip,
+            'align' => 'E',
+            'size'  => 600,
+          ),
         ),
         $revision->getMonogram());
 

webroot/rsrc/css/application/diffusion/diffusion-source.css

@@ -13,22 +13,24 @@
   -webkit-overflow-scrolling: touch;
 }
 
-.diffusion-source tr.phabricator-source-highlight th,
+.diffusion-source tr.phabricator-source-highlight {
-.diffusion-source tr.phabricator-source-highlight td {
+  background: {$sh-yellowbackground};
-  background: {$gentle.highlight};
 }
 
 .diffusion-source th {
   text-align: right;
   vertical-align: top;
-  color: {$darkbluetext};
+  background: {$lightgreybackground};
+  color: {$bluetext};
   border-right: 1px solid {$thinblueborder};
 }
 
 .diffusion-source td {
   vertical-align: top;
   white-space: pre-wrap;
-  padding: 3px 12px;
+  padding-top: 1px;
+  padding-bottom: 1px;
+  padding-left: 8px;
   width: 100%;
   word-break: break-all;
 }
@@ -43,18 +45,12 @@
 }
 
 .diffusion-blame-link,
-.diffusion-rev-link,
+.diffusion-rev-link {
-.diffusion-blame-date {
   white-space: nowrap;
 }
 
-.diffusion-blame-date,
+.diffusion-blame-link {
-.diffusion-blame-link,
+  min-width: 28px;
-.diffusion-blame-revision,
-.diffusion-rev-link {
-  background: {$lightgreybackground};
-  font: {$basefont};
-  font-size: {$smallerfontsize};
 }
 
 .diffusion-source th.diffusion-rev-link {
@@ -62,23 +58,16 @@
   min-width: 130px;
 }
 
-.diffusion-source a {
+.diffusion-blame-link a,
+.diffusion-rev-link a,
+.diffusion-line-link a {
   color: {$darkbluetext};
 }
 
-.diffusion-rev-link a {
+.diffusion-rev-link a,
-  max-width: 300px;
+.diffusion-rev-link span {
-  overflow: hidden;
+  margin: 2px 8px 0;
-  white-space: nowrap;
+  display: inline-block;
-  text-overflow: ellipsis;
-  margin: 3px 8px;
-  display: block;
-}
-
-.diffusion-blame-date a,
-.diffusion-blame-revision a {
-  float: right;
-  margin: 3px 8px;
 }
 
 .diffusion-rev-link span {
@@ -91,19 +80,7 @@
 .diffusion-line-link a {
   /* Give the user a larger click target. */
   display: block;
-  padding: 4px 8px 3px;
+  padding: 2px 8px;
-}
-
-.diffusion-line-link a {
-  color: {$lightgreytext};
-}
-
-.diffusion-blame-link a .phui-icon-view {
-  color: {$bluetext};
-}
-
-.diffusion-blame-link a:hover .phui-icon-view {
-  color: {$sky};
 }
 
 .diffusion-line-link {