Refine unusual inline comment client interactions

Summary: Ref T13513. Refine some inline behaviors, see test plan.

Test Plan:
  - Edit a comment ("A"), type text ("AB"), cancel, edit.
    - Old behavior: edit and undo states (wrong, and undo does not function).
    - New behavior: edit state only.
  - Edit a comment ("A"), type text ("AB"), cancel. Undo ("AB"), cancel. Edit.
    - Old behavior: "AB" (wrong: you never submitted this text).
    - New behavior: "A".
  - Create a comment, type text, cancel.
    - Old behavior: counter appears in filetree (wrong, comment is undo-able but should not be counted).
    - New behavior: no counter.
  - Cancel editing an empty comment with no text.
    - Old behavior: Something buggy -- undo, I think?
    - New behavior: it just vanishes (correct behavior).

Maniphest Tasks: T13513

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

Author: epriestley

resources/celerity/map.php

@@ -13,7 +13,7 @@
     'core.pkg.js' => '632fb8f5',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '2d70b7b9',
-    'differential.pkg.js' => 'b35de23a',
+    'differential.pkg.js' => '4287e51f',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,9 +379,9 @@
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
-    'rsrc/js/application/diff/DiffChangeset.js' => '9a713ba5',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'a49dc31e',
     'rsrc/js/application/diff/DiffChangesetList.js' => '10726e6a',
-    'rsrc/js/application/diff/DiffInline.js' => '02791ed9',
+    'rsrc/js/application/diff/DiffInline.js' => '7f804f2b',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17',
@@ -776,9 +776,9 @@
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '9a713ba5',
+    'phabricator-diff-changeset' => 'a49dc31e',
     'phabricator-diff-changeset-list' => '10726e6a',
-    'phabricator-diff-inline' => '02791ed9',
+    'phabricator-diff-inline' => '7f804f2b',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
     'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -944,9 +944,6 @@
       'javelin-leader',
       'javelin-json',
     ),
-    '02791ed9' => array(
-      'javelin-dom',
-    ),
     '02cb4398' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1629,6 +1626,9 @@
       'javelin-install',
       'javelin-dom',
     ),
+    '7f804f2b' => array(
+      'javelin-dom',
+    ),
     '80bff3af' => array(
       'javelin-install',
       'javelin-typeahead-source',
@@ -1797,19 +1797,6 @@
       'javelin-request',
       'javelin-util',
     ),
-    '9a713ba5' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-      'phabricator-diff-path-view',
-      'phuix-button-view',
-    ),
     '9aae2b66' => array(
       'javelin-install',
       'javelin-util',
@@ -1863,6 +1850,19 @@
       'javelin-stratcom',
       'javelin-vector',
     ),
+    'a49dc31e' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+      'phabricator-diff-path-view',
+      'phuix-button-view',
+    ),
     'a4aa75c4' => array(
       'phui-button-css',
       'phui-button-simple-css',

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -220,6 +220,14 @@ public function processRequest() {
 
         $inline->setIsEditing(false);
 
+        // If the user uses "Undo" to get into an edited state ("AB"), then
+        // clicks cancel to return to the previous state ("A"), we also want
+        // to set the stored state back to "A".
+        $text = $this->getCommentText();
+        if (strlen($text)) {
+          $inline->setContent($text);
+        }
+
         $content = $inline->getContent();
         if (!strlen($content)) {
           $this->deleteComment($inline);

webroot/rsrc/js/application/diff/DiffChangeset.js

@@ -829,6 +829,10 @@ JX.install('DiffChangeset', {
           continue;
         }
 
+        if (inline.isUndo()) {
+          continue;
+        }
+
         if (inline.isSynthetic()) {
           continue;
         }

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

@@ -65,7 +65,13 @@ JX.install('DiffInline', {
 
       this._number = parseInt(data.number, 10);
       this._length = parseInt(data.length, 10);
-      this._originalText = data.original;
+
+      var original = '' + data.original;
+      if (original.length) {
+        this._originalText = original;
+      } else {
+        this._originalText = null;
+      }
       this._isNewFile = data.isNewFile;
 
       this._replyToCommentPHID = data.replyToCommentPHID;
@@ -103,6 +109,10 @@ JX.install('DiffInline', {
       return this._isEditing;
     },
 
+    isUndo: function() {
+      return !!this._undoRow;
+    },
+
     isDeleted: function() {
       return this._isDeleted;
     },
@@ -370,8 +380,21 @@ JX.install('DiffInline', {
     },
 
     edit: function(text) {
+      // If you edit an inline ("A"), modify the text ("AB"), cancel, and then
+      // edit it again: discard the undo state ("AB"). Otherwise we end up
+      // with an open editor and an active "Undo" link, which is weird.
+
+      if (this._undoRow) {
+        JX.DOM.remove(this._undoRow);
+        this._undoRow = null;
+
+        this._undoType = null;
+        this._undoText = null;
+      }
+
       var uri = this._getInlineURI();
       var handler = JX.bind(this, this._oneditresponse);
+
       var data = this._newRequestData('edit', text || null);
 
       this.setLoading(true);
@@ -647,10 +670,21 @@ JX.install('DiffInline', {
       }
 
       this.setEditing(false);
-      this.setInvisible(false);
+
+      // If this was an empty box and we typed some text and then hit cancel,
+      // don't show the empty concrete inline.
+      if (!this._originalText) {
+        this.setInvisible(true);
+      } else {
+        this.setInvisible(false);
+      }
+
+      // If you "undo" to restore text ("AB") and then "Cancel", we put you
+      // back in the original text state ("A"). We also send the original
+      // text ("A") to the server as the current persistent state.
 
       var uri = this._getInlineURI();
-      var data = this._newRequestData('cancel');
+      var data = this._newRequestData('cancel', this._originalText);
       var handler = JX.bind(this, this._onCancelResponse);
 
       this.setLoading(true);
@@ -664,6 +698,18 @@ JX.install('DiffInline', {
 
     _onCancelResponse: function(response) {
       this.setLoading(false);
+
+      // If the comment was empty when we started editing it (there's no
+      // original text) and empty when we finished editing it (there's no
+      // undo row), just delete the comment.
+      if (!this._originalText && !this.isUndo()) {
+        this.setDeleted(true);
+
+        JX.DOM.remove(this._row);
+        this._row = null;
+
+        this._didUpdate();
+      }
     },
 
     _readText: function(row) {