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

Add source file sidebar #55707

Merged
merged 5 commits into from Dec 4, 2018

Conversation

Projects
None yet
5 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Nov 6, 2018

This is just a start currently but that gives a good overview of what it'll look like:

screenshot 2018-11-06 at 01 39 15

r? @QuietMisdreavus

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 6, 2018

Current look:

screenshot from 2018-11-06 19-11-49

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 6, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1189f255:start=1541528089004907055,finish=1541528147998072191,duration=58993165136
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:04:00] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:00] tidy error: /checkout/src/librustdoc/html/render.rs:1014: line longer than 100 chars
[00:04:01] some tidy checks failed
[00:04:01] 
[00:04:01] 
[00:04:01] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:01] 
[00:04:01] 
[00:04:01] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:01] Build completed unsuccessfully in 0:00:52
[00:04:01] Build completed unsuccessfully in 0:00:52
[00:04:01] make: *** [tidy] Error 1
[00:04:01] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:070aae40
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:048943d4:start=1541528400460105670,finish=1541528400465141165,duration=5035495
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:060f76c9
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1a6cc634
travis_time:start:1a6cc634
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0355e828
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:file-sidebar branch from 6dfde9f to 400180c Nov 8, 2018

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 8, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0c1ece6b:start=1541702770011392033,finish=1541702771049252978,duration=1037860945
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:04:09] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:10] tidy error: /checkout/src/librustdoc/html/render.rs:1014: line longer than 100 chars
[00:04:10] tidy error: /checkout/src/librustdoc/html/static/source-script.js:67: line longer than 100 chars
[00:04:11] some tidy checks failed
[00:04:11] 
[00:04:11] 
[00:04:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:11] 
[00:04:11] 
[00:04:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:11] Build completed unsuccessfully in 0:00:49
[00:04:11] Build completed unsuccessfully in 0:00:49
[00:04:11] Makefile:79: recipe for target 'tidy' failed
[00:04:11] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a1fc1d8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:00852000:start=1541703033169628585,finish=1541703033175841534,duration=6212949
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:20f281fc
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:035ef6e8
travis_time:start:035ef6e8
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1ce61238
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 11, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:01783ea6:start=1541901059980252510,finish=1541901117758125075,duration=57777872565
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:55] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:55] tidy error: /checkout/src/librustdoc/html/render.rs:1014: line longer than 100 chars
[00:03:56] some tidy checks failed
[00:03:56] 
[00:03:56] 
[00:03:56] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:56] 
[00:03:56] 
[00:03:56] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:56] Build completed unsuccessfully in 0:00:51
[00:03:56] Build completed unsuccessfully in 0:00:51
[00:03:56] make: *** [tidy] Error 1
[00:03:56] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0ca53948
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Nov 11 01:56:03 UTC 2018
---
travis_time:end:1c7c94c0:start=1541901364388409549,finish=1541901364400704474,duration=12294925
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:095f6db4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:14c19d91
travis_time:start:14c19d91
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:24fa9d30
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:file-sidebar branch 2 times, most recently from 0c54b8b to 3bba941 Nov 11, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 11, 2018

I think it's ready now. Current version screenshot:

screenshot 2018-11-11 at 17 34 11

@GuillaumeGomez GuillaumeGomez changed the title [WIP] Start source file sidebar Add source file sidebar Nov 11, 2018

@QuietMisdreavus
Copy link
Member

QuietMisdreavus left a comment

Thanks so much! This looks really great.

Some observations based on poking around with a local build:

  • That the sources sidebar is on the right is a little surprising - the sidebar is on the left on all the other pages we generate. Same for the folding nature of it - this is the first time it's appeared in our docs like that.

  • The sidebar is a little awkward on mobile - it doesn't have its own scrolling container, so the listing and the fold toggle are stuck on the top of the page - but the source pages themselves were awkward on mobile already, so that's not a blocking issue.

  • The fold toggle sometimes "hides" behind the scrollbar for me (Chrome, Windows 7) - it's visible, but not clickable until you refresh the page.

  • The fold/unfold animation of the sidebar feels slow. We don't use animations like that for any of our other folding containers, so this one could afford to be much faster, or not animated in the first place.

  • It seems weird to me that the source files list is loaded dynamically, but i think i'm okay with it, since it can allow all crates that were documented at the same time to have their source all together, regardless of the order they're documented in.

  • The listing of files seems to have an unexpected auto-fold behavior: it looks like if a listing has a subdirectory, it will start unfolded? The following screenshot was taken from core/lib.rs, and std is already open with this surprising layout:

    image

  • Can you write a test? There's a search-index.rs test that does substring searches to make sure that certain items are present or missing from the index.

  • I feel like we shouldn't touch source-index.js if #![doc(html_no_source)] is set. You can check cx.shared.include_sources to see whether we should handle source pages for the current crate.

