Page MenuHomePhabricator

Modernize QuickSearch typeahead
ClosedPublic

Authored by avivey on Nov 5 2017, 2:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 1:00 AM
Unknown Object (File)
Mon, Nov 18, 7:30 AM
Unknown Object (File)
Fri, Nov 15, 12:07 AM
Unknown Object (File)
Sat, Nov 9, 9:29 PM
Unknown Object (File)
Wed, Nov 6, 8:18 PM
Unknown Object (File)
Wed, Nov 6, 6:55 AM
Unknown Object (File)
Wed, Nov 6, 12:08 AM
Unknown Object (File)
Wed, Nov 6, 12:08 AM
Subscribers
Tokens
"Hungry Hippo" token, awarded by epriestley.

Details

Summary

Use ClassQuery to find datasources for the quick-search.

Mostly, this allows extensions to add quicksearches.

Test Plan

using /typeahead/class/, tested several search terms that make sense.
Removed the tag interface from a datasource, which removed it from results.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

avivey requested review of this revision.Nov 5 2017, 2:41 PM

I could add an isAvailableInGlobalSearch() method to PhabricatorTypeaheadDatasource, but it felt like there are too many of those to filter in run-time.

I think this feature is reasonable, but I don't like doing this with an interface because these things won't work:

First, there's no way to configure the datasource object. Some datasources are configurable, usually with setParameters(). For example, the PhabricatorProjectDatasource takes a mustHaveColumns parameter to return only projects with workboards. Written like this, it's impossible to add "Projects With Workboards" to the quicksearch unless you copy/paste the datasource and hard-code the parameters you want. Obviously, this example is not likely to arise in practice, but other datasources (now or in the future) may reasonably benefit from configuration in quicksearch. (I suspect parameters may not currently be passed properly to Composite datasources, but we could resolve that later.)

Second, there's no way for a source to disable itself based on configuration.

Third, there's no way to add an upstream source which we have decided does not make sense in the quicksearch. For example, if you want Nuance Queues to appear, you have to copy/paste the whole NuanceQueueDatasource.

I think we can address these with a QuickSearchEngine and then a QuickSearchEngineExtension class tree instead. The EngineExtension classes would have a method like this:

public function newQuickSearchDatasources() {
  return array( ... );
}

That would solve setting datasource parameters, adding or removing sources based on configuration, and adding upstream sources.

Additionally, these classes could later be made responsible for the closely related logic in PhabricatorJumpNavHandler, which should also be modularized at some point.

This revision now requires changes to proceed.Nov 16 2017, 5:59 PM

Re-implement using EngineExtension

This revision is now accepted and ready to land.Nov 30 2017, 2:56 PM

Those are really good points, thanks!

src/applications/search/engine/PhabricatorQuickSearchEngine.php
5–8

This feels a little anemic... Is there anything else this class should be doing?

I think it'll get to do the Jump nav stuff eventually (PhabricatorJumpNavHandler) but it's fine to just have the little skeleton for now.

This revision was automatically updated to reflect the committed changes.