Split Diffusion "view" preference into blame and color preferences

Summary:
We have this silly "view" preference which has a variety of silly values: "plain", "plainblame", "highlighted", and "blame", and then also "raw", which is magical. This is really just two flags: color on/off, and blame on/off (plus a separate mode for raw).

Express the code in terms of the flags and, e.g., get rid of the state transition tables we had before.

Test Plan: Viewed code in all four modes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

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

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -82,7 +82,7 @@ abstract class DiffusionBrowseController extends DiffusionController {
       id(new PhabricatorActionView())
         ->setName(pht('View History'))
         ->setHref($history_uri)
-        ->setIcon('perflab'));
+        ->setIcon('history'));
 
     $behind_head = $drequest->getRawCommit();
     $head_uri = $drequest->generateURI(

src/applications/diffusion/controller/DiffusionBrowseFileController.php

@@ -18,26 +18,42 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
     $path = $drequest->getPath();
 
-    $selected = $request->getStr('view');
     $preferences = $request->getUser()->loadPreferences();
-    if (!$selected) {
+
-      $selected = $preferences->getPreference(
+    $show_blame = $request->getBool(
-        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_VIEW,
+      'blame',
-        'highlighted');
+      $preferences->getPreference(
-    } else if ($request->isFormPost() && $selected != 'raw') {
+        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_BLAME,
+        false));
+    $show_color = $request->getBool(
+      'color',
+      $preferences->getPreference(
+        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_COLOR,
+        true));
+
+    $view = $request->getStr('view');
+    if ($request->isFormPost() && $view != 'raw') {
       $preferences->setPreference(
-        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_VIEW,
+        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_BLAME,
-        $selected);
+        $show_blame);
+      $preferences->setPreference(
+        PhabricatorUserPreferences::PREFERENCE_DIFFUSION_COLOR,
+        $show_color);
       $preferences->save();
 
-      return id(new AphrontRedirectResponse())
+      $uri = $request->getRequestURI()
-        ->setURI($request->getRequestURI()->alter('view', $selected));
+        ->alter('blame', null)
+        ->alter('color', null);
+
+      return id(new AphrontRedirectResponse())->setURI($uri);
     }
 
-    $needs_blame = ($selected == 'plainblame');
+    // We need the blame information if blame is on and we're building plain
-    if ($selected == 'blame' && $request->isAjax()) {
+    // text, or blame is on and this is an Ajax request. If blame is on and
-      $needs_blame = true;
+    // this is a colorized request, we don't show blame at first (we ajax it
-    }
+    // in afterward) so we don't need to query for it.
+    $needs_blame = ($show_blame && !$show_color) ||
+                   ($show_blame && $request->isAjax());
 
     $file_content = DiffusionFileContent::newFromConduit(
       $this->callConduitWithDiffusionRequest(
@@ -49,7 +65,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
         )));
     $data = $file_content->getCorpus();
 
-    if ($selected === 'raw') {
+    if ($view === 'raw') {
       return $this->buildRawResponse($path, $data);
     }
 
@@ -57,7 +73,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
     // Build the content of the file.
     $corpus = $this->buildCorpus(
-      $selected,
+      $show_blame,
+      $show_color,
       $file_content,
       $needs_blame,
       $drequest,
@@ -75,7 +92,11 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
     $content[] = $this->buildHeaderView($drequest);
     $view = $this->buildActionView($drequest);
-    $content[] = $this->enrichActionView($view, $drequest, $selected);
+    $content[] = $this->enrichActionView(
+      $view,
+      $drequest,
+      $show_blame,
+      $show_color);
     $content[] = $this->buildPropertyView($drequest);
 
     $follow  = $request->getStr('follow');
@@ -161,7 +182,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
   }
 
   private function buildCorpus(
-    $selected,
+    $show_blame,
+    $show_color,
     DiffusionFileContent $file_content,
     $needs_blame,
     DiffusionRequest $drequest,
@@ -181,22 +203,17 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
       }
     }
 
-    switch ($selected) {
+    if (!$show_color) {
-      case 'plain':
       $style =
         "margin: 1em 2em; width: 90%; height: 80em; font-family: monospace";
+      if (!$show_blame) {
         $corpus = phutil_tag(
           'textarea',
           array(
             'style' => $style,
           ),
           $file_content->getCorpus());
-
+      } else {
-          break;
-
-      case 'plainblame':
-        $style =
-          "margin: 1em 2em; width: 90%; height: 80em; font-family: monospace";
         $text_list = $file_content->getTextList();
         $rev_list = $file_content->getRevList();
         $blame_dict = $file_content->getBlameDict();
@@ -215,11 +232,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
             'style' => $style,
           ),
           implode("\n", $rows));
-        break;
+      }
-
+    } else {
-      case 'highlighted':
-      case 'blame':
-      default:
       require_celerity_resource('syntax-highlighting-css');
       $text_list = $file_content->getTextList();
       $rev_list = $file_content->getRevList();
@@ -232,7 +246,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
       $text_list = explode("\n", $text_list);
 
       $rows = $this->buildDisplayRows($text_list, $rev_list, $blame_dict,
-          $needs_blame, $drequest, $selected);
+        $needs_blame, $drequest, $show_blame, $show_color);
 
       $corpus_table = javelin_tag(
         'table',
@@ -286,8 +300,6 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
         $corpus_table);
 
       Javelin::initBehavior('load-blame', array('id' => $id));
