rustdoc options to set default theme (and other settings) #77213

ijackson · 2020-09-26T00:38:28Z

Hi. This is the MR I promised in #77024

It is a little more general than I envisaged there. Once I had found the settings-handling machinery it seemed foolish to add this feature just for the theme.

Closes #77024

rust-highfive · 2020-09-26T00:38:31Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

rust-highfive · 2020-09-26T00:38:32Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jyn514 · 2020-10-13T01:24:19Z

ping @GuillaumeGomez

src/librustdoc/html/layout.rs

GuillaumeGomez · 2020-10-13T08:44:58Z

You added the getSettingValue and replaced a few setting getters, but could you replace all of them please?

ijackson · 2020-10-13T18:26:30Z

Hi. Thanks for the review. I have made those changes. I rebased onto current master, and then made the requested changes as new commits, for ease of your review (and as requested by the highfive bot). I did them as git autosquash, and when you're happy with the series, will squash it down.

bors · 2020-10-16T12:28:32Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ijackson · 2020-10-16T14:56:21Z

You added the getSettingValue and replaced a few setting getters, but could you replace all of them please?

As requested, I did this. Unfortunately it didn't get a review before some other changes went in which conflicted. By its nature, the change to use getSettingValue everywhere is very textually intrusive and likely to break. If I rebase and fix the merge conflicts, will I get a prompt review?

(FTR obviously I approve of #77809 and it should work well in combination with my work in this MR.)

jyn514 · 2020-10-23T14:22:47Z

@ijackson is this PR still needed after #77809 ? It seems like #77024 should be fixed by setting your system theme to the default that you want.

ijackson · 2020-10-23T15:12:19Z

Joshua Nelson writes ("Re: [rust-lang/rust] rustdoc options to set default theme (and other settings) (#77213)"):

@ijackson is this PR still needed after #77809 ? It seems like #77024 should be fixed by setting your system theme to the default that you want.

No, I'm think this is still wanted. Honouring prefers-color-scheme is good but: * It is not available except on fairly recent browsers (caniuse.com says eary-to-mid 2019). * It is suppressed by "privacy.resistFingerprinting" [1] * It does not address the use case of a shared site-wide default, for the person running rustdoc to generate docs for their community. * My series allows setting other preferences too which might well be useful. So I intend to rebase this, assuming that it is likely to be accepted. Thanks, Ian. [1] https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

GuillaumeGomez · 2020-10-24T17:36:04Z

Please rebase to fix merge conflicts then ping me and I'll review again (but it seems mostly good already).

Currently, storage.js and main.js have many open-coded calls to getCurrentValue for "rustdoc-" values, but these are settings and should be handled by getSettingValue. So make getSettingValue part of storage.js (where everyone can call it) and use it everywhere. No functional change yet. We are going to make getSettingValue do something more sophisticated in a moment. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

rustdoc has various user-configurable preferences. These are recorded in web Local Storage (where available). But we want to provide a way to configure the default default, including for when web storage is not available. getSettingValue is the function responsible for looking up these settings. Here we make it fall back some in-DOM data, which ultimately comes from RenderOptions.default_settings. Using HTML data atrtributes is fairly convenient here, dsspite the need to transform between snake and kebab case to avoid the DOM converting kebab case to camel case (!) We cache the element and dataset lookup in a global variable, to ensure that getSettingValue remains fast. The DOM representation has to be in an element which precedes the inclusion of storage.js. That means it has to be in the <head> and we should not use an empty <div> as the container (although most browsers will accept that). An empty <script> element provides a convenient and harmless container object. <meta> would be another possibility but runs a greater risk of having unwanted behaviours on weird browsers. We trust the RenderOptions not to contain unhelpful setting names, which don't fit nicely into an HTML attribute. It's awkward to quote dataset keys. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

We just plumb through what the user tells us. This is flagged as unstable, mostly because I don't understand the compatibility rules that rustdoc obeys for local storage data, and how error handling of invalid data works. We collect() the needed HashMap from Vec of Vecs of (key, value) pairs, so that there is a nice place to add new more-specific options. It would have been possible to use Extend::extend but doing it this way ensures that all the used inputs are (and will stay) right next to each other. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

This is a fairly simple special case of --default-eetting. We must set both "theme" and "use-system-theme". Providing it separately enables us to document a way to set the theme without expoosing the individual settings keywords, which are quite complex. Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2020-10-28T18:42:45Z

@GuillaumeGomez

Please rebase to fix merge conflicts then ping me and I'll review again (but it seems mostly good already).

Now done, thanks. I made some slight improvements to the Rust parts so you may want to re-review them.

src/librustdoc/html/layout.rs

src/librustdoc/lib.rs

GuillaumeGomez · 2020-10-28T18:58:22Z

Just nits on my end. Maybe you want to take a last look @jyn514 too?

ijackson · 2020-10-28T19:30:29Z

Hi, thanks for the prompt review.

+ .map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v),)) Small nit: I think that the extra comma at the end is unnecessary. ;)

I think that used to be before a newline but rustfmt deleted the newline. I'll get rid of the comma...

+ "Set the default theme. THEME should be the theme name, generally lowercase. \ The usage of the double whitespace is bugging me a lot. Is there a reason behind it?

That's what I was taught in school on a manual typewriter :-). I am happy to change it. Would you like a new commit on top to address these nits, or amendment and force push ? Ian.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

GuillaumeGomez · 2020-10-28T19:58:20Z

In this case it's fine, you're still under the "rebase bar". :p Just push another commit to fix the nits.

@GuillaumeGomez

* Remove a needless comma in the Rust code * Replace double spaces after full stops with single spaces Requested-by: @GuillaumeGomez Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2020-10-28T20:15:51Z

@GuillaumeGomez Done, thanks.

GuillaumeGomez · 2020-10-28T20:17:31Z

All good for me. Just a last check from @jyn514 and then it's good to go!

jyn514 · 2020-10-28T20:54:14Z

src/librustdoc/lib.rs

+        unstable("default-theme", |o| {
+            o.optopt(
+                "",
+                "default-theme",
+                "Set the default theme. THEME should be the theme name, generally lowercase. \
+                 If an unknown default theme is specified, the builtin default is used. \
+                 The set of themes, and the rustdoc built-in default is not stable.",
+                "THEME",
+            )
+        }),


Why is default-theme special cased? If you want default-settings to work for arbitrary settings, I think it makes sense to use it for themes too. If you want this to be documented, I would rather document settings in the unstable section of the rustdoc book (src/doc/rustdoc/src/unstable-features.md).

(I would also be ok with getting rid of default-settings and only keeping default-themes.)

The storage names are rather internal, and --default-setting sets them directly. That's good for flexibility but it is rather too raw.

Indeed, as you can see from the implementation, --default-theme doesn't just set the theme setting. Because of the way the "use system theme" option and the light/dark themes operate, since #77809, it is necessary to change the default for "use system theme" to "false" for the --default-theme option to work.

Indeed, I think this fact about #77809 shows that my pre-#77809 approach of having a raw --default-setting option, and a cooked --default-theme option, was correct. Before #77809, --default-theme just set the theme setting. Now it does more.

Notice that the stability caveats for the two options are rather different. Another aspect of this is that it might easily make sense to stabilise the --default-theme option soon, after we've had some experience with it. Stabilising the raw --default-setting option would involve thinking about what additional promises that would imply (maybe none that we care about? but I'm not sure and it's certainly harder to think about)

jyn514 · 2020-10-28T21:16:36Z

src/librustdoc/config.rs

+                .flatten()
+                .collect(),
+            matches
+                .opt_strs("default-setting")


One more thing that --default-theme breaks is you now have different behavior for --default-theme x and --default-setting theme=x - the first will set use-system-theme=false, the second will not.

Yes. That is inevitable and deliberate. As you can see from the usage message we do not make any promises about the particular behaviour of --default-setting. The available settings and their semantics and not documented and not stable.

Whereas the semantics of --default-theme are well-defined, and are implemented by setting the two internal settings.

I do think it is worth keeping --default-setting even despite these caveats because in practice the storage settings are visible to users via their browser and of course if they look at the JS.

