-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LiveShare Functionality #626
Conversation
Implements session watcher features for LiveShare guests
As an aside, I've tried to write this with minimal impact on vscode-R development in mind. For example, the readContent function in util.ts is a drop-in replacement for readFile, which delegates to the guest service when necessary. Ideally, contributors shouldn't have to worry about making their feature work on LiveShare as well. If there's anything I can do on this PR to help with this, please let me know! |
@ElianHugh, are there additional steps required to try this PR beyond those listed here? I have not tried out development VS Code extensions before. I installed npm and Node.js with Homebrew, and can confirm that I have the R and Live Share extensions installed, and session watcher enabled. But when I press F5 to load the extension cloned from your repo, then open a folder with R files, I get the following error:
|
Thanks for checking out the PR! The install instructions should be all that you need. Just to double check, did you run npm install in the extension folder, prior to pressing F5? Also, depending on your local set-up, building the extension can hang. E.g. sometimes the npm terminal will ask if you want to use webpack, but not actually focus the terminal |
Thanks for the tip. You were correct, I had missed the
The host needed to explicitly enable read/write permissions on the shared R terminal by going to Live Share > Session Details > Shared Terminals > R Interactive (Read-only) > Right-click > Make Read/Write. I'm not sure that I know the best answer, but perhaps there can be an additional menu item in the R extension 'Live Share Controls' menu that detects and displays the status of the shared R terminal in the Live Share extension (not shared, read-only, read-write), and which ideally could be used to toggle sharing permissions for the R terminal directly from the R extension without needing to navigate to the Live Share extension? Or, perhaps this is just an issue for guests connecting from a browser and not for guests connecting from a desktop VS Code application with the R and Live Share extensions installed?
Is it possible to make the R extension available to guests connecting from a browser? Guest browsers are a special use case for Live Share so perhaps this last point could become a separate issue so this PR could be merged first. Many thanks again for the effort, this is a great contribution! |
The intent of the "forward guest commands" was to fix the issue of non-invasive vscode-R commands from failing due to a terminal being read only1. E.g. viewing a workspace viewer item would fail (something fairly innocuous). Toggling this allows the host to prevent or allow ALL users from using vscode-R commands (such as the workspace viewer commands). This is mainly handled this way due to the liveshare permissions API still being in development. Having said that, I think what the "forward guest commands" button does could definitely be better communicated.
Unfortunately I think you're on the money in terms of the difficulty in getting browsers to work with the R workspace. One issue is that there are still certain functions that require a file to parse for them to work. For instance, the (default) plot viewer requires an image file2. The PR gets around this by getting the file content from the host, and then creating the file on the guest machine. The issue, then, is handling how the browser handles (and has access to) file systems vs. how an application handles file systems. It may be possible to handle this disparity, but I suspect this may prove to be difficult. The other issue was to do with sharing terminals in browsers - last time I checked this was still in development, which meant I couldn't do much until it was finalized. Thank you very much for the feedback!
|
@ElianHugh, thanks very much for clarifying and again many thanks for the impressive effort!
I'll make a few suggestions for the display text and hover text for the menu item, but please feel free to disregard if you have better alternatives.
I just tested this again and it remains true that sharing terminals (e.g., zsh) in browsers is not supported as per this tweet. That said, if the host makes the R terminal read/write in the Live Share extension, it does work on a guest browser even though zsh does not! The R terminal is labeled as |
@renkun-ken, could you please review? It would be great to see this merged. |
Thanks! I'll do more testing on this soon. |
I didn't have experience using LiveShare before. I just play with this PR a bit, and it is astonishing. Everything seems to already work quite smoothly. I'll take a closer look at the code and use it more thoroughly and see if I can find something to improve. Thanks for the great work! |
- isLiveShare now relies on liveSession instead of a new vsls.getApi() call -- Many functions are now synchronous as a result - Various tooltip changes to clarify 'forward guest commands' - Clean up helper bool functions
@gvelasq I've had a look at this codespace FAQ and it looks like the more significant issue is that vscode-R can't be installed in the browser, and therefore custom functionality such as this PR isn't available in the browser :( From the link above:
|
As per #641, liveshare files are moved to a /liveshare/ folder, and the "r" prefix has been removed from their name
@ElianHugh thanks for looking into the Codespaces FAQ. It's good to know that it's not possible to install the Thanks to @renkun-ken and @ManuelHentschel for their technical reviews — what remains to be done before this PR can be merged? |
I tested on Ubuntu and both host and guest session works well with attached session (e.g. However, when the host is Ubuntu and the guest is macOS, the guest cannot attach to active terminal. When the host is macOS and the guest is Ubuntu, the guest cannot attach to active terminal either. |
Very weird, but that does help narrow down the issue a bit. I looked at Ubuntu host to Windows guest and that seems to work well too. I'll have access to a mac shortly, so should be able to pinpoint what the issue is! |
Hi @renkun-ken, I tried this PR on a mac with a fresh install of vscode, and it seems to work okay for me. I did, however, have to enable session watcher prior to joining the session. So, the source of the issue is likely to stem from the
Is it possible that the terminal doesn't have the names listed under |
Thanks for pointing out the I use I notice that if I don't have an active terminal before sharing the session and a guest joins, then starting an active terminal does not make the guest attach to the terminal. The only way to make it work is to attach to a session before a guest joins. |
Do you think it is handy to support join before attach? If not, would you like to write more in the error message so that user knows what should be done? |
Guest attach should definitely mirror host attach at all times (aside from when the workspace isn't shared). I'll have a commit to fix this soon |
To get around the name issue, I can make use of |
I wonder why we really need a check or filter here? |
You're probably right - previously it was to prevent undefined objects crashing the guest session, but the upcoming changes don't really warrant it |
I'm testing the latest build of this PR and it seems the guest can no longer attach to host session however I tried. |
I wonder if this is due to the API timeout implemented previously? I've noticed the same too, but usually restarting the extension fixes it. It may be worth exposing the retry command to the guest session, or prompting the guest somehow |
Looks like it is indeed caused by the default timeout 3000. I adjust it to 5000 and it works now. |
Let me test again on macOS and take a closer look at the behavior of |
I tested on macOS and I have to increase the timeout to 8000 to make it work. Let me take a closer look into this. |
Looks like |
- Change timeout default: 3000 => 10000 - Remove leftover information message - Fix accidental merge overwrite
I found a quirky behavior of guest httpd plot viewer. Make sure to put the following in the options(vsc.use_httpgd = TRUE) In a live share session, start an R session and try the following code: plot(rnorm(100)) which opens up an httpgd plot viewer tab. Then create a shinyApp: shiny::runExample("01_hello") A brower tab is shown. On both host and guest window, put these two viewers in the same editor view column. Moving the slide in the shiny app will always reveal the httpgd plot viewer on the guest side but not on the host side. |
- notifyGuestPlotManager is now called less often, preventing it from taking focus unexpectedly
I could replicate this - looks like the |
Thanks @ElianHugh for the quick fix. It works well now. |
Once this PR is merged, I suggest that we add @ElianHugh as a collaborator since this is such a nice and important feature which needs good maintainence. @Ikuyadeu @andycraig @ManuelHentschel |
- .UUID -> uuid - Shorten commandNode label
There was a problem hiding this 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 nice work!
@gvelasq @ManuelHentschel Any more comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks!
[![Badge](https://aka.ms/vsls-badge)](https://aka.ms/vsls) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, we don't have any other badges (e.g. version, cicd, license, installs, rating...) in the readme. Personally I'd suggest we either agree on a sensible set of badges or just not have any badges at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me - I'll remove the badge for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a single badge is perfectly ok to me, just like https://github.com/Microsoft/vscode-cpptools 😺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is just a matter of personal preference, so feel free to leave the badge in, if you agree on this :)
- request() to liveShareRequest() - onRequest() to liveShareOnRequest() - use index design pattern - remove global shareurl - remove badge from README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (Please remember to fix the linter warnings before merging)
Finger slipped on the GitHub app :) Thank you for the kind words, and the in-depth reviews! I will fix up the linter issues shortly |
- Appease the linter gods
Looks like this PR is good to merge now. |
Thank you for all of your amazing work on this! |
@ElianHugh As mentioned in # 98, the management of this repository will be moved from me within a year. |
Congratulations all this is looking incredible!! |
Thank you for the kind words and the invitation! More than happy to help out :) |
What problem did you solve?
Closes #555
General functionality
Vscode-R functions related to the session watcher currently do not function in LiveShare sessions. This means that the workspace viewer, dataview, plots, etc., do not work for guests.
This PR implements a 'LiveSession Listener', which allows for the sharing of file content and the forwarding of commands from a guest session to the host terminal. Thus, dataview, plots, browsers, and the workspace viewer should now function for guest sessions.
Settings & controls
This PR also contributes a liveshare control view, for tweaking guest access to the R workspace. This involves the following controls:
Default state can be controlled through three contributed settings.
Features
**I've intentionally left addin functionality out of this PR, as it is either a bigger undertaking or does not function with guest sessions at all. When initially adding guest access to addins, there was significant desyncing between host and guest sessions, so I have decided to leave it out. Further complicating the issue, there is no way (at the moment) of distinguishing between a guest addin call and a host addin call.
Extra things to note
How can I check this pull request?