In Herald transcripts, render some field values in a more readable way

Summary:
Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.

Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.

Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.

Maniphest Tasks: T13480

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

src/__phutil_library_map__.php

@@ -1528,6 +1528,7 @@ phutil_register_library_map(array(
     'HeraldApplicationActionGroup' => 'applications/herald/action/HeraldApplicationActionGroup.php',
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
     'HeraldBasicFieldGroup' => 'applications/herald/field/HeraldBasicFieldGroup.php',
+    'HeraldBoolFieldValue' => 'applications/herald/value/HeraldBoolFieldValue.php',
     'HeraldBuildableState' => 'applications/herald/state/HeraldBuildableState.php',
     'HeraldCallWebhookAction' => 'applications/herald/action/HeraldCallWebhookAction.php',
     'HeraldCommentAction' => 'applications/herald/action/HeraldCommentAction.php',
@@ -7633,6 +7634,7 @@ phutil_register_library_map(array(
     'HeraldApplicationActionGroup' => 'HeraldActionGroup',
     'HeraldApplyTranscript' => 'Phobject',
     'HeraldBasicFieldGroup' => 'HeraldFieldGroup',
+    'HeraldBoolFieldValue' => 'HeraldFieldValue',
     'HeraldBuildableState' => 'HeraldState',
     'HeraldCallWebhookAction' => 'HeraldAction',
     'HeraldCommentAction' => 'HeraldAction',

src/applications/herald/adapter/HeraldAdapter.php

@@ -1071,6 +1071,26 @@ abstract class HeraldAdapter extends Phobject {
       $condition->getValue());
   }
 
+  public function renderFieldTranscriptValue(
+    PhabricatorUser $viewer,
+    $field_type,
+    $field_value) {
+
+    $field = $this->getFieldImplementation($field_type);
+    if ($field) {
+      return $field->renderTranscriptValue(
+        $viewer,
+        $field_value);
+    }
+
+    return phutil_tag(
+      'em',
+      array(),
+      pht(
+        'Unable to render value for unknown field type ("%s").',
+        $field_type));
+  }
+
 
 /* -(  Applying Effects  )--------------------------------------------------- */
 

src/applications/herald/controller/HeraldTranscriptController.php

@@ -447,8 +447,9 @@ final class HeraldTranscriptController extends HeraldController {
   }
 
   private function buildObjectTranscriptPanel(HeraldTranscript $xscript) {
-
+    $viewer = $this->getViewer();
     $adapter = $this->getAdapter();
+
     $field_names = $adapter->getFieldNameMap();
 
     $object_xscript = $xscript->getObjectTranscript();
@@ -487,29 +488,21 @@ final class HeraldTranscriptController extends HeraldController {
     }
 
     if ($object_xscript) {
-      foreach ($object_xscript->getFields() as $field => $value) {
+      foreach ($object_xscript->getFields() as $field_type => $value) {
-        if (isset($field_names[$field])) {
+        if (isset($field_names[$field_type])) {
-          $field_name = pht('Field: %s', $field_names[$field]);
+          $field_name = pht('Field: %s', $field_names[$field_type]);
         } else {
-          $field_name = pht('Unknown Field ("%s")', $field_name);
+          $field_name = pht('Unknown Field ("%s")', $field_type);
         }
 
-        if (!is_scalar($value) && !is_null($value)) {
+        $field_value = $adapter->renderFieldTranscriptValue(
-          $value = implode("\n", $value);
+          $viewer,
-        }
+          $field_type,
-
+          $value);
-        if (strlen($value) > 256) {
-          $value = phutil_tag(
-            'textarea',
-            array(
-              'class' => 'herald-field-value-transcript',
-            ),
-            $value);
-        }
 
         $rows[] = array(
           $field_name,
-          $value,
+          $field_value,
         );
       }
     }

src/applications/herald/field/HeraldAlwaysField.php

@@ -23,7 +23,7 @@ final class HeraldAlwaysField extends HeraldField {
   }
 
   public function getHeraldFieldValueType($condition) {
-    return new HeraldEmptyFieldValue();
+    return new HeraldBoolFieldValue();
   }
 
   public function supportsObject($object) {

src/applications/herald/field/HeraldField.php

@@ -105,11 +105,16 @@ abstract class HeraldField extends Phobject {
   }
 
   public function getHeraldFieldValueType($condition) {
+
+    // NOTE: The condition type may be "null" to indicate that the caller
+    // wants a generic field value type. This is used when rendering field
+    // values in the object transcript.
+
     $standard_type = $this->getHeraldFieldStandardType();
     switch ($standard_type) {
       case self::STANDARD_BOOL:
       case self::STANDARD_PHID_BOOL:
-        return new HeraldEmptyFieldValue();
+        return new HeraldBoolFieldValue();
       case self::STANDARD_TEXT:
       case self::STANDARD_TEXT_LIST:
       case self::STANDARD_TEXT_MAP:
@@ -176,6 +181,14 @@ abstract class HeraldField extends Phobject {
     return $value_type->renderEditorValue($value);
   }
 
+  public function renderTranscriptValue(
+    PhabricatorUser $viewer,
+    $field_value) {
+    $value_type = $this->getHeraldFieldValueType($condition_type = null);
+    $value_type->setViewer($viewer);
+    return $value_type->renderTranscriptValue($field_value);
+  }
+
   public function getPHIDsAffectedByCondition(HeraldCondition $condition) {
     try {
       $standard_type = $this->getHeraldFieldStandardType();

src/applications/herald/value/HeraldBoolFieldValue.php

@@ -0,0 +1,30 @@
+<?php
+
+final class HeraldBoolFieldValue
+  extends HeraldFieldValue {
+
+  public function getFieldValueKey() {
+    return 'bool';
+  }
+
+  public function getControlType() {
+    return self::CONTROL_NONE;
+  }
+
+  public function renderFieldValue($value) {
+    return null;
+  }
+
+  public function renderEditorValue($value) {
+    return null;
+  }
+
+  public function renderTranscriptValue($value) {
+    if ($value) {
+      return pht('true');
+    } else {
+      return pht('false');
+    }
+  }
+
+}

src/applications/herald/value/HeraldFieldValue.php

@@ -36,4 +36,8 @@ abstract class HeraldFieldValue extends Phobject {
     return array();
   }
 
+  public function renderTranscriptValue($value) {
+    return $this->renderFieldValue($value);
+  }
+
 }

src/applications/herald/value/HeraldTextFieldValue.php

@@ -19,4 +19,25 @@ final class HeraldTextFieldValue
     return $value;
   }
 
+  public function renderTranscriptValue($value) {
+    if (is_array($value)) {
+      $value = implode('', $value);
+    }
+
+    if (!strlen($value)) {
+      return phutil_tag('em', array(), pht('None'));
+    }
+
+    if (strlen($value) > 256) {
+      $value = phutil_tag(
+        'textarea',
+        array(
+          'class' => 'herald-field-value-transcript',
+        ),
+        $value);
+    }
+
+    return $value;
+  }
+
 }

src/applications/herald/value/HeraldTokenizerFieldValue.php

@@ -64,17 +64,7 @@ final class HeraldTokenizerFieldValue
   }
 
   public function renderFieldValue($value) {
-    $viewer = $this->getViewer();
+    return $this->renderValueAsList($value, $for_transcript = false);
-    $value = (array)$value;
-
-    if ($this->valueMap !== null) {
-      foreach ($value as $k => $v) {
-        $value[$k] = idx($this->valueMap, $v, $v);
-      }
-      return implode(', ', $value);
-    }
-
-    return $viewer->renderHandleList((array)$value)->setAsInline(true);
   }
 
   public function renderEditorValue($value) {
@@ -87,4 +77,33 @@ final class HeraldTokenizerFieldValue
     return $datasource->getWireTokens($value);
   }
 
+  public function renderTranscriptValue($value) {
+    return $this->renderValueAsList($value, $for_transcript = true);
+  }
+
+  private function renderValueAsList($value, $for_transcript) {
+    $viewer = $this->getViewer();
+    $value = (array)$value;
+
+    if (!$value) {
+      return phutil_tag('em', array(), pht('None'));
+    }
+
+    if ($this->valueMap !== null) {
+      foreach ($value as $k => $v) {
+        $value[$k] = idx($this->valueMap, $v, $v);
+      }
+
+      return implode(', ', $value);
+    }
+
+    $list = $viewer->renderHandleList($value);
+
+    if (!$for_transcript) {
+      $list->setAsInline(true);
+    }
+
+    return $list;
+  }
+
 }