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.

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

Fix trait bounds display #127975

Merged
merged 3 commits into from
Jul 21, 2024
Merged

Fix trait bounds display #127975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
063ed0f
Improve display of trait bounds when there are more than two
GuillaumeGomez Jul 19, 2024
590c01a
Reduce width to ensure that the name is wider and thus still triggeri…
GuillaumeGomez Jul 19, 2024
eec3c3d
Add GUI test for trait bounds display
GuillaumeGomez Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions src/librustdoc/html/render/print_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2062,16 +2062,23 @@ pub(super) fn item_path(ty: ItemType, name: &str) -> String {

fn bounds(t_bounds: &[clean::GenericBound], trait_alias: bool, cx: &Context<'_>) -> String {
let mut bounds = String::new();
if !t_bounds.is_empty() {
if !trait_alias {
if t_bounds.is_empty() {
return bounds;
}
let has_lots_of_bounds = t_bounds.len() > 2;
let inter_str = if has_lots_of_bounds { "\n + " } else { " + " };
if !trait_alias {
if has_lots_of_bounds {
bounds.push_str(":\n ");
} else {
bounds.push_str(": ");
}
for (i, p) in t_bounds.iter().enumerate() {
if i > 0 {
bounds.push_str(" + ");
}
bounds.push_str(&p.print(cx).to_string());
}
for (i, p) in t_bounds.iter().enumerate() {
if i > 0 {
bounds.push_str(inter_str);
}
bounds.push_str(&p.print(cx).to_string());
}
bounds
}
Expand Down
6 changes: 6 additions & 0 deletions tests/rustdoc-gui/src/test_docs/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,3 +614,9 @@ pub mod private {
B,
}
}

pub mod trait_bounds {
pub trait OneBound: Sized {}
pub trait TwoBounds: Sized + Copy {}
pub trait ThreeBounds: Sized + Copy + Eq {}
}
35 changes: 35 additions & 0 deletions tests/rustdoc-gui/trait-with-bounds.goml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a browser, can't this be more easily checked with an HTML snapshot test (browser tests are slower to run and a lot more flaky).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I originally did. We can check that the HTML contains backline, but we can't be sure that the backline are actually processed as such unless we have access to the position unfortunately. Hence why I decided to go for a GUI test in the end.

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Check that if a trait has more than 2 bounds, they are displayed on different lines.

// It tries to load a JS for each trait but there are none since they're not implemented.
fail-on-request-error: false
go-to: "file://" + |DOC_PATH| + "/test_docs/trait_bounds/trait.OneBound.html"
// They should have the same Y position.
compare-elements-position: (
".item-decl code",
".item-decl a.trait[title='trait core::marker::Sized']",
["y"],
)
go-to: "file://" + |DOC_PATH| + "/test_docs/trait_bounds/trait.TwoBounds.html"
// They should have the same Y position.
compare-elements-position: (
".item-decl code",
".item-decl a.trait[title='trait core::marker::Copy']",
["y"],
)
go-to: "file://" + |DOC_PATH| + "/test_docs/trait_bounds/trait.ThreeBounds.html"
// All on their own line.
compare-elements-position-false: (
".item-decl code",
".item-decl a.trait[title='trait core::marker::Sized']",
["y"],
)
compare-elements-position-false: (
".item-decl a.trait[title='trait core::marker::Sized']",
".item-decl a.trait[title='trait core::marker::Copy']",
["y"],
)
compare-elements-position-false: (
".item-decl a.trait[title='trait core::marker::Copy']",
".item-decl a.trait[title='trait core::cmp::Eq']",
["y"],
)
22 changes: 13 additions & 9 deletions tests/rustdoc-gui/type-declation-overflow.goml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,34 +8,38 @@ fail-on-request-error: false

go-to: "file://" + |DOC_PATH| + "/lib2/long_trait/trait.ALongNameBecauseItHelpsTestingTheCurrentProblem.html"
// We set a fixed size so there is no chance of "random" resize.
set-window-size: (1100, 800)
set-window-size: (710, 800)
// Logically, the <body> scroll width should be the width of the window.
assert-property: ("body", {"scrollWidth": "1100"})
// However, since there is overflow in the type declaration, its scroll width is bigger.
assert-property: ("pre.item-decl", {"scrollWidth": "1324"})
assert-property: ("body", {"scrollWidth": "710"})
// We now check that the section width hasn't grown because of it.
assert-property: ("#main-content", {"scrollWidth": "450"})
// However, since there is overflow in the type declaration, its scroll width is bigger that "#main-content".
assert-property: ("pre.item-decl", {"scrollWidth": "585"})

// In the table-ish view on the module index, the name should not be wrapped more than necessary.
go-to: "file://" + |DOC_PATH| + "/lib2/too_long/index.html"

// We'll ensure that items with short documentation have the same width.
store-property: ("//*[@class='item-table']//*[@class='struct']/..", {"offsetWidth": offset_width})
assert: |offset_width| == "277"
assert: |offset_width| == "149"
assert-property: ("//*[@class='item-table']//*[@class='constant']/..", {"offsetWidth": |offset_width|})

// We now make the same check on type declaration...
go-to: "file://" + |DOC_PATH| + "/lib2/too_long/type.ReallyLongTypeNameLongLongLong.html"
assert-property: ("body", {"scrollWidth": "1100"})
assert-property: ("body", {"scrollWidth": "710"})
// Getting the width of the "<main>" element.
assert-property: ("main", {"scrollWidth": "510"})
// We now check that the section width hasn't grown because of it.
assert-property: ("#main-content", {"scrollWidth": "840"})
assert-property: ("#main-content", {"scrollWidth": "450"})
// And now checking that it has scrollable content.
assert-property: ("pre.item-decl", {"scrollWidth": "1103"})

// ... and constant.
// On a sidenote, it also checks that the (very) long title isn't changing the docblock width.
go-to: "file://" + |DOC_PATH| + "/lib2/too_long/constant.ReallyLongTypeNameLongLongLongConstBecauseWhyNotAConstRightGigaGigaSupraLong.html"
assert-property: ("body", {"scrollWidth": "1100"})
assert-property: ("body", {"scrollWidth": "710"})
// We now check that the section width hasn't grown because of it.
assert-property: ("#main-content", {"scrollWidth": "840"})
assert-property: ("#main-content", {"scrollWidth": "450"})
// And now checking that it has scrollable content.
assert-property: ("pre.item-decl", {"scrollWidth": "950"})

Expand Down
Loading