From b75517918d86c88d5f388ee2f9d47f17759ed809 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Mar 2021 10:55:56 -0700 Subject: [PATCH] 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 --- resources/celerity/map.php | 14 +++++++------- .../diff/PhabricatorInlineCommentController.php | 17 +++++++++++++++++ .../diff/interface/PhabricatorInlineComment.php | 13 +++++++++++++ .../diff/view/PHUIDiffInlineCommentEditView.php | 6 ------ webroot/rsrc/js/application/diff/DiffInline.js | 13 ++----------- 5 files changed, 39 insertions(+), 24 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 67887ef683..47f0dfd9ad 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '68f29322', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => 'ffb69e3d', - 'differential.pkg.js' => 'bbf6d742', + 'differential.pkg.js' => 'fbde899f', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '78c9885d', 'maniphest.pkg.css' => '35995d6d', @@ -385,7 +385,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', 'rsrc/js/application/diff/DiffChangeset.js' => 'd7d3ba75', 'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5', - 'rsrc/js/application/diff/DiffInline.js' => 'c794b624', + 'rsrc/js/application/diff/DiffInline.js' => '62fff8eb', 'rsrc/js/application/diff/DiffInlineContentState.js' => '68e6339d', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', @@ -788,7 +788,7 @@ return array( 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => 'd7d3ba75', 'phabricator-diff-changeset-list' => 'cc2c5de5', - 'phabricator-diff-inline' => 'c794b624', + 'phabricator-diff-inline' => '62fff8eb', 'phabricator-diff-inline-content-state' => '68e6339d', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', @@ -1532,6 +1532,10 @@ return array( 'javelin-request', 'javelin-uri', ), + '62fff8eb' => array( + 'javelin-dom', + 'phabricator-diff-inline-content-state', + ), '65bb0011' => array( 'javelin-behavior', 'javelin-dom', @@ -2088,10 +2092,6 @@ return array( 'javelin-workflow', 'javelin-json', ), - 'c794b624' => array( - 'javelin-dom', - 'phabricator-diff-inline-content-state', - ), 'cc2c5de5' => array( 'javelin-install', 'phuix-button-view', diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 353a8d4430..4930502066 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -316,11 +316,28 @@ abstract class PhabricatorInlineCommentController $this->updateCommentContentState($inline); } + // NOTE: We're writing the comment as "deleted", then reloading to + // pick up context and undeleting it. This is silly -- we just want + // to load and attach context -- but just loading context is currently + // complicated (for example, context relies on cache keys that expect + // the inline to have an ID). + + $inline->setIsDeleted(1); + $this->saveComment($inline); // Reload the inline to attach context. $inline = $this->loadCommentByIDForEdit($inline->getID()); + // Now, we can read the source file and set the initial state. + $state = $inline->getContentState(); + $default_suggestion = $inline->getDefaultSuggestionText(); + $state->setContentSuggestionText($default_suggestion); + $inline->setContentState($state); + $inline->setIsDeleted(0); + + $this->saveComment($inline); + $edit_dialog = $this->buildEditDialog($inline); if ($this->getOperation() == 'reply') { diff --git a/src/infrastructure/diff/interface/PhabricatorInlineComment.php b/src/infrastructure/diff/interface/PhabricatorInlineComment.php index 76976b5929..1ab3db30b9 100644 --- a/src/infrastructure/diff/interface/PhabricatorInlineComment.php +++ b/src/infrastructure/diff/interface/PhabricatorInlineComment.php @@ -364,6 +364,19 @@ abstract class PhabricatorInlineComment return $this->getStorageObject()->getInlineContext(); } + public function getDefaultSuggestionText() { + $context = $this->getInlineContext(); + + if (!$context) { + return null; + } + + $default = $context->getBodyLines(); + $default = implode('', $default); + + return $default; + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php index 5c29c1666c..72e5adbe1f 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php @@ -114,9 +114,6 @@ final class PHUIDiffInlineCommentEditView $main = $state->getContentSuggestionText(); $main_count = count(phutil_split_lines($main)); - $default = $context->getBodyLines(); - $default = implode('', $default); - // Browsers ignore one leading newline in text areas. Add one so that // any actual leading newlines in the content are preserved. $main = "\n".$main; @@ -127,9 +124,6 @@ final class PHUIDiffInlineCommentEditView 'class' => 'inline-suggestion-input PhabricatorMonospaced', 'rows' => max(3, $main_count + 1), 'sigil' => 'inline-content-suggestion', - 'meta' => array( - 'defaultText' => $default, - ), ), $main); diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 399f159864..dde96ea0a3 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -772,21 +772,12 @@ JX.install('DiffInline', { this.setHasSuggestion(!this.getHasSuggestion()); - // The first time the user actually clicks the button and enables - // suggestions for a given editor state, fill the input with the - // underlying text if there isn't any text yet. + // Resize the suggestion input for size of the text. if (this.getHasSuggestion()) { if (this._editRow) { var node = this._getSuggestionNode(this._editRow); if (node) { - if (!node.value.length) { - var data = JX.Stratcom.getData(node); - if (!data.hasSetDefault) { - data.hasSetDefault = true; - node.value = data.defaultText; - node.rows = Math.max(3, node.value.split('\n').length); - } - } + node.rows = Math.max(3, node.value.split('\n').length); } } } -- 2.51.2