Page MenuHomePhabricator

[Redesign] PhabricatorApplicationSearchResultView
ClosedPublic

Authored by chad on Jun 17 2015, 7:50 PM.

Details

Summary

Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?

Test Plan

Review and do a search in each application changed. Grep for all call sites.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad updated this revision to Diff 32261.Jun 17 2015, 7:50 PM
chad retitled this revision from to [Redesign] PhabricatorApplicationSearchResultView.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
chad added a comment.Jun 18 2015, 6:11 PM

Would love to know if this was a bad direction?

epriestley edited edge metadata.Jun 18 2015, 6:15 PM

Pretty sure this is great, I just want to actually look through it before I accept.

btrahan edited edge metadata.Jun 18 2015, 7:37 PM

A few inline ideas that you are welcome to take or leave in my opinion.

src/applications/almanac/query/AlmanacDeviceSearchEngine.php
83

getResultTypeDescription() may be useful here? Its more formal than what you've put in though.

src/applications/search/view/PhabricatorApplicationSearchResultView.php
13

Style nit - usually null members aren't explicitly set to null (like noDataString is already)

26–36

since you are everywhere in this diff already maybe it makes sense to make sure setNoDataString is called consistently, so you can remove this fallback code?

56

Is there a typehint we can use for $content?

epriestley accepted this revision.Jun 18 2015, 8:15 PM
epriestley edited edge metadata.

I think this can be just a touch cleaner if all the parts that pick pieces out of the new View just live inside it, couple of inlines to that effect. Not a big deal either way, though, and easy to clean up later.

src/applications/almanac/query/AlmanacDeviceSearchEngine.php
83

I think it's untranslatable too.

src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
206–209

Maybe push this stuff into the new View too? (Possibly extend AphrontTagView.)

src/applications/search/controller/PhabricatorApplicationSearchController.php
230–262

Consider making the ResultView actually extend AphrontView and putting all this logic into the render() method.

src/applications/search/view/PhabricatorApplicationSearchResultView.php
56

I think this one might need to be generic, but some of the other setX(...) methods probably can have typehints, I think.

This revision is now accepted and ready to land.Jun 18 2015, 8:15 PM
chad added a comment.Jun 18 2015, 8:53 PM

Not sure how to assemble them back in the View. Dashboards and AppSearch each transform the bits in different ways, (like filters, header changes). Maybe this is above my engineering paygrade. 8)

src/applications/search/view/PhabricatorApplicationSearchResultView.php
56

Yeah this is generic for Calendar, Notifications, Pinboard, etc. We could maybe define them all, but seems easy to do in future if needed.

No worries -- just kick it in and I'll counterdiff you the next time I'm in that code.

This revision was automatically updated to reflect the committed changes.
chad marked 4 inline comments as done.