Skip to content

Improve handling html help#427

Merged
renkun-ken merged 3 commits intoREditorSupport:masterfrom
renkun-ken:fix-426
Oct 25, 2020
Merged

Improve handling html help#427
renkun-ken merged 3 commits intoREditorSupport:masterfrom
renkun-ken:fix-426

Conversation

@renkun-ken
Copy link
Copy Markdown
Member

@renkun-ken renkun-ken commented Oct 22, 2020

What problem did you solve?

Addresses #426, #380

Some users may prefer other ways to read help documentation (e.g. text and set options(help_type="text") in .Rprofile). Using session watcher will no longer override that preference if specified.

This PR also adds printing the browsing url before sending browser request to session watcher so that vscode will capture the port in url and perform auto port forwarding under remote develoment. This is a walk around of microsoft/vscode#102449.

(If you do not have screenshot) How can I check this pull request?

Regarding options(help_type=)

  1. Add options(help_type="text") in ~/.Rprofile.
  2. Start vscode and an R session and the session is attached.
  3. Run ?get, the documentation will show up in the terminal instead of a new WebView tab.
  4. Remove the option from the .Rprofile and restart an R session
  5. Run ?get and the documentation will show up in a new WebView tab instead of text.

Regarding printing URL in browser request:

  1. Connect to a remote server via remote development via SSH/WSL/Containers
  2. Start an R session and run ?get
  3. The Browsing <URL> message will show up in the terminal output and auto port forwarding should be triggered.
  4. A new webview is open and the HTML help documentation should show properly.

@renkun-ken renkun-ken changed the title Use help_type='html' only when unspecified Improve handling html help Oct 22, 2020
@renkun-ken renkun-ken requested a review from andycraig October 24, 2020 02:47
@renkun-ken
Copy link
Copy Markdown
Member Author

@MilesMcBain Do you think if this makes sense to resolve #426?

@magic-lantern Would you like to try this one and see if it works to resolve #380 ?

@magic-lantern
Copy link
Copy Markdown

magic-lantern commented Oct 24, 2020

@renkun-ken - I'd love to test and see if this resolves the other issue. What do I need to do for testing - such as clone/build/something else?

@renkun-ken
Copy link
Copy Markdown
Member Author

@magic-lantern You could download the vsix at https://github.com/Ikuyadeu/vscode-R/actions/runs/321213341 and install in under remote development and see if ?get help could show properly as described in the first post.

@MilesMcBain
Copy link
Copy Markdown
Collaborator

Changes look good. I was just wondering about testing myself. I'll install this vsix in my docker container.

@MilesMcBain
Copy link
Copy Markdown
Collaborator

help

It seems that it already worked in docker before the change? It works now as well anyhow.

I tested over ssh and it also works now, as per screen shot. The style sheet issue is interesting, the help appeared normal in the Docker case.

I had also expected VSCode would need me to confirm port forwarding with some kind of dialog? I didn't see it.

@renkun-ken
Copy link
Copy Markdown
Member Author

@MilesMcBain Not sure if port mapping under remote via containers works differently from via ssh. It's good as long as it works anyway. Thanks for testing!

@andycraig
Copy link
Copy Markdown
Collaborator

@MilesMcBain Sounds like you've looked at the code and were happy with the changes, and have also tested it and found it works? If so, please feel free to mark this PR as approved. (There should be a 'Review' button at the top of the 'Files changed' tab.)

Copy link
Copy Markdown
Collaborator

@MilesMcBain MilesMcBain left a comment

Choose a reason for hiding this comment

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

LGTM

@renkun-ken renkun-ken merged commit 2676bf2 into REditorSupport:master Oct 25, 2020
@magic-lantern
Copy link
Copy Markdown

@renkun-ken - Thanks for the help on how to test. I have tested in my current VSCode environment and this update does fix #380 for me.

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