Reduce derps in timeline event group rendering

Summary:
Ref T4266. This possibly moves us towards getting reasonable timeline grouping:

  - Always sort icon stories to the top.
  - Render one timestamp for the whole group, using the earliest tranaction date.
  - Move any "Edit", "Edited", or "Preview" links to the top.
  - Rendering just one timestamp implicitly fixes the JS issues.
  - For stories without an icon, indent them if any member of the group has an icon.

Test Plan: See screenshots.

Reviewers: chad, wotte

Reviewed By: chad

CC: aran

Maniphest Tasks: T4266

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

src/view/layout/PhabricatorTimelineEventView.php

@@ -109,23 +109,39 @@ final class PhabricatorTimelineEventView extends AphrontView {
     return $this;
   }
 
-
+  protected function renderEventTitle($is_first_event, $force_icon) {
-  public function renderEventTitle() {
     $title = $this->title;
     if (($title === null) && !$this->hasChildren()) {
       $title = '';
     }
 
-    $extra = $this->renderExtra();
+    if ($is_first_event) {
+      $extra = array();
+      $is_first_extra = true;
+      foreach ($this->getEventGroup() as $event) {
+        $extra[] = $this->renderExtra($is_first_extra);
+        $is_first_extra = false;
+      }
+      $extra = phutil_tag(
+        'span',
+        array(
+          'class' => 'phabricator-timeline-extra',
+        ),
+        phutil_implode_html(" \xC2\xB7 ", array_mergev($extra)));
+    } else {
+      $extra = null;
+    }
 
-    if ($title !== null || $extra !== null) {
+    if ($title !== null || $extra) {
       $title_classes = array();
       $title_classes[] = 'phabricator-timeline-title';
 
       $icon = null;
-      if ($this->icon) {
+      if ($this->icon || $force_icon) {
         $title_classes[] = 'phabricator-timeline-title-with-icon';
+      }
 
+      if ($this->icon) {
         $fill_classes = array();
         $fill_classes[] = 'phabricator-timeline-icon-fill';
         if ($this->color) {
@@ -159,10 +175,24 @@ final class PhabricatorTimelineEventView extends AphrontView {
 
   public function render() {
 
+    $events = $this->getEventGroup();
+
+    // Move events with icons first.
+    $icon_keys = array();
+    foreach ($this->getEventGroup() as $key => $event) {
+      if ($event->icon) {
+        $icon_keys[] = $key;
+      }
+    }
+    $events = array_select_keys($events, $icon_keys) + $events;
+    $force_icon = (bool)$icon_keys;
+
     $group_titles = array();
     $group_children = array();
-    foreach ($this->getEventGroup() as $event) {
+    $is_first_event = true;
-      $group_titles[] = $event->renderEventTitle();
+    foreach ($events as $event) {
+      $group_titles[] = $event->renderEventTitle($is_first_event, $force_icon);
+      $is_first_event = false;
       if ($event->hasChildren()) {
         $group_children[] = $event->renderChildren();
       }
@@ -264,7 +294,7 @@ final class PhabricatorTimelineEventView extends AphrontView {
         $content));
   }
 
-  private function renderExtra() {
+  private function renderExtra($is_first_extra) {
     $extra = array();
 
     if ($this->getIsPreview()) {
@@ -272,7 +302,6 @@ final class PhabricatorTimelineEventView extends AphrontView {
     } else {
       $xaction_phid = $this->getTransactionPHID();
 
-
       if ($this->getIsEdited()) {
         $extra[] = javelin_tag(
           'a',
@@ -293,46 +322,50 @@ final class PhabricatorTimelineEventView extends AphrontView {
           pht('Edit'));
       }
 
-      $source = $this->getContentSource();
+      if ($is_first_extra) {
-      if ($source) {
+        $source = $this->getContentSource();
-        $extra[] = id(new PhabricatorContentSourceView())
+        if ($source) {
-          ->setContentSource($source)
+          $extra[] = id(new PhabricatorContentSourceView())
-          ->setUser($this->getUser())
+            ->setContentSource($source)
-          ->render();
+            ->setUser($this->getUser())
-      }
-
-      if ($this->getDateCreated()) {
-        $date = phabricator_datetime(
-          $this->getDateCreated(),
-          $this->getUser());
-        if ($this->anchor) {
-          Javelin::initBehavior('phabricator-watch-anchor');
-
-          $anchor = id(new PhabricatorAnchorView())
-            ->setAnchorName($this->anchor)
             ->render();
-
-          $date = array(
-            $anchor,
-            phutil_tag(
-              'a',
-              array(
-                'href' => '#'.$this->anchor,
-              ),
-              $date),
-          );
         }
-        $extra[] = $date;
-      }
-    }
 
-    if ($extra) {
+        $date_created = null;
-      $extra = phutil_tag(
+        foreach ($this->getEventGroup() as $event) {
-        'span',
+          if ($event->getDateCreated()) {
-        array(
+            if ($date_created === null) {
-          'class' => 'phabricator-timeline-extra',
+              $date_created = $event->getDateCreated();
-        ),
+            } else {
-        phutil_implode_html(" \xC2\xB7 ", $extra));
+              $date_created = min($event->getDateCreated(), $date_created);
+            }
+          }
+        }
+
+        if ($date_created) {
+          $date = phabricator_datetime(
+            $this->getDateCreated(),
+            $this->getUser());
+          if ($this->anchor) {
+            Javelin::initBehavior('phabricator-watch-anchor');
+
+            $anchor = id(new PhabricatorAnchorView())
+              ->setAnchorName($this->anchor)
+              ->render();
+
+            $date = array(
+              $anchor,
+              phutil_tag(
+                'a',
+                array(
+                  'href' => '#'.$this->anchor,
+                ),
+                $date),
+            );
+          }
+          $extra[] = $date;
+        }
+      }
     }
 
     return $extra;