From f0fbf7a7d32e1430880bf4ef5b28c84a0a95f1cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 14 Apr 2017 12:14:04 -0700 Subject: [PATCH] Sort "closed" results (like disabled users) to the bottom of typeaheads, but don't hide them completely Summary: Fixes T12538. Instead of hiding "closed" results unless only closed results match, show closed results but sort them to the bottom. This fixes the actual issue in T12538, and I think this is probably the correct/best behavior for the global search. It also makes all other typeaheads use this behavior. They currently have a "bug" where enabled user `abcd` makes it impossible to select disabled user `abc`. This manifests in some real cases, where enabled function `members(abc)` makes it impossible to disabled user `abc` in some function tokenizers. If ths feels worse, we could go back to filtering in the simpler cases and introduce a rule like "show closed results if only closed results would be shown OR if query is an exact match for the disabled result", but that gets dicier because "exact match" is a fuzzy concept. (There are a lot of other minor bad behaviors that this doesn't try to fix.) Test Plan: Enabled project "instabug" no longer prevents bot user "instabug" from being shown: {F4903843} Disabled user "mmaven" is sorted below enabled user "mmclewis", in defiance of the otherwise alphabetical order. There's no visual cue that this user is disabled because of T6906. {F4903845} Same as above, but this source renders "disabled" in a more obvious way: {F4903848} Function selecting members of active project `members(instabug)` no longer prevents selection of bot user `instabug`: {F4903849} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12538 Differential Revision: https://secure.phabricator.com/D17695 --- resources/celerity/map.php | 72 +++++++------- webroot/rsrc/js/core/Prefab.js | 32 ------- .../rsrc/js/core/behavior-search-typeahead.js | 95 ++++++++----------- webroot/rsrc/js/phuix/PHUIXAutocomplete.js | 1 - 4 files changed, 76 insertions(+), 124 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 33e3549d2b..d9eb3a42e0 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '437d3b5a', 'conpherence.pkg.js' => '281b1a73', 'core.pkg.css' => 'b2ad82f4', - 'core.pkg.js' => 'fbc1c380', + 'core.pkg.js' => 'bf3b5cdf', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '90b30783', 'differential.pkg.js' => 'ddfeb49b', @@ -472,7 +472,7 @@ return array( 'rsrc/js/core/KeyboardShortcutManager.js' => '4a021c10', 'rsrc/js/core/MultirowRowManager.js' => 'b5d57730', 'rsrc/js/core/Notification.js' => 'ccf1cbf8', - 'rsrc/js/core/Prefab.js' => '8d40ae75', + 'rsrc/js/core/Prefab.js' => 'c5af80a2', 'rsrc/js/core/ShapedRequest.js' => '7cbe244b', 'rsrc/js/core/TextAreaUtils.js' => '320810c8', 'rsrc/js/core/Title.js' => '485aaa6c', @@ -510,7 +510,7 @@ return array( 'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e', 'rsrc/js/core/behavior-reveal-content.js' => '60821bc7', 'rsrc/js/core/behavior-scrollbar.js' => '834a1173', - 'rsrc/js/core/behavior-search-typeahead.js' => '06c32383', + 'rsrc/js/core/behavior-search-typeahead.js' => '0f2a0820', 'rsrc/js/core/behavior-select-content.js' => 'bf5374ef', 'rsrc/js/core/behavior-select-on-click.js' => '4e3e79a6', 'rsrc/js/core/behavior-setup-check-https.js' => '491416b3', @@ -528,7 +528,7 @@ return array( 'rsrc/js/phui/behavior-phui-tab-group.js' => '0a0b10e9', 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => 'b3465b9b', - 'rsrc/js/phuix/PHUIXAutocomplete.js' => 'd5b2abf3', + 'rsrc/js/phuix/PHUIXAutocomplete.js' => 'd713a2c5', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50', 'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671', 'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b', @@ -666,7 +666,7 @@ return array( 'javelin-behavior-phabricator-oncopy' => '2926fff2', 'javelin-behavior-phabricator-remarkup-assist' => '0ca788bd', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', - 'javelin-behavior-phabricator-search-typeahead' => '06c32383', + 'javelin-behavior-phabricator-search-typeahead' => '0f2a0820', 'javelin-behavior-phabricator-show-older-transactions' => '94c65b72', 'javelin-behavior-phabricator-tooltips' => 'c420b0b9', 'javelin-behavior-phabricator-transaction-comment-form' => 'b23b49e6', @@ -792,7 +792,7 @@ return array( 'phabricator-notification-menu-css' => '6a697e43', 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', - 'phabricator-prefab' => '8d40ae75', + 'phabricator-prefab' => 'c5af80a2', 'phabricator-remarkup-css' => '17c0fb37', 'phabricator-search-results-css' => 'f87d23ad', 'phabricator-shaped-request' => '7cbe244b', @@ -885,7 +885,7 @@ return array( 'phui-workpanel-view-css' => 'a3a63478', 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => 'b3465b9b', - 'phuix-autocomplete' => 'd5b2abf3', + 'phuix-autocomplete' => 'd713a2c5', 'phuix-dropdown-menu' => '8018ee50', 'phuix-form-control-view' => '83e03671', 'phuix-icon-view' => 'bff6884b', @@ -943,17 +943,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '06c32383' => array( - 'javelin-behavior', - 'javelin-typeahead-ondemand-source', - 'javelin-typeahead', - 'javelin-dom', - 'javelin-uri', - 'javelin-util', - 'javelin-stratcom', - 'phabricator-prefab', - 'phuix-icon-view', - ), '0825c27a' => array( 'javelin-behavior', 'javelin-dom', @@ -999,6 +988,17 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), + '0f2a0820' => array( + 'javelin-behavior', + 'javelin-typeahead-ondemand-source', + 'javelin-typeahead', + 'javelin-dom', + 'javelin-uri', + 'javelin-util', + 'javelin-stratcom', + 'phabricator-prefab', + 'phuix-icon-view', + ), '0f764c35' => array( 'javelin-install', 'javelin-util', @@ -1588,18 +1588,6 @@ return array( 'javelin-stratcom', 'javelin-install', ), - '8d40ae75' => array( - 'javelin-install', - 'javelin-util', - 'javelin-dom', - 'javelin-typeahead', - 'javelin-tokenizer', - 'javelin-typeahead-preloaded-source', - 'javelin-typeahead-ondemand-source', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - ), '8fadb715' => array( 'javelin-install', 'javelin-util', @@ -1955,6 +1943,18 @@ return array( 'c587b80f' => array( 'javelin-install', ), + 'c5af80a2' => array( + 'javelin-install', + 'javelin-util', + 'javelin-dom', + 'javelin-typeahead', + 'javelin-tokenizer', + 'javelin-typeahead-preloaded-source', + 'javelin-typeahead-ondemand-source', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + ), 'c7ccd872' => array( 'phui-fontkit-css', ), @@ -2055,12 +2055,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'd5b2abf3' => array( - 'javelin-install', - 'javelin-dom', - 'phuix-icon-view', - 'phabricator-prefab', - ), 'd6a7e717' => array( 'multirow-row-manager', 'javelin-install', @@ -2070,6 +2064,12 @@ return array( 'javelin-json', 'phabricator-prefab', ), + 'd713a2c5' => array( + 'javelin-install', + 'javelin-dom', + 'phuix-icon-view', + 'phabricator-prefab', + ), 'd7a74243' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/webroot/rsrc/js/core/Prefab.js b/webroot/rsrc/js/core/Prefab.js index 2c3da10102..c5a8ef5e9d 100644 --- a/webroot/rsrc/js/core/Prefab.js +++ b/webroot/rsrc/js/core/Prefab.js @@ -101,7 +101,6 @@ JX.install('Prefab', { datasource.setSortHandler( JX.bind(datasource, JX.Prefab.sortHandler, config)); - datasource.setFilterHandler(JX.Prefab.filterClosedResults); datasource.setTransformer(JX.Prefab.transformDatasourceResults); var typeahead = new JX.Typeahead( @@ -256,37 +255,6 @@ JX.install('Prefab', { }, - /** - * Filter callback for tokenizers and typeaheads which filters out closed - * or disabled objects unless they are the only options. - */ - filterClosedResults: function(value, list) { - // Look for any open result. - var has_open = false; - var ii; - for (ii = 0; ii < list.length; ii++) { - if (!list[ii].closed) { - has_open = true; - break; - } - } - - if (!has_open) { - // Everything is closed, so just use it as-is. - return list; - } - - // Otherwise, only display the open results. - var results = []; - for (ii = 0; ii < list.length; ii++) { - if (!list[ii].closed) { - results.push(list[ii]); - } - } - - return results; - }, - /** * Transform results from a wire format into a usable format in a standard * way. diff --git a/webroot/rsrc/js/core/behavior-search-typeahead.js b/webroot/rsrc/js/core/behavior-search-typeahead.js index a981f3861e..864d45ff29 100644 --- a/webroot/rsrc/js/core/behavior-search-typeahead.js +++ b/webroot/rsrc/js/core/behavior-search-typeahead.js @@ -52,72 +52,55 @@ JX.behavior('phabricator-search-typeahead', function(config) { datasource.setTransformer(transform); - // Sort handler that orders results by type (e.g., applications, users) - // and then selects for good matches on the "priority" substrings if they - // exist (for instance, username matches are preferred over real name - // matches, and application name matches are preferred over application - // flavor text matches). - var sort_handler = function(value, list, cmp) { - var priority_hits = {}; - var type_priority = { - 'jump' : 1, - 'apps' : 2, - 'proj' : 3, - 'user' : 4, - 'repo' : 5, - 'symb' : 6 - }; - - var tokens = this.tokenize(value); + // First, sort all the results normally. + JX.bind(this, JX.Prefab.sortHandler, {}, value, list, cmp)(); + // Now we're going to apply some special rules to order results by type, + // so applications always appear near the top, then users, etc. var ii; + + var type_order = [ + 'jump', + 'apps', + 'proj', + 'user', + 'repo', + 'symb', + 'misc' + ]; + + var type_map = {}; + for (ii = 0; ii < type_order.length; ii++) { + type_map[type_order[ii]] = true; + } + + var buckets = {}; for (ii = 0; ii < list.length; ii++) { var item = list[ii]; - for (var jj = 0; jj < tokens.length; jj++) { - if (item.name.indexOf(tokens[jj]) === 0) { - priority_hits[item.id] = true; - } + var type = item.priorityType; + if (!type_map.hasOwnProperty(type)) { + type = 'misc'; } - if (!item.priority) { - continue; + if (!buckets.hasOwnProperty(type)) { + buckets[type] = []; } - for (var hh = 0; hh < tokens.length; hh++) { - if (item.priority.substr(0, tokens[hh].length) == tokens[hh]) { - priority_hits[item.id] = true; - } - } + buckets[type].push(item); } - list.sort(function(u, v) { - var u_type = type_priority[u.priorityType] || 999; - var v_type = type_priority[v.priorityType] || 999; - - if (u_type != v_type) { - return u_type - v_type; - } - - if (priority_hits[u.id] != priority_hits[v.id]) { - return priority_hits[v.id] ? 1 : -1; - } - - return cmp(u, v); - }); - // If we have more results than fit, limit each type of result to 3, so // we show 3 applications, then 3 users, etc. For jump items, we show only // one result. - var type_count = 0; - var current_type = null; - for (ii = 0; ii < list.length; ii++) { - if (list[ii].type != current_type) { - current_type = list[ii].type; - type_count = 1; - } else { - type_count++; + + var jj; + var results = []; + for (ii = 0; ii < type_order.length; ii++) { + var current_type = type_order[ii]; + var type_list = buckets[current_type] || []; + for (jj = 0; jj < type_list.length; jj++) { // Skip this item if: // - it's a jump nav item, and we already have at least one jump @@ -125,19 +108,21 @@ JX.behavior('phabricator-search-typeahead', function(config) { // - we have more items than will fit in the typeahead, and this // is the 4..Nth result of its type. - var skip = ((current_type == 'jump') && (type_count > 1)) || + var skip = ((current_type == 'jump') && (jj > 1)) || ((list.length > config.limit) && (type_count > 3)); if (skip) { - list.splice(ii, 1); - ii--; + continue; } + + results.push(type_list[jj]); } } + // Replace the list in place with the results. + list.splice.apply(list, [0, list.length].concat(results)); }; datasource.setSortHandler(JX.bind(datasource, sort_handler)); - datasource.setFilterHandler(JX.Prefab.filterClosedResults); datasource.setMaximumResultCount(config.limit); var typeahead = new JX.Typeahead(JX.$(config.id), JX.$(config.input)); diff --git a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js index a03c2adf70..16ed8b75f3 100644 --- a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js +++ b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js @@ -153,7 +153,6 @@ JX.install('PHUIXAutocomplete', { datasource.setTransformer(JX.bind(this, this._transformresult)); datasource.setSortHandler( JX.bind(datasource, JX.Prefab.sortHandler, {})); - datasource.setFilterHandler(JX.Prefab.filterClosedResults); this._datasources[code] = datasource; }