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.

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

Restore previous shiny theme when run() exits #2160

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

cpsievert
Copy link
Contributor

Closes #2159

Comment on lines +144 to 146
old_theme <- shiny::getCurrentTheme()
on.exit(set_current_theme(old_theme), add = TRUE)
set_current_theme(resolve_theme(format$options$theme))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure when I originally wrote the set_current_theme() code I was thinking that since set_current_theme() uses shiny::shinyOptions(), the previous value would be automatically restored, but it actually doesn't because this call happens outside of an app/session scope (in that case, shiny::shinyOptions() is basically like setting options()).

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Looks good to me. It makes sense to reset this.

Can you add a NEWS bullet though before we merge ?

@cderv cderv merged commit 9dc5d97 into rstudio:master Jun 4, 2021
@cderv
Copy link
Collaborator

cderv commented Jun 4, 2021

Thanks !

@cpsievert cpsievert deleted the run-theme-leak branch June 4, 2021 16:26
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
* rstudio_origin/master: (90 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
Merge remote-tracking branch 'rstudio_origin/master' into jg-devel

* rstudio_origin/master: (64 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...

# Conflicts:
#	DESCRIPTION
#	R/html_dependencies.R
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
Merge remote-tracking branch 'rstudio_origin/master' into minimal-tree-fix

* rstudio_origin/master: (223 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...

# Conflicts:
#	DESCRIPTION
#	NEWS.md
#	R/html_dependencies.R
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run() leaks shiny's theme state
2 participants