Make various UX improvements to charts so they're closer to making visual sense

Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical.

Test Plan: Looked at charts, saw fewer obvious horrors.

Subscribers: yelirekim

Maniphest Tasks: T13279

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

resources/celerity/map.php

@@ -390,7 +390,7 @@ return array(
     'rsrc/js/application/diffusion/behavior-pull-lastmodified.js' => 'c715c123',
     'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => '6a85bc5a',
     'rsrc/js/application/drydock/drydock-live-operation-status.js' => '47a0728b',
-    'rsrc/js/application/fact/Chart.js' => 'ddb9dd1f',
+    'rsrc/js/application/fact/Chart.js' => '52e3ff03',
     'rsrc/js/application/fact/ChartCurtainView.js' => '86954222',
     'rsrc/js/application/fact/ChartFunctionLabel.js' => '81de1dab',
     'rsrc/js/application/files/behavior-document-engine.js' => '243d6c22',
@@ -699,7 +699,7 @@ return array(
     'javelin-behavior-user-menu' => '60cd9241',
     'javelin-behavior-view-placeholder' => 'a9942052',
     'javelin-behavior-workflow' => '9623adc1',
-    'javelin-chart' => 'ddb9dd1f',
+    'javelin-chart' => '52e3ff03',
     'javelin-chart-curtain-view' => '86954222',
     'javelin-chart-function-label' => '81de1dab',
     'javelin-color' => '78f811c9',
@@ -1369,6 +1369,12 @@ return array(
       'javelin-dom',
       'javelin-fx',
     ),
+    '52e3ff03' => array(
+      'phui-chart-css',
+      'd3',
+      'javelin-chart-curtain-view',
+      'javelin-chart-function-label',
+    ),
     '541f81c3' => array(
       'javelin-install',
     ),
@@ -2066,12 +2072,6 @@ return array(
       'javelin-uri',
       'phabricator-notification',
     ),
-    'ddb9dd1f' => array(
-      'phui-chart-css',
-      'd3',
-      'javelin-chart-curtain-view',
-      'javelin-chart-function-label',
-    ),
     'dfa1d313' => array(
       'javelin-behavior',
       'javelin-dom',

src/__phutil_library_map__.php

@@ -3104,6 +3104,7 @@ phutil_register_library_map(array(
     'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php',
     'PhabricatorDefaultSyntaxStyle' => 'infrastructure/syntax/PhabricatorDefaultSyntaxStyle.php',
     'PhabricatorDefaultUnlockEngine' => 'applications/system/engine/PhabricatorDefaultUnlockEngine.php',
+    'PhabricatorDemoChartEngine' => 'applications/fact/engine/PhabricatorDemoChartEngine.php',
     'PhabricatorDestructibleCodex' => 'applications/system/codex/PhabricatorDestructibleCodex.php',
     'PhabricatorDestructibleCodexInterface' => 'applications/system/interface/PhabricatorDestructibleCodexInterface.php',
     'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php',
@@ -9434,6 +9435,7 @@ phutil_register_library_map(array(
     'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler',
     'PhabricatorDefaultSyntaxStyle' => 'PhabricatorSyntaxStyle',
     'PhabricatorDefaultUnlockEngine' => 'PhabricatorUnlockEngine',
+    'PhabricatorDemoChartEngine' => 'PhabricatorChartEngine',
     'PhabricatorDestructibleCodex' => 'Phobject',
     'PhabricatorDestructionEngine' => 'Phobject',
     'PhabricatorDestructionEngineExtension' => 'Phobject',

src/applications/fact/controller/PhabricatorFactChartController.php

@@ -62,50 +62,9 @@ final class PhabricatorFactChartController extends PhabricatorFactController {
   private function newDemoChart() {
     $viewer = $this->getViewer();
 
-    $argvs = array();
-
-    $argvs[] = array('fact', 'tasks.count.create');
-
-    $argvs[] = array('constant', 360);
-
-    $argvs[] = array('fact', 'tasks.open-count.create');
-
-    $argvs[] = array(
-      'sum',
-      array(
-        'accumulate',
-        array('fact', 'tasks.count.create'),
-      ),
-      array(
-        'accumulate',
-        array('fact', 'tasks.open-count.create'),
-      ),
-    );
-
-    $argvs[] = array(
-      'compose',
-      array('scale', 0.001),
-      array('cos'),
-      array('scale', 100),
-      array('shift', 800),
-    );
-
-    $datasets = array();
-    foreach ($argvs as $argv) {
-      $datasets[] = PhabricatorChartDataset::newFromDictionary(
-        array(
-          'function' => $argv,
-        ));
-    }
-
-    $chart = id(new PhabricatorFactChart())
-      ->setDatasets($datasets);
-
-    $engine = id(new PhabricatorChartRenderingEngine())
+    $chart = id(new PhabricatorDemoChartEngine())
       ->setViewer($viewer)
-      ->setChart($chart);
-
-    $chart = $engine->getStoredChart();
+      ->newStoredChart();
 
     return id(new AphrontRedirectResponse())->setURI($chart->getURI());
   }

src/applications/fact/engine/PhabricatorChartEngine.php

@@ -63,7 +63,7 @@ abstract class PhabricatorChartEngine
 
   abstract protected function newChart(PhabricatorFactChart $chart, array $map);
 
-  final public function buildChartPanel() {
+  final public function newStoredChart() {
     $viewer = $this->getViewer();
 
     $parameters = $this->getEngineParameters();
@@ -76,7 +76,11 @@ abstract class PhabricatorChartEngine
       ->setViewer($viewer)
       ->setChart($chart);
 
-    $chart = $rendering_engine->getStoredChart();
+    return $rendering_engine->getStoredChart();
+  }
+
+  final public function buildChartPanel() {
+    $chart = $this->newStoredChart();
 
     $panel_type = id(new PhabricatorDashboardChartPanelType())
       ->getPanelTypeKey();
@@ -91,7 +95,7 @@ abstract class PhabricatorChartEngine
   final protected function newFunction($name /* , ... */) {
     $argv = func_get_args();
     return id(new PhabricatorComposeChartFunction())
-      ->setArguments(array($argv));
+      ->setArguments($argv);
   }
 
 }

src/applications/fact/engine/PhabricatorChartRenderingEngine.php

@@ -178,6 +178,9 @@ final class PhabricatorChartRenderingEngine
             $rows[] = array(
               $xv,
               $yv,
+              null,
+              null,
+              null,
             );
           } else {
             foreach ($point_refs as $ref => $ref_data) {

src/applications/fact/engine/PhabricatorDemoChartEngine.php

@@ -0,0 +1,44 @@
+<?php
+
+final class PhabricatorDemoChartEngine
+  extends PhabricatorChartEngine {
+
+  const CHARTENGINEKEY = 'facts.demo';
+
+  protected function newChart(PhabricatorFactChart $chart, array $map) {
+    $viewer = $this->getViewer();
+
+    $functions = array();
+
+    $function = $this->newFunction(
+      array('scale', 0.0001),
+      array('cos'),
+      array('scale', 128),
+      array('shift', 256));
+
+    $function->getFunctionLabel()
+      ->setName(pht('cos(x)'))
+      ->setColor('rgba(0, 200, 0, 1)')
+      ->setFillColor('rgba(0, 200, 0, 0.15)');
+
+    $functions[] = $function;
+
+    $function = $this->newFunction(
+      array('constant', 345));
+
+    $function->getFunctionLabel()
+      ->setName(pht('constant(345)'))
+      ->setColor('rgba(0, 0, 200, 1)')
+      ->setFillColor('rgba(0, 0, 200, 0.15)');
+
+    $functions[] = $function;
+
+    $datasets = array();
+
+    $datasets[] = id(new PhabricatorChartStackedAreaDataset())
+      ->setFunctions($functions);
+
+    $chart->attachDatasets($datasets);
+  }
+
+}

src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php

@@ -34,62 +34,24 @@ final class PhabricatorProjectBurndownChartEngine
         $function = $this->newFunction(
           array(
             'accumulate',
-            array('fact', 'tasks.open-count.assign.project', $project_phid),
-          ),
-          array(
-            'min',
-            0,
+            array(
+              'compose',
+              array('fact', 'tasks.open-count.assign.project', $project_phid),
+              array('min', 0),
+            ),
           ));
 
         $function->getFunctionLabel()
           ->setName(pht('Tasks Moved Into Project'))
-          ->setColor('rgba(0, 200, 200, 1)')
-          ->setFillColor('rgba(0, 200, 200, 0.15)');
+          ->setColor('rgba(128, 128, 200, 1)')
+          ->setFillColor('rgba(128, 128, 200, 0.15)');
 
         $functions[] = $function;
 
         $function = $this->newFunction(
-          array(
-            'accumulate',
-            array('fact', 'tasks.open-count.status.project', $project_phid),
-          ),
-          array(
-            'min',
-            0,
-          ));
-
-        $function->getFunctionLabel()
-          ->setName(pht('Tasks Reopened'))
-          ->setColor('rgba(200, 0, 200, 1)')
-          ->setFillColor('rgba(200, 0, 200, 0.15)');
-
-        $functions[] = $function;
-
-        $function = $this->newFunction(
-          'sum',
           array(
             'accumulate',
             array('fact', 'tasks.open-count.create.project', $project_phid),
-          ),
-          array(
-            array(
-              'accumulate',
-              array('fact', 'tasks.open-count.status.project', $project_phid),
-            ),
-            array(
-              'max',
-              0,
-            ),
-          ),
-          array(
-            array(
-              'accumulate',
-              array('fact', 'tasks.open-count.assign.project', $project_phid),
-            ),
-            array(
-              'max',
-              0,
-            ),
           ));
 
         $function->getFunctionLabel()
@@ -98,27 +60,61 @@ final class PhabricatorProjectBurndownChartEngine
           ->setFillColor('rgba(0, 0, 200, 0.15)');
 
         $functions[] = $function;
+
+        $function = $this->newFunction(
+          array(
+            'accumulate',
+            array(
+              'compose',
+              array('fact', 'tasks.open-count.assign.project', $project_phid),
+              array('max', 0),
+            ),
+          ));
+
+        $function->getFunctionLabel()
+          ->setName(pht('Tasks Moved Out of Project'))
+          ->setColor('rgba(128, 200, 128, 1)')
+          ->setFillColor('rgba(128, 200, 128, 0.15)');
+
+        $functions[] = $function;
+
+        $function = $this->newFunction(
+          array(
+            'accumulate',
+            array('fact', 'tasks.open-count.status.project', $project_phid),
+          ));
+
+        $function->getFunctionLabel()
+          ->setName(pht('Tasks Closed'))
+          ->setColor('rgba(0, 200, 0, 1)')
+          ->setFillColor('rgba(0, 200, 0, 0.15)');
+
+        $functions[] = $function;
       }
     } else {
       $function = $this->newFunction(
-        'accumulate',
-        array('fact', 'tasks.open-count.create'));
+        array(
+          'accumulate',
+          array('fact', 'tasks.open-count.create'),
+        ));
 
       $function->getFunctionLabel()
         ->setName(pht('Tasks Created'))
-        ->setColor('rgba(0, 200, 200, 1)')
-        ->setFillColor('rgba(0, 200, 200, 0.15)');
+        ->setColor('rgba(0, 0, 200, 1)')
+        ->setFillColor('rgba(0, 0, 200, 0.15)');
 
       $functions[] = $function;
 
       $function = $this->newFunction(
-        'accumulate',
-        array('fact', 'tasks.open-count.status'));
+        array(
+          'accumulate',
+          array('fact', 'tasks.open-count.status'),
+        ));
 
       $function->getFunctionLabel()
-        ->setName(pht('Tasks Closed / Reopened'))
-        ->setColor('rgba(200, 0, 200, 1)')
-        ->setFillColor('rgba(200, 0, 200, 0.15)');
+        ->setName(pht('Tasks Closed'))
+        ->setColor('rgba(0, 200, 0, 1)')
+        ->setFillColor('rgba(0, 200, 0, 0.15)');
 
       $functions[] = $function;
     }

webroot/rsrc/js/application/fact/Chart.js

@@ -139,7 +139,20 @@ JX.install('Chart', {
 
       var area = d3.area()
         .x(function(d) { return x(to_date(d.x)); })
-        .y0(function(d) { return y(d.y0); })
+        .y0(function(d) {
+          // When the area is positive, draw it above the X axis. When the area
+          // is negative, draw it below the X axis. We currently avoid having
+          // functions which cross the X axis by clever construction.
+          if (d.y0 >= 0 && d.y1 >= 0) {
+            return y(d.y0);
+          }
+
+          if (d.y0 <= 0 && d.y1 <= 0) {
+            return y(d.y0);
+          }
+
+          return y(0);
+        })
         .y1(function(d) { return y(d.y1); });
 
       var line = d3.line()