From 846562158aae026a6207975c4c2681ae4856603b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 May 2020 14:50:47 -0700 Subject: [PATCH] Roughly support inline comment suggestions Summary: Ref T13513. This still has quite a few rough edges and some significant performance isssues, but appears to mostly work. Allow reviewers to "Suggest Edit" on an inline comment and provide replacement text for the highlighted source. Test Plan: Created, edited, reloaded, and submitted inline comments in various states with and without suggestion text. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21276 --- resources/celerity/map.php | 18 +- src/__phutil_library_map__.php | 4 + .../PhabricatorAuditTransactionComment.php | 15 +- .../DifferentialChangesetViewController.php | 1 + .../editor/DifferentialRevisionEditEngine.php | 7 + .../DifferentialTransactionComment.php | 15 +- .../view/DifferentialChangesetListView.php | 3 + .../PhabricatorInlineCommentController.php | 32 +++- ...abricatorDiffInlineCommentContentState.php | 2 +- .../PhabricatorDiffInlineCommentContext.php | 37 +++++ .../PhabricatorInlineCommentContext.php | 4 + .../interface/PhabricatorInlineComment.php | 4 + .../PhabricatorDiffInlineCommentQuery.php | 155 +++++++++++++++--- .../view/PHUIDiffInlineCommentDetailView.php | 74 ++++++++- .../view/PHUIDiffInlineCommentEditView.php | 127 +++++++++++++- .../diff/view/PHUIDiffInlineCommentView.php | 2 +- .../differential/phui-inline-comment.css | 57 +++++++ .../rsrc/js/application/diff/DiffInline.js | 113 ++++++++++++- 18 files changed, 615 insertions(+), 55 deletions(-) create mode 100644 src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php create mode 100644 src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index e57a9bfc0f..434ca6e869 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,8 +12,8 @@ return array( 'core.pkg.css' => 'ba768cdb', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => '42a2334f', - 'differential.pkg.js' => 'd0ddfb19', + 'differential.pkg.css' => 'f924dbcf', + 'differential.pkg.js' => '256a327a', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -65,7 +65,7 @@ return array( 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', 'rsrc/css/application/differential/changeset-view.css' => '60c3d405', 'rsrc/css/application/differential/core.css' => '7300a73e', - 'rsrc/css/application/differential/phui-inline-comment.css' => 'd5749acc', + 'rsrc/css/application/differential/phui-inline-comment.css' => '4107254a', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d', @@ -381,7 +381,7 @@ return array( '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/DiffInline.js' => '6fa445ef', + 'rsrc/js/application/diff/DiffInline.js' => '829b88bf', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -776,7 +776,7 @@ return array( 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => '6e5e03d2', 'phabricator-diff-changeset-list' => 'b51ba93a', - 'phabricator-diff-inline' => '6fa445ef', + 'phabricator-diff-inline' => '829b88bf', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -854,7 +854,7 @@ return array( 'phui-icon-view-css' => '4cbc684a', 'phui-image-mask-css' => '62c7f4d2', 'phui-info-view-css' => 'a10a909b', - 'phui-inline-comment-view-css' => 'd5749acc', + 'phui-inline-comment-view-css' => '4107254a', 'phui-invisible-character-view-css' => 'c694c4a4', 'phui-left-right-css' => '68513c34', 'phui-lightbox-css' => '4ebf22da', @@ -1561,9 +1561,6 @@ return array( 'phabricator-diff-path-view', 'phuix-button-view', ), - '6fa445ef' => array( - 'javelin-dom', - ), 70245195 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1642,6 +1639,9 @@ return array( '8207abf9' => array( 'javelin-dom', ), + '829b88bf' => array( + 'javelin-dom', + ), 83754533 => array( 'javelin-install', 'javelin-util', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 114dcc98d0..fa3eaabe7a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3161,6 +3161,7 @@ phutil_register_library_map(array( 'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php', 'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php', 'PhabricatorDiffInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php', + 'PhabricatorDiffInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php', 'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php', 'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php', 'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php', @@ -3594,6 +3595,7 @@ phutil_register_library_map(array( 'PhabricatorInlineComment' => 'infrastructure/diff/interface/PhabricatorInlineComment.php', 'PhabricatorInlineCommentAdjustmentEngine' => 'infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php', 'PhabricatorInlineCommentContentState' => 'infrastructure/diff/inline/PhabricatorInlineCommentContentState.php', + 'PhabricatorInlineCommentContext' => 'infrastructure/diff/inline/PhabricatorInlineCommentContext.php', 'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php', 'PhabricatorInlineCommentInterface' => 'applications/transactions/interface/PhabricatorInlineCommentInterface.php', 'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php', @@ -9630,6 +9632,7 @@ phutil_register_library_map(array( 'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', 'PhabricatorDiffInlineCommentContentState' => 'PhabricatorInlineCommentContentState', + 'PhabricatorDiffInlineCommentContext' => 'PhabricatorInlineCommentContext', 'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery', 'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', 'PhabricatorDiffScopeEngine' => 'Phobject', @@ -10122,6 +10125,7 @@ phutil_register_library_map(array( ), 'PhabricatorInlineCommentAdjustmentEngine' => 'Phobject', 'PhabricatorInlineCommentContentState' => 'Phobject', + 'PhabricatorInlineCommentContext' => 'Phobject', 'PhabricatorInlineCommentController' => 'PhabricatorController', 'PhabricatorInlineSummaryView' => 'AphrontView', 'PhabricatorInstructionsEditField' => 'PhabricatorEditField', diff --git a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php index 01f173bd9a..cb9d5e20eb 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransactionComment.php +++ b/src/applications/audit/storage/PhabricatorAuditTransactionComment.php @@ -17,6 +17,7 @@ final class PhabricatorAuditTransactionComment protected $attributes = array(); private $replyToComment = self::ATTACHABLE; + private $inlineContext = self::ATTACHABLE; public function getApplicationTransactionObject() { return new PhabricatorAuditTransaction(); @@ -83,12 +84,18 @@ final class PhabricatorAuditTransactionComment return $this; } - public function isEmptyInlineComment() { - return !strlen($this->getContent()); - } - public function newInlineCommentObject() { return PhabricatorAuditInlineComment::newFromModernComment($this); } + public function getInlineContext() { + return $this->assertAttached($this->inlineContext); + } + + public function attachInlineContext( + PhabricatorInlineCommentContext $context = null) { + $this->inlineContext = $context; + return $this; + } + } diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index df1ffa1d05..0e263d013c 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -200,6 +200,7 @@ final class DifferentialChangesetViewController extends DifferentialController { ->withPublishableComments(true) ->withPublishedComments(true) ->needHidden(true) + ->needInlineContext(true) ->execute(); $inlines = mpull($inlines, 'newInlineCommentObject'); diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index c5b8311630..cc90e36a61 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -328,6 +328,13 @@ final class DifferentialRevisionEditEngine $content = array(); if ($inlines) { + // Reload inlines to get inline context. + $inlines = id(new DifferentialDiffInlineCommentQuery()) + ->setViewer($viewer) + ->withIDs(mpull($inlines, 'getID')) + ->needInlineContext(true) + ->execute(); + $inline_preview = id(new PHUIDiffInlineCommentPreviewListView()) ->setViewer($viewer) ->setInlineComments($inlines); diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index f5cb772d48..491d89a968 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -18,6 +18,7 @@ final class DifferentialTransactionComment private $replyToComment = self::ATTACHABLE; private $isHidden = self::ATTACHABLE; private $changeset = self::ATTACHABLE; + private $inlineContext = self::ATTACHABLE; public function getApplicationTransactionObject() { return new DifferentialTransaction(); @@ -129,12 +130,18 @@ final class DifferentialTransactionComment return $this; } - public function isEmptyInlineComment() { - return !strlen($this->getContent()); - } - public function newInlineCommentObject() { return DifferentialInlineComment::newFromModernComment($this); } + public function getInlineContext() { + return $this->assertAttached($this->inlineContext); + } + + public function attachInlineContext( + PhabricatorInlineCommentContext $context = null) { + $this->inlineContext = $context; + return $this; + } + } diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index bf8aa8fae5..cab63eb998 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -373,6 +373,9 @@ final class DifferentialChangesetListView extends AphrontView { 'Add new inline comment on selected source text.' => pht('Add new inline comment on selected source text.'), + + 'Suggest Edit' => pht('Suggest Edit'), + 'Discard Edit' => pht('Discard Edit'), ), )); diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 83838e8385..e1b8dc7264 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -240,7 +240,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($edit_dialog); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, true); case 'cancel': $inline = $this->loadCommentByIDForEdit($this->getCommentID()); @@ -325,6 +325,9 @@ abstract class PhabricatorInlineCommentController $this->saveComment($inline); + // Reload the inline to attach context. + $inline = $this->loadCommentByIDForEdit($inline->getID()); + $edit_dialog = $this->buildEditDialog($inline); if ($this->getOperation() == 'reply') { @@ -335,7 +338,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($edit_dialog); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, true); } } @@ -431,7 +434,7 @@ abstract class PhabricatorInlineCommentController $view = $this->buildScaffoldForView($view); - return $this->newInlineResponse($inline, $view); + return $this->newInlineResponse($inline, $view, false); } private function buildScaffoldForView(PHUIDiffInlineCommentView $view) { @@ -446,11 +449,29 @@ abstract class PhabricatorInlineCommentController private function newInlineResponse( PhabricatorInlineComment $inline, - $view) { + $view, + $is_edit) { + + if ($inline->getReplyToCommentPHID()) { + $can_suggest = false; + } else { + $can_suggest = (bool)$inline->getInlineContext(); + } + + if ($is_edit) { + $viewer = $this->getViewer(); + $content_state = $inline->getContentStateForEdit($viewer); + } else { + $content_state = $inline->getContentState(); + } + + $state_map = $content_state->newStorageMap(); $response = array( 'inline' => array( 'id' => $inline->getID(), + 'contentState' => $state_map, + 'canSuggestEdit' => $can_suggest, ), 'view' => hsprintf('%s', $view), ); @@ -477,7 +498,8 @@ abstract class PhabricatorInlineCommentController $viewer = $this->getViewer(); $query = $this->newInlineCommentQuery() - ->withIDs(array($id)); + ->withIDs(array($id)) + ->needInlineContext(true); $inline = $this->loadCommentByQuery($query); diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php index 81a520b0cf..0f379acc3e 100644 --- a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php +++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContentState.php @@ -12,7 +12,7 @@ final class PhabricatorDiffInlineCommentContentState } if ($this->getContentHasSuggestion()) { - if (strlen($this->getSuggestionText())) { + if (strlen($this->getContentSuggestionText())) { return false; } } diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php new file mode 100644 index 0000000000..e46d934b5a --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php @@ -0,0 +1,37 @@ +headLines = $head_lines; + return $this; + } + + public function getHeadLines() { + return $this->headLines; + } + + public function setBodyLines(array $body_lines) { + $this->bodyLines = $body_lines; + return $this; + } + + public function getBodyLines() { + return $this->bodyLines; + } + + public function setTailLines(array $tail_lines) { + $this->tailLines = $tail_lines; + return $this; + } + + public function getTailLines() { + return $this->tailLines; + } + +} diff --git a/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php new file mode 100644 index 0000000000..a292dc72ee --- /dev/null +++ b/src/infrastructure/diff/inline/PhabricatorInlineCommentContext.php @@ -0,0 +1,4 @@ +getStorageObject()->getInlineContext(); + } + /* -( PhabricatorMarkupInterface Implementation )-------------------------- */ diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php index 4e846a0495..b5f39b1d2f 100644 --- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php +++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php @@ -9,6 +9,7 @@ abstract class PhabricatorDiffInlineCommentQuery private $publishableComments; private $needHidden; private $needAppliedDrafts; + private $needInlineContext; abstract protected function buildInlineCommentWhereClauseParts( AphrontDatabaseConnection $conn); @@ -42,6 +43,11 @@ abstract class PhabricatorDiffInlineCommentQuery return $this; } + final public function needInlineContext($need_context) { + $this->needInlineContext = $need_context; + return $this; + } + final public function needAppliedDrafts($need_applied) { $this->needAppliedDrafts = $need_applied; return $this; @@ -173,26 +179,6 @@ abstract class PhabricatorDiffInlineCommentQuery return $inlines; } - if ($this->needHidden) { - $viewer_phid = $viewer->getPHID(); - - if ($viewer_phid) { - $hidden = $this->loadHiddenCommentIDs( - $viewer_phid, - $inlines); - } else { - $hidden = array(); - } - - foreach ($inlines as $inline) { - $inline->attachIsHidden(isset($hidden[$inline->getID()])); - } - } - - if (!$inlines) { - return $inlines; - } - $need_drafts = $this->needAppliedDrafts; $drop_void = $this->publishableComments; $convert_objects = ($need_drafts || $drop_void); @@ -247,4 +233,133 @@ abstract class PhabricatorDiffInlineCommentQuery return $inlines; } + protected function didFilterPage(array $inlines) { + $viewer = $this->getViewer(); + + if ($this->needHidden) { + $viewer_phid = $viewer->getPHID(); + + if ($viewer_phid) { + $hidden = $this->loadHiddenCommentIDs( + $viewer_phid, + $inlines); + } else { + $hidden = array(); + } + + foreach ($inlines as $inline) { + $inline->attachIsHidden(isset($hidden[$inline->getID()])); + } + } + + if ($this->needInlineContext) { + $need_context = array(); + foreach ($inlines as $inline) { + $object = $inline->newInlineCommentObject(); + + if ($object->getDocumentEngineKey() !== null) { + $inline->attachInlineContext(null); + continue; + } + + $need_context[] = $inline; + } + + foreach ($need_context as $inline) { + $changeset = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs(array($inline->getChangesetID())) + ->needHunks(true) + ->executeOne(); + if (!$changeset) { + $inline->attachInlineContext(null); + continue; + } + + $hunks = $changeset->getHunks(); + + $is_simple = + (count($hunks) === 1) && + ((int)head($hunks)->getOldOffset() <= 1) && + ((int)head($hunks)->getNewOffset() <= 1); + + if (!$is_simple) { + $inline->attachInlineContext(null); + continue; + } + + if ($inline->getIsNewFile()) { + $corpus = $changeset->makeNewFile(); + } else { + $corpus = $changeset->makeOldFile(); + } + + $corpus = phutil_split_lines($corpus); + + // Adjust the line number into a 0-based offset. + $offset = $inline->getLineNumber(); + $offset = $offset - 1; + + // Adjust the inclusive range length into a row count. + $length = $inline->getLineLength(); + $length = $length + 1; + + $head_min = max(0, $offset - 3); + $head_max = $offset; + $head_len = $head_max - $head_min; + + if ($head_len) { + $head = array_slice($corpus, $head_min, $head_len, true); + $head = $this->simplifyContext($head, true); + } else { + $head = array(); + } + + $body = array_slice($corpus, $offset, $length, true); + + $tail = array_slice($corpus, $offset + $length, 3, true); + $tail = $this->simplifyContext($tail, false); + + $context = id(new PhabricatorDiffInlineCommentContext()) + ->setHeadLines($head) + ->setBodyLines($body) + ->setTailLines($tail); + + $inline->attachInlineContext($context); + } + + } + + return $inlines; + } + + private function simplifyContext(array $lines, $is_head) { + // We want to provide the smallest amount of context we can while still + // being useful, since the actual code is visible nearby and showing a + // ton of context is silly. + + // Examine each line until we find one that looks "useful" (not just + // whitespace or a single bracket). Once we find a useful piece of context + // to anchor the text, discard the rest of the lines beyond it. + + if ($is_head) { + $lines = array_reverse($lines, true); + } + + $saw_context = false; + foreach ($lines as $key => $line) { + if ($saw_context) { + unset($lines[$key]); + continue; + } + + $saw_context = (strlen(trim($line)) > 3); + } + + if ($is_head) { + $lines = array_reverse($lines, true); + } + + return $lines; + } } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index db2952faa8..b2a49f0624 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -427,6 +427,15 @@ final class PHUIDiffInlineCommentDetailView $metadata['menuItems'] = $menu_items; + $suggestion_content = $this->newSuggestionView($inline); + + $inline_content = phutil_tag( + 'div', + array( + 'class' => 'phabricator-remarkup', + ), + $content); + $markup = javelin_tag( 'div', array( @@ -445,9 +454,15 @@ final class PHUIDiffInlineCommentDetailView $group_left, $group_right, )), - phutil_tag_div( - 'differential-inline-comment-content', - phutil_tag_div('phabricator-remarkup', $content)), + phutil_tag( + 'div', + array( + 'class' => 'differential-inline-comment-content', + ), + array( + $suggestion_content, + $inline_content, + )), )); $summary = phutil_tag( @@ -491,4 +506,57 @@ final class PHUIDiffInlineCommentDetailView return true; } + private function newSuggestionView(PhabricatorInlineComment $inline) { + $content_state = $inline->getContentState(); + if (!$content_state->getContentHasSuggestion()) { + return null; + } + + $context = $inline->getInlineContext(); + if (!$context) { + return null; + } + + $head_lines = $context->getHeadLines(); + $head_lines = implode('', $head_lines); + + $tail_lines = $context->getTailLines(); + $tail_lines = implode('', $tail_lines); + + $old_lines = $context->getBodyLines(); + $old_lines = implode('', $old_lines); + $old_lines = $head_lines.$old_lines.$tail_lines; + if (strlen($old_lines) && !preg_match('/\n\z/', $old_lines)) { + $old_lines .= "\n"; + } + + $new_lines = $content_state->getContentSuggestionText(); + $new_lines = $head_lines.$new_lines.$tail_lines; + if (strlen($new_lines) && !preg_match('/\n\z/', $new_lines)) { + $new_lines .= "\n"; + } + + if ($old_lines === $new_lines) { + return null; + } + + + $raw_diff = id(new PhabricatorDifferenceEngine()) + ->generateRawDiffFromFileContent($old_lines, $new_lines); + + $raw_diff = phutil_split_lines($raw_diff); + $raw_diff = array_slice($raw_diff, 3); + $raw_diff = implode('', $raw_diff); + + $view = phutil_tag( + 'div', + array( + 'class' => 'inline-suggestion-view PhabricatorMonospaced', + ), + $raw_diff); + + return $view; + } + + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php index ce43653119..5c29c1666c 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php @@ -46,17 +46,23 @@ final class PHUIDiffInlineCommentEditView ), $this->title); + $corpus_view = $this->newCorpusView(); + $body = phutil_tag( 'div', array( 'class' => 'differential-inline-comment-edit-body', ), - $this->newTextarea()); + array( + $corpus_view, + $this->newTextarea(), + )); - $edit = phutil_tag( + $edit = javelin_tag( 'div', array( 'class' => 'differential-inline-comment-edit-buttons grouped', + 'sigil' => 'inline-edit-buttons', ), array( $buttons, @@ -91,4 +97,121 @@ final class PHUIDiffInlineCommentEditView ->setDisableFullScreen(true); } + private function newCorpusView() { + $viewer = $this->getViewer(); + $inline = $this->getInlineComment(); + + $context = $inline->getInlineContext(); + if ($context === null) { + return null; + } + + $head = $context->getHeadLines(); + $head = $this->newContextView($head); + + $state = $inline->getContentStateForEdit($viewer); + + $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; + + $textarea = javelin_tag( + 'textarea', + array( + 'class' => 'inline-suggestion-input PhabricatorMonospaced', + 'rows' => max(3, $main_count + 1), + 'sigil' => 'inline-content-suggestion', + 'meta' => array( + 'defaultText' => $default, + ), + ), + $main); + + $main = phutil_tag( + 'tr', + array( + 'class' => 'inline-suggestion-input-row', + ), + array( + phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-line-cell', + ), + null), + phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-input-cell', + ), + $textarea), + )); + + $tail = $context->getTailLines(); + $tail = $this->newContextView($tail); + + $body = phutil_tag( + 'tbody', + array(), + array( + $head, + $main, + $tail, + )); + + $table = phutil_tag( + 'table', + array( + 'class' => 'inline-suggestion-table', + ), + $body); + + $container = phutil_tag( + 'div', + array( + 'class' => 'inline-suggestion', + ), + $table); + + return $container; + } + + private function newContextView(array $lines) { + if (!$lines) { + return array(); + } + + $rows = array(); + foreach ($lines as $index => $line) { + $line_cell = phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-line-cell PhabricatorMonospaced', + ), + $index + 1); + + $text_cell = phutil_tag( + 'td', + array( + 'class' => 'inline-suggestion-text-cell PhabricatorMonospaced', + ), + $line); + + $cells = array( + $line_cell, + $text_cell, + ); + + $rows[] = phutil_tag('tr', array(), $cells); + } + + return $rows; + } + } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index 97fc9ee6e5..11be8d0ec1 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -93,7 +93,7 @@ abstract class PHUIDiffInlineCommentView extends AphrontView { 'startOffset' => $inline->getStartOffset(), 'endOffset' => $inline->getEndOffset(), 'on_right' => $this->getIsOnRight(), - 'contentState' => $inline->getContentState(), + 'contentState' => $inline->getContentState()->newStorageMap(), ); } diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css index ce5b87effd..9fefc65353 100644 --- a/webroot/rsrc/css/application/differential/phui-inline-comment.css +++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css @@ -436,3 +436,60 @@ background: {$lightyellow}; border-color: {$yellow}; } + +.inline-suggestion { + display: none; + margin: 0 -8px; +} + +.has-suggestion .inline-suggestion { + display: block; +} + +.differential-inline-comment-edit-buttons button.inline-button-left { + float: left; + margin: 0 6px 0 0; +} + +.inline-suggestion-table { + table-layout: fixed; + width: 100%; + margin-bottom: 8px; + white-space: pre-wrap; + background: {$greybackground}; + border-width: 1px 0; + border-style: solid; + border-color: {$lightgreyborder}; +} + +textarea.inline-suggestion-input { + width: 100%; + height: auto; + max-width: 100%; +} + +.inline-suggestion-line-cell { + text-align: right; + background: {$darkgreybackground}; + width: 36px; + color: {$greytext}; + border-right: 1px solid {$lightgreyborder}; +} + +.inline-suggestion-input-cell { + padding: 8px; +} + +.inline-suggestion-text-cell { + padding: 0 8px; +} + +.inline-suggestion-view { + padding: 8px 12px; + white-space: pre-wrap; + background: {$greybackground}; + margin: 0 -12px 8px; + border-width: 1px 0; + border-style: solid; + border-color: {$lightgreyborder}; +} diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 49f72101f4..e03138abfa 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -51,6 +51,7 @@ JX.install('DiffInline', { _startOffset: null, _endOffset: null, _isSelected: false, + _canSuggestEdit: false, bindToRow: function(row) { this._row = row; @@ -76,7 +77,6 @@ JX.install('DiffInline', { this._number = parseInt(data.number, 10); this._length = parseInt(data.length, 10); - this._originalState = data.contentState; this._isNewFile = data.isNewFile; this._replyToCommentPHID = data.replyToCommentPHID; @@ -602,6 +602,8 @@ JX.install('DiffInline', { _readInlineState: function(state) { this._id = state.id; + this._originalState = state.contentState; + this._canSuggestEdit = state.canSuggestEdit; }, _ondeleteresponse: function() { @@ -664,6 +666,11 @@ JX.install('DiffInline', { _drawEditRows: function(rows) { this.setEditing(true); this._editRow = this._drawRows(rows, null, 'edit'); + + this._drawSuggestionState(this._editRow); + JX.log(this._originalState); + + this.setHasSuggestion(this._originalState.hasSuggestion); }, _drawRows: function(rows, cursor, type) { @@ -719,6 +726,91 @@ JX.install('DiffInline', { return result_row; }, + _drawSuggestionState: function(row) { + if (this._canSuggestEdit) { + var button = this._getSuggestionButton(); + var node = button.getNode(); + + // As a side effect of form submission, the button may become + // visually disabled. Re-enable it. This is a bit hacky. + JX.DOM.alterClass(node, 'disabled', false); + node.disabled = false; + + var container = JX.DOM.find(row, 'div', 'inline-edit-buttons'); + container.appendChild(node); + } + }, + + _getSuggestionButton: function() { + if (!this._suggestionButton) { + var button = new JX.PHUIXButtonView() + .setIcon('fa-pencil-square-o') + .setColor('grey'); + + var node = button.getNode(); + JX.DOM.alterClass(node, 'inline-button-left', true); + + var onclick = JX.bind(this, this._onSuggestEdit); + JX.DOM.listen(node, 'click', null, onclick); + + this._suggestionButton = button; + } + + return this._suggestionButton; + }, + + _onSuggestEdit: function(e) { + e.kill(); + + 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. + 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); + } + } + } + } + } + + // Save the "hasSuggestion" part of the content state. + this.triggerDraft(); + }, + + setHasSuggestion: function(has_suggestion) { + this._hasSuggestion = has_suggestion; + + var button = this._getSuggestionButton(); + var pht = this.getChangeset().getChangesetList().getTranslations(); + if (has_suggestion) { + button + .setIcon('fa-times') + .setText(pht('Discard Edit')); + } else { + button + .setIcon('fa-plus') + .setText(pht('Suggest Edit')); + } + + if (this._editRow) { + JX.DOM.alterClass(this._editRow, 'has-suggestion', has_suggestion); + } + }, + + getHasSuggestion: function() { + return this._hasSuggestion; + }, + save: function() { var handler = JX.bind(this, this._onsubmitresponse); @@ -825,16 +917,24 @@ JX.install('DiffInline', { // Ignore. } - try { - node = JX.DOM.find(row, 'textarea', 'inline-content-suggestion'); + node = this._getSuggestionNode(row); + if (node) { state.suggestionText = node.value; - } catch (ex) { - // Ignore. } + state.hasSuggestion = this.getHasSuggestion(); + return state; }, + _getSuggestionNode: function(row) { + try { + return JX.DOM.find(row, 'textarea', 'inline-content-suggestion'); + } catch (ex) { + return null; + } + }, + _onsubmitresponse: function(response) { if (this._editRow) { JX.DOM.remove(this._editRow); @@ -1063,7 +1163,7 @@ JX.install('DiffInline', { return { text: '', suggestionText: '', - hasSuggestion: true + hasSuggestion: false }; }, @@ -1073,6 +1173,7 @@ JX.install('DiffInline', { _isSameContentState: function(u, v) { return ( + ((u === null) === (v === null)) && (u.text === v.text) && (u.suggestionText === v.suggestionText) && (u.hasSuggestion === v.hasSuggestion));