Add crate filter parameter in URL #92735

GuillaumeGomez · 2022-01-10T16:49:27Z

Fixes #92621.

r? @jsha

rust-highfive · 2022-01-10T16:49:30Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

jsha

Can you add a description of 0bd7b06fdf5c3cfa9e69728542bbc751b690a4e0 in the PR description?

src/librustdoc/html/static/js/search.js

jsha · 2022-01-15T17:57:49Z

src/librustdoc/html/static/js/search.js

            if (!history.state && !params.search) {
-                history.pushState(query, "", newURL);
+                history.pushState(query.raw, "", newURL);


Why the change to query.raw here? Do we actually use this field of the history state?

It was invalid before for some reason...

Also, we don't use as it seems. So I guess it only makes it smaller?

I'll clarify: We should just pass null here (and in the replaceState call below) because we never use the resulting state in our popstate event handler.

Oh I see! Let's do this then!

src/librustdoc/html/static/js/search.js

jsha · 2022-01-15T18:08:32Z

src/librustdoc/html/static/js/search.js

+        if (ev.target.value === "All crates") {
+            removeLocalStorageValue("saved-filter-crate");
+            // If we don't remove it from the URL, it'll be picked up again by the search.
+            var params = searchState.getQueryStringParams();
+            var query = searchState.input.value.trim();


Why is the "update URL" code path only activated when selecting "All crates"? It seems like we should always update the URL after the user makes a selection from the crates dropdown.

Because it'll be done in the search for the other cases (ie, when we have a filter crate). This needs to be changed the current URL before we make the search so that the filter crate won't be picked since there isn't one anymore.

src/librustdoc/html/static/js/search.js

GuillaumeGomez · 2022-02-02T22:44:04Z

Updated!

jsha · 2022-02-03T08:17:32Z

src/librustdoc/html/static/js/main.js

@@ -134,6 +134,18 @@ function hideThemeButtonState() {
    themePicker.style.borderBottomLeftRadius = "3px";
 }

+function buildUrl(search, filterCrates) {


Let's add some JSDoc comments to this function, indicating that filterCrates is optional - or better yet, make it non-optional, and specify that it should be "null" for "no filter". Then we can remove the !== "undefined" check below.

Also, let's move this function to search.js. It's search-specific functionality, after all. And we shouldn't call it in hideResults; we should keep the code currently there, which doesn't care about search URL parameters.

In general, many of the things in searchState can and should be moved to search.js eventually. What's in there is the stuff that had relationships with other things in main.js that I didn't want to untangle in my initial refactoring to split search.js.

jsha · 2022-02-03T08:22:08Z

src/librustdoc/html/static/js/main.js

+        // This function will be replaced once the search has been loaded.
+        getFilterCrates: function() {


Let's not do this. Instead, let's move putBackSearch into search.js.

Since putBackSearch is referenced from the event listener created in setup, we'll also have to move the registration of that event listener into search.js.

Actually it's not possible because we load the search.js file thanks to this event, making its functions unavailable until they're loaded (so after this event is triggered). I'll try to find another way.

Nevermind, it's totally possible. I misunderstood what putBackSearch was doing.

In main.js we have:

setup: function() { ... search_input.addEventListener("focus", function() { searchState.putBackSearch(this); search_input.origPlaceholder = searchState.input.placeholder; search_input.placeholder = "Type your search here."; loadSearch(); }); search_input.addEventListener("blur", function() { search_input.placeholder = searchState.input.origPlaceholder; });

Those addEventListener calls should be moved into search.js, in registerSearchEvents. I think that should work. At the time that function runs, putBackSearch will be available, even if it's defined inside search.js.

I still need to keep focus in here because it loads the script. And to make it smoother, I'll keep the placeholder update before the search scripts are loaded.

By the way, I've been thinking: maybe we can do the placeholder with CSS: https://developer.mozilla.org/en-US/docs/Web/CSS/::placeholder

#search-input::placeholder { content: "Click or press 'S' for search..."; } #search-input:focus::placeholder { content: "Type your search here."; }

I thought you didn't want to put text in the CSS? I remember something like that. Well, if so, I'll send a small PR for that. :)

Just tried it, you can't set content for ::placeholder unfortunately...

src/librustdoc/html/static/js/search.js

jsha · 2022-02-03T08:30:55Z

src/librustdoc/html/static/js/search.js

            if (!history.state && !params.search) {
-                history.pushState(query, "", newURL);
+                history.pushState(query.raw, "", newURL);


I'll clarify: We should just pass null here (and in the replaceState call below) because we never use the resulting state in our popstate event handler.

jsha · 2022-02-03T10:08:02Z

I had another thought about this: We could make the crate dropdown edit the textual search. For instance, if you are on https://doc.rust-lang.org/nightly/std/index.html?search=panic and select "core" from the drop-down, the URL would change to https://doc.rust-lang.org/nightly/std/index.html?search=panic+crate:core. And we would also parse crate:core in the search box to mean "filter to just the core crate."

GuillaumeGomez · 2022-02-03T10:10:45Z

That would mean strongly changing the search syntax (which I have another PR for :p). Better to be done as part of the other PR. Or in a follow-up, as you prefer. But changing the search input syntax isn't a good idea before the parser is merged.

GuillaumeGomez · 2022-02-03T22:10:48Z

I think I applied all your comments as much as I could. I moved parts of the event subscriptions on the search input in the search.js file but I couldn't move all of them (the focus one cannot for example).

jsha · 2022-02-04T06:34:04Z

@bors r+ rollup

Looks great, thanks!

bors · 2022-02-04T06:34:06Z

📌 Commit 829a047 has been approved by jsha

…askrgr Rollup of 11 pull requests Successful merges: - rust-lang#92735 (Add crate filter parameter in URL) - rust-lang#93402 (Windows: Disable LLVM crash dialog boxes.) - rust-lang#93508 (Add rustdoc info to jsondocck output) - rust-lang#93551 (Add package.json in gitignore) - rust-lang#93555 (Link `try_exists` docs to `Path::exists`) - rust-lang#93585 (Missing tests for rust-lang#92630) - rust-lang#93593 (Fix ret > 1 bound if shadowed by const) - rust-lang#93630 (clippy::perf fixes) - rust-lang#93631 (rustc_mir_dataflow: use iter::once instead of Some().into_iter) - rust-lang#93632 (rustdoc: clippy::complexity fixes) - rust-lang#93638 (rustdoc: remove unused Hash impl) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-search Area: Rustdoc's search feature A-rustdoc-js Area: Rustdoc's JS front-end labels Jan 10, 2022

rust-highfive assigned jsha Jan 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the crate-filter-url-param branch from 26b36ee to 50bd5fe Compare January 10, 2022 17:10

jsha requested changes Jan 15, 2022

View reviewed changes

Unify storage getter and setter functions

7efba33

GuillaumeGomez force-pushed the crate-filter-url-param branch 3 times, most recently from 339a242 to 4b216f5 Compare February 2, 2022 22:17

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the crate-filter-url-param branch 2 times, most recently from eeeb9d5 to 075a97c Compare February 2, 2022 22:43

This comment has been minimized.

Sign in to view

jsha requested changes Feb 3, 2022

View reviewed changes

GuillaumeGomez force-pushed the crate-filter-url-param branch from 075a97c to c3b9dca Compare February 3, 2022 21:43

This comment has been minimized.

Sign in to view

GuillaumeGomez added 3 commits February 3, 2022 22:59

Add filter-crate URL parameter

cfa6776

Update tester to have FILTER_CRATE set to null if undefined.

17fe306

Add GUI test for crate filter URL parameter

829a047

GuillaumeGomez force-pushed the crate-filter-url-param branch from c3b9dca to 829a047 Compare February 3, 2022 21:59

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2022

matthiaskrgr mentioned this pull request Feb 4, 2022

Rollup of 11 pull requests #93645

Merged

bors merged commit f2721fa into rust-lang:master Feb 4, 2022

rustbot added this to the 1.60.0 milestone Feb 4, 2022

GuillaumeGomez deleted the crate-filter-url-param branch February 4, 2022 23:00

camelid mentioned this pull request Aug 8, 2022

rustdoc doesn't save filtered crate for search #100265

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add crate filter parameter in URL #92735

Add crate filter parameter in URL #92735

GuillaumeGomez commented Jan 10, 2022

rust-highfive commented Jan 10, 2022

This comment has been minimized.

jsha left a comment

jsha Jan 15, 2022

GuillaumeGomez Feb 2, 2022

GuillaumeGomez Feb 2, 2022

jsha Feb 3, 2022

GuillaumeGomez Feb 3, 2022

jsha Jan 15, 2022

GuillaumeGomez Feb 2, 2022 •

edited

Loading

This comment has been minimized.

GuillaumeGomez commented Feb 2, 2022

This comment has been minimized.

jsha Feb 3, 2022

jsha Feb 3, 2022

GuillaumeGomez Feb 3, 2022

GuillaumeGomez Feb 3, 2022

GuillaumeGomez Feb 3, 2022

jsha Feb 3, 2022

GuillaumeGomez Feb 3, 2022

jsha Feb 3, 2022

GuillaumeGomez Feb 3, 2022

GuillaumeGomez Feb 3, 2022

jsha Feb 3, 2022

jsha commented Feb 3, 2022

GuillaumeGomez commented Feb 3, 2022

This comment has been minimized.

GuillaumeGomez commented Feb 3, 2022

jsha commented Feb 4, 2022

bors commented Feb 4, 2022

		// This function will be replaced once the search has been loaded.
		getFilterCrates: function() {

Add crate filter parameter in URL #92735

Add crate filter parameter in URL #92735

Conversation

GuillaumeGomez commented Jan 10, 2022

rust-highfive commented Jan 10, 2022

This comment has been minimized.

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

GuillaumeGomez commented Feb 2, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Feb 3, 2022

GuillaumeGomez commented Feb 3, 2022

This comment has been minimized.

GuillaumeGomez commented Feb 3, 2022

jsha commented Feb 4, 2022

bors commented Feb 4, 2022

GuillaumeGomez Feb 2, 2022 •

edited

Loading