Add option to collapse automatically implementors #74196

GuillaumeGomez · 2020-07-09T19:43:12Z

Fixes #73403

It adds an option (enabled by default) which collapses all implementors impl blocks.

r? @kinnison
cc @rust-lang/rustdoc

rust-highfive · 2020-07-09T19:43:14Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

GuillaumeGomez · 2020-07-09T19:44:57Z

src/librustdoc/html/static/main.js

@@ -2241,8 +2241,7 @@ function defocusSearchBar() {
                relatedDoc = relatedDoc.nextElementSibling;
            }

-            if ((!relatedDoc && hasClass(docblock, "docblock") === false) ||
-                (pageId && document.getElementById(pageId))) {
+            if (!relatedDoc && hasClass(docblock, "docblock") === false) {


Little note about this change for the reviewers: it was to prevent an element targetted by the url hash (so #variant.X for example) to be hidden. However, it has been long fixed and this code wasn't removed. And this case, it was a big issue in case you had #implementors as page id because then it prevented the impl blocks to be collapsed. I tested without this check for all other cases and it works fine.

Manishearth · 2020-07-09T19:52:33Z

src/librustdoc/html/render.rs

@@ -1291,6 +1291,7 @@ fn settings(root_path: &str, suffix: &str) -> String {
        ("auto-hide-method-docs", "Auto-hide item methods' documentation", false).into(),
        ("auto-hide-trait-implementations", "Auto-hide trait implementations documentation", true)
            .into(),
+        ("auto-collapse-implementors", "Auto-collapse implementors", true).into(),


I think this is confusing, I wonder if we can just merge this with the previous option?

Or perhaps two settings:

Auto-hide trait implementation documentation

Auto-hide trait implementation items

Hum... Maybe a rewording? But I think it'd be nice trying to make settings more obvious more generally. In this case, we target just the implementors (the section and its children), not the trait implementations generally.

Yeah I just think that when you have so many it gets confusing. Definitely when I look at the page I'm unsure as to what some of the options do.

Rewording seems fine

I'm open to suggestions then! Btw: should I do it in this PR directly or should I open a new one?

Do it here itself. I suggested options for the wording above for the two settings

Oh indeed! Thanks, sending an update shortly.

GuillaumeGomez · 2020-07-09T20:51:41Z

Updated!

src/librustdoc/html/render.rs

jyn514 · 2020-07-09T21:45:33Z

src/librustdoc/html/static/main.js

@@ -2362,6 +2361,7 @@ function defocusSearchBar() {
    (function() {
        var toggle = createSimpleToggle(false);
        var hideMethodDocs = getCurrentValue("rustdoc-auto-hide-method-docs") === "true";
+        var hideImplementors = getCurrentValue("rustdoc-auto-collapse-implementors") !== "false";


Just curious - why do these use strings instead of true and false?

When you use the local storage, it converts everything to string. As simple as that. :)

GuillaumeGomez · 2020-07-09T21:49:19Z

Updated!

Manishearth · 2020-07-09T22:42:36Z

src/librustdoc/html/render.rs

            .into(),
+        ("auto-collapse-implementors", "Auto-hide the types which implement a trait", true).into(),


"implementors of a trait"

Oh that's funny, I just asked guillaume to change it from that to the current text. Why do you think 'implementors' is more clear?

It's more concise without being ambiguous. Typically you don't want to use compound clauses in a setting name, it should be short but descriptive

Manishearth · 2020-07-09T22:43:24Z

src/librustdoc/html/render.rs

@@ -1289,8 +1289,9 @@ fn settings(root_path: &str, suffix: &str) -> String {
            .into(),
        ("auto-hide-attributes", "Auto-hide item attributes.", true).into(),


overall i think we might need to fix up the language used for all these settings in a separate issue, it seems very unclear (cc @jyn514 thoughts?)

It's not terribly clear but it does say exactly what it does (attributes is the technical term for #[some_macro], right?). I think it would be helpful to have screenshots before and after, then it's very clear to see.

I'm thinking of the entire set of names, not just this one, though. This one is kinda fine.

GuillaumeGomez · 2020-07-10T08:11:57Z

Updated!

GuillaumeGomez · 2020-07-15T11:48:18Z

ping

jyn514 · 2020-07-15T12:24:05Z

Like I said, looks good to me. Not sure if you want to wait for other reviewers though.

GuillaumeGomez · 2020-07-15T12:25:45Z

Sorry, didn't see your comment. Then let's go, thanks for your review!

@bors: r=jyn514

bors · 2020-07-15T12:25:46Z

📌 Commit 40abdf76ae572500f9c7cc50279a84b690fc500b has been approved by jyn514

Manishearth · 2020-07-15T15:04:54Z

src/librustdoc/html/render.rs

            .into(),
+        ("auto-collapse-implementors", "Auto-hide implentors of a trait", true).into(),


implementors

Outch, good catch!

GuillaumeGomez · 2020-07-15T15:13:34Z

@bors: r-

GuillaumeGomez · 2020-07-15T15:43:13Z

Updated! (the commits are just now showing up apparently?)

Manishearth · 2020-07-15T17:03:20Z

@bors r+

bors · 2020-07-15T17:03:27Z

📌 Commit 39d99ea has been approved by Manishearth

@ghost

…arth Rollup of 14 pull requests Successful merges: - rust-lang#72973 (RISC-V GNU/Linux as host platform) - rust-lang#73918 (Clean up E0715 explanation) - rust-lang#73959 (Clean up E0716 explanation) - rust-lang#74119 (Remove `Compiler::compile()`.) - rust-lang#74196 (Add option to collapse automatically implementors) - rust-lang#74218 (Add margin after doc search results) - rust-lang#74276 (improve DiscriminantKind handling) - rust-lang#74291 (Added docs for `From<c_int>` for `ExitStatus`) - rust-lang#74294 (Update cross-compilation README) - rust-lang#74337 (Handle case of incomplete local ty more gracefully) - rust-lang#74344 (Remove string comparison and use diagnostic item instead) - rust-lang#74347 (Initialize default providers only once) - rust-lang#74353 (Edit docs for rustc_middle::dep_graph::dep_node) - rust-lang#74374 (Add a 1.45 release note on lto vs. embed-bitcode) Failed merges: - rust-lang#74251 (Teach bootstrap about target files vs target triples) r? @ghost

Add option to collapse automatically implementors

970174a

rust-highfive assigned kinnison Jul 9, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2020

GuillaumeGomez commented Jul 9, 2020

View reviewed changes

Manishearth reviewed Jul 9, 2020

View reviewed changes

jyn514 reviewed Jul 9, 2020

View reviewed changes

GuillaumeGomez force-pushed the auto-collapse-implementors branch from 0c0eea6 to cb8bdd5 Compare July 9, 2020 21:49

jyn514 approved these changes Jul 9, 2020

View reviewed changes

Manishearth reviewed Jul 9, 2020

View reviewed changes

GuillaumeGomez force-pushed the auto-collapse-implementors branch from cb8bdd5 to 40abdf7 Compare July 10, 2020 08:11

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 Jul 15, 2020

Manishearth reviewed Jul 15, 2020

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2020

Improve settings wording

39d99ea

GuillaumeGomez force-pushed the auto-collapse-implementors branch from 40abdf7 to 39d99ea Compare July 15, 2020 16:39

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2020

Manishearth mentioned this pull request Jul 15, 2020

Rollup of 14 pull requests #74375

Merged

bors merged commit efad203 into rust-lang:master Jul 16, 2020

GuillaumeGomez deleted the auto-collapse-implementors branch July 16, 2020 08:07

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to collapse automatically implementors #74196

Add option to collapse automatically implementors #74196

GuillaumeGomez commented Jul 9, 2020 •

edited

Loading

rust-highfive commented Jul 9, 2020

GuillaumeGomez Jul 9, 2020

Manishearth Jul 9, 2020

GuillaumeGomez Jul 9, 2020

Manishearth Jul 9, 2020

GuillaumeGomez Jul 9, 2020

Manishearth Jul 9, 2020

GuillaumeGomez Jul 9, 2020

GuillaumeGomez commented Jul 9, 2020

jyn514 Jul 9, 2020

GuillaumeGomez Jul 9, 2020

GuillaumeGomez commented Jul 9, 2020

Manishearth Jul 9, 2020

jyn514 Jul 10, 2020

Manishearth Jul 10, 2020

Manishearth Jul 9, 2020

jyn514 Jul 10, 2020

Manishearth Jul 10, 2020

GuillaumeGomez commented Jul 10, 2020

GuillaumeGomez commented Jul 15, 2020

jyn514 commented Jul 15, 2020

GuillaumeGomez commented Jul 15, 2020

bors commented Jul 15, 2020

Manishearth Jul 15, 2020

GuillaumeGomez Jul 15, 2020

GuillaumeGomez commented Jul 15, 2020

GuillaumeGomez commented Jul 15, 2020

Manishearth commented Jul 15, 2020

bors commented Jul 15, 2020

		.into(),
		("auto-collapse-implementors", "Auto-hide the types which implement a trait", true).into(),

		@@ -1289,8 +1289,9 @@ fn settings(root_path: &str, suffix: &str) -> String {
		.into(),
		("auto-hide-attributes", "Auto-hide item attributes.", true).into(),

Add option to collapse automatically implementors #74196

Add option to collapse automatically implementors #74196

Conversation

GuillaumeGomez commented Jul 9, 2020 • edited Loading

rust-highfive commented Jul 9, 2020

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 commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 9, 2020

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 commented Jul 10, 2020

GuillaumeGomez commented Jul 15, 2020

jyn514 commented Jul 15, 2020

GuillaumeGomez commented Jul 15, 2020

bors commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 15, 2020

GuillaumeGomez commented Jul 15, 2020

Manishearth commented Jul 15, 2020

bors commented Jul 15, 2020

GuillaumeGomez commented Jul 9, 2020 •

edited

Loading