Skip to content

Fix playpen links to not all be the same #20990

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 1 commit into from
Jan 15, 2015

Conversation

estsauver
Copy link
Contributor

In #20732, that all links in some modules point to the same code
examples was reported. The ID's generated for documents in
librustdoc are not all unique, which means the code rendered as
text is not being properly selected.

This change makes the link to the code section that is next to
the current link.

@rust-highfive
Copy link
Contributor

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@estsauver
Copy link
Contributor Author

r? steveklabnik

If you have suggestions for how to fix the underlying html generation, I'd love to make that fix so that the docs divs all have unique ids.

~Earl

@steveklabnik
Copy link
Member

I'd like @huonw or @gankro or someone who knows rustdoc better to review this, I have no idea if this is right or not.

@Gankra
Copy link
Contributor

Gankra commented Jan 12, 2015

This definitely fixes the problem, but it's basically patching over the flaw that the ids are incorrectly generated in rustdoc itself.

@Gankra
Copy link
Contributor

Gankra commented Jan 12, 2015

The problem appears to be in: https://github.com/rust-lang/rust/blob/master/src/librustdoc/html/markdown.rs#L196-L213

There's clearly some code to increment a counter, but that's obviously not working.

@Gankra
Copy link
Contributor

Gankra commented Jan 12, 2015

Yeah it's predictably using a thread-local static: https://github.com/rust-lang/rust/blob/master/src/librustdoc/html/markdown.rs#L158

I haven't been following all the TLS reforms, but maybe something changed that broke this?

@alexcrichton
Copy link
Member

I think it's fine to fix the JS instead, if we can use siblings then there's no need for the counter than @gankro pointed out and it could just be removed (or the counter could be fixed).

@Gankra
Copy link
Contributor

Gankra commented Jan 12, 2015

Killing TLSes in rustdoc is a big 👍 from me

@estsauver
Copy link
Contributor Author

@alexcrichton I would really like for each of these div's to either have unique id's or hopefully use classes eventually.

Can I suggest we take this approach initially and open a subsequent bug for updating the counter to not use TLS's? I'd love to dive into that project but could use some help/a mentor to get a hand on what's going on internally.

@alexcrichton
Copy link
Member

Sure! In that case, could the selector be altered to .rusttest instead of using an id which is possibly duplicated elsewhere on the page?

Otherwise, the relevant code should be pretty easy to remove, I believe you'll just need to remove these lines:

And then just fix the fallout from that. Either way is fine by me!

Fixes rust-lang#20732, that all links in some modules point to the same code
examples was reported. The ID's generated for documents in
librustdoc are not all unique, which means the code rendered as
text is not being properly selected.

This change removes the unique id generation and instead changes the
frontend code to grab the correct code sample by it's relative
position in the dom.
@estsauver
Copy link
Contributor Author

I wasn't sure how to do some of the argument wrapping/whitespace on line 206 in html/markdown.rs.

I saw this TBD style guide, but is there another good source for "good style" rust currently?

https://github.com/rust-lang/rust-guidelines/blob/master/style/README.md

@alexcrichton
Copy link
Member

Looks good to me, thanks!

bors added a commit that referenced this pull request Jan 15, 2015
In #20732, that all links in some modules point to the same code
examples was reported. The ID's generated for documents in
librustdoc are not all unique, which means the code rendered as
text is not being properly selected.

This change makes the link to the code section that is next to
the current link.
@bors bors merged commit 2a320f2 into rust-lang:master Jan 15, 2015
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.

7 participants