Preview R Markdown files via background process by ElianHugh · Pull Request #692 · REditorSupport/vscode-R

ElianHugh · 2021-07-01T13:52:34Z

What problem did you solve?

Improves previewing rmd files, as per some discussion in #103. Not quite the same experience as vscode's markdown preview, but is a little less cumbersome than running rmarkdown::run manually.

~~The preview "engine" is controlled through a configuration setting, and currently supports rmarkdown::run or xaringan::infinite_moon_reader.~~

Some stuff that I think would be neat to have at some point:

scroll sync with rmd source
~~2. get the document's http content (similar to how the help provider functions), meaning that the iframe isn't needed~~

Screenshot

- Preview rmd w/o blocking the terminal

- Fixed a bug that meant the side preview was incorrectly opening in the active column - Fixed a bug that incorrectly caused show source to open the source doc in a new column

- Rename some commands and the PreviewProvider to be more in line with vscode's markdown preview names

renkun-ken · 2021-07-03T00:23:31Z

It works quite nicely already.

I wonder if it makes sense to create a R Markdown output channel to show the stdout and stderr of the background R process as it renders the document?

- Moved refresh to overflow menu - Change how 'Show Source' retrieves the original column - 'R Markdown' output channel for stderr and stdout - Better track and dispose of child processes - Progress bar now stays for the duration of loading - Now tells the user when the document failed to knit

- Kill processes that error *after* knitting - Fix refresh bug that caused panel to go blank

- Knitting can now be cancelled, and the process is properly killed - Can no longer spam the preview button of the same document while knitting, preventing duplicate webviews

renkun-ken · 2021-07-05T09:39:03Z

It looks like the shiny app started by rmarkdown::run() does copy the web resources to local (see https://github.com/rstudio/rmarkdown/blob/master/R/shiny.R#L264) and the urls are replaced to local urls whenever an auto refresh occurs. This violates the security policy of the WebView on accessing local resources.

renkun-ken · 2021-07-05T09:47:34Z

Another approach to avoid this is to only use "rmarkdown::render()" or any other engine that is able to produce an HTML file. And we could use fs.watch() to watch the file and implement auto-render on file changes. But this will add much complexity.

ElianHugh · 2021-07-05T09:57:00Z

I suppose using rmarkdown::render would also ensure that syntax highlighting is maintained, and that we would have more control over the document's styling. I wonder if there is much of a difference performance and compilation wise

- busyUri probably makes more sense as a set, rather than an array - Show rmarkdown output on error - change return values for RMarkdownChildStore methods to what is expected of a set<T>

renkun-ken · 2021-07-05T10:03:49Z

I suppose using rmarkdown::render would also ensure that syntax highlighting is maintained, and that we would have more control over the document's styling.

Exactly. Both mathjax and syntax highlighting should work after reload.

I wonder if there is much of a difference performance and compilation wise

I think there should not be a significant performance difference compared with rendering from a shiny app.

Changes preview engine to rmarkdown::render, and relies on fs.watch to check for updates. This is a pretty messy implementation, and will be cleaned up shortly

ElianHugh · 2021-07-06T01:48:50Z

I've added a super messy implementation of the document file watcher, which appears to function as expected. Will be cleaned up shortly

renkun-ken · 2021-07-06T05:51:53Z

I move watcherDir from session.ts to extDir in extension.ts as a global extension directory in user home. And temporary files are generated into ~/.vscode-R/tmp as tmpDir. Now session watcher uses extDir and rmarkdown preview uses tmpDir.

ElianHugh · 2021-07-07T02:40:03Z

I move watcherDir from session.ts to extDir in extension.ts as a global extension directory in user home. And temporary files are generated into ~/.vscode-R/tmp as tmpDir. Now session watcher uses extDir and rmarkdown preview uses tmpDir.

Thanks! Works quite nicely - I've also removed references to dir because it will just end up being tmpDir anyway

- CSS can now be toggled between vscode theme and base doc theme - Changed store from set to map - now cache html in RMarkdownChild, so that toggling doesn't require a re-read of the source html - Ensure output is html - Refactor is ongoing

ElianHugh · 2021-07-07T03:31:27Z

Latest push now has a 'toggle style' button similar to the plot viewer:

Refactor from set to map is ongoing - RMarkdownChild will have the input uri removed soon as its now superfluous

renkun-ken · 2021-07-08T02:19:33Z

The dark theme looks good, but the output is almost invisible under many dark themes.

Improve styling so as to not create unreadable text

ElianHugh · 2021-07-08T10:00:32Z

The dark theme looks good, but the output is almost invisible under many dark themes.

Don't see the problem here 😅

Should be fixed following last push!

renkun-ken · 2021-07-09T06:45:46Z

The theming looks good now. And vscode.env.openExternal(this.activePreview?.preview?.outputUri) works well on Ubuntu, locally. If we use an open-in-browser package, and then it won't work in remote development scenario where an HTTP server must be created to serve the HTML page. In this case, we need to rely on something like Live Preview.

ElianHugh · 2021-07-09T09:17:01Z

Makes sense to me! I had a quick look into scroll syncing and it gave me a headache. I think if we are happy with the current features then we can save that for a later date, and just iterate on the current codebase

renkun-ken

LGTM

renkun-ken · 2021-07-09T09:49:23Z

There's one more thing. I think we need to add a setting (e.g. r.rmarkdown.preview.autoRefresh) to control whether to auto-render the document on file update. Real world documents could be long and heavy, we could use such a setting to control the default behavior and also put a button or menu on the editor toolbar to toggle the behavior.

Anyway, we can merge this nice PR and implement it later.

renkun-ken · 2021-07-09T12:58:11Z

"Enable/Disable Auto Refresh" commands and r.rmarkdown.preview.autoRefresh are added.

User could control whether to auto-refresh for each preview now.

ElianHugh · 2021-07-09T13:59:36Z

Awesome, thanks for the great work! I wonder if it's worth thinking about forcing or having an option for granular caching, particularly one of the lower settings (i.e. cache = 1 or cache = 2).

renkun-ken · 2021-07-09T14:10:59Z

Awesome, thanks for the great work! I wonder if it's worth thinking about forcing or having an option for granular caching, particularly one of the lower settings (i.e. cache = 1 or cache = 2).

Should the rmd author control the cache level or should we have control on it?

ElianHugh · 2021-07-09T14:23:14Z

Hmm, after some testing, it probably shouldn't be forced, even in a document preview. There are a few situations which can be confusing if caching is forced globally:

b <- 5

b + 5

Even if chunk 1 is changed later, chunk 2 will print 10.

renkun-ken · 2021-07-09T14:50:43Z

@ElianHugh Do you have anything to add? If not, I'll merge this PR.

ElianHugh · 2021-07-09T14:52:06Z

No - looks good to me!

ManuelHentschel · 2021-07-09T20:45:26Z

The command R: Open Preview isn't working on windows. As far as I can tell, this is due to using the unqoted path here (which doesn't work for paths containing spaces), the linebreaks here, and the backslashes in the paths here.

