Skip to content

Commit 0cca40d

Browse files
author
epriestley
committedMay 12, 2020
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
1 parent 6dc20d1 commit 0cca40d

File tree

8 files changed

+75
-39
lines changed

8 files changed

+75
-39
lines changed
 

‎resources/celerity/map.php

+28-28
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
'core.pkg.js' => '1e667bcb',
1414
'dark-console.pkg.js' => '187792c2',
1515
'differential.pkg.css' => 'd71d4531',
16-
'differential.pkg.js' => '5be7941a',
16+
'differential.pkg.js' => '5ec354a0',
1717
'diffusion.pkg.css' => '42c75c37',
1818
'diffusion.pkg.js' => 'a98c0bf7',
1919
'maniphest.pkg.css' => '35995d6d',
@@ -379,9 +379,9 @@
379379
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be',
380380
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9',
381381
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
382-
'rsrc/js/application/diff/DiffChangeset.js' => '700bf848',
383-
'rsrc/js/application/diff/DiffChangesetList.js' => '6e668c5b',
384-
'rsrc/js/application/diff/DiffInline.js' => '9a3963e0',
382+
'rsrc/js/application/diff/DiffChangeset.js' => '10ddd7e0',
383+
'rsrc/js/application/diff/DiffChangesetList.js' => '303efc90',
384+
'rsrc/js/application/diff/DiffInline.js' => 'a0ef0b54',
385385
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
386386
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
387387
'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd',
@@ -774,9 +774,9 @@
774774
'phabricator-darklog' => '3b869402',
775775
'phabricator-darkmessage' => '26cd4b73',
776776
'phabricator-dashboard-css' => '5a205b9d',
777-
'phabricator-diff-changeset' => '700bf848',
778-
'phabricator-diff-changeset-list' => '6e668c5b',
779-
'phabricator-diff-inline' => '9a3963e0',
777+
'phabricator-diff-changeset' => '10ddd7e0',
778+
'phabricator-diff-changeset-list' => '303efc90',
779+
'phabricator-diff-inline' => 'a0ef0b54',
780780
'phabricator-diff-path-view' => '8207abf9',
781781
'phabricator-diff-tree-view' => '5d83623b',
782782
'phabricator-drag-and-drop-file-upload' => '4370900d',
@@ -1020,6 +1020,19 @@
10201020
'javelin-workflow',
10211021
'phuix-icon-view',
10221022
),
1023+
'10ddd7e0' => array(
1024+
'javelin-dom',
1025+
'javelin-util',
1026+
'javelin-stratcom',
1027+
'javelin-install',
1028+
'javelin-workflow',
1029+
'javelin-router',
1030+
'javelin-behavior-device',
1031+
'javelin-vector',
1032+
'phabricator-diff-inline',
1033+
'phabricator-diff-path-view',
1034+
'phuix-button-view',
1035+
),
10231036
'111bfd2d' => array(
10241037
'javelin-install',
10251038
),
@@ -1175,6 +1188,11 @@
11751188
'phuix-icon-view',
11761189
'phabricator-prefab',
11771190
),
1191+
'303efc90' => array(
1192+
'javelin-install',
1193+
'phuix-button-view',
1194+
'phabricator-diff-tree-view',
1195+
),
11781196
'308f9fe4' => array(
11791197
'javelin-install',
11801198
'javelin-util',
@@ -1545,24 +1563,6 @@
15451563
'javelin-install',
15461564
'javelin-util',
15471565
),
1548-
'6e668c5b' => array(
1549-
'javelin-install',
1550-
'phuix-button-view',
1551-
'phabricator-diff-tree-view',
1552-
),
1553-
'700bf848' => array(
1554-
'javelin-dom',
1555-
'javelin-util',
1556-
'javelin-stratcom',
1557-
'javelin-install',
1558-
'javelin-workflow',
1559-
'javelin-router',
1560-
'javelin-behavior-device',
1561-
'javelin-vector',
1562-
'phabricator-diff-inline',
1563-
'phabricator-diff-path-view',
1564-
'phuix-button-view',
1565-
),
15661566
70245195 => array(
15671567
'javelin-behavior',
15681568
'javelin-stratcom',
@@ -1808,9 +1808,6 @@
18081808
'javelin-request',
18091809
'javelin-router',
18101810
),
1811-
'9a3963e0' => array(
1812-
'javelin-dom',
1813-
),
18141811
'9aae2b66' => array(
18151812
'javelin-install',
18161813
'javelin-util',
@@ -1836,6 +1833,9 @@
18361833
'javelin-util',
18371834
'phabricator-keyboard-shortcut',
18381835
),
1836+
'a0ef0b54' => array(
1837+
'javelin-dom',
1838+
),
18391839
'a17b84f1' => array(
18401840
'javelin-behavior',
18411841
'javelin-dom',

‎src/applications/differential/parser/DifferentialChangesetParser.php

+9-1
Original file line numberDiff line numberDiff line change
@@ -1871,12 +1871,20 @@ public function newChangesetResponse() {
18711871
$undo_templates[$key] = hsprintf('%s', $undo_template);
18721872
}
18731873

1874+
$document_engine = $renderer->getDocumentEngine();
1875+
if ($document_engine) {
1876+
$document_engine_key = $document_engine->getDocumentEngineKey();
1877+
} else {
1878+
$document_engine_key = null;
1879+
}
1880+
18741881
$state = array(
18751882
'undoTemplates' => $undo_templates,
18761883
'rendererKey' => $renderer_key,
18771884
'highlight' => $viewstate->getHighlightLanguage(),
18781885
'characterEncoding' => $viewstate->getCharacterEncoding(),
1879-
'documentEngine' => $viewstate->getDocumentEngineKey(),
1886+
'requestDocumentEngineKey' => $viewstate->getDocumentEngineKey(),
1887+
'responseDocumentEngineKey' => $document_engine_key,
18801888
'isHidden' => $viewstate->getHidden(),
18811889
);
18821890

‎src/infrastructure/diff/PhabricatorInlineCommentController.php

+6-3
Original file line numberDiff line numberDiff line change
@@ -296,8 +296,6 @@ public function processRequest() {
296296
$draft_engine = $this->newDraftEngine();
297297
if ($draft_engine) {
298298
$draft_engine->synchronize();
299-
} else {
300-
phlog('no draft engine');
301299
}
302300

303301
return $this->buildEmptyResponse();
@@ -320,10 +318,15 @@ public function processRequest() {
320318
->setIsNewFile($is_new)
321319
->setLineNumber($number)
322320
->setLineLength($length)
323-
->setContent($this->getCommentText())
321+
->setContent((string)$this->getCommentText())
324322
->setReplyToCommentPHID($this->getReplyToCommentPHID())
325323
->setIsEditing(true);
326324

325+
$document_engine_key = $request->getStr('documentEngineKey');
326+
if ($document_engine_key !== null) {
327+
$inline->setDocumentEngineKey($document_engine_key);
328+
}
329+
327330
// If you own this object, mark your own inlines as "Done" by default.
328331
$owner_phid = $this->loadObjectOwnerPHID($inline);
329332
if ($owner_phid) {

‎src/infrastructure/diff/interface/PhabricatorInlineComment.php

+9
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,15 @@ public function getIsEditing() {
213213
return (bool)$this->getStorageObject()->getAttribute('editing', false);
214214
}
215215

216+
public function setDocumentEngineKey($engine_key) {
217+
$this->getStorageObject()->setAttribute('documentEngineKey', $engine_key);
218+
return $this;
219+
}
220+
221+
public function getDocumentEngineKey() {
222+
return $this->getStorageObject()->getAttribute('documentEngineKey');
223+
}
224+
216225
public function getDateModified() {
217226
return $this->getStorageObject()->getDateModified();
218227
}

‎src/infrastructure/diff/view/PHUIDiffInlineCommentView.php

+1
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ protected function getInlineCommentMetadata() {
9090
'isSynthetic' => $is_synthetic,
9191
'isDraftDone' => $is_draft_done,
9292
'isEditing' => $inline->getIsEditing(),
93+
'documentEngineKey' => $inline->getDocumentEngineKey(),
9394

9495
'on_right' => $this->getIsOnRight(),
9596
);

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

+10-4
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ JX.install('DiffChangeset', {
6464
_ref: null,
6565
_rendererKey: null,
6666
_highlight: null,
67-
_documentEngine: null,
67+
_requestDocumentEngineKey: null,
68+
_responseDocumentEngineKey: null,
6869
_characterEncoding: null,
6970
_undoTemplates: null,
7071

@@ -411,8 +412,12 @@ JX.install('DiffChangeset', {
411412
return this._highlight;
412413
},
413414

414-
getDocumentEngine: function(engine) {
415-
return this._documentEngine;
415+
getRequestDocumentEngineKey: function() {
416+
return this._requestDocumentEngineKey;
417+
},
418+
419+
getResponseDocumentEngineKey: function() {
420+
return this._responseDocumentEngineKey;
416421
},
417422

418423
getSelectableItems: function() {
@@ -665,7 +670,8 @@ JX.install('DiffChangeset', {
665670
this._rendererKey = state.rendererKey;
666671
this._highlight = state.highlight;
667672
this._characterEncoding = state.characterEncoding;
668-
this._documentEngine = state.documentEngine;
673+
this._requestDocumentEngineKey = state.requestDocumentEngineKey;
674+
this._responseDocumentEngineKey = state.responseDocumentEngineKey;
669675
this._isHidden = state.isHidden;
670676

671677
var is_hidden = !this.isVisible();

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -945,7 +945,7 @@ JX.install('DiffChangesetList', {
945945
.setName(pht('View As Document Type...'))
946946
.setHandler(function(e) {
947947
var params = {
948-
engine: changeset.getDocumentEngine(),
948+
engine: changeset.getResponseDocumentEngineKey(),
949949
};
950950

951951
new JX.Workflow('/services/viewas/', params)

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

+11-2
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ JX.install('DiffInline', {
2121
_replyToCommentPHID: null,
2222
_originalText: null,
2323
_snippet: null,
24+
_documentEngineKey: null,
2425

2526
_isDeleted: false,
2627
_isInvisible: false,
@@ -88,6 +89,7 @@ JX.install('DiffInline', {
8889
this._changesetID = data.changesetID;
8990
this._isNew = false;
9091
this._snippet = data.snippet;
92+
this._documentEngineKey = data.documentEngineKey;
9193

9294
this._isEditing = data.isEditing;
9395

@@ -174,6 +176,7 @@ JX.install('DiffInline', {
174176
this._isNewFile = inline._isNewFile;
175177
this._changesetID = inline._changesetID;
176178
this._isNew = true;
179+
this._documentEngineKey = inline._documentEngineKey;
177180

178181
this._replyToCommentPHID = inline._phid;
179182

@@ -374,6 +377,11 @@ JX.install('DiffInline', {
374377
},
375378

376379
create: function(text) {
380+
var changeset = this.getChangeset();
381+
if (!this._documentEngineKey) {
382+
this._documentEngineKey = changeset.getResponseDocumentEngineKey();
383+
}
384+
377385
var uri = this._getInlineURI();
378386
var handler = JX.bind(this, this._oncreateresponse);
379387
var data = this._newRequestData('new', text);
@@ -507,8 +515,9 @@ JX.install('DiffInline', {
507515
length: this.getLineLength(),
508516
is_new: this.isNewFile(),
509517
changesetID: this.getChangesetID(),
510-
replyToCommentPHID: this.getReplyToCommentPHID() || '',
511-
text: text || ''
518+
replyToCommentPHID: this.getReplyToCommentPHID(),
519+
text: text || null,
520+
documentEngineKey: this._documentEngineKey,
512521
};
513522
},
514523

0 commit comments

Comments
 (0)
Failed to load comments.