-
-        break;
     }
 
     return $corpus;
@@ -296,58 +308,55 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
   private function enrichActionView(
     PhabricatorActionListView $view,
     DiffusionRequest $drequest,
-    $selected) {
+    $show_blame,
+    $show_color) {
 
     $viewer = $this->getRequest()->getUser();
     $base_uri = $this->getRequest()->getRequestURI();
 
-    $toggle_blame = array(
+    $view->addAction(
-      'highlighted'   => 'blame',
+      id(new PhabricatorActionView())
-      'blame'         => 'highlighted',
+        ->setName(pht('Show Last Change'))
-      'plain'         => 'plainblame',
+        ->setHref(
-      'plainblame'    => 'plain',
+          $drequest->generateURI(
-      'raw'           => 'raw',  // not a real case.
+            array(
-    );
+              'action' => 'change',
+            )))
+        ->setIcon('new'));
 
-    $toggle_highlight = array(
+    if ($show_blame) {
-      'highlighted'   => 'plain',
-      'blame'         => 'plainblame',
-      'plain'         => 'highlighted',
-      'plainblame'    => 'blame',
-      'raw'           => 'raw',  // not a real case.
-    );
-
-    $blame_on = ($selected == 'blame' || $selected == 'plainblame');
-    if ($blame_on) {
       $blame_text = pht('Disable Blame');
       $blame_icon = 'blame-grey';
+      $blame_value = 0;
     } else {
       $blame_text = pht('Enable Blame');
       $blame_icon = 'blame';
+      $blame_value = 1;
     }
 
     $view->addAction(
       id(new PhabricatorActionView())
         ->setName($blame_text)
-        ->setHref($base_uri->alter('view', $toggle_blame[$selected]))
+        ->setHref($base_uri->alter('blame', $blame_value))
         ->setIcon($blame_icon)
         ->setUser($viewer)
         ->setRenderAsForm(true));
 
 
-    $highlight_on = ($selected == 'blame' || $selected == 'highlighted');
+    if ($show_color) {
-    if ($highlight_on) {
       $highlight_text = pht('Disable Highlighting');
       $highlight_icon = 'highlight-grey';
+      $highlight_value = 0;
     } else {
       $highlight_text = pht('Enable Highlighting');
       $highlight_icon = 'highlight';
+      $highlight_value = 1;
     }
 
     $view->addAction(
       id(new PhabricatorActionView())
         ->setName($highlight_text)
-        ->setHref($base_uri->alter('view', $toggle_highlight[$selected]))
+        ->setHref($base_uri->alter('color', $highlight_value))
         ->setIcon($highlight_icon)
         ->setUser($viewer)
         ->setRenderAsForm(true));
@@ -417,7 +426,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
     array $blame_dict,
     $needs_blame,
     DiffusionRequest $drequest,
-    $selected) {
+    $show_blame,
+    $show_color) {
 
     $handles = array();
     if ($blame_dict) {
@@ -464,7 +474,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
         'data'        => $line,
       );
 
-      if ($selected == 'blame') {
+      if ($show_blame) {
         // If the line's rev is same as the line above, show empty content
         // with same color; otherwise generate blame info. The newer a change
         // is, the more saturated the color.
@@ -574,7 +584,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
     $rows = $this->renderInlines(
       idx($inlines, 0, array()),
-      ($selected == 'blame'),
+      ($show_blame),
       $engine);
 
     foreach ($display as $line) {
@@ -764,7 +774,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
 
       $rows = array_merge($rows, $this->renderInlines(
         idx($inlines, $line['line'], array()),
-        ($selected == 'blame'),
+        ($show_blame),
         $engine));
     }
 

src/applications/settings/storage/PhabricatorUserPreferences.php

@@ -18,7 +18,8 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO {
   const PREFERENCE_SEARCHBAR_JUMP       = 'searchbar-jump';
   const PREFERENCE_SEARCH_SHORTCUT      = 'search-shortcut';
 
-  const PREFERENCE_DIFFUSION_VIEW       = 'diffusion-view';
+  const PREFERENCE_DIFFUSION_BLAME      = 'diffusion-blame';
+  const PREFERENCE_DIFFUSION_COLOR      = 'diffusion-color';
 
   const PREFERENCE_NAV_COLLAPSED        = 'nav-collapsed';
   const PREFERENCE_NAV_WIDTH            = 'nav-width';