-
-
Notifications
You must be signed in to change notification settings - Fork 978
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
render_args passed to run() can contain theme in output_options #2065
base: main
Are you sure you want to change the base?
Conversation
…er for bslib theme resolution in shiny
format <- output_format_from_yaml_front_matter( | ||
input_lines = read_utf8(target_file), | ||
output_options = render_args$output_options | ||
) | ||
set_current_theme(resolve_theme(format$options$theme)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no theme
option is provided in the YAML header, and in render_args
, this means that the format default will be used for rendering. However, output_format_from_yaml_front_matter
won't resolved default value and just return what it in the YAML, so in the case describe above, format$options$theme
will be NULL and resolve_theme()
will return NULL, as if no theme.
Is this important for set_current_theme
? I believe it should it return the default value of the format instead, but not sure.
What do you think @cpsievert ?
If you tell me what you need, I'll find the way to make it using rmarkdown internal function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be extra paranoid, maybe we should also change this line in set_current_theme()
:
if (is.function(set_theme)) set_theme(theme)
to
if (is.function(set_theme) && is_bs_theme(theme)) set_theme(theme)
just to make sure other code doesn't get confused about a character string as the theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this in ff6c7b8. It required to rewrite the test as shinyOptions()
was not reset between test and passing NULL or a character to set_current_theme
would have not reset the previous one. I could have change the order of the test, but having a clean way of testing this is safer for later.
Regarding my comment above, it is answered currently as only bs theme object will be passed. It will always be passed by users in YAML or command arguments, so it will work.
However, if a bs theme is a default for a output format (which is not the case yet), I still think we may have an issue and the theme value won't be passed to shiny as you expect. We only catch user specified theme here, not default format theme. Is it catch elsewhere ?
I'll try to check that with a test case.
R/shiny.R
Outdated
@@ -595,5 +597,5 @@ read_shiny_deps <- function(files_dir) { | |||
# shiny:::setCurrentTheme() was added in 1.6 (we may export in next version) | |||
set_current_theme <- function(theme) { | |||
set_theme <- asNamespace("shiny")$setCurrentTheme | |||
if (is.function(set_theme)) set_theme(theme) | |||
if (is.function(set_theme) && is_bs_theme(theme)) set_theme(theme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the bad advice/confusion @cderv -- What I had before was actually correct. Even when we're not using bslib, we still want to "clear" the previously set theme (if any) so that future theming code knows that bslib is not actually relevant. I was essentially worried about themable components using an is.null(theme)
/length(theme)
to check for the presence of a bslib theme (when they should be using is_bs_theme(theme)
, but after thinking about it more, I think this is the right way to go:
if (is.function(set_theme) && is_bs_theme(theme)) set_theme(theme) | |
if (is.function(set_theme)) set_theme(theme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I'll revert then.
However, my question still remains from this comment discussion: #2065 (comment)
Basically, we are retrieving the theme
information from the YAML. If there is none theme
passed to set_current_theme
is NULL but if the output format as a default theme base on bslib, then you won't have that information. Also, html_document
default theme is "default"
, but this is information is not in the YAML but in the output format itself.
format <- output_format_from_yaml_front_matter(
input_lines = read_utf8(target_file),
output_options = render_args$output_options
)
# will be NULL if no `theme` in the YAML or in `render_args$output_options`
format$options$theme
I am under the impression you need to know the theme set and used in the document, to make shiny aware.
If so, I think we are missing something.
Does that make sense ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me rephrase to see if I understand correctly: you're worried about the case where no theme
is in the target_file
's yaml and render_args
(in that case, format$options$theme
will be NULL
even though it is really formals(output_format)$theme
, right)? In theory, yes, this is a problem (in practice, it's not currently a big problem since no formats default to a {bslib}
theme). That being said, if it'd be great if we did do the right thing here (something like theme <- format$options$theme %||% formals(output_format)$theme
-- do you know how the get the output_format
function?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you rephrased correctly and this was my thoughts:
In theory, yes, this is a problem (in practice, it's not currently a big problem since no formats default to a {bslib} theme).
I think we could have in the near future some output format that would default do bslib theme maybe.
But on second thought, if this happens it would be more complicated. Usually, the theme argument is processed inside the output formats - e.g. a now format that would default to pulse bootswatch theme.
if (theme == "default") theme = bslib::bs_theme(bootswatch = "pulse")
In this case, it is not easy to retrieve the information, and formals(output_format)$theme
would be "default"
which may not help.
Example of an output format a user can create: a flexdasboard format only for BS4 bootswatch theme
bootswatch4_dashboard <- function(..., theme = "default") {
if (theme == "default") theme <- "pulse"
theme <- bslib::bs_theme(version = 4, bootswatch = theme)
flexdashboard::flex_dashboard(..., theme = theme)
}
dir.create(tmp_dir <- tempfile())
owd <- setwd(tmp_dir)
rmarkdown::draft("test.Rmd", "flex_dashboard", "flexdashboard", edit = FALSE)
rmarkdown::render("test.Rmd", bootswatch4_dashboard())
browseURL("test.html")
setwd(owd)
unlink(tmp_dir)
It is difficult to know the bslib theme in advance in rmarkdown::run()
in this case.
So in practice as you said, it is not a big problem currently. But it could be unexpected behavior for custom formats. Am I thinking too far ?
We may leave it as is for now and think more about this later when / if new formats with bslib theme as default come up ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, right, good point. Let's add a comment about this scenario just above the set_current_theme()
call. I think perhaps the best way to solve this issue would be to set_current_theme()
again inside of html_document_base()
before any user code runs. Is it possible to detect a shiny runtime the pre_knit
hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to detect a shiny runtime the pre_knit hook?
Currently it is not supported - runtime is known but not passed in pre_knit processor function, only in post_knit processor function.
But this is something we can easily add IMO - if developers have followed advice in the doc:
pre_knit: An optional function that runs before knitting which receives the input (input filename passed to render) and ... (for future expansion) arguments.
If unfortunately so custom formats use a pre_knit function without ..., then it will be difficult to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be all for it if you're happy to deal with pain it might cause :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it.
To summarize the issue we are trying to solve, theme info must be passed to shiny before knit so that customization can be done by user in a chunk (for bslib themer specifically). Otherwise, if shiny does not have the information of the document theme it won't work correctly.
Did I understand correctly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
revert previous changes ff6c7b8
This takes into account https://github.com/rstudio/rmarkdown/pull/2049/files#r586431698 with @cpsievert
I believe this is needed in case someone run
because it would overwrite any theme option set in the YAML header.
This may not be the only missed behavior - see coment below.