From 9f11f310f87209e50c48a852312cd442d0c373b4 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 25 Sep 2017 19:56:22 +0000 Subject: [PATCH 1/9] Make PHUITwoColumnView a little more printable Summary: Hide navbar, and make curtain behave like on a phone, when printing. Test Plan: {F5197340} Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D18583 --- resources/celerity/map.php | 18 ++++++------ webroot/rsrc/css/core/core.css | 5 ++++ webroot/rsrc/css/phui/phui-action-list.css | 5 ++++ webroot/rsrc/css/phui/phui-curtain-view.css | 28 +++++++++++++++++++ .../rsrc/css/phui/phui-two-column-view.css | 28 +++++++++++++++++++ 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 552fe22a68..6c7d2db90c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', - 'core.pkg.css' => 'e9473020', + 'core.pkg.css' => '87a9a59b', 'core.pkg.js' => '28552e58', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -114,7 +114,7 @@ return array( 'rsrc/css/application/slowvote/slowvote.css' => 'a94b7230', 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', - 'rsrc/css/core/core.css' => '1760853c', + 'rsrc/css/core/core.css' => '62fa3ace', 'rsrc/css/core/remarkup.css' => 'cad18339', 'rsrc/css/core/syntax.css' => 'cae95e89', 'rsrc/css/core/z-index.css' => '9d8f7c4b', @@ -137,7 +137,7 @@ return array( 'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '9d9685d6', 'rsrc/css/phui/object-item/phui-oi-list-view.css' => 'bf094950', 'rsrc/css/phui/object-item/phui-oi-simple-ui.css' => 'a8beebea', - 'rsrc/css/phui/phui-action-list.css' => 'e7eba156', + 'rsrc/css/phui/phui-action-list.css' => 'f7f61a34', 'rsrc/css/phui/phui-action-panel.css' => 'b4798122', 'rsrc/css/phui/phui-badge.css' => '22c0cf4f', 'rsrc/css/phui/phui-basic-nav-view.css' => '98c11ab3', @@ -148,7 +148,7 @@ return array( 'rsrc/css/phui/phui-comment-form.css' => 'ac68149f', 'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad', 'rsrc/css/phui/phui-crumbs-view.css' => '6ece3bbb', - 'rsrc/css/phui/phui-curtain-view.css' => 'ca363f15', + 'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026', 'rsrc/css/phui/phui-document-pro.css' => '8af7ea27', 'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf', 'rsrc/css/phui/phui-document.css' => 'c32e8dec', @@ -177,7 +177,7 @@ return array( 'rsrc/css/phui/phui-status.css' => 'd5263e49', 'rsrc/css/phui/phui-tag-view.css' => 'b4719c50', 'rsrc/css/phui/phui-timeline-view.css' => 'f21db7ca', - 'rsrc/css/phui/phui-two-column-view.css' => '1ade9c5f', + 'rsrc/css/phui/phui-two-column-view.css' => '44ec4951', 'rsrc/css/phui/workboards/phui-workboard-color.css' => '783cdff5', 'rsrc/css/phui/workboards/phui-workboard.css' => '3bc85455', 'rsrc/css/phui/workboards/phui-workcard.css' => 'cca5fa92', @@ -765,11 +765,11 @@ return array( 'path-typeahead' => 'f7fc67ec', 'people-picture-menu-item-css' => 'a06f7f34', 'people-profile-css' => '4df76faf', - 'phabricator-action-list-view-css' => 'e7eba156', + 'phabricator-action-list-view-css' => 'f7f61a34', 'phabricator-busy' => '59a7976a', 'phabricator-chatlog-css' => 'd295b020', 'phabricator-content-source-view-css' => '4b8b05d4', - 'phabricator-core-css' => '1760853c', + 'phabricator-core-css' => '62fa3ace', 'phabricator-countdown-css' => '16c52f5c', 'phabricator-darklog' => 'c8e1ffe3', 'phabricator-darkmessage' => 'c48cccdd', @@ -833,7 +833,7 @@ return array( 'phui-comment-form-css' => 'ac68149f', 'phui-comment-panel-css' => 'f50152ad', 'phui-crumbs-view-css' => '6ece3bbb', - 'phui-curtain-view-css' => 'ca363f15', + 'phui-curtain-view-css' => '2bdaf026', 'phui-document-summary-view-css' => '9ca48bdf', 'phui-document-view-css' => 'c32e8dec', 'phui-document-view-pro-css' => '8af7ea27', @@ -872,7 +872,7 @@ return array( 'phui-tag-view-css' => 'b4719c50', 'phui-theme-css' => '9f261c6b', 'phui-timeline-view-css' => 'f21db7ca', - 'phui-two-column-view-css' => '1ade9c5f', + 'phui-two-column-view-css' => '44ec4951', 'phui-workboard-color-css' => '783cdff5', 'phui-workboard-view-css' => '3bc85455', 'phui-workcard-view-css' => 'cca5fa92', diff --git a/webroot/rsrc/css/core/core.css b/webroot/rsrc/css/core/core.css index 9d471477ed..cc14d7ab5a 100644 --- a/webroot/rsrc/css/core/core.css +++ b/webroot/rsrc/css/core/core.css @@ -61,6 +61,11 @@ body { overflow-y: scroll; } +body.printable, +!print body { + background: none; +} + textarea { font: inherit; } diff --git a/webroot/rsrc/css/phui/phui-action-list.css b/webroot/rsrc/css/phui/phui-action-list.css index a7ad662d33..5e32a1ea0a 100644 --- a/webroot/rsrc/css/phui/phui-action-list.css +++ b/webroot/rsrc/css/phui/phui-action-list.css @@ -7,6 +7,11 @@ display: none; } +!print .phabricator-action-list-view { + padding: 4px 0; + display: none; +} + .device .phuix-dropdown-menu .phabricator-action-list-view { /* When an action list view appears inside a dropdown menu, don't hide it by default. */ diff --git a/webroot/rsrc/css/phui/phui-curtain-view.css b/webroot/rsrc/css/phui/phui-curtain-view.css index 8bd8a331c4..3facf0fe37 100644 --- a/webroot/rsrc/css/phui/phui-curtain-view.css +++ b/webroot/rsrc/css/phui/phui-curtain-view.css @@ -21,6 +21,26 @@ border-top: 1px solid {$greybackground}; } +!print .phui-curtain-panel + .phui-curtain-panel { + padding: 8px 0; + border-top: none; +} + +!print .device-desktop .phabricator-action-list-view + .phui-curtain-panel { + padding: 8px 0; + border-top: none; +} + +!print .device-desktop .phui-curtain-panel + .phui-curtain-panel { + padding: 8px 0; + border-top: none; +} + +!print .phabricator-action-list-view + .phui-curtain-panel { + padding: 8px 0; + border-top: none; +} + .phui-curtain-panel-header { padding: 0 0 4px; color: {$bluetext}; @@ -37,6 +57,10 @@ padding: 0; } +!print .phui-curtain-panel-body { + padding: 0; +} + /* Project tags */ .phui-curtain-panel-body .phabricator-handle-tag-list-item { @@ -50,3 +74,7 @@ .device .curtain-no-panels { display: none; } + +!print .curtain-no-panels { + display: none; +} diff --git a/webroot/rsrc/css/phui/phui-two-column-view.css b/webroot/rsrc/css/phui/phui-two-column-view.css index 2fb0eccee2..f9f86d950a 100644 --- a/webroot/rsrc/css/phui/phui-two-column-view.css +++ b/webroot/rsrc/css/phui/phui-two-column-view.css @@ -79,6 +79,30 @@ width: 300px; } +!print .phui-two-column-view .phui-main-column { + float: unset; + width: unset; + margin-bottom: 20px; +} + +!print .phui-two-column-view .phui-side-column { + float: unset; + width: unset; + margin-bottom: 20px; +} + +!print .device-desktop .phui-two-column-view .phui-main-column { + float: unset; + width: unset; + margin-bottom: 20px; +} + +!print .device-desktop .phui-two-column-view .phui-side-column { + float: unset; + width: unset; + margin-bottom: 20px; +} + .device-desktop .phui-two-column-view.phui-side-column-left .phui-main-column { float: right; width: calc(100% - 280px); @@ -307,3 +331,7 @@ .phui-mobile-menu { display: block; } + +!print .phabricator-side-menu { + display: none !important; +} From a1d9a2389db4a8c54d90560695993f4e38398593 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Sep 2017 19:11:35 -0700 Subject: [PATCH 2/9] Improve Ferret engine indexing performance for large blocks of text Summary: See PHI87. Ref T12974. Currently, we do a lot more work here than we need to: we call `phutil_utf8_strtolower()` on each token, but can do it once at the beginning on the whole block. Additionally, since ngrams don't care about order, we only need to convert unique tokens into ngrams. This saves us some `phutil_utf8v()`. These calls can be slow for large inputs. Test Plan: - Created a ~4MB task description. - Ran `bin/search index Txxx --profile ...` to profile indexing performance before and after the change. - Saw total runtime drop form 38s to 9s. - Before: - After: Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12974 Differential Revision: https://secure.phabricator.com/D18647 --- .../search/ferret/PhabricatorFerretEngine.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/applications/search/ferret/PhabricatorFerretEngine.php b/src/applications/search/ferret/PhabricatorFerretEngine.php index 3c8098c54f..7d1d03a8b1 100644 --- a/src/applications/search/ferret/PhabricatorFerretEngine.php +++ b/src/applications/search/ferret/PhabricatorFerretEngine.php @@ -88,16 +88,23 @@ abstract class PhabricatorFerretEngine extends Phobject { } private function getNgramsFromString($value, $as_term) { + $value = phutil_utf8_strtolower($value); $tokens = $this->tokenizeString($value); - $ngrams = array(); + // First, extract unique tokens from the string. This reduces the number + // of `phutil_utf8v()` calls we need to make if we are indexing a large + // corpus with redundant terms. + $unique_tokens = array(); foreach ($tokens as $token) { - $token = phutil_utf8_strtolower($token); - if ($as_term) { $token = ' '.$token.' '; } + $unique_tokens[$token] = true; + } + + $ngrams = array(); + foreach ($unique_tokens as $token => $ignored) { $token_v = phutil_utf8v($token); $len = (count($token_v) - 2); for ($ii = 0; $ii < $len; $ii++) { From 086a125ad5ee5d3de3d5b6bc718975387a2b211b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Sep 2017 09:16:42 -0700 Subject: [PATCH 3/9] Improve performance of Ferret engine ngram extraction, particularly for large input strings Summary: See PHI87. Ref T12974. The `array_slice()` method of splitting the string apart can perform poorly for large input strings. I think this is mostly just the large number of calls plus building and returning an array being not entirely trivial. We can just use `substr()` instead, as long as we're a little bit careful about keeping track of where we're slicing the string if it has UTF8 characters. Test Plan: - Created a task with a single, unbroken blob of base64 encoded data as the description, roughly 100KB long. - Saw indexing performance improve from ~6s to ~1.5s after patch. - Before: https://secure.phabricator.com/xhprof/profile/PHID-FILE-nrxs4lwdvupbve5lhl6u/ - After: https://secure.phabricator.com/xhprof/profile/PHID-FILE-6vs2akgjj5nbqt7yo7ul/ Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12974 Differential Revision: https://secure.phabricator.com/D18649 --- .../search/ferret/PhabricatorFerretEngine.php | 22 +++++++++++--- .../PhabricatorFerretEngineTestCase.php | 30 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/applications/search/ferret/PhabricatorFerretEngine.php b/src/applications/search/ferret/PhabricatorFerretEngine.php index 7d1d03a8b1..7cd06ae549 100644 --- a/src/applications/search/ferret/PhabricatorFerretEngine.php +++ b/src/applications/search/ferret/PhabricatorFerretEngine.php @@ -106,11 +106,25 @@ abstract class PhabricatorFerretEngine extends Phobject { $ngrams = array(); foreach ($unique_tokens as $token => $ignored) { $token_v = phutil_utf8v($token); - $len = (count($token_v) - 2); - for ($ii = 0; $ii < $len; $ii++) { - $ngram = array_slice($token_v, $ii, 3); - $ngram = implode('', $ngram); + $length = count($token_v); + + // NOTE: We're being somewhat clever here to micro-optimize performance, + // especially for very long strings. See PHI87. + + $token_l = array(); + for ($ii = 0; $ii < $length; $ii++) { + $token_l[$ii] = strlen($token_v[$ii]); + } + + $ngram_count = $length - 2; + $cursor = 0; + for ($ii = 0; $ii < $ngram_count; $ii++) { + $ngram_l = $token_l[$ii] + $token_l[$ii + 1] + $token_l[$ii + 2]; + + $ngram = substr($token, $cursor, $ngram_l); $ngrams[$ngram] = $ngram; + + $cursor += $token_l[$ii]; } } diff --git a/src/applications/search/ferret/__tests__/PhabricatorFerretEngineTestCase.php b/src/applications/search/ferret/__tests__/PhabricatorFerretEngineTestCase.php index a8690f1d8b..ea65a559ff 100644 --- a/src/applications/search/ferret/__tests__/PhabricatorFerretEngineTestCase.php +++ b/src/applications/search/ferret/__tests__/PhabricatorFerretEngineTestCase.php @@ -24,4 +24,34 @@ final class PhabricatorFerretEngineTestCase } } + public function testTermNgramExtraction() { + $snowman = "\xE2\x98\x83"; + + $map = array( + 'a' => array(' a '), + 'ab' => array(' ab', 'ab '), + 'abcdef' => array(' ab', 'abc', 'bcd', 'cde', 'def', 'ef '), + "{$snowman}" => array(" {$snowman} "), + "x{$snowman}y" => array( + " x{$snowman}", + "x{$snowman}y", + "{$snowman}y ", + ), + "{$snowman}{$snowman}" => array( + " {$snowman}{$snowman}", + "{$snowman}{$snowman} ", + ), + ); + + $engine = new ManiphestTaskFerretEngine(); + + foreach ($map as $input => $expect) { + $actual = $engine->getTermNgramsFromString($input); + $this->assertEqual( + $actual, + $expect, + pht('Term ngrams for: %s.', $input)); + } + } + } From 9875af739fb5d522817c849e20a9f13fec3b798e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Sep 2017 11:37:10 -0700 Subject: [PATCH 4/9] When we purge the request cache, also force PHP to collect cycles Summary: Ref T12997. See that task for more details. Briefly, an unusual dataset (where commits are mentioned hundreds of times by other commits) is causing some weird memory behavior in the daemons. Forcing PHP to GC cycles explicitly after each task completes seems to help with this, by cleaning up some of the memory between tasks. A more thorough fix might be to untangle the `$xactions` structure, but that's significantly more involved. Test Plan: - Did this locally in a controlled environment, saw an immediate collection of a 500MB `$xactions` cycle. - Put a similar change in production, memory usage seemed less to improve. It's hard to tell for sure that this does anything, but it shouldn't hurt. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12997 Differential Revision: https://secure.phabricator.com/D18657 --- src/applications/cache/PhabricatorCaches.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/cache/PhabricatorCaches.php b/src/applications/cache/PhabricatorCaches.php index 1314aa4ccc..9c22bde672 100644 --- a/src/applications/cache/PhabricatorCaches.php +++ b/src/applications/cache/PhabricatorCaches.php @@ -51,6 +51,12 @@ final class PhabricatorCaches extends Phobject { */ public static function destroyRequestCache() { self::$requestCache = null; + + // See T12997. Force the GC to run when the request cache is destroyed to + // clean up any cycles which may still be hanging around. + if (function_exists('gc_collect_cycles')) { + gc_collect_cycles(); + } } From b75a4151c8996c5dfb1c8c14378fe3259666eac2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Sep 2017 12:48:19 -0700 Subject: [PATCH 5/9] Allow the fulltext index to select only transactions with comments Summary: Ref T12997. Although we can't query by transaction type (since we can't easily enumerate all possible types which may have comments -- inline types may also have comments), we //can// just check if there's a comment row or not. This reduces the amount of garbage we need to load to rebuild indexes for unusual objects with hundreds and hundreds of mentions. Test Plan: - Used batch editor to mention a task 700 times. - Indexed it before and after this change, saw index time drop from {nav 1600ms > 160ms}. - Made some new comments on it, verified that they still indexed/queried properly. - Browsed around, made normal transactions, made inline comments. - Added a unique word to an inline comment, indexed revision, searched for word, found revision. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12997 Differential Revision: https://secure.phabricator.com/D18660 --- ...torTransactionsFulltextEngineExtension.php | 1 + ...PhabricatorApplicationTransactionQuery.php | 95 ++++++++++++------- 2 files changed, 63 insertions(+), 33 deletions(-) diff --git a/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php index 701ad34ca7..c9f43f49b2 100644 --- a/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php @@ -25,6 +25,7 @@ final class PhabricatorTransactionsFulltextEngineExtension $xactions = $query ->setViewer($this->getViewer()) ->withObjectPHIDs(array($object->getPHID())) + ->withComments(true) ->needComments(true) ->execute(); diff --git a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php index da8454c7fc..abe1498fc0 100644 --- a/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php +++ b/src/applications/transactions/query/PhabricatorApplicationTransactionQuery.php @@ -7,6 +7,7 @@ abstract class PhabricatorApplicationTransactionQuery private $objectPHIDs; private $authorPHIDs; private $transactionTypes; + private $withComments; private $needComments = true; private $needHandles = true; @@ -34,10 +35,6 @@ abstract class PhabricatorApplicationTransactionQuery abstract public function getTemplateApplicationTransaction(); - protected function buildMoreWhereClauses(AphrontDatabaseConnection $conn_r) { - return array(); - } - public function withPHIDs(array $phids) { $this->phids = $phids; return $this; @@ -58,6 +55,11 @@ abstract class PhabricatorApplicationTransactionQuery return $this; } + public function withComments($with_comments) { + $this->withComments = $with_comments; + return $this; + } + public function needComments($need) { $this->needComments = $need; return $this; @@ -70,17 +72,8 @@ abstract class PhabricatorApplicationTransactionQuery protected function loadPage() { $table = $this->getTemplateApplicationTransaction(); - $conn_r = $table->establishConnection('r'); - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T x %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - $xactions = $table->loadAllFromArray($data); + $xactions = $this->loadStandardPage($table); foreach ($xactions as $xaction) { $xaction->attachViewer($this->getViewer()); @@ -161,50 +154,86 @@ abstract class PhabricatorApplicationTransactionQuery return $xactions; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, - 'phid IN (%Ls)', + $conn, + 'x.phid IN (%Ls)', $this->phids); } - if ($this->objectPHIDs) { + if ($this->objectPHIDs !== null) { $where[] = qsprintf( - $conn_r, - 'objectPHID IN (%Ls)', + $conn, + 'x.objectPHID IN (%Ls)', $this->objectPHIDs); } - if ($this->authorPHIDs) { + if ($this->authorPHIDs !== null) { $where[] = qsprintf( - $conn_r, - 'authorPHID IN (%Ls)', + $conn, + 'x.authorPHID IN (%Ls)', $this->authorPHIDs); } - if ($this->transactionTypes) { + if ($this->transactionTypes !== null) { $where[] = qsprintf( - $conn_r, - 'transactionType IN (%Ls)', + $conn, + 'x.transactionType IN (%Ls)', $this->transactionTypes); } - foreach ($this->buildMoreWhereClauses($conn_r) as $clause) { - $where[] = $clause; + if ($this->withComments !== null) { + if (!$this->withComments) { + $where[] = qsprintf( + $conn, + 'c.id IS NULL'); + } } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->withComments !== null) { + $xaction = $this->getTemplateApplicationTransaction(); + $comment = $xaction->getApplicationTransactionCommentObject(); + + if ($this->withComments) { + $joins[] = qsprintf( + $conn, + 'JOIN %T c ON x.phid = c.transactionPHID', + $comment->getTableName()); + } else { + $joins[] = qsprintf( + $conn, + 'LEFT JOIN %T c ON x.phid = c.transactionPHID', + $comment->getTableName()); + } + } + + return $joins; + } + + protected function shouldGroupQueryResultRows() { + if ($this->withComments !== null) { + return true; + } + + return parent::shouldGroupQueryResultRows(); + } public function getQueryApplicationClass() { // TODO: Sort this out? return null; } + protected function getPrimaryTableAlias() { + return 'x'; + } + } From a3a6c4ed2edfa835c1e071f8a433380f2dfe0941 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Sep 2017 09:01:12 -0700 Subject: [PATCH 6/9] Fix fatal when searching for "r matey prepare to be boarded" Summary: See . The Ferret engine replaced `withNameContains()`, but I missed this obscure callsite. Test Plan: - Searched for `r matey prepare to be boarded`. - Before: fatal. - After: no fatal. - Also searched for `r `, got repository. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18661 --- .../engine/PhabricatorJumpNavHandler.php | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/applications/search/engine/PhabricatorJumpNavHandler.php b/src/applications/search/engine/PhabricatorJumpNavHandler.php index 6f6ea3d0a9..ddade36d84 100644 --- a/src/applications/search/engine/PhabricatorJumpNavHandler.php +++ b/src/applications/search/engine/PhabricatorJumpNavHandler.php @@ -50,17 +50,35 @@ final class PhabricatorJumpNavHandler extends Phobject { return id(new AphrontRedirectResponse()) ->setURI("/diffusion/symbol/$symbol/?jump=true$context"); case 'find-repository': - $name = $matches[1]; + $raw_query = $matches[1]; + + $engine = id(new PhabricatorRepository()) + ->newFerretEngine(); + + $compiler = id(new PhutilSearchQueryCompiler()) + ->setEnableFunctions(true); + + $raw_tokens = $compiler->newTokens($raw_query); + + $fulltext_tokens = array(); + foreach ($raw_tokens as $raw_token) { + $fulltext_token = id(new PhabricatorFulltextToken()) + ->setToken($raw_token); + $fulltext_tokens[] = $fulltext_token; + } + $repositories = id(new PhabricatorRepositoryQuery()) ->setViewer($viewer) - ->withNameContains($name) + ->withFerretConstraint($engine, $fulltext_tokens) ->execute(); if (count($repositories) == 1) { // Just one match, jump to repository. $uri = head($repositories)->getURI(); } else { // More than one match, jump to search. - $uri = urisprintf('/diffusion/?order=name&name=%s', $name); + $uri = urisprintf( + '/diffusion/?order=name&query=%s', + $raw_query); } return id(new AphrontRedirectResponse())->setURI($uri); default: From a39f5e11139ae5631eb990fd2d5ed528724ae00e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Sep 2017 14:48:38 -0700 Subject: [PATCH 7/9] Correct bad context path when doing pattern search inside a repository Summary: Ref PHI101. It looks like this was maybe copy/pasted by mistake in recent design refactoring. We need to pass the full path, not the `basename()` of the path, to the search form. Test Plan: Searched inside `scripts/test/`, found results inside `scripts/test/`. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18664 --- .../diffusion/controller/DiffusionBrowseController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index d3223e64fd..b4c0352d53 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1557,7 +1557,8 @@ final class DiffusionBrowseController extends DiffusionController { $commit_tag = $this->renderCommitHashTag($drequest); - $path = nonempty(basename($drequest->getPath()), '/'); + $path = nonempty($drequest->getPath(), '/'); + $search = $this->renderSearchForm($path); $header = id(new PHUIHeaderView()) From fe646ec328289afd6c52ef38659b92cc8d6bd211 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Sep 2017 13:25:55 -0700 Subject: [PATCH 8/9] Mark Owners package reviewers which own nothing in the current diff Summary: Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant. Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not. This is a rough cut to get the feature working, design could probably use some tweaking if it sticks. Test Plan: {F5204309} Reviewers: amckinley Reviewed By: amckinley Subscribers: jboning Differential Revision: https://secure.phabricator.com/D18663 --- .../controller/DifferentialController.php | 130 ++++++++++++------ .../DifferentialDiffViewController.php | 2 + .../DifferentialRevisionViewController.php | 10 ++ .../storage/DifferentialReviewer.php | 10 ++ .../view/DifferentialReviewersView.php | 6 + 5 files changed, 118 insertions(+), 40 deletions(-) diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php index 2258c4fdcb..fb8a6249a7 100644 --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -2,6 +2,10 @@ abstract class DifferentialController extends PhabricatorController { + private $packageChangesetMap; + private $pathPackageMap; + private $authorityPackages; + public function buildSideNavView($for_app = false) { $viewer = $this->getRequest()->getUser(); @@ -21,6 +25,88 @@ abstract class DifferentialController extends PhabricatorController { return $this->buildSideNavView(true)->getMenu(); } + protected function buildPackageMaps(array $changesets) { + assert_instances_of($changesets, 'DifferentialChangeset'); + + $this->packageChangesetMap = array(); + $this->pathPackageMap = array(); + $this->authorityPackages = array(); + + if (!$changesets) { + return; + } + + $viewer = $this->getViewer(); + + $have_owners = PhabricatorApplication::isClassInstalledForViewer( + 'PhabricatorOwnersApplication', + $viewer); + if (!$have_owners) { + return; + } + + $changeset = head($changesets); + $diff = $changeset->getDiff(); + $repository_phid = $diff->getRepositoryPHID(); + if (!$repository_phid) { + return; + } + + if ($viewer->getPHID()) { + $packages = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withAuthorityPHIDs(array($viewer->getPHID())) + ->execute(); + $this->authorityPackages = $packages; + } + + $paths = mpull($changesets, 'getOwnersFilename'); + + $control_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withControl($repository_phid, $paths); + $control_query->execute(); + + foreach ($changesets as $changeset) { + $changeset_path = $changeset->getOwnersFilename(); + + $packages = $control_query->getControllingPackagesForPath( + $repository_phid, + $changeset_path); + + $this->pathPackageMap[$changeset_path] = $packages; + foreach ($packages as $package) { + $this->packageChangesetMap[$package->getPHID()][] = $changeset; + } + } + } + + protected function getAuthorityPackages() { + if ($this->authorityPackages === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + return $this->authorityPackages; + } + + protected function getChangesetPackages(DifferentialChangeset $changeset) { + if ($this->pathPackageMap === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + + $path = $changeset->getOwnersFilename(); + return idx($this->pathPackageMap, $path, array()); + } + + protected function getPackageChangesets($package_phid) { + if ($this->packageChangesetMap === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + + return idx($this->packageChangesetMap, $package_phid, array()); + } + protected function buildTableOfContents( array $changesets, array $visible_changesets, @@ -29,40 +115,8 @@ abstract class DifferentialController extends PhabricatorController { $toc_view = id(new PHUIDiffTableOfContentsListView()) ->setViewer($viewer) - ->setBare(true); - - $have_owners = PhabricatorApplication::isClassInstalledForViewer( - 'PhabricatorOwnersApplication', - $viewer); - if ($have_owners) { - $repository_phid = null; - if ($changesets) { - $changeset = head($changesets); - $diff = $changeset->getDiff(); - $repository_phid = $diff->getRepositoryPHID(); - } - - if (!$repository_phid) { - $have_owners = false; - } else { - if ($viewer->getPHID()) { - $packages = id(new PhabricatorOwnersPackageQuery()) - ->setViewer($viewer) - ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) - ->withAuthorityPHIDs(array($viewer->getPHID())) - ->execute(); - $toc_view->setAuthorityPackages($packages); - } - - $paths = mpull($changesets, 'getOwnersFilename'); - - $control_query = id(new PhabricatorOwnersPackageQuery()) - ->setViewer($viewer) - ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) - ->withControl($repository_phid, $paths); - $control_query->execute(); - } - } + ->setBare(true) + ->setAuthorityPackages($this->getAuthorityPackages()); foreach ($changesets as $changeset_id => $changeset) { $is_visible = isset($visible_changesets[$changeset_id]); @@ -78,12 +132,8 @@ abstract class DifferentialController extends PhabricatorController { ->setCoverage(idx($coverage, $filename)) ->setCoverageID($coverage_id); - if ($have_owners) { - $packages = $control_query->getControllingPackagesForPath( - $repository_phid, - $changeset->getOwnersFilename()); - $item->setPackages($packages); - } + $packages = $this->getChangesetPackages($changeset); + $item->setPackages($packages); $toc_view->addItem($item); } diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index 7340687bc4..e56eb27d99 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -118,6 +118,8 @@ final class DifferentialDiffViewController extends DifferentialController { $changesets = $diff->loadChangesets(); $changesets = msort($changesets, 'getSortKey'); + $this->buildPackageMaps($changesets); + $table_of_contents = $this->buildTableOfContents( $changesets, $changesets, diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 06d8cf0a3f..e1da5ed32e 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -326,11 +326,21 @@ final class DifferentialRevisionViewController extends DifferentialController { $other_view = $this->renderOtherRevisions($other_revisions); } + $this->buildPackageMaps($changesets); + $toc_view = $this->buildTableOfContents( $changesets, $visible_changesets, $target->loadCoverageMap($viewer)); + // Attach changesets to each reviewer so we can show which Owners package + // reviewers own no files. + foreach ($revision->getReviewers() as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + $reviewer_changesets = $this->getPackageChangesets($reviewer_phid); + $reviewer->attachChangesets($reviewer_changesets); + } + $tab_group = id(new PHUITabGroupView()) ->addTab( id(new PHUITabView()) diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 3aa9bf6362..9df149e788 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -12,6 +12,7 @@ final class DifferentialReviewer protected $voidedPHID; private $authority = array(); + private $changesets = self::ATTACHABLE; protected function getConfiguration() { return array( @@ -54,6 +55,15 @@ final class DifferentialReviewer return $this->assertAttachedKey($this->authority, $cache_fragment); } + public function attachChangesets(array $changesets) { + $this->changesets = $changesets; + return $this; + } + + public function getChangesets() { + return $this->assertAttached($this->changesets); + } + public function isResigned() { $status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED; return ($this->getReviewerStatus() == $status_resigned); diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 3fcb1c0ccf..33aad25289 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -150,6 +150,12 @@ final class DifferentialReviewersView extends AphrontView { $item->setIcon($icon, $color, $label); $item->setTarget($handle->renderHovercardLink()); + if ($reviewer->isPackage()) { + if (!$reviewer->getChangesets()) { + $item->setNote(pht('(Owns No Changed Paths)')); + } + } + $view->addItem($item); } From 5c73c169fd31d029df764086cab24d19a6317d76 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Sep 2017 16:20:42 -0700 Subject: [PATCH 9/9] Rough cut of "Move tasks to column..." Summary: Ref T5523. See PHI50. See PHI40. This isn't perfect, but should improve things. Add a "Move tasks to column..." action to workboards which moves all visible tasks in a column to another column, either on the same board or on a different board. This is a two-step process so that I don't have to write Javascript, and because I'm not 100% sure this is what users actually want/need. If it sticks, the UI could be refined later. - The first dialog asks you to choose a project, defaulting ot the current project. - The second dialog asks you to choose a column on that project's board. Test Plan: - Moved tasks on the same board. - Moved tasks to a different board. - Tried to move tasks to the starting column, got a sensible error. - Tried to move tasks to no project, got a sensible error. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T5523 Differential Revision: https://secure.phabricator.com/D18665 --- .../PhabricatorProjectBoardViewController.php | 205 +++++++++++++++++- 1 file changed, 204 insertions(+), 1 deletion(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 8405ec677f..87cb5bc417 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -34,7 +34,9 @@ final class PhabricatorProjectBoardViewController ->setBaseURI($board_uri) ->setIsBoardView(true); - if ($request->isFormPost() && !$request->getBool('initialize')) { + if ($request->isFormPost() + && !$request->getBool('initialize') + && !$request->getStr('move')) { $saved = $search_engine->buildSavedQueryFromRequest($request); $search_engine->saveQuery($saved); $filter_form = id(new AphrontFormView()) @@ -238,6 +240,199 @@ final class PhabricatorProjectBoardViewController ->setURI($batch_uri); } + $move_id = $request->getStr('move'); + if (strlen($move_id)) { + $column_id_map = mpull($columns, null, 'getID'); + $move_column = idx($column_id_map, $move_id); + if (!$move_column) { + return new Aphront404Response(); + } + + $move_task_phids = $layout_engine->getColumnObjectPHIDs( + $board_phid, + $move_column->getPHID()); + + foreach ($move_task_phids as $key => $move_task_phid) { + if (empty($task_can_edit_map[$move_task_phid])) { + unset($move_task_phids[$key]); + } + } + + $move_tasks = array_select_keys($tasks, $move_task_phids); + $cancel_uri = $this->getURIWithState($board_uri); + + if (!$move_tasks) { + return $this->newDialog() + ->setTitle(pht('No Movable Tasks')) + ->appendParagraph( + pht( + 'The selected column contains no visible tasks which you '. + 'have permission to move.')) + ->addCancelButton($cancel_uri); + } + + $move_project_phid = $project->getPHID(); + $move_column_phid = null; + $move_project = null; + $move_column = null; + $columns = null; + $errors = array(); + + if ($request->isFormPost()) { + $move_project_phid = head($request->getArr('moveProjectPHID')); + if (!$move_project_phid) { + $move_project_phid = $request->getStr('moveProjectPHID'); + } + + if (!$move_project_phid) { + if ($request->getBool('hasProject')) { + $errors[] = pht('Choose a project to move tasks to.'); + } + } else { + $target_project = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($move_project_phid)) + ->executeOne(); + if (!$target_project) { + $errors[] = pht('You must choose a valid project.'); + } else if (!$project->getHasWorkboard()) { + $errors[] = pht( + 'You must choose a project with a workboard.'); + } else { + $move_project = $target_project; + } + } + + if ($move_project) { + $move_engine = id(new PhabricatorBoardLayoutEngine()) + ->setViewer($viewer) + ->setBoardPHIDs(array($move_project->getPHID())) + ->setFetchAllBoards(true) + ->executeLayout(); + + $columns = $move_engine->getColumns($move_project->getPHID()); + $columns = mpull($columns, null, 'getPHID'); + + $move_column_phid = $request->getStr('moveColumnPHID'); + if (!$move_column_phid) { + if ($request->getBool('hasColumn')) { + $errors[] = pht('Choose a column to move tasks to.'); + } + } else { + if (empty($columns[$move_column_phid])) { + $errors[] = pht( + 'Choose a valid column on the target workboard to move '. + 'tasks to.'); + } else if ($columns[$move_column_phid]->getID() == $move_id) { + $errors[] = pht( + 'You can not move tasks from a column to itself.'); + } else { + $move_column = $columns[$move_column_phid]; + } + } + } + } + + if ($move_column && $move_project) { + foreach ($move_tasks as $move_task) { + $xactions = array(); + + // If we're switching projects, get out of the old project first + // and move to the new project. + if ($move_project->getID() != $project->getID()) { + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue( + 'edge:type', + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST) + ->setNewValue( + array( + '-' => array( + $project->getPHID() => $project->getPHID(), + ), + '+' => array( + $move_project->getPHID() => $move_project->getPHID(), + ), + )); + } + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_COLUMNS) + ->setNewValue( + array( + array( + 'columnPHID' => $move_column->getPHID(), + ), + )); + + $editor = id(new ManiphestTransactionEditor()) + ->setActor($viewer) + ->setContinueOnMissingFields(true) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request); + + $editor->applyTransactions($move_task, $xactions); + } + + return id(new AphrontRedirectResponse()) + ->setURI($cancel_uri); + } + + if ($move_project) { + $column_form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendControl( + id(new AphrontFormSelectControl()) + ->setName('moveColumnPHID') + ->setLabel(pht('Move to Column')) + ->setValue($move_column_phid) + ->setOptions(mpull($columns, 'getDisplayName', 'getPHID'))); + + return $this->newDialog() + ->setTitle(pht('Move Tasks')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setErrors($errors) + ->addHiddenInput('move', $move_id) + ->addHiddenInput('moveProjectPHID', $move_project->getPHID()) + ->addHiddenInput('hasColumn', true) + ->addHiddenInput('hasProject', true) + ->appendParagraph( + pht( + 'Choose a column on the %s workboard to move tasks to:', + $viewer->renderHandle($move_project->getPHID()))) + ->appendForm($column_form) + ->addSubmitButton(pht('Move Tasks')) + ->addCancelButton($cancel_uri); + } + + if ($move_project_phid) { + $move_project_phid_value = array($move_project_phid); + } else { + $move_project_phid_value = array(); + } + + $project_form = id(new AphrontFormView()) + ->setViewer($viewer) + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setName('moveProjectPHID') + ->setLimit(1) + ->setLabel(pht('Move to Project')) + ->setValue($move_project_phid_value) + ->setDatasource(new PhabricatorProjectDatasource())); + + return $this->newDialog() + ->setTitle(pht('Move Tasks')) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setErrors($errors) + ->addHiddenInput('move', $move_id) + ->addHiddenInput('hasProject', true) + ->appendForm($project_form) + ->addSubmitButton(pht('Continue')) + ->addCancelButton($cancel_uri); + } + + $board_id = celerity_generate_unique_node_id(); $board = id(new PHUIWorkboardView()) @@ -851,6 +1046,14 @@ final class PhabricatorProjectBoardViewController ->setHref($batch_edit_uri) ->setDisabled(!$can_batch_edit); + $batch_move_uri = $request->getRequestURI(); + $batch_move_uri->setQueryParam('move', $column->getID()); + $column_items[] = id(new PhabricatorActionView()) + ->setIcon('fa-arrow-right') + ->setName(pht('Move Tasks to Column...')) + ->setHref($batch_move_uri) + ->setWorkflow(true); + // Column Related Actions Below // $edit_uri = 'board/'.$this->id.'/edit/'.$column->getID().'/';