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

rustdoc: design change to separate methods better and improve scanning #86283

Closed
wants to merge 4 commits into from

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jun 14, 2021

This is really more of a demo than a PR, to see if folks are interested in a significant design change like this.

I've heard multiple people say they feel that methods in rust documentation are not "scannable." In other words, it's hard to run your eyes down a page looking at all the method names to find a specific name, or to see if there's a name that matches what you're looking for.

Part of that is that there's a lot of stuff to the left of the method name, so the method name is not always in the same place. Part of that is that there's not a clear visual delineation between the end of one method's documentation and the summary of the next method.

In this demo I removed the [-] pub fn from the beginning of each function. My thinking is that [-] can be shown just when it needs to be. "fn" is implicit in the fact that we're in a section full of methods (and also the parentheses and arguments are a great hint that we're looking at fns).

"pub" is usually implicit since we're looking at documentation. There's the issue of --document-private-items, but we can solve that by marking the private items differently, rather than always marking public items so the private items can be noticed by their lack of "pub".

I've also highlighted the methods with a border-top and a light grey background-color (same as the color on examples). For this I took inspiration from hexdocs (https://hexdocs.pm/phoenix/Phoenix.html#json_library/0) and Sphinx with the readthedocs theme (https://certbot.eff.org/docs/api/certbot.plugins.dns_common_lexicon.html). Both of those use a background color and a single border to make method headings stand out: hexdocs uses a border-left, and pydoc uses a border-top. I chose the same color we use as a border-right when a heading is :target (that is, when you've clicked on it from the sidebar or from a URL with a fragment).

Anyhow, all sorts of feedback welcome. Do you see this as a problem in need of solving? Is this a good way to solve it? Are there other ways? I've intentionally not spent too much time polishing this PR, so if you don't wind up liking it don't worry that I'll be throwing too much work away.

/cc @jyn514 @Manishearth @GuillaumeGomez @Nemo157

Demo: https://hoffman-andrews.com/rust/scannable-methods/std/string/struct.String.html#implementations

Methods in default open position:

image

Methods in closed position:

image

jsha added 3 commits June 13, 2021 14:36
This reduces clutter on the page, while still supporting the "auto hide
X" settings and the "collapse all docs" button.
This was originally introduced in # 51387, to solve a UI problem: in a
list of collapsed methods, it was not clear whether the stability marker
attached to the item above it or the one below it.

That problem has now been solved in a couple of different ways:

 - The margin between the stability marker and the item it annotates is
   much smaller, indicating their relationship.
 - Stability markers are now collapsed along with the docblock.

Also, the arrow made our CSS fragile, since it was positioned absolutely
and needed to line up horizontally with the beginning of the item (or
with the link icon).

Aligning this with the rest of the docblock makes getting the CSS right
a lot easier.
When methods are toggled open, add a border-top, some background
highlighting, and tweak the spacing.

Also remove the `pub fn` in front of the method name.
@jsha jsha added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jun 14, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

Oh this is an interesting take! I like it!

"pub" is usually implicit since we're looking at documentation. There's the issue of --document-private-items, but we can solve that by marking the private items differently, rather than always marking public items so the private items can be noticed by their lack of "pub".

I think this is a good change, but we should do it consistently for all items, not just functions.

@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

I also think we should do the pub change separately and do it first, since it's a larger change and affects all items.

@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

My idea for visibility was to show pub(crate) or whatever for things that are private. How does that interact with removing fn? pub(crate) len() or whatever looks very strange. Maybe we should only remove it if the function is public?

@jsha
Copy link
Contributor Author

jsha commented Jun 14, 2021

An interesting question. The same issue applies for other keywords that appear before the fn, like const, async, and unsafe. You can see an example with unsafe in one of the screenshots above. I think it's not too bad even with the fn missing, but it could also make sense to put back the fn when there are any keywords to the left of it.

@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

