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

Unify generation of primitive links for associated types with the rest #114096

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 26, 2023

It is very weird to have this display:

image

Where you have &Rhs actually linking to the reference primitive. I think it'd be better to unify the behaviour with the other items (non-generics ones).

For some history, it was changed this way in #107244 to prevent having some symbols linking to different pages than the item following them.

It now looks like this:

image

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 26, 2023
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Updated existing tests.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Correctly updated this time...

@notriddle
Copy link
Contributor

notriddle commented Jul 26, 2023

This PR is removing a lot of links, and it results in array links that look like this:

&'static [T; 1]

That link is pretty hard to click, and its label is not very informative. I know you could get it to come up that way before, too, but if we're going to change these links again, I'd like to improve it by trying not to generate poor quality link text and link visualization.

@GuillaumeGomez
Copy link
Member Author

Good point. I'll remove this last link as well then.

@GuillaumeGomez
Copy link
Member Author

Removed link generation for arrays.

@bors
Copy link
Contributor

bors commented Jul 27, 2023

☔ The latest upstream changes (presumably #114130) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@notriddle
Copy link
Contributor

I agree that this is weird, but previous discussions have shown that links to the slice and array pages, at least, are helpful (especially as “on-ramps”)

#98069 (comment)

It looks like this change removes all of the links to primitive.slice.html from function signatures. I’m not sure that’s a good idea.

@GuillaumeGomez
Copy link
Member Author

I improved the current handling. You can test it here. I also updated the screenshot. So in short, I improved display of slice links and the reference links don't include the generic anymore.

@bors
Copy link
Contributor

bors commented Aug 16, 2023

☔ The latest upstream changes (presumably #114905) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@notriddle
Copy link
Contributor

notriddle commented Sep 28, 2023

The sample docs at https://rustdoc.crud.net/imperio/generics-display/foo/trait.Trait.html still have a reference link, &Rhs, with a single-character link. That's not a large enough click target.

They also have *const Struct, a pair of links to different targets smashed together with no ability to tell there's more than one link. That's not user-friendly.

@GuillaumeGomez
Copy link
Member Author

The sample docs at https://rustdoc.crud.net/imperio/generics-display/foo/trait.Trait.html still have a reference link, &Rhs, with a single-character link. That's not a large enough click target.

That's what I originally wanted to fix. It was bugging me that Rhs was included in the link. I can make the link bigger though (with padding). Could be an interesting lead to experiment.

They also have *const Struct, a pair of links to different targets smashed together with no ability to tell there's more than one link. That's not user-friendly.

I think the problem here is that both links have the same color. They are different types and link to different items, therefore they should not point to the same page.

@GuillaumeGomez
Copy link
Member Author

One thing I thought that could improve greatly the situation for *const struct would be to add underline on hover on the links. What do you think?

@notriddle
Copy link
Contributor

notriddle commented Sep 28, 2023

It was bugging me that Rhs was included in the link. I can make the link bigger though (with padding). Could be an interesting lead to experiment.

That would help with the click target problem. I’m not sure how to keep it from breaking the monospaced grid and looking weird.

underline on hover on the links

Underlines won’t be helpful on touch, and even with a mouse it’s not great to rely on that.

I honestly feel kind of trapped here, because all of the options I can think of have serious downsides.
  • Back-to-back links that can only be distinguished by underlines are a discoverability problem, and can be frustrating if you wind up clicking the wrong thing because you don’t realize they’re separate.
  • Giving them both different colors is a distracting UI. We used to do that, and it caused problems.
  • Not coloring the *const part at all while still making it a link would also not be discoverable.
  • Removing the link entirely makes it harder to discover the primitive page.
  • Adding a setup where these are both one link, and clicking it brings up a popover asking if you want to go to the struct Foo page or the primitive pointer page would add friction for power users, who are going to want the struct page almost every time.
  • Making struct Foo the only inline link, while making primitive pointer off to the side in an (i) popover, makes it less useful as a teaching aid for anyone who didn’t already know what *const meant.
  • Linking only the star, as in *const Struct, is not a solution that can generalize to shared references, and it adds a one-character click target.
  • Generating different links for generic and concrete types is inconsistent, and including the generic parameter name in the link text can be misleading. (I think that is why you opened this PR to begin with? Correct me if I’m wrong.)

@bors
Copy link
Contributor

bors commented Sep 28, 2023

☔ The latest upstream changes (presumably #116230) made this pull request unmergeable. Please resolve the merge conflicts.

@tranquillity-codes tranquillity-codes 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2024
@tranquillity-codes
Copy link

Ping from triage, @GuillaumeGomez any updates on resolving the conflicts? Thanks!

@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants