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

Jump to bottom

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

Merged
merged 7 commits into from
Oct 29, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e10475
rustdoc: js: Use getSettingValue for all rustdoc-* values
ijackson Sep 23, 2020
5cd96d6
rustdoc: Provide a way to set the default settings from Rust code
ijackson Sep 23, 2020
d8a4497
rustdoc: Provide a general --default-setting SETTING[=VALUE] option
ijackson Oct 13, 2020
709efd9
rustdoc: Provide a --default-theme THEME option
ijackson Oct 28, 2020
39b80cb
rustdoc: Fix some nits
ijackson Oct 28, 2020
1d6c860
Add a comment about non-panicking of splitn().next().unwrap()
ijackson Oct 28, 2020
776e204
rustdoc: Use Vec::into_iter() rather than drain()
ijackson Oct 28, 2020
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
32 changes: 31 additions & 1 deletion src/librustdoc/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};
use std::convert::TryFrom;
use std::ffi::OsStr;
use std::fmt;
Expand Down Expand Up @@ -216,6 +216,9 @@ pub struct RenderOptions {
pub extension_css: Option<PathBuf>,
/// A map of crate names to the URL to use instead of querying the crate's `html_root_url`.
pub extern_html_root_urls: BTreeMap<String, String>,
/// A map of the default settings (values are as for DOM storage API). Keys should lack the
/// `rustdoc-` prefix.
pub default_settings: HashMap<String, String>,
/// If present, suffix added to CSS/JavaScript files when referencing them in generated pages.
pub resource_suffix: String,
/// Whether to run the static CSS/JavaScript through a minifier when outputting them. `true` by
Expand Down Expand Up @@ -374,6 +377,32 @@ impl Options {
}
};

let default_settings: Vec<Vec<(String, String)>> = vec![
matches
.opt_str("default-theme")
.iter()
.map(|theme| {
vec![
("use-system-theme".to_string(), "false".to_string()),
("theme".to_string(), theme.to_string()),
]
})
.flatten()
.collect(),
matches
.opt_strs("default-setting")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

.iter()
.map(|s| {
let mut kv = s.splitn(2, '=');
// never panics because `splitn` always returns at least one element
let k = kv.next().unwrap().to_string();
ijackson marked this conversation as resolved.
Show resolved Hide resolved
let v = kv.next().unwrap_or("true").to_string();
(k, v)
})
.collect(),
];
let default_settings = default_settings.into_iter().flatten().collect();

let test_args = matches.opt_strs("test-args");
let test_args: Vec<String> =
test_args.iter().flat_map(|s| s.split_whitespace()).map(|s| s.to_string()).collect();
Expand Down Expand Up @@ -596,6 +625,7 @@ impl Options {
themes,
extension_css,
extern_html_root_urls,
default_settings,
resource_suffix,
enable_minification,
enable_index_page,
Expand Down
8 changes: 8 additions & 0 deletions src/librustdoc/html/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::HashMap;
use std::path::PathBuf;

use crate::externalfiles::ExternalHtml;
Expand All @@ -10,6 +11,7 @@ pub struct Layout {
pub logo: String,
pub favicon: String,
pub external_html: ExternalHtml,
pub default_settings: HashMap<String, String>,
pub krate: String,
/// The given user css file which allow to customize the generated
/// documentation theme.
Expand Down Expand Up @@ -53,6 +55,7 @@ pub fn render<T: Print, S: Print>(
<link rel=\"stylesheet\" type=\"text/css\" href=\"{static_root_path}rustdoc{suffix}.css\" \
id=\"mainThemeStyle\">\
{style_files}\
<script id=\"default-settings\"{default_settings}></script>\
ijackson marked this conversation as resolved.
Show resolved Hide resolved
<script src=\"{static_root_path}storage{suffix}.js\"></script>\
<noscript><link rel=\"stylesheet\" href=\"{static_root_path}noscript{suffix}.css\"></noscript>\
{css_extension}\
Expand Down Expand Up @@ -172,6 +175,11 @@ pub fn render<T: Print, S: Print>(
after_content = layout.external_html.after_content,
sidebar = Buffer::html().to_display(sidebar),
krate = layout.krate,
default_settings = layout
.default_settings
.iter()
.map(|(k, v)| format!(r#" data-{}="{}""#, k.replace('-', "_"), Escape(v)))
.collect::<String>(),
style_files = style_files
.iter()
.filter_map(|t| {
Expand Down
1 change: 1 addition & 0 deletions src/librustdoc/html/markdown.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1228,6 +1228,7 @@ fn init_id_map() -> FxHashMap<String, usize> {
map.insert("render-detail".to_owned(), 1);
map.insert("toggle-all-docs".to_owned(), 1);
map.insert("all-types".to_owned(), 1);
map.insert("default-settings".to_owned(), 1);
// This is the list of IDs used by rustdoc sections.
map.insert("fields".to_owned(), 1);
map.insert("variants".to_owned(), 1);
Expand Down
2 changes: 2 additions & 0 deletions src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ impl FormatRenderer for Context {
playground_url,
sort_modules_alphabetically,
themes: style_files,
default_settings,
extension_css,
resource_suffix,
static_root_path,
Expand All @@ -415,6 +416,7 @@ impl FormatRenderer for Context {
logo: String::new(),
favicon: String::new(),
external_html,
default_settings,
krate: krate.name.clone(),
css_file_extension: extension_css,
generate_search_filter,
Expand Down
22 changes: 11 additions & 11 deletions src/librustdoc/html/static/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function defocusSearchBar() {
"derive",
"traitalias"];

var disableShortcuts = getCurrentValue("rustdoc-disable-shortcuts") === "true";
var disableShortcuts = getSettingValue("disable-shortcuts") === "true";
var search_input = getSearchInput();
var searchTimeout = null;
var toggleAllDocsId = "toggle-all-docs";
Expand Down Expand Up @@ -1580,7 +1580,7 @@ function defocusSearchBar() {
function showResults(results) {
var search = getSearchElement();
if (results.others.length === 1
&& getCurrentValue("rustdoc-go-to-only-result") === "true"
&& getSettingValue("go-to-only-result") === "true"
// By default, the search DOM element is "empty" (meaning it has no children not
// text content). Once a search has been run, it won't be empty, even if you press
// ESC or empty the search input (which also "cancels" the search).
Expand Down Expand Up @@ -2296,7 +2296,7 @@ function defocusSearchBar() {
function autoCollapse(pageId, collapse) {
if (collapse) {
toggleAllDocs(pageId, true);
} else if (getCurrentValue("rustdoc-auto-hide-trait-implementations") !== "false") {
} else if (getSettingValue("auto-hide-trait-implementations") !== "false") {
var impl_list = document.getElementById("trait-implementations-list");

if (impl_list !== null) {
Expand Down Expand Up @@ -2370,8 +2370,8 @@ function defocusSearchBar() {
}

var toggle = createSimpleToggle(false);
var hideMethodDocs = getCurrentValue("rustdoc-auto-hide-method-docs") === "true";
var hideImplementors = getCurrentValue("rustdoc-auto-collapse-implementors") !== "false";
var hideMethodDocs = getSettingValue("auto-hide-method-docs") === "true";
var hideImplementors = getSettingValue("auto-collapse-implementors") !== "false";
var pageId = getPageId();

var func = function(e) {
Expand Down Expand Up @@ -2487,15 +2487,15 @@ function defocusSearchBar() {
});
}
}
var showItemDeclarations = getCurrentValue("rustdoc-auto-hide-" + className);
var showItemDeclarations = getSettingValue("auto-hide-" + className);
if (showItemDeclarations === null) {
if (className === "enum" || className === "macro") {
showItemDeclarations = "false";
} else if (className === "struct" || className === "union" || className === "trait") {
showItemDeclarations = "true";
} else {
// In case we found an unknown type, we just use the "parent" value.
showItemDeclarations = getCurrentValue("rustdoc-auto-hide-declarations");
showItemDeclarations = getSettingValue("auto-hide-declarations");
}
}
showItemDeclarations = showItemDeclarations === "false";
Expand Down Expand Up @@ -2569,7 +2569,7 @@ function defocusSearchBar() {
onEachLazy(document.getElementsByClassName("sub-variant"), buildToggleWrapper);
var pageId = getPageId();

autoCollapse(pageId, getCurrentValue("rustdoc-collapse") === "true");
autoCollapse(pageId, getSettingValue("collapse") === "true");

if (pageId !== null) {
expandSection(pageId);
Expand All @@ -2592,7 +2592,7 @@ function defocusSearchBar() {
(function() {
// To avoid checking on "rustdoc-item-attributes" value on every loop...
var itemAttributesFunc = function() {};
if (getCurrentValue("rustdoc-auto-hide-attributes") !== "false") {
if (getSettingValue("auto-hide-attributes") !== "false") {
itemAttributesFunc = function(x) {
collapseDocs(x.previousSibling.childNodes[0], "toggle");
};
Expand All @@ -2611,7 +2611,7 @@ function defocusSearchBar() {
(function() {
// To avoid checking on "rustdoc-line-numbers" value on every loop...
var lineNumbersFunc = function() {};
if (getCurrentValue("rustdoc-line-numbers") === "true") {
if (getSettingValue("line-numbers") === "true") {
lineNumbersFunc = function(x) {
var count = x.textContent.split("\n").length;
var elems = [];
Expand Down Expand Up @@ -2768,7 +2768,7 @@ function defocusSearchBar() {
}
return 0;
});
var savedCrate = getCurrentValue("rustdoc-saved-filter-crate");
var savedCrate = getSettingValue("saved-filter-crate");
for (var i = 0; i < crates_text.length; ++i) {
var option = document.createElement("option");
option.value = crates_text[i];
Expand Down
4 changes: 0 additions & 4 deletions src/librustdoc/html/static/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@
}
}

function getSettingValue(settingName) {
return getCurrentValue("rustdoc-" + settingName);
}

function setEvents() {
var elems = {
toggles: document.getElementsByClassName("slider"),
Expand Down
45 changes: 36 additions & 9 deletions src/librustdoc/html/static/storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,37 @@
// From rust:
/* global resourcesSuffix */
/* global resourcesSuffix, getSettingValue */

var darkThemes = ["dark", "ayu"];
var currentTheme = document.getElementById("themeStyle");
var mainTheme = document.getElementById("mainThemeStyle");
var localStoredTheme = getCurrentValue("rustdoc-theme");

var settingsDataset = (function () {
var settingsElement = document.getElementById("default-settings");
if (settingsElement === null) {
return null;
}
var dataset = settingsElement.dataset;
if (dataset === undefined) {
return null;
}
return dataset;
})();

function getSettingValue(settingName) {
var current = getCurrentValue('rustdoc-' + settingName);
if (current !== null) {
return current;
}
if (settingsDataset !== null) {
var def = settingsDataset[settingName.replace(/-/g,'_')];
if (def !== undefined) {
return def;
}
}
return null;
}

var localStoredTheme = getSettingValue("theme");

var savedHref = [];

Expand Down Expand Up @@ -156,9 +183,9 @@ var updateSystemTheme = (function() {

function handlePreferenceChange(mql) {
// maybe the user has disabled the setting in the meantime!
if (getCurrentValue("rustdoc-use-system-theme") !== "false") {
var lightTheme = getCurrentValue("rustdoc-preferred-light-theme") || "light";
var darkTheme = getCurrentValue("rustdoc-preferred-dark-theme") || "dark";
if (getSettingValue("use-system-theme") !== "false") {
var lightTheme = getSettingValue("preferred-light-theme") || "light";
var darkTheme = getSettingValue("preferred-dark-theme") || "dark";

if (mql.matches) {
// prefers a dark theme
Expand All @@ -181,11 +208,11 @@ var updateSystemTheme = (function() {
};
})();

if (getCurrentValue("rustdoc-use-system-theme") !== "false" && window.matchMedia) {
if (getSettingValue("use-system-theme") !== "false" && window.matchMedia) {
// update the preferred dark theme if the user is already using a dark theme
// See https://github.com/rust-lang/rust/pull/77809#issuecomment-707875732
if (getCurrentValue("rustdoc-use-system-theme") === null
&& getCurrentValue("rustdoc-preferred-dark-theme") === null
if (getSettingValue("use-system-theme") === null
&& getSettingValue("preferred-dark-theme") === null
&& darkThemes.indexOf(localStoredTheme) >= 0) {
updateLocalStorage("rustdoc-preferred-dark-theme", localStoredTheme);
}
Expand All @@ -196,7 +223,7 @@ if (getCurrentValue("rustdoc-use-system-theme") !== "false" && window.matchMedia
switchTheme(
currentTheme,
mainTheme,
getCurrentValue("rustdoc-theme") || "light",
getSettingValue("theme") || "light",
false
);
}
20 changes: 20 additions & 0 deletions src/librustdoc/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,26 @@ fn opts() -> Vec<RustcOptGroup> {
"sort modules by where they appear in the program, rather than alphabetically",
)
}),
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",
)
}),
Comment on lines +272 to +281
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

unstable("default-setting", |o| {
o.optmulti(
"",
"default-setting",
"Default value for a rustdoc setting (used when \"rustdoc-SETTING\" is absent \
from web browser Local Storage). If VALUE is not supplied, \"true\" is used. \
Supported SETTINGs and VALUEs are not documented and not stable.",
"SETTING[=VALUE]",
)
}),
stable("theme", |o| {
o.optmulti(
"",
Expand Down