-
Notifications
You must be signed in to change notification settings - Fork 23
Add handler to allow for autosave #944
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
base: main
Are you sure you want to change the base?
Conversation
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 and indeed we don't need to set these in the Unix path because R detects --save
/--no-save
by itself in R_common_command_line()
:
The Unix version of setup_r()
callsRf_initialize_R()
(
ark/crates/ark/src/sys/unix/interface.rs
Line 47 in 8e7ea98
Rf_initialize_R(args.len() as i32, args.as_mut_ptr() as *mut *mut c_char); |
R_common_command_line()
function.
I think we should merge this independently of whether we merge the Positron-side PR because we don't handle these arguments well currently.
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.
@DavisVaughan Another way to set these options would be to call cmdlineoptions()
from https://github.com/r-devel/r-svn/blob/48256015cc3c3e7cc2235179adccc60a641f6603/src/gnuwin32/system.c#L1098 (it calls the R_common_command_line()
function discussed above).
This is what Rgui does (https://github.com/r-devel/r-svn/blob/48256015cc3c3e7cc2235179adccc60a641f6603/src/gnuwin32/front-ends/rgui.c#L60), as well as RStudio: https://github.com/rstudio/rstudio/blob/1062bea412a4410c0761ba65ca7ba65f019ee9b8/src/cpp/r/session/REmbeddedWin32.cpp#L235-L242.
The RStudio call site also has a note regarding initialisation of translation domains. It seems like we should look into calling that routine to initialise CLI arguments and other stuff, what do you think? It does quite a lot, so maybe we could pair to carefully review it and discuss implications.
As a side note, I initially tried passing the arguments to cmdlineoptions, but somehow it was ignored (I probably did it wrongly)? I tried passing all arguments as they were (didn't work), and got an error saying unrecognized argument |
Further notes: Restoring is handled by R itself in Saving is done when REPL exits via the default clean up methods, which we don't override (yet): https://github.com/r-devel/r-svn/blob/48256015cc3c3e7cc2235179adccc60a641f6603/src/gnuwin32/system.c#L588 and https://github.com/r-devel/r-svn/blob/48256015cc3c3e7cc2235179adccc60a641f6603/src/unix/sys-std.c#L1231 See also https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#index-R_005fCleanUp-1 |
The RStudio code seems to set the options separate from cmdlinoptions, so more similar to my proposal in this PR? |
This PR goes in tandem with posit-dev/positron#9936 to allow auto-saving the workspace to
.RData
when the R session is shut down. The reason we need this is that in Windows Ark is having problems with propagating arguments to the R binary: posit-dev/positron#9927.