Skip to content

Rewrite search/browse pages #1021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 28, 2022
Merged

Rewrite search/browse pages #1021

merged 10 commits into from
Mar 28, 2022

Conversation

ysangkok
Copy link
Member

@ysangkok ysangkok commented Feb 21, 2022

This is my stab at https://summer.haskell.org/ideas.html#hackage-display .

The first commit adds the new page without disrupting the old. The second commit swaps in the new page instead of the old.

The new JSON search/browse endpoint is with POST (this error is expected):

% curl -X POST -d "browseOptions={}" "http://localhost:8080/packages/search"
Error in $: key "page" not found: 

The old JSON search endpoint is kept for compatibility reasons:

% curl -I -H "Accept: application/json" "http://localhost:8080/packages/search?terms=test"
[...]
Content-Type: application/json

The HTML endpoint now returns the new page when called with GET, but it does no computation on the server:

% curl -I "http://localhost:8080/packages/search?terms=test" 
[...]
Content-Type: text/html; charset=utf-8

Notable changes over the old:

  • Deprecation no longer shown in name column. Given that deprecated packages are how hidden by default, and the table is cramped, people can just look up this information on the package page.
  • I haven't tested on Apple, since I don't have an Apple device.
  • Explain feature is no longer available. This feature was only in the HTML version.
  • You can no longer adjust the page size.

Note that the package tag browser on /packages/tag/tagname is still generated by the old browse infrastructure.

@gbaz
Copy link
Contributor

gbaz commented Feb 23, 2022

This appears to work on a modern safari. I have only one styling comment -- as you can see in this screenshot, the "Advanced Options" expander is closer to the table than to the search box. I think it should be closer to the search box, and the bigger margin should then be between both and the table. Additionally, I think the left margin should be flush with the search box, and arguably the triangle expander should be to the left rather than right of the "Advanced Options" text:

image

@ysangkok
Copy link
Member Author

I have performed your suggested adjustments, and also similar ones inside the "Advanced options" section.

Collapsed view:
image

Expanded view:
image

Now the "Append" buttons are also flush with the left margin like the search query field. I have given "Advanced options" a larger margin to indicate how it is a "child" of the "Advanced options" heading. Of course I'll happily undo this if it is excessive.

@gbaz
Copy link
Contributor

gbaz commented Mar 6, 2022

i'm looking to merge this soon. one additional idea that's come up would be to potentially allow filtering by distros as well so people could filter by debian etc.

@ysangkok
Copy link
Member Author

I wrote some code to filter for distributions, but I still haven't tested it, so I won't push yet.

@ysangkok
Copy link
Member Author

ysangkok commented Mar 16, 2022

Ok, I have tested and pushed the distro filtering capability after executing the following commands:

# Add 'Debian' distro
curl -L -u admin:admin -XPUT localhost:8080/distro/Debian
# Mark 'tz' as available in 'Debian', note how the first response is a 3xx, which means success. Second 404 doesn't matter.
curl -D - -L -u admin:admin -XPUT "localhost:8080/distro/Debian/package/tz?version=0.1.3.6&uri=http://google.com"

@gbaz
Copy link
Contributor

gbaz commented Mar 24, 2022

can we do a global rename type thing to this pr that turns "new-browse" and "newBrowse" into just "browse" internally? If there's some resultant nameclashes, I'd rather turn whatever remains into "legacyBrowse".

Eventually the "new" thing will just be the thing, and all this new- prefixing will make the codebase more confusing.

(A lesson very much learned from cabal v2, lol).

@ysangkok
Copy link
Member Author

Ok, done. I should note that the search explanation feature is also removed in this PR. Here is a link to how it looks like: search?terms=test&explain=true (archived for future reference)

@andreasabel
Copy link
Member

Oops, looks like this PR got broken by my switch to aeson-2.0. But the fix is easy, most likely you can follow the pattern of what I did in:

@gbaz gbaz merged commit a22f92d into haskell:master Mar 28, 2022
@ysangkok ysangkok deleted the js-browse-and-search branch March 29, 2022 02:18
@gbaz
Copy link
Contributor

gbaz commented Apr 3, 2022

Now that this is deployed, I note that searching works really well, but pulling up the "full" browse results is rather expensive (8s) probably because of the sort of approx 15k items. I think it would be good to add some caching for this special case or otherwise consider how to make it more efficient.

@gbaz
Copy link
Contributor

gbaz commented Apr 3, 2022

As a smaller quality of life feature -- I think clicking on download count, upload date, or rating should first sort descending, then flip to ascending. Meanwhile, clicking on the name or the like should continue to sort ascending first.

@ysangkok
Copy link
Member Author

ysangkok commented Apr 3, 2022

It can't just be the sorting that is slow, since by default there is no sorting: note how the sort function in ApplyFilter.hs is actually the identity function if sortColumn is DefaultColumn (which it when the browse page is loaded).

I'll do some benchmarking within the next 24 hours and try to find out how it can be so slow.

After this performance issue is fixed, I'll work on having selected columns sort descending first, as suggested.

@ysangkok
Copy link
Member Author

ysangkok commented Apr 3, 2022

One problem could be that if there is that the default view actually has a filter: Exclude deprecated packages. And since we ask for the length of the result list (for calculating the page count), this filter is probably executed on every single package.

@gbaz
Copy link
Contributor

gbaz commented Apr 3, 2022

Ok my blind guess is that the not-search path calls toPackageNames, defined here:

toPackageNames :: PackageIndex.PackageIndex PkgInfo -> [PackageName]

The search path doesn't.

I think that function is probably overly expensive and can be improved, and worst case its results can simply be cached.

@ysangkok
Copy link
Member Author

ysangkok commented Apr 3, 2022

Ok, I submitted #1049 which should speed up toPackageNames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants