Skip to content

rustdoc edit links #2985

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

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

rustdoc edit links #2985

wants to merge 5 commits into from

Conversation

SOF3
Copy link

@SOF3 SOF3 commented Sep 10, 2020

Adds an edit button to rustdoc-generated pages

Related: rust-lang/docs.rs#1007

Rendered

@ehuss
Copy link
Contributor

ehuss commented Sep 16, 2020

Assuming this is intended for docs.rs to implement, how would it construct the URL? Is that something that would go in [package.metadata.docs.rs]?

@SOF3
Copy link
Author

SOF3 commented Sep 17, 2020

The repository attribute in Cargo.toml already allows to specify the repository location. After that, docs.rs can perform a best-effort attempt that covers majority of use cases to detect the location of the path inside the repository:

  • Assume the default branch (this is the tree rendered by GitHub when linked from the docs.rs navbar anyway)
  • Search all instances of Cargo.toml
  • Find the Cargo.toml package with identical name as the published one
  • Construct {repository}/edit/{default_branch}/{package_path}/src/\{file\} as the path to pass into rustdoc

It would be great enhancement to be able to specify branch and package_path, but in general this is just a best-effort approach that usually works. There is nothing we can do about renamed files, and it would be costly to try to poll the git diff for actively updating line number offsets, however I suppose it won't hurt to add a link that mostly works and usually lands near the right line.

@lebensterben
Copy link

I agree that UI design is important especially for mobile devices.

I think it'd be better not to put an "edit" button beside the "src" button.
Instead, in the "src" page, there's already a ">" button on the left edge to open a file "drawer". We can put the "edit" button below or above that.

First, I think editing the documentation itself is more developer-oriented and is not something for every library users. So it can afford to be placed deeper in UI.

More importantly, I believe the editors must have at least looked at the source for the documentation, and probably at the source code itself, then they can make a rightful judgement on whether some documentation indeed needs modifications.

@Lokathor
Copy link
Contributor

Yeah, reserving the edit button for when you're already looking at the source itself is much better.

@SOF3
Copy link
Author

SOF3 commented Sep 17, 2020

What if the user is just trying to fix a typo or paraphrase the documentation to make it simpler?

@Lokathor
Copy link
Contributor

Rustdoc already generates very busy pages that are full of too much cruft.

The common case is reading, so we should have the default view be oriented accordingly.

@lebensterben
Copy link

What if the user is just trying to fix a typo or paraphrase the documentation to make it simpler?

@SOF3 if someone is willing to spend time on making a PR to fix a typo, I believe he/she won't mind making an additional click.

@SOF3
Copy link
Author

SOF3 commented Sep 17, 2020

Rustdoc already generates very busy pages that are full of too much cruft.

The common case is reading, so we should have the default view be oriented accordingly.

I was thinking like a ✏️ icon that appears when the user hovers over the [src] button

@lebensterben
Copy link

...

@SOF3 how do you hover on touch screen?

@ehuss ehuss added the T-rustdoc Relevant to rustdoc team, which will review and decide on the RFC. label Sep 17, 2020
@SOF3
Copy link
Author

SOF3 commented Sep 17, 2020

...

@SOF3 how do you hover on touch screen?

Press down your finger on the element, then drag it a bit to any direction so that it doesn't register as a click.

@lebensterben
Copy link

...

@SOF3 how do you hover on touch screen?

Press down your finger on the element, then drag it a bit to any direction so that it doesn't register as a click.

that won't work on ios

@jyn514
Copy link
Member

jyn514 commented Sep 18, 2020

Related to rust-lang/rust#74758 - could there be two different views of the page? One that's intended for viewing and one for editing? Then the editing one would show much less detail, but would also have the ✏️ button.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea, but this RFC proposes a goal, not a way how to get there. Consider posting this as a pre-rfc on https://internals.rust-lang.org/ to get more ideas on the design - as-is, this isn't really actionable for the rustdoc team.

```
--edit-link-format "https://github.com/rust-lang/rust/edit/master/libstd/{file}#L{start_line}-L{end_line}"
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a reference-level explanation. Consider addressing what the UI would look like for this.

[drawbacks]: #drawbacks

This increases complexity of the documentation page, especially with mobile devices.
Good UX design is required to prevent bloating the content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be part of the RFC, not deferred until later. Certainly no one on the rustdoc team has the bandwidth to design it.


Pros:

- *May* support previous crate builds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to expand on why this is 'May'. The difficulty I forsee is changes in HTML page structure, but since the links always say '[src]' that could be mitigated somewhat. It highly depends where the buttons end up going, which is one more reason I think the design should be part of the RFC.

# Alternatives
[alternatives]: #alternatives

It is also possible to perform this with JavaScript on behalf of docs.rs only.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't have to be with JavaScript, docs.rs already rewrites pages on the backend: https://github.com/rust-lang/docs.rs/blob/master/src/utils/html.rs

@jyn514
Copy link
Member

jyn514 commented Sep 20, 2020

Construct {repository}/edit/{default_branch}/{package_path}/src/{file} as the path to pass into rustdoc

I don't like this approach, it breaks for gitlab. What docs.rs could do instead is have a list of supported git hosts (that's fairly easy to add to) and choose the file path based on that and the repository field. +1 to not having more docs.rs specific data though.


This increases complexity of the documentation page, especially with mobile devices.
Good UX design is required to prevent bloating the content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RFC will only show the links on new docs.rs pages, not existing builds. Can you add that as a drawback?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-rustdoc Relevant to rustdoc team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants