Skip to content

Integrate help view from vscode-R-help#433

Merged
Ikuyadeu merged 45 commits intoREditorSupport:masterfrom
ManuelHentschel:helpView
Nov 20, 2020
Merged

Integrate help view from vscode-R-help#433
Ikuyadeu merged 45 commits intoREditorSupport:masterfrom
ManuelHentschel:helpView

Conversation

@ManuelHentschel
Copy link
Copy Markdown
Member

@ManuelHentschel ManuelHentschel commented Oct 31, 2020

As mentioned in #432 I tried to integrate vscode-R-help here.
The main functionality seems to be working, simply use help(...) in an R Terminal (opened by the extension!), or the commands r.showHelp, r.showDoc in VS Code's command palette.

Here it can be configured, whether to look for help files using a custom function or to look for help files by querying an R instance running help.start(). These should mostly behave the same, but there are different problems/difficulties with each, which need to be figured out.

Remaining issues/questions include:

  • No clean overwrite of help(), since the tools from the extension are attached before utils (Implemented as proposed here)
  • Reset scroll position of webview when opening a new link (should work now)
  • Repair webpack config (there were some problems on my machine, so I disabled it altogether)
  • Use extra config to specify path to normal R executable (in case radian is used in r.rterm.XXX)
  • Improve performance when there are many links on a page (since each link is modified individually when loading a page) (Should work ok now, only notable on very long documents)
  • Preserve hyperlinks in (styled) code sections
  • Fix redirect-pages when using 'Rserver' as helpProvider
  • Implement hyperlinks that point to a different section of the same document
  • Fix non-standard hyperlinks (there are some links in help files that are not relative to the current file, e.g. base::colSums->complete.cases)
  • Focus/show the help panel if its content changes while it's not focussed/visible.
  • Improve display of non-html pages (package DESCRIPTION files and some files linked on doc/html/index.html)
  • Improve style of help pages:
    • Adapt to rest of the editor (font family and size adapts, syntax highlighting color theme does not)
    • Add back/forward buttons? (Available in the editor title menu, do not remember scroll position though)
  • Configure the helpview properly (automatically and/or by the user)
    • Use font size, font family etc. from editor settings (happens automatically in some places)
    • Choose position of webview
    • Choose which helpProvider to use
  • Fix "Index" links that return "Server error: invalid response from R" (example case?)
  • Adapt style file to theme, let user choose style file?
  • Include other files/images (e.g. the R logo on the main doc page)
  • Improve usability of r.showHelp: implement fuzzy matching, autocomplete, simulation of ? from command palette?
  • Implement ??
  • Handle links to PDF files (and other file types?)
  • General testing and bug fixing
  • ...

I just added the code to get things to work, feel free to rearrange/reformat/rename to better match the rest of the project.
If you have suggestions, questions, comments, or find bugs, please let me know. All feedback or code contributions are welcome :)

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Regarding the implementation the custom help function in R I am not quite sure what the best solution is.

In the normal case there are a number of functions involved when calling e.g. ? print: The first call is directly to
utils::`?`, which then calls help() and returns an object of class help_files_with_topic.
In an interactive session, this object is then printed with print.help_files_with_topic, which actually does the work of starting a help server (tools::startDynamicHelp) and opening the corresponding URL (browseURL).

I think the cleanest approach would be to overwrite print.help_files_with_topic, since this would preserve the behaviour of help in cases where the user actually wants to store the return value to a variable and not print it, while still giving us full control over the actual help server (e.g. to make it independent of the current R session).

One problem with this approach might be that the vsc tools are loaded before the utils package and I don't know if loading the package manually (calling library(utils)?) to reorder the search path is a good idea. Do you have any advice/opinion on this?

@tdeenes
Copy link
Copy Markdown
Contributor

tdeenes commented Nov 2, 2020

Maybe loading the 'utils' namespace and overwriting print.help_files_with_topic() could work? It does not modify the search path...

In the terminal: export R_DEFAULT_PACKAGES='base'
In the R console:

local({
  ns <- loadNamespace("utils")
  search()
  # [1] ".GlobalEnv"   "Autoloads"    "package:base"

  unlockBinding("print.help_files_with_topic", ns)
  assign(
    "print.help_files_with_topic",
    function(x, ...) print.default("Ooops"),
    envir = ns
  )
  lockBinding("print.help_files_with_topic", ns)
})

library(utils)
search()
# [1] ".GlobalEnv"    "package:utils" "Autoloads"     "package:base" 

?mean
# [1] "Ooops"

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Thanks! That approach seems to work well.

I just realized that a very similar function is already used in init.R, I guess we could use that function then.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

ManuelHentschel commented Nov 3, 2020

I tried finding a solution that does not involve modifying the package namespace of utils, but that seems to be quite hard. The following code manages to attach the tools just below the global environment:

