From 37843127e94a878a7f5bf2c65c8e7004bc65c68a Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 28 Aug 2017 10:55:23 -0700 Subject: [PATCH 01/24] Widen blame age line in blame view Summary: 50% more line, no additional cost! Order Now! Operators are standing by. Test Plan: Blame a file Reviewers: epriestley, avivey Reviewed By: avivey Spies: Korvin Differential Revision: https://secure.phabricator.com/D18481 --- .../diffusion/controller/DiffusionBrowseController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index b9d63b2f85..5eee013622 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1134,7 +1134,7 @@ final class DiffusionBrowseController extends DiffusionController { $before_link = null; $commit_date = null; - $style = 'border-right: 2px solid '.$line['color'].';'; + $style = 'border-right: 3px solid '.$line['color'].';'; if ($identifier && !$line['duplicate']) { if (isset($commit_links[$identifier])) { From b8b701faf734cd00a3ddae43d884d9d5248feb9f Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 28 Aug 2017 11:41:48 -0700 Subject: [PATCH 02/24] Clarify language when Autoclose is disabled for a repository Summary: Fixes T12051, adds additional language. Test Plan: Disable Autoclose in Actions, see updated language under Branches. {F5147291} Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T12051 Differential Revision: https://secure.phabricator.com/D18482 --- .../DiffusionRepositoryBranchesManagementPanel.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php index 17ee506b16..90b646c3f9 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php @@ -86,8 +86,11 @@ final class DiffusionRepositoryBranchesManagementPanel $repository->getHumanReadableDetail('close-commits-filter', array()), phutil_tag('em', array(), pht('Autoclose On All Branches'))); + $autoclose_disabled = false; if ($repository->getDetail('disable-autoclose')) { - $autoclose_only = phutil_tag('em', array(), pht('Disabled')); + $autoclose_disabled = true; + $autoclose_only = + phutil_tag('em', array(), pht('Autoclose has been disabled')); } $view->addProperty(pht('Autoclose Only'), $autoclose_only); @@ -133,12 +136,18 @@ final class DiffusionRepositoryBranchesManagementPanel $status = pht('Open'); } + if ($autoclose_disabled) { + $autoclose_status = pht('Disabled (Repository)'); + } else { + $autoclose_status = pht('Off'); + } + $rows[] = array( $icon, $branch_name, $status, $tracking ? pht('Tracking') : pht('Off'), - $autoclosing ? pht('Autoclose On') : pht('Off'), + $autoclosing ? pht('Autoclose On') : $autoclose_status, ); } $branch_table = new AphrontTableView($rows); From 643877b4672de01f0b0a9f5ef335db928608f1a4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Aug 2017 12:12:13 -0700 Subject: [PATCH 03/24] Don't prompt to mark notifications as read if we don't need to Summary: Fixes whatever task is tracking this junk, if one exists. Don't prompt unless there's a security issue. Test Plan: - Generated notifications from a test account. - Clicked "Mark All" from dropdown menu, no prompt. - Clicked "Mark All" from notifications screen, no prompt. - Command-Clicked "Mark All" from dropdown menu to open in new window, got normal prompt. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18483 --- .../PhabricatorNotificationClearController.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/applications/notification/controller/PhabricatorNotificationClearController.php b/src/applications/notification/controller/PhabricatorNotificationClearController.php index a826ae0a6b..1d6c7f7b97 100644 --- a/src/applications/notification/controller/PhabricatorNotificationClearController.php +++ b/src/applications/notification/controller/PhabricatorNotificationClearController.php @@ -8,6 +8,17 @@ final class PhabricatorNotificationClearController $chrono_key = $request->getStr('chronoKey'); if ($request->isDialogFormPost()) { + $should_clear = true; + } else { + try { + $request->validateCSRF(); + $should_clear = true; + } catch (AphrontMalformedRequestException $ex) { + $should_clear = false; + } + } + + if ($should_clear) { $table = new PhabricatorFeedStoryNotification(); queryfx( From ed75250f1a9d913fe71d6fb08cef38872e5d833e Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 28 Aug 2017 14:51:00 -0700 Subject: [PATCH 04/24] Update notification UI a little Summary: Fixes T8944. Adds a small dot if notification is new along with color. Goes away when clicked. Increased font and padding for readability. Test Plan: Send notifications from test account, review them in menu, application search, and in real-time display. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T8944 Differential Revision: https://secure.phabricator.com/D18485 --- resources/celerity/map.php | 10 ++--- src/view/phui/PHUIFeedStoryView.php | 16 ++++++- webroot/rsrc/css/aphront/notification.css | 2 +- .../application/base/notification-menu.css | 43 ++++++++++++++----- 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2433beba80..9aac86c370 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' => '291cbd98', + 'core.pkg.css' => 'b324663c', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -29,7 +29,7 @@ return array( 'rsrc/css/aphront/dialog-view.css' => '6bfc244b', 'rsrc/css/aphront/list-filter-view.css' => '5d6f0526', 'rsrc/css/aphront/multi-column.css' => '84cc6640', - 'rsrc/css/aphront/notification.css' => '3f6c89c9', + 'rsrc/css/aphront/notification.css' => '457861ec', 'rsrc/css/aphront/panel-view.css' => '8427b78d', 'rsrc/css/aphront/phabricator-nav-view.css' => 'faf6a6fc', 'rsrc/css/aphront/table-view.css' => 'a3aa6910', @@ -40,7 +40,7 @@ return array( 'rsrc/css/application/almanac/almanac.css' => 'dbb9b3af', 'rsrc/css/application/auth/auth.css' => '0877ed6e', 'rsrc/css/application/base/main-menu-view.css' => '1802a242', - 'rsrc/css/application/base/notification-menu.css' => '73fefdfa', + 'rsrc/css/application/base/notification-menu.css' => '10685bd4', 'rsrc/css/application/base/phui-theme.css' => '9f261c6b', 'rsrc/css/application/base/standard-page-view.css' => '34ee718b', 'rsrc/css/application/chatlog/chatlog.css' => 'd295b020', @@ -792,8 +792,8 @@ return array( 'phabricator-main-menu-view' => '1802a242', 'phabricator-nav-view-css' => 'faf6a6fc', 'phabricator-notification' => '5c3349b2', - 'phabricator-notification-css' => '3f6c89c9', - 'phabricator-notification-menu-css' => '73fefdfa', + 'phabricator-notification-css' => '457861ec', + 'phabricator-notification-menu-css' => '10685bd4', 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => 'c5af80a2', diff --git a/src/view/phui/PHUIFeedStoryView.php b/src/view/phui/PHUIFeedStoryView.php index fbba4a3f46..f0acd48201 100644 --- a/src/view/phui/PHUIFeedStoryView.php +++ b/src/view/phui/PHUIFeedStoryView.php @@ -150,13 +150,25 @@ final class PHUIFeedStoryView extends AphrontView { if ($this->getShowTimestamp()) { if ($this->epoch) { if ($user) { - $foot = phabricator_datetime($this->epoch, $user); + $marker = id(new PHUIIconView()) + ->setIcon('fa-circle') + ->addClass('phabricator-notification-status'); + $date = phabricator_datetime($this->epoch, $user); $foot = phutil_tag( 'span', array( 'class' => 'phabricator-notification-date', ), - $foot); + $date); + $foot = phutil_tag( + 'div', + array( + 'class' => 'phabricator-notification-foot', + ), + array( + $marker, + $date, + )); } else { $foot = null; } diff --git a/webroot/rsrc/css/aphront/notification.css b/webroot/rsrc/css/aphront/notification.css index ee835bba76..306075b582 100644 --- a/webroot/rsrc/css/aphront/notification.css +++ b/webroot/rsrc/css/aphront/notification.css @@ -11,7 +11,7 @@ .jx-notification { width: 240px; - padding: 8px 16px; + padding: 8px 12px; font-size: 11px; overflow: hidden; diff --git a/webroot/rsrc/css/application/base/notification-menu.css b/webroot/rsrc/css/application/base/notification-menu.css index 1a834b11fb..8e2391d57c 100644 --- a/webroot/rsrc/css/application/base/notification-menu.css +++ b/webroot/rsrc/css/application/base/notification-menu.css @@ -4,7 +4,8 @@ .phabricator-notification-menu { background: {$page.content}; - font-size: {$smallestfontsize}; + font-size: {$smallerfontsize}; + line-height: 18px; word-wrap: break-word; overflow-y: auto; box-shadow: {$dropshadow}; @@ -12,9 +13,8 @@ border-radius: 3px; } -.phabricator-notification .phabricator-notification-date { - margin-left: 8px; - color: {$lightgreytext}; +.phabricator-notification { + padding: 8px 12px; } .phabricator-notification-menu-loading { @@ -44,11 +44,7 @@ } .phabricator-notification-list .phabricator-notification { - padding: 10px 4px; -} - -.phabricator-notification { - padding: 6px 8px; + padding: 8px; } .phabricator-notification-menu .phabricator-notification { @@ -56,7 +52,7 @@ } .device-desktop .phabricator-notification-menu .phabricator-notification:hover { - background: {$hoverblue}; + background: {$lightgreybackground}; } .device-desktop .phabricator-notification-menu @@ -81,9 +77,34 @@ color: {$lightgreytext}; } +.phabricator-notification-foot { + color: {$lightgreytext}; + font-size: {$smallestfontsize}; + line-height: 18px; + position: relative; +} + +.phabricator-notification-unread .phabricator-notification-foot { + padding-left: 10px; +} + +.phabricator-notification-foot .phabricator-notification-status { + display: none; +} + +.phabricator-notification-unread .phabricator-notification-foot + .phabricator-notification-status { + font-size: 7px; + color: {$lightgreytext}; + position: absolute; + display: inline-block; + top: 6px; + left: 0; +} + .phabricator-notification-header { font-weight: bold; - padding: 10px 8px; + padding: 10px 12px; font-size: {$smallerfontsize}; border-bottom: 1px solid {$thinblueborder}; } From f97157e7edb1ca395da17e7dc0662fd0ac573675 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Aug 2017 13:04:56 -0700 Subject: [PATCH 05/24] Build a prototype fulltext engine ("Ferret") using only basic MySQL primitives Summary: Ref T12819. I gave this stuff a sweet code name because all the terms related to "fulltext" and "search" already mean 5 different things. It, uh, ferrets out documents for you? I'm building this to work a lot like the existing ngram index, which seems to work pretty well. If this sticks, it will auto-resolve the join issue (in T12443) by letting us do the entire thing locally in a JOIN and thus dodge a lot of mess. This index gets built alongside other indexes, but only shows up in the UI if you have prototypes enabled. If you do, it appears under the existing fulltext field in Maniphest. No existing functionality is affected or disrupted. NOTE: The query engine half of this is still EXTREMELY primitive, and this probably performs worse than the existing field for now. If this doesn't show obvious signs of being awful on `secure` I'll improve that in followup changes. Test Plan: Indexed my tasks, ran some simple queries, got the results I wanted, even for queries "ko", "k", "v0.1". {F5147746} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819, T12443 Differential Revision: https://secure.phabricator.com/D18484 --- .../20170828.ferret.01.taskdoc.sql | 9 ++ .../20170828.ferret.02.taskfield.sql | 7 + .../20170828.ferret.03.taskngrams.sql | 5 + src/__phutil_library_map__.php | 22 +++ .../query/ManiphestTaskSearchEngine.php | 13 ++ .../search/ManiphestTaskFerretEngine.php | 18 +++ .../maniphest/storage/ManiphestTask.php | 9 ++ .../storage/ManiphestTaskFerretDocument.php | 14 ++ .../storage/ManiphestTaskFerretField.php | 14 ++ .../storage/ManiphestTaskFerretNgrams.php | 14 ++ ...abricatorFerretFulltextEngineExtension.php | 126 +++++++++++++++ .../ferret/PhabricatorFerretDocument.php | 40 +++++ .../search/ferret/PhabricatorFerretEngine.php | 9 ++ .../search/ferret/PhabricatorFerretField.php | 36 +++++ .../ferret/PhabricatorFerretInterface.php | 7 + .../search/ferret/PhabricatorFerretNgrams.php | 35 ++++ .../search/ngrams/PhabricatorNgramEngine.php | 41 +++++ ...PhabricatorCursorPagedPolicyAwareQuery.php | 152 ++++++++++++++++++ 18 files changed, 571 insertions(+) create mode 100644 resources/sql/autopatches/20170828.ferret.01.taskdoc.sql create mode 100644 resources/sql/autopatches/20170828.ferret.02.taskfield.sql create mode 100644 resources/sql/autopatches/20170828.ferret.03.taskngrams.sql create mode 100644 src/applications/maniphest/search/ManiphestTaskFerretEngine.php create mode 100644 src/applications/maniphest/storage/ManiphestTaskFerretDocument.php create mode 100644 src/applications/maniphest/storage/ManiphestTaskFerretField.php create mode 100644 src/applications/maniphest/storage/ManiphestTaskFerretNgrams.php create mode 100644 src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php create mode 100644 src/applications/search/ferret/PhabricatorFerretDocument.php create mode 100644 src/applications/search/ferret/PhabricatorFerretEngine.php create mode 100644 src/applications/search/ferret/PhabricatorFerretField.php create mode 100644 src/applications/search/ferret/PhabricatorFerretInterface.php create mode 100644 src/applications/search/ferret/PhabricatorFerretNgrams.php create mode 100644 src/applications/search/ngrams/PhabricatorNgramEngine.php diff --git a/resources/sql/autopatches/20170828.ferret.01.taskdoc.sql b/resources/sql/autopatches/20170828.ferret.01.taskdoc.sql new file mode 100644 index 0000000000..8cb6835602 --- /dev/null +++ b/resources/sql/autopatches/20170828.ferret.01.taskdoc.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_maniphest.maniphest_task_fdocument ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARBINARY(64) NOT NULL, + isClosed BOOL NOT NULL, + authorPHID VARBINARY(64), + ownerPHID VARBINARY(64), + epochCreated INT UNSIGNED NOT NULL, + epochModified INT UNSIGNED NOT NULL +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20170828.ferret.02.taskfield.sql b/resources/sql/autopatches/20170828.ferret.02.taskfield.sql new file mode 100644 index 0000000000..5528feec8f --- /dev/null +++ b/resources/sql/autopatches/20170828.ferret.02.taskfield.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_maniphest.maniphest_task_ffield ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + documentID INT UNSIGNED NOT NULL, + fieldKey VARCHAR(4) NOT NULL COLLATE {$COLLATE_TEXT}, + rawCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT}, + normalCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT} +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20170828.ferret.03.taskngrams.sql b/resources/sql/autopatches/20170828.ferret.03.taskngrams.sql new file mode 100644 index 0000000000..a7b5180642 --- /dev/null +++ b/resources/sql/autopatches/20170828.ferret.03.taskngrams.sql @@ -0,0 +1,5 @@ +CREATE TABLE {$NAMESPACE}_maniphest.maniphest_task_fngrams ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + documentID INT UNSIGNED NOT NULL, + ngram CHAR(3) NOT NULL COLLATE {$COLLATE_TEXT} +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4575908ef8..3edb31640b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1533,6 +1533,10 @@ phutil_register_library_map(array( 'ManiphestTaskEditBulkJobType' => 'applications/maniphest/bulk/ManiphestTaskEditBulkJobType.php', 'ManiphestTaskEditController' => 'applications/maniphest/controller/ManiphestTaskEditController.php', 'ManiphestTaskEditEngineLock' => 'applications/maniphest/editor/ManiphestTaskEditEngineLock.php', + 'ManiphestTaskFerretDocument' => 'applications/maniphest/storage/ManiphestTaskFerretDocument.php', + 'ManiphestTaskFerretEngine' => 'applications/maniphest/search/ManiphestTaskFerretEngine.php', + 'ManiphestTaskFerretField' => 'applications/maniphest/storage/ManiphestTaskFerretField.php', + 'ManiphestTaskFerretNgrams' => 'applications/maniphest/storage/ManiphestTaskFerretNgrams.php', 'ManiphestTaskFulltextEngine' => 'applications/maniphest/search/ManiphestTaskFulltextEngine.php', 'ManiphestTaskGraph' => 'infrastructure/graph/ManiphestTaskGraph.php', 'ManiphestTaskHasCommitEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php', @@ -2828,6 +2832,12 @@ phutil_register_library_map(array( 'PhabricatorFeedStoryNotification' => 'applications/notification/storage/PhabricatorFeedStoryNotification.php', 'PhabricatorFeedStoryPublisher' => 'applications/feed/PhabricatorFeedStoryPublisher.php', 'PhabricatorFeedStoryReference' => 'applications/feed/storage/PhabricatorFeedStoryReference.php', + 'PhabricatorFerretDocument' => 'applications/search/ferret/PhabricatorFerretDocument.php', + 'PhabricatorFerretEngine' => 'applications/search/ferret/PhabricatorFerretEngine.php', + 'PhabricatorFerretField' => 'applications/search/ferret/PhabricatorFerretField.php', + 'PhabricatorFerretFulltextEngineExtension' => 'applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php', + 'PhabricatorFerretInterface' => 'applications/search/ferret/PhabricatorFerretInterface.php', + 'PhabricatorFerretNgrams' => 'applications/search/ferret/PhabricatorFerretNgrams.php', 'PhabricatorFile' => 'applications/files/storage/PhabricatorFile.php', 'PhabricatorFileAES256StorageFormat' => 'applications/files/format/PhabricatorFileAES256StorageFormat.php', 'PhabricatorFileBundleLoader' => 'applications/files/query/PhabricatorFileBundleLoader.php', @@ -3195,6 +3205,7 @@ phutil_register_library_map(array( 'PhabricatorNamedQueryQuery' => 'applications/search/query/PhabricatorNamedQueryQuery.php', 'PhabricatorNavigationRemarkupRule' => 'infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php', 'PhabricatorNeverTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorNeverTriggerClock.php', + 'PhabricatorNgramEngine' => 'applications/search/ngrams/PhabricatorNgramEngine.php', 'PhabricatorNgramsIndexEngineExtension' => 'applications/search/engineextension/PhabricatorNgramsIndexEngineExtension.php', 'PhabricatorNgramsInterface' => 'applications/search/interface/PhabricatorNgramsInterface.php', 'PhabricatorNotificationBuilder' => 'applications/notification/builder/PhabricatorNotificationBuilder.php', @@ -6659,6 +6670,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesInterface', 'PhabricatorConduitResultInterface', 'PhabricatorFulltextInterface', + 'PhabricatorFerretInterface', 'DoorkeeperBridgedObjectInterface', 'PhabricatorEditEngineSubtypeInterface', 'PhabricatorEditEngineLockableInterface', @@ -6682,6 +6694,10 @@ phutil_register_library_map(array( 'ManiphestTaskEditBulkJobType' => 'PhabricatorWorkerBulkJobType', 'ManiphestTaskEditController' => 'ManiphestController', 'ManiphestTaskEditEngineLock' => 'PhabricatorEditEngineLock', + 'ManiphestTaskFerretDocument' => 'PhabricatorFerretDocument', + 'ManiphestTaskFerretEngine' => 'PhabricatorFerretEngine', + 'ManiphestTaskFerretField' => 'PhabricatorFerretField', + 'ManiphestTaskFerretNgrams' => 'PhabricatorFerretNgrams', 'ManiphestTaskFulltextEngine' => 'PhabricatorFulltextEngine', 'ManiphestTaskGraph' => 'PhabricatorObjectGraph', 'ManiphestTaskHasCommitEdgeType' => 'PhabricatorEdgeType', @@ -8147,6 +8163,11 @@ phutil_register_library_map(array( 'PhabricatorFeedStoryNotification' => 'PhabricatorFeedDAO', 'PhabricatorFeedStoryPublisher' => 'Phobject', 'PhabricatorFeedStoryReference' => 'PhabricatorFeedDAO', + 'PhabricatorFerretDocument' => 'PhabricatorSearchDAO', + 'PhabricatorFerretEngine' => 'Phobject', + 'PhabricatorFerretField' => 'PhabricatorSearchDAO', + 'PhabricatorFerretFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', + 'PhabricatorFerretNgrams' => 'PhabricatorSearchDAO', 'PhabricatorFile' => array( 'PhabricatorFileDAO', 'PhabricatorApplicationTransactionInterface', @@ -8565,6 +8586,7 @@ phutil_register_library_map(array( 'PhabricatorNamedQueryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorNavigationRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorNeverTriggerClock' => 'PhabricatorTriggerClock', + 'PhabricatorNgramEngine' => 'Phobject', 'PhabricatorNgramsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'PhabricatorNgramsInterface' => 'PhabricatorIndexableInterface', 'PhabricatorNotificationBuilder' => 'Phobject', diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 150ec81def..8eb4a416e3 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -49,6 +49,8 @@ final class ManiphestTaskSearchEngine $subtype_map = id(new ManiphestTask())->newEditEngineSubtypeMap(); $hide_subtypes = (count($subtype_map) == 1); + $hide_ferret = !PhabricatorEnv::getEnvConfig('phabricator.show-prototypes'); + return array( id(new PhabricatorOwnersSearchField()) ->setLabel(pht('Assigned To')) @@ -89,6 +91,10 @@ final class ManiphestTaskSearchEngine id(new PhabricatorSearchTextField()) ->setLabel(pht('Contains Words')) ->setKey('fulltext'), + id(new PhabricatorSearchTextField()) + ->setLabel(pht('Matches (Prototype)')) + ->setKey('ferret') + ->setIsHidden($hide_ferret), id(new PhabricatorSearchThreeStateField()) ->setLabel(pht('Open Parents')) ->setKey('hasParents') @@ -145,6 +151,7 @@ final class ManiphestTaskSearchEngine 'priorities', 'subtypes', 'fulltext', + 'ferret', 'hasParents', 'hasSubtasks', 'parentIDs', @@ -224,6 +231,12 @@ final class ManiphestTaskSearchEngine $query->withFullTextSearch($map['fulltext']); } + if (strlen($map['ferret'])) { + $query->withFerretConstraint( + id(new ManiphestTask())->newFerretEngine(), + $map['ferret']); + } + if ($map['parentIDs']) { $query->withParentTaskIDs($map['parentIDs']); } diff --git a/src/applications/maniphest/search/ManiphestTaskFerretEngine.php b/src/applications/maniphest/search/ManiphestTaskFerretEngine.php new file mode 100644 index 0000000000..7232d0251f --- /dev/null +++ b/src/applications/maniphest/search/ManiphestTaskFerretEngine.php @@ -0,0 +1,18 @@ +getPHID(); + $engine = $object->newFerretEngine(); + + $ferret_document = $engine->newDocumentObject() + ->setObjectPHID($phid) + ->setIsClosed(0) + ->setEpochCreated(0) + ->setEpochModified(0); + + $stemmer = new PhutilSearchStemmer(); + + $ferret_fields = array(); + $ngrams_source = array(); + foreach ($document->getFieldData() as $field) { + list($key, $raw_corpus) = $field; + + if (!strlen($raw_corpus)) { + continue; + } + + $normal_corpus = $stemmer->stemCorpus($raw_corpus); + + $ferret_fields[] = $engine->newFieldObject() + ->setFieldKey($key) + ->setRawCorpus($raw_corpus) + ->setNormalCorpus($normal_corpus); + + $ngrams_source[] = $raw_corpus; + } + $ngrams_source = implode(' ', $ngrams_source); + + $ngrams = id(new PhabricatorNgramEngine()) + ->getNgramsFromString($ngrams_source, 'index'); + + $ferret_document->openTransaction(); + $this->deleteOldDocument($engine, $object, $document); + + $ferret_document->save(); + + $document_id = $ferret_document->getID(); + foreach ($ferret_fields as $ferret_field) { + $ferret_field + ->setDocumentID($document_id) + ->save(); + } + + $ferret_ngrams = $engine->newNgramsObject(); + $conn = $ferret_ngrams->establishConnection('w'); + + $sql = array(); + foreach ($ngrams as $ngram) { + $sql[] = qsprintf( + $conn, + '(%d, %s)', + $document_id, + $ngram); + } + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT INTO %T (documentID, ngram) VALUES %Q', + $ferret_ngrams->getTableName(), + $chunk); + } + $ferret_document->saveTransaction(); + } + + + private function deleteOldDocument( + PhabricatorFerretEngine $engine, + $object, + PhabricatorSearchAbstractDocument $document) { + + $old_document = $engine->newDocumentObject()->loadOneWhere( + 'objectPHID = %s', + $document->getPHID()); + if (!$old_document) { + return; + } + + $conn = $old_document->establishConnection('w'); + $old_id = $old_document->getID(); + + queryfx( + $conn, + 'DELETE FROM %T WHERE id = %d', + $engine->newDocumentObject()->getTableName(), + $old_id); + + queryfx( + $conn, + 'DELETE FROM %T WHERE documentID = %d', + $engine->newFieldObject()->getTableName(), + $old_id); + + queryfx( + $conn, + 'DELETE FROM %T WHERE documentID = %d', + $engine->newNgramsObject()->getTableName(), + $old_id); + } + +} diff --git a/src/applications/search/ferret/PhabricatorFerretDocument.php b/src/applications/search/ferret/PhabricatorFerretDocument.php new file mode 100644 index 0000000000..fa816c8d17 --- /dev/null +++ b/src/applications/search/ferret/PhabricatorFerretDocument.php @@ -0,0 +1,40 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'isClosed' => 'bool', + 'authorPHID' => 'phid?', + 'ownerPHID' => 'phid?', + 'epochCreated' => 'epoch', + 'epochModified' => 'epoch', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_object' => array( + 'columns' => array('objectPHID'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + public function getTableName() { + $application = $this->getApplicationName(); + $key = $this->getIndexKey(); + return "{$application}_{$key}_fdocument"; + } + +} diff --git a/src/applications/search/ferret/PhabricatorFerretEngine.php b/src/applications/search/ferret/PhabricatorFerretEngine.php new file mode 100644 index 0000000000..e816ef3f59 --- /dev/null +++ b/src/applications/search/ferret/PhabricatorFerretEngine.php @@ -0,0 +1,9 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'documentID' => 'uint32', + 'fieldKey' => 'text4', + 'rawCorpus' => 'sort', + 'normalCorpus' => 'sort', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_document' => array( + 'columns' => array('documentID', 'fieldKey'), + ), + ), + ) + parent::getConfiguration(); + } + + public function getTableName() { + $application = $this->getApplicationName(); + $key = $this->getIndexKey(); + return "{$application}_{$key}_ffield"; + } + +} diff --git a/src/applications/search/ferret/PhabricatorFerretInterface.php b/src/applications/search/ferret/PhabricatorFerretInterface.php new file mode 100644 index 0000000000..cdb651b6cf --- /dev/null +++ b/src/applications/search/ferret/PhabricatorFerretInterface.php @@ -0,0 +1,7 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'documentID' => 'uint32', + 'ngram' => 'char3', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_ngram' => array( + 'columns' => array('ngram', 'documentID'), + ), + 'key_object' => array( + 'columns' => array('documentID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function getTableName() { + $application = $this->getApplicationName(); + $key = $this->getIndexKey(); + return "{$application}_{$key}_fngrams"; + } + +} diff --git a/src/applications/search/ngrams/PhabricatorNgramEngine.php b/src/applications/search/ngrams/PhabricatorNgramEngine.php new file mode 100644 index 0000000000..e168a86127 --- /dev/null +++ b/src/applications/search/ngrams/PhabricatorNgramEngine.php @@ -0,0 +1,41 @@ +tokenizeString($value); + + $ngrams = array(); + foreach ($tokens as $token) { + $token = phutil_utf8_strtolower($token); + + switch ($mode) { + case 'query': + break; + case 'index': + $token = ' '.$token.' '; + break; + case 'prefix': + $token = ' '.$token; + break; + } + + $len = (strlen($token) - 2); + for ($ii = 0; $ii < $len; $ii++) { + $ngram = substr($token, $ii, 3); + $ngrams[$ngram] = $ngram; + } + } + + ksort($ngrams); + + return array_keys($ngrams); + } + +} diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 3ef2a72e6f..c0b2bbc100 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -27,6 +27,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $spacePHIDs; private $spaceIsArchived; private $ngrams = array(); + private $ferretEngine; + private $ferretConstraints; protected function getPageCursors(array $page) { return array( @@ -270,6 +272,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $joins[] = $this->buildEdgeLogicJoinClause($conn); $joins[] = $this->buildApplicationSearchJoinClause($conn); $joins[] = $this->buildNgramsJoinClause($conn); + $joins[] = $this->buildFerretJoinClause($conn); return $joins; } @@ -292,6 +295,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $where[] = $this->buildEdgeLogicWhereClause($conn); $where[] = $this->buildSpacesWhereClause($conn); $where[] = $this->buildNgramsWhereClause($conn); + $where[] = $this->buildFerretWhereClause($conn); return $where; } @@ -346,6 +350,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return true; } + if ($this->shouldGroupFerretResultRows()) { + return true; + } + return false; } @@ -1373,6 +1381,150 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } +/* -( Ferret )------------------------------------------------------------- */ + + + public function withFerretConstraint( + PhabricatorFerretEngine $engine, + $raw_query) { + + if ($this->ferretEngine) { + throw new Exception( + pht( + 'Query may not have multiple fulltext constraints.')); + } + + if (!strlen($raw_query)) { + return $this; + } + + $this->ferretEngine = $engine; + $this->ferretConstraints = preg_split('/\s+/', $raw_query); + + return $this; + } + + protected function buildFerretJoinClause(AphrontDatabaseConnection $conn) { + if (!$this->ferretEngine) { + return array(); + } + + $engine = $this->ferretEngine; + $ngram_engine = new PhabricatorNgramEngine(); + + $ngram_table = $engine->newNgramsObject(); + $ngram_table_name = $ngram_table->getTableName(); + + $flat = array(); + foreach ($this->ferretConstraints as $term) { + $value = $term; + $length = count(phutil_utf8v($term)); + + if ($length >= 3) { + $ngrams = $ngram_engine->getNgramsFromString($value, 'query'); + $prefix = false; + } else if ($length == 2) { + $ngrams = $ngram_engine->getNgramsFromString($value, 'prefix'); + $prefix = false; + } else { + $ngrams = array(' '.$value); + $prefix = true; + } + + foreach ($ngrams as $ngram) { + $flat[] = array( + 'table' => $ngram_table_name, + 'ngram' => $ngram, + 'prefix' => $prefix, + ); + } + } + + // MySQL only allows us to join a maximum of 61 tables per query. Each + // ngram is going to cost us a join toward that limit, so if the user + // specified a very long query string, just pick 16 of the ngrams + // at random. + if (count($flat) > 16) { + shuffle($flat); + $flat = array_slice($flat, 0, 16); + } + + $alias = $this->getPrimaryTableAlias(); + if ($alias) { + $phid_column = qsprintf($conn, '%T.%T', $alias, 'phid'); + } else { + $phid_column = qsprintf($conn, '%T', 'phid'); + } + + $document_table = $engine->newDocumentObject(); + $field_table = $engine->newFieldObject(); + + $joins = array(); + $joins[] = qsprintf( + $conn, + 'JOIN %T ftdoc ON ftdoc.objectPHID = %Q', + $document_table->getTableName(), + $phid_column); + + $idx = 1; + foreach ($flat as $spec) { + $table = $spec['table']; + $ngram = $spec['ngram']; + $prefix = $spec['prefix']; + + $alias = 'ft'.$idx++; + + if ($prefix) { + $joins[] = qsprintf( + $conn, + 'JOIN %T %T ON %T.documentID = ftdoc.id AND %T.ngram LIKE %>', + $table, + $alias, + $alias, + $alias, + $ngram); + } else { + $joins[] = qsprintf( + $conn, + 'JOIN %T %T ON %T.documentID = ftdoc.id AND %T.ngram = %s', + $table, + $alias, + $alias, + $alias, + $ngram); + } + } + + $joins[] = qsprintf( + $conn, + 'JOIN %T ftfield ON ftdoc.id = ftfield.documentID', + $field_table->getTableName()); + + return $joins; + } + + protected function buildFerretWhereClause(AphrontDatabaseConnection $conn) { + if (!$this->ferretEngine) { + return array(); + } + + $where = array(); + foreach ($this->ferretConstraints as $constraint) { + $where[] = qsprintf( + $conn, + '(ftfield.rawCorpus LIKE %~ OR ftfield.normalCorpus LIKE %~)', + $constraint, + $constraint); + } + + return $where; + } + + protected function shouldGroupFerretResultRows() { + return (bool)$this->ferretConstraints; + } + + /* -( Ngrams )------------------------------------------------------------- */ From 0609133f4564c7d86186c478a4110b0118647e0e Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 28 Aug 2017 15:10:25 -0700 Subject: [PATCH 06/24] Limit notifications to latest 10, instead of 15 Summary: This panel just gets super tall at 15 now that date is on it's own line. Test Plan: Reload panel, count to 10. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18486 --- .../controller/PhabricatorNotificationPanelController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/notification/controller/PhabricatorNotificationPanelController.php b/src/applications/notification/controller/PhabricatorNotificationPanelController.php index be7eeb914e..3e30ead9e7 100644 --- a/src/applications/notification/controller/PhabricatorNotificationPanelController.php +++ b/src/applications/notification/controller/PhabricatorNotificationPanelController.php @@ -9,7 +9,7 @@ final class PhabricatorNotificationPanelController $query = id(new PhabricatorNotificationQuery()) ->setViewer($viewer) ->withUserPHIDs(array($viewer->getPHID())) - ->setLimit(15); + ->setLimit(10); $stories = $query->execute(); From 4005a465f7d09378f5f0adf32a08161dea58a90e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Aug 2017 15:37:56 -0700 Subject: [PATCH 07/24] Make Ferret indexing more robust (UTF8, exception handling) Summary: Ref T12819. Two minor improvements from live data: - Tokenize in a UTF8-aware way. - When one document fails to index, kill the transaction explicitly (rather than leaving it hanging) so we don't cause other failures later. Test Plan: Created some UTF8 documents locally, indexed them, got clean results. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18487 --- .../PhabricatorFerretFulltextEngineExtension.php | 7 +++++++ src/applications/search/ngrams/PhabricatorNgramEngine.php | 6 ++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index bd67f3132c..04d2fad608 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -55,6 +55,8 @@ final class PhabricatorFerretFulltextEngineExtension ->getNgramsFromString($ngrams_source, 'index'); $ferret_document->openTransaction(); + + try { $this->deleteOldDocument($engine, $object, $document); $ferret_document->save(); @@ -85,6 +87,11 @@ final class PhabricatorFerretFulltextEngineExtension $ferret_ngrams->getTableName(), $chunk); } + } catch (Exception $ex) { + $ferret_document->killTransaction(); + throw $ex; + } + $ferret_document->saveTransaction(); } diff --git a/src/applications/search/ngrams/PhabricatorNgramEngine.php b/src/applications/search/ngrams/PhabricatorNgramEngine.php index e168a86127..87abdfc446 100644 --- a/src/applications/search/ngrams/PhabricatorNgramEngine.php +++ b/src/applications/search/ngrams/PhabricatorNgramEngine.php @@ -26,9 +26,11 @@ final class PhabricatorNgramEngine extends Phobject { break; } - $len = (strlen($token) - 2); + $token_v = phutil_utf8v($token); + $len = (count($token_v) - 2); for ($ii = 0; $ii < $len; $ii++) { - $ngram = substr($token, $ii, 3); + $ngram = array_slice($token_v, $ii, 3); + $ngram = implode('', $ngram); $ngrams[$ngram] = $ngram; } } From 81f42b834366a12c51dacdcdcdd68596f2e32cf5 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 28 Aug 2017 16:07:22 -0700 Subject: [PATCH 08/24] Align sidenavs better, use sky to highlight Summary: Simplifies the UI here by removing various borders, instead using just background colors and better alignment. Test Plan: Test instances, settings, home, projects, workboards. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18488 --- resources/celerity/map.php | 10 +++++----- webroot/rsrc/css/phui/phui-basic-nav-view.css | 18 ++++-------------- webroot/rsrc/css/phui/phui-two-column-view.css | 14 +++++++++++--- 3 files changed, 20 insertions(+), 22 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9aac86c370..0ecfea7dfd 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' => 'b324663c', + 'core.pkg.css' => '2bee293d', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -141,7 +141,7 @@ return array( 'rsrc/css/phui/phui-action-list.css' => 'e7eba156', 'rsrc/css/phui/phui-action-panel.css' => 'b4798122', 'rsrc/css/phui/phui-badge.css' => '22c0cf4f', - 'rsrc/css/phui/phui-basic-nav-view.css' => 'a0705f53', + 'rsrc/css/phui/phui-basic-nav-view.css' => '98c11ab3', 'rsrc/css/phui/phui-big-info-view.css' => 'acc3492c', 'rsrc/css/phui/phui-box.css' => '745e881d', 'rsrc/css/phui/phui-chart.css' => '6bf6f78e', @@ -178,7 +178,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' => '76dcd3d4', + 'rsrc/css/phui/phui-two-column-view.css' => 'bf86c483', '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', @@ -820,7 +820,7 @@ return array( 'phriction-document-css' => '4282e4ad', 'phui-action-panel-css' => 'b4798122', 'phui-badge-view-css' => '22c0cf4f', - 'phui-basic-nav-view-css' => 'a0705f53', + 'phui-basic-nav-view-css' => '98c11ab3', 'phui-big-info-view-css' => 'acc3492c', 'phui-box-css' => '745e881d', 'phui-button-bar-css' => 'f1ff5494', @@ -874,7 +874,7 @@ return array( 'phui-tag-view-css' => 'b4719c50', 'phui-theme-css' => '9f261c6b', 'phui-timeline-view-css' => 'f21db7ca', - 'phui-two-column-view-css' => '76dcd3d4', + 'phui-two-column-view-css' => 'bf86c483', 'phui-workboard-color-css' => '783cdff5', 'phui-workboard-view-css' => '3bc85455', 'phui-workcard-view-css' => 'cca5fa92', diff --git a/webroot/rsrc/css/phui/phui-basic-nav-view.css b/webroot/rsrc/css/phui/phui-basic-nav-view.css index 70882b0afd..a545ad35ce 100644 --- a/webroot/rsrc/css/phui/phui-basic-nav-view.css +++ b/webroot/rsrc/css/phui/phui-basic-nav-view.css @@ -64,7 +64,7 @@ .phui-basic-nav .phabricator-side-menu .phui-list-item-href { display: block; - padding: 6px 8px 6px 20px; + padding: 6px 8px 6px 12px; color: {$darkbluetext}; border-top-right-radius: 3px; border-bottom-right-radius: 3px; @@ -72,13 +72,8 @@ text-overflow: ellipsis } -.phui-basic-nav .phabricator-side-menu .phui-list-item-has-icon - .phui-list-item-href { - padding-left: 12px; - } - .phui-basic-nav .phabricator-side-menu .phui-list-item-icon { - margin-left: -4px; + margin-left: -8px; text-align: center; width: 30px; } @@ -100,7 +95,6 @@ .phui-basic-nav .phabricator-side-menu .phui-list-item-selected { background-color: rgba({$alphablack},.05); - border-left: 4px solid {$sky}; border-top-right-radius: 3px; border-bottom-right-radius: 3px; font-weight: bold; @@ -109,12 +103,7 @@ .device-desktop .phui-basic-nav .phabricator-side-menu .phui-list-item-selected a.phui-list-item-href:hover { - background-color: rgba({$alphablack},.05); -} - -.phui-basic-nav .phabricator-side-menu .phui-list-item-selected - .phui-list-item-href { - margin-left: -4px; + background-color: rgba({$alphablack},.05); } .phui-basic-nav .phabricator-side-menu .phui-list-item-type-label { @@ -124,6 +113,7 @@ font-size: 12px; font-weight: bold; border-style: solid; + letter-spacing: 0.02em; } .device-desktop .phui-basic-nav .phabricator-side-menu diff --git a/webroot/rsrc/css/phui/phui-two-column-view.css b/webroot/rsrc/css/phui/phui-two-column-view.css index dba1519597..73926be92a 100644 --- a/webroot/rsrc/css/phui/phui-two-column-view.css +++ b/webroot/rsrc/css/phui/phui-two-column-view.css @@ -281,9 +281,7 @@ .phui-two-column-fixed.phui-two-column-view .phui-basic-nav .phabricator-side-menu .phui-list-item-selected { border-radius: 3px; - background-color: #f5f9ff; - border: 1px solid {$sky}; - padding-left: 3px; + background-color: {$sky}; } .phui-two-column-fixed.phui-two-column-view .phui-basic-nav @@ -291,6 +289,16 @@ border-radius: 3px; } +.phui-two-column-fixed.phui-two-column-view .phui-basic-nav + .phabricator-side-menu .phui-list-item-selected a { + color: #fff; +} + +.phui-two-column-fixed.phui-two-column-view .phui-basic-nav + .phabricator-side-menu .phui-list-item-selected a .phui-icon-view { + color: #fff; +} + .phui-two-column-fixed.phui-two-column-view .phui-header-action-links .phui-mobile-menu { display: block; From f3f671aa90ec9943a1af2d27126c265f76a99772 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Tue, 29 Aug 2017 09:36:33 -0700 Subject: [PATCH 09/24] Align first nav item in settings Summary: This removes the redundant "Account" label and item, and just keeps the page better aligned. Test Plan: Review personal settings Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18489 --- .../controller/PhabricatorSettingsMainController.php | 5 ++++- .../panelgroup/PhabricatorSettingsAccountPanelGroup.php | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/settings/controller/PhabricatorSettingsMainController.php b/src/applications/settings/controller/PhabricatorSettingsMainController.php index 1e4d4a0b70..9dc84a9bd0 100644 --- a/src/applications/settings/controller/PhabricatorSettingsMainController.php +++ b/src/applications/settings/controller/PhabricatorSettingsMainController.php @@ -205,7 +205,10 @@ final class PhabricatorSettingsMainController if ($panel->getPanelGroupKey() != $group_key) { $group_key = $panel->getPanelGroupKey(); $group = $panel->getPanelGroup(); - $nav->addLabel($group->getPanelGroupName()); + $panel_name = $group->getPanelGroupName(); + if ($panel_name) { + $nav->addLabel($panel_name); + } } $nav->addFilter($panel->getPanelKey(), $panel->getPanelName()); diff --git a/src/applications/settings/panelgroup/PhabricatorSettingsAccountPanelGroup.php b/src/applications/settings/panelgroup/PhabricatorSettingsAccountPanelGroup.php index f81d5d487a..826118b881 100644 --- a/src/applications/settings/panelgroup/PhabricatorSettingsAccountPanelGroup.php +++ b/src/applications/settings/panelgroup/PhabricatorSettingsAccountPanelGroup.php @@ -6,7 +6,7 @@ final class PhabricatorSettingsAccountPanelGroup const PANELGROUPKEY = 'account'; public function getPanelGroupName() { - return pht('Account'); + return null; } protected function getPanelGroupOrder() { From f49d103af5d32a1c1772652060ea6ebc8c5f82e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Aug 2017 09:54:03 -0700 Subject: [PATCH 10/24] Fix an issue where "Close Revision" did not appear in the UI Summary: Ref T2543. When called from the UI to build the dropdown, there's no Editor, since we aren't actually in an edit flow. This logic worked for actually performing the edits, just not for getting the option into the dropdown. Test Plan: Used the dropdown to close an "Accepted" revision which I authored. Reviewers: chad Reviewed By: chad Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18490 --- .../xaction/DifferentialRevisionCloseTransaction.php | 10 ++++++---- .../storage/PhabricatorModularTransactionType.php | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index 8d01f48eff..d71b30950a 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -46,10 +46,12 @@ final class DifferentialRevisionCloseTransaction } protected function validateAction($object, PhabricatorUser $viewer) { - if ($this->getEditor()->getIsCloseByCommit()) { - // If we're closing a revision because we discovered a commit, we don't - // care what state it was in. - return; + if ($this->hasEditor()) { + if ($this->getEditor()->getIsCloseByCommit()) { + // If we're closing a revision because we discovered a commit, we don't + // care what state it was in. + return; + } } if ($object->isClosed()) { diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index 119bfedd32..f2b59c8a2b 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -134,6 +134,10 @@ abstract class PhabricatorModularTransactionType return $this->editor; } + final protected function hasEditor() { + return (bool)$this->editor; + } + final protected function getAuthorPHID() { return $this->getStorage()->getAuthorPHID(); } From b4cbea901845087f8903bdcd210303d7e6eace50 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Aug 2017 12:42:58 -0700 Subject: [PATCH 11/24] Make legacy revision statuses from "differential.query" have type "string" again Summary: Ref T2543. The type on these got changed by accident, it should be "string" (crazy nonsense, compatible) not "int" (sensible, not compatible). (New API uses sensible strings like "accepted" only.) Test Plan: Called `differential.query` from web UI, saw `"2"` and similar statuses. Reviewers: chad, jmeador, lvital Reviewed By: jmeador, lvital Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18493 --- .../conduit/DifferentialQueryConduitAPIMethod.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php index 0b26db0655..eeef36ee69 100644 --- a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php @@ -218,7 +218,11 @@ final class DifferentialQueryConduitAPIMethod 'dateCreated' => $revision->getDateCreated(), 'dateModified' => $revision->getDateModified(), 'authorPHID' => $revision->getAuthorPHID(), - 'status' => $revision->getLegacyRevisionStatus(), + + // NOTE: For backward compatibility this is explicitly a string, like + // "2", even though the value of the string is an integer. See PHI14. + 'status' => (string)$revision->getLegacyRevisionStatus(), + 'statusName' => $revision->getStatusDisplayName(), 'properties' => $revision->getProperties(), 'branch' => $diff->getBranch(), From 11046d495dbf31c36dcc1488732cad8f4aebbafa Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 30 Aug 2017 10:00:23 -0700 Subject: [PATCH 12/24] Add a selected button ui state Summary: Only for grey buttons, but can expand. Sets a selected class. Test Plan: Review new changes in UIExamples. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18501 --- resources/celerity/map.php | 18 +++++++++--------- .../uiexample/examples/PHUIButtonExample.php | 4 ++++ src/view/phui/PHUIButtonView.php | 10 ++++++++++ webroot/rsrc/css/phui/button/phui-button.css | 13 +++++++++++++ webroot/rsrc/js/phuix/PHUIXButtonView.js | 8 ++++++++ 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0ecfea7dfd..1fefbd5e67 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' => '2bee293d', + 'core.pkg.css' => '0437a674', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -127,7 +127,7 @@ return array( 'rsrc/css/layout/phabricator-source-code-view.css' => 'aea41829', 'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494', 'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68', - 'rsrc/css/phui/button/phui-button.css' => 'a37aa3a8', + 'rsrc/css/phui/button/phui-button.css' => '1863cc6e', 'rsrc/css/phui/calendar/phui-calendar-day.css' => '572b1893', 'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600', 'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf', @@ -529,7 +529,7 @@ return array( 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => '442efd08', 'rsrc/js/phuix/PHUIXAutocomplete.js' => '4b7430ab', - 'rsrc/js/phuix/PHUIXButtonView.js' => 'a37126bd', + 'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50', 'rsrc/js/phuix/PHUIXExample.js' => '68af71ca', 'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671', @@ -824,7 +824,7 @@ return array( 'phui-big-info-view-css' => 'acc3492c', 'phui-box-css' => '745e881d', 'phui-button-bar-css' => 'f1ff5494', - 'phui-button-css' => 'a37aa3a8', + 'phui-button-css' => '1863cc6e', 'phui-button-simple-css' => '8e1baf68', 'phui-calendar-css' => 'f1ddf11c', 'phui-calendar-day-css' => '572b1893', @@ -882,7 +882,7 @@ return array( 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => '442efd08', 'phuix-autocomplete' => '4b7430ab', - 'phuix-button-view' => 'a37126bd', + 'phuix-button-view' => '8a91e1ac', 'phuix-dropdown-menu' => '8018ee50', 'phuix-form-control-view' => '83e03671', 'phuix-icon-view' => 'bff6884b', @@ -1557,6 +1557,10 @@ return array( 'javelin-install', 'javelin-dom', ), + '8a91e1ac' => array( + 'javelin-install', + 'javelin-dom', + ), '8ce821c5' => array( 'phabricator-notification', 'javelin-stratcom', @@ -1696,10 +1700,6 @@ return array( 'javelin-sound', 'phabricator-notification', ), - 'a37126bd' => array( - 'javelin-install', - 'javelin-dom', - ), 'a3a63478' => array( 'phui-workcard-view-css', ), diff --git a/src/applications/uiexample/examples/PHUIButtonExample.php b/src/applications/uiexample/examples/PHUIButtonExample.php index f6e8fb526c..89f8e15fe8 100644 --- a/src/applications/uiexample/examples/PHUIButtonExample.php +++ b/src/applications/uiexample/examples/PHUIButtonExample.php @@ -83,9 +83,11 @@ final class PHUIButtonExample extends PhabricatorUIExample { ), array( 'icon' => 'fa-upload', + 'disabled' => true, ), array( 'icon' => 'fa-street-view', + 'selected' => true, ), array( 'text' => pht('Copy "Quack" to Clipboard'), @@ -99,6 +101,8 @@ final class PHUIButtonExample extends PhabricatorUIExample { ->setColor(PHUIButtonView::GREY) ->setIcon(idx($spec, 'icon')) ->setText(idx($spec, 'text')) + ->setSelected(idx($spec, 'selected')) + ->setDisabled(idx($spec, 'disabled')) ->addClass(PHUI::MARGIN_SMALL_RIGHT) ->setDropdown(idx($spec, 'dropdown')); diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 8d6eca9ec7..91c336eaaf 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -25,6 +25,7 @@ final class PHUIButtonView extends AphrontTagView { private $href = null; private $title = null; private $disabled; + private $selected; private $name; private $tooltip; private $noCSS; @@ -74,6 +75,11 @@ final class PHUIButtonView extends AphrontTagView { return $this; } + public function setSelected($selected) { + $this->selected = $selected; + return $this; + } + public function setTag($tag) { $this->tag = $tag; return $this; @@ -189,6 +195,10 @@ final class PHUIButtonView extends AphrontTagView { $classes[] = 'disabled'; } + if ($this->selected) { + $classes[] = 'selected'; + } + switch ($this->getButtonType()) { case self::BUTTONTYPE_DEFAULT: $classes[] = 'phui-button-default'; diff --git a/webroot/rsrc/css/phui/button/phui-button.css b/webroot/rsrc/css/phui/button/phui-button.css index 99fef3707c..14bbdcf46b 100644 --- a/webroot/rsrc/css/phui/button/phui-button.css +++ b/webroot/rsrc/css/phui/button/phui-button.css @@ -105,6 +105,19 @@ button[disabled] { opacity: 0.5; } +button.button-grey.selected, +a.button.button-grey.selected, +button.button-grey.selected:hover, +a.button.button-grey.selected:hover { + border-color: {$sh-orangetext}; + color: {$sh-orangetext}; +} + +button.button-grey.selected .phui-icon-view, +a.button-grey.selected .phui-icon-view { + color: {$sh-orangetext}; +} + a.phuix-dropdown-open { color: {$greytext}; } diff --git a/webroot/rsrc/js/phuix/PHUIXButtonView.js b/webroot/rsrc/js/phuix/PHUIXButtonView.js index e87db88fe4..b7ca44a677 100644 --- a/webroot/rsrc/js/phuix/PHUIXButtonView.js +++ b/webroot/rsrc/js/phuix/PHUIXButtonView.js @@ -16,6 +16,7 @@ JX.install('PHUIXButtonView', { _iconView: null, _color: null, + _selected: null, _buttonType: null, setIcon: function(icon) { @@ -43,6 +44,13 @@ JX.install('PHUIXButtonView', { return this; }, + setSelected: function(selected) { + var node = this.getNode(); + this._selected = selected; + JX.DOM.alterClass(node, 'selected', this._selected); + return this; + }, + setButtonType: function(button_type) { var self = JX.PHUIXButtonView; From 72cb3d3c84905c0d75074e4ecf74c493e3a2d527 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 11:08:50 -0700 Subject: [PATCH 13/24] Limit the damage that degenerate project name typeahead queries can cause Summary: See PHI47. When users copy/paste a wall of text into a project tokenizer, we can end up performing a very large number of JOINs. These JOINs seem okay locally and on `secure`, but the install in PHI47 reports hitting issues. Since these queries are almost certainly illegitimate (I think no one uses 5+ words to find a project), just limit the search to the 5 longest tokens. Note that typing 6 tokens will still almost always work, since the UI does additional filtering. However, if you have 100+ projects named "a b c d e ..." and search for "a b c d e z", you may not hit it. This is so degenerate that it's hard to imagine any users encountering it. This is a stopgap fix, I'll file something longer-term as a followup. Test Plan: Used `/typeahead/class/PhabricatorProjectDatasource/` to run queries. Saw the same results with shorter query plans for all reasonable queries. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18506 --- .../project/query/PhabricatorProjectQuery.php | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index 780f072678..955eba9972 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -609,7 +609,8 @@ final class PhabricatorProjectQuery } if ($this->nameTokens !== null) { - foreach ($this->nameTokens as $key => $token) { + $name_tokens = $this->getNameTokensForQuery($this->nameTokens); + foreach ($name_tokens as $key => $token) { $token_table = 'token_'.$key; $joins[] = qsprintf( $conn, @@ -797,4 +798,22 @@ final class PhabricatorProjectQuery } } + private function getNameTokensForQuery(array $tokens) { + // When querying for projects by name, only actually search for the five + // longest tokens. MySQL can get grumpy with a large number of JOINs + // with LIKEs and queries for more than 5 tokens are essentially never + // legitimate searches for projects, but users copy/pasting nonsense. + // See also PHI47. + + $length_map = array(); + foreach ($tokens as $token) { + $length_map[$token] = strlen($token); + } + arsort($length_map); + + $length_map = array_slice($length_map, 0, 5, true); + + return array_keys($length_map); + } + } From 77ef38f9a87f342721538bf4c4fabdd26b7d854b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 07:04:16 -0700 Subject: [PATCH 14/24] Aggregate corpus data in Ferret field rows Summary: Ref T12819. This addresses two issues: - One practical issue is that right now, if you search for "dog cat", and they appear in different fields (for example, "dog" appears ONLY in the title, while "cat" appears ONLY in a comment) we won't find the document. This is somewhat rare -- usually, if "dog" appears in the title, it's also repeated in the description -- but I think clearly a bug. To attack this, start automatically creating a virtual "ALL" field with the full document text which we'll use as the primary thing we match against. - For fields which may occur more than once -- today, only comments -- aggregate them all into one big "all of the text" row instead of writing one row per comment. This partly addresses the first point ("dog" in one comment and "cat" in a different comment won't be found) and partly makes some of the query gymnastics easier. Test Plan: Ran `bin/storage upgrade`, ran `bin/search index `, saw sensible corpus values in the database: ``` mysql> select * from maniphest_task_ffield\G *************************** 1. row *************************** id: 3 documentID: 1981 fieldKey: full rawCorpus: This is the task title This is the task description. normalCorpus: thi the task titl thi the task descript *************************** 2. row *************************** id: 4 documentID: 1981 fieldKey: titl rawCorpus: This is the task title normalCorpus: thi the task titl *************************** 3. row *************************** id: 5 documentID: 1981 fieldKey: body rawCorpus: This is the task description. normalCorpus: thi the task descript 3 rows in set (0.00 sec) ``` Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18497 --- .../autopatches/20170830.ferret.01.unique.sql | 4 ++ .../PhabricatorSearchDocumentFieldType.php | 1 + ...abricatorFerretFulltextEngineExtension.php | 39 ++++++++++++++++--- .../search/ferret/PhabricatorFerretField.php | 3 +- 4 files changed, 40 insertions(+), 7 deletions(-) create mode 100644 resources/sql/autopatches/20170830.ferret.01.unique.sql diff --git a/resources/sql/autopatches/20170830.ferret.01.unique.sql b/resources/sql/autopatches/20170830.ferret.01.unique.sql new file mode 100644 index 0000000000..f76c5050e8 --- /dev/null +++ b/resources/sql/autopatches/20170830.ferret.01.unique.sql @@ -0,0 +1,4 @@ +TRUNCATE TABLE {$NAMESPACE}_maniphest.maniphest_task_ffield; + +ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task_ffield + ADD UNIQUE KEY `key_documentfield` (documentID, fieldKey); diff --git a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php index 10dbf0ca65..4dd49e8c92 100644 --- a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php +++ b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php @@ -5,5 +5,6 @@ final class PhabricatorSearchDocumentFieldType extends Phobject { const FIELD_TITLE = 'titl'; const FIELD_BODY = 'body'; const FIELD_COMMENT = 'cmnt'; + const FIELD_ALL = 'full'; } diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index 04d2fad608..bafeca2c81 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -31,25 +31,52 @@ final class PhabricatorFerretFulltextEngineExtension $stemmer = new PhutilSearchStemmer(); - $ferret_fields = array(); - $ngrams_source = array(); + $key_all = PhabricatorSearchDocumentFieldType::FIELD_ALL; + + $empty_template = array( + 'raw' => array(), + 'normal' => array(), + ); + + $ferret_corpus_map = array( + $key_all => $empty_template, + ); + foreach ($document->getFieldData() as $field) { list($key, $raw_corpus) = $field; - if (!strlen($raw_corpus)) { continue; } $normal_corpus = $stemmer->stemCorpus($raw_corpus); + if (!isset($ferret_corpus_map[$key])) { + $ferret_corpus_map[$key] = $empty_template; + } + + $ferret_corpus_map[$key]['raw'][] = $raw_corpus; + $ferret_corpus_map[$key]['normal'][] = $normal_corpus; + + $ferret_corpus_map[$key_all]['raw'][] = $raw_corpus; + $ferret_corpus_map[$key_all]['normal'][] = $normal_corpus; + } + + $ferret_fields = array(); + foreach ($ferret_corpus_map as $key => $fields) { + $raw_corpus = $fields['raw']; + $raw_corpus = implode("\n", $raw_corpus); + + $normal_corpus = $fields['normal']; + $normal_corpus = implode("\n", $normal_corpus); + $ferret_fields[] = $engine->newFieldObject() ->setFieldKey($key) ->setRawCorpus($raw_corpus) ->setNormalCorpus($normal_corpus); - - $ngrams_source[] = $raw_corpus; } - $ngrams_source = implode(' ', $ngrams_source); + + $ngrams_source = $ferret_corpus_map[$key_all]['raw']; + $ngrams_source = implode("\n", $ngrams_source); $ngrams = id(new PhabricatorNgramEngine()) ->getNgramsFromString($ngrams_source, 'index'); diff --git a/src/applications/search/ferret/PhabricatorFerretField.php b/src/applications/search/ferret/PhabricatorFerretField.php index 5b2370ae8f..cd7e7c68d5 100644 --- a/src/applications/search/ferret/PhabricatorFerretField.php +++ b/src/applications/search/ferret/PhabricatorFerretField.php @@ -20,8 +20,9 @@ abstract class PhabricatorFerretField 'normalCorpus' => 'sort', ), self::CONFIG_KEY_SCHEMA => array( - 'key_document' => array( + 'key_documentfield' => array( 'columns' => array('documentID', 'fieldKey'), + 'unique' => true, ), ), ) + parent::getConfiguration(); From 0e2e525bb41a39af857c55232af54f2dcb1fb1c1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 07:32:18 -0700 Subject: [PATCH 15/24] Add a "terms" corpus to Ferret fields Summary: Ref T12819. Ferret currently does substring search, but this is not the default mode users expect: when you search for the "RICO" act, you do not expect to find documents containing "apRICOt" even though "RICO" is a substring. To support term search, index the corpus as a list of terms with puncutation removed and whitespace normalized so the engine can match against it. Test Plan: Ran `storage upgrade`, ran `search index`, saw sensible database results: ``` rawCorpus: This is the task description. Hark! Whom'st'dve eaten this "food" shall surely ~perish~?? #blessed normalCorpus: thi the task descript hark whom dve eaten food shall sure perish bless termCorpus: This is the task description Hark Whom'st'dve eaten this food shall surely perish blessed ``` Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18498 --- .../autopatches/20170830.ferret.02.term.sql | 2 + src/__phutil_library_map__.php | 2 + ...abricatorFerretFulltextEngineExtension.php | 15 +++++- .../search/ferret/PhabricatorFerretField.php | 2 + .../search/ngrams/PhabricatorNgramEngine.php | 52 +++++++++++++++++++ .../PhabricatorNgramEngineTestCase.php | 26 ++++++++++ 6 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 resources/sql/autopatches/20170830.ferret.02.term.sql create mode 100644 src/applications/search/ngrams/__tests__/PhabricatorNgramEngineTestCase.php diff --git a/resources/sql/autopatches/20170830.ferret.02.term.sql b/resources/sql/autopatches/20170830.ferret.02.term.sql new file mode 100644 index 0000000000..81a619d85d --- /dev/null +++ b/resources/sql/autopatches/20170830.ferret.02.term.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task_ffield + ADD termCorpus LONGTEXT NOT NULL COLLATE {$COLLATE_SORT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3edb31640b..ff30ddae98 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3206,6 +3206,7 @@ phutil_register_library_map(array( 'PhabricatorNavigationRemarkupRule' => 'infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php', 'PhabricatorNeverTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorNeverTriggerClock.php', 'PhabricatorNgramEngine' => 'applications/search/ngrams/PhabricatorNgramEngine.php', + 'PhabricatorNgramEngineTestCase' => 'applications/search/ngrams/__tests__/PhabricatorNgramEngineTestCase.php', 'PhabricatorNgramsIndexEngineExtension' => 'applications/search/engineextension/PhabricatorNgramsIndexEngineExtension.php', 'PhabricatorNgramsInterface' => 'applications/search/interface/PhabricatorNgramsInterface.php', 'PhabricatorNotificationBuilder' => 'applications/notification/builder/PhabricatorNotificationBuilder.php', @@ -8587,6 +8588,7 @@ phutil_register_library_map(array( 'PhabricatorNavigationRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorNeverTriggerClock' => 'PhabricatorTriggerClock', 'PhabricatorNgramEngine' => 'Phobject', + 'PhabricatorNgramEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorNgramsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'PhabricatorNgramsInterface' => 'PhabricatorIndexableInterface', 'PhabricatorNotificationBuilder' => 'Phobject', diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index bafeca2c81..6eb97e2b7c 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -30,11 +30,13 @@ final class PhabricatorFerretFulltextEngineExtension ->setEpochModified(0); $stemmer = new PhutilSearchStemmer(); + $ngram_engine = id(new PhabricatorNgramEngine()); $key_all = PhabricatorSearchDocumentFieldType::FIELD_ALL; $empty_template = array( 'raw' => array(), + 'term' => array(), 'normal' => array(), ); @@ -49,15 +51,18 @@ final class PhabricatorFerretFulltextEngineExtension } $normal_corpus = $stemmer->stemCorpus($raw_corpus); + $term_corpus = $ngram_engine->newTermsCorpus($raw_corpus); if (!isset($ferret_corpus_map[$key])) { $ferret_corpus_map[$key] = $empty_template; } $ferret_corpus_map[$key]['raw'][] = $raw_corpus; + $ferret_corpus_map[$key]['term'][] = $term_corpus; $ferret_corpus_map[$key]['normal'][] = $normal_corpus; $ferret_corpus_map[$key_all]['raw'][] = $raw_corpus; + $ferret_corpus_map[$key_all]['term'][] = $term_corpus; $ferret_corpus_map[$key_all]['normal'][] = $normal_corpus; } @@ -69,17 +74,23 @@ final class PhabricatorFerretFulltextEngineExtension $normal_corpus = $fields['normal']; $normal_corpus = implode("\n", $normal_corpus); + $term_corpus = $fields['term']; + $term_corpus = implode(' ', $term_corpus); + if (strlen($term_corpus)) { + $term_corpus = ' '.$term_corpus.' '; + } + $ferret_fields[] = $engine->newFieldObject() ->setFieldKey($key) ->setRawCorpus($raw_corpus) + ->setTermCorpus($term_corpus) ->setNormalCorpus($normal_corpus); } $ngrams_source = $ferret_corpus_map[$key_all]['raw']; $ngrams_source = implode("\n", $ngrams_source); - $ngrams = id(new PhabricatorNgramEngine()) - ->getNgramsFromString($ngrams_source, 'index'); + $ngrams = $ngram_engine->getNgramsFromString($ngrams_source, 'index'); $ferret_document->openTransaction(); diff --git a/src/applications/search/ferret/PhabricatorFerretField.php b/src/applications/search/ferret/PhabricatorFerretField.php index cd7e7c68d5..be39e745ed 100644 --- a/src/applications/search/ferret/PhabricatorFerretField.php +++ b/src/applications/search/ferret/PhabricatorFerretField.php @@ -6,6 +6,7 @@ abstract class PhabricatorFerretField protected $documentID; protected $fieldKey; protected $rawCorpus; + protected $termCorpus; protected $normalCorpus; abstract public function getIndexKey(); @@ -17,6 +18,7 @@ abstract class PhabricatorFerretField 'documentID' => 'uint32', 'fieldKey' => 'text4', 'rawCorpus' => 'sort', + 'termCorpus' => 'sort', 'normalCorpus' => 'sort', ), self::CONFIG_KEY_SCHEMA => array( diff --git a/src/applications/search/ngrams/PhabricatorNgramEngine.php b/src/applications/search/ngrams/PhabricatorNgramEngine.php index 87abdfc446..f8f55d8757 100644 --- a/src/applications/search/ngrams/PhabricatorNgramEngine.php +++ b/src/applications/search/ngrams/PhabricatorNgramEngine.php @@ -40,4 +40,56 @@ final class PhabricatorNgramEngine extends Phobject { return array_keys($ngrams); } + public function newTermsCorpus($raw_corpus) { + $term_corpus = strtr( + $raw_corpus, + array( + '!' => ' ', + '"' => ' ', + '#' => ' ', + '$' => ' ', + '%' => ' ', + '&' => ' ', + '(' => ' ', + ')' => ' ', + '*' => ' ', + '+' => ' ', + ',' => ' ', + '-' => ' ', + '/' => ' ', + ':' => ' ', + ';' => ' ', + '<' => ' ', + '=' => ' ', + '>' => ' ', + '?' => ' ', + '@' => ' ', + '[' => ' ', + '\\' => ' ', + ']' => ' ', + '^' => ' ', + '`' => ' ', + '{' => ' ', + '|' => ' ', + '}' => ' ', + '~' => ' ', + '.' => ' ', + '_' => ' ', + "\n" => ' ', + "\r" => ' ', + "\t" => ' ', + )); + + // NOTE: Single quotes divide terms only if they're at a word boundary. + // In contractions, like "whom'st've", the entire word is a single term. + $term_corpus = preg_replace('/(^| )[\']+/', ' ', $term_corpus); + $term_corpus = preg_replace('/[\']+( |$)/', ' ', $term_corpus); + + $term_corpus = preg_replace('/\s+/u', ' ', $term_corpus); + $term_corpus = trim($term_corpus, ' '); + + return $term_corpus; + } + + } diff --git a/src/applications/search/ngrams/__tests__/PhabricatorNgramEngineTestCase.php b/src/applications/search/ngrams/__tests__/PhabricatorNgramEngineTestCase.php new file mode 100644 index 0000000000..fccb6fb324 --- /dev/null +++ b/src/applications/search/ngrams/__tests__/PhabricatorNgramEngineTestCase.php @@ -0,0 +1,26 @@ + 'Hear ye hear ye', + "Thou whom'st've art worthy." => "Thou whom'st've art worthy", + 'Guaranteed to contain "food".' => 'Guaranteed to contain food', + 'http://example.org/path/to/file.jpg' => + 'http example org path to file jpg', + ); + + $engine = new PhabricatorNgramEngine(); + foreach ($map as $input => $expect) { + $actual = $engine->newTermsCorpus($input); + + $this->assertEqual( + $expect, + $actual, + pht('Terms corpus for: %s', $input)); + } + } + +} From e5a495f435105397f0100024feb50d48306151c6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 08:37:05 -0700 Subject: [PATCH 16/24] Parse raw Ferret queries into tokens before processing them Summary: Ref T12819. Depends on D18492. Instead of passing a raw query into the Query layer, parse it first. This allows the query layer to figure out which parts should be substring vs term match, and would allow the SearchEngine layer to do `author:...` eventually by picking it out before sending it to the Ferret engine. Test Plan: Ran some Ferret queries. They work like before, except that nonsense like `-+"quack"` raises an exception now. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18499 --- .../query/ManiphestTaskSearchEngine.php | 17 +++++++++++- ...PhabricatorCursorPagedPolicyAwareQuery.php | 27 +++++++++++-------- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 8eb4a416e3..956b8c168e 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -232,9 +232,24 @@ final class ManiphestTaskSearchEngine } if (strlen($map['ferret'])) { + $raw_query = $map['ferret']; + + $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; + } + $query->withFerretConstraint( id(new ManiphestTask())->newFerretEngine(), - $map['ferret']); + $fulltext_tokens); } if ($map['parentIDs']) { diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index c0b2bbc100..437ca0ce4f 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -28,7 +28,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $spaceIsArchived; private $ngrams = array(); private $ferretEngine; - private $ferretConstraints; + private $ferretTokens; protected function getPageCursors(array $page) { return array( @@ -1386,7 +1386,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery public function withFerretConstraint( PhabricatorFerretEngine $engine, - $raw_query) { + array $fulltext_tokens) { if ($this->ferretEngine) { throw new Exception( @@ -1394,12 +1394,12 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery 'Query may not have multiple fulltext constraints.')); } - if (!strlen($raw_query)) { + if (!$fulltext_tokens) { return $this; } $this->ferretEngine = $engine; - $this->ferretConstraints = preg_split('/\s+/', $raw_query); + $this->ferretTokens = $fulltext_tokens; return $this; } @@ -1416,9 +1416,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $ngram_table_name = $ngram_table->getTableName(); $flat = array(); - foreach ($this->ferretConstraints as $term) { - $value = $term; - $length = count(phutil_utf8v($term)); + foreach ($this->ferretTokens as $fulltext_token) { + $raw_token = $fulltext_token->getToken(); + $value = $raw_token->getValue(); + + $length = count(phutil_utf8v($value)); if ($length >= 3) { $ngrams = $ngram_engine->getNgramsFromString($value, 'query'); @@ -1509,19 +1511,22 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } $where = array(); - foreach ($this->ferretConstraints as $constraint) { + foreach ($this->ferretTokens as $fulltext_token) { + $raw_token = $fulltext_token->getToken(); + $value = $raw_token->getValue(); + $where[] = qsprintf( $conn, '(ftfield.rawCorpus LIKE %~ OR ftfield.normalCorpus LIKE %~)', - $constraint, - $constraint); + $value, + $value); } return $where; } protected function shouldGroupFerretResultRows() { - return (bool)$this->ferretConstraints; + return (bool)$this->ferretTokens; } From df9c24e750588ad9b9a6921665c8c1e0f26db461 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 09:24:57 -0700 Subject: [PATCH 17/24] Provide some "term vs substring" support for the Ferret engine Summary: Ref T12819. Distinguishes between "term" queries and "substring" queries, and tries to match them correctly most of the time. For example: - `example` matches "example", obviously. - `~amp` matches "example", but `amp` does not. - `examples` matches "example" through stemming. - `"examples"` does not match "example" (quoted text does not stem). - `"an examp"` does not match "an example" (quoted text is still term text). - `~"an examp"` matches "an example" (quoted, substring-operator text uses substring search). Test Plan: Ran searches similar to the above, they seemed to do what they should. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18500 --- .../query/ManiphestTaskSearchEngine.php | 10 +- ...abricatorFerretFulltextEngineExtension.php | 17 ++- ...PhabricatorCursorPagedPolicyAwareQuery.php | 140 +++++++++++++----- 3 files changed, 124 insertions(+), 43 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 956b8c168e..0e3e0db69f 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -92,8 +92,8 @@ final class ManiphestTaskSearchEngine ->setLabel(pht('Contains Words')) ->setKey('fulltext'), id(new PhabricatorSearchTextField()) - ->setLabel(pht('Matches (Prototype)')) - ->setKey('ferret') + ->setLabel(pht('Query (Prototype)')) + ->setKey('query') ->setIsHidden($hide_ferret), id(new PhabricatorSearchThreeStateField()) ->setLabel(pht('Open Parents')) @@ -150,8 +150,8 @@ final class ManiphestTaskSearchEngine 'statuses', 'priorities', 'subtypes', + 'query', 'fulltext', - 'ferret', 'hasParents', 'hasSubtasks', 'parentIDs', @@ -231,8 +231,8 @@ final class ManiphestTaskSearchEngine $query->withFullTextSearch($map['fulltext']); } - if (strlen($map['ferret'])) { - $raw_query = $map['ferret']; + if (strlen($map['query'])) { + $raw_query = $map['query']; $compiler = id(new PhutilSearchQueryCompiler()) ->setEnableFunctions(true); diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index 6eb97e2b7c..77599b9d5d 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -50,9 +50,11 @@ final class PhabricatorFerretFulltextEngineExtension continue; } - $normal_corpus = $stemmer->stemCorpus($raw_corpus); $term_corpus = $ngram_engine->newTermsCorpus($raw_corpus); + $normal_corpus = $stemmer->stemCorpus($raw_corpus); + $normal_coprus = $ngram_engine->newTermsCorpus($normal_corpus); + if (!isset($ferret_corpus_map[$key])) { $ferret_corpus_map[$key] = $empty_template; } @@ -67,16 +69,23 @@ final class PhabricatorFerretFulltextEngineExtension } $ferret_fields = array(); + $ngrams_source = array(); foreach ($ferret_corpus_map as $key => $fields) { $raw_corpus = $fields['raw']; $raw_corpus = implode("\n", $raw_corpus); + $ngrams_source[] = $raw_corpus; $normal_corpus = $fields['normal']; - $normal_corpus = implode("\n", $normal_corpus); + $normal_corpus = implode(' ', $normal_corpus); + if (strlen($normal_corpus)) { + $ngrams_source[] = $normal_corpus; + $normal_corpus = ' '.$normal_corpus.' '; + } $term_corpus = $fields['term']; $term_corpus = implode(' ', $term_corpus); if (strlen($term_corpus)) { + $ngrams_source[] = $term_corpus; $term_corpus = ' '.$term_corpus.' '; } @@ -86,9 +95,7 @@ final class PhabricatorFerretFulltextEngineExtension ->setTermCorpus($term_corpus) ->setNormalCorpus($normal_corpus); } - - $ngrams_source = $ferret_corpus_map[$key_all]['raw']; - $ngrams_source = implode("\n", $ngrams_source); + $ngrams_source = implode(' ', $ngrams_source); $ngrams = $ngram_engine->getNgramsFromString($ngrams_source, 'index'); diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 437ca0ce4f..43db65f10b 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1409,8 +1409,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return array(); } + $op_sub = PhutilSearchQueryCompiler::OPERATOR_SUBSTRING; + $engine = $this->ferretEngine; $ngram_engine = new PhabricatorNgramEngine(); + $stemmer = new PhutilSearchStemmer(); $ngram_table = $engine->newNgramsObject(); $ngram_table_name = $ngram_table->getTableName(); @@ -1422,22 +1425,49 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $length = count(phutil_utf8v($value)); - if ($length >= 3) { - $ngrams = $ngram_engine->getNgramsFromString($value, 'query'); - $prefix = false; - } else if ($length == 2) { - $ngrams = $ngram_engine->getNgramsFromString($value, 'prefix'); - $prefix = false; + if ($raw_token->getOperator() == $op_sub) { + $is_substring = true; } else { - $ngrams = array(' '.$value); - $prefix = true; + $is_substring = false; + } + + // If the user specified a substring query for a substring which is + // shorter than the ngram length, we can't use the ngram index, so + // don't do a join. We'll fall back to just doing LIKE on the full + // corpus. + if ($is_substring) { + if ($length < 3) { + continue; + } + } + + if ($raw_token->isQuoted()) { + $is_stemmed = false; + } else { + $is_stemmed = true; + } + + if ($is_substring) { + $ngrams = $ngram_engine->getNgramsFromString($value, 'query'); + } else { + $ngrams = $ngram_engine->getNgramsFromString($value, 'index'); + + // If this is a stemmed term, only look for ngrams present in both the + // unstemmed and stemmed variations. + if ($is_stemmed) { + $stem_value = $stemmer->stemToken($value); + $stem_ngrams = $ngram_engine->getNgramsFromString( + $stem_value, + 'index'); + + $ngrams = array_intersect($ngrams, $stem_ngrams); + } } foreach ($ngrams as $ngram) { $flat[] = array( 'table' => $ngram_table_name, 'ngram' => $ngram, - 'prefix' => $prefix, ); } } @@ -1472,29 +1502,17 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery foreach ($flat as $spec) { $table = $spec['table']; $ngram = $spec['ngram']; - $prefix = $spec['prefix']; $alias = 'ft'.$idx++; - if ($prefix) { - $joins[] = qsprintf( - $conn, - 'JOIN %T %T ON %T.documentID = ftdoc.id AND %T.ngram LIKE %>', - $table, - $alias, - $alias, - $alias, - $ngram); - } else { - $joins[] = qsprintf( - $conn, - 'JOIN %T %T ON %T.documentID = ftdoc.id AND %T.ngram = %s', - $table, - $alias, - $alias, - $alias, - $ngram); - } + $joins[] = qsprintf( + $conn, + 'JOIN %T %T ON %T.documentID = ftdoc.id AND %T.ngram = %s', + $table, + $alias, + $alias, + $alias, + $ngram); } $joins[] = qsprintf( @@ -1510,16 +1528,72 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return array(); } + $ngram_engine = new PhabricatorNgramEngine(); + $stemmer = new PhutilSearchStemmer(); + $op_sub = PhutilSearchQueryCompiler::OPERATOR_SUBSTRING; + $where = array(); foreach ($this->ferretTokens as $fulltext_token) { $raw_token = $fulltext_token->getToken(); $value = $raw_token->getValue(); - $where[] = qsprintf( + if ($raw_token->getOperator() == $op_sub) { + $is_substring = true; + } else { + $is_substring = false; + } + + // If we're doing substring search, we just match against the raw corpus + // and we're done. + if ($is_substring) { + $where[] = qsprintf( + $conn, + '(ftfield.rawCorpus LIKE %~)', + $value); + continue; + } + + // Otherwise, we need to match against the term corpus and the normal + // corpus, so that searching for "raw" does not find "strawberry". + if ($raw_token->isQuoted()) { + $is_quoted = true; + $is_stemmed = false; + } else { + $is_quoted = false; + $is_stemmed = true; + } + + $term_constraints = array(); + + $term_value = ' '.$ngram_engine->newTermsCorpus($value).' '; + $term_constraints[] = qsprintf( $conn, - '(ftfield.rawCorpus LIKE %~ OR ftfield.normalCorpus LIKE %~)', - $value, - $value); + '(ftfield.termCorpus LIKE %~)', + $term_value); + + if ($is_stemmed) { + $stem_value = $stemmer->stemToken($value); + $stem_value = $ngram_engine->newTermsCorpus($stem_value); + $stem_value = ' '.$stem_value.' '; + + $term_constraints[] = qsprintf( + $conn, + '(ftfield.normalCorpus LIKE %~)', + $stem_value); + } + + if ($is_quoted) { + $where[] = qsprintf( + $conn, + '(ftfield.rawCorpus LIKE %~ AND (%Q))', + $value, + implode(' OR ', $term_constraints)); + } else { + $where[] = qsprintf( + $conn, + '(%Q)', + implode(' OR ', $term_constraints)); + } } return $where; From 048aa36c236d74ac4d884c2fa1388f19044a3c21 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 09:51:52 -0700 Subject: [PATCH 18/24] Support "-term" in Ferret engine queries Summary: Ref T12819. Supports negating search terms, e.g. "apple -honeycrisp". When negating a term, we're a little more strict about what can match (that is, what can //prevent// a document from being returned) since it's easy for a user to type "apple -honeycrisp -honey -crisp -crispies -olcrispers -honeyyums" to keep refining their search, but hard/impossible to split apart an overboard term. Test Plan: - Ran `apple -smith`, `apple -"granny smith"`, etc. - Verified `phone -tact` does not exclude `phone contact`. - (In theory, `phone -~tact` would, but the parser currently doesn't support this, and I'm not champing at the bit to add support.) Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18502 --- ...PhabricatorCursorPagedPolicyAwareQuery.php | 57 ++++++++++++++++--- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 43db65f10b..4d31531d39 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1410,6 +1410,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } $op_sub = PhutilSearchQueryCompiler::OPERATOR_SUBSTRING; + $op_not = PhutilSearchQueryCompiler::OPERATOR_NOT; $engine = $this->ferretEngine; $ngram_engine = new PhabricatorNgramEngine(); @@ -1421,6 +1422,15 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $flat = array(); foreach ($this->ferretTokens as $fulltext_token) { $raw_token = $fulltext_token->getToken(); + + // If this is a negated term like "-pomegranate", don't join the ngram + // table since we aren't looking for documents with this term. (We could + // LEFT JOIN the table and require a NULL row, but this is probably more + // trouble than it's worth.) + if ($raw_token->getOperator() == $op_not) { + continue; + } + $value = $raw_token->getValue(); $length = count(phutil_utf8v($value)); @@ -1530,13 +1540,17 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $ngram_engine = new PhabricatorNgramEngine(); $stemmer = new PhutilSearchStemmer(); + $op_sub = PhutilSearchQueryCompiler::OPERATOR_SUBSTRING; + $op_not = PhutilSearchQueryCompiler::OPERATOR_NOT; $where = array(); foreach ($this->ferretTokens as $fulltext_token) { $raw_token = $fulltext_token->getToken(); $value = $raw_token->getValue(); + $is_not = ($raw_token->getOperator() == $op_not); + if ($raw_token->getOperator() == $op_sub) { $is_substring = true; } else { @@ -1546,10 +1560,17 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery // If we're doing substring search, we just match against the raw corpus // and we're done. if ($is_substring) { - $where[] = qsprintf( - $conn, - '(ftfield.rawCorpus LIKE %~)', - $value); + if ($is_not) { + $where[] = qsprintf( + $conn, + '(ftfield.rawCorpus NOT LIKE %~)', + $value); + } else { + $where[] = qsprintf( + $conn, + '(ftfield.rawCorpus LIKE %~)', + $value); + } continue; } @@ -1563,13 +1584,26 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $is_stemmed = true; } + // Never stem negated queries, since this can exclude results users + // did not mean to exclude and generally confuse things. + if ($is_not) { + $is_stemmed = false; + } + $term_constraints = array(); $term_value = ' '.$ngram_engine->newTermsCorpus($value).' '; - $term_constraints[] = qsprintf( - $conn, - '(ftfield.termCorpus LIKE %~)', - $term_value); + if ($is_not) { + $term_constraints[] = qsprintf( + $conn, + '(ftfield.termCorpus NOT LIKE %~)', + $term_value); + } else { + $term_constraints[] = qsprintf( + $conn, + '(ftfield.termCorpus LIKE %~)', + $term_value); + } if ($is_stemmed) { $stem_value = $stemmer->stemToken($value); @@ -1582,7 +1616,12 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $stem_value); } - if ($is_quoted) { + if ($is_not) { + $where[] = qsprintf( + $conn, + '(%Q)', + implode(' AND ', $term_constraints)); + } else if ($is_quoted) { $where[] = qsprintf( $conn, '(ftfield.rawCorpus LIKE %~ AND (%Q))', From 3b43a7077345bbc83699157a75d630b2a48b87ad Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Aug 2017 10:09:20 -0700 Subject: [PATCH 19/24] Add "title:..." support to the Ferret engine Summary: Ref T12819. Adds (hacky, hard-coded) field support (for now, only for "title"). I've written this so `title:quick ferret` is the same as `title:quick title:ferret`. I think this is what users probably mean. You can do the other thing as `ferret title:quick`, or `title:quick all:ferret`. Test Plan: Searched for `title:x`, `title:"x"`, `title:~"x"`, etc. Searched for "garbage:y", got an exception since that's not a recognized function. Searched for `title:x y`, saw both do title search. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18503 --- ...PhabricatorCursorPagedPolicyAwareQuery.php | 83 ++++++++++++++++--- 1 file changed, 73 insertions(+), 10 deletions(-) diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 4d31531d39..9e6da1ec00 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -29,6 +29,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery private $ngrams = array(); private $ferretEngine; private $ferretTokens; + private $ferretTables; protected function getPageCursors(array $page) { return array( @@ -1401,6 +1402,43 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $this->ferretEngine = $engine; $this->ferretTokens = $fulltext_tokens; + + $function_map = array( + 'all' => PhabricatorSearchDocumentFieldType::FIELD_ALL, + 'title' => PhabricatorSearchDocumentFieldType::FIELD_TITLE, + ); + + $current_function = 'all'; + $table_map = array(); + $idx = 1; + foreach ($this->ferretTokens as $fulltext_token) { + $raw_token = $fulltext_token->getToken(); + $function = $raw_token->getFunction(); + + if ($function === null) { + $function = $current_function; + } + + if (!isset($function_map[$function])) { + throw new PhutilSearchQueryCompilerSyntaxException( + pht( + 'Unknown search function "%s".', + $function)); + } + + if (!isset($table_map[$function])) { + $alias = 'ftfield'.$idx++; + $table_map[$function] = array( + 'alias' => $alias, + 'key' => $function_map[$function], + ); + } + + $current_function = $function; + } + + $this->ferretTables = $table_map; + return $this; } @@ -1525,10 +1563,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $ngram); } - $joins[] = qsprintf( - $conn, - 'JOIN %T ftfield ON ftdoc.id = ftfield.documentID', - $field_table->getTableName()); + foreach ($this->ferretTables as $table) { + $alias = $table['alias']; + + $joins[] = qsprintf( + $conn, + 'JOIN %T %T ON ftdoc.id = %T.documentID + AND %T.fieldKey = %s', + $field_table->getTableName(), + $alias, + $alias, + $alias, + $table['key']); + } return $joins; } @@ -1540,15 +1587,25 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $ngram_engine = new PhabricatorNgramEngine(); $stemmer = new PhutilSearchStemmer(); + $table_map = $this->ferretTables; $op_sub = PhutilSearchQueryCompiler::OPERATOR_SUBSTRING; $op_not = PhutilSearchQueryCompiler::OPERATOR_NOT; $where = array(); + $current_function = 'all'; foreach ($this->ferretTokens as $fulltext_token) { $raw_token = $fulltext_token->getToken(); $value = $raw_token->getValue(); + $function = $raw_token->getFunction(); + if ($function === null) { + $function = $current_function; + } + $current_function = $function; + + $table_alias = $table_map[$function]['alias']; + $is_not = ($raw_token->getOperator() == $op_not); if ($raw_token->getOperator() == $op_sub) { @@ -1563,12 +1620,14 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery if ($is_not) { $where[] = qsprintf( $conn, - '(ftfield.rawCorpus NOT LIKE %~)', + '(%T.rawCorpus NOT LIKE %~)', + $table_alias, $value); } else { $where[] = qsprintf( $conn, - '(ftfield.rawCorpus LIKE %~)', + '(%T.rawCorpus LIKE %~)', + $table_alias, $value); } continue; @@ -1596,12 +1655,14 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery if ($is_not) { $term_constraints[] = qsprintf( $conn, - '(ftfield.termCorpus NOT LIKE %~)', + '(%T.termCorpus NOT LIKE %~)', + $table_alias, $term_value); } else { $term_constraints[] = qsprintf( $conn, - '(ftfield.termCorpus LIKE %~)', + '(%T.termCorpus LIKE %~)', + $table_alias, $term_value); } @@ -1612,7 +1673,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $term_constraints[] = qsprintf( $conn, - '(ftfield.normalCorpus LIKE %~)', + '(%T.normalCorpus LIKE %~)', + $table_alias, $stem_value); } @@ -1624,7 +1686,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } else if ($is_quoted) { $where[] = qsprintf( $conn, - '(ftfield.rawCorpus LIKE %~ AND (%Q))', + '(%T.rawCorpus LIKE %~ AND (%Q))', + $table_alias, $value, implode(' OR ', $term_constraints)); } else { From 67c658a7ed87b4363a3b8800cadf082e50ad111f Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 30 Aug 2017 18:33:42 +0000 Subject: [PATCH 20/24] Use selected button state on blame button Summary: Visually selects the button if blame is on. Test Plan: Turn blame on and off in Diffusion on a file. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D18504 --- .../diffusion/controller/DiffusionBrowseController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 5eee013622..7d6dad7e61 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -730,6 +730,7 @@ final class DiffusionBrowseController extends DiffusionController { ->setText($blame_text) ->setIcon($blame_icon) ->setUser($viewer) + ->setSelected(!$blame_value) ->setColor(PHUIButtonView::GREY); if ($viewer->isLoggedIn()) { From 2ba5968b763906a945b2420a95f9a4fe580aaba0 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 30 Aug 2017 12:00:07 -0700 Subject: [PATCH 21/24] Mobile layouts for Diffusion Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content. Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18505 --- resources/celerity/map.php | 18 ++++++------ .../DiffusionBranchTableController.php | 1 + .../controller/DiffusionBrowseController.php | 28 ++++++++++++++----- .../controller/DiffusionCompareController.php | 2 ++ .../controller/DiffusionGraphController.php | 1 + .../DiffusionRepositoryController.php | 4 ++- .../view/DiffusionBrowseTableView.php | 2 +- .../view/DiffusionHistoryListView.php | 1 + .../diffusion/view/DiffusionReadmeView.php | 1 + webroot/rsrc/css/aphront/table-view.css | 7 +---- .../diffusion/diffusion-source.css | 11 ++++++++ .../css/application/diffusion/diffusion.css | 19 ++++--------- .../search/application-search-view.css | 2 +- 13 files changed, 59 insertions(+), 38 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 1fefbd5e67..02edbec103 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' => '0437a674', + 'core.pkg.css' => '4ac857bf', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -32,7 +32,7 @@ return array( 'rsrc/css/aphront/notification.css' => '457861ec', 'rsrc/css/aphront/panel-view.css' => '8427b78d', 'rsrc/css/aphront/phabricator-nav-view.css' => 'faf6a6fc', - 'rsrc/css/aphront/table-view.css' => 'a3aa6910', + 'rsrc/css/aphront/table-view.css' => '8c9bbafe', 'rsrc/css/aphront/tokenizer.css' => '15d5ff71', 'rsrc/css/aphront/tooltip.css' => '173b9431', 'rsrc/css/aphront/typeahead-browse.css' => 'f2818435', @@ -74,8 +74,8 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '47db8a7c', - 'rsrc/css/application/diffusion/diffusion.css' => 'ceacf994', + 'rsrc/css/application/diffusion/diffusion-source.css' => '69ac9399', + 'rsrc/css/application/diffusion/diffusion.css' => '9d5bb76d', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', 'rsrc/css/application/flag/flag.css' => 'bba8f811', @@ -110,7 +110,7 @@ return array( 'rsrc/css/application/releeph/releeph-preview-branch.css' => 'b7a6f4a5', 'rsrc/css/application/releeph/releeph-request-differential-create-dialog.css' => '8d8b92cd', 'rsrc/css/application/releeph/releeph-request-typeahead.css' => '667a48ae', - 'rsrc/css/application/search/application-search-view.css' => '66ee5d46', + 'rsrc/css/application/search/application-search-view.css' => '787f5b76', 'rsrc/css/application/search/search-results.css' => '505dd8cf', 'rsrc/css/application/slowvote/slowvote.css' => 'a94b7230', 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', @@ -543,11 +543,11 @@ return array( 'aphront-list-filter-view-css' => '5d6f0526', 'aphront-multi-column-view-css' => '84cc6640', 'aphront-panel-view-css' => '8427b78d', - 'aphront-table-view-css' => 'a3aa6910', + 'aphront-table-view-css' => '8c9bbafe', 'aphront-tokenizer-control-css' => '15d5ff71', 'aphront-tooltip-css' => '173b9431', 'aphront-typeahead-control-css' => 'a4a21016', - 'application-search-view-css' => '66ee5d46', + 'application-search-view-css' => '787f5b76', 'auth-css' => '0877ed6e', 'bulk-job-css' => 'df9c1d4a', 'conduit-api-css' => '7bc725c4', @@ -570,11 +570,11 @@ return array( 'differential-revision-history-css' => '0e8eb855', 'differential-revision-list-css' => 'f3c47d33', 'differential-table-of-contents-css' => 'ae4b7a55', - 'diffusion-css' => 'ceacf994', + 'diffusion-css' => '9d5bb76d', 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '47db8a7c', + 'diffusion-source-css' => '69ac9399', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBranchTableController.php b/src/applications/diffusion/controller/DiffusionBranchTableController.php index 801b45a450..441421ba3f 100644 --- a/src/applications/diffusion/controller/DiffusionBranchTableController.php +++ b/src/applications/diffusion/controller/DiffusionBranchTableController.php @@ -57,6 +57,7 @@ final class DiffusionBranchTableController extends DiffusionController { $content = id(new PHUIObjectBoxView()) ->setHeaderText($repository->getName()) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->setTable($list) ->setPager($pager); } diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 7d6dad7e61..076e667dc4 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -371,6 +371,7 @@ final class DiffusionBrowseController extends DiffusionController { ->setHeader($browse_header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setTable($browse_table) + ->addClass('diffusion-mobile-view') ->setPager($pager); $path = $drequest->getPath(); @@ -595,6 +596,8 @@ final class DiffusionBrowseController extends DiffusionController { ), $rows); + $corpus_table = phutil_tag_div('diffusion-source-wrap', $corpus_table); + if ($this->getRequest()->isAjax()) { return $corpus_table; } @@ -654,6 +657,7 @@ final class DiffusionBrowseController extends DiffusionController { ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->appendChild($corpus) + ->addClass('diffusion-mobile-view') ->setCollapsed(true); $messages = array(); @@ -860,6 +864,7 @@ final class DiffusionBrowseController extends DiffusionController { $view = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Owner Packages')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->setObjectList($ownership); } @@ -1341,6 +1346,7 @@ final class DiffusionBrowseController extends DiffusionController { return id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->addPropertyList($properties); } @@ -1361,6 +1367,7 @@ final class DiffusionBrowseController extends DiffusionController { $box = id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->appendChild($text); return $box; @@ -1693,15 +1700,20 @@ final class DiffusionBrowseController extends DiffusionController { $header = id(new PHUIHeaderView()) ->setHeader(pht('Recently Open Revisions')); - $view = id(new DifferentialRevisionListView()) + $list = id(new DifferentialRevisionListView()) + ->setRevisions($revisions) + ->setUser($viewer) + ->setNoBox(true); + + $phids = $list->getRequiredHandlePHIDs(); + $handles = $this->loadViewerHandles($phids); + $list->setHandles($handles); + + $view = id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->setRevisions($revisions) - ->setUser($viewer); - - $phids = $view->getRequiredHandlePHIDs(); - $handles = $this->loadViewerHandles($phids); - $view->setHandles($handles); + ->addClass('diffusion-mobile-view') + ->appendChild($list); return $view; } @@ -1838,6 +1850,7 @@ final class DiffusionBrowseController extends DiffusionController { $corpus = id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->setCollapsed(true); if ($messages) { @@ -1922,6 +1935,7 @@ final class DiffusionBrowseController extends DiffusionController { return id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->setTable($history_table); } diff --git a/src/applications/diffusion/controller/DiffusionCompareController.php b/src/applications/diffusion/controller/DiffusionCompareController.php index a3104e6da4..2361c066dd 100644 --- a/src/applications/diffusion/controller/DiffusionCompareController.php +++ b/src/applications/diffusion/controller/DiffusionCompareController.php @@ -15,6 +15,7 @@ final class DiffusionCompareController extends DiffusionController { $viewer = $this->getViewer(); $drequest = $this->getDiffusionRequest(); $repository = $drequest->getRepository(); + require_celerity_resource('diffusion-css'); if (!$repository->supportsBranchComparison()) { return $this->newDialog() @@ -315,6 +316,7 @@ final class DiffusionCompareController extends DiffusionController { ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setTable($history_table) + ->addClass('diffusion-mobile-view') ->setPager($pager); } diff --git a/src/applications/diffusion/controller/DiffusionGraphController.php b/src/applications/diffusion/controller/DiffusionGraphController.php index 5062fe9765..096204ac6d 100644 --- a/src/applications/diffusion/controller/DiffusionGraphController.php +++ b/src/applications/diffusion/controller/DiffusionGraphController.php @@ -67,6 +67,7 @@ final class DiffusionGraphController extends DiffusionController { ->setHeaderText(pht('History Graph')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setTable($graph) + ->addClass('diffusion-mobile-view') ->setPager($pager); $tabs = $this->buildTabsView('graph'); diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index 120dd0d344..f64b72e1f8 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -420,7 +420,8 @@ final class DiffusionRepositoryController extends DiffusionController { $history_table->setIsHead(true); $panel = id(new PHUIObjectBoxView()) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view'); $header = id(new PHUIHeaderView()) ->setHeader(pht('Recent Commits')); $panel->setHeader($header); @@ -583,6 +584,7 @@ final class DiffusionRepositoryController extends DiffusionController { ->setHeaderText($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setTable($browse_table) + ->addClass('diffusion-mobile-view') ->setPager($pager); } diff --git a/src/applications/diffusion/view/DiffusionBrowseTableView.php b/src/applications/diffusion/view/DiffusionBrowseTableView.php index 48729d0669..ffbfe8986f 100644 --- a/src/applications/diffusion/view/DiffusionBrowseTableView.php +++ b/src/applications/diffusion/view/DiffusionBrowseTableView.php @@ -134,7 +134,7 @@ final class DiffusionBrowseTableView extends DiffusionView { array( true, false, - true, + false, false, false, )); diff --git a/src/applications/diffusion/view/DiffusionHistoryListView.php b/src/applications/diffusion/view/DiffusionHistoryListView.php index 8f9301c53a..62ba7b70c2 100644 --- a/src/applications/diffusion/view/DiffusionHistoryListView.php +++ b/src/applications/diffusion/view/DiffusionHistoryListView.php @@ -40,6 +40,7 @@ final class DiffusionHistoryListView extends DiffusionHistoryView { $view[] = id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->setObjectList($list); } diff --git a/src/applications/diffusion/view/DiffusionReadmeView.php b/src/applications/diffusion/view/DiffusionReadmeView.php index 88e2e84115..693333249d 100644 --- a/src/applications/diffusion/view/DiffusionReadmeView.php +++ b/src/applications/diffusion/view/DiffusionReadmeView.php @@ -105,6 +105,7 @@ final class DiffusionReadmeView extends DiffusionView { return id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addClass('diffusion-mobile-view') ->appendChild($document) ->addClass('diffusion-readme-view'); } diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css index a02d4dcfbe..1a7d2eb215 100644 --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -4,6 +4,7 @@ .aphront-table-wrap { overflow-x: auto; + -webkit-overflow-scrolling: touch; } .aphront-table-view { @@ -128,14 +129,8 @@ th.aphront-table-view-sortable-selected { padding: 8px 10px; } -.device-tablet .aphront-table-view td, -.device-phone .aphront-table-view td { - padding: 6px; -} - .device-tablet .aphront-table-view th, .device-phone .aphront-table-view th { - padding: 6px; overflow: hidden; } diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index ffbed2589e..3b8b9a8a16 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -5,6 +5,12 @@ .diffusion-source { width: 100%; background: {$page.content}; + overflow: hidden; +} + +.device-phone .diffusion-source-wrap { + overflow: scroll; + -webkit-overflow-scrolling: touch; } .diffusion-source tr.phabricator-source-highlight th, @@ -27,6 +33,11 @@ word-break: break-all; } +.device .diffusion-source td { + word-break: normal; + white-space: nowrap; +} + .diffusion-browse-type-form { float: right; } diff --git a/webroot/rsrc/css/application/diffusion/diffusion.css b/webroot/rsrc/css/application/diffusion/diffusion.css index 2f7754f9f8..d994c38a8b 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion.css +++ b/webroot/rsrc/css/application/diffusion/diffusion.css @@ -207,16 +207,6 @@ border-color: {$thinblueborder}; } -.device-phone.diffusion-history-view .phui-two-column-view - .phui-two-column-footer .phui-header-view { - text-align: center; -} - -.device-phone.diffusion-history-view .phui-two-column-content { - padding: 0; - margin: 0 -4px; -} - .device-phone.diffusion-history-view .phui-oi-attribute-spacer { display: none; } @@ -245,7 +235,10 @@ margin: 0; } -.device-phone.diffusion-history-view .diffusion-history-list .button.has-icon - .phui-icon-view { - display: none; +.device-phone .phui-two-column-view .phui-two-column-content + .phui-object-box.diffusion-mobile-view { + margin: 0 -12px 20px; + border-left: none; + border-right: none; + border-color: {$thinblueborder}; } diff --git a/webroot/rsrc/css/application/search/application-search-view.css b/webroot/rsrc/css/application/search/application-search-view.css index 4036124547..afc59a7ba8 100644 --- a/webroot/rsrc/css/application/search/application-search-view.css +++ b/webroot/rsrc/css/application/search/application-search-view.css @@ -40,7 +40,7 @@ padding: 12px 0; } -.device-phone .application-search-results +.device-phone.application-search-view .application-search-results .phui-profile-header.phui-header-shell { padding: 12px 0 12px 4px; } From ab99222f28f50f67a0bfedfa152364222dd6f4cc Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 30 Aug 2017 12:56:47 -0700 Subject: [PATCH 22/24] Fix harbormaster ui on history view / mobile Summary: This is an older CSS rule that's no longer needed on mobile. Test Plan: view history view with harbormaster results Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18507 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/application/diffusion/diffusion.css | 5 ----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 02edbec103..e3854c740c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -75,7 +75,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', 'rsrc/css/application/diffusion/diffusion-source.css' => '69ac9399', - 'rsrc/css/application/diffusion/diffusion.css' => '9d5bb76d', + 'rsrc/css/application/diffusion/diffusion.css' => '45727264', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', 'rsrc/css/application/flag/flag.css' => 'bba8f811', @@ -570,7 +570,7 @@ return array( 'differential-revision-history-css' => '0e8eb855', 'differential-revision-list-css' => 'f3c47d33', 'differential-table-of-contents-css' => 'ae4b7a55', - 'diffusion-css' => '9d5bb76d', + 'diffusion-css' => '45727264', 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', diff --git a/webroot/rsrc/css/application/diffusion/diffusion.css b/webroot/rsrc/css/application/diffusion/diffusion.css index d994c38a8b..20f269dd3d 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion.css +++ b/webroot/rsrc/css/application/diffusion/diffusion.css @@ -230,11 +230,6 @@ padding-bottom: 10px; } -.device-phone.diffusion-history-view .diffusion-history-list .button.has-icon - .phui-button-text { - margin: 0; -} - .device-phone .phui-two-column-view .phui-two-column-content .phui-object-box.diffusion-mobile-view { margin: 0 -12px 20px; From f4f73e0a7e466146cf6613d59e6e12c3edd24076 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Sep 2017 09:19:01 -0700 Subject: [PATCH 23/24] Separate fulltext engine extensions into "enrich" and "index" phases Summary: Ref T12819. Some of the extensions "enrich" the document (adding more fields or relationships), while others "index" it (insert it into some kind of index for later searching). Currently, these are all muddled under a single "index" phase. However, the Ferret extension cares about fields and relationships which other extensions may add. Split this into two phases: "enrich" adds fields and relationships so other extensions can read them later if they want. "Index" happens after the document is built and has all the fields and relationships. The specific problem this solves is that comments may not have been added to the document when the Ferret extension runs. By moving them to the "enrich" phase, the Ferret engine will be able to see and index comments. Test Plan: Ran `bin/search index ...`, grepped for `indexFulltextDocument`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12819 Differential Revision: https://secure.phabricator.com/D18513 --- ...ricatorProjectsFulltextEngineExtension.php | 4 ++-- ...PhabricatorLiskFulltextEngineExtension.php | 4 ++-- .../index/PhabricatorFulltextEngine.php | 17 +++++++++++++--- .../PhabricatorFulltextEngineExtension.php | 20 ++++++++++++++++--- ...orSubscriptionsFulltextEngineExtension.php | 4 ++-- ...torTransactionsFulltextEngineExtension.php | 4 ++-- ...atorCustomFieldFulltextEngineExtension.php | 4 ++-- 7 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php b/src/applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php index 857783f811..79f395ebaa 100644 --- a/src/applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php +++ b/src/applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php @@ -9,11 +9,11 @@ final class PhabricatorProjectsFulltextEngineExtension return pht('Projects'); } - public function shouldIndexFulltextObject($object) { + public function shouldEnrichFulltextObject($object) { return ($object instanceof PhabricatorProjectInterface); } - public function indexFulltextObject( + public function enrichFulltextObject( $object, PhabricatorSearchAbstractDocument $document) { diff --git a/src/applications/search/engineextension/PhabricatorLiskFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorLiskFulltextEngineExtension.php index 9cbe384a10..1626d248da 100644 --- a/src/applications/search/engineextension/PhabricatorLiskFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorLiskFulltextEngineExtension.php @@ -9,7 +9,7 @@ final class PhabricatorLiskFulltextEngineExtension return pht('Lisk Builtin Properties'); } - public function shouldIndexFulltextObject($object) { + public function shouldEnrichFulltextObject($object) { if (!($object instanceof PhabricatorLiskDAO)) { return false; } @@ -21,7 +21,7 @@ final class PhabricatorLiskFulltextEngineExtension return true; } - public function indexFulltextObject( + public function enrichFulltextObject( $object, PhabricatorSearchAbstractDocument $document) { diff --git a/src/applications/search/index/PhabricatorFulltextEngine.php b/src/applications/search/index/PhabricatorFulltextEngine.php index 9f20917b3f..e1b2f4471d 100644 --- a/src/applications/search/index/PhabricatorFulltextEngine.php +++ b/src/applications/search/index/PhabricatorFulltextEngine.php @@ -26,9 +26,16 @@ abstract class PhabricatorFulltextEngine $object = $this->getObject(); $extensions = PhabricatorFulltextEngineExtension::getAllExtensions(); + + $enrich_extensions = array(); + $index_extensions = array(); foreach ($extensions as $key => $extension) { - if (!$extension->shouldIndexFulltextObject($object)) { - unset($extensions[$key]); + if ($extension->shouldEnrichFulltextObject($object)) { + $enrich_extensions[] = $extension; + } + + if ($extension->shouldIndexFulltextObject($object)) { + $index_extensions[] = $extension; } } @@ -36,7 +43,11 @@ abstract class PhabricatorFulltextEngine $this->buildAbstractDocument($document, $object); - foreach ($extensions as $extension) { + foreach ($enrich_extensions as $extension) { + $extension->enrichFulltextObject($object, $document); + } + + foreach ($index_extensions as $extension) { $extension->indexFulltextObject($object, $document); } diff --git a/src/applications/search/index/PhabricatorFulltextEngineExtension.php b/src/applications/search/index/PhabricatorFulltextEngineExtension.php index c52b35a58a..52aabe7c8b 100644 --- a/src/applications/search/index/PhabricatorFulltextEngineExtension.php +++ b/src/applications/search/index/PhabricatorFulltextEngineExtension.php @@ -12,11 +12,25 @@ abstract class PhabricatorFulltextEngineExtension extends Phobject { abstract public function getExtensionName(); - abstract public function shouldIndexFulltextObject($object); + public function shouldEnrichFulltextObject($object) { + return false; + } - abstract public function indexFulltextObject( + public function enrichFulltextObject( $object, - PhabricatorSearchAbstractDocument $document); + PhabricatorSearchAbstractDocument $document) { + return; + } + + public function shouldIndexFulltextObject($object) { + return false; + } + + public function indexFulltextObject( + $object, + PhabricatorSearchAbstractDocument $document) { + return; + } final public static function getAllExtensions() { return id(new PhutilClassMapQuery()) diff --git a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsFulltextEngineExtension.php b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsFulltextEngineExtension.php index 3c25618ebe..e7876136a1 100644 --- a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsFulltextEngineExtension.php +++ b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsFulltextEngineExtension.php @@ -9,11 +9,11 @@ final class PhabricatorSubscriptionsFulltextEngineExtension return pht('Subscribers'); } - public function shouldIndexFulltextObject($object) { + public function shouldEnrichFulltextObject($object) { return ($object instanceof PhabricatorSubscribableInterface); } - public function indexFulltextObject( + public function enrichFulltextObject( $object, PhabricatorSearchAbstractDocument $document) { diff --git a/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php index b95fc0167a..701ad34ca7 100644 --- a/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorTransactionsFulltextEngineExtension.php @@ -9,11 +9,11 @@ final class PhabricatorTransactionsFulltextEngineExtension return pht('Comments'); } - public function shouldIndexFulltextObject($object) { + public function shouldEnrichFulltextObject($object) { return ($object instanceof PhabricatorApplicationTransactionInterface); } - public function indexFulltextObject( + public function enrichFulltextObject( $object, PhabricatorSearchAbstractDocument $document) { diff --git a/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php b/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php index 8186ff5311..9e0684cacc 100644 --- a/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php +++ b/src/infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php @@ -9,11 +9,11 @@ final class PhabricatorCustomFieldFulltextEngineExtension return pht('Custom Fields'); } - public function shouldIndexFulltextObject($object) { + public function shouldEnrichFulltextObject($object) { return ($object instanceof PhabricatorCustomFieldInterface); } - public function indexFulltextObject( + public function enrichFulltextObject( $object, PhabricatorSearchAbstractDocument $document) { From 577d4980339f68826d534110107687f33b176e88 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Sep 2017 09:27:43 -0700 Subject: [PATCH 24/24] Create a virtual "core" field in the Ferret engine for "title and body together" Summary: See PHI46. The `core:` function means "find results in either the title or body, but not other auxiliary fields like comments". Test Plan: Searched for text present in the title (yes), body (yes), and comments (no) with the `core:...` prefix. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18514 --- .../PhabricatorSearchDocumentFieldType.php | 1 + ...abricatorFerretFulltextEngineExtension.php | 21 ++++++++++++++++++- ...PhabricatorCursorPagedPolicyAwareQuery.php | 2 ++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php index 4dd49e8c92..42fc2738f3 100644 --- a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php +++ b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php @@ -6,5 +6,6 @@ final class PhabricatorSearchDocumentFieldType extends Phobject { const FIELD_BODY = 'body'; const FIELD_COMMENT = 'cmnt'; const FIELD_ALL = 'full'; + const FIELD_CORE = 'core'; } diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index 77599b9d5d..1c9efeba9d 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -32,6 +32,25 @@ final class PhabricatorFerretFulltextEngineExtension $stemmer = new PhutilSearchStemmer(); $ngram_engine = id(new PhabricatorNgramEngine()); + // Copy all of the "title" and "body" fields to create new "core" fields. + // This allows users to search "in title or body" with the "core:" prefix. + $document_fields = $document->getFieldData(); + $virtual_fields = array(); + foreach ($document_fields as $field) { + $virtual_fields[] = $field; + + list($key, $raw_corpus) = $field; + switch ($key) { + case PhabricatorSearchDocumentFieldType::FIELD_TITLE: + case PhabricatorSearchDocumentFieldType::FIELD_BODY: + $virtual_fields[] = array( + PhabricatorSearchDocumentFieldType::FIELD_CORE, + $raw_corpus, + ); + break; + } + } + $key_all = PhabricatorSearchDocumentFieldType::FIELD_ALL; $empty_template = array( @@ -44,7 +63,7 @@ final class PhabricatorFerretFulltextEngineExtension $key_all => $empty_template, ); - foreach ($document->getFieldData() as $field) { + foreach ($virtual_fields as $field) { list($key, $raw_corpus) = $field; if (!strlen($raw_corpus)) { continue; diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 9e6da1ec00..03d56bc1d2 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1406,6 +1406,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $function_map = array( 'all' => PhabricatorSearchDocumentFieldType::FIELD_ALL, 'title' => PhabricatorSearchDocumentFieldType::FIELD_TITLE, + 'body' => PhabricatorSearchDocumentFieldType::FIELD_BODY, + 'core' => PhabricatorSearchDocumentFieldType::FIELD_CORE, ); $current_function = 'all';