Commit 84656215 authored by epriestley's avatar epriestley
Browse files

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
parent 00430fdb
......@@ -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',
......
......@@ -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',
......@@ -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;
}
}
......@@ -200,6 +200,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
->withPublishableComments(true)
->withPublishedComments(true)
->needHidden(true)
->needInlineContext(true)
->execute();
$inlines = mpull($inlines, 'newInlineCommentObject');
......
......@@ -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);
......
......@@ -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;
}
}
......@@ -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'),
),
));
......
......@@ -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);
......
......@@ -12,7 +12,7 @@ final class PhabricatorDiffInlineCommentContentState
}
if ($this->getContentHasSuggestion()) {
if (strlen($this->getSuggestionText())) {
if (strlen($this->getContentSuggestionText())) {
return false;
}
}
......
<?php
final class PhabricatorDiffInlineCommentContext
extends PhabricatorInlineCommentContext {
private $headLines;
private $bodyLines;
private $tailLines;
public function setHeadLines(array $head_lines) {
$this->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;
}
}
<?php
abstract class PhabricatorInlineCommentContext
extends Phobject {}
......@@ -350,6 +350,10 @@ abstract class PhabricatorInlineComment
return $this;
}
public function getInlineContext() {
return $this->getStorageObject()->getInlineContext();
}
/* -( PhabricatorMarkupInterface Implementation )-------------------------- */
......
......@@ -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;
}
}
......@@ -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);