RMD Preview fixes by ElianHugh · Pull Request #699 · REditorSupport/vscode-R

ElianHugh · 2021-07-10T13:40:39Z

Implements changes suggested by @ManuelHentschel for R Markdown preview. Should not have any major change in functionality.

Changes:

path -> fsPath
~~'/' -> path.sep~~
themeBool -> useDarkTheme
command category 'R' -> 'R Markdown'
getRPath(false) -> true
extDir -> ~~extensionDirectory~~ homeExtDir
~~tmpDir -> temporaryDirectory~~
Handle untitled documents by prompting the user to save the doc prior to rendering
Less eager directory creation
check exit code rather than error message for knitting errors
refresh now re-knits the doc, rather than refreshing the html

Tested on Linux, I don't currently have access to Windows.

@manuel

Implements changes as suggested by @manuel

renkun-ken · 2021-07-10T13:43:28Z

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


I think both extDir or extensionDirectory are ambiguous because it could mean the installation directory, but I don't come up with a name that makes more sense. Any ideas?

I agree. Hmm, what about... localDirectory? homeDirectory?

Given it's created in the ~/ directory, I think something relating to 'home' is closer to the right name.

Just throwing ideas out there, because honestly I'm not sure either:

homeDir

homeExtDir

extHomeDir

I like homeExtDir, which is almost the same as its path.

renkun-ken · 2021-07-10T13:45:39Z

-export const extDir: string = path.join(os.homedir(), '.vscode-R');
-export const tmpDir: string = path.join(extDir, 'tmp');
+export const extensionDirectory: string = path.join(os.homedir(), '.vscode-R');
+export const temporaryDirectory: string = path.join(extensionDirectory, 'tmp');


It looks like tmp or temp are already a convention to indicate "temporary" (e.g. os.tmpdir()). Not sure if it really looks better to call it in full.

This is a bit ambiguous too, i.e. temporaryDirectory just looks like an alias of os.tmpdir()?

Would it be feasible to simply use os.tmpdir() then (if I recall correctly, there might be issues with user privacy on some platforms)?

And if not, why is this directory declared/created here? It doesn't seem to be used anywhere eles in the code, so wouldn't it be enough to put this in the markdown-preview code?

Using os.tmpdir() will have some user privacy problem, unless we create something like /tmp/vscode-R/$USER and make the folder have mode 700.

Just notice that https://github.com/REditorSupport/vscode-R/blob/master/src/helpViewer/helpProvider.ts#L289 uses os.tmpdir() which is probably subject to multi-user clash problem.

Perhaps we should set all references of os.tmpdir (and future references) to temporaryDirectory*?

barring issues with the name

