Skip to content

Conversation

notriddle
Copy link
Contributor

Before

In Names (7) / In Parameters (0) / In Return types (0)

After

In Function Signature (7)

@rust-highfive
Copy link
Contributor

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 30, 2022
@rust-highfive
Copy link
Contributor

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 Apr 30, 2022
@GuillaumeGomez
Copy link
Member

Considering it's a UI/UX change, I'd like to have @jsha and @Manishearth's opinion before going further. Personally I think it'd be a nice improvement.

@Manishearth
Copy link
Member

I think it's a good call!

@jsha
Copy link
Contributor

jsha commented May 2, 2022

Agreed! This is something that's bugged me for a while. Thanks for fixing it.

@GuillaumeGomez
Copy link
Member

Ok so just remains to add a differentiation between returned value and "in function signature" and then let's go!

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented May 2, 2022

There is a JS error when the results are displayed:

Screenshot from 2022-05-02 20-29-56

The left/right arrows should be disabled in case there is only one column displayed, otherwise it looks like this (no "selected" header):

Screenshot from 2022-05-02 20-31-30

Please add a check in your GUI test for the left/right arrows in such case to ensure that it doesn't change the selected header.

@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/tab-bar-fn-search branch from 1a4840f to 88aabf8 Compare May 2, 2022 23:18
@notriddle
Copy link
Contributor Author

Okay, I've got the keyboard shortcut thing fixed.

@GuillaumeGomez
Copy link
Member

I can confirm it's working locally as expected. 👍

@GuillaumeGomez
Copy link
Member

And I still have a JS error:

Screenshot from 2022-05-03 17-29-49

@notriddle
Copy link
Contributor Author

I see. The error came up when there was a parse error, because the code doesn't work well when there's no tabs at all.

To avoid adding a bunch of special-cases just for the parse error screen, I've added an "In Names" tab (conceptually, this is where doc aliases go), fixing the console log error.

@GuillaumeGomez
Copy link
Member

Just one last weird thing:

Screenshot from 2022-05-03 21-41-03

It's a bit of strange rendering with doc aliases. Not a blocker though. You can try it with:

#[doc(alias = "->a")]
pub struct Foo;

Just confirming it's not an issue then let's go!

What do you think of this @Manishearth @jsha ? I personally think it's fine but asking just in case.

@notriddle
Copy link
Contributor Author

notriddle commented May 5, 2022

I get why this seems strange, but

  • It seems unavoidable, given how doc aliases work. They're allowed to have arbitrary text in them, which means they're allowed to overlap with search syntax. Disallowing this would be backwards-incompatible, and redesigning this would mean redesigning the search feature pretty heavily just for a niche feature like allowing ->a as a doc alias.
  • It seems like you're holding it wrong. This is a problem because the doc alias has valid syntax for a function return value, but it doesn't actually describe a function or the type it returns. We can't stop people from writing misleading docs without taking functionality away that would be useful in the right hands.

@jsha
Copy link
Contributor

jsha commented May 5, 2022

I think @notriddle's comment makes sense and I think the image @GuillaumeGomez showed is fine.

@bors
Copy link
Collaborator

bors commented May 5, 2022

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

@GuillaumeGomez
Copy link
Member

I said I was fine with it but thanks for the extra explanations. :) r=me once you fixed the conflict.

notriddle and others added 5 commits May 5, 2022 09:37
@notriddle notriddle force-pushed the notriddle/tab-bar-fn-search branch from 45cf0be to 4c183cd Compare May 5, 2022 16:40
@notriddle
Copy link
Contributor Author

@bors r=GuillaumeGomez,jsha

@bors
Copy link
Collaborator

bors commented May 5, 2022

📌 Commit 4c183cd has been approved by GuillaumeGomez,jsha

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2022
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 5, 2022
…ch, r=GuillaumeGomez,jsha

rustdoc: when running a function-signature search, tweak the tab bar

# Before

![In Names (7) / In Parameters (0) / In Return types (0)](https://user-images.githubusercontent.com/1593513/166122875-ffdeafe6-8d4d-4e61-84a6-f5986b50ac35.png)

# After

![In Function Signature (7)](https://user-images.githubusercontent.com/1593513/166122883-9a3d7515-3235-4ee3-8c4b-5401d109e099.png)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 6, 2022
…ch, r=GuillaumeGomez,jsha

rustdoc: when running a function-signature search, tweak the tab bar

# Before

![In Names (7) / In Parameters (0) / In Return types (0)](https://user-images.githubusercontent.com/1593513/166122875-ffdeafe6-8d4d-4e61-84a6-f5986b50ac35.png)

# After

![In Function Signature (7)](https://user-images.githubusercontent.com/1593513/166122883-9a3d7515-3235-4ee3-8c4b-5401d109e099.png)
@notriddle
Copy link
Contributor Author

r? @GuillaumeGomez

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
…ch, r=GuillaumeGomez,jsha

rustdoc: when running a function-signature search, tweak the tab bar

# Before

![In Names (7) / In Parameters (0) / In Return types (0)](https://user-images.githubusercontent.com/1593513/166122875-ffdeafe6-8d4d-4e61-84a6-f5986b50ac35.png)

# After

![In Function Signature (7)](https://user-images.githubusercontent.com/1593513/166122883-9a3d7515-3235-4ee3-8c4b-5401d109e099.png)
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
…ch, r=GuillaumeGomez,jsha

rustdoc: when running a function-signature search, tweak the tab bar

# Before

![In Names (7) / In Parameters (0) / In Return types (0)](https://user-images.githubusercontent.com/1593513/166122875-ffdeafe6-8d4d-4e61-84a6-f5986b50ac35.png)

# After

![In Function Signature (7)](https://user-images.githubusercontent.com/1593513/166122883-9a3d7515-3235-4ee3-8c4b-5401d109e099.png)
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2022
…laumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#96557 (Allow inline consts to reference generic params)
 - rust-lang#96590 (rustdoc: when running a function-signature search, tweak the tab bar)
 - rust-lang#96650 (Collect function instance used in `global_asm!` sym operand)
 - rust-lang#96733 (turn `append_place_to_string` from recursion into iteration)
 - rust-lang#96748 (Fixes reexports in search)
 - rust-lang#96752 (Put the incompatible_closure_captures lint messages in alphabetical order)
 - rust-lang#96754 (rustdoc: ensure HTML/JS side implementors don't have dups)
 - rust-lang#96772 (Suggest fully qualified path with appropriate params)
 - rust-lang#96776 (Fix two minor issues in hir.rs)
 - rust-lang#96782 (a small `mirror_expr` cleanup)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fcb0bce into rust-lang:master May 6, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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