Skip to content

Safer hook management#425

Merged
grantmcdermott merged 8 commits intograntmcdermott:mainfrom
vincentarelbundock:hooks
Jun 15, 2025
Merged

Safer hook management#425
grantmcdermott merged 8 commits intograntmcdermott:mainfrom
vincentarelbundock:hooks

Conversation

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

@grantmcdermott and @etiennebacher,

Here’s a first attempt to resolve the tinytheme in a loop issue. The idea is:

  1. Whenever tpar() sets a hook, we save that hook to the .tinyplot environment.
  2. When tinytheme() resets the theme via init_tpar(), we retrieve the hooks from the .tinyplot environment and remove only the "before.plot.new" hooks that are identical() to those.

Implementation notes:

  1. I copied the set_hooks() and remove_hooks() functions from the evaluate package and made a few minor changes to them. That package is MIT and included a note at the top of the file.
  2. To make it easier to retrieve variables from the .tinyplot environment, I added a get_environment_variable() and set_environment_variable() functions.

The tests pass on my computer, and this seems to work.

I’m not exactly sure I know all the behaviors that were broken, so I’d appreciate if you could take this for a spin.

res <- evaluate::evaluate(function(){ 
    library(tinyplot)
    for (thm in c("dark", "minimal")) {
        tinytheme(thm)
        tinyplot(I(Sepal.Length * 1e4) ~ Petal.Length | Species, data = iris)
    }
})
sapply(res, class)
#> [1] "source"       "source"       "recordedplot" "recordedplot" "recordedplot"
#> [6] "recordedplot"

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

CI error says this:

  Error: `cairo` must be a logical flag.
  Execution halted

but I have no idea what I did in the PR to generate it. Thoughts?

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

Oh, I found the issue. Tests now pass.

@grantmcdermott
Copy link
Copy Markdown
Owner

Super thanks mate. I've been slammed, but hopefully able to review later today.

@grantmcdermott
Copy link
Copy Markdown
Owner

One thing in advance, please add a news item if you can. Takk.

@grantmcdermott
Copy link
Copy Markdown
Owner

Confirmed fix for yihui/knitr#2392

This is great, thanks @vincentarelbundock!

@grantmcdermott grantmcdermott merged commit 0f3f350 into grantmcdermott:main Jun 15, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants