-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Provide doc links at item definitions on source pages #89157
Provide doc links at item definitions on source pages #89157
Conversation
Some changes occurred in HTML/CSS/JS. |
r? @ollie27 (rust-highfive has picked a reviewer for you, use r? to override) |
I'd like to propose an alternative solution that is more complete and also
helps with other use cases. Instead of a popup, how about we put a link in
the margin next to all source view items, which links to their
corresponding documentation. That would be useful for general navigation of
a code base, and would provide a complement to the current [src] link that
takes you to source view.
If you click on a link to a definition, and want the doc view, it would be
two clicks, same as this popup solution. If you only wanted the source
view, it would be just one click.
This would also maintain the property that clicking links in rustdoc
usually navigates you somewhere rather than creating a popup.
|
I really like @jsha's suggestion here! Having a context menu each time does seem awkward |
I don't understand what you're suggesting @jsha... Simpler: what would it look like?
The popup appears on hover too, so one click. But I remain very curious about what you're suggesting! |
I believe jsha is suggesting that the source view would look something like this (where
It's an interesting idea, although I'm not sure how we'd fit it into the margin. Could you elaborate @jsha?
IIUC, you have to click once to get the popup, and then click again to actually go to the destination, so it's two clicks. |
Noah Lev accurately summarized my thinking. As for how to fit it, a couple
of ideas:
We could use an icon in the left margin, probably a "..." similar to what
GitHub shows when you click on a line number. This would be handy if we
later wanted to put additional things there, like "copy permalink".
Or, we could put [docs] in the right margin, by analogy with [src] in the
doc view. We may also want to consider using an icon for both src and doc
links.
|
What happens in case you multiple items on one line like: fn foo(a: Foo, b: Bar, c: String) { ? In this case we have three different items, so that might make it a bit weird/dense, no?
I don't think using the right margin in the source code viewer is a good idea because the width depends on the content currently. |
There's only one item, |
I agree; it seems like it could make the UI confusing. The |
Yep, exactly. There is a possibility case of multiple items on one line,
like:
```
struct Foo; struct Bar;
```
But that's unusual, and rustfmt would break it up anyhow. If we encounter
such a case we should link to docs for the first one.
|
Hmm, that does seem likely uncommon, but maybe we could change the UI to avoid it altogether? E.g., we could have clicking on the |
I think I'd remove the code link highlighting. Could you stylize the popup like this for consistency with the existing
|
Yeah, I was just using |
I definitely don't agree with your proposal @jsha, I find it very counter-intuitive. Currently in one click we have access to both documentation and source. What you suggest would be a "flow break" considering you'd have to switch between documentation and source page all the time, which would really be not great... EDIT: And to comment on @camelid's explanations:
Then it's two clicks, and really not intuitive (why would I need to go to an item definition to get its documentation link?).
I like this suggestion! It'll make the popup smaller too.
It's part of the things that aren't set in stone for the moment. You can see some reflexion about it here. @camelid @jsha: I'd really like if possible to keep the possibility to have access to both |
Stepping back a bit I think it's important to settle on the purpose of jump-to-def, because what I see here are conflicting purposes. Adding a context menu for every JTD click is also two actions (hover, then click) for an operation many here would want to be immediate. We need to figure out what problems we are solving, how we expect people to use this feature, and then use that to figure out which operations should and shouldn't be convenient. In general we've had a lack of clarity on this part of this feature for a while and I suspect this will keep cropping up unless we do that work first. |
This is a good point. I like the idea to have access to both the documentation and the source from every item, but it slows down the browsing so I understand how that can be an issue. The other solution however (to put the documentation link on the definition) doesn't convince me in its current form. Oh well, we're not in a hurry so we can debate over this and try to reach the best solution for everyone. |
I suppose that, while browsing the docs, you're most likely going to see more docs next, and when browsing source, you're most likely going to navigate to more source next. So then, maybe a single click in source should navigate to the source, and the popup can just have docs (or both?). And probably add a small delay to the hover if not there already. |
Please do not make the target of the link timing sensitive. That a) is super frustrating when you mean to go to one place and instead go to another; b) makes it impossible to use the longer version on mobile; and c) makes it much more difficult for people with slow reaction times to use the faster link. In fact I'm again any version of this that relies on hover for navigation because it will be completely broken on mobile. |
@camsteffen: I agree with @jyn514: that sounds like a very bad idea.
No, currently when you click or hover the link, it makes the popup appear (I thought about mobile devices as well ;) ). It's in the "What it does" explanation in the first comment. I did it this way because if JS is enabled, then we can make the popup appear, otherwise it'll simply click on the ink and go to the item's definition. |
I don't see how that applies. The popup shows above the link. So a direct click is always to source regardless of whether the popup shows. There should be a very small delay (like .3 secs) so that when you drag your mouse across the screen there is not a firework show of popups. On mobile, a click can just open the popup. |
while browsing the docs, you're most likely going to see more docs next,
and when browsing source, you're most likely going to navigate to more
source next
Yes, this is also my intuition and underlying assumption. Another way of
putting it:
1. From a doc page, clicking an identifier takes you to docs.
2. From a source page, clicking an identifier takes you to source.
3. From a doc page, you can easily navigate to the source page for the item
you are looking at.
4. From a source page, you can easily navigate to the doc page for the item
you are looking at.
We already have 1, 2, and 3. We're only missing 4 to have a nice,
symmetric, easy to learn system. Also, 4 is useful not only in the "go to
definition" use case. It's also useful if you are manually scrolling
through a source page and decide you want to see the rendered docs for
something.
Talking about the popup implementation in this PR: We should strive to
always use existing, familiar UI elements. The closest UI element here
would be a context menu. However, the current implementation doesn't act
like a real context menu: it displays on hover (like a tooltip), but it
contains links (like some menus). It appears above the mouse cursor rather
than below and to the right. And it appears on left click rather than right
click.
So one direction we could go would be to make this more like a real context
menu. That has the advantage of being similar to how code navigation works
in an IDE. But it also has some disadvantages. For instance, overriding
right click on a web page can be quite annoying for the user, and should be
avoided without strong need. And opening a context menu for an item at the
bottom or right of the page can wind up with the menu awkwardly outside the
viewport.
I could see us wanting a context menu in the long run, if we do turn this
into a full code navigation tool. We would want multiple options, like go
to type definition, go to references, etc. However since we're considering
that explicitly out of scope, a context menu is probably overkill.
@GuillameGomez, can you say more about why you think my 1/2/3/4 approach
would wind up with lots of clicking back and forth between source and docs?
Do you disagree with the assumption that people most often want to go
docs->docs and source->source?
|
No, I completely agree. I'm just not sure that it'll be obvious to people that to go to the item's docs, you need to go to its definition (even though I'm not a big fan, but I can live with it). My biggest issue is the UI suggested with |
Triage: |
e76daa4
to
dcc6221
Compare
This comment has been minimized.
This comment has been minimized.
dcc6221
to
1553866
Compare
This comment has been minimized.
This comment has been minimized.
ec67d7c
to
0939431
Compare
Updated (and fixed the GUI tests). |
☔ The latest upstream changes (presumably #93803) made this pull request unmergeable. Please resolve the merge conflicts. |
I'll fix the conflict once #96636 is merged. |
Cross referencers like Elixir (see GuillaumeGomez/rfcs#1 (comment)) support references (i.e. uses) of an identifier (similar to "Search All References" in IDEs). Since we are discussing a link back to the doc view and how that would fit in the UI, mobile, etc., I thought it would be interesting to mention that other links may be useful in the future too. For instance, if On the UI topic, I think having a contextual menu for desktop/laptop users for extra links on a given source view link (e.g. |
0939431
to
1fa953e
Compare
Finally took the time to fixed the merge conflicts. |
☔ The latest upstream changes (presumably #98447) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: FYI: when a PR is ready for review, send a message containing |
We first need to finish the RFC so closing. |
Part of #89095.
You can test it here.
We now generate links to an item documentation on its definition.
cc @rust-lang/rustdoc