Skip to content

Commit b755179

Browse files
author
epriestley
committed
When creating an inline comment, populate the content state with the default suggestion text
Summary: Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state. This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed. Test Plan: Created inlines, clicked "Suggest Edit". See followup changes. Maniphest Tasks: T13559 Differential Revision: https://secure.phabricator.com/D21652
1 parent 5efe7fb commit b755179

File tree

5 files changed

+39
-24
lines changed

5 files changed

+39
-24
lines changed

resources/celerity/map.php

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
'core.pkg.js' => '68f29322',
1414
'dark-console.pkg.js' => '187792c2',
1515
'differential.pkg.css' => 'ffb69e3d',
16-
'differential.pkg.js' => 'bbf6d742',
16+
'differential.pkg.js' => 'fbde899f',
1717
'diffusion.pkg.css' => '42c75c37',
1818
'diffusion.pkg.js' => '78c9885d',
1919
'maniphest.pkg.css' => '35995d6d',
@@ -385,7 +385,7 @@
385385
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8',
386386
'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75',
387387
'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5',
388-
'rsrc/js/application/diff/DiffInline.js' => 'c794b624',
388+
'rsrc/js/application/diff/DiffInline.js' => '62fff8eb',
389389
'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d',
390390
'rsrc/js/application/diff/DiffPathView.js' => '8207abf9',
391391
'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b',
@@ -788,7 +788,7 @@
788788
'phabricator-dashboard-css' => '5a205b9d',
789789
'phabricator-diff-changeset' => 'd7d3ba75',
790790
'phabricator-diff-changeset-list' => 'cc2c5de5',
791-
'phabricator-diff-inline' => 'c794b624',
791+
'phabricator-diff-inline' => '62fff8eb',
792792
'phabricator-diff-inline-content-state' => '68e6339d',
793793
'phabricator-diff-path-view' => '8207abf9',
794794
'phabricator-diff-tree-view' => '5d83623b',
@@ -1532,6 +1532,10 @@
15321532
'javelin-request',
15331533
'javelin-uri',
15341534
),
1535+
'62fff8eb' => array(
1536+
'javelin-dom',
1537+
'phabricator-diff-inline-content-state',
1538+
),
15351539
'65bb0011' => array(
15361540
'javelin-behavior',
15371541
'javelin-dom',
@@ -2088,10 +2092,6 @@
20882092
'javelin-workflow',
20892093
'javelin-json',
20902094
),
2091-
'c794b624' => array(
2092-
'javelin-dom',
2093-
'phabricator-diff-inline-content-state',
2094-
),
20952095
'cc2c5de5' => array(
20962096
'javelin-install',
20972097
'phuix-button-view',

src/infrastructure/diff/PhabricatorInlineCommentController.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,11 +316,28 @@ public function processRequest() {
316316
$this->updateCommentContentState($inline);
317317
}
318318

319+
// NOTE: We're writing the comment as "deleted", then reloading to
320+
// pick up context and undeleting it. This is silly -- we just want
321+
// to load and attach context -- but just loading context is currently
322+
// complicated (for example, context relies on cache keys that expect
323+
// the inline to have an ID).
324+
325+
$inline->setIsDeleted(1);
326+
319327
$this->saveComment($inline);
320328

321329
// Reload the inline to attach context.
322330
$inline = $this->loadCommentByIDForEdit($inline->getID());
323331

332+
// Now, we can read the source file and set the initial state.
333+
$state = $inline->getContentState();
334+
$default_suggestion = $inline->getDefaultSuggestionText();
335+
$state->setContentSuggestionText($default_suggestion);
336+
$inline->setContentState($state);
337+
$inline->setIsDeleted(0);
338+
339+
$this->saveComment($inline);
340+
324341
$edit_dialog = $this->buildEditDialog($inline);
325342

326343
if ($this->getOperation() == 'reply') {

src/infrastructure/diff/interface/PhabricatorInlineComment.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,19 @@ public function getInlineContext() {
364364
return $this->getStorageObject()->getInlineContext();
365365
}
366366

367+
public function getDefaultSuggestionText() {
368+
$context = $this->getInlineContext();
369+
370+
if (!$context) {
371+
return null;
372+
}
373+
374+
$default = $context->getBodyLines();
375+
$default = implode('', $default);
376+
377+
return $default;
378+
}
379+
367380

368381
/* -( PhabricatorMarkupInterface Implementation )-------------------------- */
369382

src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,9 +114,6 @@ private function newCorpusView() {
114114
$main = $state->getContentSuggestionText();
115115
$main_count = count(phutil_split_lines($main));
116116

117-
$default = $context->getBodyLines();
118-
$default = implode('', $default);
119-
120117
// Browsers ignore one leading newline in text areas. Add one so that
121118
// any actual leading newlines in the content are preserved.
122119
$main = "\n".$main;
@@ -127,9 +124,6 @@ private function newCorpusView() {
127124
'class' => 'inline-suggestion-input PhabricatorMonospaced',
128125
'rows' => max(3, $main_count + 1),
129126
'sigil' => 'inline-content-suggestion',
130-
'meta' => array(
131-
'defaultText' => $default,
132-
),
133127
),
134128
$main);
135129

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

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -772,21 +772,12 @@ JX.install('DiffInline', {
772772

773773
this.setHasSuggestion(!this.getHasSuggestion());
774774

775-
// The first time the user actually clicks the button and enables
776-
// suggestions for a given editor state, fill the input with the
777-
// underlying text if there isn't any text yet.
775+
// Resize the suggestion input for size of the text.
778776
if (this.getHasSuggestion()) {
779777
if (this._editRow) {
780778
var node = this._getSuggestionNode(this._editRow);
781779
if (node) {
782-
if (!node.value.length) {
783-
var data = JX.Stratcom.getData(node);
784-
if (!data.hasSetDefault) {
785-
data.hasSetDefault = true;
786-
node.value = data.defaultText;
787-
node.rows = Math.max(3, node.value.split('\n').length);
788-
}
789-
}
780+
node.rows = Math.max(3, node.value.split('\n').length);
790781
}
791782
}
792783
}

0 commit comments

Comments
 (0)