.First.sys <- function(){
    base::.First.sys()
    attach(list(print.help_files_with_topic = function(...) print('Ooops')))
    rm('.First.sys', envir=globalenv())
}

And checking the print method for help files seems to look good as well:

> getS3method('print', 'help_files_with_topic')
function (...)
print("Ooops")
<environment: 0x000000001c575778>

But calling ? mean or print(help('mean')) still opens the internal help server...

@renkun-ken
Copy link
Copy Markdown
Member

It looks like this help viewer starts a standalone R session to provide the help files?

If one uses radian instead of R, then the R session won't start properly. r.rterm.* could only be used as the executable path of the interactive terminal. Any service must use R path instead like how vscode-r-lsp starts R to run languageserver.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Yep, R is either called to continuously run a help server in the background, or "on demand" to retrieve the libPaths and parse .Rd files.

Would it make sense to merge the settings in vscode-r, vscode-r-lsp, and vscode-r-debugger for this? Otherwise the user would have to specify their r path in three different config entries if it's not in a standard place...

We could e.g. introduce a setting r.rPath which can be used by all three extensions.

@tdeenes
Copy link
Copy Markdown
Contributor

tdeenes commented Nov 3, 2020

I tried finding a solution that does not involve modifying the package namespace of utils, but that seems to be quite hard. The following code manages to attach the tools just below the global environment:

.First.sys <- function(){
    base::.First.sys()
    attach(list(print.help_files_with_topic = function(...) print('Ooops')))
    rm('.First.sys', envir=globalenv())
}

And checking the print method for help files seems to look good as well:

> getS3method('print', 'help_files_with_topic')
function (...)
print("Ooops")
<environment: 0x000000001c575778>

But calling ? mean or print(help('mean')) still opens the internal help server...

This is because ? hits package:stats on the search path, so any further lookup (e.g, that of 'print.help_files_with_topic') starts there. So the direction becomes package:stats -> namespace:stats -> imports:stats -> base -> global env -> your attached environment.

See here for an old but helpful explanation.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

That would definitely make sense, if print was called from within ? or help. The problem still occurrs though when manually assigning an object to the help-file class and calling print:

> getS3method('print', 'help_files_with_topic')
function (...)
print("Ooops")
<environment: 0x000000001c573b68>
> x <- 1
> class(x) <- 'help_files_with_topic'
> print(x)
Error in if (type == "html") tools::startDynamicHelp(NA) else NULL :
  argument is of length zero

@ManuelHentschel
Copy link
Copy Markdown
Member Author

ManuelHentschel commented Nov 3, 2020

I think I've finally found a way to make this work:

.First.sys <- function(){

    base::.First.sys()

    attach(list(
        print.help_files_with_topic = function(...) print('Ooops')
    ))

    ### EDIT:
    .S3method <- function(generic, class, method) {
        method <- match.fun(method)
        registerS3method(generic, class, method, envir = parent.frame())
        invisible(NULL)
    }
    ###

    .S3method('print', 'help_files_with_topic', print.help_files_with_topic)

    rm('.First.sys', envir = globalenv())
}

The last call overwrites utils' special position when calling print on help objects.
If this does not interfere with the rest of init.R, we could use this to attach all the tools below .GlobalEnv, or else just attach this overwrite in its own search entry...

@tdeenes
Copy link
Copy Markdown
Contributor

tdeenes commented Nov 4, 2020

The problem still occurrs though when manually assigning an object to the help-file class and calling print:

Yeah, this is due to some machinery with S3 method lookup. You are calling the generic first, not your particular custom method implementation.

@tdeenes
Copy link
Copy Markdown
Contributor

tdeenes commented Nov 4, 2020

.S3method('print', 'help_files_with_topic', print.help_files_with_topic)

This is a nice trick, but will make vscode-R depend on fresh versions of R. See the official blog post here and the commit here.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

This is a nice trick, but will make vscode-R depend on fresh versions of R. See the official blog post here and the commit here.

Ok, thanks for catching that. .S3method seems to be nothing more than a wrapper for registerS3method, so we could just copy the function body to achieve the same thing. I've edited the code above and that seems to work on R 4.0.3 and R 3.5.3.

@ManuelHentschel ManuelHentschel marked this pull request as ready for review November 6, 2020 16:12
@ManuelHentschel
Copy link
Copy Markdown
Member Author

The code now uses javascript's integrated new URL() to parse hyperlinks, so the different kinds of links should work properly now.

@renkun-ken
Copy link
Copy Markdown
Member

Thanks for the fix of cross-package hyperlinks. I'm wondering if it is possible that when the link points to a function in another package, if there's only one link, then it should jump directly to that link?

@ManuelHentschel
Copy link
Copy Markdown
Member Author

ManuelHentschel commented Nov 17, 2020

Thanks for the fix of cross-package hyperlinks. I'm wondering if it is possible that when the link points to a function in another package, if there's only one link, then it should jump directly to that link?