ManuelHentschel · 2021-07-10T15:09:05Z

 export async function activate(context: vscode.ExtensionContext): Promise<apiImplementation.RExtensionImplementation> {
-    if (!fs.existsSync(extDir)) {
-        fs.mkdirSync(extDir);
+    if (!fs.existsSync(extensionDirectory)) {
+        fs.mkdirSync(extensionDirectory);
    }

-    if (!fs.existsSync(tmpDir)) {
-        fs.mkdirSync(tmpDir);
+    if (!fs.existsSync(temporaryDirectory)) {
+        fs.mkdirSync(temporaryDirectory);
    }


This creates these directories quite eagerly, i.e. as soon as the extension activates. Wouldn't it be better to create them only if they're actually used?

This could e.g. be implemented by adding a function that creates the directory (if necessary) and then returns its path

Should now only be created when the variable is referred to:

export const extensionDirectory = (): string => util.getDir(path.join(os.homedir(), '.vscode-R')); export const temporaryDirectory = (): string => util.getDir(path.join(extensionDirectory(), 'tmp'));

Names tbd :)

ManuelHentschel · 2021-07-10T15:13:57Z

This is working quite nicely now, thanks for the nice work! :)

renkun-ken · 2021-07-11T02:23:29Z

There seems an inconsistency that the commands are r.markdown.*, global context variables are r.preview.* and the settings are r.rmarkdown.*?

ElianHugh · 2021-07-11T02:55:39Z

Maybe we make it uniform: all under r.markdown?
R.rmarkdown is a bit redundant

renkun-ken · 2021-07-11T03:10:52Z

Maybe we make it uniform: all under r.markdown?
R.rmarkdown is a bit redundant

I agree. I think if we put a note of breaking changes in the news, and it is okay to rename existing settings of r.rmarkdown.* to r.markdown.*.

eitsupi · 2021-07-11T03:30:35Z

Thanks for the great work.
Does the naming convention of the command have anything to do with this issue #472?

ManuelHentschel · 2021-07-11T07:35:56Z

Maybe we make it uniform: all under r.markdown?
R.rmarkdown is a bit redundant

I agree. I think if we put a note of breaking changes in the news, and it is okay to rename existing settings of r.rmarkdown.* to r.markdown.*.

I wouldn't say r.rmarkdown is redundant. All our settings/commands start with r. since they are contributed by the R extension. Changing the setting names to r.markdown would make it look like they affect .md files rather than .rmd.

ElianHugh · 2021-07-11T07:54:55Z

To me, I think of r.markdown in my head as
"R: Markdown", and r.rmarkdown as "R: R Markdown". However, I get your point re the extension naming scheme

- Create dir only when used - useDarkTheme -> useCodeTheme

- Show warning when attempting to knit an untitled (unsaved) doc - rename temp and extension directories

- Save untitled file, then retry preview - refresh button now uses updatePreview rather than refreshPanel - rename contexts to be more uniform

renkun-ken · 2021-07-12T06:15:13Z

It looks like the progress only shows when executing the preview command. The refresh does not show up the progress, and thus there is no way to cancel the rendering. I wonder if it makes sense to also show the progress on each refresh?

ElianHugh · 2021-07-12T07:09:14Z

It looks like the progress only shows when executing the preview command. The refresh does not show up the progress, and thus there is no way to cancel the rendering. I wonder if it makes sense to also show the progress on each refresh?

Whoops, yes that should happen. I'll fix that tonight

- Show progress when refreshing a preview - Save documents prior to knitting

renkun-ken · 2021-07-12T12:55:37Z

Whenever I save the document, it looks like the document is rendered twice. It is because the file watcher might call multiple times. We might need to remember the file mtime and only call render when the file mtime changes, just like https://github.com/REditorSupport/vscode-R/blob/master/src/session.ts#L550.

ElianHugh · 2021-07-13T01:48:56Z

Seems to be an issue where the uri passed from the file explorer context menu is not equivalent to vscode.window.activeTextEditor.document.uri and causes issues. Not sure why this would be the case, but I guess we could just store uri.path as the key rather than vscode.Uri.

renkun-ken · 2021-07-13T01:59:54Z

Seems to be an issue where the uri passed from the file explorer context menu is not equivalent to vscode.window.activeTextEditor.document.uri and causes issues. Not sure why this would be the case, but I guess we could just store uri.path as the key rather than vscode.Uri.

OK. Using uri.path looks better.

ElianHugh · 2021-07-13T03:47:34Z

~~Ideally we would keep vscode.Uri so that we still have access to its attributes (e.g. fsPath). However, no matter how I create a uri, an fsPath is not supplied.~~

// no fsPath
vscode.Uri.parse(fileUri.path);
vscode.Uri.file(fileUri.path);

// no fsPath
const fragment = /([^/\\]+$)/g.exec(fileUri.path)[0];
await vscode.workspace.findFiles(`**/${fragment}`, null, 10)[0]);

// no fsPath
vscode.Uri.from(
            {
                scheme: 'file',
                path: fileUri.path
            }
        );

~~I have no idea what constitutes the creation of an fsPath.~~

Will have fix shortly

The vscode.Uri object can lead to problems of equivalence, therefore we'll use a string (fsPath) as an identifier

renkun-ken · 2021-07-13T08:24:42Z

-        const fileUri = uri ?? vscode.window.activeTextEditor.document.uri;
-        const fileName = fileUri.fsPath.substring(fileUri.fsPath.lastIndexOf(path.sep) + 1);
+        const filePath = uri ? uri.fsPath : vscode.window.activeTextEditor.document.uri.fsPath;
+        const fileName = filePath.substring(filePath.lastIndexOf(path.sep) + 1);


Is there a reason not to use path.basename()?

renkun-ken · 2021-07-13T08:25:43Z