it could also make sense to put back the fn when there are any keywords to the left of it.

I think that makes sense, yes.

I kind of want to hold off on deciding to remove fn until we remove pub, I think it might already be easier to scan at that point and it's more consistent.

@CraftSpider
Copy link
Contributor

The border looks really nice and definitely improves scanning, and I like the idea of removing pub. I'm with Joshua that it may make sense to remove pub first separately.

@jyn514
Copy link
Member

jyn514 commented Jun 15, 2021

Oh, @camelid you requested to be pinged if we were considering removing pub.

@bors
Copy link
Contributor

bors commented Jun 17, 2021

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

@Manishearth
Copy link
Member

Sorry it's taken me a while to look at this.

I love this! In fact I read the first few paragraphs and was like "hell yeah this is why I like the ReadTheDocs theme" and then you went on to say you were inspired by just that 😄 . A slight feeling is that the highlight shouldn't go away when collapsed, especially since it's a clickable region.

I'm not so sure about removing the pub and fn. I think it's nice that the docs look a lot like syntax, there's less of a learning curve. If we're doing that it might be worth askign the community for feedback as well.

And there are lots of users of --document-private-items, lots of large projects use it, so I think it's worth getting that feedback too.

@Manishearth
Copy link
Member

I think we should decouple the styling/collapsing change from removing pub/fn -- do the style change first, it stands pretty well on its own

@jsha
Copy link
Contributor Author

jsha commented Jun 18, 2021

A slight feeling is that the highlight shouldn't go away when collapsed, especially since it's a clickable region.

My thinking here is that, in a big list where every line is a method, the extra emphasis and separation of having a border and background color becomes noisy and actually hinders scanning. But I don't feel strongly about this - there's also a good argument for consistency between open and closed states.

image

I think we should decouple the styling/collapsing change from removing pub/fn -- do the style change first, it stands pretty well on its own

Yep, fine with this.

I'm not so sure about removing the pub and fn. I think it's nice that the docs look a lot like syntax, there's less of a learning curve.

To be clear: are you saying it's less of a learning curve to learn how to read the docs, or that docs looking like syntax helps with the learning curve of Rust's syntax (because people are repeatedly exposed to the syntax)?

I look at it this way: Every element we put in the summary competes with every other element for a reader's attention. So, for instance, pub competes with the method name and parameters. If we choose to make the summary use the same syntax for expressing pub (and friends), we significantly limit our ability to focus attention on what's important in that summary.

My somewhat more radical vision that I haven't fully formed is: the other modifiers (const, async, unsafe, default, and ABI) have varying levels of importance to the reader. For the functions that have const, e.g. String::new, their constness is usually not the most important thing about them; it could just as well be in the docblock. For unsafe functions, their unsafeness is very important to all uses! Why should const and unsafe get equal, top-priority billing? Another way of thinking about it is that these keywords' syntactic position is not correlated with their semantic importance. So it would be cool to potentially move some of these to another position - for instance, text in the docblock (linking to the relevant keyword), or an icon to the right of the method (with a link and suitable hover text), or different visual treatment in the summary. Or some combination of those.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jun 18, 2021

"pub" is usually implicit since we're looking at documentation. There's the issue of --document-private-items, but we can solve that by marking the private items differently, rather than always marking public items so the private items can be noticed by their lack of "pub".

When documenting private items, it's quite important to have the distinction somehow. Either by having "pub" or something else though (exactly as you said, just wanted to underline it a bit ;) ).

@GuillaumeGomez
Copy link
Member

So yes, overall I really like it. A small thought I had: for the [-], to reduce its "visual weight", we can make it appear only when we hover the item?

This looks really promising!

@camelid
Copy link
Member

camelid commented Jun 19, 2021

I like the new border styling a lot; it's helpful to have more visual separation. I wonder if impls should also have top border highlights though?

I'm not so sure about removing the pub and fn. I think it's nice that the docs look a lot like syntax, there's less of a learning curve.