Show resolved Hide resolved src/librustdoc/html/render.rs Outdated
Show resolved Hide resolved src/librustdoc/html/render.rs Outdated
Show resolved Hide resolved src/librustdoc/html/render.rs Outdated
Show resolved Hide resolved src/librustdoc/html/render.rs Outdated
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 16, 2018

Updated!

That the sources sidebar is on the right is a little surprising - the sidebar is on the left on all the other pages we generate. Same for the folding nature of it - this is the first time it's appeared in our docs like that.

Seemed better to me on the right but I've put it back to the left.

The sidebar is a little awkward on mobile - it doesn't have its own scrolling container, so the listing and the fold toggle are stuck on the top of the page - but the source pages themselves were awkward on mobile already, so that's not a blocking issue.

I didn't do much for mobile on sources yet. I intend to once this PR is merged.

The fold toggle sometimes "hides" behind the scrollbar for me (Chrome, Windows 7) - it's visible, but not clickable until you refresh the page.

Since it's now on the left, no problem anymore.

The fold/unfold animation of the sidebar feels slow. We don't use animations like that for any of our other folding containers, so this one could afford to be much faster, or not animated in the first place.

It's now two times faster. I really like the animation though. :D

It seems weird to me that the source files list is loaded dynamically, but i think i'm okay with it, since it can allow all crates that were documented at the same time to have their source all together, regardless of the order they're documented in.

Either we load it dynamically or we have to generate a lot of HTML in all source files which are already quite big. And also, it allows to just add other crates. :)

The listing of files seems to have an unexpected auto-fold behavior: it looks like if a listing has a subdirectory, it will start unfolded? The following screenshot was taken from core/lib.rs, and std is already open with this surprising layout:

It was a bug. Should be fixed now.

Can you write a test? There's a search-index.rs test that does substring searches to make sure that certain items are present or missing from the index.

I'm not sure such a test would be useful. If a file is missing, it means we're not supposed to have access to it.

I feel like we shouldn't touch source-index.js if #![doc(html_no_source)] is set. You can check cx.shared.include_sources to see whether we should handle source pages for the current crate.

You're absolutely right. I updated the code so it's not generated in such cases.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Nov 16, 2018

I'm not sure such a test would be useful. If a file is missing, it means we're not supposed to have access to it.

I'm a fan of having tests for basic operations like this. We can just have something to make sure that this file is generated in a way that we expect. We're creating new functionality here, and i want to make sure we don't accidentally break something later and not catch it because there wasn't a test. (Something like this has happened in rustdoc already - impl Trait in return position didn't have a test and then broke at some point, and nobody realized it.)

It's now two times faster. I really like the animation though. :D

Thanks, this looks much nicer!

Also, now that the sidebar is on the left, can it start closed? It covers up some source when it's open (which is probably why you had it on the right to begin with), so starting it closed will make it unobtrusive. (This probably means getting rid of the local-storage item and just always being closed on page load.)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:file-sidebar branch from 7d4304e to 1b432a6 Nov 24, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 24, 2018

Added test and the source file sidebar is now collapsed by default.

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Dec 3, 2018

Pinging from triage @QuietMisdreavus you need to review this

@QuietMisdreavus
Copy link
Member

QuietMisdreavus left a comment

There's still a problem with how the "extra scripts" get loaded in with a resource suffix.

Show resolved Hide resolved src/librustdoc/html/layout.rs Outdated
Show resolved Hide resolved src/librustdoc/html/render.rs Outdated

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:file-sidebar branch from 44a89ac to 82a7b6f Dec 3, 2018

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Dec 3, 2018

With all these changes, this looks good! r=me when travis is green.

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Dec 4, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2018

📌 Commit 82a7b6f has been approved by QuietMisdreavus

/// Files related to the sidebar in rustdoc sources.
pub mod sidebar {
/// File script to handle sidebar.
pub static SOURCE_SCRIPT: &'static str = include_str!("static/source-script.js");

This comment has been minimized.

@QuietMisdreavus

QuietMisdreavus Dec 4, 2018

Member

Note to self: When this merges, make sure docs.rs also starts caching source-script.js.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2018

⌛️ Testing commit 82a7b6f with merge 596e10f...

bors added a commit that referenced this pull request Dec 4, 2018

Auto merge of #55707 - GuillaumeGomez:file-sidebar, r=QuietMisdreavus
Add source file sidebar

This is just a start currently but that gives a good overview of what it'll look like:

<img width="1440" alt="screenshot 2018-11-06 at 01 39 15" src="https://user-images.githubusercontent.com/3050060/48035592-05336180-e165-11e8-82e1-5ead0c345eb9.png">

r? @QuietMisdreavus
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 596e10f to master...

@bors bors merged commit 82a7b6f into rust-lang:master Dec 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:file-sidebar branch Dec 4, 2018

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