The difference between --default-setting and --default-theme is like the difference between firefox's about:config and about:preferences.

I guess my question is why you would ever want to modify default-setting. All those settings are also configurable on a per-user basis on settings.html - do we really need so much flexibility as to make the 'default defaults' configurable too?

src/librustdoc/config.rs

Co-authored-by: Joshua Nelson <joshua@yottadb.com>

@jyn514

This allows removing a `mut` which is nicer. Suggested-by: @jyn514 Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

ijackson · 2020-10-28T21:40:01Z

I guess my question is why you would ever want to modify default-setting. All those settings are also configurable on a per-user basis on settings.html - do we really need so much flexibility as to make the 'default defaults' configurable too?

The reasons I gave for the default theme here: #77024 (comment) apply to all of these default options too. The same reasons motivate many of rustdoc's options, such as `--theme`, `--extend-css`, `--index-page`, etc. etc. etc.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

jyn514 · 2020-10-28T21:42:58Z

@bors r=jyn514,GuillaumeGomez

In practice I don't expect --default-settings to be used often, but since it's perma-unstable I'm ok with adding it. --default-theme definitely seems like a good improvement.

bors · 2020-10-28T21:43:00Z

📌 Commit 776e204 has been approved by jyn514,GuillaumeGomez

ijackson · 2020-10-28T21:49:08Z

In practice I don't expect --default-settings to be used often, but since it's perma-unstable I'm ok with adding it. --default-theme definitely seems like a good improvement.

Thanks. I will probably propose stabilising --default-theme after it's had a chance to bed in. Many of the other options which interact with details of the CSS are already stable. Ian.

…

-- Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

Rollup of 11 pull requests Successful merges: - rust-lang#77213 (rustdoc options to set default theme (and other settings)) - rust-lang#78224 (min_const_generics: allow ty param in repeat expr) - rust-lang#78428 (MinConstGenerics UI test for invalid values for bool & char) - rust-lang#78460 (Adjust turbofish help message for const generics) - rust-lang#78470 (Clean up intra-doc links in `std::path`) - rust-lang#78475 (fix a comment in validity check) - rust-lang#78478 (Add const generics tests for supertraits + dyn traits.) - rust-lang#78487 (Fix typo "compiltest") - rust-lang#78491 (Inline NonZeroN::from(n)) - rust-lang#78492 (Update books) - rust-lang#78494 (Fix typos) Failed merges: r? `@ghost`

As discussed in rust-lang#77213, this seems like it has bedded in and can be safely and usefully made stable. (rustdoc already has other stable options that interact quite intimately with the rustdoc-supplied CSS, and also an option for supplying entirely different CSS, so exposing the theme names this way seems a very minor step.) Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

rustdoc: stabilise --default-theme command line option As discussed in rust-lang#77213, this seems like it has bedded in and can be safely and usefully made stable. (rustdoc already has other stable options that interact quite intimately with the rustdoc-supplied CSS, and also an option for supplying entirely different CSS, so exposing the theme names this way seems a very minor step.) There is also a commit to do some minor grammar fixes to the help message.

rust-highfive assigned GuillaumeGomez Sep 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2020

ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Sep 26, 2020

camelid added A-rustdoc-themes Area: Themes for HTML pages generated by rustdoc A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Oct 13, 2020

GuillaumeGomez reviewed Oct 13, 2020

View reviewed changes

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

ijackson force-pushed the wip-rustdoc-settings branch from c50bb7b to ab195aa Compare October 13, 2020 18:25

jyn514 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 Oct 24, 2020

ijackson added 4 commits October 28, 2020 15:44

ijackson force-pushed the wip-rustdoc-settings branch from ab195aa to 709efd9 Compare October 28, 2020 18:05

GuillaumeGomez reviewed Oct 28, 2020

View reviewed changes

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

GuillaumeGomez reviewed Oct 28, 2020

View reviewed changes

src/librustdoc/lib.rs Outdated Show resolved Hide resolved

rustdoc: Fix some nits

39b80cb

* Remove a needless comma in the Rust code * Replace double spaces after full stops with single spaces Requested-by: @GuillaumeGomez Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

