diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', - 'differential.pkg.js' => '24616785', + 'differential.pkg.js' => '2de0ef01', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -379,14 +379,15 @@ '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/DiffChangesetList.js' => 'b51ba93a', + 'rsrc/js/application/diff/DiffChangeset.js' => '3a1ca35b', + 'rsrc/js/application/diff/DiffChangesetList.js' => '672754c4', '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 @@ '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' => 'd26a5b82', '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 @@ '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' => 'd26a5b82', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => '98ef467f', 'javelin-behavior-phabricator-oncopy' => 'da8f5259', @@ -709,6 +710,7 @@ '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 @@ 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => '6e5e03d2', - 'phabricator-diff-changeset-list' => 'b51ba93a', + 'phabricator-diff-changeset' => '3a1ca35b', + 'phabricator-diff-changeset-list' => '672754c4', 'phabricator-diff-inline' => '008b6a15', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', @@ -1222,6 +1224,20 @@ '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 +1322,9 @@ 'javelin-dom', 'phabricator-draggable-list', ), + '48a8641f' => array( + 'javelin-install', + ), '48fe33d0' => array( 'javelin-behavior', 'javelin-dom', @@ -1442,12 +1461,6 @@ 'javelin-util', 'javelin-magical-init', ), - '590e6527' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-history', - ), '5a6f6a06' => array( 'javelin-behavior', 'javelin-quicksand', @@ -1515,6 +1528,11 @@ 'javelin-install', 'javelin-dom', ), + '672754c4' => array( + 'javelin-install', + 'phuix-button-view', + 'phabricator-diff-tree-view', + ), '6a1583a8' => array( 'javelin-behavior', 'javelin-history', @@ -1551,19 +1569,6 @@ '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 +1975,6 @@ 'b517bfa0' => array( 'phui-oi-list-view-css', ), - 'b51ba93a' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), 'b557770a' => array( 'javelin-install', 'javelin-util', @@ -2100,6 +2100,13 @@ 'javelin-workflow', 'javelin-util', ), + 'd26a5b82' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-history', + 'javelin-external-editor-link-engine', + ), 'd3799cb4' => array( 'javelin-install', ), @@ -2422,6 +2429,7 @@ 'phuix-formation-view', 'phuix-formation-column-view', 'phuix-formation-flank-view', + 'javelin-external-editor-link-engine', ), 'diffusion.pkg.css' => array( 'diffusion-icons-css', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -220,6 +220,8 @@ 'phuix-formation-view', 'phuix-formation-column-view', 'phuix-formation-flank-view', + + 'javelin-external-editor-link-engine', ), 'diffusion.pkg.css' => array( 'diffusion-icons-css', diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -252,7 +252,7 @@ 'isLowImportance' => $changeset->getIsLowImportanceChangeset(), 'isOwned' => $changeset->getIsOwnedChangeset(), - 'editorURI' => $this->getEditorURI(), + 'editorURITemplate' => $this->getEditorURITemplate(), 'editorConfigureURI' => $this->getEditorConfigureURI(), 'loaded' => $is_loaded, @@ -321,7 +321,7 @@ return $this->diff; } - private function getEditorURI() { + private function getEditorURITemplate() { $repository = $this->getRepository(); if (!$repository) { return null; @@ -342,9 +342,7 @@ $path = $changeset->getAbsoluteRepositoryPath($repository, $diff); $path = ltrim($path, '/'); - $line = idx($changeset->getMetadata(), 'line:first', 1); - - return $link_engine->getURIForPath($path, $line); + return $link_engine->getURITokensForPath($path); } private function getEditorConfigureURI() { diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/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 @@ 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 @@ _changesetList: null, _icon: null, - _editorURI: null, + _editorURITemplate: null, _editorConfigureURI: null, _showPathURI: null, _showDirectoryURI: null, @@ -102,8 +103,8 @@ _isSelected: false, _viewMenu: null, - getEditorURI: function() { - return this._editorURI; + getEditorURITemplate: function() { + return this._editorURITemplate; }, getEditorConfigureURI: function() { diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -355,48 +355,11 @@ // lines) reply on the old file. if (cursor.type == 'change') { - var origin = cursor.nodes.begin; - var target = cursor.nodes.end; - - // The "origin" and "target" are entire rows, but we need to find - // a range of "" 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 cells = this._getLineNumberCellsForChangeBlock( + cursor.nodes.begin, + cursor.nodes.end); - var src = use_list[0]; - var dst = use_list[use_list.length - 1]; - - cursor.changeset.newInlineForRange(src, dst); + cursor.changeset.newInlineForRange(cells.src, cells.dst); this.setFocus(null); return; @@ -407,6 +370,51 @@ 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 @@ return changeset; }, - _onkeyopeneditor: function() { + _onkeyopeneditor: function(e) { var pht = this.getTranslations(); var changeset = this._getChangesetForKeyCommand(); @@ -514,13 +522,61 @@ return; } - var editor_uri = changeset.getEditorURI(); + this._openEditor(changeset); + }, + + _openEditor: function(changeset) { + var pht = this.getTranslations(); - if (editor_uri === null) { + 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) { + 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 @@ changeset.getShowPathURI()) .setKeyCommand('d'); - var editor_uri = changeset.getEditorURI(); - if (editor_uri !== null) { - add_link('fa-i-cursor', pht('Open in Editor'), editor_uri, true) - .setKeyCommand('\\'); + var editor_template = changeset.getEditorURITemplate(); + if (editor_template !== null) { + var editor_item = new JX.PHUIXActionView() + .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) { diff --git a/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js b/webroot/rsrc/js/application/diffusion/ExternalEditorLinkEngine.js new file mode 100644 --- /dev/null +++ b/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(''); + } + } +}); diff --git a/webroot/rsrc/js/core/behavior-line-linker.js b/webroot/rsrc/js/core/behavior-line-linker.js --- a/webroot/rsrc/js/core/behavior-line-linker.js +++ b/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,21 @@ 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 = []; - 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); - } - } - } - - editor_link.href = parts.join(''); + var template = data.template; + + var editor_uri = new JX.ExternalEditorLinkEngine() + .setTemplate(template) + .setVariables(variables) + .newURI(); + + JX.log(editor_uri); + + editor_link.href = editor_uri; } });