I feel similarly: In terms of pub, I want to make sure that there's still a good experience with --document-private-items, which I use heavily. And for both pub and fn, hewing close to the Rust syntax (with the current styling we use, the docs use 100% correct syntax AFAICT) makes the docs simpler to approach and doesn't require mental translation between "docs display" and "source display" (whether that's rustdoc's source view, or your editor).

My somewhat more radical vision that I haven't fully formed is: the other modifiers (const, async, unsafe, default, and ABI) have varying levels of importance to the reader. For the functions that have const, e.g. String::new, their constness is usually not the most important thing about them; it could just as well be in the docblock. For unsafe functions, their unsafeness is very important to all uses! Why should const and unsafe get equal, top-priority billing? Another way of thinking about it is that these keywords' syntactic position is not correlated with their semantic importance. So it would be cool to potentially move some of these to another position - for instance, text in the docblock (linking to the relevant keyword), or an icon to the right of the method (with a link and suitable hover text), or different visual treatment in the summary. Or some combination of those.

I wonder if a different approach could be to put things like const in a lighter color? It might be worth investigating to see if that could simplify the pages without diverging from the Rust syntax.

I'm also wondering whether it might be useful to land part of this change behind a CLI flag, and then enable that flag by default for the nightly rustc API docs so we can get some user feedback before we fully land the change?

@Manishearth
Copy link
Member

But I don't feel strongly about this - there's also a good argument for consistency between open and closed states.

Yeah the lack of consistency is jarring for me.

To be clear: are you saying it's less of a learning curve to learn how to read the docs, or that docs looking like syntax helps with the learning curve of Rust's syntax (because people are repeatedly exposed to the syntax)?

Mostly that the familiarity is nice, more of the former.

Another way of thinking about it is that these keywords' syntactic position is not correlated with their semantic importance.

Oh I totally agree! However my counterargument here is that the order in which stuff appears in the source is familiar, people are already used to scanning that, this introduces a similar-but-not-identical syntax, which is going to be confusing. Perhaps we should instead use styling for emphasis and deemphasis? Perhaps grey out pub, but use different (non bright) colors for the other keywords.

Give same highlighting to closed blocks.
Smaller font size and weight for fn qualifiers, and same for `where`
clauses.

Use a darker highlight for the part of the bar directly over the
function name.
@jsha
Copy link
Contributor Author

jsha commented Jun 20, 2021

I've made some tweaks and pushed a new candidate design. The demo is at a new URL, so you can compare with the old one:

https://hoffman-andrews.com/rust/scannable-methods-rev2/std/string/struct.String.html#implementations

image

I restored pub fn and made the fn qualifiers smaller: 0.8em, same as the where clause. I also lowered the font weight for the qualifiers and the where clause. Thanks for the idea here @camelid and @Manishearth. I also made the highlighting consistent between open/closed states.

One of the reasons I wanted to ditch pub fn where possible is that the new style emphasizes not just the line overall, but specifically its left and right sides - so the combination of highlight plus pub fn actually felt like it made the function name harder to find. To improve that, beside the font size changes, I gave the border-top a different color when it's right above the function name. The result looks kinda odd but seems to work, IMO. Curious to hear what y'all think.

for the [-], to reduce its "visual weight", we can make it appear only when we hover the item?

This is a neat idea. I tried it, but ran into a problem: We can make the [-] appear based on hovering the summary, but once you move the mouse outside the summary to click the [-], it disappears. :-)

I wonder if impls should also have top border highlights though?

Impls should have some stronger visual distinction than they currently do, but not the same exact thing as the methods have. Perhaps making their font bigger / heavier?

When documenting private items, it's quite important to have the distinction somehow. Either by having "pub" or something else though (exactly as you said, just wanted to underline it a bit ;) ).

In terms of pub, I want to make sure that there's still a good experience with --document-private-items

