Make "Open in Editor" use the simple line number of the current selected block

Summary:
Ref PHI1749. Instead of opening files to the last unchanged line on either side of the change, open files to the "simple" line number of the selected block.

For inlines, this is the inline line number.

For blocks, this is the first new-file line number, or the first old-file line number if no new-file line number exists in the block.

This may not always be what the user is hoping for (we can't know what the state of their working copy is) but should produce more obvious behavior.

Test Plan:
  - In Diffusion, used "Open in Editor" with and without line selections. Saw same behavior as before.
  - Used "n" and "r" to leave an inline with the keyboard, saw same behavior as before.
  - Used "\" and "Open in Editor" menu item to open a file with:
    - Nothing selected or changeset selected (line: 1).
    - An inline selected (line: inline line).
    - A block selected (line: first line in block, per above).

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

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '845355f4',
     'dark-console.pkg.js' => '187792c2',
     'differential.pkg.css' => '5c459f92',
-    'differential.pkg.js' => '24616785',
+    'differential.pkg.js' => 'a7171fb6',
     'diffusion.pkg.css' => '42c75c37',
     'diffusion.pkg.js' => 'a98c0bf7',
     'maniphest.pkg.css' => '35995d6d',
@@ -379,14 +379,15 @@ return array(
     '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' => '6e5e03d2',
+    'rsrc/js/application/diff/DiffChangeset.js' => '3a1ca35b',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'b51ba93a',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
     'rsrc/js/application/diff/DiffInline.js' => '008b6a15',
     'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
     'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
     'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
     'rsrc/js/application/differential/behavior-populate.js' => 'b86ef6c2',
     'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '94243d89',
+    'rsrc/js/application/diffusion/ExternalEditorLinkEngine.js' => '48a8641f',
     'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'b7b73831',
     'rsrc/js/application/diffusion/behavior-commit-branches.js' => '4b671572',
     'rsrc/js/application/diffusion/behavior-commit-graph.js' => 'ef836bf2',
@@ -484,7 +485,7 @@ return array(
     'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731',
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b',
     'rsrc/js/core/behavior-lightbox-attachments.js' => 'c7e748bf',
-    'rsrc/js/core/behavior-line-linker.js' => '590e6527',
+    'rsrc/js/core/behavior-line-linker.js' => '0d915ff5',
     'rsrc/js/core/behavior-linked-container.js' => '74446546',
     'rsrc/js/core/behavior-more.js' => '506aa3f4',
     'rsrc/js/core/behavior-object-selector.js' => '98ef467f',
@@ -645,7 +646,7 @@ return array(
     'javelin-behavior-phabricator-gesture-example' => '242dedd0',
     'javelin-behavior-phabricator-keyboard-pager' => '1325b731',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '42c44e8b',
-    'javelin-behavior-phabricator-line-linker' => '590e6527',
+    'javelin-behavior-phabricator-line-linker' => '0d915ff5',
     'javelin-behavior-phabricator-notification-example' => '29819b75',
     'javelin-behavior-phabricator-object-selector' => '98ef467f',
     'javelin-behavior-phabricator-oncopy' => 'da8f5259',
@@ -709,6 +710,7 @@ return array(
     'javelin-dom' => '94681e22',
     'javelin-dynval' => '202a2e85',
     'javelin-event' => 'c03f2fb4',
+    'javelin-external-editor-link-engine' => '48a8641f',
     'javelin-fx' => '34450586',
     'javelin-history' => '030b4f7a',
     'javelin-install' => '5902260c',
@@ -774,8 +776,8 @@ return array(
     'phabricator-darklog' => '3b869402',
     'phabricator-darkmessage' => '26cd4b73',
     'phabricator-dashboard-css' => '5a205b9d',
-    'phabricator-diff-changeset' => '6e5e03d2',
+    'phabricator-diff-changeset' => '3a1ca35b',
-    'phabricator-diff-changeset-list' => 'b51ba93a',
+    'phabricator-diff-changeset-list' => 'cc2c5de5',
     'phabricator-diff-inline' => '008b6a15',
     'phabricator-diff-path-view' => '8207abf9',
     'phabricator-diff-tree-view' => '5d83623b',
@@ -1013,6 +1015,13 @@ return array(
     '0d2490ce' => array(
       'javelin-install',
     ),
+    '0d915ff5' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-history',
+      'javelin-external-editor-link-engine',
+    ),
     '0eaa33a9' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1222,6 +1231,20 @@ return array(
       'trigger-rule',
       'trigger-rule-type',
     ),
+    '3a1ca35b' => 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',
+      'javelin-external-editor-link-engine',
+    ),
     '3ae89b20' => array(
       'phui-workcard-view-css',
     ),
@@ -1306,6 +1329,9 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
+    '48a8641f' => array(
+      'javelin-install',
+    ),
     '48fe33d0' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1442,12 +1468,6 @@ return array(
       'javelin-util',
       'javelin-magical-init',
     ),
-    '590e6527' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-history',
-    ),
     '5a6f6a06' => array(
       'javelin-behavior',
       'javelin-quicksand',
@@ -1551,19 +1571,6 @@ return array(
       'javelin-install',
       'javelin-util',
     ),
-    '6e5e03d2' => 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',
-    ),
     70245195 => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1970,11 +1977,6 @@ return array(
     'b517bfa0' => array(
       'phui-oi-list-view-css',
     ),
-    'b51ba93a' => array(
-      'javelin-install',
-      'phuix-button-view',
-      'phabricator-diff-tree-view',
-    ),
     'b557770a' => array(
       'javelin-install',
       'javelin-util',
@@ -2082,6 +2084,11 @@ return array(
       'phuix-icon-view',
       'phabricator-busy',
     ),
+    'cc2c5de5' => array(
+      'javelin-install',
+      'phuix-button-view',
+      'phabricator-diff-tree-view',
+    ),
     'cef53b3e' => array(
       'javelin-install',
       'javelin-dom',
@@ -2422,6 +2429,7 @@ return array(
       'phuix-formation-view',
       'phuix-formation-column-view',
       'phuix-formation-flank-view',
+      'javelin-external-editor-link-engine',
     ),
     'diffusion.pkg.css' => array(
       'diffusion-icons-css',

resources/celerity/packages.php

@@ -220,6 +220,8 @@ return array(
     'phuix-formation-view',
     'phuix-formation-column-view',
     'phuix-formation-flank-view',
+
+    'javelin-external-editor-link-engine',
   ),
   'diffusion.pkg.css' => array(
     'diffusion-icons-css',

src/applications/differential/view/DifferentialChangesetDetailView.php

@@ -252,7 +252,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
           'isLowImportance' => $changeset->getIsLowImportanceChangeset(),
           'isOwned' => $changeset->getIsOwnedChangeset(),
 
-          'editorURI' => $this->getEditorURI(),
+          'editorURITemplate' => $this->getEditorURITemplate(),
           'editorConfigureURI' => $this->getEditorConfigureURI(),
 
           'loaded' => $is_loaded,
@@ -321,7 +321,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
     return $this->diff;
   }
 
-  private function getEditorURI() {
+  private function getEditorURITemplate() {
     $repository = $this->getRepository();
     if (!$repository) {
       return null;
@@ -342,9 +342,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
     $path = $changeset->getAbsoluteRepositoryPath($repository, $diff);
     $path = ltrim($path, '/');
 
-    $line = idx($changeset->getMetadata(), 'line:first', 1);
+    return $link_engine->getURITokensForPath($path);
-
-    return $link_engine->getURIForPath($path, $line);
   }
 
   private function getEditorConfigureURI() {

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

@@ -11,6 +11,7 @@
  *           phabricator-diff-inline
  *           phabricator-diff-path-view
  *           phuix-button-view
+ *           javelin-external-editor-link-engine
  * @javelin
  */
 
@@ -33,7 +34,7 @@ JX.install('DiffChangeset', {
     this._pathParts = data.pathParts;
     this._icon = data.icon;
 
-    this._editorURI = data.editorURI;
+    this._editorURITemplate = data.editorURITemplate;
     this._editorConfigureURI = data.editorConfigureURI;
     this._showPathURI = data.showPathURI;
     this._showDirectoryURI = data.showDirectoryURI;
@@ -87,7 +88,7 @@ JX.install('DiffChangeset', {
     _changesetList: null,
     _icon: null,
 
-    _editorURI: null,
+    _editorURITemplate: null,
     _editorConfigureURI: null,
     _showPathURI: null,
     _showDirectoryURI: null,
@@ -102,8 +103,8 @@ JX.install('DiffChangeset', {
     _isSelected: false,
     _viewMenu: null,
 
-    getEditorURI: function() {
+    getEditorURITemplate: function() {
-      return this._editorURI;
+      return this._editorURITemplate;
     },
 
     getEditorConfigureURI: function() {

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

@@ -355,48 +355,11 @@ JX.install('DiffChangesetList', {
         // lines) reply on the old file.
 
         if (cursor.type == 'change') {
-          var origin = cursor.nodes.begin;
+          var cells = this._getLineNumberCellsForChangeBlock(
-          var target = cursor.nodes.end;
+            cursor.nodes.begin,
+            cursor.nodes.end);
 
-          // The "origin" and "target" are entire rows, but we need to find
+          cursor.changeset.newInlineForRange(cells.src, cells.dst);
-          // a range of "<th />" nodes to actually create an inline, so go
-          // fishing.
-
-          var old_list = [];
-          var new_list = [];
-
-          var row = origin;
-          while (row) {
-            var header = row.firstChild;
-            while (header) {
-              if (this.getLineNumberFromHeader(header)) {
-                if (header.className.indexOf('old') !== -1) {
-                  old_list.push(header);
-                } else if (header.className.indexOf('new') !== -1) {
-                  new_list.push(header);
-                }
-              }
-              header = header.nextSibling;
-            }
-
-            if (row == target) {
-              break;
-            }
-
-            row = row.nextSibling;
-          }
-
-          var use_list;
-          if (new_list.length) {
-            use_list = new_list;
-          } else {
-            use_list = old_list;
-          }
-
-          var src = use_list[0];
-          var dst = use_list[use_list.length - 1];
-
-          cursor.changeset.newInlineForRange(src, dst);
 
           this.setFocus(null);
           return;
@@ -407,6 +370,51 @@ JX.install('DiffChangesetList', {
       this._warnUser(pht('You must select a comment or change to reply to.'));
     },
 
+    _getLineNumberCellsForChangeBlock: function(origin, target) {
+      // The "origin" and "target" are entire rows, but we need to find
+      // a range of cell nodes to actually create an inline, so go
+      // fishing.
+
+      var old_list = [];
+      var new_list = [];
+
+      var row = origin;
+      while (row) {
+        var header = row.firstChild;
+        while (header) {
+          if (this.getLineNumberFromHeader(header)) {
+            if (header.className.indexOf('old') !== -1) {
+              old_list.push(header);
+            } else if (header.className.indexOf('new') !== -1) {
+              new_list.push(header);
+            }
+          }
+          header = header.nextSibling;
+        }
+
+        if (row == target) {
+          break;
+        }
+
+        row = row.nextSibling;
+      }
+
+      var use_list;
+      if (new_list.length) {
+        use_list = new_list;
+      } else {
+        use_list = old_list;
+      }
+
+      var src = use_list[0];
+      var dst = use_list[use_list.length - 1];
+
+      return {
+        src: src,
+        dst: dst
+      };
+    },
+
     _onkeyedit: function() {
       var cursor = this._cursorItem;
 
@@ -505,7 +513,7 @@ JX.install('DiffChangesetList', {
       return changeset;
     },
 
-    _onkeyopeneditor: function() {
+    _onkeyopeneditor: function(e) {
       var pht = this.getTranslations();
       var changeset = this._getChangesetForKeyCommand();
 
@@ -514,13 +522,61 @@ JX.install('DiffChangesetList', {
         return;
       }
 
-      var editor_uri = changeset.getEditorURI();
+      this._openEditor(changeset);
+    },
 
-      if (editor_uri === null) {
+    _openEditor: function(changeset) {
+      var pht = this.getTranslations();
+
+      var editor_template = changeset.getEditorURITemplate();
+      if (editor_template === null) {
         this._warnUser(pht('No external editor is configured.'));
         return;
       }
 
+      var line = null;
+
+      // See PHI1749. We aren't exactly sure what the user intends when they
+      // use the keyboard to select a change block and then activate the
+      // "Open in Editor" function: they might mean to open the old or new
+      // offset, and may have the old or new state (or some other state) in
+      // their working copy.
+
+      // For now, pick: the new state line number if one exists; or the old
+      // state line number if one does not. If nothing else, this behavior is
+      // simple.
+
+      // If there's a document engine, just open the file to the first line.
+      // We currently can not map display blocks to source lines.
+
+      // If there's an inline, open the file to that line.
+
+      if (changeset.getResponseDocumentEngineKey() === null) {
+        var cursor = this._cursorItem;
+        if (cursor && (cursor.changeset === changeset)) {
+          if (cursor.type == 'change') {
+            var cells = this._getLineNumberCellsForChangeBlock(
+              cursor.nodes.begin,
+              cursor.nodes.end);
+            line = this.getLineNumberFromHeader(cells.src);
+          }
+
+          if (cursor.type === 'comment') {
+            var inline = cursor.target;
+            line = inline.getLineNumber();
+          }
+        }
+      }
+
+      var variables = {
+        l: line || 1
+      };
+
+      var editor_uri = new JX.ExternalEditorLinkEngine()
+        .setTemplate(editor_template)
+        .setVariables(variables)
+        .newURI();
+
       JX.$U(editor_uri).go();
     },
 
@@ -1029,10 +1085,21 @@ JX.install('DiffChangesetList', {
         changeset.getShowPathURI())
         .setKeyCommand('d');
 
-      var editor_uri = changeset.getEditorURI();
+      var editor_template = changeset.getEditorURITemplate();
-      if (editor_uri !== null) {
+      if (editor_template !== null) {
-        add_link('fa-i-cursor', pht('Open in Editor'), editor_uri, true)
+        var editor_item = new JX.PHUIXActionView()
-          .setKeyCommand('\\');
+          .setIcon('fa-i-cursor')
+          .setName(pht('Open in Editor'))
+          .setKeyCommand('\\')
+          .setHandler(function(e) {
+
+            changeset_list._openEditor(changeset);
+
+            e.prevent();
+            menu.close();
+          });
+
+        list.addItem(editor_item);
       } else {
         var configure_uri = changeset.getEditorConfigureURI();
         if (configure_uri !== null) {

webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js

@@ -0,0 +1,41 @@
+/**
+ * @provides javelin-external-editor-link-engine
+ * @requires javelin-install
+ * @javelin
+ */
+
+JX.install('ExternalEditorLinkEngine', {
+
+  properties: {
+    template: null,
+    variables: null
+  },
+
+  members: {
+    newURI: function() {
+      var template = this.getTemplate();
+      var variables = this.getVariables();
+
+      var parts = [];
+      for (var ii = 0; ii < template.length; ii++) {
+        var part = template[ii];
+        var value = part.value;
+
+        if (part.type === 'literal') {
+          parts.push(value);
+          continue;
+        }
+
+        if (part.type === 'variable') {
+          if (variables.hasOwnProperty(value)) {
+            var replacement = variables[value];
+            replacement = encodeURIComponent(replacement);
+            parts.push(replacement);
+          }
+        }
+      }
+
+      return parts.join('');
+    }
+  }
+});

webroot/rsrc/js/core/behavior-line-linker.js

@@ -4,6 +4,7 @@
  *           javelin-stratcom
  *           javelin-dom
  *           javelin-history
+ *           javelin-external-editor-link-engine
  */
 
 JX.behavior('phabricator-line-linker', function() {
@@ -170,32 +171,19 @@ JX.behavior('phabricator-line-linker', function() {
 
       if (editor_link) {
         var data = JX.Stratcom.getData(editor_link);
-        var template = data.template;
 
         var variables = {
           l: parseInt(Math.min(o, t), 10),
         };
 
-        var parts = [];
+        var template = data.template;
-        for (var ii = 0; ii < template.length; ii++) {
-          var part = template[ii];
-          var value = part.value;
 
-          if (part.type === 'literal') {
+        var editor_uri = new JX.ExternalEditorLinkEngine()
-            parts.push(value);
+          .setTemplate(template)
-            continue;
+          .setVariables(variables)
-          }
+          .newURI();
 
-          if (part.type === 'variable') {
+        editor_link.href = editor_uri;
-            if (variables.hasOwnProperty(value)) {
-              var replacement = variables[value];
-              replacement = encodeURIComponent(replacement);
-              parts.push(replacement);
-            }
-          }
-        }
-
-        editor_link.href = parts.join('');
       }
     });