From 10f241352dcef12fd599a8b6c0f32c704d9f1f9a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 May 2020 09:01:23 -0700 Subject: [PATCH] Render inline comment suggestions as real diffs Summary: Ref T13513. When rendering an inline suggestion for display, use highlighting and diffing. Test Plan: {F7495053} Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21277 --- resources/celerity/map.php | 6 +-- .../parser/DifferentialChangesetParser.php | 5 ++- .../DifferentialChangesetOneUpRenderer.php | 39 ++++++++++++++++++- .../PhabricatorDiffInlineCommentContext.php | 10 +++++ .../PhabricatorDiffInlineCommentQuery.php | 5 +++ .../view/PHUIDiffInlineCommentDetailView.php | 27 ++++++++++--- .../differential/phui-inline-comment.css | 24 +++++++++--- 7 files changed, 98 insertions(+), 18 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 434ca6e869..c1b05eae86 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'ba768cdb', 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', - 'differential.pkg.css' => 'f924dbcf', + 'differential.pkg.css' => '5c459f92', 'differential.pkg.js' => '256a327a', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', @@ -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' => '4107254a', + 'rsrc/css/application/differential/phui-inline-comment.css' => '9863a85e', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', 'rsrc/css/application/differential/revision-list.css' => '93d2df7d', @@ -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' => '4107254a', + 'phui-inline-comment-view-css' => '9863a85e', 'phui-invisible-character-view-css' => 'c694c4a4', 'phui-left-right-css' => '68513c34', 'phui-lightbox-css' => '4ebf22da', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index d9ebad0663..35cdedbcc6 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -834,7 +834,6 @@ final class DifferentialChangesetParser extends Phobject { ->setNewAttachesToNewFile($this->rightSideAttachesToNewFile) ->setCodeCoverage($this->getCoverage()) ->setRenderingReference($this->getRenderingReference()) - ->setMarkupEngine($this->markupEngine) ->setHandles($this->handles) ->setOldLines($this->old) ->setNewLines($this->new) @@ -845,6 +844,10 @@ final class DifferentialChangesetParser extends Phobject { ->setHighlightingDisabled($this->highlightingDisabled) ->setDepthOnlyLines($this->getDepthOnlyLines()); + if ($this->markupEngine) { + $renderer->setMarkupEngine($this->markupEngine); + } + list($engine, $old_ref, $new_ref) = $this->newDocumentEngine(); if ($engine) { $engine_blocks = $engine->newEngineBlocks( diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index f4ed05545b..186924e359 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -3,6 +3,17 @@ final class DifferentialChangesetOneUpRenderer extends DifferentialChangesetHTMLRenderer { + private $simpleMode; + + public function setSimpleMode($simple_mode) { + $this->simpleMode = $simple_mode; + return $this; + } + + public function getSimpleMode() { + return $this->simpleMode; + } + public function isOneUpRenderer() { return true; } @@ -36,6 +47,8 @@ final class DifferentialChangesetOneUpRenderer protected function renderPrimitives(array $primitives, $rows) { list($left_prefix, $right_prefix) = $this->getLineIDPrefixes(); + $is_simple = $this->getSimpleMode(); + $no_copy = phutil_tag('td', array('class' => 'copy')); $no_coverage = null; @@ -185,6 +198,12 @@ final class DifferentialChangesetOneUpRenderer $cells[] = $no_coverage; } + // In simple mode, only render the text. This is used to render + // "Edit Suggestions" in inline comments. + if ($is_simple) { + $cells = array($cells[3]); + } + $out[] = phutil_tag('tr', array(), $cells); break; @@ -231,11 +250,17 @@ final class DifferentialChangesetOneUpRenderer } } + $result = null; + if ($out) { - return $this->wrapChangeInTable(phutil_implode_html('', $out)); + if ($is_simple) { + $result = $this->newSimpleTable($out); + } else { + $result = $this->wrapChangeInTable(phutil_implode_html('', $out)); + } } - return null; + return $result; } public function renderDocumentEngineBlocks( @@ -488,4 +513,14 @@ final class DifferentialChangesetOneUpRenderer ->addInlineView($view); } + + private function newSimpleTable($content) { + return phutil_tag( + 'table', + array( + 'class' => 'diff-1up-simple-table', + ), + $content); + } + } diff --git a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php index e46d934b5a..95f8473caf 100644 --- a/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php +++ b/src/infrastructure/diff/inline/PhabricatorDiffInlineCommentContext.php @@ -3,10 +3,20 @@ final class PhabricatorDiffInlineCommentContext extends PhabricatorInlineCommentContext { + private $filename; private $headLines; private $bodyLines; private $tailLines; + public function setFilename($filename) { + $this->filename = $filename; + return $this; + } + + public function getFilename() { + return $this->filename; + } + public function setHeadLines(array $head_lines) { $this->headLines = $head_lines; return $this; diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php index b5f39b1d2f..48a100f607 100644 --- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php +++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php @@ -289,8 +289,12 @@ abstract class PhabricatorDiffInlineCommentQuery } if ($inline->getIsNewFile()) { + $vector = $changeset->getNewStatePathVector(); + $filename = last($vector); $corpus = $changeset->makeNewFile(); } else { + $vector = $changeset->getOldStatePathVector(); + $filename = last($vector); $corpus = $changeset->makeOldFile(); } @@ -321,6 +325,7 @@ abstract class PhabricatorDiffInlineCommentQuery $tail = $this->simplifyContext($tail, false); $context = id(new PhabricatorDiffInlineCommentContext()) + ->setFilename($filename) ->setHeadLines($head) ->setBodyLines($body) ->setTailLines($tail); diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index b2a49f0624..ddf0dddf15 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -540,20 +540,35 @@ final class PHUIDiffInlineCommentDetailView return null; } + $viewer = $this->getViewer(); - $raw_diff = id(new PhabricatorDifferenceEngine()) - ->generateRawDiffFromFileContent($old_lines, $new_lines); + $changeset = id(new PhabricatorDifferenceEngine()) + ->generateChangesetFromFileContent($old_lines, $new_lines); - $raw_diff = phutil_split_lines($raw_diff); - $raw_diff = array_slice($raw_diff, 3); - $raw_diff = implode('', $raw_diff); + $changeset->setFilename($context->getFilename()); + + // TODO: This isn't cached! + + $viewstate = new PhabricatorChangesetViewState(); + + $parser = id(new DifferentialChangesetParser()) + ->setViewer($viewer) + ->setViewstate($viewstate) + ->setChangeset($changeset); + + $renderer = new DifferentialChangesetOneUpRenderer(); + $renderer->setSimpleMode(true); + + $parser->setRenderer($renderer); + + $diff_view = $parser->render(0, 0xFFFF, array()); $view = phutil_tag( 'div', array( 'class' => 'inline-suggestion-view PhabricatorMonospaced', ), - $raw_diff); + $diff_view); return $view; } diff --git a/webroot/rsrc/css/application/differential/phui-inline-comment.css b/webroot/rsrc/css/application/differential/phui-inline-comment.css index 9fefc65353..246c81013c 100644 --- a/webroot/rsrc/css/application/differential/phui-inline-comment.css +++ b/webroot/rsrc/css/application/differential/phui-inline-comment.css @@ -476,20 +476,32 @@ textarea.inline-suggestion-input { border-right: 1px solid {$lightgreyborder}; } -.inline-suggestion-input-cell { - padding: 8px; +.inline-suggestion-table td.inline-suggestion-input-cell { + padding: 8px 4px; } -.inline-suggestion-text-cell { - padding: 0 8px; +.inline-suggestion-table td.inline-suggestion-text-cell { + /* This is attempting to align the text in the textarea with the text on + the surrounding context lines. */ + padding: 0 8px 0 11px; } .inline-suggestion-view { - padding: 8px 12px; + padding: 4px 0; white-space: pre-wrap; - background: {$greybackground}; + background: {$lightgreybackground}; margin: 0 -12px 8px; border-width: 1px 0; border-style: solid; border-color: {$lightgreyborder}; } + +.diff-1up-simple-table { + width: 100%; + table-layout: fixed; +} + +.diff-1up-simple-table > tbody > tr > td { + padding-left: 12px; + padding-right: 12px; +}