Skip to content

Put negative implementors first and apply same ordering logic to foreign implementors #142380

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Put negative implementors first and apply same ordering logic to foreign implementors #142380

wants to merge 2 commits into from
+167 −30

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jun 11, 2025 •
edited by rustbot
Loading

Fixes #51129.

This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls.

I also used this occasion to make the foreign implementors sort the same as the local ones by using compare_names.

You can test it here.

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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jun 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 11, 2025

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jun 11, 2025
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Took me a while to figure out where the typescript definition was stored to fix the error. ^^'

lolbinarycat
lolbinarycat requested changes Jun 16, 2025
View reviewed changes
src/librustdoc/html/render/write_shared.rs
let part = OrderedJson::array_sorted(
implementors.sort_unstable();

let part = OrderedJson::array_unsorted(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely an improvement, as it eliminates an allocation, but we're still allocating an intermediate string for each element.

OrderedJson could be much more efficient if it had a method that appended using serde_json::to_writer.

I can open an issue for improving the OrderedJson interface, if you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

An issue would be welcome indeed.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat lolbinarycat mentioned this pull request Jun 17, 2025
Open
rust-bors bot added a commit that referenced this pull request Jun 17, 2025
@rust-bors
Auto merge of #142634 - lolbinarycat:rustdoc-OrderedJson-writer, r=<try>
f5363cb 
alternate interface for OrderedJson to reduce allocations

inspired by #142380 (comment)

r? `@GuillaumeGomez`
<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->
@bors
Copy link
Collaborator

bors commented Jun 22, 2025

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

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts.

@notriddle
Copy link
Contributor

Either JavaScript-loaded content needs to go on the bottom, or it needs to be loaded in a blocking manner. Otherwise, you get layout shift and potential misclicks.

output.mp4

notriddle
notriddle requested changes Jun 23, 2025
View reviewed changes
Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Need a solution to the layout shift problem.

@rustbot rustbot 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 Jun 23, 2025
@GuillaumeGomez
Copy link
Member Author

Fair enough. I can make the implementors content to be hidden by default (when JS is enabled) and display it when the updated is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lolbinarycat lolbinarycat lolbinarycat requested changes

@notriddle notriddle notriddle requested changes

Assignees

@notriddle notriddle

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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[rustdoc] Implementors section of Sync (and other similar traits) should separate implementors and !implementors
6 participants
@GuillaumeGomez @rustbot @rust-log-analyzer @bors @notriddle @lolbinarycat