-    public add(uri: vscode.Uri, preview: RMarkdownPreview): Map<vscode.Uri, RMarkdownPreview> {
-        return this.store.set(uri, preview);
+    public add(fsPath: string, preview: RMarkdownPreview): Map<string, RMarkdownPreview> {


Do we have both filePath and fsPath?

-> Use r.rmarkdown, in line with previous contributions. (probably better left for a later discussion) -> use consistent name for filePath -> use path.basename

renkun-ken · 2021-07-13T10:16:48Z

If the render process exits with an error (e.g. R syntax error in code chunks), the progress will never close.

ElianHugh · 2021-07-13T12:07:47Z

If the render process exits with an error (e.g. R syntax error in code chunks), the progress will never close.

I can't replicate this, this is what should be happening when an error occurs:

It may be that we expand or change the error detection here:

 childProcess.stderr.on('data', (data: Buffer) => {
                const dat = data.toString('utf8');
                this.rMarkdownOutput.appendLine(dat);
                if (dat.includes('Execution halted')) {
                    reject({ cp: childProcess, wasCancelled: false });
                }
            });

renkun-ken · 2021-07-13T12:52:53Z

Let me take a look at this.

renkun-ken · 2021-07-13T13:14:32Z

It turns out to be caused by my customized error handling specified in options(error = function(...)) where it does not print "Execution halted" on error but "Exiting on error".

renkun-ken · 2021-07-13T13:17:02Z

Looks like we should not rely on the error message "Execution halted" (e.g. R in other languages) but the exit code of the process.

renkun-ken

LGTM

ManuelHentschel

Works nicely, thanks!

RMD Preview fixes

450480b

Implements changes as suggested by @manuel

renkun-ken reviewed Jul 10, 2021

View reviewed changes

Comment thread src/rmarkdown/preview.ts

renkun-ken reviewed Jul 10, 2021

View reviewed changes

Comment thread src/rmarkdown/preview.ts Outdated

ManuelHentschel reviewed Jul 10, 2021

View reviewed changes

Comment thread src/rmarkdown/preview.ts Outdated

ManuelHentschel reviewed Jul 10, 2021

View reviewed changes

ElianHugh and others added 2 commits July 11, 2021 19:05

Change dir creation, rename theme bool

314b05a

- Create dir only when used - useDarkTheme -> useCodeTheme

Untitled document handling, rename directories

cff2892

- Show warning when attempting to knit an untitled (unsaved) doc - rename temp and extension directories

renkun-ken reviewed Jul 11, 2021

View reviewed changes

Comment thread src/rmarkdown/preview.ts Outdated

Untitled file handling, refresh button, rename contexts

fbd5f93

- Save untitled file, then retry preview - refresh button now uses updatePreview rather than refreshPanel - rename contexts to be more uniform

Show refresh progress, save docs

2ae9779

- Show progress when refreshing a preview - Save documents prior to knitting

Check file mtime to avoid repeated rendering

f3f69cf

Use fsPath instead of vscode.Uri

04e629e

The vscode.Uri object can lead to problems of equivalence, therefore we'll use a string (fsPath) as an identifier

renkun-ken reviewed Jul 13, 2021

View reviewed changes

path.base, name consistency

fcbaba3

-> Use r.rmarkdown, in line with previous contributions. (probably better left for a later discussion) -> use consistent name for filePath -> use path.basename

renkun-ken added 2 commits July 13, 2021 21:18

Check exit code instead of error message

921a21f

Update README

bd9e26e

renkun-ken marked this pull request as ready for review July 14, 2021 03:49

renkun-ken approved these changes Jul 14, 2021

View reviewed changes

ElianHugh requested a review from ManuelHentschel July 14, 2021 04:07

ManuelHentschel approved these changes Jul 14, 2021

View reviewed changes

renkun-ken merged commit 03fcd0f into REditorSupport:master Jul 14, 2021

Conversation

ElianHugh commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renkun-ken Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManuelHentschel commented Jul 10, 2021

Uh oh!

renkun-ken commented Jul 11, 2021

Uh oh!

ElianHugh commented Jul 11, 2021

Uh oh!

renkun-ken commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eitsupi commented Jul 11, 2021

Uh oh!

ManuelHentschel commented Jul 11, 2021

Uh oh!

ElianHugh commented Jul 11, 2021

Uh oh!

Uh oh!

renkun-ken commented Jul 12, 2021

Uh oh!

ElianHugh commented Jul 12, 2021

Uh oh!

renkun-ken commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ElianHugh commented Jul 13, 2021

Uh oh!

renkun-ken commented Jul 13, 2021

Uh oh!

ElianHugh commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renkun-ken commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ElianHugh commented Jul 13, 2021

Uh oh!

renkun-ken commented Jul 13, 2021

Uh oh!

renkun-ken commented Jul 13, 2021

Uh oh!

renkun-ken commented Jul 13, 2021

Uh oh!

renkun-ken left a comment

Choose a reason for hiding this comment

Uh oh!

ManuelHentschel left a comment

ElianHugh commented Jul 10, 2021 •

edited

Loading

renkun-ken Jul 10, 2021 •

edited

Loading

renkun-ken commented Jul 11, 2021 •

edited

Loading

renkun-ken commented Jul 12, 2021 •

edited

Loading

ElianHugh commented Jul 13, 2021 •

edited

Loading

renkun-ken commented Jul 13, 2021 •

edited

Loading