Eliminate horizontal scrolling in rust code blocks

[1dimir]

The style guide mentiones presentation use case for the course. That means much inconvenience while dealing with very long lines in the code snippets.

The problem was addressed earlier as it can be seen from rustfmt.toml configuration:

# The code blocks get a scrollbar if they are wider than this.
max_width = 85

This works well for read only blocks, but fails for editable ones. Due to line numbering, there is less space left for the code. My observations are that the scrolling threshold is 83 for less than 10 line long code, 82 - less than 100 and 81 - above 100. There are no snippets above 1k and I suppose there must not be in the context of the course.

The following merge request adopts max 81-character width and eliminates detected scrolling in rust code blocks.

I suppose it is a partial solution though. But further steps require a discussion.

There should be an automated format validation step. rustfmt can do so with rust files, but not markdown. Across the course there are examples where rust code is separated from markdown files and included like:

{{#include testing/src/lib.rs:leftpad}}

But. Firstly, that means all the rust code should be excluded from md files and guides updated. Secondly, rust code contains comments that are translated. And translations can turn out to be longer than the source. And the result should also be tested. Thirdly, rustfmt seems to ignore long use expressions and long strings in macro arguments - leaves them as they are without errors.

Also the course has some code blocks with long shell commands and text output. Were left unchanged.

Summary questions:

• Should we exclude all rust code from markdown files?
• How can we validate rust code with translated comments?
• How to deal with use and macros arguments that can still produce horizontal scrolling?
• What to do with other code blocks with very long lines?

[mgeisler]

Hi @1dimir, thanks for bringing this up!

Due to line numbering, there is less space left for the code. My observations are that the scrolling threshold is 83 for less than 10 line long code, 82 - less than 100 and 81 - above 100. There are no snippets above 1k and I suppose there must not be in the context of the course.

Oh, good point! I've probably forgotten to update the width after we turned on the line numbers.

You're completely right that we should not have code blocks with 1k lines in this course. The longest code blocks I know of are in 56.1. RTC Driver and related exercises. I think they're mostly there because @qwandor developed a tool which will generate zip files with the code blocks. I would prefer the code blocks to be removed from the pages (#625).

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide. @djmitche has been working on splitting up large slides together with @mani-chand (#1464).

There should be an automated format validation step. rustfmt can do so with rust files, but not markdown.

Luckily, dprint fmt will format the Rust code in the Markdown files! This was done in #1157 and it was the main reason for implementing google/mdbook-i18n-helpers#19.

Please see the Formatting section for all the tools to install 😄 Let us know if this works/doesn't work for you!

[mani-chand]

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

We can use editor api to get number of lines , If it excides then we will display redbox as warning.

[mgeisler]

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

Ah, I had not thought of that. It could be interesting for local development.

However, the warning/error I'm talking about here has to be generated when people make the PR — it should not show up on the deployed course on https://google.github.io/comprehensive-rust/ since that page is used by people who want to learn Rust. Warnings would be very disruptive there 😄

[mani-chand]

Yes, I am speaking the same it's should be in local development while merging PR. We have made redbox for local development if you remembered.

When a PR is created regarding the change in slide.Reviewer can easily understand that it crosses number of lines as redbox popup when the slide is visited in local development.

[mani-chand]

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

Ah, I had not thought of that. It could be interesting for local development.

However, the warning/error I'm talking about here has to be generated when people make the PR — it should not show up on the deployed course on https://google.github.io/comprehensive-rust/ since that page is used by people who want to learn Rust. Warnings would be very disruptive there 😄

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

If you are okay then we can add it in #1942 PR.

[mgeisler]

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

[mani-chand]

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

Ok .now, I am not about writing a script in github actions. I will check if i can do it and let you know.

[mani-chand]

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

I was talking about javascript code.

[1dimir]

@mgeisler, hi!

Due to line numbering, there is less space left for the code. My observations are that the scrolling threshold is 83 for less than 10 line long code, 82 - less than 100 and 81 - above 100. There are no snippets above 1k and I suppose there must not be in the context of the course.

Oh, good point! I've probably forgotten to update the width after we turned on the line numbers.

An update on thresholds. There are few places, where a code block is placed inside Speaker Notes. That area has additional margins, that further decrease available space for the editable code:

lines	editable	notes, editable
1-9	83	82
10-99	82	81
100+	81	80

86 characters seems to fit in readonly blocks regardless of their placement.

[1dimir]

Luckily, dprint fmt will format the Rust code in the Markdown files! This was done in #1157 and it was the main reason for implementing google/mdbook-i18n-helpers#19.

Please see the Formatting section for all the tools to install 😄 Let us know if this works/doesn't work for you!

Thanks! I missed that and used only rustfmt following CI steps. Although it doesn't work exactly as I expected.

Prerequisites: rustfmt.toml: max_width = 85, dprint.json: "lineWidth": 80.

dprint check indeed works on both .md and .rs files. For rust files it uses minimal width value among the configurations. For texts inside .md it applies lineWidth threshold. But it doesn't look like it affects anyhow contents of code blocks inside .md.

[1dimir]

I started thinking of browsing the book with playwright/selenium/etc to check for actual scrolling occurrences...

[mgeisler]

Thanks! I missed that and used only rustfmt following CI steps.

I think our setup instructions are a little confusing (#1426), so please let me know if I've messed up and written about rustfmt somewhere?

For rust files it uses minimal width value among the configurations. For texts inside .md it applies lineWidth threshold. But it doesn't look like it affects anyhow contents of code blocks inside .md.

Oh! I must admit that I haven't looked deeply into this myself, I was just happy to see it reformat the code blocks 🙂

I did some testing just now and I think you might have seen the effects of caching. If I run

dprint fmt --incremental=false

then I do see the Rust code change after an update to rustfmt.toml. The line widths seem independent: one for Markdown and one for Rust.

[mgeisler]

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

I was talking about javascript code.

Okay, thanks for confirming — I suggest you don't do anything here since we'll need a different tool than JavaScript here.

[mgeisler]

I started thinking of browsing the book with playwright/selenium/etc to check for actual scrolling occurrences...

That would be very interesting! I've been waiting for someone to show up with the necessary skills for something like that 😄 I would be interested in statistics that show how tall and wide each slide is.

If we have tooling for this, then I could imagine having a warning on a PR that makes the page taller than a given threshold — perhaps even with a screenshot like I did manually in #1464.

An update on thresholds.

Thank you for conducting this test! I tried reformatting the code in 80 columns and I think it is okay. A few of the changes can be counter-acted by simply trimming the code. As an example, iterator/intoiterator.md has an example with x_coords and y_coords, but I think calling them xs and ys is equally fine. That fits nicely in 80 columns.

[mgeisler]

@michael-kerscher, is this something we could detect with #2258?

[michael-kerscher]

@mgeisler Yes, we can detect it with the slide evaluator if it is extended a bit.

I had a look at the slides and the code examples use the ACE editor. This leverages a div element like this example - display: none is only set if the scrollbar is hidden.

<div class="ace_scrollbar ace_scrollbar-h" style="height: 20px; left: 42px; right: 0px; display: none;">
  <div class="ace_scrollbar-inner" style="height: 20px; width: 708px;"></div>
</div>

Selenium has a nice feature to evaluate the "Displayedness" (https://w3c.github.io/webdriver/#element-displayedness) that does not directly evaluate the display attribute but if it is shown at all. Thinking about the property we want to have, it is worthwhile implementing it via this method as slides are visible in full or have some other "policy violations" already by being to big.

I'm trying to extend the current mdbook-slide-evaluator to evaluate the scrollbar "displayedness".

[michael-kerscher]

I gave it a first shot (code will follow once it is a bit cleaned up for the commit) and these are the pages that have a visible "ace_scrollbar-h"

grep -Po "(?<=html/).*CodeExampleHScrollbar" violations.csv | sed -e "s|^|https://google.github.io/comprehensive-rust/|" | cut -d, -f 1
,3 | tr ',' ' '

Result:
https://google.github.io/comprehensive-rust/bare-metal/microcontrollers/type-state.html
https://google.github.io/comprehensive-rust/borrowing/solution.html
https://google.github.io/comprehensive-rust/concurrency/async-pitfalls/cancellation.html
https://google.github.io/comprehensive-rust/iterators/intoiterator.html
https://google.github.io/comprehensive-rust/lifetimes/exercise.html
https://google.github.io/comprehensive-rust/lifetimes/solution.html
https://google.github.io/comprehensive-rust/memory-management/exercise.html
https://google.github.io/comprehensive-rust/memory-management/solution.html
https://google.github.io/comprehensive-rust/testing/solution.html
https://google.github.io/comprehensive-rust/unsafe-rust/solution.html
https://google.github.io/comprehensive-rust/unsafe-rust/unsafe-functions.html

Most of them are not a big issue, only the exercises tend to be pretty wide. The others are mostly "a semicolon out of compliance"

[mgeisler]

Nice work! We could fix the few semicolons out of place (by adjusting rustfmt.toml) and then exclude the exercises and solutions from this tool.

That should basically allow us to run this in CI?