Make server components of inline comment content handling state-oriented

Summary: Ref T13513. Introduce a formal server-side content state object so the whole state can be saved and restored to the drafts table, read from the request, etc.

Test Plan: Created and edited inlines. Reloaded drafts with edits. Submitted normal and editing comments. Grepped for affected symbols.

Maniphest Tasks: T13513

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '42a2334f',
-    'differential.pkg.js' => 'ff8ca085',
+    'differential.pkg.js' => 'd0ddfb19',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -381,7 +381,7 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
     'rsrc/js/application/diff/DiffChangeset.js' => '6e5e03d2',
     'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
-    'rsrc/js/application/diff/DiffInline.js' => '28e53a2c',
+    'rsrc/js/application/diff/DiffInline.js' => '6fa445ef',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -776,7 +776,7 @@ return array(
     'phabricator-dashboard-css' => '5a205b9d',
     'phabricator-diff-changeset' => '6e5e03d2',
     'phabricator-diff-changeset-list' => 'b51ba93a',
-    'phabricator-diff-inline' => '28e53a2c',
+    'phabricator-diff-inline' => '6fa445ef',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1137,9 +1137,6 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    '28e53a2c' => array(
-      'javelin-dom',
-    ),
     '29819b75' => array(
       'phabricator-notification',
       'javelin-stratcom',
@@ -1564,6 +1561,9 @@ return array(
       'phabricator-diff-path-view',
       'phuix-button-view',
     ),
+    '6fa445ef' => array(
+      'javelin-dom',
+    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',

src/__phutil_library_map__.php

@@ -3160,6 +3160,7 @@ phutil_register_library_map(array(
     'PhabricatorDestructionEngineExtensionModule' => 'applications/system/engine/PhabricatorDestructionEngineExtensionModule.php',
     'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php',
+    'PhabricatorDiffInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php',
     'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php',
     'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php',
     'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php',
@@ -3592,6 +3593,7 @@ phutil_register_library_map(array(
     'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php',
     'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php',
     'PhabricatorInlineCommentAdjustmentEngine' => 'infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php',
+    'PhabricatorInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorInlineCommentContentState.php',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php',
     'PhabricatorInlineCommentInterface' => 'applications/transactions/interface/PhabricatorInlineCommentInterface.php',
     'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php',
@@ -9627,6 +9629,7 @@ phutil_register_library_map(array(
     'PhabricatorDestructionEngineExtensionModule' => 'PhabricatorConfigModule',
     'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
+    'PhabricatorDiffInlineCommentContentState' => 'PhabricatorInlineCommentContentState',
     'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery',
     'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel',
     'PhabricatorDiffScopeEngine' => 'Phobject',
@@ -10118,6 +10121,7 @@ phutil_register_library_map(array(
       'PhabricatorMarkupInterface',
     ),
     'PhabricatorInlineCommentAdjustmentEngine' => 'Phobject',
+    'PhabricatorInlineCommentContentState' => 'Phobject',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
     'PhabricatorInlineSummaryView' => 'AphrontView',
     'PhabricatorInstructionsEditField' => 'PhabricatorEditField',

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -52,10 +52,6 @@ abstract class PhabricatorInlineCommentController
     return $this->operation;
   }
 
-  public function getCommentText() {
-    return $this->commentText;
-  }
-
   public function getLineLength() {
     return $this->lineLength;
   }
@@ -270,10 +266,8 @@ abstract class PhabricatorInlineCommentController
           $viewer->getPHID(),
           $inline->getID());
 
-        $map = $this->getContentState();
+        $map = $this->newRequestContentState($inline)->newStorageMap();
-        foreach ($map as $key => $value) {
+        $versioned_draft->setProperty('inline.state', $map);
-          $versioned_draft->setProperty($key, $value);
-        }
         $versioned_draft->save();
 
         // We have to synchronize the draft engine after saving a versioned
@@ -524,14 +518,9 @@ abstract class PhabricatorInlineCommentController
     return (bool)$request->getBool('hasContentState');
   }
 
-  private function getContentState() {
+  private function newRequestContentState($inline) {
     $request = $this->getRequest();
-
+    return $inline->newContentStateFromRequest($request);
-    $comment_text = $request->getStr('text');
-
-    return array(
-      'inline.text' => (string)$comment_text,
-    );
   }
 
   private function updateCommentContentState(PhabricatorInlineComment $inline) {
@@ -542,11 +531,8 @@ abstract class PhabricatorInlineCommentController
           'content state.'));
     }
 
-    $state = $this->getContentState();
+    $state = $this->newRequestContentState($inline);
-
+    $inline->setContentState($state);
-    $text = $state['inline.text'];
-
-    $inline->setContent($text);
   }
 
   private function saveComment(PhabricatorInlineComment $inline) {

src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php

@@ -0,0 +1,69 @@
+<?php
+
+final class PhabricatorDiffInlineCommentContentState
+  extends PhabricatorInlineCommentContentState {
+
+  private $hasSuggestion = false;
+  private $suggestionText = '';
+
+  public function isEmptyContentState() {
+    if (!parent::isEmptyContentState()) {
+      return false;
+    }
+
+    if ($this->getContentHasSuggestion()) {
+      if (strlen($this->getSuggestionText())) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  public function setContentSuggestionText($suggestion_text) {
+    $this->suggestionText = $suggestion_text;
+    return $this;
+  }
+
+  public function getContentSuggestionText() {
+    return $this->suggestionText;
+  }
+
+  public function setContentHasSuggestion($has_suggestion) {
+    $this->hasSuggestion = $has_suggestion;
+    return $this;
+  }
+
+  public function getContentHasSuggestion() {
+    return $this->hasSuggestion;
+  }
+
+  public function newStorageMap() {
+    return parent::writeStorageMap() + array(
+      'hasSuggestion' => $this->getContentHasSuggestion(),
+      'suggestionText' => $this->getContentSuggestionText(),
+    );
+  }
+
+  public function readStorageMap(array $map) {
+    $result = parent::readStorageMap($map);
+
+    $has_suggestion = (bool)idx($map, 'hasSuggestion');
+    $this->setContentHasSuggestion($has_suggestion);
+
+    $suggestion_text = (string)idx($map, 'suggestionText');
+    $this->setContentSuggestionText($suggestion_text);
+
+    return $result;
+  }
+
+  protected function newStorageMapFromRequest(AphrontRequest $request) {
+    $map = parent::newStorageMapFromRequest($request);
+
+    $map['hasSuggestion'] = (bool)$request->getBool('hasSuggestion');
+    $map['suggestionText'] = (string)$request->getStr('suggestionText');
+
+    return $map;
+  }
+
+}

src/infrastructure/diff/inline/PhabricatorInlineCommentContentState.php

@@ -0,0 +1,47 @@
+<?php
+
+abstract class PhabricatorInlineCommentContentState
+  extends Phobject {
+
+  private $contentText = '';
+
+  public function setContentText($content_text) {
+    $this->contentText = $content_text;
+    return $this;
+  }
+
+  public function getContentText() {
+    return $this->contentText;
+  }
+
+  public function isEmptyContentState() {
+    return !strlen($this->getContentText());
+  }
+
+  public function writeStorageMap() {
+    return array(
+      'text' => $this->getContentText(),
+    );
+  }
+
+  public function readStorageMap(array $map) {
+    $text = (string)idx($map, 'text');
+    $this->setContentText($text);
+
+    return $this;
+  }
+
+  final public function readFromRequest(AphrontRequest $request) {
+    $map = $this->newStorageMapFromRequest($request);
+    return $this->readStorageMap($map);
+  }
+
+  protected function newStorageMapFromRequest(AphrontRequest $request) {
+    $map = array();
+
+    $map['text'] = (string)$request->getStr('text');
+
+    return $map;
+  }
+
+}

src/infrastructure/diff/interface/PhabricatorInlineComment.php

@@ -299,35 +299,58 @@ abstract class PhabricatorInlineComment
   }
 
   public function isVoidComment(PhabricatorUser $viewer) {
-    return !strlen($this->getContentForEdit($viewer));
+    return $this->getContentStateForEdit($viewer)->isEmptyContentState();
   }
 
-  public function getContentForEdit(PhabricatorUser $viewer) {
+  public function getContentStateForEdit(PhabricatorUser $viewer) {
-    $content = $this->getContent();
+    $state = $this->getContentState();
-
-    if (!$this->hasVersionedDraftForViewer($viewer)) {
-      return $content;
-    }
 
+    if ($this->hasVersionedDraftForViewer($viewer)) {
       $versioned_draft = $this->getVersionedDraftForViewer($viewer);
-    if (!$versioned_draft) {
+      if ($versioned_draft) {
-      return $content;
+        $storage_map = $versioned_draft->getProperty('inline.state');
+        if (is_array($storage_map)) {
+          $state->readStorageMap($storage_map);
+        }
+      }
     }
 
-    $draft_text = $versioned_draft->getProperty('inline.text');
+    return $state;
-    if ($draft_text === null) {
-      return $content;
   }
 
-    return $draft_text;
+  protected function newContentState() {
+    return new PhabricatorDiffInlineCommentContentState();
+  }
+
+  public function newContentStateFromRequest(AphrontRequest $request) {
+    return $this->newContentState()->readFromRequest($request);
   }
 
   public function getContentState() {
-    return array(
+    $state = $this->newContentState();
-      'text' => $this->getContent(),
+
-    );
+    $storage = $this->getStorageObject();
+    $storage_map = $storage->getAttribute('inline.state');
+    if (is_array($storage_map)) {
+      $state->readStorageMap($storage_map);
     }
 
+    $state->setContentText($this->getContent());
+
+    return $state;
+  }
+
+  public function setContentState(PhabricatorInlineCommentContentState $state) {
+    $storage = $this->getStorageObject();
+    $storage_map = $state->newStorageMap();
+    $storage->setAttribute('inline.state', $storage_map);
+
+    $this->setContent($state->getContentText());
+
+    return $this;
+  }
+
+
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 
 

src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php

@@ -218,9 +218,9 @@ abstract class PhabricatorDiffInlineCommentQuery
           // as it is currently shown to the user, not as it was stored the last
           // time they clicked "Save".
 
-          $draft_content = $inline->getContentForEdit($viewer);
+          $draft_state = $inline->getContentStateForEdit($viewer);
-          if (strlen($draft_content)) {
+          if (!$draft_state->isEmptyContentState()) {
-            $inline->setContent($draft_content);
+            $inline->setContentState($draft_state);
           }
         }
       }

src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php

@@ -101,7 +101,6 @@ final class PHUIDiffInlineCommentDetailView
       $classes[] = 'inline-comment-element';
     }
 
-    $content = $inline->getContent();
     $handles = $this->handles;
 
     $links = array();

src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php

@@ -82,13 +82,12 @@ final class PHUIDiffInlineCommentEditView
     $viewer = $this->getViewer();
     $inline = $this->getInlineComment();
 
-    $text = $inline->getContentForEdit($viewer);
+    $state = $inline->getContentStateForEdit($viewer);
 
     return id(new PhabricatorRemarkupControl())
       ->setViewer($viewer)
-      ->setSigil('differential-inline-comment-edit-textarea')
+      ->setSigil('inline-content-text')
-      ->setName('text')
+      ->setValue($state->getContentText())
-      ->setValue($text)
       ->setDisableFullScreen(true);
   }
 

webroot/rsrc/js/application/diff/DiffInline.js

@@ -701,7 +701,7 @@ JX.install('DiffInline', {
           var textareas = JX.DOM.scry(
             row,
             'textarea',
-            'differential-inline-comment-edit-textarea');
+            'inline-content-text');
           if (textareas.length) {
             var area = textareas[0];
             area.focus();
@@ -814,19 +814,25 @@ JX.install('DiffInline', {
     },
 
     _readFormState: function(row) {
-      var textarea;
+      var state = this._newContentState();
+
+      var node;
+
       try {
-        textarea = JX.DOM.find(
+        node = JX.DOM.find(row, 'textarea', 'inline-content-text');
-          row,
+        state.text = node.value;
-          'textarea',
-          'differential-inline-comment-edit-textarea');
       } catch (ex) {
-        return null;
+        // Ignore.
       }
 
-      return {
+      try {
-        text: textarea.value
+        node = JX.DOM.find(row, 'textarea', 'inline-content-suggestion');
-      };
+        state.suggestionText = node.value;
+      } catch (ex) {
+        // Ignore.
+      }
+
+      return state;
     },
 
     _onsubmitresponse: function(response) {
@@ -1053,20 +1059,24 @@ JX.install('DiffInline', {
       }
     },
 
+    _newContentState: function() {
+      return {
+        text: '',
+        suggestionText: '',
+        hasSuggestion: true
+      };
+    },
+
     _isVoidContentState: function(state) {
-      return !state.text.length;
+      return (!state.text.length && !state.suggestionText.length);
     },
 
     _isSameContentState: function(u, v) {
-      return (u.text === v.text);
+      return (
-    },
+        (u.text === v.text) &&
-
+        (u.suggestionText === v.suggestionText) &&
-    _newContentState: function() {
+        (u.hasSuggestion === v.hasSuggestion));
-      return {
-        text: ''
-      };
     }
-
   }
 
 });