Conversation
ThomasSoeiro
left a comment
There was a problem hiding this comment.
Hey, I am unable to review your changes, but I just left a quick comment just in case :) Thanks!
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed a5f505a using this PR's bundled sess 3.0.9000.9000, installed in an isolated library and confirmed loaded inside the R terminal. The latest NULL handling for deprecated plot arguments looks correct.
One remaining P2 finding is attached inline: folder-level backend settings are ignored.
Validation: all 495 R assertions and 79 existing tests in the plotBackend, terminal, session, and sessInstall extension suites pass. A native-terminal probe also passes: the configured device is preserved, plotting opens no integrated viewer, and View() still works. Build, type checking, and CI pass. An additional regression using real multi-root configuration fails as described in the inline comment.
|
|
||
| export function resolveBackend(): PlotBackend { | ||
| const selected = getMigratedSetting<string | boolean>( | ||
| config(), |
There was a problem hiding this comment.
[P2] Respect folder-level backend settings
The PR changes r.plot.backend and r.plot.useHttpgd to machine-overridable, which permits folder overrides, but resolveBackend() still calls config() without a resource. I reproduced this in a real multi-root VS Code workspace: folder A has r.plot.backend = native and the workspace has standard; getConfiguration('r', folderA.uri) returns native, but makeTerminalOptions(folderA.uri) emits SESS_PLOT_BACKEND = standard. As a result, plotting uses the integrated viewer despite the folder's native preference. Pass the terminal's resource through backend resolution and keep discovery generation consistent with that resolved backend. Please add a multi-root regression test covering different folder backend values. This reproduction used the updated sess 3.0.9000.9000 bundled in this PR.
There was a problem hiding this comment.
Thank you for pointing that out.
I've set the scope to window and configured ignoreSync.
I thought it would be more appropriate to implement machine-overridable in a subsequent PR.
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the latest changes at a624bd2. No actionable issues found in this follow-up. The previously posted P2 finding is resolved: both r.plot.backend and r.plot.useHttpgd now use window scope with ignoreSync: true, keeping backend selection consistent across the workspace while excluding these settings from Settings Sync by default.
Verified using this PR's bundled sess 3.0.9000.9000, installed in an isolated library and confirmed loaded inside the R terminal:
- In a real multi-root VS Code workspace, both folders receive the workspace backend correctly for standard, native, httpgd, and auto. Conflicting folder settings no longer override the window-scoped setting.
- Native plotting preserves the configured graphics device, opens no integrated plot viewer, and retains View() integration.
- Both focused integration probes pass. Type checking, the build, and all CI checks pass.
Closes #1806
Summary
nativetor.plot.backendand carry the choice through managed terminals, manual attach, the public session API, and reconnects.native, keep sess IPC and non-plot integrations while leaving R's graphics device option, plot hooks, plot task callback, and devices untouched.use_httpgdanduse_jgdarguments as deprecated compatibility shims, and preserve theautoorder: jgd, then httpgd, then the standard viewer.3.0.9000.9000so existing installations receive the updated API.Follow-up fix
JGD_SOCKETterminal environment mutation when switching tonative,standard, orhttpgd. This leavesprocess.env.JGD_SOCKETand existing R processes untouched.jgd/autoregistration andnative/standard/httpgdcleanup.Plot backend settings and Settings Sync
r.plot.backendand its deprecated compatibility settingr.plot.useHttpgdatwindowscope, and setignoreSync: trueon both. Backend availability depends on the local or remote R installation, so these values should not sync between machines.Deprecated plot argument handling
use_httpgdanduse_jgdtoNULLin bothconnect()andregister_hooks(). Treat non-NULL values, includingFALSE, as use of the deprecated API and emit a warning.auto. If one flag is supplied, fill in the other with its legacy default (use_httpgd = TRUE,use_jgd = FALSE). An explicitplot_backendstill takes precedence.test-native-plot.Rpass locally.