From 0cca40db3bc66d33a3de5afcdc01ef698670e688 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 May 2020 11:37:55 -0700 Subject: [PATCH] When creating an inline, save the current document engine Summary: Ref T13513. As part of inline metadata, save the document engine the change is being rendered with. This will allow other parts of the UI to detect that an inline was created on a Jupyter notebook but is being rendered on raw source, or whatever else. The immediate goal is to fix nonsensical inline snippet rendering in email on Jupyter notebooks. Test Plan: - Created inlines and replies on normal soure code, saw no document engine annotated in the database. - Created inlines and replies on a Jupyter notebook rendered in Jupyter mode, saw "jupyter" annotations in the database. - Swapped document engines between Jupyter and Source, etc. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21240 --- resources/celerity/map.php | 56 +++++++++---------- .../parser/DifferentialChangesetParser.php | 10 +++- .../PhabricatorInlineCommentController.php | 9 ++- .../interface/PhabricatorInlineComment.php | 9 +++ .../diff/view/PHUIDiffInlineCommentView.php | 1 + .../rsrc/js/application/diff/DiffChangeset.js | 14 +++-- .../js/application/diff/DiffChangesetList.js | 2 +- .../rsrc/js/application/diff/DiffInline.js | 13 ++++- 8 files changed, 75 insertions(+), 39 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index ccae91f844..9f7db23957 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '1e667bcb', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'd71d4531', - 'differential.pkg.js' => '5be7941a', + 'differential.pkg.js' => '5ec354a0', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -379,9 +379,9 @@ 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' => '700bf848', - 'rsrc/js/application/diff/DiffChangesetList.js' => '6e668c5b', - 'rsrc/js/application/diff/DiffInline.js' => '9a3963e0', + 'rsrc/js/application/diff/DiffChangeset.js' => '10ddd7e0', + 'rsrc/js/application/diff/DiffChangesetList.js' => '303efc90', + 'rsrc/js/application/diff/DiffInline.js' => 'a0ef0b54', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -774,9 +774,9 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => '700bf848', - 'phabricator-diff-changeset-list' => '6e668c5b', - 'phabricator-diff-inline' => '9a3963e0', + 'phabricator-diff-changeset' => '10ddd7e0', + 'phabricator-diff-changeset-list' => '303efc90', + 'phabricator-diff-inline' => 'a0ef0b54', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -1020,6 +1020,19 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), + '10ddd7e0' => 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', + ), '111bfd2d' => array( 'javelin-install', ), @@ -1175,6 +1188,11 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), + '303efc90' => array( + 'javelin-install', + 'phuix-button-view', + 'phabricator-diff-tree-view', + ), '308f9fe4' => array( 'javelin-install', 'javelin-util', @@ -1545,24 +1563,6 @@ return array( 'javelin-install', 'javelin-util', ), - '6e668c5b' => array( - 'javelin-install', - 'phuix-button-view', - 'phabricator-diff-tree-view', - ), - '700bf848' => 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', @@ -1808,9 +1808,6 @@ return array( 'javelin-request', 'javelin-router', ), - '9a3963e0' => array( - 'javelin-dom', - ), '9aae2b66' => array( 'javelin-install', 'javelin-util', @@ -1836,6 +1833,9 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut', ), + 'a0ef0b54' => array( + 'javelin-dom', + ), 'a17b84f1' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 102e5b9ea2..9ce06be69d 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1871,12 +1871,20 @@ final class DifferentialChangesetParser extends Phobject { $undo_templates[$key] = hsprintf('%s', $undo_template); } + $document_engine = $renderer->getDocumentEngine(); + if ($document_engine) { + $document_engine_key = $document_engine->getDocumentEngineKey(); + } else { + $document_engine_key = null; + } + $state = array( 'undoTemplates' => $undo_templates, 'rendererKey' => $renderer_key, 'highlight' => $viewstate->getHighlightLanguage(), 'characterEncoding' => $viewstate->getCharacterEncoding(), - 'documentEngine' => $viewstate->getDocumentEngineKey(), + 'requestDocumentEngineKey' => $viewstate->getDocumentEngineKey(), + 'responseDocumentEngineKey' => $document_engine_key, 'isHidden' => $viewstate->getHidden(), ); diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 529bd34c6e..51ff3f7766 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -296,8 +296,6 @@ abstract class PhabricatorInlineCommentController $draft_engine = $this->newDraftEngine(); if ($draft_engine) { $draft_engine->synchronize(); - } else { - phlog('no draft engine'); } return $this->buildEmptyResponse(); @@ -320,10 +318,15 @@ abstract class PhabricatorInlineCommentController ->setIsNewFile($is_new) ->setLineNumber($number) ->setLineLength($length) - ->setContent($this->getCommentText()) + ->setContent((string)$this->getCommentText()) ->setReplyToCommentPHID($this->getReplyToCommentPHID()) ->setIsEditing(true); + $document_engine_key = $request->getStr('documentEngineKey'); + if ($document_engine_key !== null) { + $inline->setDocumentEngineKey($document_engine_key); + } + // If you own this object, mark your own inlines as "Done" by default. $owner_phid = $this->loadObjectOwnerPHID($inline); if ($owner_phid) { diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php index 50079589fc..9de8305ad3 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php @@ -213,6 +213,15 @@ abstract class PhabricatorInlineComment return (bool)$this->getStorageObject()->getAttribute('editing', false); } + public function setDocumentEngineKey($engine_key) { + $this->getStorageObject()->setAttribute('documentEngineKey', $engine_key); + return $this; + } + + public function getDocumentEngineKey() { + return $this->getStorageObject()->getAttribute('documentEngineKey'); + } + public function getDateModified() { return $this->getStorageObject()->getDateModified(); } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index 3c3abea400..7c311008bf 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -90,6 +90,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { 'isSynthetic' => $is_synthetic, 'isDraftDone' => $is_draft_done, 'isEditing' => $inline->getIsEditing(), + 'documentEngineKey' => $inline->getDocumentEngineKey(), 'on_right' => $this->getIsOnRight(), ); diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index a6891dabcb..3b45d586b5 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -64,7 +64,8 @@ JX.install('DiffChangeset', { _ref: null, _rendererKey: null, _highlight: null, - _documentEngine: null, + _requestDocumentEngineKey: null, + _responseDocumentEngineKey: null, _characterEncoding: null, _undoTemplates: null, @@ -411,8 +412,12 @@ JX.install('DiffChangeset', { return this._highlight; }, - getDocumentEngine: function(engine) { - return this._documentEngine; + getRequestDocumentEngineKey: function() { + return this._requestDocumentEngineKey; + }, + + getResponseDocumentEngineKey: function() { + return this._responseDocumentEngineKey; }, getSelectableItems: function() { @@ -665,7 +670,8 @@ JX.install('DiffChangeset', { this._rendererKey = state.rendererKey; this._highlight = state.highlight; this._characterEncoding = state.characterEncoding; - this._documentEngine = state.documentEngine; + this._requestDocumentEngineKey = state.requestDocumentEngineKey; + this._responseDocumentEngineKey = state.responseDocumentEngineKey; this._isHidden = state.isHidden; var is_hidden = !this.isVisible(); diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index f7ebb1bf85..c794ffe1bc 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -945,7 +945,7 @@ JX.install('DiffChangesetList', { .setName(pht('View As Document Type...')) .setHandler(function(e) { var params = { - engine: changeset.getDocumentEngine(), + engine: changeset.getResponseDocumentEngineKey(), }; new JX.Workflow('/services/viewas/', params) diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 81616c1915..d06562b957 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -21,6 +21,7 @@ JX.install('DiffInline', { _replyToCommentPHID: null, _originalText: null, _snippet: null, + _documentEngineKey: null, _isDeleted: false, _isInvisible: false, @@ -88,6 +89,7 @@ JX.install('DiffInline', { this._changesetID = data.changesetID; this._isNew = false; this._snippet = data.snippet; + this._documentEngineKey = data.documentEngineKey; this._isEditing = data.isEditing; @@ -174,6 +176,7 @@ JX.install('DiffInline', { this._isNewFile = inline._isNewFile; this._changesetID = inline._changesetID; this._isNew = true; + this._documentEngineKey = inline._documentEngineKey; this._replyToCommentPHID = inline._phid; @@ -374,6 +377,11 @@ JX.install('DiffInline', { }, create: function(text) { + var changeset = this.getChangeset(); + if (!this._documentEngineKey) { + this._documentEngineKey = changeset.getResponseDocumentEngineKey(); + } + var uri = this._getInlineURI(); var handler = JX.bind(this, this._oncreateresponse); var data = this._newRequestData('new', text); @@ -507,8 +515,9 @@ JX.install('DiffInline', { length: this.getLineLength(), is_new: this.isNewFile(), changesetID: this.getChangesetID(), - replyToCommentPHID: this.getReplyToCommentPHID() || '', - text: text || '' + replyToCommentPHID: this.getReplyToCommentPHID(), + text: text || null, + documentEngineKey: this._documentEngineKey, }; }, -- 2.51.2