Yep, I am very much onboard with making sure the experience of --document-private-items is good. I actually think adding text or another indicator that affirmatively marks something as private would be a better experience than the status quo. In the browser security world, there has been some research that shows people notice negative indicators but not the absence of normally positive indicators. That is, they notice "Not Secure", but they don't notice the absence of a "Secure" marking on a page that normally has it. I would guess it's somewhat hard to notice that fn foo() happens to lack the pub, but would be easy to notice something like:

foo()
  Private

However my counterargument here is that the order in which stuff appears in the source is familiar, people are already used to scanning that, this introduces a similar-but-not-identical syntax, which is going to be confusing

Ah, this is a good way to put it. So we have two values: familiarity vs ordering information by priority, and we are deciding how to trade off between them.

The latest demo tries to make that tradeoff by using signposts (border colors, font sizes) to overlay a priority order on top of the left-to-right reading order. It works-ish, but I think it's still not ideal. It feels like improving clarity by adding complexity rather than taking away complexity.

Another way I think of it is in terms of the cost of those displaying left-side qualifiers. One cost is that they slow down finding the function name. Another cost is that by causing us to put pub fn on everything, they take up room that could be used for other valuable stuff. One example is the "Iterator of &T" annotation I proposed on the forum. We can probably squeeze it in without removing anything else, but removing some stuff makes more room available.

@Manishearth
Copy link
Member

I like this! The border-top highlight feels a bit weird, but not that weird (maybe we also need a border-bottom? idk)

The only "complaint" I have about this is that the entire bar is the collapse toggle, but some parts of it are links, so if you click wrong it doesn't toggle and instead takes you elsewhere. I think this is fine, it's just a mistake I immediately made after opening the page, and no doubt others will to. But it feels like that's a mistake you make once, and I don't know how to fix this problem otherwise.

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

I would guess it's somewhat hard to notice that fn foo() happens to lack the pub, but would be easy to notice something like:

foo()
Private

However my counterargument here is that the order in which stuff appears in the source is familiar,

I suggest using pub(self) instead, which is the same meaning, valid rust, and still a negative indicator.

@Manishearth
Copy link
Member

I suggest using pub(self) instead, which is the same meaning, valid rust, and still a negative indicator.

Yeah, that makes sense, but it seems rather "clever" in that it requires some thought before it makes sense. Idk.

@jsha
Copy link
Contributor Author

jsha commented Jun 22, 2021

the entire bar is the collapse toggle, but some parts of it are links, so if you click wrong it doesn't toggle and instead takes you elsewhere.

This behavior actually crept in somewhat by accident during the toggle refactor. Since the whole bar is part of the summary tag, the whole bar is clickable. But I think the new highlighting makes it feel more clickable.

I think the right fix here is to add a JS event handler for clicks on the summary that cancels the default action, so clicks in places other than the [+] / [-] no longer open/close.

@Manishearth
Copy link
Member

Ah. Honestly the entire bar being the toggle does make some sense. But also fine with that not being the case as long as the +/- are always visible

@jsha
Copy link
Contributor Author

jsha commented Jun 25, 2021

Thanks for all the feedback on this! I'm taking a small break from rust dev so I'm going to close this for now to keep the PR list clean, but I hope to keep working on it when I get back. The main remaining issue is that the emphasis on the method name in the overhead highlighting doesn't work right when there is an attribute, because the attribute gets displayed above the function:

https://hoffman-andrews.com/rust/scannable-methods-rev2/std/string/struct.String.html#method.split_off

image

As of #86277, that will be a rarer occurrence, but we can still see attributes on methods in the case of #[no_mangle], #[export_name], and #[link_section]. The solution I'd like to try is moving attributes inside the docblock, e.g. "Attribute: no_mangle" (with a link to the reference).

Also of course if anyone is really into this change and would like to push it forward before I have a chance to, you are more than welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants