-
Notifications
You must be signed in to change notification settings - Fork 27
Send profvis output to editor #988
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
Changes from 1 commit
e98f232
0c4af7f
5aea33f
d28e119
9877008
d3189a6
db02e6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| # | ||
| # options.R | ||
| # | ||
| # Copyright (C) 2023-2024 Posit Software, PBC. All rights reserved. | ||
| # Copyright (C) 2023-2025 Posit Software, PBC. All rights reserved. | ||
| # | ||
| # | ||
|
|
||
|
|
@@ -36,3 +36,12 @@ options(plumber.docs.callback = function(url) { | |
| options(shiny.launch.browser = function(url) { | ||
| .ps.ui.showUrl(url) | ||
| }) | ||
|
|
||
| # Show Profvis output in the viewer | ||
| options(profvis.print = function(x) { | ||
| # Render the HTML content to a temporary file | ||
| tmp_file <- htmltools::html_print(x, viewer = NULL) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably worth a note that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I was first implementing htmlwidgets support I actually hooked this up, which was when I discovered that most htmlwidgets are not theme-aware and always render in dark/black colors, so setting the background to black made them unreadable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| # Pass the file to the viewer | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is rather annoying that it doesn't resize. It means the window of text that you are reading is quite small to read. It does resize in RStudio, I wonder what is different there? Screen.Recording.2025-12-15.at.2.03.40.PM.mov
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the nudge, I think I figured it out ... the trick is to force it to render in standalone mode by rendering to tags first. (That's not how RStudio does it, but works here.) db02e6f |
||
| .ps.Call("ps_html_viewer", tmp_file, "R Profile", -1L, "editor") | ||
| }) | ||


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 looked into this again and just learned about a neat Rust feature called lifetime extension.
I originally thought I needed
_srcfileto be stored at top-level so I could pass a reference to it. But that's not what happens even in the original version of the code! Sinceunwrap()takesselfhere, the value is moved out of_srcfileinto a temporary. We then take a reference to it, and Rust extends the lifetime of the temporary to match that of the reference (which is tied toinput).More info at: https://doc.rust-lang.org/reference/destructors.html#temporary-lifetime-extension
With your change, we're creating a new
Optioncontaining a reference, so unwrapping it doesn't consume_srcfile.But armed with the knowledge of lifetime extension we can write this much cleaner version:
Can you please revert your change? Merging it would cause a conflict with PRs/branches of mine. I'll implement the new approach suggested above in these branches.
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.
The reason I made this change was that locally I can't even build ark without it. The original code generates this fatal error on my machine:
Incidentally your proposed version doesn't compile for me either:
Probably has to do with the version of Rust I'm using? I'm on 1.87.
I did revert the change, since it doesn't seem to be causing any trouble in CI.
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.
oh I'm on 1.89. Good to know!
This is from the 1.89 changelog and explains the difference: rust-lang/rust#140593
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've bumped the minimum version to 1.89 on main to avoid such issues in the future.