Add an "Unsaved" button to the Differential persistent header

Summary: Ref T12733.

Test Plan: {F4987956}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12733

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

resources/celerity/map.php

@@ -12,8 +12,8 @@ return array(
     'core.pkg.css' => '5284a0e0',
     'core.pkg.js' => '1475bd91',
     'darkconsole.pkg.js' => '1f9a31bc',
-    'differential.pkg.css' => 'a2755617',
+    'differential.pkg.css' => '1ccbf3a9',
-    'differential.pkg.js' => '9cab3335',
+    'differential.pkg.js' => 'b453b745',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -64,7 +64,7 @@ return array(
     'rsrc/css/application/dashboard/dashboard.css' => 'fe5b1869',
     'rsrc/css/application/diff/inline-comment-summary.css' => '51efda3a',
     'rsrc/css/application/differential/add-comment.css' => 'c47f8c40',
-    'rsrc/css/application/differential/changeset-view.css' => '983751ee',
+    'rsrc/css/application/differential/changeset-view.css' => 'c3f44655',
     'rsrc/css/application/differential/core.css' => '5b7b8ff4',
     'rsrc/css/application/differential/phui-inline-comment.css' => 'ffd1a542',
     'rsrc/css/application/differential/revision-comment.css' => '14b8565a',
@@ -393,8 +393,8 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/diff/DiffChangeset.js' => 'aaaf4cb5',
+    'rsrc/js/application/diff/DiffChangeset.js' => 'fc3919c8',
-    'rsrc/js/application/diff/DiffChangesetList.js' => '85abc805',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'ffa063d8',
     'rsrc/js/application/diff/DiffInline.js' => '1d17130f',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07',
@@ -526,7 +526,7 @@ return array(
     'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8',
     'rsrc/js/phuix/PHUIXActionView.js' => 'b3465b9b',
     'rsrc/js/phuix/PHUIXAutocomplete.js' => 'f6699267',
-    'rsrc/js/phuix/PHUIXButtonView.js' => '0f13520b',
+    'rsrc/js/phuix/PHUIXButtonView.js' => 'b3c515be',
     'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50',
     'rsrc/js/phuix/PHUIXExample.js' => '68af71ca',
     'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671',
@@ -560,7 +560,7 @@ return array(
     'conpherence-thread-manager' => '4d863052',
     'conpherence-transaction-css' => '85129c68',
     'd3' => 'a11a5ff2',
-    'differential-changeset-view-css' => '983751ee',
+    'differential-changeset-view-css' => 'c3f44655',
     'differential-core-view-css' => '5b7b8ff4',
     'differential-revision-add-comment-css' => 'c47f8c40',
     'differential-revision-comment-css' => '14b8565a',
@@ -772,8 +772,8 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => 'aaaf4cb5',
+    'phabricator-diff-changeset' => 'fc3919c8',
-    'phabricator-diff-changeset-list' => '85abc805',
+    'phabricator-diff-changeset-list' => 'ffa063d8',
     'phabricator-diff-inline' => '1d17130f',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
@@ -878,7 +878,7 @@ return array(
     'phuix-action-list-view' => 'b5c256b8',
     'phuix-action-view' => 'b3465b9b',
     'phuix-autocomplete' => 'f6699267',
-    'phuix-button-view' => '0f13520b',
+    'phuix-button-view' => 'b3c515be',
     'phuix-dropdown-menu' => '8018ee50',
     'phuix-form-control-view' => '83e03671',
     'phuix-icon-view' => 'bff6884b',
@@ -960,10 +960,6 @@ return array(
       'javelin-dom',
       'javelin-router',
     ),
-    '0f13520b' => array(
-      'javelin-install',
-      'javelin-dom',
-    ),
     '0f764c35' => array(
       'javelin-install',
       'javelin-util',
@@ -1514,9 +1510,6 @@ return array(
       'javelin-dom',
       'javelin-stratcom',
     ),
-    '85abc805' => array(
-      'javelin-install',
-    ),
     '85ee8ce6' => array(
       'aphront-dialog-view-css',
     ),
@@ -1621,9 +1614,6 @@ return array(
       'javelin-mask',
       'phabricator-drag-and-drop-file-upload',
     ),
-    '983751ee' => array(
-      'phui-inline-comment-view-css',
-    ),
     '9a6dd75c' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1716,17 +1706,6 @@ return array(
       'javelin-util',
       'phabricator-prefab',
     ),
-    'aaaf4cb5' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     'ab2f381b' => array(
       'javelin-request',
       'javelin-behavior',
@@ -1785,6 +1764,10 @@ return array(
       'javelin-behavior',
       'phabricator-prefab',
     ),
+    'b3c515be' => array(
+      'javelin-install',
+      'javelin-dom',
+    ),
     'b3e7d692' => array(
       'javelin-install',
     ),
@@ -1877,6 +1860,9 @@ return array(
       'javelin-dom',
       'javelin-vector',
     ),
+    'c3f44655' => array(
+      'phui-inline-comment-view-css',
+    ),
     'c420b0b9' => array(
       'javelin-behavior',
       'javelin-behavior-device',
@@ -2155,6 +2141,17 @@ return array(
       'javelin-behavior-device',
       'phabricator-keyboard-shortcut',
     ),
+    'fc3919c8' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     'fc91ab6c' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2166,6 +2163,10 @@ return array(
       'javelin-view-visitor',
       'javelin-util',
     ),
+    'ffa063d8' => array(
+      'javelin-install',
+      'phuix-button-view',
+    ),
   ),
   'packages' => array(
     'conpherence.pkg.css' => array(

src/applications/differential/view/DifferentialChangesetListView.php

@@ -275,6 +275,10 @@ final class DifferentialChangesetListView extends AphrontView {
           pht('Hide or show the current file.'),
         'You must select a file to hide or show.' =>
           pht('You must select a file to hide or show.'),
+
+        'Unsaved' => pht('Unsaved'),
+        'Unsubmitted' => pht('Unsubmitted'),
+        'Comments' => pht('Comments'),
       ),
     ));
 

webroot/rsrc/css/application/differential/changeset-view.css

@@ -413,3 +413,7 @@ tr.differential-inline-loading {
 .diff-banner-has-unsubmitted {
   background: {$sh-yellowbackground};
 }
+
+.diff-banner-buttons {
+  float: right;
+}

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

@@ -403,6 +403,8 @@ JX.install('DiffChangeset', {
         block.items.push(rows[ii]);
       }
 
+      var last_inline = null;
+      var last_inline_item = null;
       for (ii = 0; ii < blocks.length; ii++) {
         block = blocks[ii];
 
@@ -422,16 +424,38 @@ JX.install('DiffChangeset', {
           for (var jj = 0; jj < block.items.length; jj++) {
             var inline = this.getInlineForRow(block.items[jj]);
 
-            items.push({
+            // When comments are being edited, they have a hidden row with
+            // the actual comment and then a visible row with the editor.
+
+            // In this case, we only want to generate one item, but it should
+            // use the editor as a scroll target. To accomplish this, check if
+            // this row has the same inline as the previous row. If so, update
+            // the last item to use this row's nodes.
+
+            if (inline === last_inline) {
+              last_inline_item.nodes.begin = block.items[jj];
+              last_inline_item.nodes.end = block.items[jj];
+              continue;
+            } else {
+              last_inline = inline;
+            }
+
+            last_inline_item = {
               type: block.type,
               changeset: this,
               target: inline,
               hidden: inline.isHidden(),
+              deleted: !inline.getID() && !inline.isEditing(),
               nodes: {
                 begin: block.items[jj],
                 end: block.items[jj]
+              },
+              attributes: {
+                unsaved: inline.isEditing()
               }
-            });
+            };
+
+            items.push(last_inline_item);
           }
         }
       }

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -1,6 +1,7 @@
 /**
  * @provides phabricator-diff-changeset-list
  * @requires javelin-install
+ *           phuix-button-view
  * @javelin
  */
 
@@ -111,6 +112,7 @@ JX.install('DiffChangesetList', {
     _rangeTarget: null,
 
     _bannerNode: null,
+    _unsavedButton: null,
 
     sleep: function() {
       this._asleep = true;
@@ -258,7 +260,13 @@ JX.install('DiffChangesetList', {
 
     _installJumpKey: function(key, label, delta, filter, show_hidden) {
       filter = filter || null;
-      var handler = JX.bind(this, this._onjumpkey, delta, filter, show_hidden);
+
+      var options = {
+        filter: filter,
+        hidden: show_hidden
+      };
+
+      var handler = JX.bind(this, this._onjumpkey, delta, options);
       return this._installKey(key, label, handler);
     },
 
@@ -440,9 +448,14 @@ JX.install('DiffChangesetList', {
         .show();
     },
 
-    _onjumpkey: function(delta, filter, show_hidden, manager) {
+    _onjumpkey: function(delta, options) {
       var state = this._getSelectionState();
 
+      var filter = options.filter || null;
+      var hidden = options.hidden || false;
+      var wrap = options.wrap || false;
+      var attribute = options.attribute || null;
+
       var cursor = state.cursor;
       var items = state.items;
 
@@ -452,6 +465,7 @@ JX.install('DiffChangesetList', {
         return;
       }
 
+      var did_wrap = false;
       while (true) {
         if (cursor === null) {
           cursor = 0;
@@ -464,9 +478,22 @@ JX.install('DiffChangesetList', {
           return;
         }
 
-        // If we've gone forward off the end of the list, bail out.
+        // If we've gone forward off the end of the list, figure out where we
+        // should end up.
         if (cursor >= items.length) {
-          return;
+          if (!wrap) {
+            // If we aren't wrapping around, we're done.
+            return;
+          }
+
+          if (did_wrap) {
+            // If we're already wrapped around, we're done.
+            return;
+          }
+
+          // Otherwise, wrap the cursor back to the top.
+          cursor = 0;
+          did_wrap = true;
         }
 
         // If we're selecting things of a particular type (like only files)
@@ -479,17 +506,31 @@ JX.install('DiffChangesetList', {
 
         // If the item is hidden, don't select it when iterating with jump
         // keys. It can still potentially be selected in other ways.
-        if (!show_hidden) {
+        if (!hidden) {
           if (items[cursor].hidden) {
             continue;
           }
         }
 
+        // If the item has been deleted, don't select it when iterating. The
+        // cursor may remain on it until it is removed.
+        if (items[cursor].deleted) {
+          continue;
+        }
+
+        // If we're selecting things with a particular attribute, like
+        // "unsaved", skip items without the attribute.
+        if (attribute !== null) {
+          if (!(items[cursor].attributes || {})[attribute]) {
+            continue;
+          }
+        }
+
         // Otherwise, we've found a valid item to select.
         break;
       }
 
-      this._setSelectionState(items[cursor], manager);
+      this._setSelectionState(items[cursor]);
     },
 
     _getSelectionState: function() {
@@ -512,24 +553,34 @@ JX.install('DiffChangesetList', {
       };
     },
 
-    _setSelectionState: function(item, manager) {
+    _setSelectionState: function(item) {
       this._cursorItem = item;
-      this._redrawSelection(manager, true);
+      this._redrawSelection(true);
 
       return this;
     },
 
-    _redrawSelection: function(manager, scroll) {
+    _redrawSelection: function(scroll) {
       var cursor = this._cursorItem;
       if (!cursor) {
         this.setFocus(null);
         return;
       }
 
+      // If this item has been removed from the document (for example: create
+      // a new empty comment, then use the "Unsaved" button to select it, then
+      // cancel it), we can still keep the cursor here but do not want to show
+      // a selection reticle over an invisible node.
+      if (cursor.deleted) {
+        this.setFocus(null);
+        return;
+      }
+
       this.setFocus(cursor.nodes.begin, cursor.nodes.end);
 
-      if (manager && scroll) {
+      if (scroll) {
-        manager.scrollTo(cursor.nodes.begin);
+        var pos = JX.$V(cursor.nodes.begin);
+        JX.DOM.scrollToPosition(0, pos.y - 60);
       }
 
       return this;
@@ -1320,14 +1371,65 @@ JX.install('DiffChangesetList', {
         'diff-banner-has-unsubmitted',
         !!unsubmitted.length);
 
+      var unsaved_button = this._getUnsavedButton();
+      var pht = this.getTranslations();
+
+      if (unsaved.length) {
+        unsaved_button.setText(unsaved.length + ' ' + pht('Unsaved'));
+        JX.DOM.show(unsaved_button.getNode());
+      } else {
+        JX.DOM.hide(unsaved_button.getNode());
+      }
+
+      var path_view = [icon, ' ', changeset.getDisplayPath()];
+
+      var buttons_attrs = {
+        className: 'diff-banner-buttons'
+      };
+
+      var buttons_list = [
+        unsaved_button.getNode()
+      ];
+
+      var buttons_view = JX.$N('div', buttons_attrs, buttons_list);
+
       var icon = new JX.PHUIXIconView()
         .setIcon(changeset.getIcon())
         .getNode();
-      JX.DOM.setContent(node, [icon, ' ', changeset.getDisplayPath()]);
+      JX.DOM.setContent(node, [buttons_view, path_view]);
 
       document.body.appendChild(node);
     },
 
+    _getUnsavedButton: function() {
+      if (!this._unsavedButton) {
+        var button = new JX.PHUIXButtonView()
+          .setIcon('fa-commenting-o')
+          .setButtonType(JX.PHUIXButtonView.BUTTONTYPE_SIMPLE);
+
+        var node = button.getNode();
+
+        var onunsaved = JX.bind(this, this._onunsavedclick);
+        JX.DOM.listen(node, 'click', null, onunsaved);
+
+        this._unsavedButton = button;
+      }
+
+      return this._unsavedButton;
+    },
+
+    _onunsavedclick: function(e) {
+      e.kill();
+
+      var options = {
+        filter: 'comment',
+        wrap: true,
+        attribute: 'unsaved'
+      };
+
+      this._onjumpkey(1, options);
+    },
+
     _getBannerNode: function() {
       if (!this._bannerNode) {
         var attributes = {

webroot/rsrc/js/phuix/PHUIXButtonView.js

@@ -57,6 +57,7 @@ JX.install('PHUIXButtonView', {
 
     setText: function(text) {
       JX.DOM.setContent(this._getTextNode(), text);
+      this._redraw();
       return this;
     },