GuillaumeGomez approved these changes Oct 28, 2020

View reviewed changes

jyn514 reviewed Oct 28, 2020

View reviewed changes

src/librustdoc/config.rs Show resolved Hide resolved

src/librustdoc/config.rs Outdated Show resolved Hide resolved

ijackson and others added 2 commits October 28, 2020 21:25

Add a comment about non-panicking of splitn().next().unwrap()

1d6c860

Co-authored-by: Joshua Nelson <joshua@yottadb.com>

rustdoc: Use Vec::into_iter() rather than drain()

776e204

This allows removing a `mut` which is nicer. Suggested-by: @jyn514 Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2020

JohnTitor mentioned this pull request Oct 29, 2020

Rollup of 11 pull requests #78512

Merged

bors merged commit 2008d1b into rust-lang:master Oct 29, 2020

rustbot added this to the 1.49.0 milestone Oct 29, 2020

ijackson deleted the wip-rustdoc-settings branch October 29, 2020 11:04

ijackson mentioned this pull request Dec 2, 2020

rustdoc: stabilise --default-theme command line option #79642

Merged

jsha mentioned this pull request Apr 25, 2021

rustdoc: clarify theme settings #84539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc options to set default theme (and other settings) #77213

rustdoc options to set default theme (and other settings) #77213

ijackson commented Sep 26, 2020 •

edited

Loading

rust-highfive commented Sep 26, 2020

rust-highfive commented Sep 26, 2020

jyn514 commented Oct 13, 2020

GuillaumeGomez commented Oct 13, 2020

ijackson commented Oct 13, 2020

bors commented Oct 16, 2020

ijackson commented Oct 16, 2020

jyn514 commented Oct 23, 2020

ijackson commented Oct 23, 2020 via email

GuillaumeGomez commented Oct 24, 2020

ijackson commented Oct 28, 2020

GuillaumeGomez commented Oct 28, 2020

ijackson commented Oct 28, 2020 via email

GuillaumeGomez commented Oct 28, 2020

ijackson commented Oct 28, 2020

GuillaumeGomez commented Oct 28, 2020

jyn514 Oct 28, 2020

jyn514 Oct 28, 2020

ijackson Oct 28, 2020

jyn514 Oct 28, 2020

ijackson Oct 28, 2020

jyn514 Oct 28, 2020

ijackson commented Oct 28, 2020 via email

jyn514 commented Oct 28, 2020

bors commented Oct 28, 2020

ijackson commented Oct 28, 2020 via email

rustdoc options to set default theme (and other settings) #77213

rustdoc options to set default theme (and other settings) #77213

Conversation

ijackson commented Sep 26, 2020 • edited Loading

rust-highfive commented Sep 26, 2020

rust-highfive commented Sep 26, 2020

jyn514 commented Oct 13, 2020

GuillaumeGomez commented Oct 13, 2020

ijackson commented Oct 13, 2020

bors commented Oct 16, 2020

ijackson commented Oct 16, 2020

jyn514 commented Oct 23, 2020

ijackson commented Oct 23, 2020 via email

GuillaumeGomez commented Oct 24, 2020

ijackson commented Oct 28, 2020

GuillaumeGomez commented Oct 28, 2020

ijackson commented Oct 28, 2020 via email

GuillaumeGomez commented Oct 28, 2020

ijackson commented Oct 28, 2020

GuillaumeGomez commented Oct 28, 2020

jyn514 Oct 28, 2020

Choose a reason for hiding this comment

jyn514 Oct 28, 2020

Choose a reason for hiding this comment

ijackson Oct 28, 2020

Choose a reason for hiding this comment

jyn514 Oct 28, 2020

Choose a reason for hiding this comment

ijackson Oct 28, 2020

Choose a reason for hiding this comment

jyn514 Oct 28, 2020

Choose a reason for hiding this comment

ijackson commented Oct 28, 2020 via email

jyn514 commented Oct 28, 2020

bors commented Oct 28, 2020

ijackson commented Oct 28, 2020 via email

ijackson commented Sep 26, 2020 •

edited

Loading