Skip to content
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

Save crate filtering on rustdoc #62941

Open
wants to merge 1 commit into
base: master
from

Conversation

@GuillaumeGomez
Copy link
Member

commented Jul 24, 2019

Fixes #62929.

I added a hashmap and a hash encoding for the current crate list in case you have multiple crates handling on a same website (who talked about docs.rs?!). Like that, for each context, you have the filter crate selected.

r? @QuietMisdreavus

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:save-crate-filter branch from ad00098 to ef31242 Jul 24, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@bors: try

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

⌛️ Trying commit ef31242 with merge eef6649...

bors added a commit that referenced this pull request Jul 27, 2019

Auto merge of #62941 - GuillaumeGomez:save-crate-filter, r=<try>
Save crate filtering on rustdoc

Fixes #62929.

I added a hashmap and a hash encoding for the current crate list in case you have multiple crates handling on a same website (who talked about docs.rs?!). Like that, for each context, you have the filter crate selected.

r? @QuietMisdreavus
@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

☀️ Try build successful - checks-azure
Build commit: eef6649

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jul 27, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2019

Rustdoc-UI starting test...

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

☀️ Try build successful - checks-azure
Build commit: eef6649

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

☀️ Try build successful - checks-azure
Build commit: eef6649

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... FAILED (images "basic-code-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "basic-code.png" are different)
basic... FAILED (images "basic-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "basic.png" are different)
code-sidebar-toggle-mobile... FAILED (images "code-sidebar-toggle-mobile-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "code-sidebar-toggle-mobile.png" are different)
code-sidebar-toggle... FAILED (images "code-sidebar-toggle-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "code-sidebar-toggle.png" are different)
list_code_block... FAILED (images "list_code_block-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "list_code_block.png" are different)
module-mobile... FAILED (images "module-mobile-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "module-mobile.png" are different)
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... FAILED (images "toggle-docs-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "toggle-docs.png" are different)

<= doc-ui tests done: 0 succeeded, 0 ignored, 8 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... ok

<= doc-ui tests done: 7 succeeded, 0 ignored, 1 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... ok

<= doc-ui tests done: 7 succeeded, 0 ignored, 1 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... ok

<= doc-ui tests done: 7 succeeded, 0 ignored, 1 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... ok

<= doc-ui tests done: 7 succeeded, 0 ignored, 1 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@highfive: run-doc-ui eef6649

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI starting test...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Rustdoc-UI tests failed "successfully"!

Click to expand the log.
=> Starting doc-ui tests...

basic-code... ok
basic... ok
code-sidebar-toggle-mobile... ok
code-sidebar-toggle... ok
list_code_block... ok
module-mobile... ok
theme-change... FAILED (images "theme-change-eef66491f53a32b2c37f3188a8b4cf6f481488db.png" and "theme-change.png" are different)
toggle-docs... ok

<= doc-ui tests done: 7 succeeded, 0 ignored, 1 failed
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

@Mark-Simulacrum
Copy link
Member

left a comment

If I'm following this patch correctly, it'll ignore the stored preference if the crate list has changed since the last time the page was loaded. That sounds not entirely helpful, at least in most scenarios, where you might have N crates (including dependencies) and one of your dependencies changing shouldn't really lead to the selector here getting reset.

With that all in mind, maybe this PR can be simplified quite a bit by just storing the single name of the crate most recently selected in local storage?

}

function getSavedCrates(savedCrates) {
if (typeof savedCrates === "undefined") {

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

Why is this checking for undefined here? Why can't call sites just pass in getCurrentValue(...) as needed?

It looks like there's two call sites to this function: one passes getCurrentValue after a !== null check, the other calls this function with no arguments. The discrepancy there is a bit odd.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

This is a good point, this check is completely useless. Good catch!

var elems = getSavedCrates(savedCrates);
if (elems.hasOwnProperty(getCratesHash())) {
onEach(window.crates, function(e) {
if (e === elems[window.hashCode]) {

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

Can we avoid storing the crates hash in a global variable and simply have it in a variable here? It looks like this function should presumably be called only once so presumably that's equivalent performance wise?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

Sure.

});
window.crates.sort();
}
window.hashCode = window.crates.join(",").hashCode();

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

The whole list of crates isn't that long in most cases, and we store it in memory here anyway, so why do we need to compute it's hash? Can't the crate list simply be stored as a string?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

For the storage, not just as is.

if (window.hashCode === undefined || window.hashCode === 0) {
if (window.crates === undefined || window.crates.length === 0) {
window.crates = cloneArray(document.getElementById("crate-search")
.getElementsByTagName("option"), 1)

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

AFAICT, we wouldn't actually break anything if we didn't skip the first element in the options list here (it's always the same, sure, but that doesn't hurt). Can we do that to simplify the code here a little? It should also allow dropping the skip arg from cloneArray which is pretty irrelevant to the main purpose of the function.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

It adds a useless element into the hash computation and slows it down. If we remove the hash system, then this skip will become useless indeed but for the moment it's important to keep it. :)

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

The cost of hashing this string is likely to be near-zero compared to all the other JS and such that we're running. I would rather pay that cost and have simpler to read code; as-is we at the very least should have a comment to the effect of strips out default 'all crates' at the beginning.

if (selectCrate) {
selectCrate.onchange = function() {
var savedCrates = getSavedCrates();
const value = selectCrate.value;
savedCrates[getCratesHash()] = value === "All crates" ? undefined : value;

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

As above, it seems reasonable that we can store "All crates" in local storage as well since it'll simplify the code here a little and ultimately doesn't affect anything presumably?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

It's just to avoid keeping unused values. Otherwise, every time you go visit a crate, it'll be added to the local storage, which doesn't sound that nice...

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 9, 2019

Author Member

It affect local storage space (which is surprisingly limited). Like this, we don't store the defaults.

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 9, 2019

Member

Does this work if there's only one crate? It seems like we presumably just don't run this code then? Can we get comments about:

  • the 'no select crate' dialog case
  • the reasoning for why we want to not store anything for the all crates case
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

With that all in mind, maybe this PR can be simplified quite a bit by just storing the single name of the crate most recently selected in local storage?

If I do this, it'll get applied to all documentations (on docs.rs for example) and then you'll get some conflicts. Which is why I used this system. If you update dependencies, it'll be reset but it sounded better than having conflicts between crates.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Hm, sure, but that seems like an orthogonal problem. On docs.rs it doesn't matter anyway I think because all crates are built on their own, there's no crate list anyway.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Yes but then you'll save this only for one crate. That doesn't seem very useful if done this way... If you provide a website which provides documentation for multiple crates and their dependencies, you'll need it (like doc.rust-lang.org for example which provides std and rustc-internals).

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I think it being reset is fine for now, since it simplifies the code quite a bit. The only notable site for which it'll matter I think is doc.rust-lang.org, and even there it's only if you're both a) using the selection dialog and b) navigate to both std docs and rustc internals docs. I'd guess the combination there is pretty rare, so solving that use case in this PR seems unnecessary.

If we were to solve that use case, I would prefer to solve it (not in this PR!) by having rustdoc itself, not the JS, embed some sort of identifier into pages of "build unit" style that we could then use. There's a lot of complexity here (hash function, etc.) that seems like it could be avoided for relatively low impact on most users, and I think we should do that.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

I think you're right. If I just store current selected crate name, is it good for you? (the PR will become much smaller indeed!)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Yes, I think this should store the selected crate name, and if it is not found, fall back to all crates (but, importantly, not remove the stored preference). I think that gives us the behavior most users want (at least on one "build") and it doesn't erase preferences unnecessarily.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

Ok, I'll rewrite the PR then. Thanks a lot for your review @Mark-Simulacrum!

@@ -1157,7 +1157,7 @@ themePicker.onblur = handleThemeButtonsBlur;
&format!("{}\n{}", variables.join(""), all_indexes.join("\n")),
options.enable_minification),
&dst);
try_err!(write!(&mut v, "initSearch(searchIndex);addSearchOptions(searchIndex);"), &dst);
try_err!(write!(&mut v, "addSearchOptions(searchIndex);initSearch(searchIndex);"), &dst);

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Aug 10, 2019

Member

Oh, I forgot to note during initial review -- can you explain why this has changed?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Aug 11, 2019

Author Member

A global variable story. Not sure if it'll be needed anymore once I apply the new changes you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.