ManuelHentschel · 2021-07-09T20:49:28Z

+      {
+        "command": "r.markdown.showPreviewToSide",
+        "title": "Open Preview to the Side",
+        "icon": "$(open-preview)",
+        "category": "R"
+      },
+      {
+        "command": "r.markdown.showPreview",
+        "title": "Open Preview",
+        "icon": "$(open-preview)",
+        "category": "R"
+      },
+      {
+        "command": "r.markdown.preview.refresh",
+        "title": "Refresh Preview",
+        "icon": "$(refresh)",
+        "category": "R"
+      },
+      {
+        "command": "r.markdown.preview.openExternal",
+        "title": "Open in External Browser",
+        "icon": "$(link-external)",
+        "category": "R"
+      },
+      {
+        "command": "r.markdown.preview.showSource",
+        "title": "Show Source",
+        "icon": "$(go-to-file)",
+        "category": "R"
+      },
+      {
+        "title": "Toggle Style",
+        "category": "R",
+        "command": "r.markdown.preview.toggleStyle",
+        "icon": "$(symbol-color)"
+      },
+      {
+        "title": "Enable Auto Refresh",
+        "category": "R",
+        "command": "r.markdown.preview.enableAutoRefresh",
+        "icon": "$(sync)"
+      },
+      {
+        "title": "Disable Auto Refresh",
+        "category": "R",
+        "command": "r.markdown.preview.disableAutoRefresh",
+        "icon": "$(sync-ignored)"
+      },


I'd suggest using "category": "R Markdown" for (most of) these commands, otherwise it's not quite obvious what they do, when invoking them from the command palette

ManuelHentschel · 2021-07-09T20:53:36Z

+import { RMarkdownPreviewManager } from './rmarkdown/preview';

 // global objects used in other files
+export const extDir: string = path.join(os.homedir(), '.vscode-R');


Should we give this a slightly longer/more descriptive name? For local variables, these short names are fine, but global variables should be clear without having to "look them up" in a different file

ManuelHentschel · 2021-07-09T20:59:22Z

+import { config, doWithProgress, getRpath, readContent, setContext, escapeHtml } from '../util';
+import { extensionContext, tmpDir } from '../extension';
+
+class RMarkdownPreview extends vscode.Disposable {


Personally, I find it easier to just give the class a dispose() method, instead of inheriting from vscode.Disposable (afaik, there is no difference in behaviour)

ManuelHentschel · 2021-07-09T21:01:44Z

+            this.cp?.kill('SIGKILL');
+            this.panel?.dispose();
+            this.fileWatcher?.close();
+            fs.removeSync(this.outputUri.path);


Also applies to some other places: vscode.Uri has a fsPath property which is more robust regarding windows drive letters, backslashes etc.

ManuelHentschel · 2021-07-09T21:03:08Z

+        this.startFileWatcher(RMarkdownPreviewManager, uri);
+    }
+
+    public styleHtml(themeBool: boolean) {


E.g. useDarkTheme is more descriptive than themeBool

DonJayamanne · 2021-07-12T01:40:22Z

@renkun-ken @ElianHugh Apologies if this is a very silly question.
Can we not try to use the VS Codes Notebook UI to render markdown files (instead of vscodes markdown preview).
Its my understanding that this current implementation just displays the rmd file as HTML.

Does this current implementation execute cells or just display the markdown content & code in the HTML preview?
If this is the case, would the new VS Code Notebook interface been beneficial? (we can have a readonly interface).
The benefit is it will be more resource intensive & significantly performant.

ElianHugh · 2021-07-12T02:04:17Z

@renkun-ken @ElianHugh Apologies if this is a very silly question.
Can we not try to use the VS Codes Notebook UI to render markdown files (instead of vscodes markdown preview).
Its my understanding that this current implementation just displays the rmd file as HTML.
* Does this current implementation execute cells or just display the markdown content & code in the HTML preview?
  If this is the case, would the new VS Code Notebook interface been beneficial? (we can have a readonly interface).
  The benefit is it will be more resource intensive & significantly performant.

Hi @DonJayamanne, not a silly question at all :)

The current preview implementation executes all code cells, so that the output html file is essentially what the final .rmd output would be.

E.g., here I have a .rmd file on the left, and the preview on the right, executed parts of the cells in poorly scribbled red:

Apologies, but I'm not super familiar with the Notebook UI, so @renkun-ken would probably have a better answer re: using it for previews

DonJayamanne · 2021-07-12T02:08:30Z

Thanks for the prompt response.

The current preview implementation executes all code cells, so that the output html file is essentially what the final .rmd

Awesome.
Nothing to change here. I was hoping we weren't executing the code, but looks like we are doing both, executing & generating HTML output in one step (using the cli).
More info about the notebook ui can be found here https://code.visualstudio.com/api/extension-guides/notebook

renkun-ken · 2021-07-12T02:08:43Z

@renkun-ken @ElianHugh Apologies if this is a very silly question.
Can we not try to use the VS Codes Notebook UI to render markdown files (instead of vscodes markdown preview).
Its my understanding that this current implementation just displays the rmd file as HTML.

I don't quite understand the question. The rendering is all done by rmarkdown::render("file.Rmd"), we just call it in background and present the resulted html file in a WebView.

This is a just equivalent of RStudio feature.

DonJayamanne · 2021-07-12T02:15:33Z

This is a just equivalent of RStudio feature.

Thats fine.
My assumption was that we weren't executing the code and we were using this tool to generate HTML to simply preview the RMD with math renderer (markdown) correctly & felt the VS Codes Notebook UI would achieve that.

But that's not the case, hence we can consider my question resolved. Thanks

ElianHugh and others added 6 commits July 1, 2021 13:59

Initial rmd preview implementation

20e5096

- Preview rmd w/o blocking the terminal

Add custom context for previews

7ec5c65

Show source, refresh, and open external for rmd previews

7a7559d

Refactor, bug fix view column, capitalisation

2bd4d53

- Fixed a bug that meant the side preview was incorrectly opening in the active column - Fixed a bug that incorrectly caused show source to open the source doc in a new column

Bugfix inf_mr, R to rPath, progress notification

5631062

Rename commands

2dd2410

- Rename some commands and the PreviewProvider to be more in line with vscode's markdown preview names

renkun-ken reviewed Jul 3, 2021

View reviewed changes