When viewing the R help in a normal browser, the help server sends a redirect response (HTTP 302) after clicking a cross-package link, but when querying the site from within the extension, the help server returns that "forwarding page" with an Ok response (HTTP 200). The cleanest way to fix this would be to somehow indicate that we also want to receive the redirect response when querying the help site, not sure how this would be done.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Btw, when using "r.helpPanel.helpProvider": "custom", cross-package links seem to work fine.
However, the custom helpProvider seems to have some problems on linux (and probably also macOS?).

@tdeenes
Copy link
Copy Markdown
Contributor

tdeenes commented Nov 17, 2020

@ManuelHentschel FYI: after your new commits, cross-pkg hyperlinks, DESCRIPTION line breaks, forward/back buttons all work nicely on Linux.

@renkun-ken
Copy link
Copy Markdown
Member

I tried ??help and it prints an hsearch object and the original R html help server is started and a WebView is shown. I guess we have not done anything about hsearch yet.

Also, I navigate though different links around and sometimes clicking the "Index" at the bottom of the help page will result in a text page saying "Server error: invalid response from R".

Another thing is that if I view User guides, package vignettes and other documentation. and click utils::Sweave which links to a PDF file, the help pane directly tries to show its binary content in text. I guess we could call vscode.env.openExternal() in this case as if it was an external link.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

I tried ??help and it prints an hsearch object and the original R html help server is started and a WebView is shown. I guess we have not done anything about hsearch yet.

Yup, the hsearch object is not handled by us yet. The method described here might work for us.

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Also, I navigate though different links around and sometimes clicking the "Index" at the bottom of the help page will result in a text page saying "Server error: invalid response from R".

I haven't encountered this yet, so if you find a reproducible example, please share :)

@ManuelHentschel
Copy link
Copy Markdown
Member Author

Another thing is that if I view User guides, package vignettes and other documentation. and click utils::Sweave which links to a PDF file, the help pane directly tries to show its binary content in text. I guess we could call vscode.env.openExternal() in this case as if it was an external link.

This might be a bit more complicated, since we receive the contents of the pdf file directly from the help server. Hence we do not know the location of these pdf files on disk for sure. We can take a guess using R.home() and .libPaths but these values are usually not queried by the default helpProvider ("r.helpPanel.helpProvider": "Rserver", implemented in rHelpProviderBuiltin.ts).

Since most of the documentation is provided as html, I wouldn't consider this a priority right now

@renkun-ken
Copy link
Copy Markdown
Member

Also, I navigate though different links around and sometimes clicking the "Index" at the bottom of the help page will result in a text page saying "Server error: invalid response from R".

I haven't encountered this yet, so if you find a reproducible example, please share :)

I could reproduce this on both Ubuntu and macOS.

  1. ?mean
  2. Click link weighted.mean
  3. Click Index at the bottom of page.
  4. Server error: invalid response from R

@ManuelHentschel
Copy link
Copy Markdown
Member Author

I could reproduce this on both Ubuntu and macOS.

1. `?mean`

2. Click link `weighted.mean`

3. Click Index at the bottom of page.

4. `Server error: invalid response from R`

Thanks that example works on windows as well. Does the last push fix the issue for you?

@renkun-ken
Copy link
Copy Markdown
Member

It works nicely now. Thanks for the quick fix!

@ManuelHentschel
Copy link
Copy Markdown
Member Author

If there aren't any more (known) bugs to fix, I would consider the remaining items on the to-do list "optional" improvements and propose merging the PR.

Comment thread html/theme.css
Copy link
Copy Markdown
Member

@renkun-ken renkun-ken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the great work!

I believe this PR is ok for merge. For the remaining items, we could always improve later.

@Ikuyadeu Any more comments?

@Ikuyadeu
Copy link
Copy Markdown
Member

LGTM, it is great.
I will publish a new version this weekend.

@ManuelHentschel If you have an interest in this extension, I want to invite you as a collaborator.
Because you already submit the 2 pull requests that have large contributions.
And, normally new functions are unstable and required high knowledge to resolving issues.

@Ikuyadeu Ikuyadeu merged commit e44ab78 into REditorSupport:master Nov 20, 2020
@Ikuyadeu Ikuyadeu changed the title WIP: Help view Integrate help view from vscode-R-help Nov 20, 2020
@ManuelHentschel
Copy link
Copy Markdown
Member Author

LGTM, it is great.
I will publish a new version this weekend.

@ManuelHentschel If you have an interest in this extension, I want to invite you as a collaborator.
Because you already submit the 2 pull requests that have large contributions.
And, normally new functions are unstable and required high knowledge to resolving issues.

Thanks! I'm definitely interested in this extension and would be happy to help where I can :)

@ManuelHentschel ManuelHentschel deleted the helpView branch November 20, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants