From d2d7e7f5ff1b473f92137c9954dd14dcbd44f25a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 May 2020 09:18:18 -0700 Subject: [PATCH] Clean up Diffusion behaviors for inline edit suggestions Summary: Ref T13513. For now, I'm not supporting inline edit suggestions in Diffusion, although it's likely not difficult to do so in the future. Clean up some of the code so that plain ol' inlines work the same way they did before. Test Plan: - Created, edited, reloaded, submitted inlines in Diffusion: familiar old behavior. - Created, edited, reloaded, submitted inlines with suggestions in Differential: familiar new behavior. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21278 --- resources/celerity/map.php | 12 +-- .../DifferentialDiffInlineCommentQuery.php | 74 +++++++++++++++++++ .../query/DiffusionDiffInlineCommentQuery.php | 4 + .../PhabricatorDiffInlineCommentQuery.php | 74 ++----------------- .../rsrc/js/application/diff/DiffInline.js | 2 - 5 files changed, 92 insertions(+), 74 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c1b05eae86..6dd33c64bf 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', - 'differential.pkg.js' => '256a327a', + 'differential.pkg.js' => '24616785', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -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' => '829b88bf', + 'rsrc/js/application/diff/DiffInline.js' => '008b6a15', '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' => '829b88bf', + 'phabricator-diff-inline' => '008b6a15', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -917,6 +917,9 @@ return array( 'unhandled-exception-css' => '9ecfc00d', ), 'requires' => array( + '008b6a15' => array( + 'javelin-dom', + ), '0116d3e8' => array( 'javelin-behavior', 'javelin-dom', @@ -1639,9 +1642,6 @@ return array( '8207abf9' => array( 'javelin-dom', ), - '829b88bf' => array( - 'javelin-dom', - ), 83754533 => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php index b40589bb36..b8392c474d 100644 --- a/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php +++ b/src/applications/differential/query/DifferentialDiffInlineCommentQuery.php @@ -67,4 +67,78 @@ final class DifferentialDiffInlineCommentQuery return $id_map; } + protected function newInlineContextMap(array $inlines) { + $viewer = $this->getViewer(); + + $map = array(); + + foreach ($inlines as $key => $inline) { + $changeset = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs(array($inline->getChangesetID())) + ->needHunks(true) + ->executeOne(); + if (!$changeset) { + continue; + } + + $hunks = $changeset->getHunks(); + + $is_simple = + (count($hunks) === 1) && + ((int)head($hunks)->getOldOffset() <= 1) && + ((int)head($hunks)->getNewOffset() <= 1); + + if (!$is_simple) { + continue; + } + + if ($inline->getIsNewFile()) { + $vector = $changeset->getNewStatePathVector(); + $filename = last($vector); + $corpus = $changeset->makeNewFile(); + } else { + $vector = $changeset->getOldStatePathVector(); + $filename = last($vector); + $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()) + ->setFilename($filename) + ->setHeadLines($head) + ->setBodyLines($body) + ->setTailLines($tail); + + $map[$key] = $context; + } + + return $map; + } + } diff --git a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php index af116a4097..c395231808 100644 --- a/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php +++ b/src/applications/diffusion/query/DiffusionDiffInlineCommentQuery.php @@ -66,4 +66,8 @@ final class DiffusionDiffInlineCommentQuery return array(); } + protected function newInlineContextMap(array $inlines) { + return array(); + } + } diff --git a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php index 48a100f607..85987c89a5 100644 --- a/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php +++ b/src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php @@ -18,6 +18,8 @@ abstract class PhabricatorDiffInlineCommentQuery $viewer_phid, array $comments); + abstract protected function newInlineContextMap(array $inlines); + final public function withFixedStates(array $states) { $this->fixedStates = $states; return $this; @@ -265,72 +267,12 @@ abstract class PhabricatorDiffInlineCommentQuery $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; + if ($need_context) { + $context_map = $this->newInlineContextMap($need_context); + + foreach ($need_context as $key => $inline) { + $inline->attachInlineContext(idx($context_map, $key)); } - - $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()) { - $vector = $changeset->getNewStatePathVector(); - $filename = last($vector); - $corpus = $changeset->makeNewFile(); - } else { - $vector = $changeset->getOldStatePathVector(); - $filename = last($vector); - $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()) - ->setFilename($filename) - ->setHeadLines($head) - ->setBodyLines($body) - ->setTailLines($tail); - - $inline->attachInlineContext($context); } } @@ -338,7 +280,7 @@ abstract class PhabricatorDiffInlineCommentQuery return $inlines; } - private function simplifyContext(array $lines, $is_head) { + final protected 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. diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index e03138abfa..1e700855f1 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -668,8 +668,6 @@ JX.install('DiffInline', { this._editRow = this._drawRows(rows, null, 'edit'); this._drawSuggestionState(this._editRow); - JX.log(this._originalState); - this.setHasSuggestion(this._originalState.hasSuggestion); },